893682 - Default @chromemargin to the eventual value when CAN_DRAW_IN_TITLEBAR is true

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Matthew N. [:MattN]]

Attachment: v.1 set attribute in browser.xul whenever CAN_DRAW_IN_TITLEBAR and fix updateTitlebarDisplay

When CAN_DRAW_IN_TITLEBAR is true, updateTitlebarDisplay ends up changing the chromemargin attribute value shortly after creating the window. We should default the attribute to the eventual value so it is already correct when updateTitlebarDisplay runs. This showed a perf improvement for OS X on talos with a WIP patch.

Also note that this fixes a bug where chromemargin was getting set to 0,2,2,2 in updateTitlebarDisplay on OS X instead of the intended 0,-1,-1,-1. I don't think that the values other than the first (top) have any effect on OS X at the moment but we shouldn't rely on that.

Comment 1

[:Gijs (he/him)]

Comment on attachment 775519 [details] [diff] [review]
v.1 set attribute in browser.xul whenever CAN_DRAW_IN_TITLEBAR and fix updateTitlebarDisplay

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -30,24 +30,28 @@
>         title="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
>         title_normal="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
> #ifdef XP_MACOSX
>         title_privatebrowsing="&mainWindow.title;@PRE_RELEASE_SUFFIX@&mainWindow.titlemodifiermenuseparator;&mainWindow.titlePrivateBrowsingSuffix;"
>         titledefault="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
>         titlemodifier=""
>         titlemodifier_normal=""
>         titlemodifier_privatebrowsing="&mainWindow.titlePrivateBrowsingSuffix;"
>-        chromemargin="0,-1,-1,-1"
> #else
>         title_privatebrowsing="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@ &mainWindow.titlePrivateBrowsingSuffix;"
>         titlemodifier="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@"
>         titlemodifier_normal="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@"
>         titlemodifier_privatebrowsing="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@ &mainWindow.titlePrivateBrowsingSuffix;"
> #endif
> #ifdef CAN_DRAW_IN_TITLEBAR
>+#ifdef XP_WIN
>+        chromemargin="0,2,2,2"
>+#else
>+        chromemargin="0,-1,-1,-1"
>+#endif
>         tabsintitlebar="true"
> #endif

This will not set the attribute at all for OS X if CAN_DRAW_IN_TITLEBAR is not true. With that addressed, r=me.

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> Comment on attachment 775519 [details] [diff] [review]
> v.1 set attribute in browser.xul whenever CAN_DRAW_IN_TITLEBAR and fix
> updateTitlebarDisplay
> 
> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> >--- a/browser/base/content/browser.xul
> >+++ b/browser/base/content/browser.xul
> >@@ -30,24 +30,28 @@
> >         title="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
> >         title_normal="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
> > #ifdef XP_MACOSX
> >         title_privatebrowsing="&mainWindow.title;@PRE_RELEASE_SUFFIX@&mainWindow.titlemodifiermenuseparator;&mainWindow.titlePrivateBrowsingSuffix;"
> >         titledefault="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
> >         titlemodifier=""
> >         titlemodifier_normal=""
> >         titlemodifier_privatebrowsing="&mainWindow.titlePrivateBrowsingSuffix;"
> >-        chromemargin="0,-1,-1,-1"
> > #else
> >         title_privatebrowsing="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@ &mainWindow.titlePrivateBrowsingSuffix;"
> >         titlemodifier="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@"
> >         titlemodifier_normal="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@"
> >         titlemodifier_privatebrowsing="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@ &mainWindow.titlePrivateBrowsingSuffix;"
> > #endif
> > #ifdef CAN_DRAW_IN_TITLEBAR
> >+#ifdef XP_WIN
> >+        chromemargin="0,2,2,2"
> >+#else
> >+        chromemargin="0,-1,-1,-1"
> >+#endif
> >         tabsintitlebar="true"
> > #endif
> 
> This will not set the attribute at all for OS X if CAN_DRAW_IN_TITLEBAR is
> not true. With that addressed, r=me.

Also, we weren't setting it for Linux; now it'll get set to 0,-1,-1,-1 if CAN_DRAW_IN_TITLEBAR is true. Is that intentional and/or have you checked that that doesn't mess anything up?

Comment 3

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #1)
> > #ifdef CAN_DRAW_IN_TITLEBAR
> >+#ifdef XP_WIN
> >+        chromemargin="0,2,2,2"
> >+#else
> >+        chromemargin="0,-1,-1,-1"
> >+#endif
> >         tabsintitlebar="true"
> > #endif
> 
> This will not set the attribute at all for OS X if CAN_DRAW_IN_TITLEBAR is
> not true. With that addressed, r=me.

That was an intentional change as it should have the default value of -1 for all sides if it's not true.

(In reply to :Gijs Kruitbosch from comment #2)
> Also, we weren't setting it for Linux; now it'll get set to 0,-1,-1,-1 if
> CAN_DRAW_IN_TITLEBAR is true. Is that intentional and/or have you checked
> that that doesn't mess anything up?

Linux doesn't set that define because it doesn't support drawing in the titlebar so there isn't a supported option to test. I did intentionally default to that value over the other because it's more like the defaults.

Comment 4

[Matthew N. [:MattN]]

Pushed: https://hg.mozilla.org/projects/ux/rev/078338b16829

Try results: https://tbpl.mozilla.org/?tree=Try&rev=3061c14de8ce

Comment 5

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/078338b16829