Zoom level limits must be set before setting a zoom level.

Zoom level limits must be set before setting a zoom level.

The default zoom level upperbound of WebKit is 300%,
while content::kMaximumZoomFactor is 500%. The larger
limit must be set before restoring the host-URL zoom levels,
otherwise 400% or 500% zoom level is clipped to 300%.

BUG=139559
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150284

[kinaba, 2012-08-03 06:56:19]

Darin, could you have a look?

[darin (slow to review), 2012-08-03 16:52:05]

LGTM, but can you please include a test for this bug?

[kinaba, 2012-08-04 01:09:45]

Added a test exhibiting the bug.
Would you mind taking one more look on the CL? I'm not so sure about manners on
writing tests for RenderView...

[darin (slow to review), 2012-08-06 20:44:16]

http://codereview.chromium.org/10828143/diff/4002/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

http://codereview.chromium.org/10828143/diff/4002/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:1702: TEST_F(RenderViewImplTest,
ZoomLimit) {
it is not clear to me how this test would have caught the bug.  can you please
explain to me how it works?

[kinaba, 2012-08-06 23:39:23]

Here is the explanation how it works.
Maybe should I added this in the source code comment?

http://codereview.chromium.org/10828143/diff/4002/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

http://codereview.chromium.org/10828143/diff/4002/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:1702: TEST_F(RenderViewImplTest,
ZoomLimit) {
On 2012/08/06 20:44:16, darin wrote:
> it is not clear to me how this test would have caught the bug.  can you please
> explain to me how it works?

See the inline comments below.

http://codereview.chromium.org/10828143/diff/4002/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:1712: params.url =
GURL("data:text/html,min_zoomlimit_test");
Here, RenderViewImpl stores zoom level setting (25%) for this url.

http://codereview.chromium.org/10828143/diff/4002/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:1713:
view()->OnSetZoomLevelForLoadingURL(params.url, kMinZoomLevel);
Navigate to the URL. UndateURL is called and the stored zoom level (25%) is
loaded. What happened before this patch is:
1. webview()->setZoomLevel(0.25) is called
2. 25% is rounded up to 50% (because by  default WebView::minTextSizeMultiplier
= 0.5 is set as the limit in WebView).
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebViewImpl.c...
3. webview()->zoomLimitsChanged(0.25, 5.0) is called.

After this patch:
1. webview()->zoomLimitsChanged(0.25, 5.0) is called.
2. webview()->setZoomLevel(0.25) is called.
3. 25% is 25%. Because lower bound is already changed to 0.25.

http://codereview.chromium.org/10828143/diff/4002/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:1722:
EXPECT_DOUBLE_EQ(kMaxZoomLevel, view()->GetWebView()->zoomLevel());
Ah, this "max" level part does not trigger the bug because the 5.0 upper bound
is already set above. I'll soon add some line for it to do some meaningful
test...

[kinaba, 2012-08-07 04:27:52]

Added the explanation as source code comments.

[darin (slow to review), 2012-08-07 05:30:22]

OK, LGTM