Default @chromemargin to the eventual value when CAN_DRAW_IN_TITLEBAR is true

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

({perf})

Trunk
Firefox 28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M8])

Attachments

(1 attachment)

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.
Attachment #775519 - Flags: review?(jaws)

Comment 1

6 years ago
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.
Attachment #775519 - Flags: review?(jaws) → review+

Comment 2

6 years ago
(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?
(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 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/078338b16829
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.