1157874 - Remove screen related global variables from nsWindow.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Sotaro Ikeda [:sotaro]]

nsWindow.cpp uses some global variables like the following. It seems better to move them to nsScreenGonk or nsScreenManagerGonk.


> nsIntRect gScreenBounds;
> static uint32_t sScreenRotation;
> static uint32_t sPhysicalScreenRotation;
> static nsIntRect sVirtualBounds;

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

The patch passed compiling, but it does not work.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Fix crash during booting.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove screen related global variables from nsWindow.cpp

Some clean up.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove screen related global variables from nsWindow.cpp

Update nits.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove screen related global variables from nsWindow.cpp

Update nits.

Comment 6

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0231373170fe

Comment 7

[Sotaro Ikeda [:sotaro]]

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

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 8597365 [details] [diff] [review]
patch - Remove screen related global variables from nsWindow.cpp

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

Looks about right overall - I just have a nit and a few suggestions for simplifying things.

::: widget/gonk/nsAppShell.cpp
@@ +575,5 @@
>                    DISPLAY_ORIENTATION_270,
>                    "Orientation enums not matched!");
>  
> +    nsCOMPtr<nsIScreen> screen = nsScreenManagerGonk::GetPrimaryScreen();
> +    MOZ_ASSERT(screen);

Can we assert/crash in GetPrimaryScreen instead? Seems like that shouldn't return null.

::: widget/gonk/nsScreenManagerGonk.h
@@ +54,5 @@
> +    {
> +      return mTopWindows;
> +    }
> +
> +    nsIntRect mScreenBounds;

Seems like some or all of these members can be protected or private.

::: widget/gonk/nsWindow.cpp
@@ +730,5 @@
>          MOZ_CRASH("nsWindow::StartRemoteDrawing failed in CreateDrawTargetForData");
>      }
>      if (!mBackBuffer ||
>          mBackBuffer->GetSize() != mFramebufferTarget->GetSize() ||
> +        mBackBuffer->GetFormat() != mFramebufferTarget->GetFormat())

Keep the { on this line, here and elsewhere.

@@ +1094,5 @@
>  nsScreenManagerGonk::~nsScreenManagerGonk()
>  {
>  }
>  
> +/* static */ already_AddRefed<nsIScreenManager>

Can we return nsScreenManagerGonk here?

@@ +1102,5 @@
> +    screenManager = do_GetService("@mozilla.org/gfx/screenmanager;1");
> +    return screenManager.forget();
> +}
> +
> +/* static */ already_AddRefed<nsIScreen>

Same here - can we return nsScreenGonk instead?

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #8)
> Comment on attachment 8597365 [details] [diff] [review]
> patch - Remove screen related global variables from nsWindow.cpp
> 
> Review of attachment 8597365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks about right overall - I just have a nit and a few suggestions for
> simplifying things.
> 
> ::: widget/gonk/nsAppShell.cpp
> @@ +575,5 @@
> >                    DISPLAY_ORIENTATION_270,
> >                    "Orientation enums not matched!");
> >  
> > +    nsCOMPtr<nsIScreen> screen = nsScreenManagerGonk::GetPrimaryScreen();
> > +    MOZ_ASSERT(screen);
> 
> Can we assert/crash in GetPrimaryScreen instead? Seems like that shouldn't
> return null.

Yes, I will update in a next patch.

> 
> ::: widget/gonk/nsScreenManagerGonk.h
> @@ +54,5 @@
> > +    {
> > +      return mTopWindows;
> > +    }
> > +
> > +    nsIntRect mScreenBounds;
> 
> Seems like some or all of these members can be protected or private.

Oh, I forgot to it. It will be updated in a next patch.

> 
> ::: widget/gonk/nsWindow.cpp
> @@ +730,5 @@
> >          MOZ_CRASH("nsWindow::StartRemoteDrawing failed in CreateDrawTargetForData");
> >      }
> >      if (!mBackBuffer ||
> >          mBackBuffer->GetSize() != mFramebufferTarget->GetSize() ||
> > +        mBackBuffer->GetFormat() != mFramebufferTarget->GetFormat())
> 
> Keep the { on this line, here and elsewhere.

I misunderstood about it. In the past review, I requested to move { to next line if a condition check is multiple line. But it was my misunderstanding :-( I checked it to :Ehsan and he answered your comment is correct. I am going to update it in a next patch. 

> 
> @@ +1094,5 @@
> >  nsScreenManagerGonk::~nsScreenManagerGonk()
> >  {
> >  }
> >  
> > +/* static */ already_AddRefed<nsIScreenManager>
> 
> Can we return nsScreenManagerGonk here?

I am going to update it in a next patch.

> 
> @@ +1102,5 @@
> > +    screenManager = do_GetService("@mozilla.org/gfx/screenmanager;1");
> > +    return screenManager.forget();
> > +}
> > +
> > +/* static */ already_AddRefed<nsIScreen>
> 
> Same here - can we return nsScreenGonk instead?

I am going to update it in a next patch.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove screen related global variables from nsWindow.cpp

Apply the comment.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove screen related global variables from nsWindow.cpp

Update nsAppShell::NotifyScreenRotation(). I forgot to update it.

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 8598830 [details] [diff] [review]
patch - Remove screen related global variables from nsWindow.cpp

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

Looks good. Just need some adjustments to take advantage of using concrete classes.

::: hal/gonk/GonkHal.cpp
@@ +985,5 @@
>  
>  void
>  GetCurrentScreenConfiguration(hal::ScreenConfiguration* aScreenConfiguration)
>  {
> +  nsCOMPtr<nsScreenGonk> screen = nsScreenManagerGonk::GetPrimaryScreen();

For concrete classes, nsRefPtr or RefPtr should be used since it has less overhead.

::: widget/gonk/nsAppShell.cpp
@@ +574,5 @@
>      static_assert(nsIScreen::ROTATION_270_DEG ==
>                    DISPLAY_ORIENTATION_270,
>                    "Orientation enums not matched!");
>  
> +    nsCOMPtr<nsScreenGonk> screen = nsScreenManagerGonk::GetPrimaryScreen();

RefPtr

@@ +1059,5 @@
>  {
>      gAppShell->mReaderPolicy->setDisplayInfo();
>      gAppShell->mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_DISPLAY_INFO);
>  
> +    nsCOMPtr<nsScreenGonk> screen = nsScreenManagerGonk::GetPrimaryScreen();

RefPtr

::: widget/gonk/nsScreenManagerGonk.h
@@ +51,5 @@
> +    void BringToTop(nsWindow* aWindow);
> +
> +    const nsTArray<nsWindow*>& GetTopWindows() const
> +    {
> +      return mTopWindows;

nit: 4 space indent

::: widget/gonk/nsWindow.cpp
@@ +88,5 @@
>          } else {
>            ProcessPriorityManager::Freeze();
>          }
>  
> +        nsCOMPtr<nsScreenGonk> screen = nsScreenManagerGonk::GetPrimaryScreen();

RefPtr

@@ +119,4 @@
>  static void
>  displayEnabledCallback(bool enabled)
>  {
> +    nsCOMPtr<nsScreenManagerGonk> screenManager = nsScreenManagerGonk::GetInstance();

RefPtr

@@ +130,5 @@
>  nsWindow::nsWindow()
>  {
>      mFramebuffer = nullptr;
>  
> +    nsCOMPtr<nsScreenManagerGonk> screenManager = nsScreenManagerGonk::GetInstance();

RefPtr

@@ +159,5 @@
>          gDrawRequest = true;
>          return;
>      }
>  
> +    nsCOMPtr<nsScreenGonk> screen = nsScreenManagerGonk::GetPrimaryScreen();

RefPtr

@@ +1089,5 @@
> +
> +/* static */ already_AddRefed< nsScreenGonk>
> +nsScreenManagerGonk::GetPrimaryScreen()
> +{
> +    nsCOMPtr<nsScreenManagerGonk> manager = nsScreenManagerGonk::GetInstance();

RefPtr

::: widget/gonk/nsWindow.h
@@ +155,5 @@
>      // This is used by SynthesizeNativeTouchPoint to maintain state between
>      // multiple synthesized points
>      nsAutoPtr<mozilla::MultiTouchInput> mSynthesizedTouchInput;
> +
> +    nsCOMPtr<nsScreenGonk> mScreen;

RefPtr

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove screen related global variables from nsWindow.cpp

Apply the comments. Carry "r=mwu".

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

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

Comment 16

[Sotaro Ikeda [:sotaro]]

waiting m-i open.

Comment 17

[Henry Chang [:hchang]]

Comment on attachment 8598871 [details] [diff] [review]
patch - Remove screen related global variables from nsWindow.cpp

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

::: widget/gonk/nsScreenManagerGonk.h
@@ +59,5 @@
> +    nsIntRect mScreenBounds;
> +    nsIntRect mVirtualBounds;
> +    uint32_t mScreenRotation;
> +    uint32_t mPhysicalScreenRotation;
> +    nsTArray<nsWindow*> mTopWindows;

Sorry for jumping in but I am wondering in what situation one screen would have multiple top level windows? Thanks!

Comment 18

[Henry Chang [:hchang]]

Comment on attachment 8598871 [details] [diff] [review]
patch - Remove screen related global variables from nsWindow.cpp

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

::: widget/gonk/nsScreenManagerGonk.h
@@ +71,5 @@
>      NS_DECL_ISUPPORTS
>      NS_DECL_NSISCREENMANAGER
>  
> +    static already_AddRefed<nsScreenManagerGonk> GetInstance();
> +    static already_AddRefed<nsScreenGonk> GetPrimaryScreen();

Another question here. Returning an "already_AddRefed object" makes us unable to code like this "nsScreenManagerGonk::GetInstance()->Foo()". What do you think if we change it to returning a RefPtr?

Comment 19

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #17)
> Comment on attachment 8598871 [details] [diff] [review]
> patch - Remove screen related global variables from nsWindow.cpp
> 
> Review of attachment 8598871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nsScreenManagerGonk.h
> @@ +59,5 @@
> > +    nsIntRect mScreenBounds;
> > +    nsIntRect mVirtualBounds;
> > +    uint32_t mScreenRotation;
> > +    uint32_t mPhysicalScreenRotation;
> > +    nsTArray<nsWindow*> mTopWindows;
> 
> Sorry for jumping in but I am wondering in what situation one screen would
> have multiple top level windows? Thanks!

On current B2G it wouldn't happen but I think it still makes sense that
one screen has multiple top level window. Please ignore this question.
Sorry about that :p

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37018cb81886

Comment 21

[Sotaro Ikeda [:sotaro]]

Even on b2g, one primary display has at most 3 nsWindow instances. In the following diagram, you can find 3 nsWindows. And only one nsWindow creates LayerManager.
https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/widget_nsWindow_FirefoxOS_2_2.pdf

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> Even on b2g, one primary display has at most 3 nsWindow instances. In the
> following diagram, you can find 3 nsWindows. And only one nsWindow creates
> LayerManager.
> https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/
> widget_nsWindow_FirefoxOS_2_2.pdf

one nsWindow is for blank document and it is destroyed soon by nsWindow::Destroy() during creating nsWindow for shall.html.

Comment 23

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> Even on b2g, one primary display has at most 3 nsWindow instances. In the
> following diagram, you can find 3 nsWindows. And only one nsWindow creates
> LayerManager.
> https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/
> widget_nsWindow_FirefoxOS_2_2.pdf

As in the diagram, other 2 nsWindow is created by kCChildCID. one kCChildCID's nsWindow(shell.html) creates compositor and LayerManager.

Comment 24

[Henry Chang [:hchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> Even on b2g, one primary display has at most 3 nsWindow instances. In the
> following diagram, you can find 3 nsWindows. And only one nsWindow creates
> LayerManager.
> https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/
> widget_nsWindow_FirefoxOS_2_2.pdf

There would be 3 nsWindow created but only one of them will be added to 
"top level window list". (in your patch, only one nsWindow will be
registered to nsScreen)

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Henry Chang [:henry] from comment #24)
> (In reply to Sotaro Ikeda [:sotaro] from comment #21)
> > Even on b2g, one primary display has at most 3 nsWindow instances. In the
> > following diagram, you can find 3 nsWindows. And only one nsWindow creates
> > LayerManager.
> > https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/
> > widget_nsWindow_FirefoxOS_2_2.pdf
> 
> There would be 3 nsWindow created but only one of them will be added to 
> "top level window list". (in your patch, only one nsWindow will be
> registered to nsScreen)

Thanks, I did not checked that part. My patch just keep current implementation.

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Henry Chang [:henry] from comment #18)
> Comment on attachment 8598871 [details] [diff] [review]
> patch - Remove screen related global variables from nsWindow.cpp
> 
> Review of attachment 8598871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nsScreenManagerGonk.h
> @@ +71,5 @@
> >      NS_DECL_ISUPPORTS
> >      NS_DECL_NSISCREENMANAGER
> >  
> > +    static already_AddRefed<nsScreenManagerGonk> GetInstance();
> > +    static already_AddRefed<nsScreenGonk> GetPrimaryScreen();
> 
> Another question here. Returning an "already_AddRefed object" makes us
> unable to code like this "nsScreenManagerGonk::GetInstance()->Foo()". What
> do you think if we change it to returning a RefPt

I think it is ok to change the type. It seems better to use nsRefPtr. RefPtr usage seems not good here. gecko uses RefPtr only for the class that need to be used without xpcom. like under gfx\2d.

Comment 27

[Michael Wu [:mwu]]

(In reply to Henry Chang [:henry] from comment #18)
> Another question here. Returning an "already_AddRefed object" makes us
> unable to code like this "nsScreenManagerGonk::GetInstance()->Foo()". What
> do you think if we change it to returning a RefPtr?

RefPtr is not for returning - already_addRefed and TemporaryRef is for that. If you don't think you need refcounting, return the raw pointer.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37018cb81886

Comment 29

[Henry Chang [:hchang]]

(In reply to Michael Wu [:mwu] from comment #27)
> (In reply to Henry Chang [:henry] from comment #18)
> > Another question here. Returning an "already_AddRefed object" makes us
> > unable to code like this "nsScreenManagerGonk::GetInstance()->Foo()". What
> > do you think if we change it to returning a RefPtr?
> 
> RefPtr is not for returning - already_addRefed and TemporaryRef is for that.
> If you don't think you need refcounting, return the raw pointer.

Sweet! It seems TemporaryRef is safer than already_AddRefed. |already_AddRefed| 
only MOZ_ASSERT the case that the ownership is not transferred [1] :(

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/AlreadyAddRefed.h#101