| Summary: | [css shapes] Layout support for new circle shape syntax | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Bem Jones-Bey <bjonesbe> | ||||||
| Component: | CSS | Assignee: | Bem Jones-Bey <bjonesbe> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, eflews.bot, esprehn+autocc, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, rwlbuis, syoichi | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 124173 | ||||||||
| Attachments: |
|
||||||||
|
Description
Bem Jones-Bey
2013-11-19 17:01:47 PST
Created attachment 218213 [details]
Patch
Comment on attachment 218213 [details] Patch Attachment 218213 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/42298013 Comment on attachment 218213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218213&action=review r=me but still with some nits. > Source/WebCore/css/CSSParser.cpp:5460 > + if (value->id == CSSValueClosestSide || value->id == CSSValueFarthestSide) Is there a test for the change? I wonder why previous tests didn't detect this. > Source/WebCore/rendering/style/BasicShapes.cpp:135 > + // if radius.type() == BasicShapeRadius::FarthestSide. s/if/If/ :) > Source/WebCore/rendering/style/BasicShapes.h:146 > + return m_keyword == None && other.keyword() == None; It would be great if we can investigate if it is possible to interpolate between some keywords. Can you add an FIXME with a link to a bug report please? > Source/WebCore/rendering/style/BasicShapes.h:179 > + return m_type == Value && other.type() == Value; Ditto. Can be the same bug. > LayoutTests/css3/masking/clip-path-circle-overflow-hidden.html:25 > \ No newline at end of file Hm, do we really want that in this file? > LayoutTests/css3/masking/clip-path-circle-overflow.html:17 > \ No newline at end of file Ditto. > LayoutTests/css3/masking/clip-path-circle-relative-overflow.html:17 > \ No newline at end of file Ditto. > LayoutTests/css3/masking/clip-path-circle.html:16 > \ No newline at end of file Ditto. > LayoutTests/css3/masking/clip-path-restore.html:32 > \ No newline at end of file Ditto. Comment on attachment 218213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218213&action=review >> Source/WebCore/css/CSSParser.cpp:5460 >> + if (value->id == CSSValueClosestSide || value->id == CSSValueFarthestSide) > > Is there a test for the change? I wonder why previous tests didn't detect this. Because I failed to add a test for this when I wrote the parsing patch. I added a test to the parsing tests for this in this patch. >> Source/WebCore/rendering/style/BasicShapes.cpp:135 >> + // if radius.type() == BasicShapeRadius::FarthestSide. > > s/if/If/ :) Heh. Applied. >> Source/WebCore/rendering/style/BasicShapes.h:146 >> + return m_keyword == None && other.keyword() == None; > > It would be great if we can investigate if it is possible to interpolate between some keywords. Can you add an FIXME with a link to a bug report please? Ok, will do. >> LayoutTests/css3/masking/clip-path-circle-overflow-hidden.html:25 >> \ No newline at end of file > > Hm, do we really want that in this file? That's not in the file, it was just mentioning that a newline was added at the end of the file when I made my change, because I edited it using Vim , and if there isn't a newline at the end of a file it edits, it adds one. (Or are you saying that it's bad that a newline got added?) Created attachment 218220 [details]
Updated Patch
Comment on attachment 218220 [details] Updated Patch Clearing flags on attachment: 218220 Committed r159979: <http://trac.webkit.org/changeset/159979> All reviewed patches have been landed. Closing bug. |