Summary: | Introduce new CSS property for clip-path | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||
Component: | CSS | Assignee: | Dirk Schulze <krit> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cmarcelo, dino, eric, macpherson, menard, mkwst, ossy, simon.fraser, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html#the-clip-path | ||||||||||
Bug Depends on: | 95639, 95653 | ||||||||||
Bug Blocks: | 95389 | ||||||||||
Attachments: |
|
Description
Dirk Schulze
2012-08-30 10:20:30 PDT
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. |