Refactor viewport initialization logic out of WebViewImpl.

Refactor viewport initialization logic out of WebViewImpl.

(Second commit attempt)

This centralizes and brings some structure to viewport initialization,
expressing it as a cascade of constraints expressed as
ViewportAttributes objects.  When any of the constraints changes, a
dirty bit is set, and the page scale factor and fixed-layout size is
recalculated on the next layout.

This refactor mostly maintains existing behavior, but also includes two bugfixes
(with new tests for them):
1. Page scale is now reset to minimum when the content width increases,
even if it was reset in the past.
2. setIgnoreViewportTagMaximumScale now also ignores the minimum (I'll
need to rename this WebKit API).

NOTRY=true
BUG=176998, 161695, 165688
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151134

[aelias_OOO_until_Jul13, 2013-05-13 10:24:58]

As several people have started to hack on viewport-tag related stuff, here's a
refactor to clarify the blobs of logic and let folks make changes without
stomping on hidden assumptions.

[kenneth.r.christiansen, 2013-05-13 11:00:18]

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/ViewportAttributesManager.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:37: static const float
defaultMaximumScale = 4.0f;
doesn't iOS use 5 as maximum scale by default? I am pretty certain that it does

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:58: // Only set
initialScale to a value if it was explicitly set.
Why not fix this in the actual ViewportArguments.cpp instead?

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:113: m_constraintsDirty
= true;
So above you sometimes call setNeedsReset which will also just end up calling
m_constraintsDirty = true

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:117: static float
calculateTargetDensityDPIFactor(const ViewportArguments& arguments, float
deviceScaleFactor)
computeDeprecatedTargetDensityDPIFactor?

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:123: if
(arguments.deprecatedTargetDensityDPI == ViewportArguments::ValueLowDPI)
Have any one measure if we can get rid of this? I think it is really rarely used

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:139: void
ViewportAttributesManager::adjustPageDefinedForAndroidWebView(const
ViewportArguments& arguments, IntSize viewSize, int layoutFallbackWidth, float
deviceScaleFactor, bool useWideViewport, bool initializeAtMinimumPageScale)
So this does not affect the android chrome browser?

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/ViewportAttributesManager.h (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:50:
WebCore::ViewportAttributes pageDefined() const { return m_pageDefined; }
Why not call it pageDefinedConstraints() to be consistent with
defaultConstraints

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:57: void
setEmbedderConstraints(WebCore::ViewportAttributes);
setUserAgentConstraints - UA is the terminology used by the W3C, hense
userAgentString

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:78:
WebCore::ViewportAttributes m_finalAttributes;
The naming is a bit confusing now with attributes vs constraints

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:1180: legibleScale = (scale ==
minimumPageScaleFactor()) ? defaultScaleWhenAlreadyLegible :
minimumPageScaleFactor();
Why are these changes not in a separate patch

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:2955: void
WebViewImpl::overridePageScaleConstraints(ViewportAttributes newConstraints)
Here the methods is called overwriteSomething

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:2968: void
WebViewImpl::setInitialPageScaleOverride(float initialPageScaleFactorOverride)
here setSomethingOverride.. why the inconsistency?

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:3007:
updatePageDefinedViewportAttributes(mainFrameImpl()->frame()->document()->viewportArguments());
why bother with all this if viewportEnabled() is not true/?

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.h (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.h:598: float
clampPageScaleFactorToLimits(float scale) const;
I dont know about the style today, but in WebKit you would leave out "scale"
after float

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:387: // Make sure we don't reset
to initial scale if the page continues to load.
What about the case where the use already started a pinch zoom etc? then you
should also not set the initial scale. Something like hadUserInteraction.

https://codereview.chromium.org/14813025/diff/15001/Source/core/dom/ViewportA...
File Source/core/dom/ViewportAttributes.h (right):

https://codereview.chromium.org/14813025/diff/15001/Source/core/dom/ViewportA...
Source/core/dom/ViewportAttributes.h:41: float initialScale;
Maybe we should start  following the CSS Device Adapt spec and call these zoom,
minZoom, maxZoom instead? ViewportArguments already follow that nomenclature

[mnaganov (inactive), 2013-05-13 11:58:38]

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/ViewportAttributesManager.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:123: if
(arguments.deprecatedTargetDensityDPI == ViewportArguments::ValueLowDPI)
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> Have any one measure if we can get rid of this? I think it is really rarely
used

This is used by existing Android WebView-based applications. It's hard to
measure the number of them, as there are no counters in the current WebKit used
on Android system. But we have already encountered some while doing very basic
testing of the new version.

Using 'target-densitydpi' in new applications is discouraged by the official
documentation, we are looking towards removing this support in future.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:139: void
ViewportAttributesManager::adjustPageDefinedForAndroidWebView(const
ViewportArguments& arguments, IntSize viewSize, int layoutFallbackWidth, float
deviceScaleFactor, bool useWideViewport, bool initializeAtMinimumPageScale)
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> So this does not affect the android chrome browser?

Correct. Chrome for Android and Chromium-based Android WebView are different.
The new Chromium-based WebView has to retain some compatibility with the current
WebView. Also, WebView has a lot of settings that simply can't be altered in
Chrome for Android.

[dshwang, 2013-05-13 12:38:00]

Great job! This patch dramatically reduces WebViewImpl.
I leave some comments.

btw, can I ask questions? FYI, I work on Ubuntu 12.10.
1. Is it possible to test pinch zoom in desktop environment? 
2. Does ctrl+wheel change pageScale?

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/ViewportAttributesManager.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:37: static const float
defaultMaximumScale = 4.0f;
Is any reason that two static values out of WebKit namespace?

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:49: ViewportAttributes
ViewportAttributesManager::defaultConstraints() const
I think this function can be static in this file scope.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:484:
setDeviceScaleFactor(m_client->screenInfo().deviceScaleFactor);
Could you explain what this code results?

https://codereview.chromium.org/14813025/diff/15001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/core/page/Page.cpp...
Source/core/page/Page.cpp:450: bool oldProgrammaticScroll =
view->inProgrammaticScroll();
It is so confusing.
How about extracting scroll something from this method?

After that, if WebViewImpl want to both scale and scroll, we must call both this
method and scrolling method.

[jamesr, 2013-05-13 17:44:00]

On 2013/05/13 12:38:00, dshwang wrote:
> Great job! This patch dramatically reduces WebViewImpl.
> I leave some comments.
> 
> btw, can I ask questions? FYI, I work on Ubuntu 12.10.
> 1. Is it possible to test pinch zoom in desktop environment? 
> 2. Does ctrl+wheel change pageScale?
> 

It is, with a few command line flags to use the mouse to emulate gestures:

--enable-pinch --simulate-touch-screen-with-mouse --force-compositing-mode
--enable-threaded-compositing

then right-click-drag to pinch zoom in/out.  Keep in mind it's just a simulation
so you may hit some rough spots (for example I'm hitting an ASSERT on ToT,
apparently due to the simulated events not matching the actual ones closely
enough).

[aelias_OOO_until_Jul13, 2013-05-13 18:13:05]

Thanks for the detailed look.

> 1. Is it possible to test pinch zoom in desktop environment? 

Yes.  Use flags --force-compositing-mode --enable-threaded-compositing
--simulate-touch-screen-with-mouse --enable-pinch .  Then right-click and drag
vertically to produce pinch gestures.

> 2. Does ctrl+wheel change pageScale?

No, it changes full-page zoom.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/ViewportAttributesManager.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:37: static const float
defaultMaximumScale = 4.0f;
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> doesn't iOS use 5 as maximum scale by default? I am pretty certain that it
does

OK, changed to 5.0 and moved into namespace WebKit.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:49: ViewportAttributes
ViewportAttributesManager::defaultConstraints() const
On 2013/05/13 12:38:01, dshwang wrote:
> I think this function can be static in this file scope.

No, there is a callsite from WebViewImpl::setIgnoreViewportTagScaleLimits

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:58: // Only set
initialScale to a value if it was explicitly set.
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> Why not fix this in the actual ViewportArguments.cpp instead?

I tried that, but it causes a large number of fast/viewport layout tests to
fail.  If we're going to rebaseline them, I'd rather not do it as part of this
patch.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:113: m_constraintsDirty
= true;
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> So above you sometimes call setNeedsReset which will also just end up calling
> m_constraintsDirty = true

Yes, but I also want to mark them dirty in all cases.  If content width shrinks,
we don't want to reset to initial scale but minimum-scale may change.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.cpp:117: static float
calculateTargetDensityDPIFactor(const ViewportArguments& arguments, float
deviceScaleFactor)
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> computeDeprecatedTargetDensityDPIFactor?

Done.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/ViewportAttributesManager.h (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:50:
WebCore::ViewportAttributes pageDefined() const { return m_pageDefined; }
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> Why not call it pageDefinedConstraints() to be consistent with
> defaultConstraints

Done.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:57: void
setEmbedderConstraints(WebCore::ViewportAttributes);
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> setUserAgentConstraints - UA is the terminology used by the W3C, hense
> userAgentString

Done.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:78:
WebCore::ViewportAttributes m_finalAttributes;
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> The naming is a bit confusing now with attributes vs constraints

Renamed to finalConstraints as well.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:484:
setDeviceScaleFactor(m_client->screenInfo().deviceScaleFactor);
On 2013/05/13 12:38:01, dshwang wrote:
> Could you explain what this code results?

This setDeviceScaleFactor() call previously happened in
ChromeClientImpl::dispatchViewportArgumentsDidChange.  Because it has nothing to
do with viewport tag, I moved the call here.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:1180: legibleScale = (scale ==
minimumPageScaleFactor()) ? defaultScaleWhenAlreadyLegible :
minimumPageScaleFactor();
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> Why are these changes not in a separate patch

Because the field m_minimumScaleFactor was deleted and collapsed into
m_pageScaleManager.finalConstraints

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:2955: void
WebViewImpl::overridePageScaleConstraints(ViewportAttributes newConstraints)
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> Here the methods is called overwriteSomething

I renamed this to setUserAgentViewportConstraints.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:3007:
updatePageDefinedViewportAttributes(mainFrameImpl()->frame()->document()->viewportArguments());
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> why bother with all this if viewportEnabled() is not true/?

Because we want to support pinch zoom on touch-enabled laptops that may not turn
on viewport tag support.  These will set the userAgentConstraints and share the
scale min/max and initialization codepaths.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.h (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.h:598: float
clampPageScaleFactorToLimits(float scale) const;
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> I dont know about the style today, but in WebKit you would leave out "scale"
> after float

Done.

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:387: // Make sure we don't reset
to initial scale if the page continues to load.
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> What about the case where the use already started a pinch zoom etc? then you
> should also not set the initial scale. Something like hadUserInteraction.

The setPageScaleFactorPreservingScrollOffset call just above is meant to test
this already.

Note that the setNeedsReset(false) in
WebViewImpl::deviceOrPageScaleFactorChanged() is supposed to preserve that rule.

https://codereview.chromium.org/14813025/diff/15001/Source/core/dom/ViewportA...
File Source/core/dom/ViewportAttributes.h (right):

https://codereview.chromium.org/14813025/diff/15001/Source/core/dom/ViewportA...
Source/core/dom/ViewportAttributes.h:41: float initialScale;
On 2013/05/13 11:00:18, kenneth.r.christiansen wrote:
> Maybe we should start  following the CSS Device Adapt spec and call these
zoom,
> minZoom, maxZoom instead? ViewportArguments already follow that nomenclature

I disagree, the term "zoom" is overloaded and generally used to refer to Ctrl-+
style zoom in Blink.  I've been following the convention to call
viewport-tag/pinch "scale" and if anything would prefer to switch
ViewportArguments to say "scale".

https://codereview.chromium.org/14813025/diff/15001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/14813025/diff/15001/Source/core/page/Page.cpp...
Source/core/page/Page.cpp:450: bool oldProgrammaticScroll =
view->inProgrammaticScroll();
On 2013/05/13 12:38:01, dshwang wrote:
> It is so confusing.
> How about extracting scroll something from this method?
> 
> After that, if WebViewImpl want to both scale and scroll, we must call both
this
> method and scrolling method.

The reason is that changing page scale may have the side effect of clamping the
scroll position anyway.  So if someone wants to change both scale and scroll,
they may hit the side effects of changing scroll twice.

[abarth-chromium, 2013-05-13 18:22:24]

https://codereview.chromium.org/14813025/diff/27001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.h (left):

https://codereview.chromium.org/14813025/diff/27001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.h:737: bool m_pageScaleFactorIsSet;
Thank you!!

[mnaganov (inactive), 2013-05-14 09:31:23]

Android-related parts LGTM % comment

https://codereview.chromium.org/14813025/diff/27001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/14813025/diff/27001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:596:
EXPECT_EQ(minimumPageScaleFactor, m_webView->pageScaleFactor());
I'm a bit worried about this condition. There is a CTS test in Android, called
WebViewTest.testSetInitialZoom
(https://code.google.com/p/android-source-browsing/source/browse/tests/tests/w...).
The test verifies that after resetting the scale override, page scale is set to
1.0. Perhaps, we need to do setInitializeAtMinimumPageScale(false) in this test
to resemble default WebView conditions?

(Please note when looking at the Android test, that their 'scale' is multiplied
by the device pixel ratio.)

[aelias_OOO_until_Jul13, 2013-05-14 21:39:52]

Note that Chromium-side android_webview test package also all passes.

https://codereview.chromium.org/14813025/diff/27001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/14813025/diff/27001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:596:
EXPECT_EQ(minimumPageScaleFactor, m_webView->pageScaleFactor());
OK, changed this to 1.0 + setInitializeAtMinimumPageScale(false).

[aelias_OOO_until_Jul13, 2013-05-20 18:04:41]

Adam, could you take a look for OWNERS approval?  I addressed the previous
feedback.

[abarth-chromium, 2013-05-20 18:22:25]

https://codereview.chromium.org/14813025/diff/39001/Source/WebKit/chromium/We...
File Source/WebKit/chromium/WebKit.gyp (right):

https://codereview.chromium.org/14813025/diff/39001/Source/WebKit/chromium/We...
Source/WebKit/chromium/WebKit.gyp:569: 'src/ViewportAttributesManager.h',
Please list alphabetically

https://codereview.chromium.org/14813025/diff/39001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/ViewportAttributesManager.h (right):

https://codereview.chromium.org/14813025/diff/39001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:42: class
ViewportAttributesManager {
Should this object be in Core?  I don't see any client dependencies.

https://codereview.chromium.org/14813025/diff/39001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/ViewportAttributesManager.h:50:
WebCore::ViewportAttributes pageDefinedConstraints() const { return
m_pageDefinedConstraints; }
Should we be returning this object by const reference?  It seems like we're
making some unnecessary copies in a bunch of places.

https://codereview.chromium.org/14813025/diff/39001/Source/core/testing/Inter...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/14813025/diff/39001/Source/core/testing/Inter...
Source/core/testing/Internals.cpp:909: static FloatSize convertToUserSpace(const
FloatSize& deviceSize, float devicePixelRatio)
What is "user space" ?

https://codereview.chromium.org/14813025/diff/39001/Source/core/testing/Inter...
Source/core/testing/Internals.cpp:936: }
I don't understand why we need all of this code in Internals.  Maybe we should
output information in a different format that requires less computation?

[abarth-chromium, 2013-05-20 18:25:53]

LGTM.  I wanted to like this CL more, but there were some hairy parts that made
me less excited that I would have been otherwise.

As a general rule, "manager" isn't a good suffix for a class name because it
doesn't really mean anything.  It tends to lead to classes that have ill-defined
jobs.

The amount of code in Internals is concerning.  Internals is just meant to be a
thin layer to let you poke at the internals of Core.  The fact that we've grown
a bunch of code here is worrying because it means we're testing Internals itself
more than we're really testing the product.  Should we change how these tests
work so that less "glue" code is required?

Also, your CL has a bunch of unnecessary copying.  I doubt that it matters for
performance, but it's just sloppy.  We should avoid unnecessary copies by habit.

[aelias_OOO_until_Jul13, 2013-05-21 00:51:46]

OK, comments addressed; I'll land when the trybots look good.

On 2013/05/20 18:25:53, abarth wrote:
> As a general rule, "manager" isn't a good suffix for a class name because it
> doesn't really mean anything.  It tends to lead to classes that have
ill-defined
> jobs.

OK, I renamed ViewportAttributes/ViewportAttributesManager to the less generic
PageScaleConstraints and PageScaleConstraintsSet, and moved them to core/page.

> 
> The amount of code in Internals is concerning.  Internals is just meant to be
a
> thin layer to let you poke at the internals of Core.  The fact that we've
grown
> a bunch of code here is worrying because it means we're testing Internals
itself
> more than we're really testing the product.  Should we change how these tests
> work so that less "glue" code is required?

OK, I deleted all of the static glue methods.  This caused the following layout
test failures:
- viewport-91.html started failing.  This one tests deviceScaleFactor
interaction.  As viewport tag has nothing to do with deviceScaleFactor anymore
in Blink, I deleted the test case and asserted deviceScaleFactor is 1.

- Viewport-height related tests started failing.  This is an exposure of an
existing problem.  I set failing expectations and filed http://crbug.com/242336

> 
> Also, your CL has a bunch of unnecessary copying.  I doubt that it matters for
> performance, but it's just sloppy.  We should avoid unnecessary copies by
habit.

Fixed.

[commit-bot: I haz the power, 2013-05-21 05:59:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/14813025/69001

[commit-bot: I haz the power, 2013-05-21 05:59:41]

Change committed as 150743

[rune, 2013-05-21 07:59:10]

Was the removal of orientation intentional? It breaks compilation for
ENABLE(CSS_DEVICE_ADAPTATION).

[rune, 2013-05-21 08:36:43]

On 2013/05/21 07:59:10, rune wrote:
> Was the removal of orientation intentional? It breaks compilation for
> ENABLE(CSS_DEVICE_ADAPTATION).

Compile fix for review:

https://codereview.chromium.org/15547002/

[aelias_OOO_until_Jul13, 2013-05-21 18:59:36]

Reopening as this got reverted in r150797 due to Android WebView test failures.

[mnaganov (inactive), 2013-05-24 09:26:37]

https://codereview.chromium.org/14813025/diff/88001/Source/core/page/Settings.h
File Source/core/page/Settings.h (right):

https://codereview.chromium.org/14813025/diff/88001/Source/core/page/Settings...
Source/core/page/Settings.h:181: bool m_useWideViewport : 1;
You should also add them into Settings.in to get them properly initialized.

[mnaganov (inactive), 2013-05-24 09:29:26]

https://codereview.chromium.org/14813025/diff/88001/Source/core/page/Settings.h
File Source/core/page/Settings.h (right):

https://codereview.chromium.org/14813025/diff/88001/Source/core/page/Settings...
Source/core/page/Settings.h:181: bool m_useWideViewport : 1;
On 2013/05/24 09:26:38, Mikhail Naganov (Chromium) wrote:
> You should also add them into Settings.in to get them properly initialized.

Ah, sorry. In your case, you should list them in the Settings constructor.

[aelias_OOO_until_Jul13, 2013-05-24 20:59:23]

Adam, I needed to add side effects to the useWideViewport and
loadWithOverviewMode settings changes because they are inputs to the
PageScaleConstraints and require a recomputation.  (This was causing
AwSettingsTest#testLayoutAlgorithmWithTwoViews to fail because it was no longer
getting lucky with timing.)  Is it OK for me to land this and move them to
non-settings calls later?

[abarth-chromium, 2013-05-24 21:01:50]

Yeah.

[aelias_OOO_until_Jul13, 2013-05-24 21:04:13]

Thanks.  Layout test bots look as green as they'll get, relanding.

[commit-bot: I haz the power, 2013-05-24 21:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/14813025/88001

[commit-bot: I haz the power, 2013-05-24 21:05:58]

Change committed as 151134