1441873 - [CSD] Remove nsWindow::mIsCSDAvailable

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement, P3)

Description

[Martin Stránský [:stransky] (ni? me)]

We don't need mIsCSDAvailable as CSDSupportLevel may serve here as well.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1441873 - [CSD] Remove nsWindow::mIsCSDAvailable and replace with CSDSupportLevel state,

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

Comment 2

[Jan Horak [:jhorak]]

Comment on attachment 8954794 [details]
Bug 1441873 - [CSD] Remove nsWindow::mIsCSDAvailable and replace with CSDSupportLevel state,

https://reviewboard.mozilla.org/r/223916/#review233806

::: widget/gtk/nsWindow.h:497
(Diff revision 1)
>      // window. See bug 1225044.
>      unsigned int mPendingConfigures;
>  
> -    bool               mIsCSDAvailable;
> +    // Window titlebar rendering mode, CSD_SUPPORT_NONE if it's disabled
> +    // for this window.
> +    CSDSupportLevel    mCSDAvailable;

I'd better prefer to name this mCSDSupportLevel. It makes more sence in later code and it also reflect the type of the variable.

::: widget/gtk/nsWindow.cpp:6564
(Diff revision 1)
>    SetDrawsInTitlebar(aMargins.top == 0);
>    return NS_OK;
>  }
>  
>  void
>  nsWindow::SetDrawsInTitlebar(bool aState)

Looking at the method name and usage of mIsCSDEnabled, wouldn't renaming mIsCSDEnabled to mDrawInTitlebar also make a sense?

Also consider rename of SetDrawsInTitlebar to SetDrawInTitlebar as it sounds a bit awkward.

::: widget/gtk/nsWindow.cpp:6569
(Diff revision 1)
>  nsWindow::SetDrawsInTitlebar(bool aState)
>  {
> -  if (!mIsCSDAvailable || aState == mIsCSDEnabled)
> +  if (mCSDAvailable == CSD_SUPPORT_NONE || aState == mIsCSDEnabled)
>        return;
>  
>    if (mShell) {

Does it make sense to set mIsCSDEnabled when mShell==nullptr? Maybe we could move it above together with aState == mIsCSDEnabled.

::: widget/gtk/nsWindow.cpp:6570
(Diff revision 1)
>  {
> -  if (!mIsCSDAvailable || aState == mIsCSDEnabled)
> +  if (mCSDAvailable == CSD_SUPPORT_NONE || aState == mIsCSDEnabled)
>        return;
>  
>    if (mShell) {
> -      if (GetCSDSupportLevel() == CSD_SUPPORT_SYSTEM) {
> +      if (mCSDAvailable == CSD_SUPPORT_SYSTEM) {

Hm, looking at this, maybe switch could be more straighforward (with moving the (mCSDAvailable == CSD_SUPPORT_NONE) also to the switch

::: widget/gtk/nsWindow.cpp:6902
(Diff revision 1)
>    return NS_OK;
>  }
>  #endif
>  
>  bool
>  nsWindow::DoDrawTitlebar() const

Looks like this method is no longer used anywhere. Do the mSizeState needs to be reflected somewhere?

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8954794 [details]
Bug 1441873 - [CSD] Remove nsWindow::mIsCSDAvailable and replace with CSDSupportLevel state,

https://reviewboard.mozilla.org/r/223916/#review233806

> Hm, looking at this, maybe switch could be more straighforward (with moving the (mCSDAvailable == CSD_SUPPORT_NONE) also to the switch

That makes the code too much indented and also we'd need to set mIsCSDEnabled = aState; for CSD_SUPPORT_SYSTEM/CLIENT only. I'd rather leave it as is.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8954794 [details]
Bug 1441873 - [CSD] Remove nsWindow::mIsCSDAvailable and replace with CSDSupportLevel state,

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

Comment 5

[Jan Horak [:jhorak]]

Comment on attachment 8954794 [details]
Bug 1441873 - [CSD] Remove nsWindow::mIsCSDAvailable and replace with CSDSupportLevel state,

https://reviewboard.mozilla.org/r/223916/#review237452

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/179e31d8306f
[CSD] Remove nsWindow::mIsCSDAvailable and replace with CSDSupportLevel state, r=jhorak

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/179e31d8306f