Closed Bug 1157874 Opened 5 years ago Closed 5 years ago

Remove screen related global variables from nsWindow.cpp

Categories

(Core Graveyard :: Widget: Gonk, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 7 obsolete files)

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;
Attached patch wip patch (obsolete) — Splinter Review
The patch passed compiling, but it does not work.
Assignee: nobody → sotaro.ikeda.g
Blocks: 1116089
Attached patch wip patch (obsolete) — Splinter Review
Fix crash during booting.
Attachment #8596779 - Attachment is obsolete: true
Some clean up.
Attachment #8596843 - Attachment is obsolete: true
Update nits.
Attachment #8597354 - Attachment is obsolete: true
Update nits.
Attachment #8597360 - Attachment is obsolete: true
Attachment #8597365 - Flags: review?(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?
(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.
Apply the comment.
Attachment #8597365 - Attachment is obsolete: true
Attachment #8597365 - Flags: review?(mwu)
Update nsAppShell::NotifyScreenRotation(). I forgot to update it.
Attachment #8598826 - Attachment is obsolete: true
Attachment #8598830 - Flags: review?(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
Attachment #8598830 - Flags: review?(mwu) → review+
Apply the comments. Carry "r=mwu".
Attachment #8598830 - Attachment is obsolete: true
Attachment #8598871 - Flags: review+
Blocks: 1138288
waiting m-i open.
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 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?
(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
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
(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.
(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.
(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)
(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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/37018cb81886
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.