Bug 94935 – [chromium] High-DIP pixel test output should be the correct size and also work with --force-compositing-mode

• Component: WebKit Misc.
• Status: RESOLVED FIXED

We need to output the results of hidpi tests ad device resolution, not layout resolution.

Created attachment 160431 [details]
Patch

[Terry Anderson]

Comment on attachment 160431 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        marking the files on other platforms as expected to fail until I can update them later 

Won't the expected render tree dumps also need to be updated since the size has changed from 800x600 to 1600x1200?

Render tree should remain the same because the sizes in it are in px which are angular units and could map to different pixel resolutions depending on the device scale factor.

[Terry Anderson]

*** Bug 93647 has been marked as a duplicate of this bug. ***

[Dirk Pranke]

This patch looks plausible to me but I'm not close to how we're trying to handle the hidpi stuff.

James, does this look good to you?

[Alexandre Elias]

Comment on attachment 160431 [details]
Patch

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

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1876
> +        int scaledX = static_cast<int>(static_cast<float>(m_paintRect.x) * deviceScaleFactor);

Seems odd to multiply here.  How about multiplying widgetSize by deviceScaleFactor instead?

(In reply to comment #6)
> (From update of attachment 160431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160431&action=review
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1876
> > +        int scaledX = static_cast<int>(static_cast<float>(m_paintRect.x) * deviceScaleFactor);
> 
> Seems odd to multiply here.  How about multiplying widgetSize by deviceScaleFactor instead?

Well, m_paintRect is in layout units as well, so doing it after the invalidate rect math was done seemed like the least invasive change.

[Alexandre Elias]

OK, seems reasonable.  In that case, I suggest moving the conversion into WebViewHost::paintRect instead.  Also, please use ceiling for the size scaling or you would miss a pixel for deviceScaleFactor = 1.5.

Created attachment 161052 [details]
Patch

(In reply to comment #8)
> OK, seems reasonable.  In that case, I suggest moving the conversion into WebViewHost::paintRect instead.  Also, please use ceiling for the size scaling or you would miss a pixel for deviceScaleFactor = 1.5.

Very good point, I added the ceil. Although, is it actually the case that it is used throughout the codebase? For example WebViewImpl seems to clamp when it scales the device viewport. I want to make sure I am not missing something and we are consistent.

[Alexandre Elias]

The one in WebViewImpl is unfinished code. It doesn't make much sense to infer deviceViewportSize from layoutSize anyway (there is no way to get it correct -- the inference should be in the other direction).

For invalidation paint rects, it's definitely better to err on the side of bigger invalidations.

[Alexandre Elias]

Comment on attachment 161052 [details]
Patch

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

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1844
> +    int scaledX = static_cast<int>(ceil(static_cast<float>(rect.x) * deviceScaleFactor));

Now you're losing a pixel in the other direction.  Please use a plain cast for the offset.

(In reply to comment #12)
> (From update of attachment 161052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161052&action=review
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1844
> > +    int scaledX = static_cast<int>(ceil(static_cast<float>(rect.x) * deviceScaleFactor));
> 
> Now you're losing a pixel in the other direction.  Please use a plain cast for the offset.

(In reply to comment #12)
> (From update of attachment 161052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161052&action=review
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1844
> > +    int scaledX = static_cast<int>(ceil(static_cast<float>(rect.x) * deviceScaleFactor));
> 
> Now you're losing a pixel in the other direction.  Please use a plain cast for the offset.

D'oh! Copy/paste, I'll address. thanks.

Created attachment 161217 [details]
Patch

[Alexandre Elias]

Looks good to me, thanks.  Someone with review power can r+.

[WebKit Review Bot]

Comment on attachment 161217 [details]
Patch

Clearing flags on attachment: 161217

Committed r127061: <http://trac.webkit.org/changeset/127061>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.