898012 - Clean up OS X titlebar placeholder code

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (out for now; he/him)]

There are a couple of things going on in the titlebar placeholder code which should be cleaned up for code sanity reasons, and may also gain us some performance. I'll split them up into "essentially dead code that should go away" and "things that it might be possible to do", which hopefully together get us some perf. Even if it doesn't, I think we should do part 1.

Comment 1

[:Gijs (out for now; he/him)]

Attachment: Remove useless bits in OS X titlebar placeholder stuff

Part 1. Note that the $ shortcut function is already defined a couple of lines above, and I don't know why we have two definitions. All the menubar deletions are there because it makes no sense to do work for an element we're never showing.

Comment 2

[:Gijs (out for now; he/him)]

Attachment: Optimize static bits of titlebar placeholder code on OS X

Less sure about this, but pushed to try to figure out if this is a good idea. I don't think the fullscreen button really changes size beyond not existing on pre-Lion. Holding off on review while waiting for try: https://tbpl.mozilla.org/?tree=Try&rev=4252b5f66d87

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 781045 [details] [diff] [review]
Optimize static bits of titlebar placeholder code on OS X

(In reply to :Gijs Kruitbosch from comment #2)
> Created attachment 781045 [details] [diff] [review]
> Optimize static bits of titlebar placeholder code on OS X
> 
> Less sure about this, but pushed to try to figure out if this is a good
> idea. I don't think the fullscreen button really changes size beyond not
> existing on pre-Lion. Holding off on review while waiting for try:
> https://tbpl.mozilla.org/?tree=Try&rev=4252b5f66d87

Tentatively, this might provide some small wins, but they're not very significant if they exist, which makes it hard to be sure:

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=732794f8a4d4&newRev=4252b5f66d87&submit=true

Dao, I wrote this under the assumption that the fullscreen button never changes size (except that it's not visible on 10.6). Do you think that assumption is correct, and if so, do you think it'd make sense to take this patch?

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 781045 [details] [diff] [review]
Optimize static bits of titlebar placeholder code on OS X

> Dao, I wrote this under the assumption that the fullscreen button never
> changes size (except that it's not visible on 10.6). Do you think that
> assumption is correct, and if so, do you think it'd make sense to take this
> patch?

I guess it's correct for released versions of OS X. For future versions, we don't know. Like any hardcoded assumption about things we don't control and that aren't guaranteed to stay the same, there's some fragility to it, so we should probably do it only if it's significantly cheaper.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 781045 [details] [diff] [review]
> Optimize static bits of titlebar placeholder code on OS X
> 
> > Dao, I wrote this under the assumption that the fullscreen button never
> > changes size (except that it's not visible on 10.6). Do you think that
> > assumption is correct, and if so, do you think it'd make sense to take this
> > patch?
> 
> I guess it's correct for released versions of OS X. For future versions, we
> don't know. Like any hardcoded assumption about things we don't control and
> that aren't guaranteed to stay the same, there's some fragility to it, so we
> should probably do it only if it's significantly cheaper.

That doesn't seem to be the case. Landed only the first patch as https://hg.mozilla.org/projects/ux/rev/c1657630b126 , closing this so we can move on.

Comment 6

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/mozilla-central/rev/c1657630b126