Bug 61730

Created attachment 95427 [details] Patch Patch for parsing the 'flow' property. The code assumes the define #ENABLE(REGIONS), set up in bug 61631.

Created attachment 95460 [details] Patch 2 The previous one failed to apply.

Created attachment 95462 [details] Patch 2 Forgot to set the review flag.

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.

Created attachment 95467 [details] Patch 3 Corrected the style issues for CSSParser.cpp

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 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)

Created attachment 96401 [details] Patch reworked Reworked patch, taking new ENABLE(CSS_REGIONS) into account.

Created attachment 96417 [details] Patch

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 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 on attachment 96417 [details] Patch Reworking the patch

Created attachment 98131 [details] Patch 4 Reworked the previous patch based on Simon suggestions. The patch takes into account the fixed bug 62114.

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

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

Created attachment 98136 [details] Patch 5 Update the patch after modifying the chromium/test_expectations.txt to ignore the css-regions tests

Created attachment 98139 [details] Patch Reworked

Created attachment 99276 [details] Patch Moved the tests in a new location, LayoutTests/fast/regions.

Looks sane to me. Has there been some reviewer recently workign in CSSParser who would be a better reviewer than I?

Comment on attachment 99276 [details] Patch r=me

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

Created attachment 99934 [details] For asking cq+

Comment on attachment 99934 [details] For asking cq+ Clearing flags on attachment: 99934 Committed r90541: <http://trac.webkit.org/changeset/90541>

All reviewed patches have been landed. Closing bug.

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?

(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.