Bug 86819 - [chromium] Support mobile device rotation resizing
Summary: [chromium] Support mobile device rotation resizing
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: Alexandre Elias
URL:
Keywords:
Depends on: 87468
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-05-18 00:18 PDT by Alexandre Elias
Modified: 2012-06-20 15:58 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.11 KB, patch)
2012-05-18 00:29 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (4.01 KB, patch)
2012-05-23 14:47 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (712.21 KB, application/zip)
2012-05-23 16:03 PDT, WebKit Review Bot
no flags Details
Patch (4.00 KB, patch)
2012-05-23 16:55 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (4.05 KB, patch)
2012-06-19 19:04 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2012-05-18 00:18:32 PDT
[chromium] Support mobile device rotation resizing
Comment 1 Alexandre Elias 2012-05-18 00:29:26 PDT
Created attachment 142653 [details]
Patch
Comment 2 Adam Barth 2012-05-18 04:49:02 PDT
Comment on attachment 142653 [details]
Patch

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

This looks reasonable, but I'm not an expert on such things.  Is there someone else you'd like to review your patch as well?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1350
> +        int dpiIndependentViewportWidth = newSize.width /
> +            page()->settings()->defaultDeviceScaleFactor();
> +        settings()->setLayoutFallbackWidth(
> +            std::max(kStandardFallbackWidth, dpiIndependentViewportWidth));

There's no 80 col limit in WebKit. These can each be on one line.
Comment 3 James Robinson 2012-05-21 17:37:25 PDT
Comment on attachment 142653 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1346
> +        const int kStandardFallbackWidth = 980;

we don't typically use the "k" prefix for constants in WebKit - constants are named the same way as normal variables
Comment 4 Alexandre Elias 2012-05-23 14:47:09 PDT
Created attachment 143656 [details]
Patch
Comment 5 Alexandre Elias 2012-05-23 14:47:31 PDT
Syntax problems fixed.
Comment 6 WebKit Review Bot 2012-05-23 16:03:46 PDT
Comment on attachment 143656 [details]
Patch

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

New failing tests:
WebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag
WebFrameTest.FixedLayoutInitializeAtMinimumPageScale
Comment 7 WebKit Review Bot 2012-05-23 16:03:56 PDT
Created attachment 143667 [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 8 Alexandre Elias 2012-05-23 16:55:27 PDT
Created attachment 143681 [details]
Patch
Comment 9 Alexandre Elias 2012-05-23 16:56:34 PDT
Ouch, while fixing style warnings to remove "== 0", I had forgotten to replace them with ! symbols.  Good thing these unit tests just landed.
Comment 10 WebKit Review Bot 2012-05-24 18:56:26 PDT
Comment on attachment 143681 [details]
Patch

Clearing flags on attachment: 143681

Committed r118461: <http://trac.webkit.org/changeset/118461>
Comment 11 WebKit Review Bot 2012-05-24 18:56:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2012-05-25 00:49:40 PDT
Re-opened since this is blocked by 87468
Comment 14 Vsevolod Vlasov 2012-05-25 01:20:35 PDT
AutomationRendererHelperTest.RTLSnapshot: 
chrome/renderer/automation/automation_renderer_helper_browsertest.cc:40: Failure
Value of: base::MD5String(png_data_str).c_str()
  Actual: "d81f041da73f0e5eee86ecb995410b4b"
Expected: reference_md5.c_str()
Which is: "3adefecb4472e6ba1812f9af1fdea3e4"



AutomationRendererHelperTest.ScrollingSnapshot: 
chrome/renderer/automation/automation_renderer_helper_browsertest.cc:40: Failure
Value of: base::MD5String(png_data_str).c_str()
  Actual: "d81f041da73f0e5eee86ecb995410b4b"
Expected: reference_md5.c_str()
Which is: "3adefecb4472e6ba1812f9af1fdea3e4"
Comment 15 Alexandre Elias 2012-06-19 19:04:03 PDT
Created attachment 148485 [details]
Patch
Comment 16 Alexandre Elias 2012-06-19 19:06:06 PDT
OK, replaced the fixedLayoutEnabled() guards with settings()->viewportEnabled() instead, because AutomationRendererHelper is using fixed layout for the purpose of grabbing a snapshot of the contents while having resizes ignored.
Comment 17 WebKit Review Bot 2012-06-20 15:58:15 PDT
Comment on attachment 148485 [details]
Patch

Clearing flags on attachment: 148485

Committed r120877: <http://trac.webkit.org/changeset/120877>
Comment 18 WebKit Review Bot 2012-06-20 15:58:20 PDT
All reviewed patches have been landed.  Closing bug.