Bug 92794 – [chromium] deviceViewportSize cleanup

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

[chromium] deviceViewportSize cleanup

[Alexandre Elias]

Created attachment 155646 [details]
Patch

[Alexandre Elias]

I haven't updated all the tests yet, I'll do it when this change is confirmed OK.

[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.

[WebKit Review Bot]

Comment on attachment 155646 [details]
Patch

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

[Adrienne Walker]

Comment on attachment 155646 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:-808
>      if (CCLayerImpl* clipLayer = m_rootScrollLayerImpl->parent()) {
> -        if (clipLayer->masksToBounds())
> +        // Compensate for non-overlay scrollbars.
> +        if (clipLayer->masksToBounds()) {
>              viewBounds = clipLayer->bounds();
> +            viewBounds.scale(m_deviceScaleFactor);
> +        }
>      }
>      viewBounds.scale(1 / m_pageScaleDelta);
> -    viewBounds.scale(m_deviceScaleFactor);

I don't understand how this math is consistent with your ChangeLog description.  If layoutViewportSize * deviceScale != deviceViewportSize, then why can you multiply clipLayer bounds * deviceScale to be clipped physical pixels?

> Source/WebKit/chromium/src/WebLayerTreeView.cpp:118
> +    if (deviceViewportSize().isEmpty() && !layoutViewportSize().isEmpty()) {
> +        IntSize deviceViewportSize = layoutViewportSize();
> +        deviceViewportSize.scale(deviceScaleFactor);
> +        setViewportSize(layoutViewportSize(), deviceViewportSize);
> +    }

Re: temporary scaffolding.  Are you sure that setDeviceScaleFactor is always called after setViewportSize?

[Alexandre Elias]

Created attachment 155910 [details]
Patch

Changed scaffolding, updated unit tests

[Alexandre Elias]

(In reply to comment #5)
> (From update of attachment 155646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155646&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:-808
> >      if (CCLayerImpl* clipLayer = m_rootScrollLayerImpl->parent()) {
> > -        if (clipLayer->masksToBounds())
> > +        // Compensate for non-overlay scrollbars.
> > +        if (clipLayer->masksToBounds()) {
> >              viewBounds = clipLayer->bounds();
> > +            viewBounds.scale(m_deviceScaleFactor);
> > +        }
> >      }
> >      viewBounds.scale(1 / m_pageScaleDelta);
> > -    viewBounds.scale(m_deviceScaleFactor);
> 
> I don't understand how this math is consistent with your ChangeLog description.  If layoutViewportSize * deviceScale != deviceViewportSize, then why can you multiply clipLayer bounds * deviceScale to be clipped physical pixels?

I was also uncertain about this part, but I've convinced myself that it's correct.  clipLayer bounds still remains (and has to remain) closely related to deviceViewportSize.  Multiplying it by deviceScaleFactor will do the right thing.

> 
> > Source/WebKit/chromium/src/WebLayerTreeView.cpp:118
> > +    if (deviceViewportSize().isEmpty() && !layoutViewportSize().isEmpty()) {
> > +        IntSize deviceViewportSize = layoutViewportSize();
> > +        deviceViewportSize.scale(deviceScaleFactor);
> > +        setViewportSize(layoutViewportSize(), deviceViewportSize);
> > +    }
> 
> Re: temporary scaffolding.  Are you sure that setDeviceScaleFactor is always called after setViewportSize?

You're right, it seems that in Aura setDeviceScaleFactor is never called at all, and it's relying on the default value of 1.  I changed the scaffolding appropriately.

[Adrienne Walker]

Comment on attachment 155910 [details]
Patch

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

> Source/WebKit/chromium/src/WebLayerTreeView.cpp:100
> +    if (!deviceViewportSize.isEmpty())
> +        m_private->layerTreeHost()->setViewportSize(layoutViewportSize, deviceViewportSize);
> +    else
> +        m_private->layerTreeHost()->setViewportSize(layoutViewportSize, layoutViewportSize);

I know it's just temporary, but you need to handle non-1 device scale factors to not break Aura in the meantime.

Would it be easier to just ignore this deviceViewportSize parameter in all cases until the Chromium side of this change lands, and just call m_private->layerTreeHost()->setViewportSize whenever layoutViewportSize or deviceScale changes?

[Alexandre Elias]

The root compositor in Aura currently has its own device scale implementation and does not rely on the CC functionality.  I tested with Linux Aura with "--force-device-scale-factor=2 --force-compositing-mode" and the display looked the same as without my patch.

[Adrienne Walker]

(In reply to comment #9)
> The root compositor in Aura currently has its own device scale implementation and does not rely on the CC functionality.  I tested with Linux Aura with "--force-device-scale-factor=2 --force-compositing-mode" and the display looked the same as without my patch.

I don't understand how that could be.  With --force-device-scale-factor=2, layoutViewport != deviceViewport.  The deviceViewportSize is used for setting the root surface content rect, so this seems like it could cause visibility or occlusion errors.

[Alexandre Elias]

The renderer compositor uses the WebViewImpl call I changed as part of this patch.  For the browser compositor, there is no call to WebLayerTreeView::setDeviceScaleFactor anywhere in the chromium repository.

[Adrienne Walker]

Comment on attachment 155910 [details]
Patch

(In reply to comment #11)
> The renderer compositor uses the WebViewImpl call I changed as part of this patch.  For the browser compositor, there is no call to WebLayerTreeView::setDeviceScaleFactor anywhere in the chromium repository.

Thanks for being patient with my worries about breaking changes.  I misunderstood how device scale worked for the browser compositor.

R=me.

[WebKit Review Bot]

Comment on attachment 155910 [details]
Patch

Clearing flags on attachment: 155910

Committed r124543: <http://trac.webkit.org/changeset/124543>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.