Bug 100061 – [chromium] Device scale factor should be no-op when applyDeviceScaleFactorInCompositor == 0

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

[Tien-Ren Chen]

[chromium] Device scale factor should be no-op when applyPageScaleFactorInCompositor == 0

[Tien-Ren Chen]

Created attachment 170045 [details]
Patch

[Tien-Ren Chen]

FYI this patch will not affect platforms other than Android, and Android only uses software rendering path for generating disambiguation popups.

[Peter Beverloo (cr-android ews)]

Comment on attachment 170045 [details]
Patch

Attachment 170045 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14483745

[Tien-Ren Chen]

Created attachment 170048 [details]
Patch

[Alexandre Elias]

LGTM.  Adam, could you r+?

[WebKit Review Bot]

Comment on attachment 170048 [details]
Patch

Rejecting attachment 170048 [details] from commit-queue.

New failing tests:
fast/hidpi/image-set-out-of-order.html
fast/hidpi/image-set-as-background.html
fast/hidpi/image-set-background-dynamic.html
fast/hidpi/image-set-in-content-dynamic.html
fast/hidpi/device-scale-factor-paint.html
fast/hidpi/resize-corner-hidpi.html
fast/hidpi/image-set-border-image-comparison.html
fast/hidpi/video-controls-in-hidpi.html
fast/hidpi/image-set-border-image-simple.html
fast/hidpi/image-set-background-repeat-without-size.html
fast/hidpi/image-set-simple.html
fast/hidpi/image-set-border-image-dynamic.html
fast/hidpi/image-set-without-specified-width.html
fast/hidpi/focus-rings.html
fast/hidpi/gradient-with-scaled-ancestor.html
editing/spelling/inline-spelling-markers-hidpi.html
fast/hidpi/clip-text-in-hidpi.html
editing/spelling/grammar-markers-hidpi.html
fast/hidpi/image-set-background-repeat.html
Full output: http://queues.webkit.org/results/14505339

[Tien-Ren Chen]

Created attachment 170223 [details]
Patch

[Adam Barth]

Comment on attachment 170223 [details]
Patch

Seems to be causing many test failures.

[Tien-Ren Chen]

Created attachment 170770 [details]
Patch

[Adam Barth]

Comment on attachment 170770 [details]
Patch

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

Why do we need to teach WebCore about this setting if it is only ever used in the WebKit layer?  It seems instead that we should teach PageWidgetDelegate.cpp how to find the WebSettings.

> Source/WebCore/page/Settings.h:644
> +        void setApplyDefaultDeviceScaleFactorInCompositor(bool enabled) { m_applyDefaultDeviceScaleFactorInCompositor = enabled; }
> +        bool applyDefaultDeviceScaleFactorInCompositor() const { return m_applyDefaultDeviceScaleFactorInCompositor; }

There is no such thing as a "default" device scale factor.  There is only a "device scale factor" and a "page scale factor".  There used to be a "default" device scale factor, but we deleted that notion.  Can you rename this setting?

[Tien-Ren Chen]

Created attachment 174606 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174606 [details]
Patch

Attachment 174606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14859223

[Peter Beverloo (cr-android ews)]

Comment on attachment 174606 [details]
Patch

Attachment 174606 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14843979

[Adam Barth]

Comment on attachment 174606 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.h:590
> +    bool applyDeviceScaleFactorInCompositor() const;

This looks more like a command than a state query...

[Tien-Ren Chen]

(In reply to comment #14)
> (From update of attachment 174606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174606&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.h:590
> > +    bool applyDeviceScaleFactorInCompositor() const;
> 
> This looks more like a command than a state query...

Do you think shouldApplyDeviceScaleFactorInCompositor sounds better?

[Tien-Ren Chen]

Created attachment 174772 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174772 [details]
Patch

Attachment 174772 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14877092

New failing tests:
fast/hidpi/image-set-out-of-order.html
fast/hidpi/image-set-as-background.html
fast/hidpi/image-set-background-dynamic.html
fast/hidpi/image-set-in-content-dynamic.html
fast/hidpi/device-scale-factor-paint.html
fast/hidpi/resize-corner-hidpi.html
fast/hidpi/image-set-border-image-comparison.html
fast/hidpi/video-controls-in-hidpi.html
fast/hidpi/image-set-border-image-simple.html
fast/hidpi/image-set-background-repeat-without-size.html
fast/hidpi/image-set-simple.html
fast/hidpi/image-set-border-image-dynamic.html
fast/hidpi/image-set-without-specified-width.html
fast/hidpi/focus-rings.html
fast/hidpi/gradient-with-scaled-ancestor.html
editing/spelling/inline-spelling-markers-hidpi.html
fast/hidpi/clip-text-in-hidpi.html
editing/spelling/grammar-markers-hidpi.html
fast/hidpi/image-set-background-repeat.html

[Tien-Ren Chen]

Created attachment 174796 [details]
Patch

[Adam Barth]

> Do you think shouldApplyDeviceScaleFactorInCompositor sounds better?

Yeah.

[Adam Barth]

Comment on attachment 174796 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:4349
> +bool WebViewImpl::shouldApplyDeviceScaleFactorInCompositor() const
> +{
> +    return m_webSettings->applyDeviceScaleFactorInCompositor();
> +}

I don't really understand the point of this function.  Why don't callers just get the information from the settings themselves?  We don't want to create these sorts of accessors for every setting...

[Tien-Ren Chen]

(In reply to comment #20)
> (From update of attachment 174796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174796&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:4349
> > +bool WebViewImpl::shouldApplyDeviceScaleFactorInCompositor() const
> > +{
> > +    return m_webSettings->applyDeviceScaleFactorInCompositor();
> > +}
> 
> I don't really understand the point of this function.  Why don't callers just get the information from the settings themselves?  We don't want to create these sorts of accessors for every setting...

I can only get WebSettings but not WebSettingsImpl from the outside of WebViewImpl. :(

The options are:
1. Create an accessor on WebViewImpl.
2. Make WebViewImpl::settingsImpl() public.
3. Add virtual getters on WebSettings.
4. Passing it as an argument to WebPagePopupImpl::create

Which solution you think is the best?

[Adam Barth]

> The options are:
> 1. Create an accessor on WebViewImpl.
> 2. Make WebViewImpl::settingsImpl() public.
> 3. Add virtual getters on WebSettings.
> 4. Passing it as an argument to WebPagePopupImpl::create
> 
> Which solution you think is the best?

I'd got with option (2).  That's going to generalize best for the future and won't pollute the API with implementation concerns.

[Tien-Ren Chen]

Created attachment 175097 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175097 [details]
Patch

Clearing flags on attachment: 175097

Committed r135459: <http://trac.webkit.org/changeset/135459>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.