Bug 61730 - [CSSRegions] Parse flow property
Summary: [CSSRegions] Parse flow property
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-05-30 07:51 PDT by Mihnea Ovidenie
Modified: 2011-08-16 03:00 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.73 KB, patch)
2011-05-31 06:39 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (18.89 KB, patch)
2011-05-31 12:28 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (18.89 KB, patch)
2011-05-31 12:34 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 3 (18.89 KB, patch)
2011-05-31 13:11 PDT, Mihnea Ovidenie
mihnea: review-
Details | Formatted Diff | Diff
Patch reworked (17.63 KB, patch)
2011-06-08 02:15 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch (17.54 KB, patch)
2011-06-08 07:12 PDT, Mihnea Ovidenie
mihnea: review-
Details | Formatted Diff | Diff
Patch 4 (20.66 KB, patch)
2011-06-22 01:13 PDT, Mihnea Ovidenie
mihnea: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.32 MB, application/zip)
2011-06-22 01:46 PDT, WebKit Review Bot
no flags Details
Patch 5 (21.23 KB, patch)
2011-06-22 02:15 PDT, Mihnea Ovidenie
mihnea: review-
Details | Formatted Diff | Diff
Patch (21.24 KB, patch)
2011-06-22 02:34 PDT, Mihnea Ovidenie
mihnea: review-
Details | Formatted Diff | Diff
Patch (17.60 KB, patch)
2011-06-30 04:37 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
For asking cq+ (18.76 KB, patch)
2011-07-06 22:05 PDT, Hajime Morrita
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-05-30 07:51:20 PDT
Implement parsing of 'flow' property: http://dev.w3.org/csswg/css3-regions/
Comment 1 Mihnea Ovidenie 2011-05-31 06:39:26 PDT
Created attachment 95427 [details]
Patch

Patch for parsing the 'flow' property. The code assumes the define #ENABLE(REGIONS), set up in bug 61631.
Comment 2 Mihnea Ovidenie 2011-05-31 12:28:13 PDT
Created attachment 95460 [details]
Patch 2

The previous one failed to apply.
Comment 3 Mihnea Ovidenie 2011-05-31 12:34:23 PDT
Created attachment 95462 [details]
Patch 2

Forgot to set the review flag.
Comment 4 WebKit Review Bot 2011-05-31 12:37:42 PDT
Attachment 95462 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSParser.cpp:5791:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/css/CSSParser.cpp:5794:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mihnea Ovidenie 2011-05-31 13:11:48 PDT
Created attachment 95467 [details]
Patch 3

Corrected the style issues for CSSParser.cpp
Comment 6 Mark Rowe (bdash) 2011-06-06 02:17:39 PDT
Comment on attachment 95467 [details]
Patch 3

Please get rid of the bogus addition of whitespace you have in numerous places in the patch.
Comment 7 Mihnea Ovidenie 2011-06-06 02:42:31 PDT
Comment on attachment 95467 [details]
Patch 3

I have to rework it since it is based on ENABLE(REGIONS). It should be based on ENABLE(CSS_REGIONS)
Comment 8 Mihnea Ovidenie 2011-06-08 02:15:20 PDT
Created attachment 96401 [details]
Patch reworked

Reworked patch, taking new ENABLE(CSS_REGIONS) into account.
Comment 9 Mihnea Ovidenie 2011-06-08 07:12:36 PDT
Created attachment 96417 [details]
Patch
Comment 10 Mihnea Ovidenie 2011-06-15 06:32:14 PDT
The error from GTK EWS is not very descriptive and i cannot see the reason for failure. Before posting the last version of the patch, i applied it locally on my machine to double check it.

Could somebody give me more details about the failure on GTK, if possible?
Comment 11 Simon Fraser (smfr) 2011-06-17 07:53:05 PDT
Comment on attachment 96417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96417&action=review

> LayoutTests/fast/css-regions/css-regions-parse-flow.html:1
> +<p>Testing the parsing of the -webkit-flow property. You should see only PASS lines below.</p>

THis should be a script-driven test (see LayoutTests/fast/css/parsing*).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1671
> +#if ENABLE(CSS_REGIONS)
> +        case CSSPropertyWebkitFlow:
> +            if (style->flowThread().isNull())
> +                return primitiveValueCache->createIdentifierValue(CSSValueAuto);
> +            return primitiveValueCache->createValue(style->flowThread(), CSSPrimitiveValue::CSS_STRING);
> +#else
> +        case CSSPropertyWebkitFlow:
> +            break;
> +#endif

I'd prefer the #ifdef inside the case, like you did for exclusions.
Comment 12 Mihnea Ovidenie 2011-06-20 01:36:43 PDT
Comment on attachment 96417 [details]
Patch

Reworking the patch
Comment 13 Mihnea Ovidenie 2011-06-22 01:13:32 PDT
Created attachment 98131 [details]
Patch 4

Reworked the previous patch based on Simon suggestions. The patch takes into account the fixed bug 62114.
Comment 14 WebKit Review Bot 2011-06-22 01:46:30 PDT
Comment on attachment 98131 [details]
Patch 4

Attachment 98131 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8925294

New failing tests:
fast/css-regions/webkit-flow-parsing.html
Comment 15 WebKit Review Bot 2011-06-22 01:46:36 PDT
Created attachment 98134 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 16 Mihnea Ovidenie 2011-06-22 02:15:29 PDT
Created attachment 98136 [details]
Patch 5

Update the patch after modifying the chromium/test_expectations.txt to ignore the css-regions tests
Comment 17 Mihnea Ovidenie 2011-06-22 02:34:39 PDT
Created attachment 98139 [details]
Patch

Reworked
Comment 18 Mihnea Ovidenie 2011-06-30 04:37:56 PDT
Created attachment 99276 [details]
Patch

Moved the tests in a new location, LayoutTests/fast/regions.
Comment 19 Eric Seidel (no email) 2011-07-05 17:54:07 PDT
Looks sane to me.  Has there been some reviewer recently workign in CSSParser who would be a better reviewer than I?
Comment 20 Dave Hyatt 2011-07-06 14:35:06 PDT
Comment on attachment 99276 [details]
Patch

r=me
Comment 21 WebKit Review Bot 2011-07-06 14:38:05 PDT
Comment on attachment 99276 [details]
Patch

Rejecting attachment 99276 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
ebCore/rendering/style/RenderStyle.cpp
Hunk #1 FAILED at 344.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/rendering/style/RenderStyle.cpp.rej
patching file Source/WebCore/rendering/style/RenderStyle.h
patching file Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp
patching file Source/WebCore/rendering/style/StyleRareNonInheritedData.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8984903
Comment 22 Hajime Morrita 2011-07-06 22:05:39 PDT
Created attachment 99934 [details]
For asking cq+
Comment 23 WebKit Review Bot 2011-07-06 22:36:27 PDT
Comment on attachment 99934 [details]
For asking cq+

Clearing flags on attachment: 99934

Committed r90541: <http://trac.webkit.org/changeset/90541>
Comment 24 WebKit Review Bot 2011-07-06 22:36:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Luke Macpherson 2011-08-14 23:31:30 PDT
In CSSStyleSelector.cpp there is a case:
if (isInherit) {
   m_style->setFlowThread(nullAtom);
   return;
}

I wonder if this code is reachable? Should the parser ignore "-webkit-flow: inherit;", or treat it as a string value?
Comment 26 Mihnea Ovidenie 2011-08-16 03:00:06 PDT
(In reply to comment #25)
> In CSSStyleSelector.cpp there is a case:
> if (isInherit) {
>    m_style->setFlowThread(nullAtom);
>    return;
> }
> 
> I wonder if this code is reachable? Should the parser ignore "-webkit-flow: inherit;", or treat it as a string value?

Inherit is not a valid flow name. The parser should not treat inherit as a string value but rather ignore it, which should be similar to auto. The next version of the spec will state that explicitly, along with other changes to the definitions of the properties involved. When the new working draft will get published, i plan to rewrite this part of code asap to take the new changes into account.