Bug 63133 - [CSSRegions]Parse content: -webkit-from-flow
Summary: [CSSRegions]Parse content: -webkit-from-flow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Mihnea Ovidenie
URL:
Keywords:
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2011-06-22 05:44 PDT by Mihnea Ovidenie
Modified: 2011-07-08 11:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch (21.44 KB, patch)
2011-06-29 07:01 PDT, Mihnea Ovidenie
mihnea: review-
Details | Formatted Diff | Diff
Patch 2 (18.09 KB, patch)
2011-07-01 06:59 PDT, Mihnea Ovidenie
hyatt: review-
Details | Formatted Diff | Diff
Patch 3 (17.27 KB, patch)
2011-07-07 02:00 PDT, Mihnea Ovidenie
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch 4 (17.86 KB, patch)
2011-07-08 00:06 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 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.
Comment 1 Mihnea Ovidenie 2011-06-29 07:01:54 PDT
Created attachment 99084 [details]
Patch
Comment 2 Mihnea Ovidenie 2011-07-01 06:59:49 PDT
Created attachment 99470 [details]
Patch 2

Moved tests into LayoutTests/fast/regions
Comment 3 Dave Hyatt 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.
Comment 4 Mihnea Ovidenie 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.
Comment 5 Dave Hyatt 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.
Comment 6 Mihnea Ovidenie 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
Comment 7 Dave Hyatt 2011-07-08 09:47:03 PDT
Comment on attachment 100083 [details]
Patch 4

r=me
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2011-07-08 10:29:30 PDT
All reviewed patches have been landed.  Closing bug.