remove the CSS_GRID_LAYOUT compiler define, but default grid layout to off

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

remove the CSS_GRID_LAYOUT compiler define, but default grid layout to off

[Tony Chang]

Created attachment 142527 [details]
Patch

[Tony Chang]

Created attachment 142528 [details]
Patch

[Tony Chang]

This will make it easier to work on grid layout (compiles everywhere, runs tests), but won't expose it to the web yet since it's still in flux.

This is similar to the approach taken for regions in bug 78525.

[Ojan Vafai]

Comment on attachment 142528 [details]
Patch

Would be good to put up a patch the properly patches in to see the EWS bots go green before committing. Up to you though.

[Tony Chang]

Created attachment 142547 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 142547 [details]
Patch

Attachment 142547 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12720638

[Tony Chang]

Created attachment 142560 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 142560 [details]
Patch

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

> Source/WebKit/chromium/public/WebSettings.h:101
> +    virtual void setExperimentalCSSGridLayoutEnabled(bool) = 0;

LGTM for the Chromium WebKit API part

[Tony Chang]

Thanks for the reviews. I'll land this once the bots go green.

Hyatt also said this was fine on IRC:

14:03 < tony^work> dhyatt: Do you mind if we compile css3 grid layout by 
                   default, but turn it off at the CSS parser level?
14:03 < dhyatt> similar to regions? sure.
14:04 < tony^work> dhyatt: yes, exactly the same as regions: 
                   https://bugs.webkit.org/show_bug.cgi?id=86767

[WebKit Review Bot]

Comment on attachment 142560 [details]
Patch

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

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[WebKit Review Bot]

Created attachment 142584 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Chang]

Created attachment 142727 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142727 [details]
Patch for landing

Clearing flags on attachment: 142727

Committed r117613: <http://trac.webkit.org/changeset/117613>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

CMake option removed in http://trac.webkit.org/changeset/117618

[Tony Chang]

(In reply to comment #16)
> CMake option removed in http://trac.webkit.org/changeset/117618

Thanks!

[Ryosuke Niwa]

It appears that this patch regressed Dromaeo/jslib-style-prototype by roughly 20%:
http://webkit-perf.appspot.com/graph.html#tests=[[39020,2001,3001]]&sel=1337320768437.756,1337421568437.756,3097.560975609756,5841.463414634147&displayrange=7&datatype=running
http://webkit-perf.appspot.com/graph.html#tests=[[39020,2001,2389050]]&sel=1337353333822.5122,1337436309432.2683&displayrange=7&datatype=running
http://webkit-perf.appspot.com/graph.html#tests=[[39020,2001,32196]]&sel=1337333308373.6829,1337437181544.4146,4594.076655052265,8513.937282229965&displayrange=7&datatype=running

[Tony Chang]

(In reply to comment #18)
> It appears that this patch regressed Dromaeo/jslib-style-prototype by roughly 20%:
> http://webkit-perf.appspot.com/graph.html#tests=[[39020,2001,3001]]&sel=1337320768437.756,1337421568437.756,3097.560975609756,5841.463414634147&displayrange=7&datatype=running
> http://webkit-perf.appspot.com/graph.html#tests=[[39020,2001,2389050]]&sel=1337353333822.5122,1337436309432.2683&displayrange=7&datatype=running
> http://webkit-perf.appspot.com/graph.html#tests=[[39020,2001,32196]]&sel=1337333308373.6829,1337437181544.4146,4594.076655052265,8513.937282229965&displayrange=7&datatype=running

I bet this was due to moving CSSPropertyDisplay from the fast table (isValidKeywordPropertyAndValue) to the slow table (parseValue).

I've filed bug 87142 for this and I'll look into it today.  We can probably get the time back.