350129 - Improve toolbar customization UE on OS X

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P2)

Description

[Asaf Romano (gone)]

Improve toolbar customization UE on OS X.

Filing this as the FF2.0 doable version of bug 206649.

Comment 1

[Asaf Romano (gone)]

Attachment: screenshot

Comment 2

[Asaf Romano (gone)]

Attachment: work in progress

Comment 3

[Asaf Romano (gone)]

Attachment: more...

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 235498 [details] [diff] [review]
patch

This looks pretty good and seems to work great!

couple of questions: the overlay for reporter fails gracefully on non-Mac, right?  and there's nothing anywhere than has any valid reason to be broken by the XUL structure change, right? (not XBL etc, so no childNodes nonsense here)

please run the various leak tools here, just to be sure, but I think this should be better than the old impl

Comment 6

[Asaf Romano (gone)]

(In reply to comment #5)
> (From update of attachment 235498 [details] [diff] [review] [edit])
> This looks pretty good and seems to work great!
> 
> couple of questions: the overlay for reporter fails gracefully on non-Mac,
> right?

Yeah, see the jar.mn file of DOMi.

> and there's nothing anywhere than has any valid reason to be broken by
> the XUL structure change, right? (not XBL etc, so no childNodes nonsense here)

Let's hope so!

> please run the various leak tools here, just to be sure, but I think this
> should be better than the old impl

Same amount of leaks (three objects) when testing with Leak Monitor.

Comment 7

[Asaf Romano (gone)]

trunk:
mozilla/browser/base/Makefile.in 1.18
mozilla/browser/base/jar.mn,v 1.104
mozilla/browser/base/content/browser-doctype.inc 1.9
mozilla/browser/base/content/browser.js 1.693
mozilla/browser/base/content/browser.xul 1.316
mozilla/browser/base/content/customizeToolbarSheet.js  1.1
mozilla/browser/base/content/customizeToolbarSheet.xul 1.1
mozilla/toolkit/content/customizeToolbar.js 1.30
mozilla/browser/themes/pinstripe/browser/browser.css 1.29
mozilla/extensions/reporter/jar.mn 1.6

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 235498 [details] [diff] [review]
patch

please get this in ASAP, and follow any regressions closely.

Comment 9

[Asaf Romano (gone)]

1.8 branch (including the fix for bug 350282):
mozilla/browser/base/Makefile.in 1.16.2.2
mozilla/browser/base/jar.mn 1.95.2.10
mozilla/browser/base/content/browser-doctype.inc 1.6.2.3
mozilla/browser/base/content/browser.js 1.479.2.195
mozilla/browser/base/content/browser.xul 1.268.2.60
mozilla/browser/base/content/customizeToolbarSheet.js 1.1.2.1
mozilla/browser/base/content/customizeToolbarSheet.xul 1.2.2.1
mozilla/browser/themes/pinstripe/browser/browser.css 1.11.4.34
mozilla/extensions/reporter/jar.mn 1.4.4.2
mozilla/toolkit/content/customizeToolbar.js 1.26.8.3

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

What was the reasoning behind adding a <stack> to the browser chrome hierarchy?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> What was the reasoning behind adding a <stack> to the browser chrome hierarchy?

It was needed to position the customize sheet over the browser content. Bug 395122 has a patch to remove it.