Bug 95474

Created attachment 161545 [details] Patch

Comment on attachment 161545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161545&action=review Looks nice. Just small comments. > Source/WebCore/css/CSSParser.cpp:2744 > + if (RefPtr<CSSBasicShape> wrapShape = parseBasicShape()) { I would rather pass the value to the parseBasicShape instead of relying on reading it from the m_valueList. That's mostly because it is not incrementing the m_valueList anymore. > Source/WebCore/css/CSSParser.cpp:2749 > + return false; I suppose there's no need to return false. The parsing will fail anyway. > Source/WebCore/css/CSSParser.cpp:4749 > CSSParserValueList* args = value->function->args.get(); I know this is old code, but I think we might need to assert that the value is a function. > Source/WebCore/rendering/style/RenderStyle.h:1469 > + if (rareNonInheritedData->m_clipPath != shape) Should clip-path overwrite the clip-rect property?

Comment on attachment 161545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161545&action=review Thank you for the review Alex! >> Source/WebCore/css/CSSParser.cpp:2744 >> + if (RefPtr<CSSBasicShape> wrapShape = parseBasicShape()) { > > I would rather pass the value to the parseBasicShape instead of relying on reading it from the m_valueList. That's mostly because it is not incrementing the m_valueList anymore. Actually it is more common on the Parser code to let the helper function return a value. parseBasicShape was an exclusion so far. And the old code incremented as well, if the value is a CSSBasicShape. No difference to the old code beside the shapeInside that I can't pass for clip-path. It seemed more logical to follow the general pattern so. >> Source/WebCore/css/CSSParser.cpp:2749 >> + return false; > > I suppose there's no need to return false. The parsing will fail anyway. I am following the pattern of CSSWebkitFilter and text-shadow, box-shadow and others. As long as we can be sure that validPrimitive and parsedValue is false, you are right. >> Source/WebCore/css/CSSParser.cpp:4749 >> CSSParserValueList* args = value->function->args.get(); > > I know this is old code, but I think we might need to assert that the value is a function. Even if we just call the function if value is a function, it seems a good idea to make it safe against possible changes in the future. I'll add an assert. >> Source/WebCore/rendering/style/RenderStyle.h:1469 >> + if (rareNonInheritedData->m_clipPath != shape) > > Should clip-path overwrite the clip-rect property? No, it is different to 'clip', which just applies to absolute positioned elements. However, it is undecided if 'clip' shadows 'clip-path' in the future. I will refactor the code more to support FuncIRI later anyway.

Created attachment 161588 [details] Patch

Created attachment 161773 [details] Patch

Worked together with Alex to clean up the code more.

Comment on attachment 161773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161773&action=review Is there another bug about parsing the <clip-source> part too? It might be nice to have comments among the file list in the changelog (and to filter the ugly meaningless lines like "(WebCore):"). > Source/WebCore/ChangeLog:9 > + CSS Masking specification. In a first step the property just excepts s/excepts/accepts/, I think.

Committed r127327: <http://trac.webkit.org/changeset/127327>

(In reply to comment #8) > Committed r127327: <http://trac.webkit.org/changeset/127327> It broke the Qt minimal build. Could you fix it, please?

/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp: In static member function 'static void WebCore::ApplyPropertyClipPath<getterFunction, setterFunction, initialFunction>::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*)': /ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1718: error: there are no arguments to 'basicShapeForValue' that depend on a template parameter, so a declaration of 'basicShapeForValue' must be available /ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1718: note: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated) /ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp: In static member function 'static void WebCore::ApplyPropertyClipPath<getterFunction, setterFunction, initialFunction>::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*) [with WebCore::BasicShape* (WebCore::RenderStyle::* getterFunction)()const = &WebCore::RenderStyle::clipPath, void (WebCore::RenderStyle::* setterFunction)(WTF::PassRefPtr<WebCore::BasicShape>) = &WebCore::RenderStyle::setClipPath, WebCore::BasicShape* (* initialFunction)() = WebCore::RenderStyle::initialClipPath]': /ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1726: instantiated from 'static WebCore::PropertyHandler WebCore::ApplyPropertyClipPath<getterFunction, setterFunction, initialFunction>::createHandler() [with WebCore::BasicShape* (WebCore::RenderStyle::* getterFunction)()const = &WebCore::RenderStyle::clipPath, void (WebCore::RenderStyle::* setterFunction)(WTF::PassRefPtr<WebCore::BasicShape>) = &WebCore::RenderStyle::setClipPath, WebCore::BasicShape* (* initialFunction)() = WebCore::RenderStyle::initialClipPath]' /ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:2074: instantiated from here /ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1718: error: 'basicShapeForValue' was not declared in this scope

I filed a new bug report for fixing the build - https://bugs.webkit.org/show_bug.cgi?id=95639 (Because reopening bugs is forbidden, and a closed bug is absolutely useless to track valid failures.)

Re-opened since this is blocked by 95653

(In reply to comment #12) > Re-opened since this is blocked by 95653 Sorry, didn't mean to reopen the bug, just wanted to track the build fix.