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
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

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.