Closed Bug 1194751 Opened 9 years ago Closed 7 years ago

PScreenManager should not use sync messages

Categories

(Core :: DOM: Content Processes, defect, P3)

Product:
Core
Component:
DOM: Content Processes
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
e10s + ---
firefox55 --- fixed

People

(Reporter: mconley, Assigned: kanru)

References

(Depends on 2 open bugs, Blocks 3 open bugs, Regressed 1 open bug)

Details

Attachments

(9 files)

Bug 1194751 - Part 1. Remove nsIScreenManager::GetNumberOfScreens.
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1194751 - Part 2. Remove unused nsIScreen::LockMinimumBrightness and related methods.
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1194751 - Part 3. Remove unused nsIScreen::rotation attribute.
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.
59 bytes, text/x-review-board-request
jimm
: review+
mconley
: review+
karlt
: review+
Details
Bug 1194751 - Part 5. Implement ScreenHelperGTK and delete old nsScreenManagerGtk/nsScreenGtk.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1194751 - Part 6. Use mozilla::widget::ScreenManager in content process.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1194751 - Part 6.1 change nsScreenManagerAndroid::ScreenForId to a concrete method.
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1194751 - Part 7. Implement ScreenHelperWin and delete old nsScreenManagerWin/nsScreenWin.
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1194751 - Part 8. Implement ScreenHelperCocoa and delete old nsScreenManagerCocoa/nsScreenCocoa.
59 bytes, text/x-review-board-request
mstange
: review+
Details 
I've got a profile that shows some kind of Google ad querying for the screen dimensions on a timer:

http://people.mozilla.org/~bgirard/cleopatra/#report=5570a1eb67c4ef69a9e2d938476070e25ae05ce2&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A52967,%22end%22%3A53566%7D%5D&selection=0,4068,4069,4070,4071,4072,4073,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,1674,1675,1676,1677,474,32,33,34,35,41,42,4076,4432,4923,4480,4660,4661,4079,50,32,33,34,35,41,42,4076,4662,4663,4079,50,32,33,40,33,34,35,41,42,4076,4924,5081,4480,4672,5087,4480,4702,4927,4928,4704,4929,4454,1761,1568,4705,4706,4707,4708,4930,4931,4932,4933,3387,3388,3389,1961

This makes our ScreenProxy send sync messages up to the parent...every...time.

This is silly. I'm kind of at fault here, since I wrote ScreenProxy early on in my e10s work. Instead of querying the parent every time the child wants to know the screen dimensions, I think the child should cache this value, and the parent should alert the child when it moves to a screen with different dimensions.
Priority: -- → P3
For folks who eventually hack on this, here are (I think) ways of knowing when a window has changed screens:

OS X: https://developer.apple.com/reference/appkit/nswindowdidchangescreennotification?language=objc
Windows *: https://msdn.microsoft.com/en-us/library/windows/desktop/ms632631(v=vs.85).aspx
GTK *: http://zetcode.com/gui/gtk2/gtkevents/ (see "Moving a window")

* For both Windows and GTK, it doesn't look like any events or notifications get fired when a window moves between screens. There are, however, events for the movement of windows in general. We might be able to get what we need if we monitor window movement events, and then send updated screen dimensions if we notice that the movement caused the window to appear on a new screen.
I've seen this crash a11y on debug builds because of the sync calls interleaving with a11y reentry in bad ways. I might need to take this in order to fix the a11y stability problems. Flagging so that it stays on our radar.
Whiteboard: aes+
Whiteboard: aes+
This sounds like a really bad bug in certain cases, and a case which makes performance profiles less useful since they look like we're just waiting for some data from parent process.
Blocks: SyncIPC
I'm going to look at this tomorrow to see how hard this actually is.
Assignee: nobody → mconley
(In reply to Mike Conley (:mconley) from comment #1)
> * For both Windows and GTK, it doesn't look like any events or notifications
> get fired when a window moves between screens. There are, however, events
> for the movement of windows in general. We might be able to get what we need
> if we monitor window movement events, and then send updated screen
> dimensions if we notice that the movement caused the window to appear on a
> new screen.

On Windows, IIRC you should get a WM_DISPLAYCHANGE message when the resolution of the screen a window is on changes for any reason.
The sync messaging (and e10s window.screen support in general) was added in bug 1002354.
See Also: → 1002354
Well well well - it looks like a WindowMoved nsIWidgetListener method exists: http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/widget/nsIWidgetListener.h#68

I can probably just use that to detect if we've switched screens. Sad that I didn't catch that the first time. :/
So I _think_ it's more complicated than just having the TabChild cache the screen properties of the screen the TabParent is on (and invalidating that cache if the TabParent's frameloader moves to another screen). There are other things in the content process that use nsIScreenManager, and those need to continue to work too.

Here's what I suggest instead:

1) Get rid of the PScreenManager protocol entirely
2) Have each ContentChild singleton receive and store a cache of the properties of each screen that is available on the system. If the parent process notices that a new monitor has been added or a screen's dimensions have changed*, then that cache will need to be invalidated. The cache might not be held directly within ContentChild - nsScreenManagerProxy seems like the right place (though since it's no longer a proxy, we can probably rename this to ContentScreenManager or something).
3) With the cache in the ContentScreenManager, calls to nsIScreenManager screenForRect, primaryScreen, etc should be pretty straight forward - the ContentScreenManager will just reach into its cache and return the result that it has (and it will be periodically updated by the parent process if the size or number of displays changes).
4) Have each TabParent and TabChild remember which screenID (since each nsIScreen has a unique ID) they were most recently on. That way, if some web content accesses window.screen, it should be able to get the screen properties by asking the screen manager for the screen data for that TabChild's ID.
5) If a TabParent's frameloader is ever moved (see comment 9 - we have WindowMoved!), have it check to see if the screen ID is different from the one that TabParent already knows about. If so, send a message down to the TabChild to update its internal notion of which screen ID it belongs to.

I _think_ that'll do it.
Clearing myself as assignee. Apparently kanru might take a look at this?
Assignee: mconley → nobody
Flags: needinfo?(kchen)
you're right. Also thanks for the excellent summery!
Assignee: nobody → kchen
Flags: needinfo?(kchen)
I started to write patches and found that:

1. To pass details of all screens from parent to child, we need to enumerate all the screens and get updates of them. Currently we always query info about the current screen from the OS directly. Maybe we can use a generic cache class for both parent and child.

2. For windows, coca and gtk, we never remove a nsIScreen from the internal array of nsIScreenManager. So I'm wondering the relation between widget and screen. It seems a screen is always associated with a widget and the whole idea of nsIScreenManager is actually widget centric. For example to find ScreenForRect, it actually finds the screen which has a widget in it and has largest intersection of the rect. I'm not sure if I break this assumption will break consumers or not.

2. nsIScreenManager::GetNumberOfScreens is never used, so I removed that.

3. window.screen does not use nsIScreen directly, instead it uses DeviceContext to get the screen info. DeviceContext will use nsIScreenManager to get the screen for the current widget or tabchild.

4. We have two nsIScreenManager impl for PuppetWidget, one is ScreenManagerProxy, one is nsScreenManagerGonk. The latter one is not used anymore.
Status: NEW → ASSIGNED
Per https://docs.google.com/spreadsheets/d/1x_BWVlnQPg0DHbsrvPFX7g89lnFGa3lAIHWD_pLa_dE/edit#gid=738048355, this sync IPC is the second worst that we have after bug 1337062 (we can't do anything about Msg_LoadPlugin unfrotunately.)
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #13)
> 2. For windows, coca and gtk, we never remove a nsIScreen from the internal
> array of nsIScreenManager. So I'm wondering the relation between widget and
> screen. It seems a screen is always associated with a widget and the whole
> idea of nsIScreenManager is actually widget centric. For example to find
> ScreenForRect, it actually finds the screen which has a widget in it and has
> largest intersection of the rect. I'm not sure if I break this assumption
> will break consumers or not.

nsScreenManagerGtk does correctly update the screen info when there are updates. I'll update windows and coca to match that.
Whiteboard: [qf:p1]
Last try shows that toolkit/components/extensions/test/xpcshell/test_ext_background_window_properties.js test expect to get a zero sized screen.
Maybe we should return such screen when in xpcshell-test instead of return nullptr.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1fd15bebf9b5cd201e35c6abe2679d84ccaf7ec
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review118674

Holy smokes, this is some really nice code. I really love how you've broken this project up, it's very easy to reason about, and so much simpler than the old implementation!

1000 times yes, let's take this patch.

::: widget/ScreenManager.cpp:78
(Diff revision 1)
> +NS_IMETHODIMP
> +ScreenManager::ScreenForRect(int32_t aX, int32_t aY,
> +                             int32_t aWidth, int32_t aHeight,
> +                             nsIScreen** aOutScreen)
> +{
> +  MOZ_ASSERT(!mScreenList.IsEmpty(), "mScreenList was not properly initialized");

These will fail in debug builds, but in the wild, should we return NS_ERROR_FAILURE here if IsEmpty() or something?

Same goes for the other IsEmpty() asserts. Or am I being overcautious?
Attachment #8843217 - Flags: review?(mconley) → review+
Comment on attachment 8843219 [details]
Bug 1194751 - Part 6. Use mozilla::widget::ScreenManager in content process.

https://reviewboard.mozilla.org/r/117014/#review118676

This is great stuff. Thank you so much for ripping out all of my old, crappy code! This reads much better, and is so much simpler. A++ would review again. :)

::: dom/ipc/PContent.ipdl
(Diff revision 1)
> -    nested(inside_sync) sync PScreenManager()
> -        returns (float systemDefaultScale,
> -                 bool success);

Gooooooood riddance! :)

::: widget/Screen.h:16
(Diff revision 1)
>  
>  #include "Units.h"
>  
>  namespace mozilla {
> +namespace dom {
> +class ScreenDetails;

I guess ScreenDetails can't really live under mozilla::widget?

::: widget/ScreenManager.cpp:71
(Diff revision 1)
> +ScreenManager::CopyScreensToRemote(ContentParent* aContentParent)
> +{
> +  MOZ_ASSERT(aContentParent);
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +
> +  AutoTArray<ScreenDetails, 4> screens;
>    for (auto& screen : mScreenList) {
> -    uint32_t id;
> -    rv = screen->GetId(&id);
> -    if (NS_SUCCEEDED(rv) && id == aId) {
> -      RefPtr<Screen> ret = new Screen(*screen);
> -      ret.forget(aOutScreen);
> -      return NS_OK;
> +    screens.AppendElement(screen->ToScreenDetails());
> +  }
> +  MOZ_LOG(sScreenLog, LogLevel::Debug, ("Send screens to [Pid %d]", aContentParent->Pid()));
> +  if (!aContentParent->SendRefreshScreens(screens)) {
> +    MOZ_LOG(sScreenLog, LogLevel::Error,
> +            ("[Pid %d] SendRefreshScreens failed", aContentParent->Pid()));
> -    }
> +  }
> -  }
> +}
>  
> -  return NS_ERROR_FAILURE;
> +void
> +ScreenManager::CopyScreensToAllRemotesIfIsParent()
> +{
> +  if (XRE_IsContentProcess()) {
> +    return;
> +  }
> +
> +  MOZ_LOG(sScreenLog, LogLevel::Debug, ("Refreshing all ContentParents"));
> +
> +  AutoTArray<ScreenDetails, 4> screens;
> +  for (auto& screen : mScreenList) {
> +    screens.AppendElement(screen->ToScreenDetails());
> +  }
> +  for (auto* cp : ContentParent::AllProcesses(ContentParent::eLive)) {
> +    MOZ_LOG(sScreenLog, LogLevel::Debug, ("Send screens to [Pid %d]", cp->Pid()));
> +    if (!cp->SendRefreshScreens(screens)) {
> +      MOZ_LOG(sScreenLog, LogLevel::Error,
> +              ("SendRefreshScreens to [Pid %d] failed", cp->Pid()));
> +    }
> +  }

Feels like we could de-dupe some of the logic / logging here, and have a third, private helper function that takes a list of ContentParent's to send all ScreenDetails to. `CopyScreensToRemote` can pass just a single element Array with the one ContentParent, whereas `CopyScreensToAllRemotesIfIsParent` will pass the whole collection of ContentParent's.

Might help reduce some duplication here.
Attachment #8843219 - Flags: review?(mconley) → review+
Comment on attachment 8843215 [details]
Bug 1194751 - Part 2. Remove unused nsIScreen::LockMinimumBrightness and related methods.

https://reviewboard.mozilla.org/r/117006/#review118852
Attachment #8843215 - Flags: review?(snorp) → review+
Comment on attachment 8843220 [details]
Bug 1194751 - Part 6.1 change nsScreenManagerAndroid::ScreenForId to a concrete method.

https://reviewboard.mozilla.org/r/117016/#review118854
Attachment #8843220 - Flags: review?(snorp) → review+
Comment on attachment 8843218 [details]
Bug 1194751 - Part 5. Implement ScreenHelperGTK and delete old nsScreenManagerGtk/nsScreenGtk.

https://reviewboard.mozilla.org/r/117012/#review118970

::: widget/gtk/ScreenHelperGtk.h:21
(Diff revision 1)
> +#endif
> +
> +namespace mozilla {
> +namespace widget {
> +
> +class ScreenHelperGtk final : public ScreenManager::Helper

Please use "GTK" not "Gtk" in identifier and file names, as "GTK" is closer to the name of the toolkit.  "Gtk" in nsScreenGtk was a mistake that couldn't be corrected due to problems with version control systems and filenames on case-independent file systems.

I wonder about calling this ScreenWatcher to better describe what it does.

::: widget/gtk/ScreenHelperGtk.cpp:79
(Diff revision 1)
> +  : mXineramalib(nullptr)
> +  , mRootWindow(nullptr)
> +  , mNetWorkareaAtom(0)
> +{
> +  MOZ_LOG(sScreenLog, LogLevel::Debug, ("ScreenHelperGtk created"));
> +  mRootWindow = gdk_get_default_root_window();

Please hg copy from nsScreenManagerGtk.cpp to minimize changes in the diff and keep some of the history.

::: widget/gtk/ScreenHelperGtk.cpp:144
(Diff revision 1)
> +#endif
> +  return 1;
> +}
> +
> +static double
> +GetGtkDpiScale()

"Gtk" in the name here is not helpful, because this scale is not for use with GTK.

nsIScreen.defaultCSSScaleFactor is well documented, and so I suggest naming this function after that.

::: widget/gtk/nsAppShell.cpp:172
(Diff revision 1)
> +        RefPtr<ScreenManager> screenManager = ScreenManager::GetSingleton();
> +        screenManager->SetHelper(mozilla::MakeUnique<ScreenHelperGtk>());

I find it odd that the screen manager owns the helper, when it doesn't use the helper.  I suggest ScreenHelperGTK.cpp manage the ownership of the helper and nsAppShell::Init() merely trigger construction of the helper/watcher.
Attachment #8843218 - Flags: review?(karlt)
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review118992

::: widget/ScreenManager.h:33
(Diff revision 1)
> +
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSISCREENMANAGER
> +
> +  static already_AddRefed<ScreenManager> GetSingleton();

It seems odd to force the caller to manage references to a main-thread-only singleton.

I suggest making this return ScreenManager* without the refcount bump, and adding GetAddRefedSingleton() for the NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR special case.

::: widget/ScreenManager.cpp:38
(Diff revision 1)
> +ScreenManager::SetHelper(UniquePtr<Helper>&& aHelper)
> +{
> +  mHelper = Move(aHelper);

SetHelper(UniquePtr<Helper>) would clarify that the method always takes ownership from the argument.

https://groups.google.com/forum/#!msg/mozilla.dev.platform/VLtl2yL_TlA/2Xst3B8NCAAJ
https://groups.google.com/d/msg/mozilla.dev.platform/Ji3T5kipJzA/4GIk9BEtjsoJ

::: widget/ScreenManager.cpp:120
(Diff revision 1)
> +NS_IMETHODIMP
> +ScreenManager::GetPrimaryScreen(nsIScreen** aPrimaryScreen)
> +{
> +  MOZ_ASSERT(!mScreenList.IsEmpty(), "mScreenList was not properly initialized");
> +
> +  RefPtr<Screen> ret = new Screen(*mScreenList[0]);

Why is this returning a copy?

::: widget/ScreenManager.cpp:126
(Diff revision 1)
> +ScreenManager::GetSystemDefaultScale(float* aDefaultScale)
> +{
> +  *aDefaultScale = 1;

What is the plan for systems with non-unit scale?
Better to throw not-implemented, than return the wrong value.

::: widget/ScreenManager.cpp:144
(Diff revision 1)
> +  // one then just return the primary screen.
> +  if (mScreenList.Length() == 1) {
> +    return GetPrimaryScreen(aOutScreen);
> +  }
> +
> +  nsCOMPtr<nsIWidget> widget = static_cast<nsIWidget*>(aWidget);

This is not right, so please throw not-implemented and document in the commit message that uses will be replaced in part 6 (assuming that swapping the order of parts 5 and 6 does not keep a working browser).
Thank you for the helpful commit messages.  That makes review much easier.
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review118674

> These will fail in debug builds, but in the wild, should we return NS_ERROR_FAILURE here if IsEmpty() or something?
> 
> Same goes for the other IsEmpty() asserts. Or am I being overcautious?

Indeed. I fixed this in the latter try version.
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review118992

> Why is this returning a copy?

I intent to make Screen a pure data structure but XPCOM forces me to refcont it so I return a copy here. I guess return a reference will also work if nsIScreen has only readonly attributes (which it is after rotation is removed)

> What is the plan for systems with non-unit scale?
> Better to throw not-implemented, than return the wrong value.

My fault. Here we should call a helper method to get the scale but I forgot to do it.

> This is not right, so please throw not-implemented and document in the commit message that uses will be replaced in part 6 (assuming that swapping the order of parts 5 and 6 does not keep a working browser).

Yeah, I originally throw not-implemented but I thought it will be replaced anyway so make the code closer to the final version. If it looks confusing I can make it two steps.
Comment on attachment 8843218 [details]
Bug 1194751 - Part 5. Implement ScreenHelperGTK and delete old nsScreenManagerGtk/nsScreenGtk.

https://reviewboard.mozilla.org/r/117012/#review118970

> I find it odd that the screen manager owns the helper, when it doesn't use the helper.  I suggest ScreenHelperGTK.cpp manage the ownership of the helper and nsAppShell::Init() merely trigger construction of the helper/watcher.

I'd like to keep this. It's easier to reason about the lifetime since the helper's life time is tightly tied to the manager. It's really part of the manager so that's why I called it helper.

When one has two singleton that one uses the other, one must be very careful for the order of destruction especially when the helper can send events at anytime. With ClearOnShutdown the order is not very clear.
Comment on attachment 8843214 [details]
Bug 1194751 - Part 1. Remove nsIScreenManager::GetNumberOfScreens.

https://reviewboard.mozilla.org/r/117004/#review119272

::: widget/nsIScreenManager.idl:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  #include "nsIScreen.idl"
>  
> -[scriptable, uuid(e8a96e60-6b61-4a14-bacc-53891604b502)]
> +[scriptable, uuid(6a20c7e4-57ae-47cb-956e-19f7e9c305bd)]

nit - no need to rev these anymore.
Attachment #8843214 - Flags: review?(jmathies) → review+
Comment on attachment 8843216 [details]
Bug 1194751 - Part 3. Remove unused nsIScreen::rotation attribute.

https://reviewboard.mozilla.org/r/117008/#review119276
Attachment #8843216 - Flags: review?(jmathies) → review+
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review119282
Attachment #8843217 - Flags: review?(jmathies) → review+
Comment on attachment 8843221 [details]
Bug 1194751 - Part 7. Implement ScreenHelperWin and delete old nsScreenManagerWin/nsScreenWin.

https://reviewboard.mozilla.org/r/117018/#review119294
Attachment #8843221 - Flags: review?(jmathies) → review+
Blocks: 1300235
Comment on attachment 8843222 [details]
Bug 1194751 - Part 8. Implement ScreenHelperCocoa and delete old nsScreenManagerCocoa/nsScreenCocoa.

https://reviewboard.mozilla.org/r/117020/#review119678

I agree with karlt that the interaction of these objects is a little funny, but I think it's ok if it makes ownership and lifetime reasoning simpler.

Let me make sure I have this right: The manager owns a helper which has a subclass which owns a delegate. If the screen configuration changes, the delegate notifies the helper subclass which notifies the manager singleton. That seems fine.

::: widget/cocoa/ScreenHelperCocoa.mm:28
(Diff revision 1)
> +@end
> +
> +@implementation ScreenHelperDelegate
> +- (id)initWithScreenHelper:(mozilla::widget::ScreenHelperCocoa*)aScreenHelper
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;

You can leave out exception guards in objective C functions. I know we're not consistent about this, but it's worth doing in new code.

::: widget/cocoa/ScreenHelperCocoa.mm:95
(Diff revision 1)
> +}
> +
> +static already_AddRefed<Screen>
> +MakeScreen(NSScreen* aScreen)
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;

Please make this block a *_BLOCK_RETURN that returns nullptr when it gets an exception.

::: widget/cocoa/ScreenHelperCocoa.mm:133
(Diff revision 1)
> +  NSEnumerator* screenEnum = [[NSScreen screens] objectEnumerator];
> +
> +  while (NSScreen* screen = [screenEnum nextObject]) {

You can use "fast enumeration" these days, which I think would make this:

for (NSScreen* screen in [NSScreen screens]) { ... }

::: widget/cocoa/nsCocoaWindow.mm:1399
(Diff revision 1)
>  @end
>  
>  /* virtual */ bool
>  nsCocoaWindow::PrepareForFullscreenTransition(nsISupports** aData)
>  {
> -  nsCOMPtr<nsIScreen> widgetScreen = GetWidgetScreen();
> +  NSScreen* cocoaScreen = [NSScreen mainScreen];

I think this needs to find the screen that the window is on, not the main screen.
Attachment #8843222 - Flags: review?(mstange) → review+
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review118992

> It seems odd to force the caller to manage references to a main-thread-only singleton.
> 
> I suggest making this return ScreenManager* without the refcount bump, and adding GetAddRefedSingleton() for the NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR special case.

I can make this change. I'm not a big fan of using raw pointers though. Do we have a convention to mark a return value is not addrefed? I found some GetInstance/GetSingleton return addrefed and some not. It looks scary to me.
You could make sSingleton a UniquePtr and make GetSingleton() return a reference to the ScreenManager instead of a pointer ("return *sSingleton.get()").
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review120136

::: widget/ScreenManager.cpp:9
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "ScreenManager.h"
> +
> +#include "mozilla/ClearOnShutdown.h"

You're including ClearOnShutdown.h but not calling ClearOnShutdown anywhere.
Comment on attachment 8843219 [details]
Bug 1194751 - Part 6. Use mozilla::widget::ScreenManager in content process.

https://reviewboard.mozilla.org/r/117014/#review118676

> I guess ScreenDetails can't really live under mozilla::widget?

It can't because a weird bug in ipdl parser: if I put a WidgetTypes.ipdlh in widget/moz.build, the compiler complains PCompositorWidget is not managed by PCompositorBridge. I think it's because the ipdl parser doesn't handle having multiple PCompositorWidget.ipdl under widget/ well and adding widget/ to the ipdl includes dir triggers that. This should be fixed in a followup bug.
Sigh. The rebase was completely busted.
Comment on attachment 8843218 [details]
Bug 1194751 - Part 5. Implement ScreenHelperGTK and delete old nsScreenManagerGtk/nsScreenGtk.

https://reviewboard.mozilla.org/r/117012/#review118970

> I'd like to keep this. It's easier to reason about the lifetime since the helper's life time is tightly tied to the manager. It's really part of the manager so that's why I called it helper.
> 
> When one has two singleton that one uses the other, one must be very careful for the order of destruction especially when the helper can send events at anytime. With ClearOnShutdown the order is not very clear.

OK.  This makes more sense now that Helper::GetSystemDefaultScale() exists, anyway.
Comment on attachment 8843218 [details]
Bug 1194751 - Part 5. Implement ScreenHelperGTK and delete old nsScreenManagerGtk/nsScreenGtk.

https://reviewboard.mozilla.org/r/117012/#review121844

Good, thanks!

::: widget/gtk/ScreenHelperGTK.h
(Diff revision 2)
>  
>  #ifdef MOZ_X11
>    Atom NetWorkareaAtom() { return mNetWorkareaAtom; }
>  #endif
> -  
> -  // For internal use, or reinitialization from change notification.

Please keep this or a similar comment on RefreshScreens(), as this is only public for the sake of the internal signal callback functions.

::: widget/gtk/ScreenHelperGTK.cpp:255
(Diff revision 2)
> +  availRectDisplayPix = RoundedToInt(availRect / contentsScale);
> +  screen = new Screen(rect, availRect, rectDisplayPix, availRectDisplayPix,
> +                      pixelDepth, pixelDepth,
> +                      contentsScale, defaultCssScale);

I wonder whether the Screen constructor needs rects in both coordinates, given the conversion factor is provided.
Attachment #8843218 - Flags: review?(karlt) → review+
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review121890

::: widget/ScreenManager.cpp:107
(Diff revision 2)
> +  if (mScreenList.Length() == 1) {
> +    return GetPrimaryScreen(aOutScreen);
> +  }
> +
> +  // which screen should we return?
> +  RefPtr<Screen>& which = mScreenList[0];

|which| is a non-const reference to element 0 in mScreenList.

::: widget/ScreenManager.cpp:122
(Diff revision 2)
> +    // calculate the surface area
> +    DesktopIntRect screenRect(x, y, width, height);
> +    screenRect.IntersectRect(screenRect, windowRect);
> +    uint32_t tempArea = screenRect.width * screenRect.height;
> +    if (tempArea >= area) {
> +      which = screen;

So now the first element of mScreenList is another reference to the Screen with the largest intersection.  If that is not the same as the Screen that was previously referenced by element 0, then the Screen that was owned by mScreenList[0] may be released?
Attachment #8843217 - Flags: review?(karlt)
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review121890

> |which| is a non-const reference to element 0 in mScreenList.

Thanks! This should be a pointer to the element instead.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #62)
> Comment on attachment 8843217 [details]
> Bug 1194751 - Part 4. Add ScreenManager and Screen classes.
> 
> https://reviewboard.mozilla.org/r/117010/#review121890
> 
> > |which| is a non-const reference to element 0 in mScreenList.
> 
> Thanks! This should be a pointer to the element instead.

Do you want to review this part again after I fix this?
Flags: needinfo?(karlt)
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review118992

> I can make this change. I'm not a big fan of using raw pointers though. Do we have a convention to mark a return value is not addrefed? I found some GetInstance/GetSingleton return addrefed and some not. It looks scary to me.

If a return value is passing ownership of a reference, then it should use
already_AddRefed or RefPtr.  The reinterpret cast in
NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR indicates that this has not always
been the case.  I don't know why.

If a function is not passing ownership of a reference through the
return value, then a bare pointer is fine if the callee's class is keeping the
object alive, as is the case in singleton classes.  If the caller doesn't take
a reference, then it needs to ensure it doesn't do anything (e.g. dispatch
events to JS) that might cause the callee's class to release its reference,
but that's easy to avoid often enough.

(In reply to Markus Stange [:mstange] from comment #42)
> You could make sSingleton a UniquePtr and make GetSingleton() return a
> reference to the ScreenManager instead of a pointer ("return
> *sSingleton.get()").

I usually prefer to avoid mutable references because aliasing is all too easy
to overlook.  The style guide, however, only goes as far as excluding
references in out parameters, and I can't imagine any such problems with uses
of GetSingleton(), so I guess the reference is OK here.

> I intent to make Screen a pure data structure but XPCOM forces me to refcont it so I return a copy here. I guess return a reference will also work if nsIScreen has only readonly attributes (which it is after rotation is removed)

Previous implementations returned a reference, and so thank you for changing this.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #63)
> Do you want to review this part again after I fix this?

Someone should review the changes that have been made since review was
granted.  It doesn't need to be I who reviews the changes, but I'm happy to do
so.
Flags: needinfo?(karlt)
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

https://reviewboard.mozilla.org/r/117010/#review122340

(I just reviewed changes from revision 1 to 3, because the rest already has two r+.)

::: widget/Screen.cpp:22
(Diff revisions 1 - 3)
> -  , mRectDisplayPix(aRectDisplayPix)
> -  , mAvailRectDisplayPix(aAvailRectDisplayPix)
> +  , mRectDisplayPix(RoundedToInt(aRect / aContentsScale))
> +  , mAvailRectDisplayPix(RoundedToInt(aAvailRect / aContentsScale))

The rounding of corners to integers, and always rounding ties in the same direction is a little different to nsScreenWin rounding position with ties away from zero and rounding width and height instead of right and bottom.  Better get jfkthame to check this and the changes from what nsScreenCocoa does will be OK.  nsScreenCocoa rounding involves corners, but I don't know what assumptions can be made about the floating point NSScreen frame and backingScaleFactor values.
Attachment #8843217 - Flags: review?(karlt) → review+
Comment on attachment 8843217 [details]
Bug 1194751 - Part 4. Add ScreenManager and Screen classes.

Jonathan, can you check this re comment 72, please?
Attachment #8843217 - Flags: feedback?(jfkthame)
(In reply to Karl Tomlinson (:karlt) from comment #73)
> Comment on attachment 8843217 [details]
> Bug 1194751 - Part 4. Add ScreenManager and Screen classes.
> 
> Jonathan, can you check this re comment 72, please?

Ping?  Fixing this would make a *lot* of our sync IPCs go away which would be really nice.  Thanks!
IIUC, on any normal macOS configuration, aContentsScale will be either 1.0 or 2.0, and aRect and aAvailRect will have even-number coordinates and sizes, so we shouldn't be running into cases where different rounding conventions would make any practical difference here.

I'd suggest asking QA to specifically look out for any glitches on hi-dpi (and multi-display/mixed-dpi) systems, but I don't expect the rounding here to be a problem.
Attachment #8843217 - Flags: feedback?(jfkthame) → feedback+
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57f989c1010f
Part 1. Remove nsIScreenManager::GetNumberOfScreens. r=jimm
https://hg.mozilla.org/integration/autoland/rev/5df5fa8ee892
Part 2. Remove unused nsIScreen::LockMinimumBrightness and related methods. r=snorp
https://hg.mozilla.org/integration/autoland/rev/ace97d197b69
Part 3. Remove unused nsIScreen::rotation attribute. r=jimm
https://hg.mozilla.org/integration/autoland/rev/b18a43068dc6
Part 4. Add ScreenManager and Screen classes. r=jimm,karlt,mconley
https://hg.mozilla.org/integration/autoland/rev/4be4367d022d
Part 5. Implement ScreenHelperGTK and delete old nsScreenManagerGtk/nsScreenGtk. r=karlt
https://hg.mozilla.org/integration/autoland/rev/5a58b77947bf
Part 6. Use mozilla::widget::ScreenManager in content process. r=mconley
https://hg.mozilla.org/integration/autoland/rev/ad1425bed4ea
Part 6.1 change nsScreenManagerAndroid::ScreenForId to a concrete method. r=snorp
https://hg.mozilla.org/integration/autoland/rev/f4fa50b511ae
Part 7. Implement ScreenHelperWin and delete old nsScreenManagerWin/nsScreenWin. r=jimm
https://hg.mozilla.org/integration/autoland/rev/81099dbf284b
Part 8. Implement ScreenHelperCocoa and delete old nsScreenManagerCocoa/nsScreenCocoa. r=mstange
Depends on: 1351246
Depends on: 1351630
Depends on: 1351832
Depends on: 1352773
Depends on: 1401877
Depends on: 1440793
Depends on: 1475875
Performance Impact: --- → P1
Whiteboard: [qf:p1]
Regressions: 1807825
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: