Add CSD config to nsWindow.cpp

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Add CSD config to nsWindow to control CSD state.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1411013

Comment 2

2 years ago
mozreview-review
Comment on attachment 8921061 [details]
Bug 1410894 - Add decoration drawing setup to nsWindow,

https://reviewboard.mozilla.org/r/192028/#review198482

::: widget/gtk/nsWindow.h:126
(Diff revision 1)
>                                           double aY,
>                                           double aWidth,
>                                           double aHeight,
>                                           bool   aRepaint) override;
>      virtual bool       IsEnabled() const override;
> +    bool               IsComposited() const;

The reason for this change is not clear from this patch. Please move it to the patch where it is relevant.

::: widget/gtk/nsWindow.h:355
(Diff revision 1)
>  #ifdef MOZ_X11
>      Display* XDisplay() { return mXDisplay; }
>  #endif
>      virtual void GetCompositorWidgetInitData(mozilla::widget::CompositorWidgetInitData* aInitData) override;
>  
> +    void SetDrawsInTitlebar(bool aState) override;

nit: Move below, to the Decoration section.

::: widget/gtk/nsWindow.h:445
(Diff revision 1)
> -
>  
>      GtkWidget          *mShell;
>      MozContainer       *mContainer;
>      GdkWindow          *mGdkWindow;
> +    bool                mIsCSDEnabled;

The name is a bit confusing, please rename to mIsCSDAvailable to match lookAndFeel code.

::: widget/gtk/nsWindow.h:544
(Diff revision 1)
>  
>      // Remember the last sizemode so that we can restore it when
>      // leaving fullscreen
>      nsSizeMode         mLastSizeMode;
>  
> +    // If true, draw our own window decorations (where supported).

Please elaborate further what exactly counts as window decorations (shadow/corners?, drawing in titlebar?, both?).

::: widget/gtk/nsWindow.h:587
(Diff revision 1)
> +    } CSDSupportLevel;
> +    /**
> +     * Get the support of Client Side Decoration by checking
> +     * the XDG_CURRENT_DESKTOP environment variable.
> +     */
> +    CSDSupportLevel GetCSDSupportLevel();

Please move to static, no need to query environment variable for each nsWindow instance.

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

nit: consider renaming to ShouldClientDrawDecoration, current name implies as it is already drawn. If that's the case, you're free to ignore it.

Comment 3

2 years ago
mozreview-review
Comment on attachment 8921061 [details]
Bug 1410894 - Add decoration drawing setup to nsWindow,

https://reviewboard.mozilla.org/r/192030/#review198594
Attachment #8921061 - Flags: review?(jhorak) → review-
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8921061 [details]
Bug 1410894 - Add decoration drawing setup to nsWindow,

https://reviewboard.mozilla.org/r/192030/#review198972

Looks good, just some minor stuff to address.

::: widget/gtk/nsWindow.h:588
(Diff revision 2)
> +    /**
> +     * Get the support of Client Side Decoration by checking
> +     * the XDG_CURRENT_DESKTOP environment variable.
> +     */
> +    static CSDSupportLevel GetCSDSupportLevel();
> +    static CSDSupportLevel mCSDSupportLevel;

nit: For the static members of the class usually instead of 'm' the 's' is used in name, ie: sCSDSupportLevel.

::: widget/gtk/nsWindow.cpp:6616
(Diff revision 2)
> +{
> +  if (!mIsCSDAvailable || aState == mIsCSDEnabled)
> +      return;
> +
> +  if (mShell) {
> +      gtk_widget_set_app_paintable(mShell, aState);

By our discussion, this should not be necessary ATM.
Attachment #8921061 - Flags: review?(jhorak) → review-
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1411013
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8921061 [details]
Bug 1410894 - Add decoration drawing setup to nsWindow,

https://reviewboard.mozilla.org/r/192030/#review199000

Cool thanks a lot. Lets wait for the try run to finish before setting checkin-needed.
Attachment #8921061 - Flags: review?(jhorak) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8922770 [details]
Bug 1410894 - Load CSD config from nsLookAndFeel,

https://reviewboard.mozilla.org/r/193922/#review199002
Attachment #8922770 - Flags: review?(jhorak) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1db61cd981b7
Add decoration drawing setup to nsWindow, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/3bca24d8a264
Load CSD config from nsLookAndFeel, r=jhorak
Keywords: checkin-needed
Shouldn't this be reviewed by a peer of the widget::gtk module, which karlt is the only remaining one according to https://wiki.mozilla.org/Modules/All ?
Flags: needinfo?(karlt)
https://hg.mozilla.org/mozilla-central/rev/1db61cd981b7
https://hg.mozilla.org/mozilla-central/rev/3bca24d8a264
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 14

2 years ago
(In reply to Mike Hommey [:glandium] from comment #12)
> Shouldn't this be reviewed by a peer of the widget::gtk module, which karlt
> is the only remaining one according to https://wiki.mozilla.org/Modules/All ?

We discussed that with Jim Mathies, Karl has changed his role and no longer works on widget/gtk code although he does some review if possible. Feel free to help with the reviews here and/or review any widget patches, well be glad for any help and insight here.
Confirming comment 14.  Some public discussion in bug 1364843, but much of this was discussed in private email.
Flags: needinfo?(karlt)
Please update module ownership/peership, then.
You need to log in before you can comment on or make changes to this bug.