Closed Bug 1410894 Opened 7 years ago Closed 7 years ago

Add CSD config to nsWindow.cpp

Categories

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

Product:
Core
Component:
Widget: Gtk
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1410894 - Add decoration drawing setup to nsWindow,
59 bytes, text/x-review-board-request
jhorak
: review+
Details
Bug 1410894 - Load CSD config from nsLookAndFeel,
59 bytes, text/x-review-board-request
jhorak
: review+
Details 
Add CSD config to nsWindow to control CSD state.
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 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 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-
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 on attachment 8922770 [details]
Bug 1410894 - Load CSD config from nsLookAndFeel,

https://reviewboard.mozilla.org/r/193922/#review199002
Attachment #8922770 - Flags: review?(jhorak) → review+
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)
(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.

Attachment

General

Creator:
Created:
Updated:
Size: