Bug 86767 - remove the CSS_GRID_LAYOUT compiler define, but default grid layout to off
Summary: remove the CSS_GRID_LAYOUT compiler define, but default grid layout to off
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2012-05-17 12:23 PDT by Tony Chang
Modified: 2012-10-24 06:03 PDT (History)
17 users (show)

See Also:


Attachments
Patch (54.04 KB, patch)
2012-05-17 12:32 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (54.07 KB, patch)
2012-05-17 12:34 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (71.10 KB, patch)
2012-05-17 14:03 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (71.66 KB, patch)
2012-05-17 14:50 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (587.55 KB, application/zip)
2012-05-17 16:31 PDT, WebKit Review Bot
no flags Details
Patch for landing (100.97 KB, patch)
2012-05-18 09:30 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-05-17 12:23:06 PDT
remove the CSS_GRID_LAYOUT compiler define, but default grid layout to off
Comment 1 Tony Chang 2012-05-17 12:32:34 PDT
Created attachment 142527 [details]
Patch
Comment 2 Tony Chang 2012-05-17 12:34:05 PDT
Created attachment 142528 [details]
Patch
Comment 3 Tony Chang 2012-05-17 12:35:43 PDT
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.
Comment 4 Ojan Vafai 2012-05-17 13:52:57 PDT
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.
Comment 5 Tony Chang 2012-05-17 14:03:05 PDT
Created attachment 142547 [details]
Patch
Comment 6 WebKit Review Bot 2012-05-17 14:05:11 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 7 Build Bot 2012-05-17 14:29:06 PDT
Comment on attachment 142547 [details]
Patch

Attachment 142547 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12720638
Comment 8 Tony Chang 2012-05-17 14:50:29 PDT
Created attachment 142560 [details]
Patch
Comment 9 Darin Fisher (:fishd, Google) 2012-05-17 15:17:37 PDT
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
Comment 10 Tony Chang 2012-05-17 15:23:23 PDT
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
Comment 11 WebKit Review Bot 2012-05-17 16:31:19 PDT
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
Comment 12 WebKit Review Bot 2012-05-17 16:31:24 PDT
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
Comment 13 Tony Chang 2012-05-18 09:30:59 PDT
Created attachment 142727 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-05-18 12:29:31 PDT
Comment on attachment 142727 [details]
Patch for landing

Clearing flags on attachment: 142727

Committed r117613: <http://trac.webkit.org/changeset/117613>
Comment 15 WebKit Review Bot 2012-05-18 12:29:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-05-18 13:37:20 PDT
CMake option removed in http://trac.webkit.org/changeset/117618
Comment 17 Tony Chang 2012-05-18 13:38:33 PDT
(In reply to comment #16)
> CMake option removed in http://trac.webkit.org/changeset/117618

Thanks!
Comment 19 Tony Chang 2012-05-22 10:12:53 PDT
(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.