Bug 63133

Summary: [CSSRegions]Parse content: -webkit-from-flow
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Enhancement CC: eoconnor, rcaliman, shanestephens, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
mihnea: review-
Patch 2
hyatt: review-
Patch 3
hyatt: review-, hyatt: commit-queue-
Patch 4 none

Mihnea Ovidenie
Reported 2011-06-22 05:44:00 PDT
Add support for css regions content: from-flow: http://www.w3.org/TR/css3-regions/#the-content-property.
Attachments
Patch (21.44 KB, patch)
2011-06-29 07:01 PDT, Mihnea Ovidenie 		mihnea: review-
DetailsFormatted DiffDiff
Patch 2 (18.09 KB, patch)
2011-07-01 06:59 PDT, Mihnea Ovidenie 		hyatt: review-
DetailsFormatted DiffDiff
Patch 3 (17.27 KB, patch)
2011-07-07 02:00 PDT, Mihnea Ovidenie 		hyatt: review-
hyatt: commit-queue-
DetailsFormatted DiffDiff
Patch 4 (17.86 KB, patch)
2011-07-08 00:06 PDT, Mihnea Ovidenie 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Mihnea Ovidenie
Comment 1 2011-06-29 07:01:54 PDT
Created attachment 99084 [details] Patch
Mihnea Ovidenie
Comment 2 2011-07-01 06:59:49 PDT
Created attachment 99470 [details] Patch 2 Moved tests into LayoutTests/fast/regions
Dave Hyatt
Comment 3 2011-07-06 14:38:11 PDT
Comment on attachment 99470 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=99470&action=review Couple of nits. > Source/WebCore/css/CSSParser.h:160 > + PassRefPtr<CSSValue> parseFromFlowContent(CSSParserValueList* args); You can omit the word "args" here. Typically if the type of the parameter makes it obvious what you've got, we just omit the name in headers. > Source/WebCore/css/CSSStyleSelector.cpp:4165 > + Useless whitespace change. Get rid of this. Thanks. > Source/WebCore/rendering/style/RenderStyle.h:1099 > + void setRegionThread(const AtomicString& n) { SET_VAR(rareNonInheritedData, m_regionThread, n); } Can omit "n" here.
Mihnea Ovidenie
Comment 4 2011-07-07 02:00:59 PDT
Created attachment 99958 [details] Patch 3 Thanks for the review. I have reworked the patch based on your suggestions and taking into account the previous CSSRegions patch that got into trunk.
Dave Hyatt
Comment 5 2011-07-07 15:15:15 PDT
Comment on attachment 99958 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=99958&action=review Looks good but I forgot you need the -webkit- prefix for any new values you add to already-standardized properties. > Source/WebCore/css/CSSParser.cpp:2661 > + } else if (equalIgnoringCase(val->function->name, "from-flow(")) { Seems like this needs to be "-webkit-from-flow", since we're putting it into an existing property, and this value is still non-standard. > Source/WebCore/css/CSSPrimitiveValue.cpp:695 > + text = "from-flow(" + quoteCSSStringIfNeeded(m_value.string) + ")"; Same here... -webkit-from-flow and not just from-flow.
Mihnea Ovidenie
Comment 6 2011-07-08 00:06:42 PDT
Created attachment 100083 [details] Patch 4 Reworking the patch: 1. Changed from content: from-flow to content: -webkit-from-flow. 2. Reworked tests to take into account -webkit-from-flow 3. Renamed test files
Dave Hyatt
Comment 7 2011-07-08 09:47:03 PDT
Comment on attachment 100083 [details] Patch 4 r=me
WebKit Review Bot
Comment 8 2011-07-08 10:29:24 PDT
Comment on attachment 100083 [details] Patch 4 Clearing flags on attachment: 100083 Committed r90642: <http://trac.webkit.org/changeset/90642>
WebKit Review Bot
Comment 9 2011-07-08 10:29:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.