1282003 - Make nsScreenManagerAndroid support multiple screens.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect, P1)

Description

[Tommy Kuo (away forever...)]

Currently, there is only primary screen on Fennec. In bug 1252788, we want to make Presentation API support Chromecast. We need to create a custom surface and stream it to Chromecast.

So, we need to let nsScreenManagerAndroid support multiple screens to get the correct device context.

Comment 1

[Tommy Kuo (away forever...)]

There are two parts need to do in this bug:

1. Detect Chormecast devices in network on Fennec. [1]
2. Add screen to nsScreenManagerAndroid when a Chromecast device is selected. [2][3]

[1]: https://developer.android.com/guide/topics/media/mediarouter.html
[2]: https://developer.android.com/reference/android/support/v7/media/MediaRouter.Callback.html
[3]: https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsScreenManagerGonk.cpp#1005-1036

--

Currently, onRouteRemoved wouldn't be called when I shutdown the Chromecast. Need to figure out a way to remove screen from nsScreenManagerAndroid.

Comment 2

[Tommy Kuo (away forever...)]

I found onRouteRemoved would be called after I shut down the Chromecast about 1 min.

Comment 3

[Tommy Kuo (away forever...)]

And we should remove screen when user stop casting or the Chromecast be shut down.

Comment 4

[Tommy Kuo (away forever...)]

Attachment: WIP

Comment 5

[Tommy Kuo (away forever...)]

Because bug 1136364 changed the behavior of nsWindow::GetNativeData, it affects the design of `nsScreenManagerAndroid::ScreenForNativeWidget`. I opened a discussion about `nsScreenManagerAndroid::ScreenForNativeWidget` there. Please see the discussion after bug 1136364 comment 17.

Comment 6

[Tommy Kuo (away forever...)]

I'm thinking about how to get the rect from non-primary screens. For the primary screen, we get the rect by GeckoAppShell::GetScreenSize()[1]. I'm not sure we can get the display of non-primary screens from WindowManager in Android. 

Consider that there is no IMU sensor on non-primary screens, so we don't need to detect the rotation of displays. It would be a fixed value, so we don't need to query the rect every time. I think we can set the rect as a fixed value when we construct a new nsScreen.

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java#2504-2512

Comment 7

[Tommy Kuo (away forever...)]

According to the discussion in bug 1136364, I want to keep the java surface in nsScreenAndroid. When `nsScreenManagerAndroid::ScreenForNativeWidget()` called, I want to use this surface to compare. BUT, I found the address I kept in nsScreenAndroid is different to the one I queried from the same nsWindow.

Comment 8

[Tommy Kuo (away forever...)]

Attachment: Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review commit: https://reviewboard.mozilla.org/r/65480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65480/

Comment 9

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/1-2/

Comment 10

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/2-3/

Comment 11

[Tommy Kuo (away forever...)]

Hi Jim, can you give me some feedback on the patch? There is the summery of this patch following:

- Introduce display type to nsScreen/nsWindow.
  - Use the same definition in https://dxr.mozilla.org/mozilla-central/source/widget/gonk/libdisplay/GonkDisplay.h#43-48
  - DISPLAY_EXTERNAL means the wired display, e.g. HDMI display.
  - DISPLAY_VIRTUAL means the wireless display, e.g. Chromecast, Wifi display.

- Add display type and density parameter to GeckoViewSupport::Open (Window::open at Java).
  - Use display type to add a new nsScreenAndroid into nsScreenManagerAndroid.
  - Set the density to nsWindow to make the other nsWindow get the correct pixel ratio.

- Construct a new nsScreenAndroid with an initial rect.
  - I think we can set a fixed rect for non-primary screen, because non-primary screens would not rotate to change the resolution size.

- Query nsScreenAndroid by ID in nsScreenManagerAndroid.
  - Update the logic in nsDeviceContext to get the correct nsScreenAndroid in non-primary display.

- Launch remote_browser.xul for non-primary screens.

---

And I also encountered a problem. When I started casting something to Chromecast, Fennec would loss the ability to render on the primary screen.

Comment 12

[Tommy Kuo (away forever...)]

BTW, if you need the full source code, you can find them at https://github.com/KuoE0/gecko-dev/tree/chromecast-on-fennec.

Comment 13

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/3-4/

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

https://reviewboard.mozilla.org/r/65480/#review62954

::: gfx/src/moz.build:97
(Diff revision 4)
>  if 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']:
>      CXXFLAGS += CONFIG['MOZ_PANGO_CFLAGS']
> +
> +if 'android' in CONFIG['MOZ_WIDGET_TOOLKIT']:
> +    LOCAL_INCLUDES += [
> +        '/widget/android',

You shouldn't do this. You probably want to add a function to nsIWidget.

::: mobile/android/base/java/org/mozilla/gecko/GeckoView.java:43
(Diff revision 4)
>  import android.view.inputmethod.InputConnection;
>  
>  public class GeckoView extends LayerView
>      implements ContextGetter, GeckoEventListener, NativeEventListener {
>  
> +    public enum DisplayType {

No need to use enum. Just use constants like,

    @WrapForJNI public static const int DISPLAY_PRIMARY = 0;
    
And you'll be able to use it in C++,

    if (aDisplayType == java::GeckoView::DISPLAY_PRIMARY) {
        ...
    }

::: mobile/android/chrome/jar.mn:55
(Diff revision 4)
>    content/Linkify.js                   (content/Linkify.js)
>    content/ZoomHelper.js                (content/ZoomHelper.js)
>    content/CastingApps.js               (content/CastingApps.js)
>    content/RemoteDebugger.js            (content/RemoteDebugger.js)
> +  content/remote_browser.xul           (content/remote_browser.xul)
> +  content/remote_browser.js            (content/remote_browser.js)

You can probably use geckoview.xul instead of adding another xul file.

::: widget/android/nsScreenManagerAndroid.cpp:184
(Diff revision 4)
> +    RefPtr<nsScreenAndroid> screen = new nsScreenAndroid(aType, aRect);
> +    mScreens.AppendElement(screen);
> +}
> +
> +void
> +nsScreenManagerAndroid::RemoveScreen(DisplayType aType)

It's possible to add multiple screens of the same type, but when RemoveScreen is called, all of the screens of that type will be removed?

::: widget/android/nsWindow.h:54
(Diff revision 4)
> +    DisplayType GetDisplayType() const;
> +
> +    void SetDensity(double aDensity) { mDensity = aDensity; }
>  
>  private:
> +    double mDensity;

The density should be a property of the screen, not the window?

::: widget/android/nsWindow.cpp:1211
(Diff revision 4)
> +        do_GetService("@mozilla.org/gfx/screenmanager;1");
> +    MOZ_ASSERT(screenMgr, "Failed to get nsIScreenManager!");
> +
> +    RefPtr<nsScreenManagerAndroid> screenMgrAndroid = (nsScreenManagerAndroid*) screenMgr.get();
> +    MOZ_ASSERT(screenMgrAndroid, "Failed to convert to nsScreenManagerAndroid!");
> +    screenMgrAndroid->AddScreen(displayType, nsIntRect(0, 0, aWidth, aHeight));

I think you should separate screen creation from window creation, because there can be multiple windows on one screen. So we don't always want to create a new screen for every new window.

Comment 15

[Tommy Kuo (away forever...)]

https://reviewboard.mozilla.org/r/65480/#review62954

> You can probably use geckoview.xul instead of adding another xul file.

If I use geckoview.xul directly, should I add the functions in remote_browser.js into geckoview.js?

> It's possible to add multiple screens of the same type, but when RemoveScreen is called, all of the screens of that type will be removed?

I don't whether it is possible or not. Currently, I assume that we handle only one screen for any type.

> The density should be a property of the screen, not the window?

Gecko call nsWindow::GetDefaultScaleInternal()[1] to get the density. That why I put the density information in the window. Maybe we can keep the nsScreen in the window to get the density.

[1]: https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.h#111-118

> I think you should separate screen creation from window creation, because there can be multiple windows on one screen. So we don't always want to create a new screen for every new window.

I thought it is only one top-level window on Fennec. What is the scenario with multiple top-level window?

Comment 16

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/4-5/

Comment 17

[Tommy Kuo (away forever...)]

Hi Sotaro, I'm working on Chromecast Remote Display. There is a problem related to GFX, I think maybe you know how to fix it or some suggestions. In my WIP patch[1], when I succeed to display something on the Chromecast, the primary screen can not render anything.

I dumped the frames from CompositorOGL, and they are what they should be. But they doesn't be shown on the primary screen. Can you give me some suggestions?

[1]: https://github.com/KuoE0/gecko-dev/tree/chromecast-on-fennec

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Tommy Kuo [:KuoE0] from comment #17)
> 
> I dumped the frames from CompositorOGL, and they are what they should be.
> But they doesn't be shown on the primary screen. Can you give me some
> suggestions?

I checked attachment 8772714 [details], it seems to lack multiple screens handling in some parts. Some codes under android widget assumes that it has only one screen. All of them need to be changed  as to handle multiple screens. For example, nsWindow has the following static functions but they does not handle multiple screens. A different TopWindow exits in each screen.

  - static nsWindow* TopWindow();
  - static bool IsCompositionPaused();
  - static void InvalidateAndScheduleComposite();
  - static void SchedulePauseComposition();
  - static void ScheduleResumeComposition();
  - static float ComputeRenderIntegrity();

And it seems better to check which code part runs correctly related to primary screen when the problem happens in the following diagram. Though it is a bit old.
  https://github.com/sotaroikeda/firefox-diagrams/blob/master/mobile/mobile_GeckoView_47.pdf

Comment 19

[Tommy Kuo (away forever...)]

Hi Sotaro, thank for you help. But I didn't update `gGeckoViewWindow` when the second window is created. So I think it should not make the primary screen malfunction. And I have no idea about `TopWindow()`, should I bring the primary screen to front if I want to update it?

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

(In reply to Tommy Kuo [:KuoE0] from comment #15)
> https://reviewboard.mozilla.org/r/65480/#review62954
> 
> > You can probably use geckoview.xul instead of adding another xul file.
> 
> If I use geckoview.xul directly, should I add the functions in
> remote_browser.js into geckoview.js?

That's fine. Also, I think some of the code in remote_browser.js is not necessary.

> > It's possible to add multiple screens of the same type, but when RemoveScreen is called, all of the screens of that type will be removed?
> 
> I don't whether it is possible or not. Currently, I assume that we handle
> only one screen for any type.

Then we should assert in AddScreen that we only have one screen for any type.

> > The density should be a property of the screen, not the window?
> 
> Gecko call nsWindow::GetDefaultScaleInternal()[1] to get the density. That
> why I put the density information in the window. Maybe we can keep the
> nsScreen in the window to get the density.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.h#111-
> 118

Okay, maybe query screen manager for the density from nsWindow?

> > I think you should separate screen creation from window creation, because there can be multiple windows on one screen. So we don't always want to create a new screen for every new window.
> 
> I thought it is only one top-level window on Fennec. What is the scenario
> with multiple top-level window?

We should support multiple top-level windows (even though Fennec only uses one top-level window), because an app using GeckoView can create multiple GeckoViews on the same screen.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Tommy Kuo [:KuoE0] from comment #19)
> Hi Sotaro, thank for you help. But I didn't update `gGeckoViewWindow` when
> the second window is created. So I think it should not make the primary
> screen malfunction. And I have no idea about `TopWindow()`, should I bring
> the primary screen to front if I want to update it?

It might be better to check what is working correctly for primary screen by adding logout manually.

TopLevelWindow is a root window of gecko's content handling/compositing and is created via nsIAppShellService::CreateTopLevelWindow(). Therefore, if gecko composites to multiple targets, there should be multiple top level windows.

The following is a diagram of nsWindow on gonk. Even when gecko has only one nsIScreen and one TopLevelWindow by default, there are 3 nsWindow instances, only one nsWindow is TopLevelWindow.
  https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/widget_nsWindow_FirefoxOS_2_2.pdf

On gonk, TopLevelWindow could exist for each physical Display and each virtual display. From comment 20, on android TopLevelWindow seems to exit for each GeckoView and each virtual display.

some android widget code seem to assume only one composite target as in comment 18. It might cause the problem.

Comment 22

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/5-6/

Comment 23

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/6-7/

Comment 24

[Tommy Kuo (away forever...)]

Hi Sotaro, in [1], I found the `newSurface` of the primary-screen and the Chromecast have same address. I printed the following variables, `widget`, `javaSurface`, `nativeWindow` and `newSurface`. All of them except `newSurface` have different address. I think it should be a problem, but I have no idea about how to fix it.

Can you give me some suggestions? Thank you!

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#181-189

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Tommy Kuo [:KuoE0] from comment #24)
> Hi Sotaro, in [1], I found the `newSurface` of the primary-screen and the
> Chromecast have same address. I printed the following variables, `widget`,
> `javaSurface`, `nativeWindow` and `newSurface`. All of them except
> `newSurface` have different address. I think it should be a problem, but I
> have no idea about how to fix it.

Hmm, it seems wired. Did you confirmed that EGLSurface of primary-screen is not destroyed?

Comment 26

[Tommy Kuo (away forever...)]

Sorry... I found I printed surface with `&surface` and it should be printed without `&`. That's why I got the same address.

Comment 27

[Sotaro Ikeda [:sotaro]]

No problem:)

Comment 28

[Tommy Kuo (away forever...)]

Hi Jim, thank you for the suggestion. I think the issue is caused by the second compositor. I tried to stop the compositor creation for Chromecast, so there is only blank screen on Chromecast. And the issue I mentioned on the primary screen doesn't happen again. I'll be continued to figure out the problem.

Comment 29

[Tommy Kuo (away forever...)]

Hi Jim, I found this problem is caused by [1]. In [1], we set the layerClient in AndroidBridge. When Chromecast started, mLayerClient of the primary screen in AndroidBridge will be replaced by the one of Chromecast. But I'm not sure how to modify this logic to make it work. It seems to be related to AsyncCompositionManager.

[1]: https://dxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1027

Comment 30

[Tommy Kuo (away forever...)]

Hi Kats, I think you are familiar with AsyncCompositionManager. I'm trying to make the second GeckoView work. As I mentioned in comment #29, we keep only one GeckoLayerClient in AndroidBridge. If I create another GeckoView, the GeckoLayerClient would be replaced by the new one. And the primary screen would lose the rendering ability. Could you know how to make it work? Is it possible to make AsyncCompositionManager know which GeckoLayerView should be used?

Comment 31

[(inactive) Jim Chen [:jchen] [:darchons]]

Yeah we really shouldn't be using the layer client in AndroidBridge. :rbarker is working on getting rid of GeckoLayerClient, so maybe that will fix this as well.

Comment 32

[Tommy Kuo (away forever...)]

Thank you, Jim. And do you know the bug ID? I want to set that bug as the block of this bug.

Comment 33

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Hi Randall, Jim said that you are working on getting rid of GeckoLayerClient. I met some problem with GeckoLayerClient (comment #29), can you give me some feedback on my patch? Does it affect your work? And can you give me the bug number you're working on? Thank you!

Comment 34

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

Randall/Jim can answer your questions in this area, dropping needinfo on me.

Comment 35

[Randall Barker [:rbarker]]

(In reply to Tommy Kuo [:KuoE0] from comment #33)
> Comment on attachment 8772714 [details]
> Bug 1282003 - Make nsScreenManagerAndroid support multiple screens.
> 
> Hi Randall, Jim said that you are working on getting rid of
> GeckoLayerClient. I met some problem with GeckoLayerClient (comment #29),
> can you give me some feedback on my patch? Does it affect your work? And can
> you give me the bug number you're working on? Thank you!

My work landed in Bug 1291373 - [geckoview] Remove JavaPanZoomController (JPZ) from mobile/android. While the GeckoLayerClient was reduced, I was not able to remove it completely. As the gecko view work progresses this issue will need to be addressed but I'm afraid I don't have any short term solution for dealing with the singleton nature of the code as it is written today.

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review75086

::: mobile/android/app/mobile.js:30
(Diff revision 7)
>  
>  pref("toolkit.defaultChromeURI", "chrome://browser/content/browser.xul");
>  pref("browser.chromeURL", "chrome://browser/content/");
>  
> +// multi-screen
> +pref("toolkit.geckoviewURI", "chrome://browser/content/geckoview.xul");

This pref name should be something like 'toolkit.presentationURI'.

::: mobile/android/base/java/org/mozilla/gecko/GeckoView.java:44
(Diff revision 7)
>  import android.view.inputmethod.InputConnection;
>  
>  public class GeckoView extends LayerView
>      implements ContextGetter, GeckoEventListener, NativeEventListener {
>  
> +    @WrapForJNI public static int DISPLAY_PRIMARY  = 0;

I don't like this because it doesn't mean anything to a normal GeckoView consumer. I would suggest making a GeckoView subclass (PresentationView?) and put that stuff there.

You would also need to find a different way to create the XUL window with the right flags.

::: mobile/android/chrome/content/geckoview.js:34
(Diff revision 7)
>  
>      // Notify Java that Gecko has loaded.
>      Messaging.sendRequest({ type: "Gecko:Ready" });
>  }
> +
> +function loadURI(aURI) {

Does anything call this?

::: widget/android/nsScreenManagerAndroid.cpp:41
(Diff revision 7)
>  }
>  
>  NS_IMETHODIMP
>  nsScreenAndroid::GetRect(int32_t *outLeft, int32_t *outTop, int32_t *outWidth, int32_t *outHeight)
>  {
> +    if (mDisplayType != widget::GeckoView::DISPLAY_PRIMARY()) {

Using GeckoView types in nsScreenAndroid is kinda gross. Surely we already have some enum in Gecko for this?

::: widget/android/nsWindow.cpp:1141
(Diff revision 7)
>      if (aChromeURI) {
>          url = aChromeURI->ToCString();
>      } else {
> -        url = Preferences::GetCString("toolkit.defaultChromeURI");
> +        url = aDisplayType == widget::GeckoView::DISPLAY_PRIMARY()
> +            ? Preferences::GetCString("toolkit.defaultChromeURI")
> +            : Preferences::GetCString("toolkit.geckoviewURI");

I don't like this pref at all, actually. Your PresentationView should just pass the URL directly in aChromeURI.

::: widget/android/nsWindow.cpp:1185
(Diff revision 7)
>      window->mGLControllerSupport = mozilla::MakeUnique<GLControllerSupport>(
>              window, GLController::LocalRef(
>              aCls.Env(), GLController::Ref::From(aGLController)));
>  
> +    // Set display type for nsWindow
> +    window->SetDisplayType(aDisplayType);

Is the nsWindow going to be attached to the virtual Screen? Do we need to have a display type for the nsWindow if we do that?

Comment 37

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think this patch has a lot of problems even without the GeckoLayerClient/APZ stuff, but maybe we can find a way to just not use APZ/GLC with these types of windows? The user can't touch this window, can he?

Comment 38

[Tommy Kuo (away forever...)]

For the use case of Chromecase. The answer is no. User can not touch this window. I don't think Chromecast supports touch screen.

Comment 39

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/7-8/

Comment 40

[Tommy Kuo (away forever...)]

Hi snorp, can you take a look on this file[1]? I tried to create a new class PresentationView inherited GeckoView. And when PresentationView is created for Chromecast, there is only a black screen on Chromecast. Could you give me some feedback on this? Thank you!

[1]: https://github.com/KuoE0/gecko-dev/blob/chromecast-on-fennec-with-presentation-api-refactoring/mobile/android/geckoview/src/main/java/org/mozilla/gecko/PresentationView.java

> ::: widget/android/nsScreenManagerAndroid.cpp:41
> (Diff revision 7)
> >  }
> >  
> >  NS_IMETHODIMP
> >  nsScreenAndroid::GetRect(int32_t *outLeft, int32_t *outTop, int32_t *outWidth, int32_t *outHeight)
> >  {
> > +    if (mDisplayType != widget::GeckoView::DISPLAY_PRIMARY()) {
> 
> Using GeckoView types in nsScreenAndroid is kinda gross. Surely we already have some enum in Gecko for this?

For this issue, we don't have any enum for this in Gecko. Did you think we should have two enums for this, one in Gecko and another one in Java?

Comment 41

[Tommy Kuo (away forever...)]

Oh, I fixed it! I forgot to call LayerView::onAttachedToWindow.

Comment 42

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review75086

> Is the nsWindow going to be attached to the virtual Screen? Do we need to have a display type for the nsWindow if we do that?

I just set display type here to query in nsDeviceContext. I need them to get the correct density of non-primary screen. Do you have any better suggestion to do that? Please see the line 620~627 in nsDeviceContext.cpp. Thank you!

Comment 43

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review75086

> Using GeckoView types in nsScreenAndroid is kinda gross. Surely we already have some enum in Gecko for this?

Do you think it's better to declare this enums in both C++ and Java? I think there is a risk if someone want to change it and forget to update the other side.

Comment 44

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Tommy Kuo [:KuoE0] from comment #40)
> Hi snorp, can you take a look on this file[1]? I tried to create a new class
> PresentationView inherited GeckoView. And when PresentationView is created
> for Chromecast, there is only a black screen on Chromecast. Could you give
> me some feedback on this? Thank you!
> 
> [1]:
> https://github.com/KuoE0/gecko-dev/blob/chromecast-on-fennec-with-
> presentation-api-refactoring/mobile/android/geckoview/src/main/java/org/
> mozilla/gecko/PresentationView.java
> 
> > ::: widget/android/nsScreenManagerAndroid.cpp:41
> > (Diff revision 7)
> > >  }
> > >  
> > >  NS_IMETHODIMP
> > >  nsScreenAndroid::GetRect(int32_t *outLeft, int32_t *outTop, int32_t *outWidth, int32_t *outHeight)
> > >  {
> > > +    if (mDisplayType != widget::GeckoView::DISPLAY_PRIMARY()) {
> > 
> > Using GeckoView types in nsScreenAndroid is kinda gross. Surely we already have some enum in Gecko for this?
> 
> For this issue, we don't have any enum for this in Gecko. Did you think we
> should have two enums for this, one in Gecko and another one in Java?

I think you should only use the Java enums when interacting with the Java instance.

Comment 45

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Tommy Kuo [:KuoE0] from comment #42)
> Comment on attachment 8772714 [details]
> Bug 1282003 - Make nsScreenManagerAndroid support multiple screens.
> 
> https://reviewboard.mozilla.org/r/65480/#review75086
> 
> > Is the nsWindow going to be attached to the virtual Screen? Do we need to have a display type for the nsWindow if we do that?
> 
> I just set display type here to query in nsDeviceContext. I need them to get
> the correct density of non-primary screen. Do you have any better suggestion
> to do that? Please see the line 620~627 in nsDeviceContext.cpp. Thank you!

IMO the 'display type' should be a property of the nsIScreen. You can get the nsIScreen that a widget uses with nsIWidget::GetWidgetScreen().

As for creating a GeckoView that points to such a screen, I think you would need to add some JNI stuff for creating the new screen, then pass that screen into GeckoView.Window.open.

Comment 46

[Tommy Kuo (away forever...)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #45)
> (In reply to Tommy Kuo [:KuoE0] from comment #42)
> > Comment on attachment 8772714 [details]
> > Bug 1282003 - Make nsScreenManagerAndroid support multiple screens.
> > 
> > https://reviewboard.mozilla.org/r/65480/#review75086
> > 
> > > Is the nsWindow going to be attached to the virtual Screen? Do we need to have a display type for the nsWindow if we do that?
> > 
> > I just set display type here to query in nsDeviceContext. I need them to get
> > the correct density of non-primary screen. Do you have any better suggestion
> > to do that? Please see the line 620~627 in nsDeviceContext.cpp. Thank you!
> 
> IMO the 'display type' should be a property of the nsIScreen. You can get
> the nsIScreen that a widget uses with nsIWidget::GetWidgetScreen().
> 
> As for creating a GeckoView that points to such a screen, I think you would
> need to add some JNI stuff for creating the new screen, then pass that
> screen into GeckoView.Window.open.

Hi Snorp, sorry, I don't understand what you mean. May you describe more detail?

Comment 47

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can try, which part is unclear? In nsDeviceContext() you can just call nsIWidget::GetWidgetScreen()[0] instead of what you have there currently. Additionally, I think all of the stuff about display types (primary, external, virtual, etc) should be a property of the nsIScreen. Finally, you could add some stuff to GeckoView (or elsewhere) that allows you to create a new nsIScreen and use that as the screen for the nsIWidget that the GeckoView creates.

[0] https://dxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h?q=nsIWidget&redirect_type=direct#1197

Comment 48

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/8-9/

Comment 49

[Tommy Kuo (away forever...)]

Attachment: Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review commit: https://reviewboard.mozilla.org/r/82054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82054/

Comment 50

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/9-10/

Comment 51

[Tommy Kuo (away forever...)]

Attachment: Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review commit: https://reviewboard.mozilla.org/r/82076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82076/

Comment 52

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/1-2/

Comment 53

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/10-11/

Comment 54

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

https://reviewboard.mozilla.org/r/82054/#review80640

I'm not a fan of the hardcoded mapping from display type to ID, but otherwise this looks pretty good.

::: widget/android/nsScreenManagerAndroid.h:28
(Diff revision 2)
>      NS_IMETHOD GetRect(int32_t* aLeft, int32_t* aTop, int32_t* aWidth, int32_t* aHeight) override;
>      NS_IMETHOD GetAvailRect(int32_t* aLeft, int32_t* aTop, int32_t* aWidth, int32_t* aHeight) override;
>      NS_IMETHOD GetPixelDepth(int32_t* aPixelDepth) override;
>      NS_IMETHOD GetColorDepth(int32_t* aColorDepth) override;
>  
> +    static uint32_t GetIdFromDisplayType(int32_t aDisplayType) {

I think it would be better to just have a monotonically increasing ID here, otherwise you couldn't have multiple screens of the same type.

::: widget/android/nsScreenManagerAndroid.cpp:19
(Diff revision 2)
> +#define ALOG(args...) __android_log_print(ANDROID_LOG_INFO, "nsScreenManagerAndroid", ## args)
> +
>  using namespace mozilla;
>  
> -nsScreenAndroid::nsScreenAndroid(void *nativeScreen)
> +nsScreenAndroid::nsScreenAndroid(int32_t aDisplayType, nsIntRect aRect)
> +    : mId(nsScreenAndroid::GetIdFromDisplayType(aDisplayType))

See comment above about the ID

::: widget/android/nsScreenManagerAndroid.cpp:96
(Diff revision 2)
>  
> +
>  void
>  nsScreenAndroid::ApplyMinimumBrightness(uint32_t aBrightness)
>  {
> +    if (mDisplayType != nsIScreen::DISPLAY_PRIMARY) {

Maybe just roll this check into the condition we already have below

::: widget/android/nsScreenManagerAndroid.cpp:120
(Diff revision 2)
>  }
>  
>  NS_IMETHODIMP
>  nsScreenManagerAndroid::GetPrimaryScreen(nsIScreen **outScreen)
>  {
> -    NS_IF_ADDREF(*outScreen = mOneScreen.get());
> +    ScreenForId(nsScreenAndroid::GetIdFromDisplayType(nsIScreen::DISPLAY_PRIMARY), outScreen);

I think we could just always assign ID 0 to the primary screen, so this would just be:

ScreenForId(PRIMARY_SCREEN_ID, outScreen);

::: widget/android/nsScreenManagerAndroid.cpp:184
(Diff revision 2)
> +
> +    nsCOMPtr<nsIScreen> screen;
> +    ScreenForId(nsScreenAndroid::GetIdFromDisplayType(aDisplayType),
> +                getter_AddRefs(screen));
> +    // There is only one nsScreen for each display type.
> +    MOZ_ASSERT(!screen, "nsScreenAndroid with this type already exists.");

It's good that you have this assertion, but why do we want to restrict it this way? Is it so you can more easily find the presentation display later? You could just take the first matching VIRTUAL one in if you need to look it up that way.

Comment 55

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

https://reviewboard.mozilla.org/r/82076/#review80644

I don't think we want to store the display type in nsWindow. The nsIScreen is the single source of truth for that information. The widget is just a widget, it shouldn't care much about the display type. We should instead add a way to create the window with a given nsIScreen. If you want to know if a window is on a virtual screen, you would call nsIWidget::GetWidgetScreen() and query the resulting nsIScreen. nsIWidget::GetWidgetScreen() would be a simple getter in that case.

Maybe we could just add a setter (nsIWidget::SetWidgetScreen or SetScreen) if it's too cumbersome to set the nsIScreen during creation.

Comment 56

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review80682

Looks ok if you fix the displaytype-as-id stuff

::: mobile/android/base/java/org/mozilla/gecko/ScreenManagerHelper.java:17
(Diff revision 11)
> +class ScreenManagerHelper {
> +    @WrapForJNI public static final int DISPLAY_PRIMARY  = 0;
> +    @WrapForJNI public static final int DISPLAY_EXTERNAL = 1;
> +    @WrapForJNI public static final int DISPLAY_VIRTUAL  = 2;
> +
> +    public native static void addDisplay(int displayType, int width, int height);

Return the ID

::: mobile/android/base/java/org/mozilla/gecko/ScreenManagerHelper.java:18
(Diff revision 11)
> +    @WrapForJNI public static final int DISPLAY_PRIMARY  = 0;
> +    @WrapForJNI public static final int DISPLAY_EXTERNAL = 1;
> +    @WrapForJNI public static final int DISPLAY_VIRTUAL  = 2;
> +
> +    public native static void addDisplay(int displayType, int width, int height);
> +    public native static void removeDisplay(int displayType);

Take the ID instead of the display type

Comment 57

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/2-3/

Comment 58

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/11-12/

Comment 59

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/1-2/

Comment 60

[Tommy Kuo (away forever...)]

Hi snorp, I updated the 3 patches. Currently, I changed all display type to screen ID now. And I also add a member variable `mScreenId` in nsWindow to identify which screen it belongs to.

Comment 61

[Tommy Kuo (away forever...)]

Hi Kats and Jonathan, I found the patch 2.0 in Bug 1288760 is backed out in Bug 1300421. I think I need that patch for my work (see my patch part 3). May I use the patch 2.0 in Bug 1288760 in my work? Or, is there another suggestion to query the right nsScreen in nsDeviceContext?

Thanks!

Comment 62

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/3-4/

Comment 63

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/12-13/

Comment 64

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/2-3/

Comment 65

[Jonathan Kew [:jfkthame]]

(In reply to Tommy Kuo [:KuoE0] from comment #61)
> Hi Kats and Jonathan, I found the patch 2.0 in Bug 1288760 is backed out in
> Bug 1300421. I think I need that patch for my work (see my patch part 3).
> May I use the patch 2.0 in Bug 1288760 in my work? Or, is there another
> suggestion to query the right nsScreen in nsDeviceContext?

It should be fine for you to use bug 1288760 patch 2.0; the regressions that caused us to back out those patches were caused by the later parts (2.1 onwards), but I believe 2.0 is safe. So you could re-land it as part of this bug, if that's convenient.

Comment 66

[Tommy Kuo (away forever...)]

Ok, I'll merge the patch with my patch! Thanks!

Comment 67

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/4-5/

Comment 68

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/13-14/

Comment 69

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/3-4/

Comment 70

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/4-5/

Comment 71

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review81390

I think this one will be good once you fix the main thread dispatch stuff.

::: mobile/android/base/java/org/mozilla/gecko/ScreenManagerHelper.java:16
(Diff revision 14)
> +@WrapForJNI
> +class ScreenManagerHelper {
> +    /**
> +     * The following display types use the same definition in nsIScreen.idl
> +     */
> +    @WrapForJNI public static final int DISPLAY_PRIMARY  = 0;

Can we use an enum here instead of 3 fields?

::: mobile/android/base/java/org/mozilla/gecko/ScreenManagerHelper.java:30
(Diff revision 14)
> +     * @param height      the height of the new nsScreen
> +     * @param density     the density of the new nsScreen
> +     *
> +     * @return            return the ID of the added nsScreen
> +     */
> +    public native static int addDisplay(int displayType,

You can just use `@WrapForJNI(dispatchTo = "gecko")` to make this run on the main thread instead of the Runnable stuff you have in the helper class.

::: mobile/android/base/java/org/mozilla/gecko/ScreenManagerHelper.java:40
(Diff revision 14)
> +    /**
> +     * Remove the nsScreen by the specific screen ID.
> +     *
> +     * @param screenId    the ID of the screen would be removed.
> +     */
> +    public native static void removeDisplay(int screenId);

Same as addDisplay()

::: widget/android/nsScreenManagerAndroid.cpp:125
(Diff revision 14)
> +
> +    static int32_t AddDisplay(int32_t aDisplayType, int32_t aWidth, int32_t aHeight, float aDensity) {
> +        class Runnable final : public mozilla::Runnable
> +        {
> +            public:
> +                Runnable(int32_t aDisplayType, int32_t aWidth, int32_t aHeight, float aDensity)

See the comment above re: @WrapForJNI about avoiding this Runnable.

::: widget/android/nsScreenManagerAndroid.cpp:157
(Diff revision 14)
> +                int32_t mDisplayType, mWidth, mHeight;
> +                uint32_t mScreenId;
> +                float mDensity;
> +        };
> +
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();

After you do the `dispatchTo` thing, you can assert that you are on the main thread here.

Comment 72

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

https://reviewboard.mozilla.org/r/82076/#review81392

This one seems fine now if you fix the couple small issues.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:54
(Diff revision 5)
>      private InputConnectionListener mInputConnectionListener;
>  
>      private boolean onAttachedToWindowCalled;
> +    protected int screenId = 0;
> +
> +    public void setScreenId(int screenId) {

This doesn't really make any sense as a public API. I would just remove this and set the screenId in the constructor of the PresentationView (or maybe in onAttachedToWindow).

::: widget/android/nsWindow.cpp:1681
(Diff revision 5)
>  }
>  
>  double
>  nsWindow::GetDefaultScaleInternal()
>  {
> -    static double density = 0.0;
> +    nsCOMPtr<nsIScreenManager> screenMgr =

Just call GetWidgetScreen() instead of duplicating all of the screenmanager boilerplate

Comment 73

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

https://reviewboard.mozilla.org/r/82054/#review81394

Getting closer. I wonder if we want all of the display type stuff to be part of nsIScreen so we can do similar stuff on other platforms, but for now it's probably not necessary.

::: widget/android/nsScreenManagerAndroid.cpp:28
(Diff revision 5)
> +nsScreenAndroid::nsScreenAndroid(int32_t aDisplayType, nsIntRect aRect)
> +    : mId(sScreenId++)
> +    , mDisplayType(aDisplayType)
> +    , mRect(aRect)
> +    , mDensity(mozilla::java::GeckoAppShell::GetDensity())
>  {

We need to do some checking here to make sure the primary display has the right ID. Maybe just:

`if (mDisplayType == DISPLAY_PRIMARY) {
    mId = PRIMARY_SCREEN_ID;
}`

And then we should check for duplicate IDs in the screen manager.

::: widget/nsIScreen.idl:15
(Diff revision 5)
>  interface nsIScreen : nsISupports
>  {
>    /**
> +   * The display type of this screen belongs to.
> +   */
> +  const long DISPLAY_PRIMARY  = 0; // primary screen

use an enum

Comment 74

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review81390

> You can just use `@WrapForJNI(dispatchTo = "gecko")` to make this run on the main thread instead of the Runnable stuff you have in the helper class.

I found that if I want to return the screen ID, I can't use `@WrapForJNI(dispatchTo = "gecko")`.  Please the following error message:

```
Exception in thread "main" java.lang.IllegalStateException: Must return void when not dispatching to current thread for native method ScreenMa
nagerHelper::AddDisplay
 2:32.79        at org.mozilla.gecko.annotationProcessors.CodeGenerator.generateNative(CodeGenerator.java:358)
 2:32.79        at org.mozilla.gecko.annotationProcessors.AnnotationProcessor.generateClass(AnnotationProcessor.java:131)
 2:32.79        at org.mozilla.gecko.annotationProcessors.AnnotationProcessor.main(AnnotationProcessor.java:85)
```

> Same as addDisplay()

Same error message as addDisplay().

Comment 75

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review81390

> Can we use an enum here instead of 3 fields?

Did you mean like this?

```
enum DisplayType {
    DISPLAY_PRIMARY(0),  // primary screen
    DISPLAY_EXTERNAL(1), // wired displays, such as HDMI, DisplayPort, etc.
    DISPLAY_VIRTUAL(2);  // wireless displays, such as Chromecast, WiFi-Display, etc.

    private int value;
    private DisplayType(int value) {
        this.value = value;
    }
    public int getValue() {
        return value;
    }
};
```

So, if I want to pass it to `addDisplay(...)`, I have to call `ScreenManagerHelper.addDisplay(DisplayType.DISPLAY_VIRTUAL.getValue(), ...)`. For JNI function, I think the type of the parameter should use `int`.

Comment 76

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/5-6/

Comment 77

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/14-15/

Comment 78

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/5-6/

Comment 79

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review81740

Still need to use @WrapForJNI correctly to avoid proxying to other threads yourself.

::: mobile/android/base/java/org/mozilla/gecko/ScreenManagerHelper.java:14
(Diff revision 15)
> +import org.mozilla.gecko.annotation.WrapForJNI;
> +
> +/**
> + * The following display types use the same definition in nsIScreen.idl
> + */
> +enum DisplayType {

OK I forgot that Java enums are just the worst, and now kinda regret this suggestion. You can change this back to fields if you want, or leave it as is. Sorry!

::: mobile/android/base/java/org/mozilla/gecko/ScreenManagerHelper.java:40
(Diff revision 15)
> +     * @param height      the height of the new nsScreen
> +     * @param density     the density of the new nsScreen
> +     *
> +     * @return            return the ID of the added nsScreen
> +     */
> +    @WrapForJNI

I don't think you understood my comment. If you use `@WrapForJNI(dispatchTo = "gecko")` here, it will proxy the native call to the gecko thread for you, and you can remove the Runnable stuff you have in ScreenManagerHelper::AddDisplay(). This is preferable to having to do it yourself everywhere.

Comment 80

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

https://reviewboard.mozilla.org/r/65480/#review81744

I just now saw the comment about @WrapToJNI not really working if you have a return value, so that makes sense.

Comment 81

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

https://reviewboard.mozilla.org/r/82054/#review81748

Very nice

Comment 82

[(inactive) Jim Chen [:jchen] [:darchons]]

Drive-by:

> > +/**
> > + * The following display types use the same definition in nsIScreen.idl
> > + */
> > +enum DisplayType {
> 
> OK I forgot that Java enums are just the worst, and now kinda regret this
> suggestion. You can change this back to fields if you want, or leave it as
> is. Sorry!

Yeah Java enums are gross and inflate our apk size / memory usage. Please use static final fields.


> I found that if I want to return the screen ID, I can't use
> `@WrapForJNI(dispatchTo = "gecko")`.  Please the following error message:

You can still use a lambda to make this prettier. Use nsAppShell::SyncRunEvent if you can,

> int32_t ret = -1;
> auto lambda = [arg1, arg2, &ret] {
>     ...
> };
> nsAppShell::SyncRunEvent(
>     nsAppShell::LambdaEvent<decltype(lambda)>(Move(lambda)));
> return ret;

or, if you really want to use SyncRunnable,

> int32_t ret = -1;
> nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> SyncRunnable::DispatchToThread(mainThread, NS_NewRunnableFunction(
>    [arg1, arg2, &ref] {
>        ...
>    }
> ));
> return ret;

Comment 83

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/15-16/

Comment 84

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/6-7/

Comment 85

[Tommy Kuo (away forever...)]

I think this bug is ready, I'll run the tests and mark 'checkin-needed' after all tests passed.

Comment 86

[Tommy Kuo (away forever...)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afbfb7261f46&selectedJob=28592310

Comment 87

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ddf5af982d8b
(Part 1) Add display type/density info and addScreen/removeScreen function. r=snorp
https://hg.mozilla.org/integration/fx-team/rev/13db73034f80
(Part 2) Add screenManagerHelper in Java. r=snorp
https://hg.mozilla.org/integration/fx-team/rev/1d7748535fa3
(Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density. r=snorp

Comment 88

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Android xpcshell X3 crash while running test_ext_alarms.js (this also showed up in the Try push):

https://hg.mozilla.org/integration/fx-team/rev/ef345e3b5fa9fabdfd7d0a93ff01ec9c085968df
https://hg.mozilla.org/integration/fx-team/rev/54ed443a6951acc52d169114d5989e61c672d644
https://hg.mozilla.org/integration/fx-team/rev/1196bb89841328837886df509bb2eca1f8c619d6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=1d7748535fa370332623ac29e50dd19e1ef6133b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11837842&repo=fx-team

03:08:16     INFO -  TEST-START | toolkit/components/extensions/test/xpcshell/test_ext_alarms.js
03:08:31  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/xpcshell/test_ext_alarms.js | xpcshell return code: 139
03:08:31     INFO -  TEST-INFO took 15033ms
03:08:31     INFO -  >>>>>>>
03:08:31     INFO -  PROCESS | toolkit/components/extensions/test/xpcshell/test_ext_alarms.js | xpcw: cd /storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell
03:08:31     INFO -  PROCESS | toolkit/components/extensions/test/xpcshell/test_ext_alarms.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/fennec-52.0a1.en-US.android-arm.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head.js"]; -e const _TAIL_FILES = []; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_ext_alarms.js"]; -e const _TEST_NAME = "toolkit/components/extensions/test/xpcshell/test_ext_alarms.js" -e _execute_test(); quit(0);
03:08:31     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
03:08:31     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
03:08:31     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
03:08:31     INFO -  running event loop
03:08:31     INFO -  toolkit/components/extensions/test/xpcshell/test_ext_alarms.js | Starting test_alarm_without_permissions
03:08:31     INFO -  (xpcshell/head.js) | test test_alarm_without_permissions pending (2)
03:08:31     INFO -  "Extension loaded"
03:08:31     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
03:08:31     INFO -  PROCESS | toolkit/components/extensions/test/xpcshell/test_ext_alarms.js | Segmentation fault
03:08:31     INFO -  PROCESS | toolkit/components/extensions/test/xpcshell/test_ext_alarms.js | 13
03:08:31     INFO -  <<<<<<<
03:08:32     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/f7XYrtCDTOW7AntJL2TnyQ/artifacts/public/build/fennec-52.0a1.en-US.android-arm.crashreporter-symbols.zip
03:08:48     INFO -  mozcrash Copy/paste: /builds/slave/test/build/linux64-minidump_stackwalk /tmp/tmpX2G4MH/0067e14f-528d-5553-0b9185a6-7682c1c1.dmp /tmp/tmpkhYFvM
03:09:02     INFO -  mozcrash Saved minidump as /builds/slave/test/build/blobber_upload_dir/0067e14f-528d-5553-0b9185a6-7682c1c1.dmp
03:09:02     INFO -  mozcrash Saved app info as /builds/slave/test/build/blobber_upload_dir/0067e14f-528d-5553-0b9185a6-7682c1c1.extra
03:09:02  WARNING -  PROCESS-CRASH | toolkit/components/extensions/test/xpcshell/test_ext_alarms.js | application crashed [@ mozilla::jni::GetEnvForThread]
03:09:02     INFO -  Crash dump filename: /tmp/tmpX2G4MH/0067e14f-528d-5553-0b9185a6-7682c1c1.dmp
03:09:02     INFO -  Operating system: Android
03:09:02     INFO -                    0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l
03:09:02     INFO -  CPU: arm
03:09:02     INFO -       ARMv7 ARM Cortex-A8 features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3
03:09:02     INFO -       1 CPU
03:09:02     INFO -  GPU: UNKNOWN
03:09:02     INFO -  Crash reason:  SIGSEGV
03:09:02     INFO -  Crash address: 0x0
03:09:02     INFO -  Process uptime: not available
03:09:02     INFO -  Thread 0 (crashed)
03:09:02     INFO -   0  libxul.so!mozilla::jni::GetEnvForThread [jni.h:1d7748535fa3 : 1091 + 0x0]
03:09:02     INFO -       r0 = 0x00000000    r1 = 0xbeea53fc    r2 = 0x00000000    r3 = 0x427011e8
03:09:02     INFO -       r4 = 0x00000000    r5 = 0x427011e4    r6 = 0x422a2b00    r7 = 0x422a2b00
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x422a2b00   r10 = 0x4205c25d   r12 = 0x426b8110
03:09:02     INFO -       fp = 0x43d16080    sp = 0xbeea53f8    lr = 0x412d3e59    pc = 0x412d3e68
03:09:02     INFO -      Found by: given as instruction pointer in context
03:09:02     INFO -   1  libxul.so!nsScreenManagerAndroid::nsScreenManagerAndroid [Refs.h:1d7748535fa3 : 69 + 0x3]
03:09:02     INFO -       r4 = 0x47cdeec0    r5 = 0xbeea5498    r6 = 0x422a2b00    r7 = 0x422a2b00
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x422a2b00   r10 = 0x4205c25d    fp = 0x43d16080
03:09:02     INFO -       sp = 0xbeea5410    pc = 0x412cbda1
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   2  libxul.so!nsScreenManagerAndroidConstructor [nsWidgetFactory.cpp:1d7748535fa3 : 34 + 0x3]
03:09:02     INFO -       r4 = 0x47cdeec0    r5 = 0xbeea5498    r6 = 0x422a2b00    r7 = 0x422a2b00
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x422a2b00   r10 = 0x4205c25d    fp = 0x43d16080
03:09:02     INFO -       sp = 0xbeea5450    pc = 0x412cbe61
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   3  libxul.so!nsComponentManagerImpl::CreateInstanceByContractID [nsComponentManager.cpp:1d7748535fa3 : 1170 + 0x9]
03:09:02     INFO -       r4 = 0x4061f3b1    r5 = 0x4205c25d    r6 = 0xbeea5498    r7 = 0x422a2b00
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x422a2b00   r10 = 0x4205c25d    fp = 0x43d16080
03:09:02     INFO -       sp = 0xbeea5460    pc = 0x4060d72d
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   4  libxul.so!nsComponentManagerImpl::GetServiceByContractID [nsComponentManager.cpp:1d7748535fa3 : 1527 + 0xd]
03:09:02     INFO -       r4 = 0x43da8920    r5 = 0x43d441a0    r6 = 0x00000000    r7 = 0x43d44214
03:09:02     INFO -       r8 = 0xbeea54ec    r9 = 0x422a2b00   r10 = 0x4205c25d    fp = 0x43d16080
03:09:02     INFO -       sp = 0xbeea5488    pc = 0x4060e41b
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   5  libxul.so!nsGetServiceByContractID::operator() [nsComponentManagerUtils.cpp:1d7748535fa3 : 280 + 0x5]
03:09:02     INFO -       r4 = 0xbeea54ec    r5 = 0x00000000    r6 = 0x46fb97e0    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
03:09:02     INFO -       sp = 0xbeea54d8    pc = 0x40625b8b
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   6  libxul.so!nsCOMPtr_base::assign_from_gs_contractid [nsCOMPtr.cpp:1d7748535fa3 : 95 + 0x7]
03:09:02     INFO -       r4 = 0xbeea5500    r5 = 0x00000000    r6 = 0x46fb97e0    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
03:09:02     INFO -       sp = 0xbeea54e0    pc = 0x4062133b
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   7  libxul.so!gfxPlatform::PopulateScreenInfo [nsCOMPtr.h:1d7748535fa3 : 548 + 0x5]
03:09:02     INFO -       r4 = 0xbeea5500    r5 = 0x00000000    r6 = 0x46fb97e0    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
03:09:02     INFO -       sp = 0xbeea54f8    pc = 0x40af75bf
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   8  libxul.so!gfxPlatform::Init [gfxPlatform.cpp:1d7748535fa3 : 687 + 0x5]
03:09:02     INFO -       r4 = 0xbeea5554    r5 = 0x426ef544    r6 = 0x46fb97e0    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
03:09:02     INFO -       sp = 0xbeea5528    pc = 0x40af9211
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -   9  libxul.so!gfxPlatform::GetPlatform [gfxPlatform.cpp:1d7748535fa3 : 518 + 0x3]
03:09:02     INFO -       r4 = 0x47c6e400    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x00000001
03:09:02     INFO -       r8 = 0xbeea5608    r9 = 0xbeea5668   r10 = 0x00000001    fp = 0xbeea5710
03:09:02     INFO -       sp = 0xbeea55d8    pc = 0x40af955f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  10  libxul.so!mozilla::widget::PuppetWidget::InfallibleCreate [PuppetWidget.cpp:1d7748535fa3 : 119 + 0x3]
03:09:02     INFO -       r3 = 0x00000000    r4 = 0x47c6e400    r5 = 0x00000000    r6 = 0x00000000
03:09:02     INFO -       r7 = 0x00000001    r8 = 0xbeea5608    r9 = 0xbeea5668   r10 = 0x00000001
03:09:02     INFO -       fp = 0xbeea5710    sp = 0xbeea55e0    pc = 0x412ab4cd
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  11  libxul.so!mozilla::widget::PuppetWidget::Create [PuppetWidget.cpp:1d7748535fa3 : 145 + 0x7]
03:09:02     INFO -       r4 = 0x00000000    r5 = 0x00000000    r6 = 0xbeea5650    r7 = 0xbeea5654
03:09:02     INFO -       r8 = 0xbeea5658    r9 = 0xbeea5668   r10 = 0x00000001    fp = 0xbeea5710
03:09:02     INFO -       sp = 0xbeea5628    pc = 0x412ab5e3
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  12  libxul.so!nsAppShellService::CreateWindowlessBrowser [nsAppShellService.cpp:1d7748535fa3 : 530 + 0x3]
03:09:02     INFO -       r4 = 0x474b1ac0    r5 = 0x00000000    r6 = 0xbeea5650    r7 = 0xbeea5654
03:09:02     INFO -       r8 = 0xbeea5658    r9 = 0xbeea5668   r10 = 0x00000001    fp = 0xbeea5710
03:09:02     INFO -       sp = 0xbeea5638    pc = 0x415b29cf
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  13  libxul.so!NS_InvokeByIndex [xptcinvoke_arm.cpp:1d7748535fa3 : 169 + 0x11]
03:09:02     INFO -       r4 = 0xbeea56b0    r5 = 0x415b28f9    r6 = 0x00000004    r7 = 0xbeea56b8
03:09:02     INFO -       r8 = 0x00000012    r9 = 0xbeea5768   r10 = 0x00000002    fp = 0xbeea5710
03:09:02     INFO -       sp = 0xbeea56a0    pc = 0x4061a831
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  14  libxul.so!XPCWrappedNative::CallMethod [XPCWrappedNative.cpp:1d7748535fa3 : 2064 + 0xf]
03:09:02     INFO -       r3 = 0xbeea57a8    r4 = 0xbeea57b8    r5 = 0x00000002    r6 = 0x00000010
03:09:02     INFO -       r7 = 0x00000001    r8 = 0x00000012    r9 = 0xbeea5768   r10 = 0x00000002
03:09:02     INFO -       fp = 0xbeea5710    sp = 0xbeea56d8    pc = 0x4093f717
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  15  libxul.so!XPC_WN_CallMethod [XPCWrappedNativeJSOps.cpp:1d7748535fa3 : 1143 + 0x7]
03:09:02     INFO -       r4 = 0x00000001    r5 = 0xbeea58a4    r6 = 0xbeea5884    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x47bfa920    r9 = 0x46f556c8   r10 = 0x00000001    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5860    pc = 0x40943557
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  16  libxul.so!js::InternalCallOrConstruct [jscntxtinlines.h:1d7748535fa3 : 239 + 0x3]
03:09:02     INFO -       r4 = 0x46f556d8    r5 = 0x4662c000    r6 = 0xbeea5b50    r7 = 0x4094345d
03:09:02     INFO -       r8 = 0x4748ac00    r9 = 0xbeea5bc8   r10 = 0xbeea5ab4    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5918    pc = 0x41c5de83
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  17  libxul.so!Interpret [Interpreter.cpp:1d7748535fa3 : 509 + 0x7]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0x4261562c    r6 = 0x00000000    r7 = 0xffffff8c
03:09:02     INFO -       r8 = 0x46f55678    r9 = 0xbeea5bc8   r10 = 0xbeea5ab4    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5970    pc = 0x41c55c29
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  18  libxul.so!js::RunScript [Interpreter.cpp:1d7748535fa3 : 404 + 0x7]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0xbeea5cb0    r6 = 0xbeea5c78    r7 = 0xbeea5d68
03:09:02     INFO -       r8 = 0x47e1a340    r9 = 0x46d27400   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5c70    pc = 0x41c5dbe7
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  19  libxul.so!js::InternalCallOrConstruct [Interpreter.cpp:1d7748535fa3 : 476 + 0x7]
03:09:02     INFO -       r4 = 0x00000200    r5 = 0x4662c000    r6 = 0xbeea5d68    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x00000003    r9 = 0x46d27400   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5c98    pc = 0x41c5dd4d
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  20  libxul.so!js::Call [Interpreter.cpp:1d7748535fa3 : 522 + 0x3]
03:09:02     INFO -       r4 = 0xbeea5d68    r5 = 0xbeea5d98    r6 = 0x47a7d288    r7 = 0xffffff8c
03:09:02     INFO -       r8 = 0x00000003    r9 = 0x46d27400   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5cf0    pc = 0x41c5e17f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  21  libxul.so!js::Wrapper::call [Wrapper.cpp:1d7748535fa3 : 165 + 0x7]
03:09:02     INFO -       r4 = 0x46f555d0    r5 = 0xbeea5d98    r6 = 0x00000001    r7 = 0xbeea5e38
03:09:02     INFO -       r8 = 0x00000003    r9 = 0x46d27400   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5d00    pc = 0x41bf3319
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  22  libxul.so!js::CrossCompartmentWrapper::call [CrossCompartmentWrapper.cpp:1d7748535fa3 : 333 + 0xb]
03:09:02     INFO -       r4 = 0x00000001    r5 = 0x4662c000    r6 = 0xbeea5e38    r7 = 0xbeea5e4c
03:09:02     INFO -       r8 = 0x42613984    r9 = 0x46d27400   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5df8    pc = 0x41be6641
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  23  libxul.so!js::proxy_Call [Proxy.cpp:1d7748535fa3 : 400 + 0xd]
03:09:02     INFO -       r4 = 0x41be6559    r5 = 0x42613b70    r6 = 0xbeea5e4c    r7 = 0x42613984
03:09:02     INFO -       r8 = 0x4271b8b8    r9 = 0x46d27400   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5e28    pc = 0x41bead1f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  24  libxul.so!js::InternalCallOrConstruct [jscntxtinlines.h:1d7748535fa3 : 239 + 0x3]
03:09:02     INFO -       r4 = 0x46f555f0    r5 = 0x4662c000    r6 = 0xbeea60a0    r7 = 0xbeea5e80
03:09:02     INFO -       r8 = 0x41beac9d    r9 = 0x46d27400   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5e68    pc = 0x41c5dfcb
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  25  libxul.so!Interpret [Interpreter.cpp:1d7748535fa3 : 509 + 0x7]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0x46f555e8    r6 = 0x00000000    r7 = 0xffffff8c
03:09:02     INFO -       r8 = 0x46f55580    r9 = 0xffffff8c   r10 = 0xbeea6004    fp = 0xbeea6118
03:09:02     INFO -       sp = 0xbeea5ec0    pc = 0x41c55c29
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  26  libxul.so!js::RunScript [Interpreter.cpp:1d7748535fa3 : 404 + 0x7]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0xbeea6200    r6 = 0xbeea61c8    r7 = 0xbeea62b8
03:09:02     INFO -       r8 = 0x465fd040    r9 = 0x4748ac00   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea61c0    pc = 0x41c5dbe7
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  27  libxul.so!js::InternalCallOrConstruct [Interpreter.cpp:1d7748535fa3 : 476 + 0x7]
03:09:02     INFO -       r4 = 0x00000001    r5 = 0x4662c000    r6 = 0xbeea62b8    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x00000002    r9 = 0x4748ac00   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea61e8    pc = 0x41c5dd4d
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  28  libxul.so!js::Call [Interpreter.cpp:1d7748535fa3 : 522 + 0x3]
03:09:02     INFO -       r4 = 0xbeea62b8    r5 = 0xbeea62e8    r6 = 0x47abce90    r7 = 0xffffff8c
03:09:02     INFO -       r8 = 0x00000002    r9 = 0x4748ac00   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea6240    pc = 0x41c5e17f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  29  libxul.so!js::Wrapper::call [Wrapper.cpp:1d7748535fa3 : 165 + 0x7]
03:09:02     INFO -       r4 = 0x46f55450    r5 = 0xbeea62e8    r6 = 0x00000000    r7 = 0xbeea6388
03:09:02     INFO -       r8 = 0x00000002    r9 = 0x4748ac00   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea6250    pc = 0x41bf3319
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  30  libxul.so!js::CrossCompartmentWrapper::call [CrossCompartmentWrapper.cpp:1d7748535fa3 : 333 + 0xb]
03:09:02     INFO -       r4 = 0x00000000    r5 = 0x4662c000    r6 = 0xbeea6388    r7 = 0xbeea639c
03:09:02     INFO -       r8 = 0x42613984    r9 = 0x4748ac00   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea6348    pc = 0x41be6641
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  31  libxul.so!js::proxy_Call [Proxy.cpp:1d7748535fa3 : 400 + 0xd]
03:09:02     INFO -       r4 = 0x41be6559    r5 = 0x42613b70    r6 = 0xbeea639c    r7 = 0x42613984
03:09:02     INFO -       r8 = 0x4271b8b8    r9 = 0x4748ac00   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea6378    pc = 0x41bead1f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  32  libxul.so!js::InternalCallOrConstruct [jscntxtinlines.h:1d7748535fa3 : 239 + 0x3]
03:09:02     INFO -       r4 = 0x46f55468    r5 = 0x4662c000    r6 = 0xbeea65f0    r7 = 0xbeea63d0
03:09:02     INFO -       r8 = 0x41beac9d    r9 = 0x4748ac00   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea63b8    pc = 0x41c5dfcb
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  33  libxul.so!Interpret [Interpreter.cpp:1d7748535fa3 : 509 + 0x7]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0x4261562c    r6 = 0x00000000    r7 = 0xffffff8c
03:09:02     INFO -       r8 = 0x46f55408    r9 = 0xffffff8c   r10 = 0xbeea6554    fp = 0xbeea6668
03:09:02     INFO -       sp = 0xbeea6410    pc = 0x41c55c29
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  34  libxul.so!js::RunScript [Interpreter.cpp:1d7748535fa3 : 404 + 0x7]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0xbeea6750    r6 = 0xbeea6718    r7 = 0xbeea6808
03:09:02     INFO -       r8 = 0x473e56a0    r9 = 0x4745c800   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea6710    pc = 0x41c5dbe7
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  35  libxul.so!js::InternalCallOrConstruct [Interpreter.cpp:1d7748535fa3 : 476 + 0x7]
03:09:02     INFO -       r4 = 0x00000001    r5 = 0x4662c000    r6 = 0xbeea6808    r7 = 0x00000000
03:09:02     INFO -       r8 = 0x00000006    r9 = 0x4745c800   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea6738    pc = 0x41c5dd4d
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  36  libxul.so!js::Call [Interpreter.cpp:1d7748535fa3 : 522 + 0x3]
03:09:02     INFO -       r4 = 0xbeea6808    r5 = 0xbeea6838    r6 = 0x00000000    r7 = 0xffffff82
03:09:02     INFO -       r8 = 0x00000006    r9 = 0x4745c800   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea6790    pc = 0x41c5e17f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  37  libxul.so!js::Wrapper::call [Wrapper.cpp:1d7748535fa3 : 165 + 0x7]
03:09:02     INFO -       r4 = 0xbeea6a18    r5 = 0xbeea6838    r6 = 0x00000004    r7 = 0xbeea68d8
03:09:02     INFO -       r8 = 0x00000006    r9 = 0x4745c800   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea67a0    pc = 0x41bf3319
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  38  libxul.so!js::CrossCompartmentWrapper::call [CrossCompartmentWrapper.cpp:1d7748535fa3 : 333 + 0xb]
03:09:02     INFO -       r4 = 0x00000004    r5 = 0x4662c000    r6 = 0xbeea68d8    r7 = 0xbeea68ec
03:09:02     INFO -       r8 = 0x42613984    r9 = 0x4745c800   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea6898    pc = 0x41be6641
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  39  libxul.so!js::proxy_Call [Proxy.cpp:1d7748535fa3 : 400 + 0xd]
03:09:02     INFO -       r4 = 0x41be6559    r5 = 0x42613b70    r6 = 0xbeea68ec    r7 = 0x42613984
03:09:02     INFO -       r8 = 0x4271b8b8    r9 = 0x4745c800   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea68c8    pc = 0x41bead1f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  40  libxul.so!js::InternalCallOrConstruct [jscntxtinlines.h:1d7748535fa3 : 239 + 0x3]
03:09:02     INFO -       r4 = 0xbeea6a50    r5 = 0x4662c000    r6 = 0xbeea69e8    r7 = 0xbeea6920
03:09:02     INFO -       r8 = 0x41beac9d    r9 = 0x4745c800   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea6908    pc = 0x41c5dfcb
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  41  libxul.so!js::Call [Interpreter.cpp:1d7748535fa3 : 522 + 0x3]
03:09:02     INFO -       r4 = 0xbeea69e8    r5 = 0x46f0f8c7    r6 = 0x00000000    r7 = 0xffffff82
03:09:02     INFO -       r8 = 0xbeea6bcc    r9 = 0xbeea6a18   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea6960    pc = 0x41c5e17f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  42  libxul.so!js::SpreadCallOperation [Interpreter.cpp:1d7748535fa3 : 4802 + 0xf]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0x46f0f8c7    r6 = 0x46f553d0    r7 = 0x46f553d8
03:09:02     INFO -       r8 = 0xbeea6bcc    r9 = 0xbeea6a18   r10 = 0x00000029    fp = 0x00000004
03:09:02     INFO -       sp = 0xbeea6970    pc = 0x41c5eb7b
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  43  libxul.so!Interpret [Interpreter.cpp:1d7748535fa3 : 2876 + 0x5]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0xbeea6c98    r6 = 0x00000000    r7 = 0x46f553e0
03:09:02     INFO -       r8 = 0x47231700    r9 = 0xbeea6eb8   r10 = 0xbeea6bc4    fp = 0x00000005
03:09:02     INFO -       sp = 0xbeea6a80    pc = 0x41c55b5d
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  44  libxul.so!js::RunScript [Interpreter.cpp:1d7748535fa3 : 404 + 0x7]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0xbeea6dc0    r6 = 0xbeea6d88    r7 = 0xbeea6e88
03:09:02     INFO -       r8 = 0x47231700    r9 = 0xbeea6eb8   r10 = 0x00000029    fp = 0x00000005
03:09:02     INFO -       sp = 0xbeea6d80    pc = 0x41c5dbe7
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  45  libxul.so!js::InternalCallOrConstruct [Interpreter.cpp:1d7748535fa3 : 476 + 0x7]
03:09:02     INFO -       r4 = 0x00000001    r5 = 0x4662c000    r6 = 0xbeea6e88    r7 = 0x00000000
03:09:02     INFO -       r8 = 0xbeea6f54    r9 = 0xbeea6eb8   r10 = 0x00000029    fp = 0x00000005
03:09:02     INFO -       sp = 0xbeea6da8    pc = 0x41c5dd4d
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  46  libxul.so!js::Call [Interpreter.cpp:1d7748535fa3 : 522 + 0x3]
03:09:02     INFO -       r4 = 0xbeea6e88    r5 = 0x46fb9b84    r6 = 0x00000000    r7 = 0xffffff82
03:09:02     INFO -       r8 = 0xbeea6f54    r9 = 0xbeea6eb8   r10 = 0x00000029    fp = 0x00000005
03:09:02     INFO -       sp = 0xbeea6e00    pc = 0x41c5e17f
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  47  libxul.so!js::SpreadCallOperation [Interpreter.cpp:1d7748535fa3 : 4802 + 0xf]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0x46fb9b84    r6 = 0xbeea6f60    r7 = 0xbeea6f70
03:09:02     INFO -       r8 = 0xbeea6f54    r9 = 0xbeea6eb8   r10 = 0x00000029    fp = 0x00000005
03:09:02     INFO -       sp = 0xbeea6e10    pc = 0x41c5eb7b
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  48  libxul.so!js::jit::DoSpreadCallFallback [BaselineIC.cpp:1d7748535fa3 : 6078 + 0x1b]
03:09:02     INFO -       r4 = 0x4662c000    r5 = 0x46fb9b84    r6 = 0x00000001    r7 = 0x47d432d0
03:09:02     INFO -       r8 = 0x00000000    r9 = 0xbeea7060   r10 = 0x00000029    fp = 0xbeea6fa8
03:09:02     INFO -       sp = 0xbeea6f20    pc = 0x41a42b9d
03:09:02     INFO -      Found by: call frame info
03:09:02     INFO -  49  0x43b586da
03:09:02     INFO -       r4 = 0xbeea6ffc    r5 = 0xbeea7014    r6 = 0x00000002    r7 = 0xffffff81
03:09:02     INFO -       r8 = 0x00002844    r9 = 0x47d432d0   r10 = 0x00000002    fp = 0xbeea7038
03:09:02     INFO -       sp = 0xbeea6ff0    pc = 0x43b586dc
03:09:02     INFO -      Found by: call frame info

Comment 89

[Tommy Kuo (away forever...)]

I'm trying to fix it.

Comment 90

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/6-7/

Comment 91

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/16-17/

Comment 92

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/7-8/

Comment 93

[Carsten Book [:Tomcat]]

does not apply cleanly:

603330ba03a7 transplanted to c21c53c9d0f9
applying cf23ab8a50c7
patching file mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
Hunk #3 FAILED at 110
Hunk #4 FAILED at 227
2 out of 4 hunks FAILED -- saving rejects to file mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java.rej
patching file widget/android/GeneratedJNIWrappers.h
Hunk #1 FAILED at 3269
1 out of 1 hunks FAILED -- saving rejects to file widget/android/GeneratedJNIWrappers.h.rej
patching file widget/android/nsWindow.cpp
Hunk #2 FAILED at 341
Hunk #3 FAILED at 1318
2 out of 7 hunks FAILED -- saving rejects to file widget/android/nsWindow.cpp.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 94

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/7-8/

Comment 95

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/17-18/

Comment 96

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/8-9/

Comment 97

[Tommy Kuo (away forever...)]

Hi snorp, I found when I called JNI function when nsScreenManagerAndroid is initing, the crash happens. Did you have any experience of that? Is JVM not already at that moment? The log is following: https://treeherder.mozilla.org/logviewer.html#?job_id=29173838&repo=try#L1921

Comment 98

[(inactive) Jim Chen [:jchen] [:darchons]]

xpcshell tests do not run inside a Java environment, so you need to check for Java support by calling `mozilla::jni::IsAvailable()` (e.g. [1])

[1] https://dxr.mozilla.org/mozilla-central/rev/9079d167112122805f99f57bb8856e1b1675af0f/widget/android/nsScreenManagerAndroid.cpp#61

Comment 99

[Tommy Kuo (away forever...)]

Comment on attachment 8796070 [details]
Bug 1282003 - (Part 1) Add display type/density info and addScreen/removeScreen function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82054/diff/8-9/

Comment 100

[Tommy Kuo (away forever...)]

Comment on attachment 8772714 [details]
Bug 1282003 - (Part 2) Add screenManagerHelper in Java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65480/diff/18-19/

Comment 101

[Tommy Kuo (away forever...)]

Comment on attachment 8796101 [details]
Bug 1282003 - (Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82076/diff/9-10/

Comment 102

[Tommy Kuo (away forever...)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73fc4fa8c15cde5f678bfdd16c5568289594abba

Comment 103

[Ryan VanderMeulen [:RyanVM]]

Please mark the pending issues as resolved in MozReview so that autoland can push it.

Comment 104

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6be3aa402016
(Part 1) Add display type/density info and addScreen/removeScreen function. r=snorp
https://hg.mozilla.org/integration/autoland/rev/d9148f4b2ef5
(Part 2) Add screenManagerHelper in Java. r=snorp
https://hg.mozilla.org/integration/autoland/rev/44bae97b9e6f
(Part 3) Add screen ID to nsWindow and GeckoView to get the correct nsScreen and density. r=snorp

Comment 105

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6be3aa402016
https://hg.mozilla.org/mozilla-central/rev/d9148f4b2ef5
https://hg.mozilla.org/mozilla-central/rev/44bae97b9e6f