Bug 94935

Summary:

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

Product:

WebKit

Reporter:

alexst

Component:

WebKit Misc.

Assignee:

alexst

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aelias, dpranke, eric.carlson, feature-media-reviews, jamesr, tdanderson, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

90192

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

alexst

Reported 2012-08-24 07:04:31 PDT

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

Attachments
Patch (580.31 KB, patch) 2012-08-24 08:59 PDT, alexst	no flags	Details Formatted Diff Diff
Patch (580.12 KB, patch) 2012-08-28 13:55 PDT, alexst	no flags	Details Formatted Diff Diff
Patch (580.11 KB, patch) 2012-08-29 07:03 PDT, alexst	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

alexst

Comment 1 2012-08-24 08:59:35 PDT

Created attachment 160431 [details] Patch

Terry Anderson

Comment 2 2012-08-24 10:36:41 PDT

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?

alexst

Comment 3 2012-08-24 10:50:55 PDT

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

Comment 4 2012-08-24 10:59:52 PDT

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

Dirk Pranke

Comment 5 2012-08-28 11:16:21 PDT

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 6 2012-08-28 11:26:23 PDT

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?

alexst

Comment 7 2012-08-28 11:39:02 PDT

(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

Comment 8 2012-08-28 12:23:45 PDT

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.

alexst

Comment 9 2012-08-28 13:55:57 PDT

Created attachment 161052 [details] Patch

alexst

Comment 10 2012-08-28 14:26:33 PDT

(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

Comment 11 2012-08-28 14:43:04 PDT

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 12 2012-08-28 14:45:09 PDT

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.

alexst

Comment 13 2012-08-28 14:48:58 PDT

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

alexst

Comment 14 2012-08-28 14:51:31 PDT

alexst

Comment 15 2012-08-29 07:03:45 PDT

Created attachment 161217 [details] Patch

Alexandre Elias

Comment 16 2012-08-29 11:19:41 PDT

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

WebKit Review Bot

Comment 17 2012-08-29 15:58:54 PDT

Comment on attachment 161217 [details] Patch Clearing flags on attachment: 161217 Committed r127061: <http://trac.webkit.org/changeset/127061>

WebKit Review Bot

Comment 18 2012-08-29 15:58:57 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.