Closed Bug 1350643 Opened 7 years ago Closed 7 years ago

Remove the PBrowser::Msg_GetDPI sync IPC

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: freesamael)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

Bug 1350643 - Part 1: Rename gfxPlatformGtk::GetDPI/GetDPIScale to GetFontScaleDPI/GetFontScaleFactor to better distinguish from nsIWidget::GetDPI.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1350643 - Part 2: Use gdk functions to enumerate monitors instead of Xinerama.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1350643 - Part 3: Add GetDPI to nsIScreen & ScreenDetails.
59 bytes, text/x-review-board-request
kanru
: review+
Details
Bug 1350643 - Part 4.1: Get per-monitor dpi in ScreenHelperCocoa.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1350643 - Part 5.1: Get per-monitor dpi in ScreenHelperGTK & use the same value in nsWindow::GetDPI.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1350643 - Part 6.2: Get per-monitor dpi in ScreenHelperWin & use the same value in nsWindow::GetDPI.
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1350643 - Part 7: Remove sync GetDPI/DefaultScale/WidgetRounding. Use primary screen's value until RecvShow.
59 bytes, text/x-review-board-request
kanru
: review+
Details
Bug 1350643 - Part 4.2: Use screen.dpi in nsChildView::GetDPI.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1350643 - Part 5.2: Use per-monitor gdkScaleFactor to set contentsScaleFactor & nsWindow::GetDesktopToDeviceScale if running in gtk/wayland.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1350643 - Part 6.1: Extract getting DPI logic from LogToPhysFactor & SystemScaleFactor to provide new functions MonitorDPI & SystemDPI.
59 bytes, text/x-review-board-request
jimm
: review+
Details 
This one is just stupid, with a median of 23.84ms to deliver a constant float!

This is probably very similar to bug 1347035.  I can take a look at it once that one is done...
Priority: -- → P1
Maybe kats has a few spare minutes to help here? (based on blame and the fact that he's an all-around great person :)
Flags: needinfo?(bugmail)
Are you seeing this get hit a lot? From when I fixed bug 1331988 my understanding was that the GetDPI IPC message should not actually be getting used under normal circumstances (for some definition of "normal"). At the moment I don't have much time to look into this, I already have bug 1350638 on my plate to help with the sync-IPC-removal work.
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Are you seeing this get hit a lot?
Flags: needinfo?(ehsan)
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
(In reply to Andrew Overholt [:overholt] from comment #3)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> > Are you seeing this get hit a lot?

Samael would help this out after bug 1337064 and bug 1347823, starting from looking at the hit again. Thanks Samael!
I'm trying to catch up; My understanding is that with kats's patch mentioned in comment 2, the sync GetDPI call should only happen if ContentChild::ProvideWindowCommon was invoked without aParent - in that case TabChild::DoFakeShow is called but mDPI is not set yet until RecvShow. One example I can think of is ServiceWorker::OpenWindow.

Telemetry shows ~50k samples of Msg_GetDPI in last 7 days on nightly 55; In comparison there were ~1.2M of Msg_CreateWindow.

It seems we can apply the same approach of bug 1347035 - to piggyback DPI info with Msg_WindowCreated introduced in bug 1343728, and Msg_BrowserFrameOpenWindow for the <iframe mozbrowser> case.
Assignee: nobody → sawang
Here's an example profile: https://perfht.ml/2qwrKl0

It has a stack for the sync PBrowser::Msg_GetDPI call, and a bonus one for a sync PBrowser::Msg_GetDefaultScale call.

Steps to reproduce:
 1. Open two tabs with two different sites, in such a way that these two tabs could be hosted in two different content processes.
 2. Go to about:preferences and change your "When Nightly starts" pref to "Show your windows and tabs from last time".
 3. Restart your browser.
 4. Wait for startup to complete, and then select the other tab.

Selecting the other tab will create a content process to load that tab. During the load, the content process will send the sync GetDefaultScale message.
(In reply to Markus Stange [:mstange] from comment #6)
> Here's an example profile: https://perfht.ml/2qwrKl0
> 
> It has a stack for the sync PBrowser::Msg_GetDPI call, and a bonus one for a
> sync PBrowser::Msg_GetDefaultScale call.
> 
> Steps to reproduce:
>  1. Open two tabs with two different sites, in such a way that these two
> tabs could be hosted in two different content processes.
>  2. Go to about:preferences and change your "When Nightly starts" pref to
> "Show your windows and tabs from last time".
>  3. Restart your browser.
>  4. Wait for startup to complete, and then select the other tab.
> 
> Selecting the other tab will create a content process to load that tab.
> During the load, the content process will send the sync GetDefaultScale
> message.

Thank you for providing the info! I thought loading initiated from parent process wouldn't encouter the case but clearly I was wrong. 

I haven't dig it deeply but I can reproduce it locally and have some finding; Originally I thought TabParent::LoadURL would be able to send DPI info to child, but in this case the XULElement's SetPrimaryFrame happens after TabParent::LoadURL, so it couldn't get correct show info. I need to understand these stuff better before judging what I should do here.
Kanru hinted me at ScreenManager. It looks we can collect the dpi of each screen into nsIScreen, and make PuppetWidget::GetDPI just use nsIScreen.dpi of the screen it belongs to. I'm currently working on the gtk implementation of this approach as a start.

I believe we can take the same approach on PuppetWidget::GetDefaultScaleInternal to eliminate PBrowser::GetDefaultScale, although adding another scale factor to nsIScrenn could make it more confusing...
Comment on attachment 8870689 [details]
Bug 1350643 - Part 3: Add GetDPI to nsIScreen & ScreenDetails.

I was thinking of making PuppetWidget gets DPI from screen manager; then I realize it needs correct dimension & scale factor to find the screen it belongs to, which happens at the same time it gets DPI from the parent, so that doesn't solve the problem. 

I turned back and looked at TabChild::GetDPI - this could just be terribly wrong, but if we were able to use DPI=-1 until RecvShow, I feel that we're almost able to just remove the sync protocol directly. So I think maybe we could use the primary screen's value until we get precious DPI, which might be better in some cases. The problem is I don't really know what that implies to our layout system, and I'm not sure if that would cause unexpected behaviors.

Since I'm super unconfident about what I'm doing, maybe we should start by looking at part 3 for feedbacks & suggestions?
Attachment #8870689 - Flags: feedback?(mconley)
Comment on attachment 8870689 [details]
Bug 1350643 - Part 3: Add GetDPI to nsIScreen & ScreenDetails.

I'm going to redirect to kanru, who I suspect has more of this stuff swapped into his head.
Attachment #8870689 - Flags: feedback?(mconley) → feedback?(kchen)
Comment on attachment 8870689 [details]
Bug 1350643 - Part 3: Add GetDPI to nsIScreen & ScreenDetails.

https://reviewboard.mozilla.org/r/142160/#review146524

::: widget/PuppetWidget.cpp:1205
(Diff revision 1)
> -  if (mDefaultScale < 0) {
> -    if (mTabChild) {
> -      mTabChild->GetDefaultScale(&mDefaultScale);
> -    } else {
> -      mDefaultScale = 1;
> +  if (mDefaultScale <= 0) {
> +    // Let's try to use primaryScreen's scale as a fallback.
> +    // mDefaultScale will be updated on UpdateBackingScaleCache later.
> +    nsCOMPtr<nsIScreenManager> screenManager =
> +      do_GetService("@mozilla.org/gfx/screenmanager;1");
> +    nsCOMPtr<nsIScreen> primaryScreen;
> +    screenManager->GetPrimaryScreen(getter_AddRefs(primaryScreen));
> +    primaryScreen->GetDefaultCSSScaleFactor(&mDefaultScale);

I would avoid using screenManager in widget code because screenManager might be implemented on top of widget!

But PuppetWidget is a weird thing so this is probably fine. Just need to be careful about the layering.
Just some further notes. My understanding is that there are 2 cases where sync GetDPI would be called:

1. If GetDPI is called after DoFakeShow but before RecvShow. One example is ServiceWorker::OpenWindow. 
I _think_ this case will be elimiated in bug 1343728; the patch shows DoFakeShow will be in RecvWindowCreated. Since in the parent side CommonCreateWindow calls OpenURIInFrame which should trigger a SendShow before SendWindowCreated, in the child RecvShow should happen before DoFakeShow, hopefully.

2. If GetDPI is called after RecvShow, but parent wasn't able to get widget in that SendShow IPC call - in this case ShowInfo::dpi() would be 0.
I don't know the reason, but comment 6 is an example. In that case TabParent::mFrameElement was set, but mFrameElement->mPrimaryFrame was still a nullptr. There's a related warning message in this case [1]. 

[1] http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/dom/base/nsFrameLoader.cpp#785
Attachment #8870689 - Flags: feedback?(kchen)
Some updates:

> 1. If GetDPI is called after DoFakeShow but before RecvShow. One example is
> ServiceWorker::OpenWindow. 
> I _think_ this case will be elimiated in bug 1343728; the patch shows
> DoFakeShow will be in RecvWindowCreated. Since in the parent side
> CommonCreateWindow calls OpenURIInFrame which should trigger a SendShow
> before SendWindowCreated, in the child RecvShow should happen before
> DoFakeShow, hopefully.

I'm seeing lots of NS_ASSERTIONS running dom/workers/test/serviceworkers/test_openWindow.html [1]; I noticed that these windows opened by service work never get RecvShow with fakeShowInfo=false, so it was fully relying on SendGetDPI before. 

I think it implies nsFrameLoader::mRemoteBrowserShown were never set to true in this case, so I'm trying to make an extra ShowRemoteFrame call in frameloader if !mRemoteBrowserShown when UpdatePositionAndSize called. Not sure if it's the right approach.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=104500625&repo=try&lineNumber=10111

> 2. If GetDPI is called after RecvShow, but parent wasn't able to get widget
> in that SendShow IPC call - in this case ShowInfo::dpi() would be 0.
> I don't know the reason, but comment 6 is an example. In that case
> TabParent::mFrameElement was set, but mFrameElement->mPrimaryFrame was still
> a nullptr.

Now I understand that the case in comment 6 is caused by that the lazy browser was inserted to the document lately.
Oh and a fun fact: for some reason `gdk_screen_get_height_mm` can return a fake value. On my laptop, which of course has only one monitor, calling `gdk_screen_get_height_mm(screen)` & `gdk_screen_get_monitor_height_mm(screen, 0)` return different values; the later seems to be the real one, while the former feels like a value calculated by 96dpi.
Comment on attachment 8875102 [details]
Bug 1350643 - Part 4.1: Get per-monitor dpi in ScreenHelperCocoa.

https://reviewboard.mozilla.org/r/146488/#review150470
Attachment #8875102 - Flags: review?(mstange) → review+
Comment on attachment 8870687 [details]
Bug 1350643 - Part 1: Rename gfxPlatformGtk::GetDPI/GetDPIScale to GetFontScaleDPI/GetFontScaleFactor to better distinguish from nsIWidget::GetDPI.

https://reviewboard.mozilla.org/r/142156/#review150476

::: widget/gtk/nsWindow.h:107
(Diff revision 2)
>      virtual double     GetDefaultScaleInternal() override;
> -    // Under Gtk, we manage windows using device pixels so no scaling is needed:
>      mozilla::DesktopToLayoutDeviceScale GetDesktopToDeviceScale() final
>      {
> -        return mozilla::DesktopToLayoutDeviceScale(1.0);
> +        // We use GDK's application pixels as our desktop pixels.
> +        return mozilla::DesktopToLayoutDeviceScale(GdkScaleFactor());

This isn't a necessary change, but it looks to me that GDK's application pixels should fit gecko's desktop pixels concept, and that could also make nsIScreen.contentsScaleFactor match the value of gdk_screen_get_monitor_scale_factor.

However if this looks incorrect or too risky I can revert this change.
Comment on attachment 8870689 [details]
Bug 1350643 - Part 3: Add GetDPI to nsIScreen & ScreenDetails.

https://reviewboard.mozilla.org/r/142160/#review151150
Attachment #8870689 - Flags: review?(kchen) → review+
Comment on attachment 8875105 [details]
Bug 1350643 - Part 7: Remove sync GetDPI/DefaultScale/WidgetRounding. Use primary screen's value until RecvShow.

https://reviewboard.mozilla.org/r/146494/#review151152
Attachment #8875105 - Flags: review?(kchen) → review+
Comment on attachment 8875103 [details]
Bug 1350643 - Part 5.1: Get per-monitor dpi in ScreenHelperGTK & use the same value in nsWindow::GetDPI.

https://reviewboard.mozilla.org/r/146490/#review151592
Attachment #8875103 - Flags: review?(karlt) → review+
Comment on attachment 8870687 [details]
Bug 1350643 - Part 1: Rename gfxPlatformGtk::GetDPI/GetDPIScale to GetFontScaleDPI/GetFontScaleFactor to better distinguish from nsIWidget::GetDPI.

https://reviewboard.mozilla.org/r/142156/#review150536

The "GetFontScale" name is good, thanks.

::: commit-message-c3e54:3
(Diff revision 2)
> +1. Make gtk/nsWindow::GetDPI be mulit-monitor aware.
> +2. Rename gfxPlatformGtk::GetDPI / GetDPIScale to GetFontScaleDPI /
> +   GetFontScaleFactor to better distinguish from nsIWidget::GetDPI.
> +3. We used to manage GTK windows in device pixels; change it to use GDK's
> +   application pixels to better match the concept of desktop pixels in gecko.

These are separate changes that don't need to be in one patch, and so please break them up into separate patches.

::: widget/gtk/nsWindow.h
(Diff revision 2)
> -    // Under Gtk, we manage windows using device pixels so no scaling is needed:
>      mozilla::DesktopToLayoutDeviceScale GetDesktopToDeviceScale() final
>      {
> -        return mozilla::DesktopToLayoutDeviceScale(1.0);
> +        // We use GDK's application pixels as our desktop pixels.
> +        return mozilla::DesktopToLayoutDeviceScale(GdkScaleFactor());

This is probably correct for Wayland IIUC.  For X11, desktop pixels are device
pixels, but using GTK pixels would work fine too because the same scale factor
is used on all monitors.

There is code, in ScreenHelperGTK at least, that assumes desktop pixels are
device pixels.  If you make this change, then please include, in the same
patch, the relevant change to contentsScale in MakeScreen().

I don't know whether or not there are similar assumptions elsewhere.  That
makes this a risky change, and may jeopardize successful landing of the other
parts of these patches.  If you have tested opening popup windows in various
corners of multiple monitors with GDK_SCALE=2 in the environment, then I'm
fine if you'd like to change GetDesktopToDeviceScale and contentsScale in a
single separate patch.

::: widget/gtk/nsWindow.cpp:808
(Diff revision 2)
> -    double heightInches = gdk_screen_get_height_mm(screen)/MM_PER_INCH_FLOAT;
> +    double heightInches;
> +    if (mGdkWindow) {
> +        gint monitor = gdk_screen_get_monitor_at_window(screen, mGdkWindow);
> +        heightInches =
> +            gdk_screen_get_monitor_height_mm(screen, monitor)/MM_PER_INCH_FLOAT;
> +    } else {
> +        heightInches = gdk_screen_get_height_mm(screen)/MM_PER_INCH_FLOAT;
> +    }

gdk_screen_get_monitor_at_window() is implemented with multiple synchronous
round trips to the X server to get the window geometry,
which is inefficient and similar to the kind of
bug this report is about.

The simplest solution is to use GetWidgetScreen(), given the screen will have
the physical dpi.
Attachment #8870687 - Flags: review?(karlt) → review-
Comment on attachment 8870688 [details]
Bug 1350643 - Part 2: Use gdk functions to enumerate monitors instead of Xinerama.

https://reviewboard.mozilla.org/r/142158/#review151586

::: widget/gtk/ScreenHelperGTK.h
(Diff revision 2)
> -#ifdef MOZ_X11
> -#include <X11/Xlib.h>
> -#endif

Keep this while |Atom| is still used in this file.

(It looks like GDK does not dispatch monitors-changed is not dispatched on changes to workarea, and so we need to keep mNetWorkareaAtom.)

::: widget/gtk/ScreenHelperGTK.cpp:10
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "ScreenHelperGTK.h"
>  
>  #ifdef MOZ_X11
>  #include <X11/Xatom.h>

X11/Xatom.h is no longer used, now that XA_CARDINAL is not used.

::: widget/gtk/ScreenHelperGTK.cpp:151
(Diff revision 2)
> +  LayoutDeviceIntRect availRect(workarea.x * gdkScaleFactor,
> +                                workarea.y * gdkScaleFactor,
> +                                workarea.width * gdkScaleFactor,
> +                                workarea.height * gdkScaleFactor);
>    uint32_t pixelDepth = GetGTKPixelDepth();
> -  DesktopToLayoutDeviceScale contentsScale(1.0);
> +  DesktopToLayoutDeviceScale contentsScale(gdkScaleFactor);

Keep this at 1.0, if you decide against the change to GetDesktopToDeviceScale().  If you decide to keep the desktop pixel change, then just drop this issue.
Attachment #8870688 - Flags: review?(karlt) → review+
Comment on attachment 8875104 [details]
Bug 1350643 - Part 6.2: Get per-monitor dpi in ScreenHelperWin & use the same value in nsWindow::GetDPI.

https://reviewboard.mozilla.org/r/146492/#review151806

::: widget/windows/ScreenHelperWin.cpp:54
(Diff revision 1)
>      pixelDepth = 24;
>    }
>    float dpi = 96.0f;
> +  int heightMM = ::GetDeviceCaps(hDCScreen, VERTSIZE);
> +  if (heightMM > 0) {
> +    dpi = rect.height / (heightMM / MM_PER_INCH_FLOAT);

VERTSIZE is only valid for printing hdcs, you won't get valid data from it from monitors. I think what you want here is the logical DPI (per monitor). On Windows 8.1 and up there's an api for this [1] which you can dynamically grab a pointer to and call when present. On Win7, you can grab  this from GetDeviceCaps(hdc, LOGPIXELSX).

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dn280510(v=vs.85).aspx
Attachment #8875104 - Flags: review?(jmathies) → review-
sample app:

#include "stdafx.h"
#include <Windows.h>

#define MM_PER_INCH_FLOAT 25.4f

int main()
{ 
  SetProcessDPIAware();
  HDC hdc = GetDC(GetDesktopWindow());
  {
    MONITORINFO info; 
    info.cbSize = sizeof(MONITORINFO);
    POINT pt = {10, 10};
    HMONITOR hMon = MonitorFromPoint(pt, 0);
    GetMonitorInfo(hMon, &info);
    RECT rect = {info.rcMonitor.left, info.rcMonitor.top, info.rcMonitor.right,  info.rcMonitor.bottom};
    int logHeight = rect.bottom - rect.top;
    int heightMM = GetDeviceCaps(hdc, VERTSIZE);
    float dpi = (logHeight / (heightMM / MM_PER_INCH_FLOAT));
    printf("#1 dpi=%f\n", dpi);
  }

  {
    int dpiX = GetDeviceCaps(hdc, LOGPIXELSX);
    printf("#2 dpi=%d\n", dpiX);
  }

  ReleaseDC(GetDesktopWindow(), hdc);
  return 0;
}
Comment on attachment 8878972 [details]
Bug 1350643 - Part 4.2: Use screen.dpi in nsChildView::GetDPI.

https://reviewboard.mozilla.org/r/150220/#review154836

::: widget/cocoa/nsChildView.mm:827
(Diff revision 1)
>  float
>  nsChildView::GetDPI()
>  {
> -  NSWindow* window = [mView window];
> -  if (window && [window isKindOfClass:[BaseWindow class]]) {
> -    return [(BaseWindow*)window getDPI];
> +  float dpi = 96.0;
> +  nsCOMPtr<nsIScreen> screen(GetWidgetScreen());
> +  if(screen) {

space between if and (
Attachment #8878972 - Flags: review?(mstange) → review+
Comment on attachment 8870687 [details]
Bug 1350643 - Part 1: Rename gfxPlatformGtk::GetDPI/GetDPIScale to GetFontScaleDPI/GetFontScaleFactor to better distinguish from nsIWidget::GetDPI.

https://reviewboard.mozilla.org/r/142156/#review150536

> These are separate changes that don't need to be in one patch, and so please break them up into separate patches.

Part 1 now only contains the change for renaming gfxPlatformGtk::GetDPI/GetDPIScale to GetFontScaleDPI/GetFontScaleFactor.

> This is probably correct for Wayland IIUC.  For X11, desktop pixels are device
> pixels, but using GTK pixels would work fine too because the same scale factor
> is used on all monitors.
> 
> There is code, in ScreenHelperGTK at least, that assumes desktop pixels are
> device pixels.  If you make this change, then please include, in the same
> patch, the relevant change to contentsScale in MakeScreen().
> 
> I don't know whether or not there are similar assumptions elsewhere.  That
> makes this a risky change, and may jeopardize successful landing of the other
> parts of these patches.  If you have tested opening popup windows in various
> corners of multiple monitors with GDK_SCALE=2 in the environment, then I'm
> fine if you'd like to change GetDesktopToDeviceScale and contentsScale in a
> single separate patch.

I decided to keep 1.0 in gtk/11, and use gdkScaleFactor in gtk/wayland (in Part 5.2). I couldn't make meanful verification on wayland, however. I tried rebase patches on top of bug 635134 comment 55 and run it in a nested wayland compositor, but the coordinate never change when I move the window.

> gdk_screen_get_monitor_at_window() is implemented with multiple synchronous
> round trips to the X server to get the window geometry,
> which is inefficient and similar to the kind of
> bug this report is about.
> 
> The simplest solution is to use GetWidgetScreen(), given the screen will have
> the physical dpi.

This is merged to Part 5.1.
Comment on attachment 8878973 [details]
Bug 1350643 - Part 5.2: Use per-monitor gdkScaleFactor to set contentsScaleFactor & nsWindow::GetDesktopToDeviceScale if running in gtk/wayland.

https://reviewboard.mozilla.org/r/150222/#review154850

::: commit-message-6196d:2
(Diff revision 1)
> +Bug 1350643 - Part 5.3: Use per-monitor gdkScaleFactor to set contentsScaleFactor & nsWindow::GetDesktopToDeviceScale if running in gtk/wayland. r?karlt
> +

Should be Part 5.2, I'll change the commit message before landing...
Comment on attachment 8870687 [details]
Bug 1350643 - Part 1: Rename gfxPlatformGtk::GetDPI/GetDPIScale to GetFontScaleDPI/GetFontScaleFactor to better distinguish from nsIWidget::GetDPI.

https://reviewboard.mozilla.org/r/142156/#review155432
Attachment #8870687 - Flags: review?(karlt) → review+
Comment on attachment 8875103 [details]
Bug 1350643 - Part 5.1: Get per-monitor dpi in ScreenHelperGTK & use the same value in nsWindow::GetDPI.

https://reviewboard.mozilla.org/r/146490/#review155434

Thank you for using GetWidgetScreen().

::: widget/gtk/nsWindow.cpp:808
(Diff revision 2)
>  
>  float
>  nsWindow::GetDPI()
>  {
> -    GdkScreen *screen = gdk_display_get_default_screen(gdk_display_get_default());
> -    double heightInches = gdk_screen_get_height_mm(screen)/MM_PER_INCH_FLOAT;
> +    float dpi = 96.0f;
> +    nsCOMPtr<nsIScreen> screen(GetWidgetScreen());

Initialize with '=', where that works, and I assume that works here.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices
Comment on attachment 8878973 [details]
Bug 1350643 - Part 5.2: Use per-monitor gdkScaleFactor to set contentsScaleFactor & nsWindow::GetDesktopToDeviceScale if running in gtk/wayland.

https://reviewboard.mozilla.org/r/150222/#review155436
Attachment #8878973 - Flags: review?(karlt) → review+
Comment on attachment 8875104 [details]
Bug 1350643 - Part 6.2: Get per-monitor dpi in ScreenHelperWin & use the same value in nsWindow::GetDPI.

https://reviewboard.mozilla.org/r/146492/#review155694

::: widget/windows/nsWindow.cpp:1306
(Diff revision 2)
>  }
>  
>  float nsWindow::GetDPI()
>  {
> -  HDC dc = ::GetDC(mWnd);
> -  if (!dc)
> +  float dpi = 96.0f;
> +  nsCOMPtr<nsIScreen> screen(GetWidgetScreen());

Can you check to see how often this gets called? GetWidgetScreen() looks expensive. Please make sure you're not introducing a performance regression here. (A push to try for some talos runs would be good.)

http://searchfox.org/mozilla-central/source/widget/nsBaseWidget.cpp#1891
Attachment #8875104 - Flags: review?(jmathies) → review+
Comment on attachment 8878974 [details]
Bug 1350643 - Part 6.1: Extract getting DPI logic from LogToPhysFactor & SystemScaleFactor to provide new functions MonitorDPI & SystemDPI.

https://reviewboard.mozilla.org/r/150224/#review155700
Attachment #8878974 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #51)
> Can you check to see how often this gets called? GetWidgetScreen() looks
> expensive. Please make sure you're not introducing a performance regression
> here. (A push to try for some talos runs would be good.)

Comparison of try pushes before / after patch shows there might be regression. I'm looking into it. 

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ff0d22ee3809&newProject=try&newRevision=2ed8f834ef19&framework=1
Comment on attachment 8878974 [details]
Bug 1350643 - Part 6.1: Extract getting DPI logic from LogToPhysFactor & SystemScaleFactor to provide new functions MonitorDPI & SystemDPI.

https://reviewboard.mozilla.org/r/150224/#review163278

::: widget/windows/WinUtils.cpp:561
(Diff revision 2)
>    // per-monitor DPI and support for on-the-fly resolution changes.
>    // Therefore, we only need to look it up once.
> -  static double systemScale = 0;
> -  if (systemScale == 0) {
> +  static float dpi = 0;
> +  if (dpi <= 0) {
>      HDC screenDC = GetDC(nullptr);
> -    systemScale = GetDeviceCaps(screenDC, LOGPIXELSY) / 96.0;
> +    int dpi = GetDeviceCaps(screenDC, LOGPIXELSY);

Couldn't believe I made such a silly mistake but anyway this looks to be the root cause. Making another talos run to verify.
The base has been too far from current trunk so I'll manually rebase the patches.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/caf2bbdc4100
Part 1: Rename gfxPlatformGtk::GetDPI/GetDPIScale to GetFontScaleDPI/GetFontScaleFactor to better distinguish from nsIWidget::GetDPI. r=karlt
https://hg.mozilla.org/integration/autoland/rev/dce3ec16f2b5
Part 2: Use gdk functions to enumerate monitors instead of Xinerama. r=karlt
https://hg.mozilla.org/integration/autoland/rev/abfd88baf391
Part 3: Add GetDPI to nsIScreen & ScreenDetails. r=kanru
https://hg.mozilla.org/integration/autoland/rev/ea47118382b9
Part 4.1: Get per-monitor dpi in ScreenHelperCocoa. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d1529cb8f16c
Part 4.2: Use screen.dpi in nsChildView::GetDPI. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8e85fca9fd90
Part 5.1: Get per-monitor dpi in ScreenHelperGTK & use the same value in nsWindow::GetDPI. r=karlt
https://hg.mozilla.org/integration/autoland/rev/070ca389c435
Part 5.2: Use per-monitor gdkScaleFactor to set contentsScaleFactor & nsWindow::GetDesktopToDeviceScale if running in gtk/wayland. r=karlt
https://hg.mozilla.org/integration/autoland/rev/bb0663673b11
Part 6.1: Extract getting DPI logic from LogToPhysFactor & SystemScaleFactor to provide new functions MonitorDPI & SystemDPI. r=jimm
https://hg.mozilla.org/integration/autoland/rev/9fe134e61ce1
Part 6.2: Get per-monitor dpi in ScreenHelperWin & use the same value in nsWindow::GetDPI. r=jimm
https://hg.mozilla.org/integration/autoland/rev/b43d89d13360
Part 7: Remove sync GetDPI/DefaultScale/WidgetRounding. Use primary screen's value until RecvShow. r=kanru
Keywords: checkin-needed
Depends on: 1383879
No longer depends on: 1383879
Flags: needinfo?(ehsan)
Component: DOM → DOM: Core & HTML
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: