Bug 106021 – [Chromium] Add support for emulating legacy Android WebView 'UseWideViewport' setting

• Component: New Bugs
• Status: RESOLVED FIXED

[Mikhail Naganov]

Android WebView contains a setting called 'UseWideViewport'. It is processed in the following way: if the value is 'false', then page's 'meta viewport' tag is ignored and layout width is set to the device width in CSS pixels. If the value is 'true', then if the page doesn't have the 'meta viewport' tag, layout width is set to 980px, otherwise the specified width value is used.

The current code of Chromium port has a boolean setting named 'viewportEnabled'. Setting it to 'true' produces behavior similar to having 'UseWideViewport' set to 'true'. But having 'viewportEnabled' set to 'false' sets layout width to the device width in physical pixels. To overcome this, we can convert 'viewportEnabled' into a 3-way switch, where the new 3rd value specifies to ignore page's 'meta viewport' tag, but set layout width to the device width in CSS pixels.

[Mikhail Naganov]

Created attachment 181184 [details]
WIP

[Alexandre Elias]

Comment on attachment 181184 [details]
WIP

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

I don't think we need a third value.  Just make the "disabled" setting have the behavior you want.  I don't see any significant difference between what WebView wants and the desktop behavior.  The issue is just confused because of deviceScaleFactor coordinate space issues.  This will get cleaned up and converged between platforms over time when we move to applying device scale factor in the compositor on Android as well.  (Currently, Android's WebViewImpl::m_size is in physical pixels but ChromeOS's is in CSS pixels.)

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:639
> +        effectiveFallbackWidth = std::max(settings->layoutFallbackWidth(), dpiIndependentViewportWidth);

Are you sure settings->layoutFallbackWidth() ends up being 980 since you never set it?

[Mikhail Naganov]

Created attachment 181307 [details]
Patch

Thanks for the suggestion, Alexandre! I've updated the patch.

As for the fallback setting code I'm removing -- the default value for 'LayoutFallbackWidth' is set in Source/WebCore/page/Settings.in to 980. The code I'm removing actually violates the usage pattern of WebCore settings, as they should be updated by the embedder. For example, in the case when the embedder sets LayoutFallbackWidth to a non-default value, the aforementioned code will just reset it with its own value, which seems to be unacceptable.

[Alexandre Elias]

Comment on attachment 181307 [details]
Patch

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:634
> +    int dpiIndependentViewportWidth = m_webView->size().width / devicePixelRatio;

For this, please add a new method:

FloatSize WebViewImpl::dpiIndependentSize() const;

And this method should return just m_size if the setting applyDeviceScaleInCompositor is true.

[Mikhail Naganov]

Created attachment 181500 [details]
Factored out dpiIndependentSize

> For this, please add a new method:
>
> FloatSize WebViewImpl::dpiIndependentSize() const;
>
> And this method should return just m_size if the setting applyDeviceScaleInCompositor is true.

Done. But I can't add it to the WebView.h interface since FloatSize is a WebCore type. There is no corresponding type in the public API. The closest ones are WebFloatPoint and WebFloatRect. Do you think it's OK to leave this method to be impl-only? Or perhaps we should clamp up floats and return WebSize?

[Alexandre Elias]

Let's leave it WebViewImpl-only (and non-virtual) as it's not for use from the Chromium tree.

Adding abarth@ for WebKit review.

[Adam Barth]

Comment on attachment 181500 [details]
Factored out dpiIndependentSize

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:637
> +        Settings* settings = m_webView->page()->settings();

I'd skip creating this temporary.  There isn't an 80 column limit in WebKit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3125
> +WebCore::FloatSize WebViewImpl::dpiIndependentSize() const

The term dpiIndependent is confusing to me.  Is there some terminology on http://trac.webkit.org/wiki/ScalesAndZooms that accurately captures what you're trying to express here?  For example, is this quantity a number of physical pixels, logical pixels, or CSS pixels?

In general, I'd like to remove the notion of "DPI" from this code as much as possible.  We should be talking in terms of scale factors between different coordinate spaces.

[Alexandre Elias]

It's "logical pixels".  We can change it to logicalSize() if you prefer.

We haven't been using the term "logical pixels" anywhere outside that document yet.  In Chromium land it's called "DIP pixels" (for Device Independent Pixels).

Note that this is somewhat transitional, as I'm hoping that eventually we can move to a model where WebKit doesn't hear about physical pixels at all.  Chromium would tell it everything in logical pixels and CC would take care the conversion to physical pixels.

[Adam Barth]

(In reply to comment #8)
> It's "logical pixels".  We can change it to logicalSize() if you prefer.
> 
> We haven't been using the term "logical pixels" anywhere outside that document yet.  In Chromium land it's called "DIP pixels" (for Device Independent Pixels).

Maybe it would be easier to name "logical pixels" to "DIP pixels" in the ScalesAndZooms page instead?

> Note that this is somewhat transitional, as I'm hoping that eventually we can move to a model where WebKit doesn't hear about physical pixels at all.  Chromium would tell it everything in logical pixels and CC would take care the conversion to physical pixels.

Understood.  I would still like to use a consistent terminology even during the transition.  I don't have much of an opinion about what that terminology should be, I'd like to be consistent.  :)

[Adam Barth]

> Maybe it would be easier to name "logical pixels" to "DIP pixels" in the ScalesAndZooms page instead?

s/name/change/

[Adam Barth]

Comment on attachment 181500 [details]
Factored out dpiIndependentSize

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

Seems fine apart from naming nits.

> Source/WebKit/chromium/src/WebViewImpl.cpp:-1592
> -        const int standardFallbackWidth = 980;
> -        int dpiIndependentViewportWidth = newSize.width / page()->deviceScaleFactor();
> -        settings()->setLayoutFallbackWidth(std::max(standardFallbackWidth, dpiIndependentViewportWidth));

What happened to this code?

[Alexandre Elias]

OK, let's go with DIPSize() as that will be clearer to ChromeOS folks, and change the doc.  Actually, it seems that DIP stands for Density Independent Pixel.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:-1592
> > -        const int standardFallbackWidth = 980;
> > -        int dpiIndependentViewportWidth = newSize.width / page()->deviceScaleFactor();
> > -        settings()->setLayoutFallbackWidth(std::max(standardFallbackWidth, dpiIndependentViewportWidth));
> 
> What happened to this code?

It's no longer needed, since it turns out that our only use of layoutFallbackWidth was in dispatchViewportPropertiesDidChange anyway.

[Mikhail Naganov]

Created attachment 181677 [details]
Comments addressed

I addressed all the comments and renamed "logical pixels" to "DIP pixels" on the wiki page

[Adam Barth]

Comment on attachment 181677 [details]
Comments addressed

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3123
> +WebCore::FloatSize WebViewImpl::DIPSize() const

nit: DIPSize -> dipSize

WebKit uses lower case for initial acronyms.

[Mikhail Naganov]

Created attachment 181874 [details]
s/DIPSize/dipSize/

[Mikhail Naganov]

Committed r139177: <http://trac.webkit.org/changeset/139177>

[noel gordon]

http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/32529

viewport webkit-unit-tests started failing with this change, expected?

[Mikhail Naganov]

(In reply to comment #17)
> http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/32529
> 
> viewport webkit-unit-tests started failing with this change, expected?

Thanks for a heads-up! Perhaps, the test needs some adjustment. Let me check that.

[Mikhail Naganov]

I've made a speculative fix to WebFrameTest.DivAutoZoomParamsTest by moving the line that enables fixed layout after lines that set size and scale of the WebView:

http://trac.webkit.org/changeset/139187

Although, I must admit, I don't know all the scenarios of viewport and fixed layout usage. The change that I've made to ChromeClientImpl makes dispatchViewportPropertiesDidChange to be executed when 'enableViewport' is false. In Chrome for Android, we have both viewport and fixed layout enabled, while WebFrameTests only enable fixed layout.