476814 - Unfork/Unify customizeToolbar.* and customizeToolbarSheet.*

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, enhancement)

Description

[Philip Chee]

Attachment: Patch v1.0 Unfork OSX [Checkin: Comment 5]

Currently the toolkit implementation of Customize Toolbars does not support OSX. Firefox has a forked implementation where they fake a sheet with a panel. Thunderbird (and probably every other toolkit app) has similarly copied the Firefox fork for OSX.

In investigating the current differences between the browser/customizeToolbarSheet.* files vs the toolkit files, I noticed that (modulo whitespace and wrapping changes) currently the differences are rather minimal and I thought it would be doable to create a merged implementation that would work in both popup window mode and in panel mode.

In the Suite Bug 406780 I now have a working implementation that can be used either way. See attachment 360307 [details] [diff] [review] for how the application tells the customize code the toolbox to use when in panel mode.

This patch ports my work in SeaMonkey back to toolkit. This should reduce the amount of forking and also the inevertible bit-rot that this causes.

Comment 1

[Asaf Romano (gone)]

Comment on attachment 360463 [details] [diff] [review]
Patch v1.0 Unfork OSX
[Checkin: Comment 5]

r=mano. If you don't have time to do the fx-part, let me know and I will.

Comment 2

[Philip Chee]

> If you don't have time to do the fx-part, let me know and I will.

I hope someone can do the Firefox part. I don't actually have OSX so for the Suite I had to rely on someone else to test my patch for me which slowed things down a bit. It would be faster for someone who has OSX to do the Firefox side of things.

Comment 3

[Dão Gottwald [:dao]]

I don't understand what "checkin comment #1" means.

Comment 4

[Philip Chee]

> I don't understand what "checkin comment #1" means.

Sorry, it means that the patch for checkin is referenced in comment 1. I don't have the necessary permissions to checkin this patch so some kind soul will have to do it for me.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 360463 [details] [diff] [review]
Patch v1.0 Unfork OSX
[Checkin: Comment 5]


http://hg.mozilla.org/mozilla-central/rev/2e222e703e81

Comment 6

[Philip Chee]

OK Resolving as FIXED. Separate bugs need to be filed in Firefox and Thunderbird for the front end UI.

Comment 7

[Philip Chee]

Comment on attachment 360463 [details] [diff] [review]
Patch v1.0 Unfork OSX
[Checkin: Comment 5]

Approval Criteria:

1. Tests: No new functionality introduced. Existing unit tests should be sufficient to catch any regressions (none found to date)

2. Has landed on trunk and baked for more than a month. The forked version in SeaMonkey 2.0a3 (Gecko 1.9.1) has been out in the wild for more than a month with no reported regressions.

3. Performance impact: No impact on startup. Any possible impact would be when the customize dialog/panel is called and if any would be below noise levels.

4. Risk assessment: I don't see any increased or new risks. This patch refactors code without introducing any new functionality.

5. Reasons for taking patch: Allows Firefox and Thunderbird to unfork their OSX implementations of customizeToolbar.*. Allows SeaMonkey to remove their forked version of this patch. A (very small) reduction of code size would be possible by removing /browser/customizeToolbarSheet* and mail/customizeToolbarSheet.*

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 360463 [details] [diff] [review]
Patch v1.0 Unfork OSX
[Checkin: Comment 5]

a191=beltzner

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 360463 [details] [diff] [review]
Patch v1.0 Unfork OSX
[Checkin: Comment 5]

Does not apply cleanly to 1.9.1:
{
patching file toolkit/content/customizeToolbar.js
Hunk #2 FAILED at 70
1 out of 2 hunks FAILED
patching file toolkit/content/customizeToolbar.xu
Hunk #2 FAILED at 90
1 out of 2 hunks FAILED
}

Comment 10

[Philip Chee]

Attachment: 1.9.1 branch patch [for checkin to 1.9.1][has approval191+]

> (From update of attachment 360463 [details] [diff] [review])
> Does not apply cleanly to 1.9.1:

Patch generated against 1.9.1.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/98d6f6029300