[chromium] Device scale factor should be no-op when applyPageScaleFactorInCompositor == 0
Created attachment 170045 [details] Patch
FYI this patch will not affect platforms other than Android, and Android only uses software rendering path for generating disambiguation popups.
Comment on attachment 170045 [details] Patch Attachment 170045 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14483745
Created attachment 170048 [details] Patch
LGTM. Adam, could you r+?
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
Created attachment 170223 [details] Patch
Comment on attachment 170223 [details] Patch Seems to be causing many test failures.
Created attachment 170770 [details] Patch
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?
Created attachment 174606 [details] Patch
Comment on attachment 174606 [details] Patch Attachment 174606 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14859223
Comment on attachment 174606 [details] Patch Attachment 174606 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14843979
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...
(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?
Created attachment 174772 [details] Patch
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
Created attachment 174796 [details] Patch
> Do you think shouldApplyDeviceScaleFactorInCompositor sounds better? Yeah.
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...
(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?
> 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.
Created attachment 175097 [details] Patch
Comment on attachment 175097 [details] Patch Clearing flags on attachment: 175097 Committed r135459: <http://trac.webkit.org/changeset/135459>
All reviewed patches have been landed. Closing bug.