Bug 110835 – [Chromium] Implement target-densityDpi viewport property emulation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Bo Liu]

Add Chrome::processUnrecognizedViewportArgument method

[Bo Liu]

Created attachment 190177 [details]
Patch

[Adam Barth]

Looks reasonable.  Let me know when you'd like an actual review.  (I'll probably ask you to add a test.)

[Mikhail Naganov]

Created attachment 195362 [details]
A new approach suggested by aelias@

I can't make a navigation in the same WebView in order to test the resetting of the target-density-dpi scaling factor.

[WebKit Review Bot]

Attachment 195362 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/ViewportArguments.cpp', u'Source/WebCore/page/Chrome.cpp', u'Source/WebCore/page/Chrome.h', u'Source/WebCore/page/ChromeClient.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/ChromeClientImpl.cpp', u'Source/WebKit/chromium/src/ChromeClientImpl.h', u'Source/WebKit/chromium/tests/WebFrameTest.cpp', u'Source/WebKit/chromium/tests/data/viewport-target-densitydpi-high.html']" exit_code: 1
Source/WebKit/chromium/src/ChromeClientImpl.cpp:693:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/ChromeClientImpl.h:262:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandre Elias]

Trying to keep code out of ViewportArguments.cpp makes it very complicated.  How about introducing a new WebCore setting "supportsTargetDensityDpi" and put all the code in ViewportArguments instead?  We'll need a setting in any case.  You could then avoid ChromeClientImpl changes entirely.

[Build Bot]

Comment on attachment 195362 [details]
A new approach suggested by aelias@

Attachment 195362 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17253597

[Jonathan Dixon]

Adam, could you comment if this is going the direction you intend? (e.g.  #5?)

From internal email thread (b/6634479)


"""
Bo: you suggested adding something like onUnknownMetaAttribute to WebFrameClient? Does that sound more reasonable?

Adam: That sounds fine.  It's also fine to tell the WebFrameClient about
target-densityDPI specifically.  The important thing is for the embedder to do the scaling instead of having WebKit know about another scale factor.
"""


(I don't have any opinion on this, just trying to head off needless iterations on it)

[Bo Liu]

Comment on attachment 195362 [details]
A new approach suggested by aelias@

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:675
> +    if (equalIgnoringCase(keyString, "target-densitydpi")) {

I believe Adam said this block of logic needs to live in android webview only code, ie don't re-introduce target-densitydpi on other platforms

[Alexandre Elias]

Adam is currently on vacation.  In my understanding, the main thing is to make sure the functionality is not actually enabled on other platforms to avoid open web developers starting to use it.  Whether the code lives inside WebKit/ or WebCore/ is secondary.  And the current attempt to avoid WebCore is clearly ugly.  Adding a WebCore setting will allow us to let the code only *run* in AWV mode.

[Jonathan Dixon]

Based on "There's a lot of confusion in the code base about how page and device scaling works" and "The important thing is for the embedder to do the scaling instead of having WebKit know about another scale factor" I thought this was more about code maintainability than risk of feature pollution into other platform/configurations.

(I do agree the net effect of patch set 2 is confusing, but was hoping to get a reviewer opinion on it before we iterate back to effectively reverting https://bugs.webkit.org/show_bug.cgi?id=88047 but wrapping an extra setting around it)

[Alexandre Elias]

We found that doing the scaling on the embedder isn't tractable, unfortunately.  As the patch stands, it still needs a setting in order to avoid adding support to mainline Chromium, and the mutable Document pointer definitely should be avoided somehow.

Anyway, Adam should be back next week.

[Build Bot]

Comment on attachment 195362 [details]
A new approach suggested by aelias@

Attachment 195362 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17342104

[Mikhail Naganov]

Created attachment 195547 [details]
Move parsing code to ViewportArguments

Alexandre, thanks for your suggestions!

This is a refined version. I have moved the parsing code into ViewportArguments. But parsing can't depend on a setting, because if the setting will be enabled after the document has been parsed, that's too late, and I don't want to trigger reparsing when the setting changes. So I have stuck with a Chromium-only setting that controls, whether we are actually using the provided value.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 195547 [details]
Move parsing code to ViewportArguments

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

> Source/WebCore/ChangeLog:10
> +        This is needed for supporting existing WebView-based applications
> +        that rely on this property.

When do we expect to be able to remove this code?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:668
> +    if (m_webView->settingsImpl()->supportDeprecatedTargetDensityDpi()) {
> +        float targetDensityDpiFactor = calculateTargetDensityDpiFactor(arguments, deviceScaleFactor);
> +        computed.initialScale *= targetDensityDpiFactor;
> +        computed.minimumScale *= targetDensityDpiFactor;
> +        computed.maximumScale *= targetDensityDpiFactor;
> +        computed.layoutSize.scale(1.0f / targetDensityDpiFactor);
> +    }

This work should be done by the embedder.

[Adam Barth]

> We found that doing the scaling on the embedder isn't tractable, unfortunately.

Can you explain in more detail?

I would like to avoid having WebKit/WebCore know anything about target-densitydpi.  It's a concern of the Android WebView to support this legacy behavior, not of WebKit.

[Adam Barth]

Comment on attachment 195547 [details]
Move parsing code to ViewportArguments

r- pending more discussion.

[Alexandre Elias]

(In reply to comment #16)
> > We found that doing the scaling on the embedder isn't tractable, unfortunately.
> 
> Can you explain in more detail?
> 
> I would like to avoid having WebKit/WebCore know anything about target-densitydpi.  It's a concern of the Android WebView to support this legacy behavior, not of WebKit.

I was referring to an idea we had earlier to set a different size in WebView::resize() and deviceScaleFactor.  But that won't work because input events are scaled by deviceScaleFactor in the browser process, and Android (having no multi-monitor support) assumes a single global value of deviceScaleFactor.  So this patch's approach of simply scaling the fixedLayoutSize works better.

In any case, are you asking that we expose the ViewportArguments struct in the WebKit API and do the scaling in Chromium-side code?  If so, that would certainly work.  Although, I'm concerned that if we go down that path, would end up adding an equal or greater amount of cruft in the form of otherwise-unnecessary API hooks.

[Adam Barth]

Comment on attachment 195547 [details]
Move parsing code to ViewportArguments

This approach is an appealingly small amount of code.  Maybe this approach is better than spilling a bunch of API.

[Mikhail Naganov]

I'm now confused. Should we try another approach (from the comment 18)? Or are we OK with this one?

[Adam Barth]

Comment on attachment 195547 [details]
Move parsing code to ViewportArguments

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

Ok, I'm convinced about this approach.  It's nice that it's not much code.  However, if the implementation starts spidering throughout the codebase like the old implementation, we might want to reconsider.

> Source/WebCore/dom/ViewportArguments.h:96
> +        , deprecatedTargetDensityDpi(ValueAuto)

Dpi -> DPI

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:662
> +    if (m_webView->settingsImpl()->supportDeprecatedTargetDensityDpi()) {

Dpi -> DPI

[Mikhail Naganov]

Created attachment 196305 [details]
Dpi -> DPI

Thank you, Adam!

[Mikhail Naganov]

Committed r147529: <http://trac.webkit.org/changeset/147529>