[Android WebView] Avoid expanding layout width to match the old WebView

[Android WebView] Avoid expanding layout width to match the old WebView

The old WebView doesn't expand the layout width for the resolved initial scale.

BUG=277369
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156969

[mnaganov (inactive), 2013-08-21 13:26:33]

Alexandre, PTAL!

If you are OK with the change, please forward it for owner's review afterwards.

[aelias_OOO_until_Jul13, 2013-08-21 20:23:54]

https://codereview.chromium.org/23038013/diff/1/Source/core/page/PageScaleCon...
File Source/core/page/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/23038013/diff/1/Source/core/page/PageScaleCon...
Source/core/page/PageScaleConstraintsSet.cpp:140: adjustedLayoutSizeWidth =
arguments.width;
This ends up bypassing every other width computation in ViewportArguments, for
example min-width.  It looks like the only way to do make the behavior change as
narrow as we want is via a parameter used by ViewportArguments::resolve().  

I suggest adding a new enum value to ViewportArguments::resolve,
ViewportMetaSupportingTargetDensityDpi.  Change arguments.type to that in
WebViewImpl::updatePageDefinedPageScaleConstraints by making a local copy of the
object.  Then ViewportArguments::resolve() will already do the right thing
without any further changes (because CSS-device-adaptation standard doesn't want
the weird initial-scale behavior of Safari either).

[mnaganov (inactive), 2013-08-22 08:51:55]

https://codereview.chromium.org/23038013/diff/1/Source/core/page/PageScaleCon...
File Source/core/page/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/23038013/diff/1/Source/core/page/PageScaleCon...
Source/core/page/PageScaleConstraintsSet.cpp:140: adjustedLayoutSizeWidth =
arguments.width;
On 2013/08/21 20:23:54, aelias wrote:
> This ends up bypassing every other width computation in ViewportArguments, for
> example min-width.  It looks like the only way to do make the behavior change
as
> narrow as we want is via a parameter used by ViewportArguments::resolve().  
> 
> I suggest adding a new enum value to ViewportArguments::resolve,
> ViewportMetaSupportingTargetDensityDpi.  Change arguments.type to that in
> WebViewImpl::updatePageDefinedPageScaleConstraints by making a local copy of
the
> object.  Then ViewportArguments::resolve() will already do the right thing
> without any further changes (because CSS-device-adaptation standard doesn't
want
> the weird initial-scale behavior of Safari either).

Thanks for the suggestion, Alexandre! I didn't consider adding a new
ViewportArguments type, thinking only of adding an extra argument to ::resolve,
which seemed too ugly, so I resorted to post-patching the value. You approach is
much more creative!

[aelias_OOO_until_Jul13, 2013-08-24 00:53:34]

[+abarth] Hmm, I'm a bit on the fence about whether this is fine, or whether we
should tie this quirk instead to a new setting that is already disabled when
target API = the one for K release.  On the one hand it's simpler to set all the
viewport-tag quirks with a single setting, on the other hand disabling the one
quirk now would give us more standardsy behavior earlier.  I'm leaning towards
the former view.  Adam, do you have an opinion on this?

[abarth-chromium, 2013-08-24 18:26:18]

On 2013/08/24 00:53:34, aelias wrote:
> Adam, do you have an opinion on this?

I'd recommend using a separate setting.  That way the concerns are separated and
we can drop support for this quirk independently of target-densitydpi.

[mnaganov (inactive), 2013-08-27 15:47:30]

OK, I created a separate setting for that.

[kenneth.r.christiansen, 2013-08-27 17:42:21]

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImp...
Source/web/WebSettingsImpl.h:96: virtual void
setExpandLayoutToFillVisualViewport(bool);
As this is a Quirk can be append Quirk?

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImp...
Source/web/WebSettingsImpl.h:200: bool m_expandLayoutToFillVisualViewport;
*Quirk

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2999:
m_pageScaleConstraintsSet.updatePageDefinedConstraints(adjustedArguments,
m_size, page()->settings().layoutFallbackWidth());
I would like us to be able to do this in the algorithm instead of afterward, as
we are moving to use @viewport internally

[mnaganov (inactive), 2013-08-27 18:16:50]

Hello, Kenneth! I'm glad to get your feedback. Can you please be more specific
in your third comment?

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImp...
Source/web/WebSettingsImpl.h:96: virtual void
setExpandLayoutToFillVisualViewport(bool);
On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> As this is a Quirk can be append Quirk?

I have no problem with adding it, although, I would like to hear what other
reviewers think about this.

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2999:
m_pageScaleConstraintsSet.updatePageDefinedConstraints(adjustedArguments,
m_size, page()->settings().layoutFallbackWidth());
On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> I would like us to be able to do this in the algorithm instead of afterward,
as
> we are moving to use @viewport internally

Sorry, it's not clear to me what do you mean by "this", and what do you mean by
"the algorithm". Are you proposing to pass the flag to
ViewportArguments::resolve instead of suppressing the viewport expansion logic
by tweaking the VA type?

[mnaganov (inactive), 2013-08-28 18:52:00]

On 2013/08/27 18:16:50, Mikhail Naganov (Chromium) wrote:
> Hello, Kenneth! I'm glad to get your feedback. Can you please be more specific
> in your third comment?
> 
>
https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImpl.h
> File Source/web/WebSettingsImpl.h (right):
> 
>
https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImp...
> Source/web/WebSettingsImpl.h:96: virtual void
> setExpandLayoutToFillVisualViewport(bool);
> On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> > As this is a Quirk can be append Quirk?
> 
> I have no problem with adding it, although, I would like to hear what other
> reviewers think about this.
> 
> https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cp...
> Source/web/WebViewImpl.cpp:2999:
> m_pageScaleConstraintsSet.updatePageDefinedConstraints(adjustedArguments,
> m_size, page()->settings().layoutFallbackWidth());
> On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> > I would like us to be able to do this in the algorithm instead of afterward,
> as
> > we are moving to use @viewport internally
> 
> Sorry, it's not clear to me what do you mean by "this", and what do you mean
by
> "the algorithm". Are you proposing to pass the flag to
> ViewportArguments::resolve instead of suppressing the viewport expansion logic
> by tweaking the VA type?

abarth@, aelias@: Can you please take a look at the patch set 3 -- do you think
anything needs to be fixed?

[aelias_OOO_until_Jul13, 2013-08-28 21:26:46]

lg2m modulo the below changes.

https://codereview.chromium.org/23038013/diff/12001/Source/core/dom/ViewportA...
File Source/core/dom/ViewportArguments.h (right):

https://codereview.chromium.org/23038013/diff/12001/Source/core/dom/ViewportA...
Source/core/dom/ViewportArguments.h:58: NoExpansionToFillVisualViewport
Please move it one line up (next to "ViewportMeta") and rename to
ViewportMetaLayoutSizeQuirk.

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImp...
Source/web/WebSettingsImpl.h:96: virtual void
setExpandLayoutToFillVisualViewport(bool);
On 2013/08/27 18:16:51, Mikhail Naganov (Chromium) wrote:
> On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> > As this is a Quirk can be append Quirk?
> 
> I have no problem with adding it, although, I would like to hear what other
> reviewers think about this.

Adding "Quirk" sounds fine to me.

Maybe name it ViewportMetaLayoutSizeQuirk?  And invert the meaning of the bool
so that if "true" then it is the WebView quirk.

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2999:
m_pageScaleConstraintsSet.updatePageDefinedConstraints(adjustedArguments,
m_size, page()->settings().layoutFallbackWidth());
On 2013/08/27 18:16:51, Mikhail Naganov (Chromium) wrote:
> On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> > I would like us to be able to do this in the algorithm instead of afterward,
> as
> > we are moving to use @viewport internally
> 
> Sorry, it's not clear to me what do you mean by "this", and what do you mean
by
> "the algorithm". Are you proposing to pass the flag to
> ViewportArguments::resolve instead of suppressing the viewport expansion logic
> by tweaking the VA type?

I think Kenneth is concerned that you're clobbering the ViewportArguments type
if it's something other than meta viewport tag.  I propose changing your if
statement above to:

if (settingsImpl()->viewportMetaLayoutSizeQuirk && arguments.type ==
ViewportArguments::ViewportMeta)

[aelias_OOO_until_Jul13, 2013-08-28 21:35:20]

Well, also kenneth's deeper concern is that
https://codereview.chromium.org/19555002/ refactors a lot of this and deletes
"enum Type" entirely.  But this patch is for an M30 blocker (which has already
branched) so we need to land something that works with the current code, and I
unfortunately don't see any way to fix this orthogonally to that refactoring.

(I think in Kenneth's css-adaptation-based implementation, we will be able to
get the same effect by disabling the "expand-to-width" property in the generated
@viewport structure, when the quirk setting is specified, and rely on the unit
test to check the desired quirk behavior is preserved.)

[mnaganov (inactive), 2013-08-29 12:09:54]

https://codereview.chromium.org/23038013/diff/12001/Source/core/dom/ViewportA...
File Source/core/dom/ViewportArguments.h (right):

https://codereview.chromium.org/23038013/diff/12001/Source/core/dom/ViewportA...
Source/core/dom/ViewportArguments.h:58: NoExpansionToFillVisualViewport
On 2013/08/28 21:26:47, aelias wrote:
> Please move it one line up (next to "ViewportMeta") and rename to
> ViewportMetaLayoutSizeQuirk.

Done.

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebSettingsImp...
Source/web/WebSettingsImpl.h:96: virtual void
setExpandLayoutToFillVisualViewport(bool);
On 2013/08/28 21:26:47, aelias wrote:
> On 2013/08/27 18:16:51, Mikhail Naganov (Chromium) wrote:
> > On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> > > As this is a Quirk can be append Quirk?
> > 
> > I have no problem with adding it, although, I would like to hear what other
> > reviewers think about this.
> 
> Adding "Quirk" sounds fine to me.
> 
> Maybe name it ViewportMetaLayoutSizeQuirk?  And invert the meaning of the bool
> so that if "true" then it is the WebView quirk.

Done.

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23038013/diff/12001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2999:
m_pageScaleConstraintsSet.updatePageDefinedConstraints(adjustedArguments,
m_size, page()->settings().layoutFallbackWidth());
On 2013/08/28 21:26:47, aelias wrote:
> On 2013/08/27 18:16:51, Mikhail Naganov (Chromium) wrote:
> > On 2013/08/27 17:42:21, kenneth.r.christiansen wrote:
> > > I would like us to be able to do this in the algorithm instead of
afterward,
> > as
> > > we are moving to use @viewport internally
> > 
> > Sorry, it's not clear to me what do you mean by "this", and what do you mean
> by
> > "the algorithm". Are you proposing to pass the flag to
> > ViewportArguments::resolve instead of suppressing the viewport expansion
logic
> > by tweaking the VA type?
> 
> I think Kenneth is concerned that you're clobbering the ViewportArguments type
> if it's something other than meta viewport tag.  I propose changing your if
> statement above to:
> 
> if (settingsImpl()->viewportMetaLayoutSizeQuirk && arguments.type ==
> ViewportArguments::ViewportMeta)

Done.

[aelias_OOO_until_Jul13, 2013-08-29 18:18:05]

lgtm, just needs OWNERS approval.  Adam/Kenneth?

[abarth-chromium, 2013-08-30 06:50:32]

LGTM.  Thank you for the test.

[commit-bot: I haz the power, 2013-08-30 06:50:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/23038013/23001

[commit-bot: I haz the power, 2013-08-30 08:08:27]

Change committed as 156969