Bug 79514 - [Chromium] Enable Viewport define by default
Summary: [Chromium] Enable Viewport define by default
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: Fady Samuel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-24 11:20 PST by Fady Samuel
Modified: 2012-03-28 10:55 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.06 KB, patch)
2012-02-24 11:22 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
[not for review] alternate implementation? (480 bytes, patch)
2012-02-27 15:18 PST, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (1.16 KB, patch)
2012-02-28 12:46 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch for landing (3.93 KB, patch)
2012-03-21 10:44 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch for landing (4.02 KB, patch)
2012-03-28 09:06 PDT, Terry Anderson
tdanderson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2012-02-24 11:20:44 PST
[Chromium] Enable Viewport define by default
Comment 1 Fady Samuel 2012-02-24 11:22:05 PST
Created attachment 128773 [details]
Patch
Comment 2 Fady Samuel 2012-02-24 11:34:21 PST
Decided to only enable for Chromium.
Comment 3 Fady Samuel 2012-02-27 15:06:48 PST
This still needs to be done, but we need to make sure we only enable it on chromium.
Comment 4 Robert Kroeger 2012-02-27 15:17:01 PST
Comment on attachment 128773 [details]
Patch

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

I think you could edit Source/WebKit/chromium/features.gypi to accomplish the same objective? (see the patch)

> Source/JavaScriptCore/wtf/Platform.h:872
> +#define ENABLE_VIEWPORT 1

I think it's preferable to do this a different way.
Comment 5 Robert Kroeger 2012-02-27 15:18:36 PST
Created attachment 129109 [details]
[not for review] alternate implementation?
Comment 6 Fady Samuel 2012-02-28 12:41:27 PST
(In reply to comment #5)
> Created an attachment (id=129109) [details]
> [not for review] alternate implementation?

Looks like this does the trick. Reuploading.
Comment 7 Fady Samuel 2012-02-28 12:46:22 PST
Created attachment 129310 [details]
Patch
Comment 8 WebKit Review Bot 2012-02-28 14:00:57 PST
Comment on attachment 129310 [details]
Patch

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

New failing tests:
platform/chromium/fast/repaint/fixed-layout-360x240.html
inspector/styles/override-screen-size.html
Comment 9 Terry Anderson 2012-03-21 10:44:17 PDT
Created attachment 133073 [details]
Patch for landing
Comment 10 Fady Samuel 2012-03-21 10:45:56 PDT
(In reply to comment #8)
> (From update of attachment 129310 [details])
> Attachment 129310 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11710074
> 
> New failing tests:
> platform/chromium/fast/repaint/fixed-layout-360x240.html
> inspector/styles/override-screen-size.html

The fixed-layout-360x240 test is obsolete due to a redesign in the way things are done. Deleting it.
Comment 11 WebKit Review Bot 2012-03-21 12:28:09 PDT
Comment on attachment 133073 [details]
Patch for landing

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

New failing tests:
inspector/styles/override-screen-size.html
Comment 12 WebKit Review Bot 2012-03-21 13:47:04 PDT
Comment on attachment 133073 [details]
Patch for landing

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

New failing tests:
inspector/styles/override-screen-size.html
Comment 13 Fady Samuel 2012-03-21 13:50:03 PDT
(In reply to comment #12)
> (From update of attachment 133073 [details])
> Attachment 133073 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12071606
> 
> New failing tests:
> inspector/styles/override-screen-size.html

Argh, fixed layout and viewport are tightly coupled on Chromium, which turns out to make things a lot more complicated from a testing standpoint. We're looking into introducing a new flag to make the two operations independently testable.
Comment 14 Terry Anderson 2012-03-28 09:06:09 PDT
Created attachment 134307 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-03-28 09:09:04 PDT
Attachment 134307 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
ERROR: File does not exist: 'LayoutTests/platform/chromium/fast/repaint/fixed-layout-360x240-expected.png'


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 WebKit Review Bot 2012-03-28 10:55:23 PDT
Comment on attachment 129310 [details]
Patch

Clearing flags on attachment: 129310

Committed r112411: <http://trac.webkit.org/changeset/112411>
Comment 17 WebKit Review Bot 2012-03-28 10:55:28 PDT
All reviewed patches have been landed.  Closing bug.