Closed Bug 770135 Opened 12 years ago Closed 11 years ago

New PanelUI and toolbar customization (Milestone 1)

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Unfocused, Assigned: jaws)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(12 files, 58 obsolete files)

166.14 KB, patch
Unfocused
: review+
jaws
: review+
mconley
: review+
Details | Diff | Splinter Review
112.85 KB, patch
mconley
: review+
Details | Diff | Splinter Review
3.41 KB, patch
Details | Diff | Splinter Review
1.37 KB, patch
Details | Diff | Splinter Review
6.90 KB, patch
Details | Diff | Splinter Review
2.98 KB, patch
Details | Diff | Splinter Review
10.29 KB, patch
jaws
: review+
Details | Diff | Splinter Review
904 bytes, patch
Details | Diff | Splinter Review
1.98 KB, patch
Details | Diff | Splinter Review
6.50 KB, patch
jaws
: review+
Details | Diff | Splinter Review
356.34 KB, patch
Details | Diff | Splinter Review
6.64 KB, patch
jaws
: review+
Details | Diff | Splinter Review
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.
Attached patch Prototype v1 (obsolete) — Splinter Review
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
Attachment #638318 - Flags: feedback?(shorlander)
Attachment #638318 - Flags: feedback?(jaws)
Attachment #638318 - Flags: feedback?(dtownsend+bugmail)
Attachment #638318 - Flags: feedback?(dolske)
Attachment #638318 - Flags: feedback?(fryn)
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.
Attachment #638318 - Flags: feedback?(shorlander) → feedback+
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/
Attachment #638318 - Flags: feedback?(jaws)
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. :)
Attachment #638318 - Flags: feedback?(fryn) → feedback+
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.
Attachment #638318 - Flags: feedback?(dtownsend+bugmail) → feedback+
Comment on attachment 638318 [details] [diff] [review]
Prototype v1

(Already gave Blair feedback on this -- no issues -- forgot I was flagged)
Attachment #638318 - Flags: feedback?(dolske) → feedback+
Attached patch WiP v1 (obsolete) — Splinter Review
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.
Attachment #638318 - Attachment is obsolete: true
Depends on: 793093
Summary: Prototype/explore new Panel UI, and interactions with addons → New PanelUI and toolbar customization backend
Attached patch WiP v2 - backend (obsolete) — Splinter Review
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.
Attachment #658462 - Attachment is obsolete: true
Attached patch WiP v2 - customization mode (obsolete) — Splinter Review
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.
Attached file WiP v2 - demo (obsolete) —
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.
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
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).
Component: General → Toolbars
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.
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.
(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 :)
Where can I download the try build ? It's not available.
(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
Attached patch WiP v3 - backend (obsolete) — Splinter Review
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.
Attachment #672732 - Attachment is obsolete: true
Attached patch WiP v3 - customization mode (obsolete) — Splinter Review
Just fixing bitrot.
Attachment #672734 - Attachment is obsolete: true
Attached file WiP v3 - demo (obsolete) —
More bitrot fixes (ie, you *need* to use this version of the demo addon).

Again, everything still Windows focused.
Attachment #672737 - Attachment is obsolete: true
Attached patch WiP v3.001 - backend (obsolete) — Splinter Review
Er, helps if I attach the right patch.
Attachment #680574 - Attachment is obsolete: true
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
Just one little detail on the current implementation : it would be more logical to have plus-minus zoom button inverted since we read LTR.
(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.
(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.
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
(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.
Attached patch WIP v3.002 - backend (rebased) (obsolete) — Splinter Review
Assignee: bmcbride → jaws
Attachment #680577 - Attachment is obsolete: true
Attached patch WIP v3.002 - frontend (rebased) (obsolete) — Splinter Review
Attachment #680575 - Attachment is obsolete: true
Attached patch WiP v4 - backend (needs rebased) (obsolete) — Splinter Review
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.
Attachment #705508 - Attachment is obsolete: true
Attachment #705509 - Attachment is obsolete: true
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
Attached patch WIP v4.01 - backend (rebased) (obsolete) — Splinter Review
Attachment #705686 - Attachment is obsolete: true
Attached patch WIP v4.01 - frontend (rebased) (obsolete) — Splinter Review
Attachment #705687 - Attachment is obsolete: true
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.
Depends on: 840697
Summary: New PanelUI and toolbar customization backend → New PanelUI and toolbar customization
Attached patch WIP Patch v4.02 (obsolete) — Splinter Review
Rebased and fixed some small issues. Uploading this WIP patch here so mconley can split out the menu panel from the customization code.
Attachment #711495 - Attachment is obsolete: true
Attachment #711496 - Attachment is obsolete: true
Attached patch Customization WIP Patch v4.03 (obsolete) — Splinter Review
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).
Attachment #716660 - Attachment is obsolete: true
Depends on: 844281
Comment on attachment 717280 [details] [diff] [review]
AppMenu WIP Patch v4.03

Moving the menu work out to bug 844281.
Attachment #717280 - Attachment is obsolete: true
Attached patch Customization WIP Patch v4.04 (obsolete) — Splinter Review
Updated for latest UX (1dda6c9bc6a6 - Feb 28, 2013)
Attachment #717285 - Attachment is obsolete: true
Blocks: 347446
Flags: sec-review?(fbraun)
(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?
(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.
Attached patch 9a2477c31b28.patch (part 1) (obsolete) — Splinter Review
Attachment #680576 - Attachment is obsolete: true
Attachment #719573 - Attachment is obsolete: true
Attachment #728398 - Flags: review?(bmcbride)
Attached patch de041c8ef0c7.patch (part 2) (obsolete) — Splinter Review
Attachment #728399 - Flags: review?(bmcbride)
Attachment #728398 - Attachment description: 9a2477c31b28.patch → 9a2477c31b28.patch (part 1)
Attached patch 89287cdca338.patch (part 3) (obsolete) — Splinter Review
Attachment #728400 - Flags: review?(jAwS)
Attached patch e4ac9e227efe.patch (part 4) (obsolete) — Splinter Review
Attachment #728401 - Flags: review?(mconley)
Attached patch 8bc3c78c12fe.patch (part 5) (obsolete) — Splinter Review
Attachment #728403 - Flags: review?(mconley)
Attached patch 6235f69f1647.patch (part 6) (obsolete) — Splinter Review
Attachment #728404 - Flags: review?(jAwS)
Attachment #728404 - Attachment description: 6235f69f1647.patch → 6235f69f1647.patch (part 6)
Attached patch 53cf1ebc6e85.patch (part 7) (obsolete) — Splinter Review
Attachment #728405 - Flags: review?(jAwS)
Attachment #728405 - Attachment description: 53cf1ebc6e85.patch → 53cf1ebc6e85.patch (part 7)
Attached patch d678b862dbfd.patch (part 8) (obsolete) — Splinter Review
Attachment #728407 - Flags: review?(bmcbride)
Attached patch deec4e29ba17.patch (part 9) (obsolete) — Splinter Review
Attachment #728409 - Flags: review?(mconley)
Attached patch fd97c40999fc.patch (part 10) (obsolete) — Splinter Review
Attachment #728411 - Flags: review?(jAwS)
Attached patch d410a4e584e3.patch (part 11) (obsolete) — Splinter Review
Attachment #728412 - Flags: review?(jAwS)
Attached patch f0e94249a97a.patch (part 12) (obsolete) — Splinter Review
Attachment #728414 - Flags: review?(bmcbride)
Attached patch 0c5206f0ed6b.patch (part 13) (obsolete) — Splinter Review
Attachment #728415 - Flags: review?(jAwS)
Attachment #728414 - Attachment description: f0e94249a97a.patch (part 11) → f0e94249a97a.patch (part 12)
Attached patch 952c9ccc247d.patch (part 14) (obsolete) — Splinter Review
Attachment #728418 - Flags: review?(mconley)
Attached patch 7534fd7a2bac.patch (part 15) (obsolete) — Splinter Review
Attachment #728419 - Flags: review?(jAwS)
Attached patch a18848287a4f.patch (part 17) (obsolete) — Splinter Review
Attachment #728425 - Flags: review?(mconley)
Attachment #728420 - Flags: review?(jAwS)
Attached patch 48f24a30c6ac.patch (part 18) (obsolete) — Splinter Review
Attachment #728426 - Flags: review?(mconley)
Attached patch e907595d9ce3.patch (part 19) (obsolete) — Splinter Review
Attachment #728427 - Flags: review?(bmcbride)
Attached patch c1859c500536.patch (part 20) (obsolete) — Splinter Review
Attachment #728428 - Flags: review?(jAwS)
Attached patch 29650070976e.patch (part 21) (obsolete) — Splinter Review
Attachment #728430 - Flags: review?(bmcbride)
Attached patch 176c822ef6a3.patch (part 22) (obsolete) — Splinter Review
Attachment #728432 - Flags: review?(jAwS)
Attached patch cbe88d315a03.patch (part 23) (obsolete) — Splinter Review
Attachment #728433 - Flags: review?(jAwS)
Attached patch 9e5a55078be5.patch (part 24) (obsolete) — Splinter Review
Attachment #728435 - Flags: review?(mconley)
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/*
Attachment #728398 - Attachment is obsolete: true
Attachment #728399 - Attachment is obsolete: true
Attachment #728400 - Attachment is obsolete: true
Attachment #728401 - Attachment is obsolete: true
Attachment #728403 - Attachment is obsolete: true
Attachment #728404 - Attachment is obsolete: true
Attachment #728405 - Attachment is obsolete: true
Attachment #728407 - Attachment is obsolete: true
Attachment #728409 - Attachment is obsolete: true
Attachment #728411 - Attachment is obsolete: true
Attachment #728412 - Attachment is obsolete: true
Attachment #728414 - Attachment is obsolete: true
Attachment #728415 - Attachment is obsolete: true
Attachment #728418 - Attachment is obsolete: true
Attachment #728419 - Attachment is obsolete: true
Attachment #728420 - Attachment is obsolete: true
Attachment #728425 - Attachment is obsolete: true
Attachment #728426 - Attachment is obsolete: true
Attachment #728427 - Attachment is obsolete: true
Attachment #728428 - Attachment is obsolete: true
Attachment #728430 - Attachment is obsolete: true
Attachment #728432 - Attachment is obsolete: true
Attachment #728433 - Attachment is obsolete: true
Attachment #728435 - Attachment is obsolete: true
Attachment #728398 - Flags: review?(bmcbride)
Attachment #728399 - Flags: review?(bmcbride)
Attachment #728400 - Flags: review?(jAwS)
Attachment #728401 - Flags: review?(mconley)
Attachment #728403 - Flags: review?(mconley)
Attachment #728404 - Flags: review?(jAwS)
Attachment #728405 - Flags: review?(jAwS)
Attachment #728407 - Flags: review?(bmcbride)
Attachment #728409 - Flags: review?(mconley)
Attachment #728411 - Flags: review?(jAwS)
Attachment #728412 - Flags: review?(jAwS)
Attachment #728414 - Flags: review?(bmcbride)
Attachment #728415 - Flags: review?(jAwS)
Attachment #728418 - Flags: review?(mconley)
Attachment #728419 - Flags: review?(jAwS)
Attachment #728420 - Flags: review?(jAwS)
Attachment #728425 - Flags: review?(mconley)
Attachment #728426 - Flags: review?(mconley)
Attachment #728427 - Flags: review?(bmcbride)
Attachment #728428 - Flags: review?(jAwS)
Attachment #728430 - Flags: review?(bmcbride)
Attachment #728432 - Flags: review?(jAwS)
Attachment #728433 - Flags: review?(jAwS)
Attachment #728435 - Flags: review?(mconley)
Attachment #728439 - Flags: review?(mconley)
Attachment #728439 - Flags: review?(jAwS)
Attachment #728439 - Flags: review?(bmcbride)
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
(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.
Depends on: 854450
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.
Attachment #728439 - Flags: review?(jAwS) → review+
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 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.
Attachment #728439 - Flags: review?(mconley) → review+
Attached patch Addressing my review comments (obsolete) — Splinter Review
Attachment #729145 - Flags: review?(mconley)
Attachment #729145 - Attachment is obsolete: true
Attachment #729145 - Flags: review?(mconley)
Attached patch Addressing my review comments (obsolete) — Splinter Review
Attachment #729180 - Flags: review?(mconley)
Attachment #729248 - Flags: review?(jAwS)
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
Flags: needinfo?(bmcbride)
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.
Attachment #729180 - Flags: review?(mconley) → review+
Re-based on top of jaws' fixes.
Attachment #729248 - Attachment is obsolete: true
Attachment #729248 - Flags: review?(jAwS)
Attachment #729253 - Flags: review?(jAwS)
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.
Attachment #729248 - Attachment is obsolete: false
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.
Attachment #729253 - Flags: review?(jAwS) → review+
Attachment #729248 - Attachment is obsolete: true
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+.
Before, we only appended to the palette. This allows us to drop amongst the palette items.
Programmatically generated widgets wouldn't behave properly after dragging / dropping them once. After dragging or dropping them, they'd act as their unwrapped selves.
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.
This patch allows us to exit customize mode either by pressing Esc, or by clicking on the menu panel button.
This adds a very basic (and temporary) drop indicator to the nav-bar when customizing.
Our drop targets had a purplish hue about them. This makes them more blue.
The menu button should be in the disabled state while in customizing mode.
When "picking up" a customizable item, we scale it up and increase its opacity.
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.
Attachment #729293 - Flags: review?(mconley)
Attachment #729287 - Flags: review?(mconley)
Attachment #729295 - Flags: review?(mconley)
Attachment #729289 - Flags: review?(bmcbride)
Attachment #729290 - Flags: review?(bmcbride)
Attachment #729291 - Flags: review?(bmcbride)
Attachment #729292 - Flags: review?(jAwS)
Attachment #729294 - Flags: review?(jAwS)
Attachment #729296 - Flags: review?(jAwS)
(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?
(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.
Attachment #729287 - Flags: review?(mconley) → review+
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
Attachment #729293 - Flags: review?(mconley) → review+
Attachment #729295 - Flags: review?(mconley) → review+
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.
Flags: needinfo?(bmcbride)
(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.
(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 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.
Attachment #729292 - Flags: review?(jAwS) → review+
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.
Attachment #729293 - Flags: review-
Attachment #729294 - Flags: review?(jAwS) → review+
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...
Attachment #729289 - Flags: review?(bmcbride) → review+
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`.
Attachment #729296 - Flags: review?(jAwS) → review-
(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 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.
Attachment #729290 - Flags: review?(bmcbride) → review+
Attached patch Addressing comment #79 (obsolete) — Splinter Review
Attachment #729598 - Flags: review+
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.
Attachment #729291 - Flags: review?(bmcbride) → review+
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.
Attachment #729291 - Flags: review+ → review-
(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.
(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 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).
Attachment #728439 - Flags: review?(bmcbride) → review+
Attachment #728439 - Attachment description: Patch (rollup to 77c33bac1b29) → Milestone 1 patch
Here are all of the fixes for the Milestone 1 patch.
Attachment #729180 - Attachment is obsolete: true
Attachment #729253 - Attachment is obsolete: true
Attachment #729598 - Attachment is obsolete: true
Attachment #729704 - Flags: review+
(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.
(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."
(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 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").
Attachment #729735 - Flags: review?(jAwS)
(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 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.
Attachment #729735 - Flags: review?(jAwS) → review+
Attachment #729737 - Flags: review?(jAwS) → review+
Attachment #729742 - Flags: review?(jAwS) → review+
Moved the customization grid background texture to the individual theme folders and updated the jar.mn files.
Attachment #729735 - Attachment is obsolete: true
I had to fix these to make the panel look more acceptable on OSX and Linux.
Attachment #729874 - Flags: review?(jAwS)
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?
Attachment #729874 - Flags: review?(jAwS) → review+
(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.
Depends on: 855452
Depends on: 855483
Depends on: 855799
No longer depends on: 855799
Depends on: 858056
Depends on: 858067
Depends on: 858068
Depends on: 858132
Depends on: 858196
Depends on: 858219
No longer blocks: 858584
Depends on: 858584
Depends on: 858597
Depends on: 857542
Depends on: 858660
Depends on: 858662
Depends on: 855305
Depends on: 860487
Depends on: 860535
Depends on: 860562
Depends on: 860642
Depends on: 860646
Depends on: 860648
Depends on: 860814
Blocks: 860814
No longer depends on: 860814
Depends on: 861087
Depends on: 861088
Depends on: 861092
Depends on: 861401
Depends on: 861702
Depends on: 861703
Depends on: 863314
Blocks: 184147
Depends on: 863753
Depends on: 863780
Depends on: 866590
Depends on: 866978
Depends on: 867682
Depends on: 868068
Depends on: 868410
Depends on: 868433
Depends on: 868464
Depends on: 868612
Depends on: 868636
Depends on: 868640
Depends on: 868993
Depends on: 869101
Depends on: 869069
Depends on: 869349
Depends on: 869377
Depends on: 869581
Depends on: 870309
Depends on: 870461
Depends on: 870897
Depends on: 870901
Depends on: 870903
Depends on: 870985
Depends on: 870989
Depends on: 871151
Depends on: 871154
Depends on: 871158
Depends on: 871170
Depends on: 871203
Depends on: 871204
Depends on: 871296
Depends on: 872160
Depends on: 872403
Blocks: 872446
No longer blocks: 872446
Depends on: 872446
Depends on: 752434
Depends on: 872463
No longer depends on: 870605
No longer depends on: 872446
No longer depends on: 868433
No longer depends on: 870309
No longer depends on: 868612
No longer depends on: 871203
No longer depends on: 863780
No longer depends on: 870903
No longer depends on: 870593
No longer depends on: 868464
No longer depends on: 860648
No longer depends on: 868068
No longer depends on: 872463
No longer depends on: 870989
No longer depends on: 871170
No longer depends on: 871296
No longer depends on: 871158
No longer depends on: 871204
No longer depends on: 855683
No longer depends on: 868640
No longer depends on: 858056
No longer depends on: 861703
No longer depends on: 870897
No longer depends on: 871154
No longer depends on: 853453
No longer depends on: 870471
No longer depends on: 870901
No longer depends on: 870461
No longer depends on: 869377
No longer depends on: 860642
No longer depends on: 866978
No longer depends on: 868636
No longer depends on: 872403
Whiteboard: [fixed-in-ux]
No longer blocks: australis
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?
(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.
(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.
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? :)
(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).
> 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.
I'm doing that myself. It will be inconvenient, yes, but I can live with it.
I was referring to Windows, where the bright orange Firefox button has always been at the top-left and non-customizable/movable.
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.
(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).
Depends on: 881041
Blocks: 212990
Summary: New PanelUI and toolbar customization → New PanelUI and toolbar customization (Milestone 1)
Security Review finished, see bug 853453.
Flags: sec-review?(fbraun) → sec-review+
Restrict Comments: true
Depends on: 961823
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: