WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
94935
[chromium] High-DIP pixel test output should be the correct size and also work with --force-compositing-mode
https://bugs.webkit.org/show_bug.cgi?id=94935
Summary
[chromium] High-DIP pixel test output should be the correct size and also wor...
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
(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.
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.
Top of Page
Format For Printing
XML
Clone This Bug