1002354 - [e10s] DOM Screen object returns bogus values in content process on Retina display

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. In an e10s window, load https://mail.mozilla.com/
2. Change Version from "Default" to "Advanced (AJAX)".

RESULT:
Zimbra shows the following error message:

"Note that your web browser or display does not fully support the Advanced version. We strongly recommend that you use the Standard client."

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Yoink

Comment 2

[Mike Conley (:mconley) (:⚙️)]

So this has nothing to do with supporting XHR, and everything to do with the window.screen global.

Here's the code for the Zimbra login page:

https://pastebin.mozilla.org/5419770

Lines 341-347 are the most relevant:

// show a message if they should be using the 'standard' client, but have chosen 'advanced' instead
function clientChange(selectValue) {
  var useStandard = false;
  useStandard = useStandard || (screen && (screen.width <= 800 && screen.height <= 600));
  var div = document.getElementById("ZLoginUnsupported");
  if (div)
    div.style.display = ((selectValue == 'advanced') && useStandard) ? 'block' : 'none';
}


So this code simply checks the available space on the screen, and uses that to determine whether or not to show the warning.

cpeterson has a 13" MBP with a Retina display. With a non e10s window, window.screen on this page reports the following values:

Screen {
  availWidth: 1440,
  availHeight: 874,
  width: 1440,
  height: 900,
  colorDepth: 24,
  pixelDepth: 24,
  top: 0,
  left: 0,
  availTop: 22,
  availLeft: 0
}

For the e10s window case, window.screen reports:

Screen {
  availWidth: 720,
  availHeight: 437,
  width: 720,
  height: 450,
  colorDepth: 24,
  pixelDepth: 24,
  top: 0,
  left: 0,
  availTop: 11,
  availLeft: 0
}

These are all exactly half of the values of the non-e10s version, so we're missing a multiplication by the scaling factor for hidpi somewhere when calculating the values to report for screen.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

That's interesting. I've seen Ally.com redirect to m.ally.com in e10s and I bet this is why.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

So I've been digging at this a bit, and here are my findings:

There are, I think, several bugs here, but they all center on the fact that the content process really doesn't have good access to information about the screen.

When getting the DOM Screen object, we end up here in both the chrome and content processes:

http://hg.mozilla.org/mozilla-central/file/bb35d1b73634/gfx/src/nsDeviceContext.cpp#l631

In the non-e10s case, we're able to successfully get the screen based on the widget via GetNativeData, and we return the proper screen to interrogate for information for the DOM Screen object. This works well.

In the e10s case, things go a bit wrong. First of all, the widget in this case is the PuppetWidget, and the PuppetWidget has no ability to get a handle on the native window with GetNativeData (even if we passed the NSScreen handle down from the parent process via NS_NATIVE_SHAREABLE_WINDOW, it wouldn't be something that the content process can touch, since the NSScreen is just some data structure that's allocated on the heap). So we go for the primary screen instead.

So that's the first problem - this function is busted for e10s if the browser is currently on a screen other than the primary one. That's a separate bug which I'll file after I'm done writing this up.

Even if I get this function to return the PrimaryScreen for both e10s and non-e10s, things still go wrong for HiDPI, and this is because the content process NSScreen reports a backing scale factor of 1.0. mstange suspects this is related to [1], and bets that the patch from bug 846566 will address this. However, since that patch causes Netflix / Silverlight to show all black for HiDPI displays, that's kind of a non-starter (unless someone can patch Silverlight, or we invent a way of turning of HiDPI on a per-plugin basis).

Probably the sanest way forward is to have the content process ask the parent process for screen data that it can use for populating its DOM Screen object with.

So I'm going to attack this on that front unless someone else has a different suggestion.

[1]: http://forum.openframeworks.cc/t/retina-detection-backingscalefactor/13087/2

Comment 5

[Kartikaya Gupta (email:[e-mail])]

My drive-by suggestion is to reuse the code that B2G has to handle this case.

http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/PuppetWidget.cpp#750

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Filed bug 1026595 for the non-primary display issue.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> My drive-by suggestion is to reuse the code that B2G has to handle this case.
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/PuppetWidget.
> cpp#750

Looks like we should already be querying the parent for this information based on that tab child call - 

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#2582
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#1601

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Proof-of-concept fix that forwards GetRect and GetAvailRect to parent process

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Proof-of-concept fix that forwards GetRect and GetAvailRect to parent process

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8441721 [details] [diff] [review]
Proof-of-concept fix that forwards GetRect and GetAvailRect to parent process

So, while it works, I'm not 100% jazzed by this patch, because of both the duplication, and how much IPC it implies (calling window.screen from the console calls RecvGetScreenRect and RecvGetScreenAvailRect 4 times each!).

So one thing would be to make it so that we go over IPC once in order to get all of these values. Or, even better, when a widget changes screens, asynchronously have the TabParent update the TabChild with values that it can cache.

The second thing I don't like about this patch is the duplication, with both GetScreenRect and GetScreenAvailRect doing essentially the same work, and just calling some different function at the very bottom (either GetRect, or GetAvailRect).

I'm also not too happy about needing to store a reference to the nsIScreenManager inside every widget, but maybe that's unavoidable unless I want to query for it every time.

jimm, you know Widget pretty well, and roc, you seem to know nsDeviceContext pretty well. Do either of you have feedback on what I've got here, and other suggestions on how to move forward?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. Interesting tradeoffs. It would be nice to preserve the invariant there's at most one nsIScreen per actual screen.

So, how about creating an IPC PScreen protocol?
-- Make PuppetWidget::GetNativeData(NS_NATIVE_WINDOW) return 'this'
-- Make PuppetScreenManager::ScreenForNativeWidget ask PuppetWidget's TabChild for its ScreenChild (which implements nsIScreen). ScreenChild would replace PuppetScreen.
-- ScreenChild can cache its data. Once you've fetched some data, use appShell->RunInStableState to dispatch a runnable to invalidate the cache once you've returned to the event loop.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Thanks roc, that sounds like a good plan. I'm going to move ahead in that direction.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Ok, so I've got the beginnings of this put together in my (very much a) WIP patch.

What I don't have is the lifecycle of the ScreenChild / ScreenParent down just yet.

roc - is the idea that we want the ScreenChild to be instantiated in TabChild::Init, and destroyed in TabChild::DestroyWindow? At what point do I send the construction call to the TabParent to allocate the ScreenParent? (I suppose I need to read up a bit more on IPC construction / destruction)...

Assuming that's the case (or pretty close), another question: Using PuppetScreenManager will be new for desktop - up until now, the content process on Desktop has been using the platform-specific nsIScreenManager. I've reconfigured the nsContentProcessWidgetFactory to have content processes use the PuppetScreenManager instead, and that'll work with your plan for nsDeviceContext::FindScreen... but I don't exactly know how to make PuppetScreenManager "not lie" when it comes to its other methods.

For example - PuppetScreenManager::GetNumberOfScreens. What TabChild's ScreenChild do I use to get that information? Or should I be communicating with the ContentParent for that one?

I think the architecture of this is just a little unclear to me just yet.

I've posted what I've got so far - I've got the boilerplate more or less done, but I think I need a better sense of how PuppetScreenManager is going to work for its other methods (such that it doesn't break Gonk), and the life-cycles of ScreenChild/ScreenParent.

When you have a second, do you think you could help clear that up a bit?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: [WIP] Let content processes query parent process for DOM Screen values

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mike Conley (:mconley) from comment #14)
> roc - is the idea that we want the ScreenChild to be instantiated in
> TabChild::Init, and destroyed in TabChild::DestroyWindow? At what point do I
> send the construction call to the TabParent to allocate the ScreenParent? (I
> suppose I need to read up a bit more on IPC construction / destruction)...

I think PScreens should be global, so the lifetime should not be managed by TabParent/Child. Probably they should be managed by PContent instead, which I think is global. I think ContentParent/Child should manage a set of PScreens. We probably don't need to destroy any Screens until we shut down. There should be at most one PScreen per nsIScreen, and I don't think we churn nsIScreens.

> Assuming that's the case (or pretty close), another question: Using
> PuppetScreenManager will be new for desktop - up until now, the content
> process on Desktop has been using the platform-specific nsIScreenManager.
> I've reconfigured the nsContentProcessWidgetFactory to have content
> processes use the PuppetScreenManager instead, and that'll work with your
> plan for nsDeviceContext::FindScreen... but I don't exactly know how to make
> PuppetScreenManager "not lie" when it comes to its other methods.
> 
> For example - PuppetScreenManager::GetNumberOfScreens. What TabChild's
> ScreenChild do I use to get that information? Or should I be communicating
> with the ContentParent for that one?

Yeah let's do that. Or better still, we could be more modular and let PContent manage a singleton PScreenManager which PuppetScreenManager can proxy to.

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 8441721 [details] [diff] [review]
Proof-of-concept fix that forwards GetRect and GetAvailRect to parent process

Chatted with mconely via irc yesterday, he's going to try to work on thew new protocol.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: [WIP] Let content processes query parent process for DOM Screen values

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8446288 [details] [diff] [review]
Let content processes query parent process for DOM Screen values

So this is still very much WIP, so please excuse some commented out code, and things saying "TODO: Do this reasonably", etc.

I also haven't checked whether or not this compiles or runs on Windows (I'm booting up my Win 7 VM as we speak to check). In theory, it _should_, since I did the work in the widget/windows nsWidgetFactory to make the Windows screen manager only load for the main process.

So, what I've done here is commented out the code in PuppetWidget's GetNativeData handler that returns the tabchild. This forces nsDeviceContext to attempt to load the PrimaryScreen in nsDeviceContext::FindScreen.

I'm stashing the PScreenChild that's created in mPrimaryScreen in nsScreenManagerProxy, and trying to wipe it out in the InvalidateCache function that's called on next tick.

It seems, however, that just by stashing the PScreenChild in mPrimaryScreen, I've somehow introduced some reference counting error, where consumers of nsDeviceContext::FindScreen will find that the returned nsIScreen has been destructed, and the child process crashes.

So, STR:

1) Apply patch and make a debug build. Hopefully it compiles on Windows.
2) Open an e10s window
3) Open the devtools console for that window, and just type "screen".

Under normal circumstances (ie, in a non-e10s window), that command will return various facts about the current screen. With this patch however, the child process crashes, because of the problem I described above - the PScreen gets destructed before anybody can make use of it.

If I _don't_ cache the mPrimaryScreen, this works properly. We don't accidentally destruct the PScreen too soon. However, the ScreenChild is immediately destructed after being used, and the associated ScreenParent is still hanging around in the parent process. That's why I wanted to do this caching business; so I could send the __delete__ up to the parent.

Anyhow, take a gander at this and tell me what you think and where I've gone wrong. I'm 100% open to alternative suggestions / approaches to what I've got going on here.

Thanks Jim!

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Yes, this patch should build on Windows properly - just need to de-bitrot PuppetWidget.h (not a huge piece of bitrot, and might not even be necessary to fix).

Comment 24

[Josh Matthews [:jdm]]

That seems really brittle to me - what happens if we call GetPrimaryScreen twice before the runnable to invalidate the cache executes? You say "the ScreenChild is immediately destructed after being used, and the associated ScreenParent is still hanging around in the parent process" - that sounds to me like the lifetime of the actors is unrelated to IPDL which is scary. In other places in the code, we add an explicit AddRef when sending the IPDL constructor, and perform a Release in DeallocPFoo, which ensures that the concrete actor remains alive at least as long as the IPDL connection exists.

Comment 25

[Jim Mathies [:jimm]]

(In reply to Josh Matthews [:jdm] from comment #24)
> That seems really brittle to me - what happens if we call GetPrimaryScreen
> twice before the runnable to invalidate the cache executes? You say "the
> ScreenChild is immediately destructed after being used, and the associated
> ScreenParent is still hanging around in the parent process" - that sounds to
> me like the lifetime of the actors is unrelated to IPDL which is scary.

Yes this is definitely the case and it's the remaining problem mconely is trying to sort out.

We don't have good docs here, there's a small blurb in the ipdl docs about ref counting actors, but it's very limited info.

If you have a good example of where we've run into this before and how we handled it please post a mxr link.

Comment 26

[Josh Matthews [:jdm]]

See the uses of AddIPDLReference and ReleaseIPDLReference throughout the codebase, such as http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#123.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8449482 [details] [diff] [review]
Let content processes query parent process for DOM Screen values

Ok, I've cleaned this up a bit, and made the following changes:

1) At billm's suggestion, I got rid of PScreen, and just pass a struct back and forth to give the ScreenProxies their values.

This has the consequence that we don't have "live" nsIScreens for content processes, meaning that any consumers of nsIScreenManager that get a reference to an nsIScreen and hold that reference might find that they're eventually holding on to stale information, since the nsIScreen will not automatically refresh itself with the "right stuff".

2) Put the PuppetScreenManager and PuppetScreen implementations back the way they were. I think that's the better approach than just special-casing Gonk for each method in the proxy.

Feedback on this approach?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8449482 [details] [diff] [review]
Let content processes query parent process for DOM Screen values

Review of attachment 8449482 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

How about we give nsIScreen a unique ID (simple 64bit counter)? Then we can pass that ID back to the content process and then it should be easy for nsScreenManagerProxy to keep around a list of active ScreenProxy objects and update them, making liveness work.

::: widget/xpwidgets/nsScreenManagerProxy.cpp
@@ +63,5 @@
> +nsScreenManagerProxy::ScreenForRect(int32_t inLeft,
> +                                   int32_t inTop,
> +                                   int32_t inWidth,
> +                                   int32_t inHeight,
> +                                   nsIScreen** outScreen)

indentation

::: widget/xpwidgets/nsScreenManagerProxy.h
@@ +10,5 @@
> +#include "mozilla/dom/PScreenManagerChild.h"
> +#include "ScreenProxy.h"
> +
> +using namespace mozilla::dom;
> +using namespace mozilla::widget;

You can't do this in a header file

@@ +12,5 @@
> +
> +using namespace mozilla::dom;
> +using namespace mozilla::widget;
> +
> +class nsScreenManagerProxy MOZ_FINAL : public nsIScreenManager,

Needs documentation

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Tested on OS X, Windows and Linux, and seems to do the job.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values. try: -b do -p all -u all -t none

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let content processes query parent process for DOM Screen values. r=?

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452061 [details] [diff] [review]
Let content processes query parent process for DOM Screen values. r=?

Ok, this seems to do the job.

Try pushes:

https://tbpl.mozilla.org/?tree=Try&rev=8f72f03f57b7
and a re-push to fix some missing headers when building with non-unified sources:
https://tbpl.mozilla.org/?tree=Try&rev=7f72a232a0cc

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8452061 [details] [diff] [review]
Let content processes query parent process for DOM Screen values. r=?

Review of attachment 8452061 [details] [diff] [review]:
-----------------------------------------------------------------

Can you split this up into a patch that adds screen IDs and a patch with everything else?

::: gfx/src/nsDeviceContext.cpp
@@ +638,4 @@
>          mScreenManager->ScreenForNativeWidget(mWidget->GetNativeData(NS_NATIVE_WINDOW),
>                                                outScreen);
>      else
>          mScreenManager->GetPrimaryScreen(outScreen);

add some {} love

::: widget/nsIScreenManager.idl
@@ +17,5 @@
>      // The coordinates are in pixels (not twips) and in screen coordinates.
>      //
>    nsIScreen screenForRect ( in long left, in long top, in long width, in long height ) ;
> +
> +  nsIScreen screenForId ( in unsigned long id ) ;

Document this

::: widget/xpwidgets/ScreenProxy.h
@@ +10,5 @@
> +#include "nsBaseScreen.h"
> +#include "mozilla/dom/PScreenManagerChild.h"
> +#include "mozilla/dom/TabChild.h"
> +
> +using namespace mozilla::dom;

You can't do this in a header file.

@@ +36,5 @@
> +                            int32_t* aHeight) MOZ_OVERRIDE;
> +    NS_IMETHOD GetPixelDepth(int32_t* aPixelDepth) MOZ_OVERRIDE;
> +    NS_IMETHOD GetColorDepth(int32_t* aColorDepth) MOZ_OVERRIDE;
> +
> +    nsIntRect mCacheRect;

Document this. I have a feeling it probably shouldn't be here.

@@ +37,5 @@
> +    NS_IMETHOD GetPixelDepth(int32_t* aPixelDepth) MOZ_OVERRIDE;
> +    NS_IMETHOD GetColorDepth(int32_t* aColorDepth) MOZ_OVERRIDE;
> +
> +    nsIntRect mCacheRect;
> +    nsRefPtr<TabChild> mTabChild;

Maybe these shouldn't be public

::: widget/xpwidgets/nsScreenManagerProxy.cpp
@@ +15,5 @@
> +#include "nsWidgetsCID.h"
> +
> +static NS_DEFINE_CID(kAppShellCID, NS_APPSHELL_CID);
> +
> +using mozilla::unused; // irony

using namespace mozilla;

@@ +24,5 @@
> +nsScreenManagerProxy::nsScreenManagerProxy()
> +: mNumberOfScreens(-1)
> +, mSystemDefaultScale(1.0)
> +, mCacheValid(true)
> +, mCacheWillInvalidate(false)

Indent these

@@ +39,5 @@
> +    // be lying.
> +    NS_WARNING("Setting up communications with the parent nsIScreenManager failed.");
> +  }
> +
> +  InvalidateCacheOnNextTick();

Why? The initial state should be that the cache is invalid...

@@ +95,5 @@
> +  // the TabChild that we're looking for...
> +  for (uint32_t i = 0; i < mScreenCache.Length(); ++i) {
> +      nsRefPtr<ScreenProxy> sp = mScreenCache[i];
> +      if (sp->mCacheRect.IsEqualEdges(queryRect) ||
> +          sp->mCacheRect.Contains(queryRect)) {

The IsEqualEdges check is redundant and can be removed.

Actually I think we probably don't even need to do a cache lookup here. ScreenForRect is not exposed to Web content and is unlikely ever to be used from a content process.

::: widget/xpwidgets/nsScreenManagerProxy.h
@@ +10,5 @@
> +#include "mozilla/dom/PScreenManagerChild.h"
> +#include "ScreenProxy.h"
> +
> +class nsScreenManagerProxy MOZ_FINAL : public nsIScreenManager,
> +                                       public mozilla::dom::PScreenManagerChild

Document stuff in this file. Especially, document how the caching works.

Comment 43

[Jim Mathies [:jimm]]

Comment on attachment 8452061 [details] [diff] [review]
Let content processes query parent process for DOM Screen values. r=?

Review of attachment 8452061 [details] [diff] [review]:
-----------------------------------------------------------------

As roc suggests lets get this broken up for simpler reviews:

- new screen manager
- base xp widget
- isolate platform widget changes so you can seek reviews from the right widget peers
- dom/ipc changes

::: widget/windows/nsScreenManagerWin.cpp
@@ +59,5 @@
>    return retScreen;
>  }
>  
> +NS_IMETHODIMP
> +nsScreenManagerWin :: ScreenForId ( uint32_t aId, nsIScreen **outScreen )

nit - 

nsScreenManagerWin::ScreenForId(uint32_t aId, nsIScreen **outScreen)

when in doubt, match existing formatting of the file you're working in.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Attachment: dom/ipc changes to introduce PScreenManager and ScreenManagerParent

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Attachment: Add unique ID to nsIScreens, and query by ID to nsIScreenManager

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Attachment: Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets

Comment 47

[Mike Conley (:mconley) (:⚙️)]

Attachment: Changes to the Android nsIScreen and nsIScreenManager implementations

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Attachment: Cocoa widget updates to nsIScreen and nsIScreenManager implementations

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Attachment: Gonk widget updates to nsIScreen and nsIScreenManager implementations

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Attachment: GTK widget updates to nsIScreen and nsIScreenManager implementations

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows widget updates to nsIScreen and nsIScreenManager implementations

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make nsDeviceContext handle the child process / TabChild case in FindScreen (r=roc)

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Attachment: dom/ipc changes to introduce PScreenManager and ScreenManagerParent

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Ok, I've broken this up, and I'll attempt to target the reviews at the right people.

Thanks for the initial look at it, btw!


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> Comment on attachment 8452061 [details] [diff] [review]
> Let content processes query parent process for DOM Screen values. r=?
> 
> Review of attachment 8452061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you split this up into a patch that adds screen IDs and a patch with
> everything else?
> 

Broke it up even more than that.

> ::: gfx/src/nsDeviceContext.cpp
> @@ +638,4 @@
> >          mScreenManager->ScreenForNativeWidget(mWidget->GetNativeData(NS_NATIVE_WINDOW),
> >                                                outScreen);
> >      else
> >          mScreenManager->GetPrimaryScreen(outScreen);
> 
> add some {} love
> 

Done!

> ::: widget/nsIScreenManager.idl
> @@ +17,5 @@
> >      // The coordinates are in pixels (not twips) and in screen coordinates.
> >      //
> >    nsIScreen screenForRect ( in long left, in long top, in long width, in long height ) ;
> > +
> > +  nsIScreen screenForId ( in unsigned long id ) ;
> 
> Document this
> 

Done.

> ::: widget/xpwidgets/ScreenProxy.h
> @@ +10,5 @@
> > +#include "nsBaseScreen.h"
> > +#include "mozilla/dom/PScreenManagerChild.h"
> > +#include "mozilla/dom/TabChild.h"
> > +
> > +using namespace mozilla::dom;
> 
> You can't do this in a header file.
> 

Ah, whoops - removed. I refer to mozilla::dom::ScreenDetails now directly.

> @@ +36,5 @@
> > +                            int32_t* aHeight) MOZ_OVERRIDE;
> > +    NS_IMETHOD GetPixelDepth(int32_t* aPixelDepth) MOZ_OVERRIDE;
> > +    NS_IMETHOD GetColorDepth(int32_t* aColorDepth) MOZ_OVERRIDE;
> > +
> > +    nsIntRect mCacheRect;
> 
> Document this. I have a feeling it probably shouldn't be here.
> 
> @@ +37,5 @@
> > +    NS_IMETHOD GetPixelDepth(int32_t* aPixelDepth) MOZ_OVERRIDE;
> > +    NS_IMETHOD GetColorDepth(int32_t* aColorDepth) MOZ_OVERRIDE;
> > +
> > +    nsIntRect mCacheRect;
> > +    nsRefPtr<TabChild> mTabChild;
> 
> Maybe these shouldn't be public
> 

For these, I made nsScreenProxyManager a friend class of ScreenProxy, so it can get access to those private members. Is that what you had in mind?

> ::: widget/xpwidgets/nsScreenManagerProxy.cpp
> @@ +15,5 @@
> > +#include "nsWidgetsCID.h"
> > +
> > +static NS_DEFINE_CID(kAppShellCID, NS_APPSHELL_CID);
> > +
> > +using mozilla::unused; // irony
> 
> using namespace mozilla;
> 

Done.

> @@ +24,5 @@
> > +nsScreenManagerProxy::nsScreenManagerProxy()
> > +: mNumberOfScreens(-1)
> > +, mSystemDefaultScale(1.0)
> > +, mCacheValid(true)
> > +, mCacheWillInvalidate(false)
> 
> Indent these
> 

Done.

> @@ +39,5 @@
> > +    // be lying.
> > +    NS_WARNING("Setting up communications with the parent nsIScreenManager failed.");
> > +  }
> > +
> > +  InvalidateCacheOnNextTick();
> 
> Why? The initial state should be that the cache is invalid...
> 

Why? On construction, the constructor is passed all of the initial values for things like the number of screents, etc. These values can be read and used immediately.


> @@ +95,5 @@
> > +  // the TabChild that we're looking for...
> > +  for (uint32_t i = 0; i < mScreenCache.Length(); ++i) {
> > +      nsRefPtr<ScreenProxy> sp = mScreenCache[i];
> > +      if (sp->mCacheRect.IsEqualEdges(queryRect) ||
> > +          sp->mCacheRect.Contains(queryRect)) {
> 
> The IsEqualEdges check is redundant and can be removed.
> 

Done.

> Actually I think we probably don't even need to do a cache lookup here.
> ScreenForRect is not exposed to Web content and is unlikely ever to be used
> from a content process.

It's not exposed directly, no - but it's used indirectly by nsDeviceContext::ComputeClientRectUsingScreen and nsDeviceContext::ComputeFullAreaUsingScreen by way of the DOM Screen API.

Are you sure we don't want to cache these things? Web content could, for example, make several references to window.screen within the same tick, and not caching would cause us to IPC for each of these...

> 
> ::: widget/xpwidgets/nsScreenManagerProxy.h
> @@ +10,5 @@
> > +#include "mozilla/dom/PScreenManagerChild.h"
> > +#include "ScreenProxy.h"
> > +
> > +class nsScreenManagerProxy MOZ_FINAL : public nsIScreenManager,
> > +                                       public mozilla::dom::PScreenManagerChild
> 
> Document stuff in this file. Especially, document how the caching works.

Added documentation.

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Attachment: Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452515 [details] [diff] [review]
Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets

So, you requested that mTabChild and mCacheRect should not be public, but I'm confused how that should work with our caching scheme.

I'm keying the caching off of both the TabChild and the nsIntRect of previous queries. In order for the "keying" to work, I stash those values into the ScreenProxy object from nsScreenManagerProxy, and then put the cached values into an Array.

nsScreenManagerProxy therefore needs to be able to read and write to these cache key values for this to work. Did you want me to use accessor methods instead? Or did you have a different idea in mind?

(Note that it seems I can't easily set nsScreenManagerProxy as a friend class of ScreenProxy because of the forward-declaration of nsScreenManagerProxy in ScreenProxy.h, in case that's what you had in mind).

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows widget updates to nsIScreen and nsIScreenManager implementations

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452536 [details] [diff] [review]
Windows widget updates to nsIScreen and nsIScreenManager implementations

As discussed in IRC, I'll go with the better spacing styling in nsScreenManagerWin.cpp and nsScreenWin.cpp, and file a follow-up bug to fix the rest of the styling in there.

I've filed that bug as bug 1036003.

Comment 59

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452536 [details] [diff] [review]
Windows widget updates to nsIScreen and nsIScreenManager implementations

Er, let me fix the formatting in that for loop as well.

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows widget updates to nsIScreen and nsIScreenManager implementations

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452547 [details] [diff] [review]
Windows widget updates to nsIScreen and nsIScreenManager implementations

Ok, I feel better about this one.

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452372 [details] [diff] [review]
Changes to the Android nsIScreen and nsIScreenManager implementations

Not sure if you're the right person to ask for review on this, dougt - but I noticed you were a reviewer on the initial landing of the Android widget stuff. Let me know if there's somebody better to retarget this to.

Comment 63

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452430 [details] [diff] [review]
dom/ipc changes to introduce PScreenManager and ScreenManagerParent

Not 100% sure if you're the right person to r? for this, jimm. Let me know if I should redirect it.

Comment 64

[Mike Conley (:mconley) (:⚙️)]

Attachment: All patches folded together

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8452425 [details] [diff] [review]
Make nsDeviceContext handle the child process / TabChild case in FindScreen (r=roc)

Review of attachment 8452425 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsDeviceContext.cpp
@@ +634,5 @@
> +    if (mWidget && mWidget->GetOwningTabChild()) {
> +        mScreenManager->ScreenForNativeWidget((void *)mWidget->GetOwningTabChild(),
> +                                              outScreen);
> +    }
> +    else if (mWidget && mWidget->GetNativeData(NS_NATIVE_WINDOW)) {

Seems to me PuppetWidget::GetNativeData(NS_NATIVE_WINDOW) should just return GetOwningTabChild().

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mike Conley (:mconley) from comment #54)
> > @@ +39,5 @@
> > > +    // be lying.
> > > +    NS_WARNING("Setting up communications with the parent nsIScreenManager failed.");
> > > +  }
> > > +
> > > +  InvalidateCacheOnNextTick();
> > 
> > Why? The initial state should be that the cache is invalid...
> 
> Why? On construction, the constructor is passed all of the initial values
> for things like the number of screents, etc. These values can be read and
> used immediately.

OK

> It's not exposed directly, no - but it's used indirectly by
> nsDeviceContext::ComputeClientRectUsingScreen and
> nsDeviceContext::ComputeFullAreaUsingScreen by way of the DOM Screen API.

Can you explain more? I don't see how ScreenForRect is called from those functions.

Comment 67

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8452373 [details] [diff] [review]
Cocoa widget updates to nsIScreen and nsIScreenManager implementations

This looks fine to me.

>+static uint32_t sScreenId = 0;
>+
> nsScreenCocoa::nsScreenCocoa (NSScreen *screen)
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> 
>   mScreen = [screen retain];
>+  mId = ++sScreenId;
> 
>   NS_OBJC_END_TRY_ABORT_BLOCK;
> }

It appears that nsScreenCocoa objects are long-lived -- that each generally lives as long as the process that instantiates it.

If not, it might be better to make the "id" the value of mScreen, so that new nsScreenCocoa objects subsequently created for the same NSScreen would have the same id.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8452515 [details] [diff] [review]
Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets

Review of attachment 8452515 [details] [diff] [review]:
-----------------------------------------------------------------

Two significant questions remaining:
-- Does ScreenProxy really need its own invalidation protocol or can we just have it share the invalidation state of the ScreenManagerProxy? The latter would be simpler I think.
-- Would it be a good idea to have a single PScreenManager method that retrieves an array of all screens, and a separate method that retrieves the screen ID for a TabChild? I think that would simplify things.

::: widget/xpwidgets/ScreenProxy.cpp
@@ +18,5 @@
> +
> +ScreenProxy::ScreenProxy(nsScreenManagerProxy* aScreenManager, ScreenDetails aDetails)
> +: mCacheValid(false)
> +, mCacheWillInvalidate(false)
> +, mScreenManager(aScreenManager)

indent

::: widget/xpwidgets/ScreenProxy.h
@@ +70,5 @@
> +    nsIntRect mRect;
> +    nsIntRect mAvailRect;
> +    int32_t mPixelDepth;
> +    int32_t mColorDepth;
> +    double mContentsScaleFactor;

Order these fields by size: first the double, then pointers, then the int32s/nsIntRects, then the bools.

::: widget/xpwidgets/nsScreenManagerProxy.cpp
@@ +96,5 @@
> +  for (uint32_t i = 0; i < mScreenCache.Length(); ++i) {
> +      nsRefPtr<ScreenProxy> sp = mScreenCache[i];
> +      if (sp->mCacheRect.Contains(queryRect)) {
> +          NS_ADDREF(*outScreen = static_cast<nsIScreen*>(sp));
> +          return NS_OK;

Still unclear we really need this...

@@ +130,5 @@
> +  // the TabChild that we're looking for...
> +  for (uint32_t i = 0; i < mScreenCache.Length(); ++i) {
> +      nsRefPtr<ScreenProxy> sp = mScreenCache[i];
> +      if (sp->mTabChild &&
> +          sp->mTabChild == aWidget) {

Put these on same line.

Comment 69

[Jim Mathies [:jimm]]

Comment on attachment 8452547 [details] [diff] [review]
Windows widget updates to nsIScreen and nsIScreenManager implementations

Review of attachment 8452547 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsScreenWin.cpp
@@ +7,5 @@
>  #include "nsCoord.h"
>  #include "gfxWindowsPlatform.h"
>  #include "nsIWidget.h"
>  
> +static uint32_t sScreenId = 0;

nit - there's no need to init this, the default value for a static int is zero, so you're just setting it twice.

@@ +19,5 @@
>    NS_ASSERTION ( ::GetDeviceCaps(hDCScreen, TECHNOLOGY) == DT_RASDISPLAY, "Not a display screen");
>    ::ReleaseDC(nullptr,hDCScreen);
>  #endif
>  
> +  mId = ++sScreenId;

nit - this would look cleaner if you put it up in the member variable init block.

Comment 70

[Mike Conley (:mconley) (:⚙️)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
> Comment on attachment 8452425 [details] [diff] [review]
> Make nsDeviceContext handle the child process / TabChild case in FindScreen
> 
> Review of attachment 8452425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/nsDeviceContext.cpp
> @@ +634,5 @@
> > +    if (mWidget && mWidget->GetOwningTabChild()) {
> > +        mScreenManager->ScreenForNativeWidget((void *)mWidget->GetOwningTabChild(),
> > +                                              outScreen);
> > +    }
> > +    else if (mWidget && mWidget->GetNativeData(NS_NATIVE_WINDOW)) {
> 
> Seems to me PuppetWidget::GetNativeData(NS_NATIVE_WINDOW) should just return
> GetOwningTabChild().

I had that to begin with, but it seemed to introduce crashes:

See - https://tbpl.mozilla.org/?tree=Try&rev=c25b9d758499

I suspected it had to do with callers in the content process attempting to GetNativeData(NS_NATIVE_WINDOW) on the PuppetWidget, and us (up until now) relying on the fact that GetNativeData will return nullptr for NS_NATIVE_WINDOW. By supplying a value for NS_NATIVE_WINDOW that is not a valid window handle... I think that's where the badness occurred.

Now, this is really not a scientific analysis, but I _do_ know that once I stopped supporting NS_NATIVE_WINDOW from the PuppetWidget, the crashes went away:

https://tbpl.mozilla.org/?tree=Try&rev=8f72f03f57b7

I _could_ attempt to determine what in the child process is likely calling GetNativeData(NS_NATIVE_WINDOW), and get it to stop doing that. Let me know if you think that's worth doing.

If not, we might want to stick with this approach.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mike Conley (:mconley) from comment #70)
> I suspected it had to do with callers in the content process attempting to
> GetNativeData(NS_NATIVE_WINDOW) on the PuppetWidget, and us (up until now)
> relying on the fact that GetNativeData will return nullptr for
> NS_NATIVE_WINDOW. By supplying a value for NS_NATIVE_WINDOW that is not a
> valid window handle... I think that's where the badness occurred.
> 
> Now, this is really not a scientific analysis, but I _do_ know that once I
> stopped supporting NS_NATIVE_WINDOW from the PuppetWidget, the crashes went
> away:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=8f72f03f57b7
> 
> I _could_ attempt to determine what in the child process is likely calling
> GetNativeData(NS_NATIVE_WINDOW), and get it to stop doing that. Let me know
> if you think that's worth doing.

I don't want you to do much extra work for this, but it seems to me that to fix the crashes you might just need to fix NativeWindowForFrame in nsNativeCocoaTheme.mm to check that the nsIWidget is a Cocoa widget (possibly by QIing to nsPIWidgetCocoa). If so, I think that's cleaner than continuing to rely on PuppetWidget NS_NATIVE_WINDOW returning null. 

A better fix would be to revise GetNativeWindow to make its parameters more explicit about the type of what is returned, e.g. to add NS_NATIVE_WINDOW_COCOA which guarantees to return an NSWindow* and call that from NativeWindowForFrame. But that's a little more work.

Comment 72

[Mike Conley (:mconley) (:⚙️)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #71)
> I don't want you to do much extra work for this, but it seems to me that to
> fix the crashes you might just need to fix NativeWindowForFrame in
> nsNativeCocoaTheme.mm to check that the nsIWidget is a Cocoa widget
> (possibly by QIing to nsPIWidgetCocoa). If so, I think that's cleaner than
> continuing to rely on PuppetWidget NS_NATIVE_WINDOW returning null. 

I agree that's cleaner, but do we need to be concerned about other calls to PuppetWidget::GetNativeData that are expecting nullptr's that aren't being hit by our test coverage?

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looking at the uses of NS_NATIVE_WINDOW in the tree, I think only the uses in nsPluginInstanceOwner are potentially a problem. I don't know what the current state of plugins in e10s is.

On reflection, I think it makes a good deal of sense for NS_NATIVE_WINDOW to return null for puppet widgets. So let's keep on doing that, and we'll stick with your existing code to avoid calling NS_NATIVE_WINDOW in nsDeviceContext. Sorry for the distraction.

Comment 74

[Mike Conley (:mconley) (:⚙️)]

(In reply to Steven Michaud from comment #67)
> Comment on attachment 8452373 [details] [diff] [review]
> Cocoa widget updates to nsIScreen and nsIScreenManager implementations
> 
> This looks fine to me.
> 
> >+static uint32_t sScreenId = 0;
> >+
> > nsScreenCocoa::nsScreenCocoa (NSScreen *screen)
> > {
> >   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> > 
> >   mScreen = [screen retain];
> >+  mId = ++sScreenId;
> > 
> >   NS_OBJC_END_TRY_ABORT_BLOCK;
> > }
> 
> It appears that nsScreenCocoa objects are long-lived -- that each generally
> lives as long as the process that instantiates it.
> 
> If not, it might be better to make the "id" the value of mScreen, so that
> new nsScreenCocoa objects subsequently created for the same NSScreen would
> have the same id.

Correct! I see no removals from that mScreenList array in nsScreenManagerCocoa, so I assume they're being kept around for the lifetime of the screen manager.

Thanks Steven!

Comment 75

[Mike Conley (:mconley) (:⚙️)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #73)
> Looking at the uses of NS_NATIVE_WINDOW in the tree, I think only the uses
> in nsPluginInstanceOwner are potentially a problem. I don't know what the
> current state of plugins in e10s is.
> 
> On reflection, I think it makes a good deal of sense for NS_NATIVE_WINDOW to
> return null for puppet widgets. So let's keep on doing that, and we'll stick
> with your existing code to avoid calling NS_NATIVE_WINDOW in
> nsDeviceContext. Sorry for the distraction.

Ok, I'll flip your r- to an r+ on that patch then. Thanks!

Comment 76

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452425 [details] [diff] [review]
Make nsDeviceContext handle the child process / TabChild case in FindScreen (r=roc)

r+'ing based on roc's last comment.

Comment 77

[Mike Conley (:mconley) (:⚙️)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)

> > It's not exposed directly, no - but it's used indirectly by
> > nsDeviceContext::ComputeClientRectUsingScreen and
> > nsDeviceContext::ComputeFullAreaUsingScreen by way of the DOM Screen API.
> 
> Can you explain more? I don't see how ScreenForRect is called from those
> functions.

I'm gonna go ahead and backpedal on this. You're absolutely right - ScreenForRect doesn't appear to be used via the DOM Screen API. I think you're right - I don't have to cache this one.

I'll also remove the public mTabChild member from ScreenProxy, and just bind the cached ScreenForNativeWidget screens to TabChild's via a struct that I'll hold in the cache array.

Thanks for looking at the patch!

Comment 78

[Jim Mathies [:jimm]]

Comment on attachment 8452430 [details] [diff] [review]
dom/ipc changes to introduce PScreenManager and ScreenManagerParent

Review of attachment 8452430 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ScreenManagerParent.cpp
@@ +20,5 @@
> +                                         bool* aSuccess)
> +{
> +  mScreenMgr = do_GetService(sScreenManagerContractID);
> +  if (!mScreenMgr) {
> +    NS_WARNING("Couldn't get nsIScreenManager! ScreenManagerParent is likely to misbehave wildly.");

well this isn't going to end well, since RecvRefresh invokes mScreenMgr. Maybe we should just MOZ_CRASH here since we'll end up crashing anyway on subsequent calls. At least here, we'll know the cause and get a more useful stack.

@@ +56,5 @@
> +  *aSuccess = false;
> +
> +  nsCOMPtr<nsIScreen> screen;
> +  nsresult rv = mScreenMgr->ScreenForId(aId, getter_AddRefs(screen));
> +  if (NS_FAILED(rv) || !screen) {

In your widget implementations you should return a failure result when we don't find the screen (see the fall through in each of the ScreenForId calls that return NS_OK), then here you don't have to check !screen. Plus, those widget calls should probably fail if they can't find it anyway.

Comment 79

[Mike Conley (:mconley) (:⚙️)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> Comment on attachment 8452515 [details] [diff] [review]
> Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets
> 
> Review of attachment 8452515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Two significant questions remaining:
> -- Does ScreenProxy really need its own invalidation protocol or can we just
> have it share the invalidation state of the ScreenManagerProxy? The latter
> would be simpler I think.
> -- Would it be a good idea to have a single PScreenManager method that
> retrieves an array of all screens, and a separate method that retrieves the
> screen ID for a TabChild? I think that would simplify things.

I thought about this some, and in order to keep our IPC traffic down, it seems that if we go this route, what we do is the following:

1) For a call to ScreenForNativeWidget or ScreenForRect, we return both the details of _all_ system screens, as well as the ID for the result screen.
2) For every ScreenProxy created and returned by nsScreenManagerProxy, we hold them via a weakref in an array inside nsScreenManagerProxy.
3) Every time nsScreenManagerProxy is invalidated, it marks all of the ScreenProxy's it's holding onto via weakref as invalid as well.

For any call that attempts to get information on a ScreenProxy:

  a) If we're not yet invalidated, we return the current information being stored
  b) If we're invalidated, ask the nsScreenManagerProxy for a refresh, and then return the current information being stored.

When the nsScreenManagerProxy gets a screen refresh, it retrieves information about all of the system screens again, and then goes through each of the ScreenProxy's in the weakref array, updating their information. It then schedules an invalidation on next tick.

This solves the problem of ScreenProxy and nsScreenManagerProxy being disjoint, and keeps our IPC traffic down.

I _think_ that's what you're suggesting, but I'm not sure it's much more simple. Is this what you'd like me to pursue? Or did you have something else in mind?

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds right. I hope that it's at least a little simpler in terms of the state we keep around and the complexity of the IPC protocol.

Comment 82

[Mike Conley (:mconley) (:⚙️)]

roc and I just had a conversation on IRC, and we're going to go back to the original caching scheme as in the current set of patches. The idea of passing along the entire set of screens to the child process turned out to be more complicated than we originally thought.

Comment 83

[Mike Conley (:mconley) (:⚙️)]

Attachment: dom/ipc changes to introduce PScreenManager and ScreenManagerParent (r=jimm).

Comment 84

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8454494 [details] [diff] [review]
dom/ipc changes to introduce PScreenManager and ScreenManagerParent (r=jimm).

This incorporates jimm's review fixes, so carrying forward r+.

Comment 85

[Mike Conley (:mconley) (:⚙️)]

Attachment: Add unique ID to nsIScreens, and query by ID to nsIScreenManager

Comment 86

[Mike Conley (:mconley) (:⚙️)]

Attachment: Add unique ID to nsIScreens, and query by ID to nsIScreenManager. (r=roc)

Comment 87

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8454501 [details] [diff] [review]
Add unique ID to nsIScreens, and query by ID to nsIScreenManager. (r=roc)

Fixed documentation regarding how screenForId behaves when no screen can be found (we throw NS_ERROR_FAILURE now). Carrying forward r+ from roc.

Comment 88

[Mike Conley (:mconley) (:⚙️)]

Attachment: Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets

Comment 89

[Mike Conley (:mconley) (:⚙️)]

Attachment: Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets

Comment 90

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8454508 [details] [diff] [review]
Introduce ScreenProxy and nsScreenManagerProxy to widgets/xpwidgets

We no longer cache results for nsScreenManagerProxy::ScreenForRect - we're only caching for ScreenForNativeWidget. Instead of tossing nsIScreens into the cache directly and keying off of a public member of ScreenProxy, I use a ScreenCacheEntry struct to map TabChild to ScreenProxy's.

Comment 91

[Mike Conley (:mconley) (:⚙️)]

Attachment: Cocoa widget updates to nsIScreen and nsIScreenManager implementations (r=smichaud).

Comment 92

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8454515 [details] [diff] [review]
Cocoa widget updates to nsIScreen and nsIScreenManager implementations (r=smichaud).

The only change here is that we return NS_ERROR_FAILURE for ScreenForId in the event that we cannot find a screen for the passed id.

Carrying forward smichaud r+.

Comment 93

[Mike Conley (:mconley) (:⚙️)]

Attachment: GTK widget updates to nsIScreen and nsIScreenManager implementations

Comment 94

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8454516 [details] [diff] [review]
GTK widget updates to nsIScreen and nsIScreenManager implementations

I'm afraid karlt is away until next week - can you also look at this, roc?

Comment 95

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows widget updates to nsIScreen and nsIScreenManager implementations

Comment 96

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows widget updates to nsIScreen and nsIScreenManager implementations. (r=jimm)

Comment 97

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8454523 [details] [diff] [review]
Windows widget updates to nsIScreen and nsIScreenManager implementations. (r=jimm)

The only change to this from last time is that ScreenForId returns NS_ERROR_FAILURE in the event that it cannot find the screen with the passed ID.

Carrying forward r+ from jimm.

Comment 98

[Mike Conley (:mconley) (:⚙️)]

Attachment: All patches folded together

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8454516 [details] [diff] [review]
GTK widget updates to nsIScreen and nsIScreenManager implementations

Review of attachment 8454516 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/nsScreenGtk.cpp
@@ +14,5 @@
>  
> +
> +static uint32_t sScreenId = 0;
> +
> +

Remove one of the double blank lines

@@ +36,5 @@
> +  *aId = mId;
> +  return NS_OK;
> +} // GetId
> +
> +

here too

Comment 100

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8452372 [details] [diff] [review]
Changes to the Android nsIScreen and nsIScreenManager implementations

So I found widget/android on the module owner list, which is probably what I should have looked at first. Redirecting the review to snorp!

Comment 101

[Mike Conley (:mconley) (:⚙️)]

Thanks! Corrections made, and landed all folded on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf72cebcd85

Comment 102

[Mike Conley (:mconley) (:⚙️)]

Aaaaand had to land a follow-up because I broke the tree due to a bad rebase:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e623938ea6c7

Comment 103

[Mike Conley (:mconley) (:⚙️)]

Aaaaaaaand backed out both patches because I broke the tree:

https://hg.mozilla.org/integration/mozilla-inbound/rev/161857889ed5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0295cdddbce7

The bad rebase is probably still at fault here, but seems to be isolated to Linux only, so I think I can track it down.

Comment 104

[Mike Conley (:mconley) (:⚙️)]

Re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/692a0f99d09d

Comment 105

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/692a0f99d09d

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Just FYI, this added mContentsScaleFactor member variable which isn't ever used.