Last Comment Bug 770135 - New PanelUI and toolbar customization (Milestone 1)
: New PanelUI and toolbar customization (Milestone 1)
Status: RESOLVED FIXED
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: unspecified
: All All
: -- normal with 11 votes (vote)
: Firefox 28
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
: 844281 (view as bug list)
Depends on: 752434 793093 840697 844281 854450 855305 855452 855483 857542 858067 858068 858132 858196 858219 858584 858597 858660 858662 860487 860535 860562 860646 861087 861088 861092 861401 861702 863314 863753 866590 867682 868410 868993 869069 869101 869349 869581 870985 871151 872160 881041 961823
Blocks: 212990 347446 australis-cust 184147 855287 860814
  Show dependency treegraph
 
Reported: 2012-07-02 05:12 PDT by Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
Modified: 2014-01-20 17:44 PST (History)
51 users (show)
fbraun: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prototype v1 (29.96 KB, patch)
2012-07-02 05:25 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
dtownsend: feedback+
dolske: feedback+
shorlander: feedback+
fryn: feedback+
Details | Diff | Splinter Review
WiP v1 (40.81 KB, patch)
2012-09-05 05:50 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WiP v2 - backend (79.01 KB, patch)
2012-10-18 03:22 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WiP v2 - customization mode (17.39 KB, patch)
2012-10-18 03:35 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WiP v2 - demo (3.15 KB, application/x-xpinstall)
2012-10-18 03:47 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details
WiP v3 - backend (27.55 KB, patch)
2012-11-12 03:14 PST, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WiP v3 - customization mode (18.18 KB, patch)
2012-11-12 03:15 PST, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WiP v3 - demo (3.34 KB, application/x-xpinstall)
2012-11-12 03:17 PST, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details
WiP v3.001 - backend (108.61 KB, patch)
2012-11-12 03:20 PST, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WIP v3.002 - backend (rebased) (109.15 KB, patch)
2013-01-23 12:51 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
WIP v3.002 - frontend (rebased) (17.47 KB, patch)
2013-01-23 12:52 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
WiP v4 - backend (needs rebased) (114.73 KB, patch)
2013-01-23 18:28 PST, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WiP v4 - frontend (needs rebased) (31.00 KB, patch)
2013-01-23 18:29 PST, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
WIP v4.01 - backend (rebased) (115.28 KB, patch)
2013-02-07 13:44 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
WIP v4.01 - frontend (rebased) (30.22 KB, patch)
2013-02-07 13:45 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
WIP Patch v4.02 (138.62 KB, patch)
2013-02-21 10:28 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
AppMenu WIP Patch v4.03 (105.30 KB, patch)
2013-02-22 13:13 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Customization WIP Patch v4.03 (35.56 KB, patch)
2013-02-22 13:15 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Customization WIP Patch v4.04 (35.66 KB, patch)
2013-02-28 10:37 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
9a2477c31b28.patch (part 1) (105.42 KB, patch)
2013-03-22 14:00 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
de041c8ef0c7.patch (part 2) (35.90 KB, patch)
2013-03-22 14:01 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
89287cdca338.patch (part 3) (798 bytes, patch)
2013-03-22 14:02 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
e4ac9e227efe.patch (part 4) (19.59 KB, patch)
2013-03-22 14:03 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
8bc3c78c12fe.patch (part 5) (1.07 KB, patch)
2013-03-22 14:05 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
6235f69f1647.patch (part 6) (7.87 KB, patch)
2013-03-22 14:06 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
53cf1ebc6e85.patch (part 7) (22.05 KB, patch)
2013-03-22 14:07 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
d678b862dbfd.patch (part 8) (19.61 KB, patch)
2013-03-22 14:08 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
deec4e29ba17.patch (part 9) (9.38 KB, patch)
2013-03-22 14:08 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
fd97c40999fc.patch (part 10) (33.34 KB, patch)
2013-03-22 14:11 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
d410a4e584e3.patch (part 11) (8.89 KB, patch)
2013-03-22 14:12 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
f0e94249a97a.patch (part 12) (3.94 KB, patch)
2013-03-22 14:12 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
0c5206f0ed6b.patch (part 13) (1.13 KB, patch)
2013-03-22 14:13 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
952c9ccc247d.patch (part 14) (3.85 KB, patch)
2013-03-22 14:15 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
7534fd7a2bac.patch (part 15) (1.20 KB, patch)
2013-03-22 14:16 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
b9bee2009e45.patch (part 16) (42.81 KB, patch)
2013-03-22 14:16 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
a18848287a4f.patch (part 17) (2.19 KB, patch)
2013-03-22 14:21 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
48f24a30c6ac.patch (part 18) (3.33 KB, patch)
2013-03-22 14:22 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
e907595d9ce3.patch (part 19) (3.45 KB, patch)
2013-03-22 14:23 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
c1859c500536.patch (part 20) (1.37 KB, patch)
2013-03-22 14:23 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
29650070976e.patch (part 21) (6.52 KB, patch)
2013-03-22 14:24 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
176c822ef6a3.patch (part 22) (119.21 KB, patch)
2013-03-22 14:25 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
cbe88d315a03.patch (part 23) (3.79 KB, patch)
2013-03-22 14:25 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
9e5a55078be5.patch (part 24) (10.47 KB, patch)
2013-03-22 14:26 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Milestone 1 patch (166.14 KB, patch)
2013-03-22 14:39 PDT, Jared Wein [:jaws] (please needinfo? me)
bmcbride: review+
jaws: review+
mconley: review+
Details | Diff | Splinter Review
Addressing my review comments (10.20 KB, patch)
2013-03-25 12:38 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Addressing my review comments (25.97 KB, patch)
2013-03-25 13:57 PDT, Jared Wein [:jaws] (please needinfo? me)
mconley: review+
Details | Diff | Splinter Review
Addressing my review comments as well (73.08 KB, patch)
2013-03-25 15:32 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Addressing my review comments (apply on top of jaws' patch) (73.05 KB, patch)
2013-03-25 15:46 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review+
Details | Diff | Splinter Review
After-big-patch Follow-up #1 - Make sure panel size is sync'd. (3.45 KB, patch)
2013-03-25 16:40 PDT, Mike Conley (:mconley) - (Needinfo me!)
mconley: review+
Details | Diff | Splinter Review
After-big-patch Follow-up #2 - Allow items to be dropped among palette items (1.37 KB, patch)
2013-03-25 16:42 PDT, Mike Conley (:mconley) - (Needinfo me!)
bmcbride: review+
Details | Diff | Splinter Review
After-big-patch Follow-up #3 - Make programmatically generated widgets behave (6.67 KB, patch)
2013-03-25 16:43 PDT, Mike Conley (:mconley) - (Needinfo me!)
bmcbride: review+
Details | Diff | Splinter Review
After-big-patch Follow-up #4 - Make going into customize mode more awesome (119.28 KB, patch)
2013-03-25 16:45 PDT, Mike Conley (:mconley) - (Needinfo me!)
bmcbride: review-
Details | Diff | Splinter Review
After-big-patch Follow-up #5 - More ways to exit customize mode (3.80 KB, patch)
2013-03-25 16:46 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review+
Details | Diff | Splinter Review
After-big-patch Follow-up #6 - Add rudimentary drop indicator to toolbar (10.28 KB, patch)
2013-03-25 16:47 PDT, Mike Conley (:mconley) - (Needinfo me!)
mconley: review+
jaws: review-
Details | Diff | Splinter Review
After-big-patch Follow-up #7 - Make drop targets glow blue instead of purple (904 bytes, patch)
2013-03-25 16:48 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review+
Details | Diff | Splinter Review
After-big-patch Follow-up #8 - Disable the menu button when customizing (2.05 KB, patch)
2013-03-25 16:49 PDT, Mike Conley (:mconley) - (Needinfo me!)
mconley: review+
Details | Diff | Splinter Review
After-big-patch Follow-up #9 - Add a grab effect when drag starts (3.88 KB, patch)
2013-03-25 16:50 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review-
Details | Diff | Splinter Review
Addressing comment #79 (2.88 KB, patch)
2013-03-26 09:38 PDT, Jared Wein [:jaws] (please needinfo? me)
jaws: review+
Details | Diff | Splinter Review
Milestone 1.1 interdiff (r=jaws, Unfocused, mconley) (112.85 KB, patch)
2013-03-26 12:15 PDT, Mike Conley (:mconley) - (Needinfo me!)
mconley: review+
Details | Diff | Splinter Review
M1 - Follow-up #1 - Make sure panel size is sync'd. r=mconley. (3.41 KB, patch)
2013-03-26 13:02 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
M1 - Follow-up #2 - Allow items to be dropped among palette items (r=Unfocused) (1.37 KB, patch)
2013-03-26 13:03 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
M1 - Follow-up #3 - Make programmatically generated widgets behave (r=Unfocused) (6.90 KB, patch)
2013-03-26 13:04 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
M1 - Follow-up #4 - Make going into customize mode more awesome (125.24 KB, patch)
2013-03-26 13:04 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review+
Details | Diff | Splinter Review
M1 - Follow-up #5 - More ways to exit customize mode (r=jaws) (2.98 KB, patch)
2013-03-26 13:08 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
M1 - Follow-up #6 - Add rudimentary drop indicator to toolbar (10.29 KB, patch)
2013-03-26 13:10 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review+
Details | Diff | Splinter Review
M1 - Follow-up #7 - Make drop targets glow blue instead of purple (r=jaws) (904 bytes, patch)
2013-03-26 13:11 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
M1 - Follow-up #8 - Disable the menu button when customizing (r=mconley) (1.98 KB, patch)
2013-03-26 13:12 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
M1 - Follow-up #9 - Add a grab effect when drag starts (6.50 KB, patch)
2013-03-26 13:13 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review+
Details | Diff | Splinter Review
M1 - Follow-up #4 - Make going into customize mode more awesome (r=jaws) (356.34 KB, patch)
2013-03-26 13:43 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
M1 - Follow-up #10 - Make buttons look right on OSX and Linux, and make grid texture work on Linux (6.64 KB, patch)
2013-03-26 16:49 PDT, Mike Conley (:mconley) - (Needinfo me!)
jaws: review+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-07-02 05:12:36 PDT
This is a bug to explore some fun ideas to do with the Panel UI. The wiki page provides significant context:
https://wiki.mozilla.org/Features/Desktop/Panel_Menu


Everything is hacky, everything is temporary, everything is exploration. And at this point, everything looks ugly.
Comment 1 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-07-02 05:25:11 PDT
Created attachment 638318 [details] [diff] [review]
Prototype v1

This is light on interaction stuff, but it does have:
* Widget that has a popout (click the Weather widget)
* Integration with the Add-ons Manager

For technical notes on how addons register widgets, see the big comment in ManifestParser.cpp.

PanelUI.getWidget("weather-indicator") allows playing around with an API similar to whats described in the wiki notes, where you can alter a widget for all windows, or for just one window (via .forWindow()). Only the setter for "disabled" is implemented.

Builds should automagically appear here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-fe19ad22bc7d
Comment 2 Stephen Horlander [:shorlander] 2012-07-13 08:32:57 PDT
Comment on attachment 638318 [details] [diff] [review]
Prototype v1

Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------

Not much UX to comment on yet, but it looks like a good start! :)

Observation about the placement in the add-ons manager: The menu/panel is restricted to the viewport but in the actual toolbar it isn't.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-07-16 05:08:24 PDT
Comment on attachment 638318 [details] [diff] [review]
Prototype v1

Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------

I've looked at the patch a couple times but I don't have any feedback to give at this point, except to say that I'm happy to see that the Panel UI is moving forward!1! \o/
Comment 4 Frank Yan (:fryn) 2012-08-09 14:31:14 PDT
Comment on attachment 638318 [details] [diff] [review]
Prototype v1

Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------

I looked at the patch too, and I echo Jared's sentiment. :)
Comment 5 Dave Townsend [:mossop] 2012-08-10 10:13:37 PDT
Comment on attachment 638318 [details] [diff] [review]
Prototype v1

Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, neato. I think we want to have a programmatic way to register widgets at runtime (just pass the JS object directly to the manager I guess). You probably want to get bsmedberg's take on the manifest parsing stuff, the manager could handle reloads itself if in a bit of a clunky way.

Presumably you're also thinking of more properties to expose for the widget, like being able to change the icon etc.
Comment 6 Justin Dolske [:Dolske] 2012-08-27 15:04:44 PDT
Comment on attachment 638318 [details] [diff] [review]
Prototype v1

(Already gave Blair feedback on this -- no issues -- forgot I was flagged)
Comment 7 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-09-05 05:50:15 PDT
Created attachment 658462 [details] [diff] [review]
WiP v1

Going to start dumping regular work-in-progress patches here. Most obvious change here is getting the Panel and associated button looking like it fits in with Australis.
Comment 8 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-10-18 03:22:44 PDT
Created attachment 672732 [details] [diff] [review]
WiP v2 - backend

This implements a decent amount of the desired functionality - a new app panel UI, integrates with hardcoded/overlaid toolbar buttons, integrates with in-content UI, provides an API for defining new widgets which are automatically handled for all windows, provides an API and events for a new customization mode to customize toolbars/the panel, stores/restores state (moving away from localstore.rdf), implements a large number of bugs, and maybe even does other things I've forgotten.

Note: Windows-only for now.
Comment 9 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-10-18 03:35:19 PDT
Created attachment 672734 [details] [diff] [review]
WiP v2 - customization mode

And this is an initial exploration into how the new customization mode would integrate with the rest of the browser - without actually getting into doing anything useful like customizing stuff.

Uses CSS transitions for the zoom-in animation when entering the customization mode - which works very smoothly on my desktop, but is very far from smooth on my Win8 Slate (which has much slower hardware). I haven't done any investigation into how to make that better on slow hardware, but at the moment the performance is unacceptable.

Note: This patch needs to be applied on top of the other patch. And it's also Windows-only.
Comment 10 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-10-18 03:47:03 PDT
Created attachment 672737 [details]
WiP v2 - demo

And finally, this is an add-on that acts as a demo for UI customization, in lieu of a proper customization mode. It'll add a "UI Demo" button to the new panel - hitting that will open a tab with a craptastic UI to add/remove/reorganise items in the panel (changes are reflected immediately).

Obvious bug: Disabling/uninstalling this add-on currently won't remove it's button in the panel. That's a pending to-do item.
Comment 11 Mozilla RelEng Bot 2012-10-18 05:30:38 PDT
Try run for 4c6aade0dec7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4c6aade0dec7
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-4c6aade0dec7
Comment 12 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-10-18 15:00:12 PDT
In case it wasn't clear: The demo add-on will *only* work when using the above Try build (or any build with the attached patches applied).
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-11-01 14:36:46 PDT
First off - this is really cool. I remember seeing shorlander / bwinton's mockup, and really digging it. It's cool to see this stuff get close.

Anyhow, a few things I noticed - not sure how you want these - as bugs, or a bullet list, but here goes:

* Clicking on menu button, then new window - menu stays on top and opened once new window has spawned. I expected that menu to close.
* The + and - buttons in the menu are not updating the text field between them.
* It's not clear to me what is customizable and what is not... I'm guessing dragging the toolbar buttons is currently not implemented? But if it is, it's currently busted.
Comment 14 scheco 2012-11-01 17:37:23 PDT
Awesome guys, I'm following the progress of this feature and am very happy with the results.
   I will send all the feedback that can help you.
Comment 15 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-04 17:03:57 PST
(In reply to Mike Conley (:mconley) from comment #13)
> * Clicking on menu button, then new window - menu stays on top and opened
> once new window has spawned. I expected that menu to close.

Yes, it should - consider that a bug. Seems to work for some things and not others.

> * The + and - buttons in the menu are not updating the text field between
> them.

Interesting, you expect that to reflect the current zoom level? The "100%" is intended to indicate that clicking it will reset the zoom to normal.

> * It's not clear to me what is customizable and what is not... I'm guessing
> dragging the toolbar buttons is currently not implemented? But if it is,
> it's currently busted.

Indeed, not implemented. The only way to do any customizing at the moment is via the demo add-on, and that only affects what's in the panel (and no dragging there either - all done via the 4 buttons).

(In reply to cuentaparafacultad from comment #14)
>    I will send all the feedback that can help you.

Please do :)
Comment 16 Darren Kalck [:D-Kalck] 2012-11-05 03:25:02 PST
Where can I download the try build ? It's not available.
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-11-05 11:11:47 PST
(In reply to Darren Kalck [:D-Kalck] from comment #16)
> Where can I download the try build ? It's not available.

http://people.mozilla.org/~jwein/australis-customization.zip
Comment 18 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-12 03:14:53 PST
Created attachment 680574 [details] [diff] [review]
WiP v3 - backend

General theme since previous iteration has been stabilization, formalization, bug fixes. ie, ripping out quick hacks and doing things properly :)
* Cleaner API
* Formalized registering of widgets, areas, and build instances
* Properly handling restoring of state
* Properly handling auto-adding widgets when they're first registered by an addon
* Start of standardizing widget wrappers (ie, even old-style XUL widgets can get nice wrappers now)
* Maybe other stuff I've forgotten

Also exploring some re-organization of the panel.
Comment 19 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-12 03:15:46 PST
Created attachment 680575 [details] [diff] [review]
WiP v3 - customization mode

Just fixing bitrot.
Comment 20 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-12 03:17:52 PST
Created attachment 680576 [details]
WiP v3 - demo

More bitrot fixes (ie, you *need* to use this version of the demo addon).

Again, everything still Windows focused.
Comment 21 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-12 03:20:32 PST
Created attachment 680577 [details] [diff] [review]
WiP v3.001 - backend

Er, helps if I attach the right patch.
Comment 22 Mozilla RelEng Bot 2012-11-12 05:15:35 PST
Try run for 974fca53ce6a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=974fca53ce6a
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-974fca53ce6a
Comment 23 Guillaume C. [:ge3k0s] 2012-11-12 10:24:18 PST
Just one little detail on the current implementation : it would be more logical to have plus-minus zoom button inverted since we read LTR.
Comment 24 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-12 17:31:31 PST
(In reply to Guillaume C. [:ge3k0s] from comment #23)
> Just one little detail on the current implementation : it would be more
> logical to have plus-minus zoom button inverted since we read LTR.

Indeed - fixed.
Comment 25 Guillaume C. [:ge3k0s] 2012-11-12 23:43:28 PST
(In reply to Mike Conley (:mconley) from comment #13)
> * The + and - buttons in the menu are not updating the text field between
> them.

I agree that this should be the default behaviour. The fact to click on this button could however always bring back 100% zoom.
Comment 26 Alejandro Rodriguez [:Alopepeo] 2012-12-02 15:54:35 PST
Where can i download the latest try-builds? I test the build into "australis-customization" zip, but it freezes and doesn't let me do anything.

Thanks
Comment 27 Alejandro Rodriguez [:Alopepeo] 2012-12-02 18:30:01 PST
(In reply to Alejandro from comment #26)
> Where can i download the latest try-builds? I test the build into
> "australis-customization" zip, but it freezes and doesn't let me do anything.
> 
> Thanks

Sorry, the problem was Stylish. I have an style that hides the app button and that caused the problem. 

Thanks anyway and sorry.
Comment 28 Jared Wein [:jaws] (please needinfo? me) 2013-01-23 12:51:32 PST
Created attachment 705508 [details] [diff] [review]
WIP v3.002 - backend (rebased)
Comment 29 Jared Wein [:jaws] (please needinfo? me) 2013-01-23 12:52:13 PST
Created attachment 705509 [details] [diff] [review]
WIP v3.002 - frontend (rebased)
Comment 30 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-01-23 18:28:39 PST
Created attachment 705686 [details] [diff] [review]
WiP v4 - backend (needs rebased)

For those following this project, and wondering WTF is going on: I unexpectedly had to take quite a bit of time off (concussion), so things stalled. While I'm still getting back on my feet, Jared is able to put some time into this.

This is the latest code I had - I had been about to upload these as a new version of the patches, so IIRC there are quite a bit of changes. Mostly in the implementation of the customization mode, and changes to support that. They applied cleaning at the end of November, but will need rebasing now.
Comment 31 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-01-23 18:29:33 PST
Created attachment 705687 [details] [diff] [review]
WiP v4 - frontend (needs rebased)
Comment 32 Alejandro Rodriguez [:Alopepeo] 2013-01-25 12:52:11 PST
Anyone knows wich UX or Nightly version will come with this features? (Approximately). Is just to have an idea of when they're going to be landed.

Thanks
Comment 33 Jared Wein [:jaws] (please needinfo? me) 2013-02-07 13:44:54 PST
Created attachment 711495 [details] [diff] [review]
WIP v4.01 - backend (rebased)
Comment 34 Jared Wein [:jaws] (please needinfo? me) 2013-02-07 13:45:29 PST
Created attachment 711496 [details] [diff] [review]
WIP v4.01 - frontend (rebased)
Comment 35 Jared Wein [:jaws] (please needinfo? me) 2013-02-07 13:47:25 PST
Pushed the rebased patches to try server:
https://tbpl.mozilla.org/?tree=Try&rev=04e9583cb516

(In reply to Alejandro from comment #32)
> Anyone knows wich UX or Nightly version will come with this features?
> (Approximately). Is just to have an idea of when they're going to be landed.

Approximate goal for this feature is Nightly 22.
Comment 36 Jared Wein [:jaws] (please needinfo? me) 2013-02-21 10:28:42 PST
Created attachment 716660 [details] [diff] [review]
WIP Patch v4.02

Rebased and fixed some small issues. Uploading this WIP patch here so mconley can split out the menu panel from the customization code.
Comment 37 Mike Conley (:mconley) - (Needinfo me!) 2013-02-22 13:13:08 PST
Created attachment 717280 [details] [diff] [review]
AppMenu WIP Patch v4.03
Comment 38 Mike Conley (:mconley) - (Needinfo me!) 2013-02-22 13:15:46 PST
Created attachment 717285 [details] [diff] [review]
Customization WIP Patch v4.03

Ok, I've split up WIP Patch v4.02 into two parts. I had to bring CustomizationUI.jsm over to the Menu patch, since it seems to be responsible for opening and managing the panel / button state. We might want to extract it out and put that work somewhere else, but this is a start.

I also had to de-bitrot v4.03 a little bit. These patches apply cleanly on top of UX from Feb 19 (840adae6b6e4).
Comment 39 Mike Conley (:mconley) - (Needinfo me!) 2013-02-22 13:47:34 PST
Comment on attachment 717280 [details] [diff] [review]
AppMenu WIP Patch v4.03

Moving the menu work out to bug 844281.
Comment 40 Mike Conley (:mconley) - (Needinfo me!) 2013-02-28 10:37:36 PST
Created attachment 719573 [details] [diff] [review]
Customization WIP Patch v4.04

Updated for latest UX (1dda6c9bc6a6 - Feb 28, 2013)
Comment 41 Mark Goodwin [:mgoodwin] 2013-03-22 04:56:34 PDT
(In reply to Mike Conley (:mconley) from comment #40)
> Created attachment 719573 [details] [diff] [review]
> Customization WIP Patch v4.04
> 
> Updated for latest UX (1dda6c9bc6a6 - Feb 28, 2013)

I'm attempting to apply this patch to UX and failing; from looking at the patches there seem to be some references to PanelUI pieces that aren't in UX. am I missing something? Is there another patch somewhere I must apply first?
Comment 42 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 07:26:13 PDT
(In reply to Mark Goodwin [:mgoodwin] from comment #41)
> (In reply to Mike Conley (:mconley) from comment #40)
> > Created attachment 719573 [details] [diff] [review]
> > Customization WIP Patch v4.04
> > 
> > Updated for latest UX (1dda6c9bc6a6 - Feb 28, 2013)
> 
> I'm attempting to apply this patch to UX and failing; from looking at the
> patches there seem to be some references to PanelUI pieces that aren't in
> UX. am I missing something? Is there another patch somewhere I must apply
> first?

Current work is proceeding at https://tbpl.mozilla.org/?tree=Jamun. Please download the builds from there or use https://hg.mozilla.org/projects/jamun for source.
Comment 43 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:00:31 PDT
Created attachment 728398 [details] [diff] [review]
9a2477c31b28.patch (part 1)
Comment 44 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:01:20 PDT
Created attachment 728399 [details] [diff] [review]
de041c8ef0c7.patch (part 2)
Comment 45 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:02:24 PDT
Created attachment 728400 [details] [diff] [review]
89287cdca338.patch (part 3)
Comment 46 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:03:00 PDT
Created attachment 728401 [details] [diff] [review]
e4ac9e227efe.patch (part 4)
Comment 47 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:05:31 PDT
Created attachment 728403 [details] [diff] [review]
8bc3c78c12fe.patch (part 5)
Comment 48 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:06:15 PDT
Created attachment 728404 [details] [diff] [review]
6235f69f1647.patch (part 6)
Comment 49 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:07:13 PDT
Created attachment 728405 [details] [diff] [review]
53cf1ebc6e85.patch (part 7)
Comment 50 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:08:10 PDT
Created attachment 728407 [details] [diff] [review]
d678b862dbfd.patch (part 8)
Comment 51 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:08:54 PDT
Created attachment 728409 [details] [diff] [review]
deec4e29ba17.patch (part 9)
Comment 52 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:11:27 PDT
Created attachment 728411 [details] [diff] [review]
fd97c40999fc.patch (part 10)
Comment 53 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:12:02 PDT
Created attachment 728412 [details] [diff] [review]
d410a4e584e3.patch (part 11)
Comment 54 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:12:41 PDT
Created attachment 728414 [details] [diff] [review]
f0e94249a97a.patch (part 12)
Comment 55 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:13:42 PDT
Created attachment 728415 [details] [diff] [review]
0c5206f0ed6b.patch (part 13)
Comment 56 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:15:09 PDT
Created attachment 728418 [details] [diff] [review]
952c9ccc247d.patch (part 14)
Comment 57 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:16:00 PDT
Created attachment 728419 [details] [diff] [review]
7534fd7a2bac.patch (part 15)
Comment 58 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:16:32 PDT
Created attachment 728420 [details] [diff] [review]
b9bee2009e45.patch (part 16)
Comment 59 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:21:13 PDT
Created attachment 728425 [details] [diff] [review]
a18848287a4f.patch (part 17)
Comment 60 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:22:38 PDT
Created attachment 728426 [details] [diff] [review]
48f24a30c6ac.patch (part 18)
Comment 61 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:23:13 PDT
Created attachment 728427 [details] [diff] [review]
e907595d9ce3.patch (part 19)
Comment 62 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:23:49 PDT
Created attachment 728428 [details] [diff] [review]
c1859c500536.patch (part 20)
Comment 63 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:24:29 PDT
Created attachment 728430 [details] [diff] [review]
29650070976e.patch (part 21)
Comment 64 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:25:05 PDT
Created attachment 728432 [details] [diff] [review]
176c822ef6a3.patch (part 22)
Comment 65 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:25:36 PDT
Created attachment 728433 [details] [diff] [review]
cbe88d315a03.patch (part 23)
Comment 66 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:26:21 PDT
Created attachment 728435 [details] [diff] [review]
9e5a55078be5.patch (part 24)
Comment 67 Jared Wein [:jaws] (please needinfo? me) 2013-03-22 14:39:55 PDT
Created attachment 728439 [details] [diff] [review]
Milestone 1 patch

This is a big rollup patch. Hopefully this will be easier to review and contain less obsoleted code. The parts to review can be split up by file for simplicity.

Blair, can you review the code changes in /browser/base/*
Mike, can you review the code changes in /browser/components/*, /browser/modules/*, /toolkit/mozapps/*
I'll review the changes in /browser/themes/*, toolkit/themes/*, /browser/locales/*
Comment 68 Frederik Braun [:freddyb] [unavailable June 4th to August 29th] 2013-03-25 07:02:53 PDT
Can someone explain how this is supposed to work? It looks quite a bit different to what I expected from the Feature page on https://wiki.mozilla.org/Features/Desktop/Panel_Menu
Comment 69 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 07:05:13 PDT
(In reply to Frederik Braun [:freddyb] from comment #68)
> Can someone explain how this is supposed to work? It looks quite a bit
> different to what I expected from the Feature page on
> https://wiki.mozilla.org/Features/Desktop/Panel_Menu

We're attacking this bug iteratively, and this is our first iteration. We'll review this, and generate patches on top of it, and review those, etc.

So yes, it will look a bit different from https://wiki.mozilla.org/Features/Desktop/Panel_Menu - we haven't quite gotten there yet.
Comment 70 Jared Wein [:jaws] (please needinfo? me) 2013-03-25 09:58:46 PDT
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch

Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------

I think we can undo the changes to syncProgress.css and all three of the inContentUI.css files. They aren't necessary for this first part, and will make the impact more focused.

I'll put together a patch to fix the issues that I have found and request review for it.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +14,5 @@
>  <!-- LOCALIZATION NOTE (mainWindow.titlePrivateBrowsingSuffix): This will be appended to the window's title
>                                                                  inside the private browsing mode -->
>  <!ENTITY mainWindow.titlePrivateBrowsingSuffix "(Private Browsing)">
>  
> +<!ENTITY appmenu.title                       "&brandFullName; menu">

"Customize and Control &brandFullName;"

::: browser/themes/linux/panelUIOverlay.css
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +%filter substitution
> +
> +%define menuPanelWidth 23em

Uber nit, the linux/panelUIOverlay.css and osx/panelUIOverlay.css have a blank line between the %define and the %filter, but in windows/panelUIOverlay.css the %define is immediately below the %filter. We should make all these files the same.

Also, they all have the same menuPanelWidth. Should that get moved to the shared file?

::: browser/themes/shared/panelUIOverlay.inc.css
@@ +77,5 @@
> +}
> +
> +#PanelUI-subViews {
> +  -moz-stack-sizing: ignore;
> +  background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */

This and other places should use the unprefixed syntax. Note that the we can't just drop the -moz- to fix this.

@@ +79,5 @@
> +#PanelUI-subViews {
> +  -moz-stack-sizing: ignore;
> +  background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */
> +  background-color: -moz-dialog;
> +  box-shadow: -1px 0px 0px rgba(0, 0, 0, 0.2), -1px 0px 2px rgba(0, 0, 0, 0.1), 1px 0px 0px rgba(255, 255, 255, 0.2) inset; /* stolen from shorlander's mock-up */

Probably can remove the "stolen from shorlander's mockup" comment.

@@ +206,5 @@
> +#customization-palette .toolbarbutton-text {
> +  display: none;
> +}
> +
> +/* XXXunf Hack! Copied from general toolbar styles in winstripe. */

This styling will be used for all platforms. Should the windows theme reference this? Or should they just be duplicated since they serve different purposes and they may intentionally diverge over time? I'm fine with the duplication.

@@ +240,5 @@
> +  background-color: hsla(210,54%,20%,.15);
> +  border-color: hsla(210,54%,20%,.3) hsla(210,54%,20%,.35) hsla(210,54%,20%,.4);
> +  box-shadow: 0 1px 1px hsla(210,54%,20%,.1) inset,
> +              0 0 1px hsla(210,54%,20%,.2) inset,
> +              /* allows winstripe-keyhole-forward-clip-path to be used for non-hover as well as hover: */

This comment doesn't apply here.

::: browser/themes/windows/browser.css
@@ +2805,5 @@
> +
> +
> +
> +
> +/* Woo! Customization Mode! YEA!!!! */

We can remove this comment now, and move this code closer to the above section.

::: browser/themes/windows/jar.mn
@@ +19,5 @@
>  #ifdef MOZ_SERVICES_SYNC
>          skin/classic/browser/aboutSyncTabs.css
>  #endif
>          skin/classic/browser/actionicon-tab.png
> +        skin/classic/aero/browser/appmenu.png                        (appmenu.png)

This is in the wrong section, it should be down with the other aero files. Also, the rename here (appmenu.png) is unnecessary since it's the same as the original filename.

We also need the appmenu.png images for linux and osx.
Comment 71 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 11:37:25 PDT
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch

Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------

So, I had a sweet 2 hour review, and then Bugzilla ate it. :/ Doing another one now, hopefully it's as cool as my last one. *sigh*

I'll review CustomizeMode.jsm next.

::: browser/components/CustomizableUIMediator.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Needs documentation describing what this service is responsible for.

@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI", 

Trailing whitespace and busted alignment on line 13.

@@ +13,5 @@
> +                                   "resource:///modules/CustomizableUI.jsm");
> +
> +
> +function CustomizableUIMediator() {
> +  this.wrappedJSObject = this;

Does any caller ever use the wrappedJSObject? If not, we should probably axe it.

::: browser/modules/CustomizableUI.jsm
@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +
> +const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

As per https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style, consts should be k prefixed and camelcased, like:

const kNSXUL = ...;
const kPrefCustomizationState = ...;
etc.

@@ +16,5 @@
> +const PREF_CUSTOMIZATION_STATE        = "browser.uiCustomization.state";
> +const PREF_CUSTOMIZATION_AUTOADD      = "browser.uiCustomization.autoAdd";
> +const PREF_CUSTOMIZATION_DEBUG        = "browser.uiCustomization.debug";
> +
> +const kEventHandlers = {

This name is too generic. We either need a better name, or some documentation here, or both.

@@ +28,5 @@
> + */
> +let gPalette = new Map();
> +
> +/**
> + * gAreas maps area IDs to properties about those areas. An area is a place

Sets of properties might be more accurate.

@@ +31,5 @@
> +/**
> + * gAreas maps area IDs to properties about those areas. An area is a place
> + * where a widget can be put.
> + */
> +let gAreas = new Map();

Blair: I switched from using generic area names to using IDs for the area nodes. I didn't see any advantage to another layer of abstraction - but I could be wrong... any objections?

@@ +101,5 @@
> +  gDebug = Services.prefs.getBoolPref(PREF_CUSTOMIZATION_DEBUG);
> +} catch (e) {}
> +
> +function LOG(aMsg) {
> +  if (gDebug)

Throughout both CustomizableUI.jsm and CustomizeMode.jsm, I see a lot of no-braces-for-one-liners. According to https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style, we need to brace these - so this is just a general issue for both files.

@@ +119,5 @@
> +    this.registerArea(CustomizableUI.AREA_PANEL);
> +    this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> +  },
> +
> +  _defineBuiltInWidgets: function() {

A general note - all of these functions require documentation.

@@ +120,5 @@
> +    this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> +  },
> +
> +  _defineBuiltInWidgets: function() {
> +    //XXXunf Need a better place to define these.

Is this a better-enough place?

@@ +253,5 @@
> +  },
> +
> +  registerArea: function(aName, aProperties) {
> +    if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> +      throw Error("Invalid area name");

throw new Error

@@ +255,5 @@
> +  registerArea: function(aName, aProperties) {
> +    if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> +      throw Error("Invalid area name");
> +    if (gAreas.has(aName))
> +      throw Error("Area already registered");

throw new Error

@@ +274,5 @@
> +    let document = aToolbar.ownerDocument;
> +    let area = aToolbar.id;
> +
> +    if (!gAreas.has(area))
> +      throw Error("Unknown customization area");

throw new Error

@@ +286,5 @@
> +      let legacyState = aToolbar.getAttribute("currentset");
> +      if (legacyState)
> +        legacyState = legacyState.split(",");
> +      //XXXunf should the legacy attribute be purged? 
> +      //       kinda messes up switching to older builds

I think we should do best-effort to maintain currentset, until this new customization stuff is shipped, and then I think we should drop currentset like a hot rock.

@@ +320,5 @@
> +      if (provider == CustomizableUI.PROVIDER_XUL && aArea == CustomizableUI.AREA_PANEL)
> +        this.ensureButtonClosesPanel(node);
> +
> +      container.insertBefore(node, currentNode);
> +      currentNode = node.nextSibling;

Since node was just inserted before currnetNode, node.nextSibling is already currentNode, so I don't think we need line 324.

@@ +328,5 @@
> +      let palette = aAreaNode.toolbox ? aAreaNode.toolbox.palette : null;
> +      let limit = currentNode.previousSibling;
> +      let node = container.lastChild;
> +      while (node != limit) {
> +        // XXXunf Depreciating the old "removable" attribute, is this right?

Not sure. The Social API button, for example, isn't supposed to be move-able. I'm not even sure how that is supposed to play in this new Australis customization world we're building here. Worth investigating.

@@ +355,5 @@
> +    if (this.isSpecialWidget(aWidgetId))
> +      return CustomizableUI.PROVIDER_SPECIAL;
> +    if (gPalette.has(aWidgetId))
> +      return CustomizableUI.PROVIDER_API;
> +    //XXXunf Er, this isn't entirely true.. could also not exist.

We should handle that case, and log or throw as necessary.

@@ +393,5 @@
> +
> +    aPanel.toolbox = document.getElementById("navigator-toolbox");
> +    aPanel.customizationTarget = panelContents;
> +
> +    //XXXjaws Not sure if this is correct, the build area while in customization mode is actually the panelContents.

This has been corrected - this comment can be removed.

@@ +398,5 @@
> +    let placements = gPlacements.get(CustomizableUI.AREA_PANEL);
> +    this.buildArea(CustomizableUI.AREA_PANEL, placements, aPanel);
> +    this.registerBuildArea(CustomizableUI.AREA_PANEL, aPanel);
> +
> +    // XXXmconley: not sure if registering the panel is when we'd want to

I still think this is true. I think windows on load should register with CustomizableUI, so that when they unload, CustomizableUI can clean up.

@@ +451,5 @@
> +    if (!areaNodes)
> +      return;
> +    
> +    for (let areaNode of areaNodes) {
> +      let container = areaNode.customizationTarget;

Can we assume that every area node has a customizationTarget? If not, we should check for it when they register.

@@ +544,5 @@
> +
> +    if (aToolbox.palette) {
> +      // Attempt to locate an node with a matching ID within
> +      // the palette.
> +      for (let node of aToolbox.palette.children) {

I to believe we could just use aToolbox.palette.querySelector("#" + aId) here.

@@ +552,5 @@
> +    }
> +    return null;
> +  },
> +
> +  //XXXunf Need to free this from aMenu's tyranny.

Not sure what this comment means, though I do appreciate the use of the word tyranny. :) This should either be made more clear, or just be removed.

@@ +557,5 @@
> +  buildWidget: function(aDocument, aMenu, aWidget) {
> +    if (typeof aWidget == "string")
> +      aWidget = gPalette.get(aWidget);
> +    if (!aWidget)
> +      return null;

We should log / throw here, instead of just returning null.

@@ +621,5 @@
> +        } catch (e) {
> +          Cu.reportError(e);
> +        }
> +      } else {
> +        //XXXunf Need to think this through more, and formalize.

Blair: What scenario is this else for?

@@ +668,5 @@
> +      if (!this.getPlacementOfWidget(node.id))
> +        widgets.add(node.id);
> +    }
> +
> +    // XXXmconley: is there any good reason why we're not just returning

As far as I can tell, the answer is no.

@@ +675,5 @@
> +  },
> +
> +  getPlacementOfWidget: function(aWidgetId) {
> +    let widget = gPalette.get(aWidgetId);
> +    //XXXunf Pretty sure this won't work anymore, thanks to restoring widget

registerWidget isn't a function anymore, and restoring widget placement is handled elsewhere. I think this block can go.

@@ +694,5 @@
> +  },
> +
> +  addWidgetToArea: function(aWidgetId, aArea, aPosition) {
> +    if (!gAreas.has(aArea))
> +      throw Error("Unknown customization area");

throw new Error

@@ +913,5 @@
> +    if (aForceSave === true)
> +      gDirty = true;
> +    this.saveState();
> +  },
> +  //XXXunf Used only for conveience when developing stuff, need to remove this.

This function is no longer used and can be removed.

@@ +1059,5 @@
> +
> +    if (typeof aData.id != "string" || !/^[a-z0-9-_]{1,}$/i.test(aData.id))
> +      return null;
> +
> +    const REQ_STRING_PROPS = ["id", "name"];

consts should be k prefixed and camel-cased.

@@ +1099,5 @@
> +                           aData.onCommand :
> +                           null;
> +    } else if (widget.type == "view") {
> +      if (typeof aData.viewId != "string")
> +        return null;

Should log / throw here.

@@ +1147,5 @@
> +    this.notifyListeners("onWidgetDestroyed", aWidgetId);
> +  },
> +
> +  registerManifest: function(aBaseLocation, aDirective) {
> +    let tokens = aData.split(/\s+/);

aData is not defined - did you mean aDirective?

@@ +1310,5 @@
> +    );
> +  },
> +  getWidgetsInArea: function(aArea) {
> +    if (!gAreas.has(aArea))
> +      throw Error("Unknown customization area");

throw new Error

@@ +1312,5 @@
> +  getWidgetsInArea: function(aArea) {
> +    if (!gAreas.has(aArea))
> +      throw Error("Unknown customization area");
> +    if (!gPlacements.has(aArea))
> +      throw Error("Area not yet restored");

throw new Error

@@ +1322,5 @@
> +  },
> +  get areas() {
> +    return [area for ([area, props] of gAreas)];
> +  },
> +  getCustomizeTargetsForArea: function(aArea) {

This is getting a bit complex. The different customize targets for an area are the different instances of that area across each browser window. We must make sure that we don't give the impression that an area can have multiple customize targets. I do know that UX wants to have, for example, multiple targets in the main panel...those should be individual areas, I think.

@@ +1333,5 @@
> +Object.freeze(this.CustomizableUI);
> +
> +
> +
> +function WidgetGroupWrapper(aWidget) {

The purpose and function of these wrappers isn't 100% clear to me. I think we need some documentation here.
Comment 72 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 11:51:22 PDT
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch

Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/CustomizeMode.jsm
@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = ["CustomizeMode"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +const DEBUG = true;

kDebug, as per https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style. Should default to false.

@@ +33,5 @@
> +  // areas and added to the palette, they're added to the visible palette.
> +  // _stowedPalette is a reference to the old invisible palette so we can
> +  // restore gNavToolbox.palette to its original state after exiting
> +  // customization mode.
> +  this._stowedPalette = null;

Might as well just toss this into the prototype.

@@ +39,5 @@
> +
> +CustomizeMode.prototype = {
> +  window: null,
> +  document: null,
> +  palette: null,

window and document are, imo, self explanatory. palette and areas should have some documentation as to their purpose.

@@ +42,5 @@
> +  document: null,
> +  palette: null,
> +  areas: null,
> +
> +  enter: function() {

Each of these functions requires documentation, and newlines between.

@@ +50,5 @@
> +    let document = this.document;
> +
> +    // Disable the toolbar context menu items
> +    let menubar = document.getElementById("main-menubar");
> +    for (let childNode of menubar.childNodes)

So I think what might be cleaner is if we fire a "customize start" event, and have code in browser.js listen for and react to that event to "batten down the hatches" for customization mode. This is also more extensible.

@@ +89,5 @@
> +
> +      // Add drag-and-drop event handlers to all of the customizable areas.
> +      self.areas = [];
> +      for (let area of CustomizableUI.areas) {
> +        let targets = CustomizableUI.getCustomizeTargetsForArea(area);

This should just get the area instance for this particular window.

@@ +118,5 @@
> +    CustomizableUI.removeListener(this);
> +
> +    this.depopulatePalette();
> +
> +    this.visiblePalette.removeEventListener("dragstart", this._onDragStart);

These won't remove correctly, since the function signatures don't match with when we added the listeners.

@@ +131,5 @@
> +    for (let target of this.areas) {
> +      for (let toolbarItem of target.children) {
> +        this.unwrapToolbarItem(toolbarItem);
> +      }
> +      target.removeEventListener("dragstart", this._onDragStart);

Same as above - these won't remove correctly.

@@ +142,5 @@
> +    let browser = document.getElementById("browser");
> +    browser.parentNode.selectedPanel = browser;
> +
> +    // Update global UI elements that may have been added or removed
> +    if (aToolboxChanged) {

aToolboxChanged is *never* true, so this block is never executed.

As in enter, I think we should fire a "customization done" event when completed, and then have browser.js react to it to get us back to normal browsing. I also think we should track whether or not customization took place, and pass a "aToolboxChanged" equivalent bool to the event listeners.

@@ +168,5 @@
> +      //             to old builds. We might want to keep this around for a little
> +      //             bit.
> +      this.persistCurrentSets();
> +    } else {
> +      LOG("Exiting customize mode with no changes.");

This message isn't necessarily true, and should be removed.

@@ +212,5 @@
> +  populatePalette: function() {
> +    let toolboxPalette = this.window.gNavToolbox.palette;
> +
> +    let unusedWidgets = CustomizableUI.getUnusedWidgets(toolboxPalette);
> +    LOG("List of unused widgets: " + unusedWidgets);

This log message just outputs [Object, Object, Object...]. Not useful, should be removed.

@@ +215,5 @@
> +    let unusedWidgets = CustomizableUI.getUnusedWidgets(toolboxPalette);
> +    LOG("List of unused widgets: " + unusedWidgets);
> +    for (let widget of unusedWidgets) {
> +      let paletteItem = this.makePaletteItem(widget, "palette");
> +      paletteItem.setAttribute("style", "display: inline-block; -moz-orient: vertical; padding: 1em;"); //XXXunf Hack!

Hack indeed - this should be taken care of in CSS.

@@ +222,5 @@
> +
> +    this._stowedPalette = this.window.gNavToolbox.palette;
> +    this.window.gNavToolbox.palette = this.visiblePalette;
> +  },
> +  //XXXunf Maybe this should use -moz-element instead of wrapping the node?

I'm not opposed to trying this - makes more sense to me to "imitate"/"mimic" the toolbaritems rather than wrapping them. Also means we don't have to slurp the items in and out of the invisible palette each time.

Might be worth investigating later.

@@ +259,5 @@
> +        }
> +
> +        this._stowedPalette.appendChild(widgetNode);
> +      } else if (provider = CustomizableUI.PROVIDER_API) {
> +        //XXXunf Currently this doesn't destroy the (now unused) node. It would

Let's just leave this for now, and deal with it in a later iteration.

@@ +281,5 @@
> +  },
> +  createWrapper: function(aNode, aPlace) {
> +    let wrapper = this.document.createElement("toolbarpaletteitem");
> +
> +    //XXXjaws Is this attribute needed?

This conversation should be replaced with an explanation of what "place" is for.

@@ +344,5 @@
> +  //XXXmconley While CustomizableUI.jsm uses prefs to preserve state, we might
> +  //           also want to (try) persisting with currentset as well to make it
> +  //           less painful to switch to older builds.
> +  persistCurrentSets: function()  {
> +    //XXXjaws The toolbar bindings that are included in this changeset (/browser/base/content/toolbar.xml)

jaws is right - we should try to fix the binding to inherit from toolkit toolbar.

@@ +352,5 @@
> +    let document = this.document;
> +    let toolbar = document.getElementById("nav-bar");
> +
> +    // Calculate currentset and store it in the attribute.
> +    var currentSet = toolbar.currentSet;

Use let, not var.

@@ +374,5 @@
> +  },
> +  onWidgetDestroyed: function(aWidgetId) {
> +  },
> +
> +  _onDragStart: function(aEvent) {

In the following functions, I see a lot of "var" usage. Switch these to "let".

@@ +383,5 @@
> +        return;
> +      item = item.parentNode;
> +    }
> +
> +    item.setAttribute("dragactive", "true");

What is this attribute used for?

@@ +415,5 @@
> +    let targetParent = targetNode.parentNode;
> +    let targetArea = this._getCustomizableParent(targetNode);
> +    let originArea = this._getCustomizableParent(draggedWrapper);
> +
> +    // Do nothing if the target is not the panel or the palette, or if the

Comment is inaccurate - do nothing if the targetArea or originArea aren't customizable.

@@ +468,5 @@
> +      let itemFlex = parseInt(draggedWrapper.getAttribute("flex"), 10);
> +      parent.setAttribute("flex", parentFlex + itemFlex);
> +    }
> +
> +    LOG("Done!");

Not a very useful log message. Probably safe to remove.
Comment 73 Jared Wein [:jaws] (please needinfo? me) 2013-03-25 12:38:33 PDT
Created attachment 729145 [details] [diff] [review]
Addressing my review comments
Comment 74 Jared Wein [:jaws] (please needinfo? me) 2013-03-25 13:57:14 PDT
Created attachment 729180 [details] [diff] [review]
Addressing my review comments
Comment 75 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 15:32:46 PDT
Created attachment 729248 [details] [diff] [review]
Addressing my review comments as well
Comment 76 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 15:35:13 PDT
Hey Blair,

Is it possible for you to review your section sometime today? Jared and I would like to land a version of this patch on UX sometime on March 26th.

Thanks,

-Mike
Comment 77 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 15:41:40 PDT
Comment on attachment 729180 [details] [diff] [review]
Addressing my review comments

Review of attachment 729180 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Thanks Jared!

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +14,5 @@
>  <!-- LOCALIZATION NOTE (mainWindow.titlePrivateBrowsingSuffix): This will be appended to the window's title
>                                                                  inside the private browsing mode -->
>  <!ENTITY mainWindow.titlePrivateBrowsingSuffix "(Private Browsing)">
>  
> +<!ENTITY appmenu.title                       "Customize and Control &brandFullName;">

I'm assuming this was part of a spec somewhere.

::: browser/themes/windows/browser.css
@@ +2841,5 @@
>  #customization-panelHolder > #PanelUI-mainView {
>    box-shadow: inset 0 0 3px blue;
>    border-radius: 2px;
>  }
> +/* end Customization mode */

I added a rule here in my fixup patch, so we're going to conflict here. No biggie.
Comment 78 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 15:46:33 PDT
Created attachment 729253 [details] [diff] [review]
Addressing my review comments (apply on top of jaws' patch)

Re-based on top of jaws' fixes.
Comment 79 Jared Wein [:jaws] (please needinfo? me) 2013-03-25 16:14:03 PDT
Comment on attachment 729248 [details] [diff] [review]
Addressing my review comments as well

Review of attachment 729248 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/CustomizableUI.jsm
@@ +337,5 @@
>          continue;
>        }
>  
> +      if (provider == CustomizableUI.PROVIDER_XUL
> +          && aArea == CustomizableUI.AREA_PANEL) {

nit: && should go at the end of the first line, like so:

if (provider == CustomizableUI.PROVIDER_XUL &&
    aArea == CustomizableUI.AREA_PANEL) {
  ...
}

here, and lower in the file too.

::: browser/modules/CustomizeMode.jsm
@@ +115,5 @@
>  
>      let window = this.window;
>      let document = this.document;
>  
> +    // Let everybody in this window know that we're about to customize.

// Let everybody in this window know that we're about to finish customizing.
Comment 80 Jared Wein [:jaws] (please needinfo? me) 2013-03-25 16:14:58 PDT
Comment on attachment 729253 [details] [diff] [review]
Addressing my review comments (apply on top of jaws' patch)

Review of attachment 729253 [details] [diff] [review]:
-----------------------------------------------------------------

See feedback from previous review.
Comment 81 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:40:47 PDT
Created attachment 729287 [details] [diff] [review]
After-big-patch Follow-up #1 - Make sure panel size is sync'd.

So, the UX team had some further suggestions / requests for us to get in before we try merging this stuff over to the UX branch. This is part #1 of #9 for these fixes.

Don't worry - these fixes are pretty small.

The big roll-up patch should have these 9 patches applied on top of it once it has full r+.
Comment 82 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:42:02 PDT
Created attachment 729289 [details] [diff] [review]
After-big-patch Follow-up #2 - Allow items to be dropped among palette items

Before, we only appended to the palette. This allows us to drop amongst the palette items.
Comment 83 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:43:30 PDT
Created attachment 729290 [details] [diff] [review]
After-big-patch Follow-up #3 - Make programmatically generated widgets behave

Programmatically generated widgets wouldn't behave properly after dragging / dropping them once. After dragging or dropping them, they'd act as their unwrapped selves.
Comment 84 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:45:17 PDT
Created attachment 729291 [details] [diff] [review]
After-big-patch Follow-up #4 - Make going into customize mode more awesome

This patch replace the flat blue background with the grid texture, which I cribbed from the shorlander / bwinton mock-up. It's a JPG for now, so we're going to want to get this replaced.
Comment 85 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:46:33 PDT
Created attachment 729292 [details] [diff] [review]
After-big-patch Follow-up #5 - More ways to exit customize mode

This patch allows us to exit customize mode either by pressing Esc, or by clicking on the menu panel button.
Comment 86 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:47:41 PDT
Created attachment 729293 [details] [diff] [review]
After-big-patch Follow-up #6 - Add rudimentary drop indicator to toolbar

This adds a very basic (and temporary) drop indicator to the nav-bar when customizing.
Comment 87 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:48:32 PDT
Created attachment 729294 [details] [diff] [review]
After-big-patch Follow-up #7 - Make drop targets glow blue instead of purple

Our drop targets had a purplish hue about them. This makes them more blue.
Comment 88 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:49:15 PDT
Created attachment 729295 [details] [diff] [review]
After-big-patch Follow-up #8 - Disable the menu button when customizing

The menu button should be in the disabled state while in customizing mode.
Comment 89 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:50:21 PDT
Created attachment 729296 [details] [diff] [review]
After-big-patch Follow-up #9 - Add a grab effect when drag starts

When "picking up" a customizable item, we scale it up and increase its opacity.
Comment 90 Mike Conley (:mconley) - (Needinfo me!) 2013-03-25 16:51:29 PDT
So, just to give a sense of review priority: the big base-line patch is absolutely top priority.

The After-big-patch Follow-ups come after.
Comment 91 Dão Gottwald [:dao] 2013-03-26 06:26:53 PDT
(In reply to Mike Conley (:mconley) from comment #71)
> > +const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> 
> As per
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style,
> consts should be k prefixed and camelcased, like:

No, there's no such rule. That document just explains what k means when it's used. It correctly points out that we don't use that style across the code base.

> > +  registerArea: function(aName, aProperties) {
> > +    if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> > +      throw Error("Invalid area name");
> 
> throw new Error

Why?
Comment 92 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 06:51:52 PDT
(In reply to Dão Gottwald [:dao] from comment #91)
> (In reply to Mike Conley (:mconley) from comment #71)
> > > +const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> > 
> > As per
> > https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style,
> > consts should be k prefixed and camelcased, like:
> 
> No, there's no such rule. That document just explains what k means when it's
> used. It correctly points out that we don't use that style across the code
> base.

Ah, true. You're right. Didn't realize we had a spectrum of styles for constants in Javascript.

> 
> > > +  registerArea: function(aName, aProperties) {
> > > +    if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> > > +      throw Error("Invalid area name");
> > 
> > throw new Error
> 
> Why?

Because I saw both "throw new Error" and "throw Error" being used, and I opted for visual consistency. Functionally, I believe they're equivalent.
Comment 93 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 07:09:45 PDT
Comment on attachment 729293 [details] [diff] [review]
After-big-patch Follow-up #6 - Add rudimentary drop indicator to toolbar

Review of attachment 729293 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few minor suggestions, but otherwise, this looks good to me.

::: browser/modules/CustomizeMode.jsm
@@ +105,5 @@
>                                           this._onDragStart.bind(this));
>      this.visiblePalette.addEventListener("dragover",
>                                           this._onDragOver.bind(this));
> +    this.visiblePalette.addEventListener("dragexit",
> +                                         this._onDragOver.bind(this));

Nope, this is wrong - this should be this._onDragExit.bind(this).

Probably my fault when de-bitrotting these.

Code review saves the day again!

@@ +409,5 @@
> +    let targetParent = targetNode.parentNode;
> +    let targetArea = this._getCustomizableParent(targetNode);
> +    let originArea = this._getCustomizableParent(draggedWrapper);
> +
> +    // Do nothing if the target is not the panel or the palette, or if the

Comment should be, "Do nothing if the target or origin is not customizable"

@@ +421,5 @@
> +    // the target.
> +    let position = Array.indexOf(targetParent.children, targetNode);
> +    let dragOverItem = position == -1 ? targetParent.lastChild : targetParent.children[position];
> +
> +    if (this._dragOverItem && dragOverItem != this._dragOverItem)

Brace the one-liner.

@@ +508,5 @@
>      }
>    },
>  
> +  _onDragExit: function(aEvent) {
> +    if (this._dragOverItem)

Brace the one-liner

@@ +514,5 @@
> +  },
> +
> +  // XXXjaws Show a ghost image or blank area where the item could be added, instead of black bar
> +  _setDragActive: function(aItem, aValue) {
> +    var node = aItem;

These var's should be let's.

@@ +523,5 @@
> +      node = aItem.lastChild;
> +      value = direction == "ltr"? "right" : "left";
> +    }
> +
> +    if (!node)

Brace this one-liner

@@ +527,5 @@
> +    if (!node)
> +      return;
> +
> +    if (aValue) {
> +      if (!node.hasAttribute("dragover"))

Brace this one-liner

@@ +560,5 @@
> +function getPlaceForItem(aElement) {
> +  let place;
> +  let node = aElement;
> +  while (node && !place) {
> +    if (node.localName == "toolbar")

Brace the one-liners in here.

@@ +562,5 @@
> +  let node = aElement;
> +  while (node && !place) {
> +    if (node.localName == "toolbar")
> +      place = "toolbar";
> +    else if (node.id == "PanelUI-contents")

Instead of hard-coding this ID, I think we should use CustomizableUI.AREA_PANEL
Comment 94 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 08:17:53 PDT
Comment on attachment 729253 [details] [diff] [review]
Addressing my review comments (apply on top of jaws' patch)

Review of attachment 729253 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/CustomizableUIMediator.js
@@ +5,5 @@
> +/**
> + * This component is responsible for catching unhandled directive events from
> + * the chrome manifest parser, and sending those off to the CustomizableUI
> + * module to see if the unhandled directives are add-ons registering
> + * customizable widgets.

This whole file should just be removed for now, given bug 793093 was WONTFIXed. No point in having this until we know what alternate API (if any) we'll have.

::: browser/modules/CustomizableUI.jsm
@@ +21,5 @@
> + * The keys are the handlers that are fired when the event type (the value)
> + * is fired on the subview. A widget that provides a subview has the option
> + * of providing onViewShowing and onViewHiding event handlers.
> + */
> +const kSubviewEventHandlers = {

Hardcoding both handler and event seems error-prone - given the handler name is always going to be consistent, "onEventName" for an event named "EventName".

@@ +302,4 @@
>        //       kinda messes up switching to older builds
> +      //XXXmconley No, I don't think so - I think we want to make it easy to
> +      //           switch back and forth between builds until this thing hits
> +      //           release.

Yea, that was more of a long-term thought.

@@ +382,5 @@
>        return CustomizableUI.PROVIDER_API;
> +    }
> +
> +    // It's also possible that this widget doesn't exist (yet). In either case,
> +    // we fall back to the XUL provider.

My originally comment here meant that we fall back to the XUL provider, but we don't know for sure (at this point) whether it exists there either. So the API is technically lying. Ideally, it would be able to return an error value (or throw an exception) if it really didn't exist. Our code calling this function handles that fine, but this is a public API.

@@ +591,1 @@
>    buildWidget: function(aDocument, aMenu, aWidget) {

Re-add my to-do comment here (re-worded)? Depending on aMenu here is a hack that should be fixed.

@@ +1109,5 @@
>        icons: {}
>      };
>  
> +    if (typeof aData.id != "string" || !/^[a-z0-9-_]{1,}$/i.test(aData.id)) {
> +      throw new Error("Given an illegal id in normalizeWidget: " + aData.id);

If this is throwing an exception now, then a missing required property should do so too (and the case where it already exists). Also need to make sure the exception gets safely propagated to the outside caller, and doesn't break everything if this is during startup (because callers currently assume this doesn't throw).

::: browser/modules/CustomizeMode.jsm
@@ +7,5 @@
>  this.EXPORTED_SYMBOLS = ["CustomizeMode"];
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
> +const kDebug = true;

This should be changed to use the browser.uiCustomization.debug pref at some stage.

@@ +110,5 @@
> +                                            this._onDragStart.bind(this));
> +    this.visiblePalette.removeEventListener("dragover",
> +                                            this._onDragOver.bind(this));
> +    this.visiblePalette.removeEventListener("drop",
> +                                            this._onDragDrop.bind(this));

Fun fact: this.someFunction.bind(this) !== this.someFunction.bind(this)

Therefore, removing event listeners like this doesn't work, because you're trying to remove a different listener :( Need to store the value you originally pass into addEventListener() if you want to use bind().

Same issue later in this function too.

@@ +115,5 @@
>  
>      let window = this.window;
>      let document = this.document;
>  
> +    // Let everybody in this window know that we're about to customize.

This comment is inaccurate.
Comment 95 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 08:25:26 PDT
(In reply to Mike Conley (:mconley) from comment #71)
> Blair: I switched from using generic area names to using IDs for the area
> nodes. I didn't see any advantage to another layer of abstraction - but I
> could be wrong... any objections?

I think that should be fine. IIRC, it was just cosmetic.
Comment 96 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 08:33:12 PDT
(In reply to Mike Conley (:mconley) from comment #71)
> @@ +621,5 @@
> > +        } catch (e) {
> > +          Cu.reportError(e);
> > +        }
> > +      } else {
> > +        //XXXunf Need to think this through more, and formalize.
> 
> Blair: What scenario is this else for?

That was for when the widget was registered via chrome.manifest - since that's more declarative, you generally want to avoid including code in the definition. And the JSON parser doesn't support code anyway (can work around that, if need be). So in lieu of a callable onCommand property, it would send some kind of event/notification that the add-on would listen for.
Comment 97 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 08:56:07 PDT
Comment on attachment 729292 [details] [diff] [review]
After-big-patch Follow-up #5 - More ways to exit customize mode

Review of attachment 729292 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/CustomizeMode.jsm
@@ +361,5 @@
> +          this.exit();
> +        }
> +        break;
> +      case "click":
> +        if (aEvent.originalTarget = this.window.PanelUI.menuButton) {

This should also check aEvent.button == 0.
Comment 98 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 08:59:10 PDT
Comment on attachment 729293 [details] [diff] [review]
After-big-patch Follow-up #6 - Add rudimentary drop indicator to toolbar

Review of attachment 729293 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/CustomizeMode.jsm
@@ +126,5 @@
>                                              this._onDragStart.bind(this));
>      this.visiblePalette.removeEventListener("dragover",
>                                              this._onDragOver.bind(this));
> +    this.visiblePalette.removeEventListener("dragexit",
> +                                            this._onDragExit.bind(this));

Calling .bind() like this for add/removeEventListener is bad since it's not actually removing the event listener that was added before. Each call to bind() generates a new function, so calling removeEventListener with fn.bind isn't going to remove the original event listener. If we need to use .bind(), then we need to cache the result of .bind() and pass that return value to both add/removeEventListener.
Comment 99 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 09:00:46 PDT
Comment on attachment 729289 [details] [diff] [review]
After-big-patch Follow-up #2 - Allow items to be dropped among palette items

Review of attachment 729289 [details] [diff] [review]:
-----------------------------------------------------------------

... even though the next patch rewrites the only change here...
Comment 100 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 09:02:31 PDT
Comment on attachment 729296 [details] [diff] [review]
After-big-patch Follow-up #9 - Add a grab effect when drag starts

Review of attachment 729296 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the event listener leak.

::: browser/modules/CustomizeMode.jsm
@@ +290,5 @@
>        wrapper.setAttribute("flex", aNode.getAttribute("flex"));
>      }
>  
> +    wrapper.addEventListener("mousedown", this._onMouseDown.bind(this));
> +    wrapper.addEventListener("mouseup", this._onMouseUp.bind(this));

Same comment here as in previous review, we can't use .bind() like this for event listeners since we won't be able to remove these event listeners.

::: browser/themes/windows/browser.css
@@ +2841,5 @@
>  }
>  
> +toolbarpaletteitem {
> +  cursor: -moz-grab;
> +  transition: -moz-transform, background-color, border-color, box-shadow;

-moz-transform was unprefixed, we can just use `transform`.
Comment 101 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 09:04:33 PDT
(In reply to Mike Conley (:mconley) from comment #77)
> Comment on attachment 729180 [details] [diff] [review]
> Addressing my review comments
> 
> Review of attachment 729180 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me. Thanks Jared!
> 
> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +14,5 @@
> >  <!-- LOCALIZATION NOTE (mainWindow.titlePrivateBrowsingSuffix): This will be appended to the window's title
> >                                                                  inside the private browsing mode -->
> >  <!ENTITY mainWindow.titlePrivateBrowsingSuffix "(Private Browsing)">
> >  
> > +<!ENTITY appmenu.title                       "Customize and Control &brandFullName;">
> 
> I'm assuming this was part of a spec somewhere.

This wasn't part of a spec, but I think that it describes the intentions of the menu and gives better information to the user as to what they can expect before they click on the button.
Comment 102 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 09:05:15 PDT
Comment on attachment 729290 [details] [diff] [review]
After-big-patch Follow-up #3 - Make programmatically generated widgets behave

Review of attachment 729290 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/CustomizeMode.jsm
@@ +221,5 @@
>  
>    wrapToolbarItem: function(aNode, aPlace) {
>      let wrapper = this.createWrapper(aNode, aPlace);
> +    // It's possible that this toolbar node is "mid-flight" and doesn't have
> +    // a parent, in which case we skip replacing it.

When is this possible? The comment doesn't elaborate.
Comment 103 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 09:38:43 PDT
Created attachment 729598 [details] [diff] [review]
Addressing comment #79
Comment 104 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 10:08:56 PDT
Comment on attachment 729291 [details] [diff] [review]
After-big-patch Follow-up #4 - Make going into customize mode more awesome

Review of attachment 729291 [details] [diff] [review]:
-----------------------------------------------------------------

Missing OSX/Linux here. As mentioned on IRC, I think we should probably try to have some parity for when this arrives on UX branch - I'd expect there to be a greater percentage of people on OSX using UX-branch than what we expect anywhere else.
Comment 105 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 10:10:37 PDT
Comment on attachment 729291 [details] [diff] [review]
After-big-patch Follow-up #4 - Make going into customize mode more awesome

Review of attachment 729291 [details] [diff] [review]:
-----------------------------------------------------------------

Er, lets make this a r- given that comment.
Comment 106 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 10:51:36 PDT
(In reply to Mike Conley (:mconley) from comment #71)
> @@ +120,5 @@
> > +    this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> > +  },
> > +
> > +  _defineBuiltInWidgets: function() {
> > +    //XXXunf Need a better place to define these.
> 
> Is this a better-enough place?

Feels awkward to me. At the very least, it'd be worth moving them to the top of the file, out of the code that implements everything.
Comment 107 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 11:14:15 PDT
(In reply to Mike Conley (:mconley) from comment #72)
> @@ +344,5 @@
> > +  //XXXmconley While CustomizableUI.jsm uses prefs to preserve state, we might
> > +  //           also want to (try) persisting with currentset as well to make it
> > +  //           less painful to switch to older builds.
> > +  persistCurrentSets: function()  {
> > +    //XXXjaws The toolbar bindings that are included in this changeset (/browser/base/content/toolbar.xml)
> 
> jaws is right - we should try to fix the binding to inherit from toolkit
> toolbar.

I'm not convinced its worth it (over just re-implementing a simple currentSet) - the toolkit binding has a lot of cruft we don't want.
Comment 108 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 11:18:08 PDT
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch

Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/customize.inc
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<vbox id="customization-container" flex="1" style="background: #DBEAF9;">

Lets continue to ignore this transgression for now.

::: browser/base/content/panelUI.inc
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!-- XXXjaws Need to remove the noautohide="true" -->

Should probably do this on UX-branch.

::: browser/base/content/panelUI.js
@@ +4,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> +                                  "resource:///modules/CustomizableUI.jsm");
> +
> +const PanelUI = {

Could do with some documentation describing what this object is generally for.

@@ +82,5 @@
> +      this.panel.removeEventListener(event, this);
> +    }
> +
> +    this.clickCapturer.removeEventListener("click",
> +                                           this._onCapturerClick.bind(this),

Danger, Will Robinson.

@@ +210,5 @@
> +   */
> +  _updatePanelButton: function() {
> +    if (this.panel.anchorNode) {
> +      this.panel.anchorNode.open = this.panel.state == "open" ||
> +                                   this.panel.state == "showing";

Just built Jamun branch, and this doesn't seem to be working for me. ie, the button doesn't become depressed.

::: browser/base/content/panelUI.xml
@@ +1,4 @@
> +<?xml version="1.0"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

This file should eventually be merged with the new toolbar.xml (since they're all related to the customizable UI, and fewer documents is less memory consumed).
Comment 109 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 12:15:35 PDT
Created attachment 729704 [details] [diff] [review]
Milestone 1.1 interdiff (r=jaws, Unfocused, mconley)

Here are all of the fixes for the Milestone 1 patch.
Comment 110 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 12:15:59 PDT
(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #70)
> Comment on attachment 728439 [details] [diff] [review]
> Patch (rollup to 77c33bac1b29)
> ::: browser/themes/linux/panelUIOverlay.css
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +%filter substitution
> > +
> > +%define menuPanelWidth 23em
> 
> Uber nit, the linux/panelUIOverlay.css and osx/panelUIOverlay.css have a
> blank line between the %define and the %filter, but in
> windows/panelUIOverlay.css the %define is immediately below the %filter. We
> should make all these files the same.
> 
> Also, they all have the same menuPanelWidth. Should that get moved to the
> shared file?
> 

Good question - added to my list of things to address in milestone 2.

> ::: browser/themes/shared/panelUIOverlay.inc.css
> @@ +77,5 @@
> > +}
> > +
> > +#PanelUI-subViews {
> > +  -moz-stack-sizing: ignore;
> > +  background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */
> 
> This and other places should use the unprefixed syntax. Note that the we
> can't just drop the -moz- to fix this.

Good call - I've deferred this, and added to my list of things to have fixed
during milestone 2.

> @@ +206,5 @@
> > +#customization-palette .toolbarbutton-text {
> > +  display: none;
> > +}
> > +
> > +/* XXXunf Hack! Copied from general toolbar styles in winstripe. */
> 
> This styling will be used for all platforms. Should the windows theme
> reference this? Or should they just be duplicated since they serve different
> purposes and they may intentionally diverge over time? I'm fine with the
> duplication.
> 

Let's leave this for now. OS-specific theming will clear this up.

> We also need the appmenu.png images for linux and osx.

Added to my list of things to get for milestone 2 (or later).


(In reply to Mike Conley (:mconley) from comment #71)

> @@ +120,5 @@
> > +    this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> > +  },
> > +
> > +  _defineBuiltInWidgets: function() {
> > +    //XXXunf Need a better place to define these.
> 
> Is this a better-enough place?

As per Blair's suggestion, I've (at least for now) put the widget definitions
in a lazy getter at the top of CustomizableUI.jsm.

> 
> @@ +253,5 @@
> > +  },
> > +
> > +  registerArea: function(aName, aProperties) {
> > +    if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> > +      throw Error("Invalid area name");
> 
> throw new Error
> 

This used to just return null. Throwing error risks busting the entire UI if
the exception isn't caught. I've opted, at least for now, to log the problem
with ERROR, and continue to return null.

> @@ +328,5 @@
> > +      let palette = aAreaNode.toolbox ? aAreaNode.toolbox.palette : null;
> > +      let limit = currentNode.previousSibling;
> > +      let node = container.lastChild;
> > +      while (node != limit) {
> > +        // XXXunf Depreciating the old "removable" attribute, is this right?
> 
> Not sure. The Social API button, for example, isn't supposed to be
> move-able. I'm not even sure how that is supposed to play in this new
> Australis customization world we're building here. Worth investigating.
> 

Added to my list of things to investigate during milestone 2

> @@ +355,5 @@
> > +    if (this.isSpecialWidget(aWidgetId))
> > +      return CustomizableUI.PROVIDER_SPECIAL;
> > +    if (gPalette.has(aWidgetId))
> > +      return CustomizableUI.PROVIDER_API;
> > +    //XXXunf Er, this isn't entirely true.. could also not exist.
> 
> We should handle that case, and log or throw as necessary.
> 

Blair explained this more thoroughly, and I've used his comment as the basis
for clearer documentation.

> @@ +552,5 @@
> > +    }
> > +    return null;
> > +  },
> > +
> > +  //XXXunf Need to free this from aMenu's tyranny.
> 
> Not sure what this comment means, though I do appreciate the use of the word
> tyranny. :) This should either be made more clear, or just be removed.
> 

This was replaced with clearer documentation - basically, highlighting the fact
that aMenu shouldn't be necessary.

> @@ +222,5 @@
> > +
> > +    this._stowedPalette = this.window.gNavToolbox.palette;
> > +    this.window.gNavToolbox.palette = this.visiblePalette;
> > +  },
> > +  //XXXunf Maybe this should use -moz-element instead of wrapping the node?
> 
> I'm not opposed to trying this - makes more sense to me to "imitate"/"mimic"
> the toolbaritems rather than wrapping them. Also means we don't have to
> slurp the items in and out of the invisible palette each time.
> 
> Might be worth investigating later.
> 

Added to my list of deferred decisions / things to investigate.

> @@ +259,5 @@
> > +        }
> > +
> > +        this._stowedPalette.appendChild(widgetNode);
> > +      } else if (provider = CustomizableUI.PROVIDER_API) {
> > +        //XXXunf Currently this doesn't destroy the (now unused) node. It would
> 
> Let's just leave this for now, and deal with it in a later iteration.
> 

Added to my list of deferred decisions / things to investigate.

> @@ +344,5 @@
> > +  //XXXmconley While CustomizableUI.jsm uses prefs to preserve state, we might
> > +  //           also want to (try) persisting with currentset as well to make it
> > +  //           less painful to switch to older builds.
> > +  persistCurrentSets: function()  {
> > +    //XXXjaws The toolbar bindings that are included in this changeset (/browser/base/content/toolbar.xml)
> 
> jaws is right - we should try to fix the binding to inherit from toolkit
> toolbar.

Instead of inheriting from toolkit's toolbar, we'll implement a simple version
with minimal currentset support.

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #108)
> Comment on attachment 728439 [details] [diff] [review]
> Patch (rollup to 77c33bac1b29)
> 
> Review of attachment 728439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/customize.inc
> @@ +1,5 @@
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +
> > +<vbox id="customization-container" flex="1" style="background: #DBEAF9;">
> 
> Lets continue to ignore this transgression for now.
> 

Added to my list of deferred things to fix.

> @@ +210,5 @@
> > +   */
> > +  _updatePanelButton: function() {
> > +    if (this.panel.anchorNode) {
> > +      this.panel.anchorNode.open = this.panel.state == "open" ||
> > +                                   this.panel.state == "showing";
> 
> Just built Jamun branch, and this doesn't seem to be working for me. ie, the
> button doesn't become depressed.
> 

Good call. this.panel.anchorNode has no notion of open / close. Fixed this.

> ::: browser/base/content/panelUI.xml
> @@ +1,4 @@
> > +<?xml version="1.0"?>
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> 
> This file should eventually be merged with the new toolbar.xml (since
> they're all related to the customizable UI, and fewer documents is less
> memory consumed).

Noted in my list of things to fix later.

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #94)
> Comment on attachment 729253 [details] [diff] [review]
> Addressing my review comments (apply on top of jaws' patch)
> 
> Review of attachment 729253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +110,5 @@
> > +                                            this._onDragStart.bind(this));
> > +    this.visiblePalette.removeEventListener("dragover",
> > +                                            this._onDragOver.bind(this));
> > +    this.visiblePalette.removeEventListener("drop",
> > +                                            this._onDragDrop.bind(this));
> 
> Fun fact: this.someFunction.bind(this) !== this.someFunction.bind(this)
> 
> Therefore, removing event listeners like this doesn't work, because you're
> trying to remove a different listener :( Need to store the value you
> originally pass into addEventListener() if you want to use bind().
> 
> Same issue later in this function too.

Gah, embarrassing to only find this out now. :/ I've fixed this, and will fix
it in the follow-up patches as well.
Comment 111 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 12:26:30 PDT
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #102)
> Comment on attachment 729290 [details] [diff] [review]
> After-big-patch Follow-up #3 - Make programmatically generated widgets behave
> 
> Review of attachment 729290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/CustomizeMode.jsm
> @@ +221,5 @@
> >  
> >    wrapToolbarItem: function(aNode, aPlace) {
> >      let wrapper = this.createWrapper(aNode, aPlace);
> > +    // It's possible that this toolbar node is "mid-flight" and doesn't have
> > +    // a parent, in which case we skip replacing it.
> 
> When is this possible? The comment doesn't elaborate.

I've added a clearer comment:

"It's possible that this toolbar node is "mid-flight" and doesn't have a parent, in which case we skip replacing it. This can happen if a toolbar item has been dragged into the palette. In that case, we tell CustomizableUI to remove the widget from its area before putting the widget in the palette - so the node will have no parent."
Comment 112 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 12:37:58 PDT
(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #97)
> Comment on attachment 729292 [details] [diff] [review]
> After-big-patch Follow-up #5 - More ways to exit customize mode
> 
> Review of attachment 729292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/CustomizeMode.jsm
> @@ +361,5 @@
> > +          this.exit();
> > +        }
> > +        break;
> > +      case "click":
> > +        if (aEvent.originalTarget = this.window.PanelUI.menuButton) {
> 
> This should also check aEvent.button == 0.

Done. Also, just noticed I used the assignment operator instead of the equivalence operator. Fixed that too.
Comment 113 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:02:01 PDT
Created attachment 729731 [details] [diff] [review]
M1 - Follow-up #1 - Make sure panel size is sync'd. r=mconley.
Comment 114 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:03:01 PDT
Created attachment 729732 [details] [diff] [review]
M1 - Follow-up #2 - Allow items to be dropped among palette items (r=Unfocused)
Comment 115 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:04:02 PDT
Created attachment 729733 [details] [diff] [review]
M1 - Follow-up #3 - Make programmatically generated widgets behave (r=Unfocused)
Comment 116 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:04:53 PDT
Created attachment 729735 [details] [diff] [review]
M1 - Follow-up #4 - Make going into customize mode more awesome
Comment 117 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:06:46 PDT
Comment on attachment 729735 [details] [diff] [review]
M1 - Follow-up #4 - Make going into customize mode more awesome

Copied the CSS from windows/browser.css over to osx and linux. Added a note to revisit OS specific theme-ing in a future milestone (falls under "Make pretty").
Comment 118 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:08:12 PDT
Created attachment 729736 [details] [diff] [review]
M1 - Follow-up #5 - More ways to exit customize mode (r=jaws)
Comment 119 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:10:13 PDT
Created attachment 729737 [details] [diff] [review]
M1 - Follow-up #6 - Add rudimentary drop indicator to toolbar
Comment 120 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:11:38 PDT
Created attachment 729739 [details] [diff] [review]
M1 - Follow-up #7 - Make drop targets glow blue instead of purple (r=jaws)
Comment 121 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:12:21 PDT
Created attachment 729740 [details] [diff] [review]
M1 - Follow-up #8 - Disable the menu button when customizing (r=mconley)
Comment 122 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:13:37 PDT
Created attachment 729742 [details] [diff] [review]
M1 - Follow-up #9 - Add a grab effect when drag starts
Comment 123 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 13:27:09 PDT
(In reply to Mike Conley (:mconley) from comment #110)
> (In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #70)
> > Comment on attachment 728439 [details] [diff] [review]
> > Patch (rollup to 77c33bac1b29)
> > ::: browser/themes/linux/panelUIOverlay.css
> > @@ +3,5 @@
> > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > > +
> > > +%filter substitution
> > > +
> > > +%define menuPanelWidth 23em
> > 
> > Uber nit, the linux/panelUIOverlay.css and osx/panelUIOverlay.css have a
> > blank line between the %define and the %filter, but in
> > windows/panelUIOverlay.css the %define is immediately below the %filter. We
> > should make all these files the same.
> > 
> > Also, they all have the same menuPanelWidth. Should that get moved to the
> > shared file?
> > 
> 
> Good question - added to my list of things to address in milestone 2.

I fixed the uber nit already in "addressed review comments" patch but didn't adjust the location of menuPanelWidth.

> > ::: browser/themes/shared/panelUIOverlay.inc.css
> > @@ +77,5 @@
> > > +}
> > > +
> > > +#PanelUI-subViews {
> > > +  -moz-stack-sizing: ignore;
> > > +  background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */
> > 
> > This and other places should use the unprefixed syntax. Note that the we
> > can't just drop the -moz- to fix this.
> 
> Good call - I've deferred this, and added to my list of things to have fixed
> during milestone 2.

Already fixed now.

> 
> > We also need the appmenu.png images for linux and osx.
> 
> Added to my list of things to get for milestone 2 (or later).

Already fixed.
Comment 124 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 13:33:57 PDT
Comment on attachment 729735 [details] [diff] [review]
M1 - Follow-up #4 - Make going into customize mode more awesome

Review of attachment 729735 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/jar.mn
@@ +24,5 @@
>          skin/classic/browser/appmenu-icons.png
>          skin/classic/browser/appmenu-dropmarker.png
>  *       skin/classic/browser/browser.css
>          skin/classic/browser/click-to-play-warning-stripes.png
> +        skin/classic/browser/customization/customization-mode-background.jpg (../shared/customization/customization-mode-background.jpg)

As discussed on IRC, we should just have separate copies of this graphic for the three platforms. This should be more consistent with the rest of the file.
Comment 125 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 13:43:54 PDT
Created attachment 729774 [details] [diff] [review]
M1 - Follow-up #4 - Make going into customize mode more awesome (r=jaws)

Moved the customization grid background texture to the individual theme folders and updated the jar.mn files.
Comment 126 Mike Conley (:mconley) - (Needinfo me!) 2013-03-26 16:49:55 PDT
Created attachment 729874 [details] [diff] [review]
M1 - Follow-up #10 - Make buttons look right on OSX and Linux, and make grid texture work on Linux

I had to fix these to make the panel look more acceptable on OSX and Linux.
Comment 127 Jared Wein [:jaws] (please needinfo? me) 2013-03-26 20:18:00 PDT
Comment on attachment 729874 [details] [diff] [review]
M1 - Follow-up #10 - Make buttons look right on OSX and Linux, and make grid texture work on Linux

Review of attachment 729874 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/linux/panelUIOverlay.css
@@ +33,5 @@
> +  padding-left: 12px;
> +  padding-right: 12px;
> +}
> +
> +#PanelUI-zoomIn-btn {

zoomIn and zoomOut can get disabled when the user has reached the zoom limit, right?
Comment 128 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-26 22:43:44 PDT
(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #127)
> zoomIn and zoomOut can get disabled when the user has reached the zoom
> limit, right?

No - neither these controls or the original ones become disabled when you reach the max in either direction (in fact, doesn't look like the menuitems in the main menu do either). I think they ideally should get disabled, but that's an existing bug - not something introduced here.
Comment 130 Mike Conley (:mconley) - (Needinfo me!) 2013-04-05 07:11:10 PDT
Missed one - this is follow-up #10 on jamun:

https://hg.mozilla.org/projects/jamun/rev/43a95bc2eddb
Comment 131 Jared Wein [:jaws] (please needinfo? me) 2013-05-09 11:38:09 PDT
*** Bug 844281 has been marked as a duplicate of this bug. ***
Comment 132 mdew 2013-06-01 01:18:50 PDT
Is the menu button movable, rather than being forced in a right alligned.. Really for ex-Australis users since I prefer having my menu options on the left, and no doubt other users would want something similar.. Is there a Bugzilla ID for this?
Comment 133 Jared Wein [:jaws] (please needinfo? me) 2013-06-01 07:03:23 PDT
(In reply to mdew from comment #132)
> Is the menu button movable, rather than being forced in a right alligned..
> Really for ex-Australis users since I prefer having my menu options on the
> left, and no doubt other users would want something similar.. Is there a
> Bugzilla ID for this?

I don't know of a bug being filed for it, but there are no plans for this functionality to be included in Firefox by default.
Comment 134 mdew 2013-06-01 16:33:51 PDT
(In reply to Jared Wein [:jaws] from comment #133)
> 
> I don't know of a bug being filed for it, but there are no plans for this
> functionality to be included in Firefox by default.

Given all incarnations of Firefox have been left-aligned up till now, I dont mind having right-aligned being the default.I just prefer having my menus (menu option/bookmark button) on the left. Such a radical change, non configurable change, may annoy existing firefox users.
Comment 135 Justin Dolske [:Dolske] 2013-06-02 19:41:46 PDT
I think we definitely want it non-removable by default (like certain other important navbar items), but I'm sure sure there's any reason to disallow moving it. (OTOH, it doesn't seem like a big deal, since the Firefox button wasn't movable.)

Related: where's it get shown in RTL mode? :)
Comment 136 Mike Conley (:mconley) - (Needinfo me!) 2013-06-02 20:16:42 PDT
(In reply to Justin Dolske [:Dolske] from comment #135)
> Related: where's it get shown in RTL mode? :)

It's correctly on the left-hand side. The subview transitions are wrong though (bug 872548).
Comment 137 Adam Kowalczyk 2013-06-03 11:26:58 PDT
> OTOH, it doesn't seem like a big deal, since the Firefox button wasn't movable.

The Firefox button was on the tab bar, though. One of the reasons why some people may want to move the menu button is that they use extensions to hide the nav bar for pinned tabs - a feature which was actually planned to be included by default, at one point.
Comment 138 Peter Henkel [:Terepin] 2013-06-03 22:05:52 PDT
I'm doing that myself. It will be inconvenient, yes, but I can live with it.
Comment 139 Justin Dolske [:Dolske] 2013-06-04 15:48:10 PDT
I was referring to Windows, where the bright orange Firefox button has always been at the top-left and non-customizable/movable.
Comment 140 Adam Kowalczyk 2013-06-04 18:31:19 PDT
What I meant was that the Firefox button, though not customizable, was guaranteed to be always visible. This isn't the case with the new menu button because the navigation bar can be hidden and in fact some users do hide it regularly for pinned tabs. These users might want to move the menu button to the tab bar.
Comment 141 Jared Wein [:jaws] (please needinfo? me) 2013-06-04 18:33:29 PDT
(In reply to Adam Kowalczyk from comment #140)
> What I meant was that the Firefox button, though not customizable, was
> guaranteed to be always visible. This isn't the case with the new menu
> button because the navigation bar can be hidden and in fact some users do
> hide it regularly for pinned tabs. These users might want to move the menu
> button to the tab bar.

The navigation bar cannot be hidden anymore via the menus or context menu (bug 870545) and we no longer hide the navigation bar when viewing the add-ons manager (bug 752434).
Comment 142 Frederik Braun [:freddyb] [unavailable June 4th to August 29th] 2013-09-09 01:16:29 PDT
Security Review finished, see bug 853453.

Note You need to log in before you can comment on or make changes to this bug.