[CSD] Remove nsWindow::mIsCSDAvailable

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
minor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks 1 bug, {polish})

60 Branch
mozilla61
Points:
---

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
We don't need mIsCSDAvailable as CSDSupportLevel may serve here as well.
(Assignee)

Updated

a year ago
Summary: [CSD] Remove mIsCSDAvailable → [CSD] Remove nsWindow::mIsCSDAvailable
(Assignee)

Updated

a year ago
Severity: normal → minor
Priority: -- → P3
(Assignee)

Updated

a year ago
Keywords: polish

Comment 2

a year ago
mozreview-review
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?
Attachment #8954794 - Flags: review?(jhorak) → review-
(Assignee)

Comment 3

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 5

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

https://reviewboard.mozilla.org/r/223916/#review237452
Attachment #8954794 - Flags: review?(jhorak) → review+

Comment 6

a year ago
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/179e31d8306f
[CSD] Remove nsWindow::mIsCSDAvailable and replace with CSDSupportLevel state, r=jhorak

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/179e31d8306f
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.