Bug 101769 – Use device scale factor instead of physical screen DPI for screen DPI

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sami Kyöstilä]

Use device scale factor instead of physical screen DPI for screen DPI

[Sami Kyöstilä]

Created attachment 173322 [details]
Patch

[Sami Kyöstilä]

Thanks Adam. I'll land this once all the prerequisites are in place.

[WebKit Review Bot]

Comment on attachment 173322 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 173322 [details]
Patch

Attachment 173322 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14785335

[John Mellor]

Comment on attachment 173322 [details]
Patch

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

Nice cleanup - looks good!

> Source/WebCore/page/Screen.cpp:NaN
>  unsigned Screen::verticalDPI() const

Screen::horizontalDPI and Screen::verticalDPI are now redundant. Could we remove one of them, and rename the other to Screen::cssDPI (to emphasize that it computes the CSS dpi unit, not the true physical dpi of the screen)?

> Source/WebCore/platform/blackberry/PlatformScreenBlackBerry.cpp:-32
> -    FloatSize currentPPI = BlackBerry::Platform::Graphics::Screen::primaryScreen()->pixelsPerInch(-1);

Seems like blackberry implemented this too. Might want to ping them on the review.

[Sami Kyöstilä]

(In reply to comment #5)
> Screen::horizontalDPI and Screen::verticalDPI are now redundant. Could we remove one of them, and rename the other to Screen::cssDPI (to emphasize that it computes the CSS dpi unit, not the true physical dpi of the screen)?

Good idea. Let me try to see how that works out.

> > Source/WebCore/platform/blackberry/PlatformScreenBlackBerry.cpp:-32
> > -    FloatSize currentPPI = BlackBerry::Platform::Graphics::Screen::primaryScreen()->pixelsPerInch(-1);
> 
> Seems like blackberry implemented this too. Might want to ping them on the review.

Antonio, any thoughts about this one?

[Jacky Jiang]

(In reply to comment #6)
> (In reply to comment #5)
> > > Source/WebCore/platform/blackberry/PlatformScreenBlackBerry.cpp:-32
> > > -    FloatSize currentPPI = BlackBerry::Platform::Graphics::Screen::primaryScreen()->pixelsPerInch(-1);
> > 
> > Seems like blackberry implemented this too. Might want to ping them on the review.
> 
> Antonio, any thoughts about this one?

Looks good to me.

[WebKit Review Bot]

Comment on attachment 173322 [details]
Patch

Rejecting attachment 173322 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
j.target/webkit/Source/WebKit/chromium/src/DatabaseObserver.o
Source/WebKit/chromium/src/ChromeClientImpl.cpp: In member function 'virtual void WebKit::ChromeClientImpl::dispatchViewportPropertiesDidChange(const WebCore::ViewportArguments&) const':
Source/WebKit/chromium/src/ChromeClientImpl.cpp:625: error: 'screenHorizontalDPI' was not declared in this scope
make: *** [out/Release/obj.target/webkit/Source/WebKit/chromium/src/ChromeClientImpl.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/14872336

[WebKit Review Bot]

Comment on attachment 173322 [details]
Patch

Clearing flags on attachment: 173322

Committed r135165: <http://trac.webkit.org/changeset/135165>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.