Closed Bug 865916 Opened 11 years ago Closed 11 years ago

Create a Character Encoding widget and subview

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 28

People

(Reporter: zfang, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M8][Australis:P1])

Attachments

(3 files, 11 obsolete files)

Encoding widget, button screenshot OSX
(deleted), image/png
Details
Encoding widget, subview screenshot OSX
(deleted), image/png
Details
Patch v10: create a Character Encoding widget and subview
(deleted), patch
dao
: review+
Details | Diff | Splinter Review 
Character Encoding will not be in the menu panel by default, but user can add it to the panel via the customization page. The goal here to figure out the best UX approach with minimum redesign of the current model, and make sure the UI is consistent with the rest of Australis customization UI (one level subview, no scrolling in panel, etc)

- Assumptions:
1. Not a lot of users in North America use this feature but people in other countries use it quite often. Therefore we don't ship it by default but provide the option to add it back.
2. People who use this feature switch between two or several encoding methods, not choosing a different one every time. So we don't have to show the entire encoding list at all time.

- Design Proposal:
http://people.mozilla.com/~zfang/Customization/Character%20Encoding.pdf
See "Design 1"
1. In Character Encoding submenu, show "customize list", "Auto-Detect" and a list of encoding methods (based on popularity, frequency or location)
2. User can check/uncheck auto-detect or switch between encoding methods in the list.
3. User can open "customize list" panel and add a encoding in the list if it's not in submenu.
Assignee: nobody → bmcbride
Blocks: 855287
No longer blocks: 855290
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
As mentioned in bug 865914 and on IRC, this is a hard blocker - we cannot ship without this, as doing so would break the web for some people.
Summary: Character Encoding when moved into Australis menu panel → Create a Character Encoding widget and subview
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #2)
> As mentioned in bug 865914 and on IRC, this is a hard blocker - we cannot
> ship without this, as doing so would break the web for some people.

Is that true? We are still going to continue to provide the View -> Character Encoding menu accessible via the Alt key on Windows.
Bug 575182 was considered a blocker! :) The hidden Alt-only menu isn't really a suitable solution.
If the menu bar which is shown with Alt key is enough for users, why we hide it and made Firefox button? :-p
Note that bug 863753 removed the Firefox button and the usage of browser.menu.showCharacterEncoding. If this new widget doesn't make use of that localized pref then it should be removed from firefox.js and browser.properties.

Perhaps we would use that pref to decide whether to include the character encoding menu in the default set of widgets upon migration to Australis and in a new profile.
No longer blocks: 855287
Whiteboard: [Australis:M6]
M6 is scheduled to finish tomorrow. Are there any updates on this widget?
Flags: needinfo?(bmcbride)
*sigh* Making progress, but its been slow. Doubt I'll make the M6 deadline.
Flags: needinfo?(bmcbride)
(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #8)
> *sigh* Making progress, but its been slow. Doubt I'll make the M6 deadline.

Ok, sounds like this is slipping to M7.
Whiteboard: [Australis:M6] → [Australis:M7]
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Attached patch WiP v1 (obsolete) (deleted) — — Splinter Review 
Here's a WiP patch, without much recent progress due to lack of time. Its needs the following done to work as a first pass (equivalent to the mockups) that can land:
* Update checkmark for currently selected charset/detector
* Making clicking things work

Then, as a second step, we can add the extra menus (categorized full list of charsets). These aren't in the mockups, but AFAICT they're necessary for basic functionality of it all.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P1]
Assignee: bmcbride → mdeboer
Pawning this off to Mike, who hopefully has a greater chance of finishing it this century.

Brain dump (typos included):

The guts of this is pretty much all there - it's a JS rewrite of the original C++ implementation (from 14+ years ago) which acts as a RDF datasource for XUL templates (yes, really). See the almost 2000 lines of toolkit/components/intl/nsCharsetMenu.cpp for the gory details. But hopefully you won't need to refer to that much, if at all, since I spent quiet a bit of time making sure the new code duplicated how the old code worked (while simplifying as much as possible). It should be just a matter of hooking it up to UI now. browser/base/content/browser-charsetmenu.inc has the XUL template for the existing menus, though that's probably not very useful.

Although this JS implementation could (and should) be used as a replacement everywhere, that should be left as a followup after Australis (along with refactoring into a reusable JSM).

Existing tests: HAHAHAHAHA, no. In the 14+ years there has been UI for this, AFAIK there has never been a single test for it.

I've been following Design 2 in the mockup (see comment 0), as that includes the autodetect items. However, the mockup doesn't include the categories full list of character encodings, which I believe we do need (see comment 11). The code to get each of those lists is already implemented (call getCharsetList("more1"), getCharsetList("more2"), etc), it just needs UI. To do that, I was thinking of having a "More" button at the subview (like the history subview has), that would show the categories (West European, East European, etc), clicking one of those would show their contents. See toolkit/locales/en-US/chrome/global/charsetOverlay.dtd for the strings for those categories. But we need a design for how to transition to showing them - eg, instantly hiding the existing subview contents, animating an expanding list, animating a sub-subview, etc.

The patch here also contains a modification to how CustomizableUI delegates events to widgets, to allow widgets to implement their functionality using methods on the object passed to createWidget()/createBuiltinWidget().

For landing, I think we should do it in two phases:
1) Implement the Design 2 mockup
2) Add the extra categorized lists

For that first phase, the following would need completed:
* Update checkmark for currently selected charset/detector
* Making clicking things work
* Listen to pref changes, and update the nodes as appropriate (probably not needed if you end up just re-generating everything every time onViewShowing is called).
(In reply to Blair McBride [:Unfocused] from comment #12)

> The guts of this is pretty much all there - it's a JS rewrite of the
> original C++ implementation (from 14+ years ago) which acts as a RDF
> datasource for XUL templates (yes, really).

Erhm. Did you forget to |hg add| this rewrite? Or are you talking about the addition in CustomizableWidgets.jsm?


> Existing tests: HAHAHAHAHA, no. In the 14+ years there has been UI for this,
> AFAIK there has never been a single test for it.

I think we'll want some; it hasn't been a problem to date because we've barely touched the existing code. But now we very much are touching it, and it would be good to know we're not breaking something (especially because this I don't think this feature is used by more than a handful of Nightly users/developers, so we're more likely to miss issues until release).

Dunno if it's practical, but would be useful to have a test that exercises the existing mozilla-central code (or existing menubar menu, since that's staying around?), but is also easily adaptable to test this widget. That would help with understanding the existing behavior, and gaining confidence that we're not breaking it.
Fang, I'll attach a few screenshots too, but the main question is: what do you think of the current implementation, considering the following points?

* There are no checkmarks for selected charsets. These would have to be designed, if required.
* There is no icon yet for Linux
* The 'More Encodings' subview is not implemented (yet); like Unfocused mentioned in comment 12, this would require some more thought.
Attachment #765635 - Attachment is obsolete: true
Attachment #771427 - Flags: ui-review?(zfang)
Attached image Encoding widget, button screenshot OSX (deleted) — 

    
    

    
  


  

    
    

    
  
Comment on attachment 771427 [details] [diff] [review]
Patch v1: create a Character Encoding widget and subview

Blair: flagging you for feedback, since you've done most of the work ;) Does it look OK to you?

Jared: Apart from the points I mentioned for fang, the patch is ready for review. I will add a unit test in a follow-up patch, which also might be best to include in part deux (which'll complete this implementation with the 'More Encodings' sub-sub-view.

        Attachment #771427 -
        Flags: review?(jaws)

        Attachment #771427 -
        Flags: feedback?(bmcbride)

    
    

    
  
Comment on attachment 771427 [details] [diff] [review]
Patch v1: create a Character Encoding widget and subview

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

You should get ui-review before requesting review on this.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +43,5 @@
>  
> +    <panelview id="PanelUI-characterencoding" flex="1">
> +      <label value="&charsetMenu.label;"/>
> +      <toolbarbutton label="&charsetCustomize.label;"
> +                     oncommand="PanelUI.hide();window.openDialog('chrome://global/content/customizeCharset.xul', 'PrefWindow', 'chrome,modal=yes,resizable=yes', 'browser');"/>

This code is too long to put in the oncommand attribute. Please refactor this out.

::: browser/themes/osx/browser.css
@@ +682,5 @@
> +    -moz-image-region: rect(36px, 648px, 72px, 612px);
> +  }
> +
> +  #characterencoding-panelmenu[customizableui-areatype="toolbar"][open] {
> +    -moz-image-region: rect(72px, 648px, 108px, 612px);

Do you also need to set HiDPI regions? Why is there no related change to themes/windows/browser.css?

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +265,5 @@
>    background-color: Highlight;
>    background-image: linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
>    background-repeat: repeat-x;
>    color: HighlightText;
> +  text-shadow: none;

Why is this necessary? Where is a text-shadow value being inherited from?

        Attachment #771427 -
        Flags: review?(jaws)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> Created attachment 771427 [details] [diff] [review]
> Patch v1: create a Character Encoding widget and subview
> 
> Fang, I'll attach a few screenshots too, but the main question is: what do
> you think of the current implementation, considering the following points?
> 
> * There are no checkmarks for selected charsets. These would have to be
> designed, if required.

Yes. I think the current checked state is a bit confusing since it looks similar to the hover state. Is a checkmark icon enough or there's anything else you need?

> * There is no icon yet for Linux

I'm assuming this is the icon for character encoding. and yes we need one for Linux.

> * The 'More Encodings' subview is not implemented (yet); like Unfocused
> mentioned in comment 12, this would require some more thought.

Correct me if I'm wrong but isn't the list of encoding in "customize list" the same with "more encoding" but just not categorized? If so then we don't have to show them both. Users can go to "customize list" and add the encoding they want. It does add an extra click but users only need to do it once and then can use it in the submenu.

    
    

    
  
Zhenshuo, thanks for checking this out!

(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #19)
> 
> Yes. I think the current checked state is a bit confusing since it looks
> similar to the hover state. Is a checkmark icon enough or there's anything
> else you need?
> 

I won't need anything else than a checkmark icon indeed.

> 
> I'm assuming this is the icon for character encoding. and yes we need one
> for Linux.
> 

You assumed correctly :)

> 
> Correct me if I'm wrong but isn't the list of encoding in "customize list"
> the same with "more encoding" but just not categorized? If so then we don't
> have to show them both. Users can go to "customize list" and add the
> encoding they want. It does add an extra click but users only need to do it
> once and then can use it in the submenu.

Sounds ok to me, but I do not know enough about this menu to be certain. Regardless, it is fine to leave the subview alone for now and tackle that problem in a follow-up.

    
    

    
  
Comment on attachment 771427 [details] [diff] [review]
Patch v1: create a Character Encoding widget and subview

Cancelling ui-r.

At this point, this bug is blocked on UX (Zhenshuo) to provide a checkmark icon and a Character Encoding toolbarbutton icon for Linux.

        Attachment #771427 -
        Flags: ui-review?(zfang)

    
    

    
  
Not sure it's necessary to block on graphic assets - those can land afterwards.

    
    

    
  
Good news -- we already have checkmarks!

  http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/menu/
  chrome://global/skin/menu/menu-check.png
  chrome://global/skin/menu/menu-check@2x.png

    
    

    
  
(I guess you'd have to dupe them for not-OSX. OTOH, should we be styling these subviews items like normal <menuitems>? That would give such things for free.)

    
    

    
  
Comment on attachment 771427 [details] [diff] [review]
Patch v1: create a Character Encoding widget and subview

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

Just remembered: we need to look at the browser.menu.showCharacterEncoding preference (which is a boolean stored as a nsIPrefLocalizedString, so it can be set per-locale) to see whether this widget should be in the panel by default or not.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +743,5 @@
>      let node;
>      if (aWidget.type == "custom") {
>        if (aWidget.onBuild) {
>          try {
> +          node = aWidget.onBuild(aWidget, aDocument);

In the interests of actually landing this, the following could be done as followups:

I don't think we should be exposing the underlying widget object anywhere, even to the widget itself - as it lets a 3rd party implementation change our internal data structures, which could lead to Bad Things. That was a problem before I refactored the event handlers, but being able to fix that too was a nice side-effect. Instead, I'd rather we pass a wrapper to the widget implementation (which I'd agree could be seen as mildly weird, but I think its safe).

Bonus points if you can somehow figure out how to make the wrapper initialize lazily, since Object.freeze() in the wrapper's constructor can be a bit expensive.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +227,5 @@
>      type: "custom",
>      removable: true,
>      defaultArea: CustomizableUI.AREA_PANEL,
>      allowedAreas: [CustomizableUI.AREA_PANEL, CustomizableUI.AREA_NAVBAR],
> +    onBuild: function(aWidget, aDocument) {

Nit: And if/when the first argument here turns into a wrapper, might want to s/aWidget/aWrapper/

@@ +314,4 @@
>              adjustPosition(node);
>            }
>  
> +          if (aWidgetId != aWidget.id)

Nit: May as well just keep using this.id for |id|, since that's not going to change.

@@ +557,5 @@
> +        try {
> +          title = this.charsetManager.getCharsetTitle(charset);
> +        } catch (e) {}
> +
> +        items.push({value: charset, name: title, checked: charset == currCharset});

Nit: "checked" is very UI-centric, but the rest of this function is abstracted away from anything UI related (or its easier to refactor to a JSM later). Name it something like "current" instead?

@@ +566,5 @@
> +    getAutoDetectors: function(aDocument) {
> +      let detectorEnum = this.charsetManager.GetCharsetDetectorList();
> +      let currDetector;
> +      try {
> +        currDetector = aDocument.defaultView.gPrefService.getComplexValue(

Services.prefs

@@ +583,5 @@
> +        try {
> +          title = this.charsetManager.getCharsetTitle(detector);
> +        } catch (e) {}
> +
> +        items.push({value: detector, name: title, checked: detector == currDetector});

Nit: Ditto, regarding "checked".

@@ +601,5 @@
> +      }
> +
> +      if (!containerElem._hasListener) {
> +        containerElem.addEventListener("command", this.onCommand, false);
> +        containerElem._hasListener = true;

Don't need to track this - you can't add duplicate event listeners.

Also, most of this code uses the convention of passing just |this| to addEventListener, and handling/delegating in handleEvent().

@@ +628,5 @@
> +        elem.setAttribute("value", item.value);
> +        elem.setAttribute("type", "radio");
> +        if (item.checked)
> +          elem.setAttribute("checked", "true");
> +        elem.setAttribute("group", aSection);

Is this unique enough document-wide? Might be safer to prefix it with aContainerId or something.

@@ +647,5 @@
> +      this.populateList(document,
> +                        "PanelUI-characterencoding-autodetect",
> +                        "autodetect");
> +    },
> +    onCommand: function(event) {

Nit: aEvent

@@ +664,5 @@
> +      if (section == "browser") {
> +        window.BrowserSetForcedCharacterSet(value);
> +      } else if (section == "autodetect") {
> +        // Prepare a browser page reload with a changed charset.
> +        window.BrowserCharsetReload();

Shouldn't this come last? (Even if it is async, reading the code as this happening first feels... wrong)

@@ +665,5 @@
> +        window.BrowserSetForcedCharacterSet(value);
> +      } else if (section == "autodetect") {
> +        // Prepare a browser page reload with a changed charset.
> +        window.BrowserCharsetReload();
> +        value = value.replace("chardet.", "");

Should ensure you're only stripping off the prefix.

@@ +674,5 @@
> +        try {
> +          let str =  Cc["@mozilla.org/supports-string;1"]
> +                       .createInstance(Ci.nsISupportsString);
> +          str.data = value;
> +          window.gPrefService.setComplexValue("intl.charset.detector", Ci.nsISupportsString, str);

Services.prefs

@@ +682,5 @@
> +      }
> +    },
> +    onCreated: function(node) {
> +      let window = node.ownerDocument.defaultView;
> +      window.PanelUI.panel.addEventListener("popupshowing", () => {

This will only update if the widget is placed in the panel - it needs to be updated live if the widget is in the navbar too.

        Attachment #771427 -
        Flags: feedback?(bmcbride) → feedback+

    
    

    
  


  
Can you tell me if you're too busy to review this patch? Thanks!

I think I touched all bases here, but please correct me if I'm wrong.
Dolske suggested to use <menuitem> elements here, which need a <menupopup> to get their styling applied for free and I gave that a shot. The result looks pretty good. I'll post screenshots soon.
Good thing you remembered 'browser.menu.showCharacterEncoding', but to get that working without needing to refactor anything was challenging: https://hg.mozilla.org/projects/ux/file/cf38a7ab3035/browser/components/customizableui/src/CustomizableUI.jsm#l194

That is all for now! ;)

        Attachment #771427 -
        Attachment is obsolete: true

        Attachment #777166 -
        Flags: review?(bmcbride)

    
  

        Attachment #777166 -
        Flags: review?(bmcbride) → review?(mconley)

    
    

    
  
Comment on attachment 777166 [details] [diff] [review]
Patch v2: create a Character Encoding widget and subview

Stealing this as I don't have a queue and Mike does. :-)

        Attachment #777166 -
        Flags: review?(mconley) → review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 777166 [details] [diff] [review]
Patch v2: create a Character Encoding widget and subview

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

This is an impressive patch, but I'm just going for feedback+ because of the performance issues with the lists (which I'm fairly sure we can just cache, as they're all sourced from locale files?), the listener issues... as well as the use of menuitems.

I realize several people suggested this, but all of the other views use toolbarbuttons and I'm a little bit apprehensive about suddenly changing course here just to get a checkmark. As it is, you're rebuilding the views all the time, and even if we change the code to only update the currently selected item for the page, I don't think you need the radio behaviour for that. So it boils down to adding an attribute and styling the button differently with a list-style-image (they currently have none). As Dolske pointed out, icons are available. This means we're not reusing menuitem styles - but you had to override lots of them anyway, so I suspect it won't actually be any more complex.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +42,5 @@
>  
> +    <panelview id="PanelUI-characterencoding" flex="1">
> +      <label value="&charsetMenu.label;"/>
> +      <toolbarbutton label="&charsetCustomize.label;"
> +                     oncommand="PanelUI.hide();window.openDialog('chrome://global/content/customizeCharset.xul', 'PrefWindow', 'chrome,modal=yes,resizable=yes', 'browser');"/>

As Jared said, please split this out.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +844,5 @@
>      let node;
>      if (aWidget.type == "custom") {
>        if (aWidget.onBuild) {
>          try {
> +          node = aWidget.onBuild(aWidget, aDocument);

It seems the only thing we ever really want is aWidget.currentArea.

Can I suggest, in normalizeWidget:

delete widget.implementation.currentArea;
widget.implementation.__defineGetter__("currentArea", function() { return widget.currentArea; });

and then leave this as well as most code in CustomizableWidgets untouched?

@@ +2012,5 @@
>      CustomizableUIInternal.removePanelCloseListeners(aPanel);
>    },
> +  onBrowserUpdate: function() {
> +    return CustomizableUIInternal.notifyListeners("onBrowserUpdate");
> +   }

Nit: this should line up.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +306,5 @@
>        }];
>  
>        let node = aDocument.createElementNS(kNSXUL, "toolbaritem");
>        node.setAttribute("id", "zoom-controls");
> +      node.setAttribute("title", CustomizableUI.getLocalizedProperty(aWidget, "tooltiptext"));

Even without my suggestion regarding CustomizableUI, I don't think this change is necessary. If |this| really doesn't work, AFAICT s/this/this.id/ should, at least, work.

@@ +444,5 @@
>        }];
>  
>        let node = aDocument.createElementNS(kNSXUL, "toolbaritem");
>        node.setAttribute("id", "edit-controls");
> +      node.setAttribute("title", CustomizableUI.getLocalizedProperty(aWidget, "tooltiptext"));

Ditto.

@@ +570,5 @@
> +    removable: true,
> +    defaultArea: CustomizableUI.AREA_PANEL,
> +    allowedAreas: [CustomizableUI.AREA_PANEL],
> +    charsetManager: null,
> +    getCharsetList: function(aSection, aDocument) {

As far as I can tell, we only ever pass "cache" or "static" here, and the values you get back come from properties files, which don't change. If that understanding is correct, please make a single lazily-got property (say, charsetList?) that contains the de-duplicated concatenation of both. Use a set to continuously add the lowercased names to and track duplicates as you're pulling in both sets of data.

@@ +604,5 @@
> +        try {
> +          title = this.charsetManager.getCharsetTitle(charset);
> +        } catch (e) {}
> +
> +        items.push({value: charset, name: title, current: charset == currCharset});

You can leave out the 'current' property in that case.

@@ +609,5 @@
> +      }
> +
> +      return items;
> +    },
> +    getAutoDetectors: function(aDocument) {

I suspect you could also cache this, but please check.

@@ +630,5 @@
> +        try {
> +          title = this.charsetManager.getCharsetTitle(detector);
> +        } catch (e) {}
> +
> +        items.push({value: detector, name: title, current: detector == currDetector});

Idem.

@@ +661,5 @@
> +          if (!aList.some((item) => item.name.toLowerCase() == aCurrent.name.toLowerCase())) {
> +            aList.push(aCurrent);
> +          }
> +          return aList;
> +        }, []);

Please make sure, when you're caching this, that you use a set. That gives us O(1) lookup for duplication rather than O(n). As it is we'd run O(n^2) code every time we open this view, which is sad.

@@ +662,5 @@
> +            aList.push(aCurrent);
> +          }
> +          return aList;
> +        }, []);
> +      } else {

Can this happen? I didn't see it in your patch.

@@ +670,5 @@
> +      for (let item of list) {
> +        let elem = aDocument.createElementNS(kNSXUL, "menuitem");
> +        elem.setAttribute("label", item.name);
> +        elem.setAttribute("value", item.value);
> +        elem.setAttribute("type", "radio");

Is this required to get the checkbox? There should only ever be one item checked, and clicking any of them will close the view, and we'll re-set the current item when you reopen it, right?

@@ +693,5 @@
> +    onViewShowing: function(aEvent) {
> +      if (!this.charsetManager) {
> +        this.charsetManager = Cc["@mozilla.org/charset-converter-manager;1"]
> +                                .getService(Ci.nsICharsetConverterManager);
> +      }

Can we use XPCOMUtils.defineLazyServiceGetter(..., ...) here?

@@ +706,5 @@
> +                        "autodetect");
> +    },
> +    onCommand: function(aEvent) {
> +      let node = aEvent.target;
> +      if (!node.hasAttribute || !node.hasAttribute("group")) {

Please use a class for the actual charset items. This helps with styling and also lets you remove the group/radio bits.

@@ +720,5 @@
> +
> +      if (section == "browser") {
> +        window.BrowserSetForcedCharacterSet(value);
> +      } else if (section == "autodetect") {
> +        value = value.replace("chardet.", "");

You didn't fix the prefix thing Blair noted.

@@ +737,5 @@
> +        // Prepare a browser page reload with a changed charset.
> +        window.BrowserCharsetReload();
> +      }
> +    },
> +    onCreated: function(aNode) {

Unless my memory is playing tricks on me, when I added support for this we were only calling it for non-custom widgets. This was by design, as it's essentially called when, for custom widgets, we call onBuild. If my memory is indeed not playing tricks on me, I wonder how this was working at all, and if you could please move it into onBuild.

@@ +748,5 @@
> +          // encoding.
> +          this.maybeDisableButton(aNode);
> +        }
> +      };
> +      CustomizableUI.addListener(listener);

This code is deceptively simple, but I think has a couple of problems. In particular, you're registering a listener to CustomizableUI, which is app-global. So it'll get called for any window. So any window's events are going to interfere with all windows' buttons. That's not great.

Then you're adding another listener for each window (as they'll all have onBuild called, AIUI). Not great either.

But this is also never being removed. And you're keeping the reference to aNode. If I customize and move the node to another area, won't onBuild be called again? That'll re-add the listener, too, so now we have two listeners. But the old listener is still around. It has a reference to aNode because of the closure, and we leak.

Please fix this. The easiest way is probably to have a static listener which gets a window/document reference in onBrowserUpdate, and use that to document.getElementById() the node (make sure to deal with it not existing in case the panel isn't initialized yet or the node is customized away).

To be fair, thinking about it I guess our existing listener implementations for custom widgets have similar issues, at the very least about having multiple listeners which all get notified if anything happens to any widget at all. If I'm not wrong, please file a bug about this.

::: browser/themes/linux/customizableui/panelUIOverlay.css
@@ +8,5 @@
>  %include ../../shared/customizableui/panelUIOverlay.inc.css
> +
> +#PanelUI-characterencoding > toolbarbutton {
> +  -moz-padding-start: 16px;
> +}

This makes me feel dumb. Why do we need this? Why only on this view? Why a different amount on all the platforms?

::: browser/themes/osx/browser.css
@@ +454,5 @@
> +  }
> +
> +  #characterencoding-panelmenu[open] {
> +    -moz-image-region: rect(36px, 324px, 54px, 306px);
> +  }

Nit: these are in sprite left-to-right,top-to-bottom order. Please put this in the right place.

@@ +662,5 @@
>    #history-panelmenu[customizableui-areatype="toolbar"][open] {
>      -moz-image-region: rect(72px, 360px, 108px, 324px);
>    }
>  
> +  #characterencoding-panelmenu[customizableui-areatype="toolbar"] {

Ditto.

@@ +666,5 @@
> +  #characterencoding-panelmenu[customizableui-areatype="toolbar"] {
> +    -moz-image-region: rect(0, 648px, 36px, 612px);
> +  }
> +
> +  #characterencoding-panelmenu[customizableui-areatype="toolbar"]:hover:active:not([disabled="true"]) {

Can we not use @toolbarButtonPressed@ here, too?

If so, please file a followup bug to fix this throughout the double-dpi block.

@@ +920,5 @@
>    toolbarpaletteitem[place="palette"] > #history-panelmenu {
>      -moz-image-region: rect(0px, 448px, 64px, 384px);
>    }
>  
> +  #characterencoding-panelmenu[customizableui-areatype="menu-panel"],

Please reorder this, too.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +246,5 @@
>  }
>  
>  panelview toolbarbutton,
>  #widget-overflow-list > toolbarbutton,
> +.customizationmode-button,

Is it really that hard to get checked toolbarbuttons? These selectors (well, the ones below, really) are very sadmaking.

@@ +269,5 @@
>  #PanelUI-contents #edit-controls toolbarbutton,
>  #PanelUI-contents #zoom-controls toolbarbutton,
> +.customizationmode-button,
> +#PanelUI-characterencoding-customlist > menupopup > menuitem:not([disabled]):not([checked]):not([open]):not(:active):hover,
> +#PanelUI-characterencoding-autodetect > menupopup > menuitem:not([disabled]):not([checked]):not([open]):not(:active):hover {

Open? How do submenus work inside the menupanel? Also, thinking about it, I think it's <menu>s that open, not menuitems. Are you sure you need this?

Finally, can we just add classes to the menuitems to remedy some of the hideous selector? :-)

@@ +309,5 @@
>    background-color: Highlight;
>    background-image: linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
>    background-repeat: repeat-x;
>    color: HighlightText;
> +  text-shadow: none;

I concur with Jared - what is this fixing?

::: browser/themes/shared/menupanel.inc.css
@@ +33,5 @@
>  toolbarpaletteitem[place="palette"] > #history-panelmenu {
>    -moz-image-region: rect(0px, 224px, 32px, 192px);
>  }
>  
> +#characterencoding-panelmenu[customizableui-areatype="menu-panel"],

Nit: reorder this, too.

::: browser/themes/shared/toolbarbuttons.inc.css
@@ +42,5 @@
>  #history-panelmenu[customizableui-areatype="toolbar"]{
>    -moz-image-region: rect(0, 180px, 18px, 162px);
>  }
>  
> +#characterencoding-panelmenu[customizableui-areatype="toolbar"]{

And this.

        Attachment #777166 -
        Flags: review?(gijskruitbosch+bugs) → feedback+

    
    

    
  
Thanks for the review!

(In reply to :Gijs Kruitbosch from comment #28)
> 
> I realize several people suggested this, but all of the other views use
> toolbarbuttons and I'm a little bit apprehensive about suddenly changing
> course here just to get a checkmark. As it is, you're rebuilding the views
> all the time, and even if we change the code to only update the currently
> selected item for the page, I don't think you need the radio behaviour for
> that. So it boils down to adding an attribute and styling the button
> differently with a list-style-image (they currently have none). As Dolske
> pointed out, icons are available. This means we're not reusing menuitem
> styles - but you had to override lots of them anyway, so I suspect it won't
> actually be any more complex.

I will track back to using toolbarbuttons again, as using a menu was an experiment. This will definitely be less complex.

> As far as I can tell, we only ever pass "cache" or "static" here, and the
> values you get back come from properties files, which don't change. If that
> understanding is correct, please make a single lazily-got property (say,
> charsetList?) that contains the de-duplicated concatenation of both. Use a
> set to continuously add the lowercased names to and track duplicates as
> you're pulling in both sets of data.

Unfortunately we can not be sure about the static and cache-able properties of the datasource. The user can manipulate the 'autodetect' list for sure in a special dialog and the datasource may pickup system charset changes during the applications' lifecycle. Not that this is likely to happen, but people actually using this panel at all is just as unlikely. I mean, did you? Ever? ;)

> Unless my memory is playing tricks on me, when I added support for this we
> were only calling it for non-custom widgets. This was by design, as it's
> essentially called when, for custom widgets, we call onBuild. If my memory
> is indeed not playing tricks on me, I wonder how this was working at all,
> and if you could please move it into onBuild.

`onBuild()` is specifically meant to allow custom widgets to be built, not widget of type 'view'. `onCreated()` is fired when any widget (custom, view or other) is created by `buildWidget()`.

> To be fair, thinking about it I guess our existing listener implementations
> for custom widgets have similar issues, at the very least about having
> multiple listeners which all get notified if anything happens to any widget
> at all. If I'm not wrong, please file a bug about this.

I'll investigate this and post the results in the follow-up patch.

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #29)
> > As far as I can tell, we only ever pass "cache" or "static" here, and the
> > values you get back come from properties files, which don't change. If that
> > understanding is correct, please make a single lazily-got property (say,
> > charsetList?) that contains the de-duplicated concatenation of both. Use a
> > set to continuously add the lowercased names to and track duplicates as
> > you're pulling in both sets of data.
> 
> Unfortunately we can not be sure about the static and cache-able properties
> of the datasource. The user can manipulate the 'autodetect' list for sure in
> a special dialog and the datasource may pickup system charset changes during
> the applications' lifecycle. Not that this is likely to happen, but people
> actually using this panel at all is just as unlikely. I mean, did you? Ever?
> ;)

I did, actually, because people are sometimes idiots about UTF8 vs. ISO-8859-1 on the web. But anyway. Yes, it seems I misunderstood. I'd still like to do more caching than this does, though, if that is at all possible.

> > Unless my memory is playing tricks on me, when I added support for this we
> > were only calling it for non-custom widgets. This was by design, as it's
> > essentially called when, for custom widgets, we call onBuild. If my memory
> > is indeed not playing tricks on me, I wonder how this was working at all,
> > and if you could please move it into onBuild.
> 
> `onBuild()` is specifically meant to allow custom widgets to be built, not
> widget of type 'view'. `onCreated()` is fired when any widget (custom, view
> or other) is created by `buildWidget()`.

No.

https://hg.mozilla.org/projects/ux/file/5dd57b76bc71/browser/components/customizableui/src/CustomizableUI.jsm#l826

In the if block, we call onBuild. In the else block (ie, non-custom widgets) we call onCreated. It's not called anywhere else that I can see.

However, if my suspicions about how this listener is working were right you'll need to take this out of the onBuild/onCreated functions anyway.

    
    

    
  


  
Updated patch that uses toolbarbuttons instead of menuitems

        Attachment #777166 -
        Attachment is obsolete: true

        Attachment #779156 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  


  
do not touch zoom controls styling (dunno how that happened)

        Attachment #779156 -
        Attachment is obsolete: true

        Attachment #779156 -
        Flags: review?(gijskruitbosch+bugs)

        Attachment #779157 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 779157 [details] [diff] [review]
Patch v3: create a Character Encoding widget and subview

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

Another f+, but in future, please either address review comments or explain why you haven't. For the next patch, please ask review from (or check out-of-band with) Dao about copying the checkmark files and/or what alternatives we have.

::: browser/components/customizableui/content/panelUI.js
@@ +237,5 @@
> +  /**
> +   * Open a dialog window that allow the user to customize listed character sets.
> +   */
> +  onCharsetCustomizeCommand: function() {
> +    this.hide();

Why is this necessary?

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +842,5 @@
>      let node;
>      if (aWidget.type == "custom") {
>        if (aWidget.onBuild) {
>          try {
> +          node = aWidget.onBuild(aWidget, aDocument);

Was there a particular reason you didn't take my suggestion in the previous round of reviews? That is,


It seems the only thing we ever really want is aWidget.currentArea.

Can I suggest, in normalizeWidget:

delete widget.implementation.currentArea;
widget.implementation.__defineGetter__("currentArea", function() { return widget.currentArea; });

and then leave this as well as most code in CustomizableWidgets untouched?

@@ +2014,5 @@
>      CustomizableUIInternal.removePanelCloseListeners(aPanel);
>    },
> +  onBrowserUpdate: function() {
> +    return CustomizableUIInternal.notifyListeners("onBrowserUpdate");
> +   }

Same nit: please line this up.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +584,5 @@
> +      try {
> +        let pref = "intl.charsetmenu.browser." + aSection;
> +        list = Services.prefs.getComplexValue(pref,
> +                                              Ci.nsIPrefLocalizedString).data;
> +      } catch (e) {}

OK, so it seems we can't cache all of this... but the amount of XPCOM boundary crossing here makes me uneasy.

We should probably file a followup bug. In particular, I'm fairly sure the actual charsetdata (the title and notForBrowser value) doesn't change, so we could cache that and avoid the costly XPCOM calls in the loop, or we could reimplement the whole manager in a JSM, but that's definitely not for this bug.

@@ +666,5 @@
> +          if (!aList.some((item) => item.name.toLowerCase() == aCurrent.name.toLowerCase())) {
> +            aList.push(aCurrent);
> +          }
> +          return aList;
> +        }, []);

Even if we can't cache anything here, please fix this nested reduce/some to be less inefficient.

@@ +684,5 @@
> +      let window = aNode.ownerDocument.defaultView;
> +      if (window.gBrowser &&
> +          window.gBrowser.docShell &&
> +          window.gBrowser.docShell.mayEnableCharacterEncodingMenu) {
> +        aNode.removeAttribute("disabled"); 

Nit: trailing whitespace

@@ +706,5 @@
> +        return;
> +      }
> +
> +      // The behavior as implemented here is directly based off of the
> +      // `MultiplexHandler()` method in browser.js.

Please put this comment next to the code it belongs to.

@@ +732,5 @@
> +        // Prepare a browser page reload with a changed charset.
> +        window.BrowserCharsetReload();
> +      }
> +    },
> +    onCreated: function(aNode) {

It doesn't seem like you've addressed my review comments from last time here...

::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ +72,5 @@
> +
> +# LOCALIZATION NOTE (characterencoding-panelmenu.label): Use the unicode ellipsis char,
> +# \u2026, or use "..." if \u2026 doesn't suit traditions in your locale.
> +characterencoding-panelmenu.label = Character Encoding…
> +characterencoding-panelmenu.tooltiptext = Character encoding

Please do a quick check (on IRC?) with Matej if he has a better suggestion for the tooltiptext, or file a followup bug about that once we're done here, should you prefer.

::: browser/themes/linux/browser.css
@@ +616,5 @@
>    -moz-image-region: rect(0px 48px 24px 24px);
>  }
>  
> +toolbar > .customization-target > #characterencoding-panelmenu,
> +toolbar > .customization-target > toolbarpaletteitem > #characterencoding-panelmenu {

*shudder*

Well, it seems all the other styles are doing this and you're fixing this in bug 892959, right? If that lands first, please fix this here to use the same style, and vice versa.

::: browser/themes/osx/browser.css
@@ +688,5 @@
> +  #characterencoding-panelmenu[customizableui-areatype="toolbar"] {
> +    -moz-image-region: rect(0, 648px, 36px, 612px);
> +  }
> +
> +  #characterencoding-panelmenu[customizableui-areatype="toolbar"]:hover:active:not([disabled="true"]) {

Still curious why we can't use @toolbarButtonPressed@.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +337,5 @@
>  }
>  
> +
> +.PanelUI-characterencoding-list > toolbarbutton.current {
> +  list-style-image: url(chrome://browser/skin/menu-check.png);

Hmm. I take it you've done this because this only exists on OS X? Do we not have an existing icon we can reuse? I'd like to get review from Dao on having to copy this to other platforms.

(also, if you copy, please use hg copy to track blame etc., and copy it once to ../shared, seeing as we're reusing the same icon everywhere. No need to have 4 copies in the source tree)

@@ +343,5 @@
> +  -moz-padding-start: 2px;
> +}
> +
> +.PanelUI-characterencoding-list > toolbarbutton.current > .toolbarbutton-icon {
> +  width: 11px;

I suspect we only need this on retina, don't we? In which case, please move it to the retina-specific block in the mac version of panelUIOverlay.css

        Attachment #779157 -
        Flags: review?(gijskruitbosch+bugs) → feedback+

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #33)
> Another f+, but in future, please either address review comments or explain
> why you haven't.

I was having a bad day, sorry to get you involved in that.

> ::: browser/components/customizableui/content/panelUI.js
> @@ +237,5 @@
> > +  /**
> > +   * Open a dialog window that allow the user to customize listed character sets.
> > +   */
> > +  onCharsetCustomizeCommand: function() {
> > +    this.hide();
> 
> Why is this necessary?

The panel doesn't hide itself when a toolbarbutton inside a view is clicked.

> OK, so it seems we can't cache all of this... but the amount of XPCOM
> boundary crossing here makes me uneasy.
> 
> We should probably file a followup bug. In particular, I'm fairly sure the
> actual charsetdata (the title and notForBrowser value) doesn't change, so we
> could cache that and avoid the costly XPCOM calls in the loop, or we could
> reimplement the whole manager in a JSM, but that's definitely not for this
> bug.

I'd go for a follow-up that will address this problem. I agree that values can potentially be cached and/ or the whole mechanism can be more efficient, but I'd like to fold that into JSM like Blair mentioned/ intended when he implemented this.

> Please do a quick check (on IRC?) with Matej if he has a better suggestion
> for the tooltiptext, or file a followup bug about that once we're done here,
> should you prefer.

I'll file a follow-up, so we can think of a better tooltiptext asynchronously.

> Still curious why we can't use @toolbarButtonPressed@.

I think we can use this var here, but I'd prefer to apply that change in a follow-up too, since quite a number of selectors do this. It'd look quite odd if I update only one. Perhaps bug 892959 is a good place to catch this?

I'll post a proper follow-up patch soon.

    
    

    
  


  
New (complete this time 'round ;) ) patch with review comments addressed.

        Attachment #779157 -
        Attachment is obsolete: true

        Attachment #779690 -
        Flags: review?(gijskruitbosch+bugs)

        Attachment #779690 -
        Flags: feedback?(dao)

    
    

    
  
Dão, I requested your feedback on the menu-check.png icons that I copied from toolkit/themes/osx/menu/menu-check.png, to check if you see a problem or know of a better way to introduce the check mark to the menu-panel views.

Thanks!

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #34)
> > Still curious why we can't use @toolbarButtonPressed@.
> 
> I think we can use this var here, but I'd prefer to apply that change in a
> follow-up too, since quite a number of selectors do this. It'd look quite
> odd if I update only one. Perhaps bug 892959 is a good place to catch this?

Scratch that, It needs to have its own follow-up bug. Not related to Linux at all.

    
    

    
  
Comment on attachment 779690 [details] [diff] [review]
Patch v4: create a Character Encoding widget and subview

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

Getting there!

::: browser/components/customizableui/content/panelUI.js
@@ +237,5 @@
> +  /**
> +   * Open a dialog window that allow the user to customize listed character sets.
> +   */
> +  onCharsetCustomizeCommand: function() {
> +    this.hide();

Does the handler above this not take care of this? I thought it would... That's sort of a separate bug, so please file that.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +309,5 @@
>        }];
>  
>        let node = aDocument.createElementNS(kNSXUL, "toolbaritem");
>        node.setAttribute("id", "zoom-controls");
> +      node.setAttribute("title", CustomizableUI.getLocalizedProperty(this.id, "tooltiptext"));

Did using |this| by itself really not work? Same question for the other cases where you changed this.

@@ -366,5 @@
>            if (aWidgetId != this.id)
>              return;
>  
>            updateWidgetStyle(aArea == CustomizableUI.AREA_PANEL);
> -        }.bind(this),

I'm confused. You removed all these binds, but AFAICT the listeners aren't called with the widget as |this| so you still need them. Ditto for all the others you removed...

@@ -526,5 @@
>  
>        return node;
>      }
> -  },
> -  {

Don't change this whitespace. :-)

@@ +569,5 @@
>        if (!feeds || !feeds.length) {
>          node.setAttribute("disabled", "true");
>        }
>      }
> +  }, {

And be consistent here.

@@ +769,5 @@
> +        // Widget instance removed from a document that owns it.
> +        onWidgetInstanceRemoved: (aWidgetId, aDocument) => {
> +          if (aWidgetId != this.id || aDocument != aNode.ownerDocument)
> +            return;
> +          CustomizableUI.removeListener(updateListener);

I still think this would be a lot simpler if you had a static listener and onBrowserUpdate passed the window or document, and then you simply checked for the node existing in that document. That'll guarantee there's 1 listener from the first insertion onwards, no leaks, and no 20 listeners that all fire and then go "oh, this isn't the droid, err, widget, I was looking for" and no-op.

@@ +771,5 @@
> +          if (aWidgetId != this.id || aDocument != aNode.ownerDocument)
> +            return;
> +          CustomizableUI.removeListener(updateListener);
> +          CustomizableUI.removeListener(listener);
> +        }

And, to get back to the bind thing, you use => notation here, which implicitly binds to the |this| of the context. So, fairly sure you should leave the bind calls above alone (and probably use the same bind notation here for consistency - except that I just suggested you use a static listener instead).

::: browser/themes/linux/browser.css
@@ +618,5 @@
>  
> +toolbar > .customization-target > #characterencoding-panelmenu,
> +toolbar > .customization-target > toolbarpaletteitem > #characterencoding-panelmenu {
> +  -moz-image-region: rect(0px 216px 24px 192px);
> +}

This is still/again in the wrong place. :-)

::: browser/themes/osx/jar.mn
@@ +47,5 @@
>    skin/classic/browser/KUI-background.png
>    skin/classic/browser/subtle-pattern.png
>    skin/classic/browser/menu-back.png
> +  skin/classic/browser/menu-check.png                       (../shared/menu/menu-check.png)
> +  skin/classic/browser/menu-check@2x.png                    (../shared/menu/menu-check@2x.png)

I think on OS X you could probably just reuse the existing one, but I'll let Dao respond about what the preferred way would be to deal with this.

        Attachment #779690 -
        Flags: review?(gijskruitbosch+bugs) → feedback+

    
    

    
  
I suspect Henri may have an opinion on this bug.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #38)
> Does the handler above this not take care of this? I thought it would...
> That's sort of a separate bug, so please file that.

No, and that's not a bug. The method above is not 'magically' invoked, only used by the developer-button and applied to the main button node there. This is a button INSIDE the subview, so a different logic applies. Therefore, the panel is hidden explicitly, just like CustomizableUI.hidePanelForNode does when another, dynamically generated, button inside the subview is clicked.

> I still think this would be a lot simpler if you had a static listener and
> onBrowserUpdate passed the window or document, and then you simply checked
> for the node existing in that document. That'll guarantee there's 1 listener
> from the first insertion onwards, no leaks, and no 20 listeners that all
> fire and then go "oh, this isn't the droid, err, widget, I was looking for"
> and no-op.

I don't exactly understand what you want me to do here, but allow me to try: IF onBrowserUpdate would be a static listener that exists on the widget object and is passed a document or window object, we'd be steering away from a signal-slot mechanism that implies shallow use; when a method is dispatched, it only needs to iterate the list of subscribers - O(n) where n is number of subscribers, quite low in general - whereas a static method requires the implementation to iterate over all the widgets, check IF the onBrowserUpdate method is implemented and invoke it - O(n) where n is number of widgets, generally quite large.
If this is what you meant, I'd like to understand why that is a lot simpler?
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #40)
> (In reply to :Gijs Kruitbosch from comment #38)
> > Does the handler above this not take care of this? I thought it would...
> > That's sort of a separate bug, so please file that.
> 
> No, and that's not a bug. The method above is not 'magically' invoked, only
> used by the developer-button and applied to the main button node there. This
> is a button INSIDE the subview, so a different logic applies. Therefore, the
> panel is hidden explicitly, just like CustomizableUI.hidePanelForNode does
> when another, dynamically generated, button inside the subview is clicked.

I'm not sure, because AFAICT your patch is based on an older revision of panelUI.js. At least, I can't find this function in the copy of panelUI.js which is on my machine and which was updated earlier today. In any case, it's fine to leave it as is, but really, I already did some work to consolidate our hiding logic in bug 878550 and if we need to do more, I think we should do that rather than coming up with individual solutions. So, please file a followup bug on this subject. I'm happy to look at it if nobody else does, once our perf stuff is out of the way.

> > I still think this would be a lot simpler if you had a static listener and
> > onBrowserUpdate passed the window or document, and then you simply checked
> > for the node existing in that document. That'll guarantee there's 1 listener
> > from the first insertion onwards, no leaks, and no 20 listeners that all
> > fire and then go "oh, this isn't the droid, err, widget, I was looking for"
> > and no-op.
> 
> I don't exactly understand what you want me to do here, but allow me to try:
> IF onBrowserUpdate would be a static listener that exists on the widget
> object and is passed a document or window object, we'd be steering away from
> a signal-slot mechanism that implies shallow use; when a method is
> dispatched, it only needs to iterate the list of subscribers - O(n) where n
> is number of subscribers, quite low in general - whereas a static method
> requires the implementation to iterate over all the widgets, check IF the
> onBrowserUpdate method is implemented and invoke it - O(n) where n is number
> of widgets, generally quite large.
> If this is what you meant, I'd like to understand why that is a lot simpler?

I must not have been clear. I mean that in the global scope of CustomizableWidgets.jsm, you do something like:

let CharsetButtonListener = {
  onBrowserUpdate: function(aDoc) {
    let node = aDoc.getElementById("whatever");
    if (node) {
      // maybe disable node functionality goes here
    }
  },
  register: function() {
    CustomizableUI.addListener(this);
    this.register = function() {};
  }
};

And then you call CharsetButtonListener.register() in the onBuild of the charset widget, and you make the onBrowserUpdate listener call pass the document that's actually updating.
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  


  
Gijs, thanks for the hint towards the listener struct, it simplifies the code a lot!

        Attachment #779690 -
        Attachment is obsolete: true

        Attachment #779690 -
        Flags: feedback?(dao)

        Attachment #779826 -
        Flags: review?(gijskruitbosch+bugs)

        Attachment #779826 -
        Flags: feedback?(dao)

    
    

    
  
...except now the listener is never removed... which is bad 'mkay

    
    

    
  
Comment on attachment 779826 [details] [diff] [review]
Patch v5: create a Character Encoding widget and subview

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

Lookin' good, r=me with the following tidbits still addressed.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +602,5 @@
>        if (!feeds || !feeds.length) {
>          node.setAttribute("disabled", "true");
>        }
>      }
> +  }, {

Nit: please still make this consistent as indicated in my last review (newline after ,)

@@ +697,5 @@
> +        // Combine lists, and de-duplicate.
> +        let checkedIn = {};
> +        for (let item of staticList.concat(cacheList)) {
> +          let itemName = item.name.toLowerCase();
> +          if (!checkedIn[itemName]) {

Please use a set rather than an object. :-)

::: browser/themes/linux/browser.css
@@ +713,5 @@
>  }
>  
> +toolbar > .customization-target > #characterencoding-panelmenu,
> +toolbar > .customization-target > toolbarpaletteitem > #characterencoding-panelmenu {
> +  -moz-image-region: rect(0px 216px 24px 192px);

Do file that followup bug, because thinking about it, this will also fail epically for e.g. putting the icon in any non-navbar-toolbar (because no customization target). Sadfaces all around.

        Attachment #779826 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #44)
> Nit: please still make this consistent as indicated in my last review
> (newline after ,)

I'd love to (well, not really - it hurts my eyes in truth), but it would be inconsistent with the rest of the file. The curly-on-newline is only used before the feed-button, thus being an exception (that's why I changed it in a previous patch, which I shouldn't have - I'll file a separate bug for that).

> Please use a set rather than an object. :-)

Can you explain to me why a set would be an improvement here due to its inert semantics?
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #45)
> (In reply to :Gijs Kruitbosch from comment #44)
> > Nit: please still make this consistent as indicated in my last review
> > (newline after ,)
> 
> I'd love to (well, not really - it hurts my eyes in truth), but it would be
> inconsistent with the rest of the file. The curly-on-newline is only used
> before the feed-button, thus being an exception (that's why I changed it in
> a previous patch, which I shouldn't have - I'll file a separate bug for
> that).

Ah. That's possibly my fault then. Oops. Anyway, then please disregard this nit.

> > Please use a set rather than an object. :-)
> 
> Can you explain to me why a set would be an improvement here due to its
> inert semantics?

I don't know what you mean with "inert semantics" (maybe you meant insert? Even then, not sure what you mean!). I just know that a map/dictionary/object is used to store values by key, and Sets are, well, sets. You're not really wanting to store values by key, so a Set seems like a better choice, and AFAIK should offer at-least-as-good perf guarantees. If you think objects are better, I'm happy to hear your reasoning! :-)
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #46)
> I don't know what you mean with "inert semantics" (maybe you meant insert?
> Even then, not sure what you mean!).

I tried fancy English word, failed miserably.

Oh, and I will use a Set ;)

    
    

    
  


  
Addressed final review comments. Carrying over r=Gijs.

        Attachment #779826 -
        Attachment is obsolete: true

        Attachment #779826 -
        Flags: feedback?(dao)

        Attachment #780050 -
        Flags: review+

        Attachment #780050 -
        Flags: feedback?(dao)

    
    

    
  
Comment on attachment 780050 [details] [diff] [review]
Patch v6: create a Character Encoding widget and subview

>+# LOCALIZATION NOTE (characterencoding-panelmenu.label): Use the unicode ellipsis char,
>+# \u2026, or use "..." if \u2026 doesn't suit traditions in your locale.
>+characterencoding-panelmenu.label = Character Encodingâ¦
>+characterencoding-panelmenu.tooltiptext = Character encoding

Why does this label have an ellipsis?

>diff --git a/toolkit/themes/osx/global/menu/menu-check.png b/browser/themes/shared/menu/menu-check.png
>copy from toolkit/themes/osx/global/menu/menu-check.png
>copy to browser/themes/shared/menu/menu-check.png
>diff --git a/toolkit/themes/osx/global/menu/menu-check@2x.png b/browser/themes/shared/menu/menu-check@2x.png
>copy from toolkit/themes/osx/global/menu/menu-check@2x.png
>copy to browser/themes/shared/menu/menu-check@2x.png

menu-check.png was hand-crafted for OS X and doesn't have quite the right shape on Windows/Linux.

More problematic, though, is that it won't work on a dark background. That's not a problem on OS X where we don't deal with multiple OS themes. You could probably try to inject an UTF-8 checkmark (e.g. U+2713 or U+2714) using ::before that would automatically pick up the text color.

        Attachment #780050 -
        Flags: feedback?(dao) → feedback-

    
    

    
  


  
Dão, I'm using unicode checkmarks now in a pseudo-element. I must say, I looks exquisite on all OSes, also in RTL mode!

Thanks for the suggestion, I hope you're OK with it too.

        Attachment #780050 -
        Attachment is obsolete: true

        Attachment #781628 -
        Flags: review+

        Attachment #781628 -
        Flags: feedback?(dao)

    
    

    
  
Comment on attachment 781628 [details] [diff] [review]
Patch v7: create a Character Encoding widget and subview

>+#PanelUI-characterencoding-customlist > menupopup > menuitem > .menu-iconic-text,
>+#PanelUI-characterencoding-autodetect > menupopup > menuitem > .menu-iconic-text {
>+  font-size: 11px;
>+  margin: 2px 6px 6px 2px;
>+  color: ButtonText;
>+}

Why are you setting font-size here?

The margin doesn't seem RTL-ready.

>+.PanelUI-characterencoding-list > toolbarbutton.current {

'current' should be an attribute, not a class

>+#characterencoding-panelmenu[customizableui-areatype="toolbar"]{

nit: space before {

This file seems to contain two patches (look for "# HG changeset patch").

        Attachment #781628 -
        Flags: feedback?(dao) → feedback-

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #51)
> This file seems to contain two patches (look for "# HG changeset patch").

Bah. `hg export -o` on windows from the command line apparently appends to existing patch files, instead of overwriting. I had this happening before in bug 755598, remember?

    
    

    
  


  
Dão, now it's 1 patch 1 file, fo'sho. Flagging you for sr, because that seems more appropriate. Thanks!

        Attachment #781628 -
        Attachment is obsolete: true

        Attachment #782503 -
        Flags: superreview?(dao)

        Attachment #782503 -
        Flags: review+

    
  

        Attachment #782503 -
        Flags: superreview?(dao)

        Attachment #782503 -
        Flags: review?(dao)

        Attachment #782503 -
        Flags: review+

    
  
Depends on: 899033

    
  

        Attachment #782503 -
        Flags: review?(dao)

        Attachment #782503 -
        Flags: review+

        Attachment #782503 -
        Flags: feedback?(dao)

    
    

    
  
Comment on attachment 782503 [details] [diff] [review]
Patch v8: create a Character Encoding widget and subview

Sorry for the delay, I was pondering about the onBrowserUpdate thingy. In particular, the code running in _maybeDisableButton. It's crossing the chrome/content boundary for every page load and when switching tabs which is particularly problematic considering the current effort to make Firefox e10s ready. For now, can we prevent the button from being placed on the toolbar and just update its disabled state when opening the panel menu?

>+.PanelUI-characterencoding-list > toolbarbutton[current]::before {
>+  content: "â";
>+  display: -moz-box;
>+}

Can you put this in a content stylesheet?

        Attachment #782503 -
        Flags: feedback?(dao) → feedback-

    
    

    
  
(In reply to :Ms2ger from comment #39)
> I suspect Henri may have an opinion on this bug.

Better yet, I have data!

(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #0)
> 1. Not a lot of users in North America use this feature but people in other
> countries use it quite often.

"Quite often" based on what data? How often is "quite often"?

We've had telemetry for encoding override usage in Firefox, Firefox for Android and MetroFirefox since Firefox 22. On the release channel, sessions where the character encoding override isn't used at all account for 99.98% or 99.99% (depending on how I do the query) of Firefox sessions. (The telemetry flag is CHARSET_OVERRIDE_USED.)

Do we have a way to query the telemetry data by locale?

Also, 32% of the times when the override is used, it is used to override a previously applied override. So when the users do use this feature, there seems to be quite a bit of trial and error.

Curiously, it's more common for the override to be used to correct pages that come with an encoding label (43%) than pages that don't (26%).

(The percentages add up to 101% due to rounding. See the telemetry variable CHARSET_OVERRIDE_SITUATION. The numbers are from desktop Firefox 22 with submission reason session-restore. idle-daily was similar.)
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #54)
> >+.PanelUI-characterencoding-list > toolbarbutton[current]::before {
> >+  content: "â";
> >+  display: -moz-box;
> >+}
> 
> Can you put this in a content stylesheet?

Actually, since different themes may want to use different check marks, you can leave it where it is.

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #54)
> Sorry for the delay, I was pondering about the onBrowserUpdate thingy. In
> particular, the code running in _maybeDisableButton. It's crossing the
> chrome/content boundary for every page load and when switching tabs which is
> particularly problematic considering the current effort to make Firefox e10s
> ready. For now, can we prevent the button from being placed on the toolbar
> and just update its disabled state when opening the panel menu?

Well, since bug 873066 was WONTFIXed, we don't have a generic method to prevent the button from being placed on the toolbar, right? I *could* move the button back to its previous placement once it's placed on the toolbar (from within the widget code), but that might look odd/ frustrating to users. Or, at least the 0.02% of users that use this feature. Henri, those numbers are kinda depressing! ;)

    
  
Flags: needinfo?(dao)

    
  
Blocks: 598516

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #57)
> (In reply to Dão Gottwald [:dao] from comment #54)
> > Sorry for the delay, I was pondering about the onBrowserUpdate thingy. In
> > particular, the code running in _maybeDisableButton. It's crossing the
> > chrome/content boundary for every page load and when switching tabs which is
> > particularly problematic considering the current effort to make Firefox e10s
> > ready. For now, can we prevent the button from being placed on the toolbar
> > and just update its disabled state when opening the panel menu?
> 
> Well, since bug 873066 was WONTFIXed, we don't have a generic method to
> prevent the button from being placed on the toolbar, right? I *could* move
> the button back to its previous placement once it's placed on the toolbar
> (from within the widget code), but that might look odd/ frustrating to
> users. Or, at least the 0.02% of users that use this feature. Henri, those
> numbers are kinda depressing! ;)

I'd say it's time to revisit bug 873066, then. Either that or we need a smarter way to update the button's disabled state.
Flags: needinfo?(dao)

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #58)
> I'd say it's time to revisit bug 873066, then. Either that or we need a
> smarter way to update the button's disabled state.

What if I changed the whole direct-function-calling onBrowserUpdate thingy to a CustomEvent called "browserupdate" or "asyncbrowserupdate" on the document? This way the widget can contain the _maybeDisableButton logic in a scoped event listener? Would that make it more e10s compliant?

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #59)
> (In reply to Dão Gottwald [:dao] from comment #58)
> > I'd say it's time to revisit bug 873066, then. Either that or we need a
> > smarter way to update the button's disabled state.
> 
> What if I changed the whole direct-function-calling onBrowserUpdate thingy
> to a CustomEvent called "browserupdate" or "asyncbrowserupdate" on the
> document? This way the widget can contain the _maybeDisableButton logic in a
> scoped event listener? Would that make it more e10s compliant?

I don't see how that would make a difference. Also, multiple process or not, it strikes me as inefficient that the button's disabled state is being updated all the time even if it's in the panel and the panel isn't open, whereas for the traditional menu, we call updateCharacterEncodingMenuState only when opening the popup.

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #60)
> I don't see how that would make a difference.

Ok. :(

> Also, multiple process or not,
> it strikes me as inefficient that the button's disabled state is being
> updated all the time even if it's in the panel and the panel isn't open,
> whereas for the traditional menu, we call updateCharacterEncodingMenuState
> only when opening the popup.

Certainly true! This is only needed when the char. enc. button is on the toolbar. When it's in the panel, it only needs to be updated when the panel is opened. So, yes, I'd rather have this button ONLY exist in the panel too.

Mike, how do you feel about re-opening bug 873066 for this purpose? It seems like we've hit some evidence that might result in re-opening and fixing bug 873066.
Flags: needinfo?(mconley)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #61)
> (In reply to Dão Gottwald [:dao] from comment #60)
> > I don't see how that would make a difference.
> 
> Ok. :(
> 
> > Also, multiple process or not,
> > it strikes me as inefficient that the button's disabled state is being
> > updated all the time even if it's in the panel and the panel isn't open,
> > whereas for the traditional menu, we call updateCharacterEncodingMenuState
> > only when opening the popup.
> 
> Certainly true! This is only needed when the char. enc. button is on the
> toolbar. When it's in the panel, it only needs to be updated when the panel
> is opened. So, yes, I'd rather have this button ONLY exist in the panel too.
> 
> Mike, how do you feel about re-opening bug 873066 for this purpose? It seems
> like we've hit some evidence that might result in re-opening and fixing bug
> 873066.

Hm. I'd likely treat bug 873066 as a last resort.

If the concern is that the button will still receive notifications in the panel when the panel isn't open, perhaps the button can simply know what area it was moved to last. If the area is the menu panel, then it deregisters itself from hearing the "browserupdate" event. And the opposite if it's in a toolbar.
Flags: needinfo?(mconley)

    
    

    
  
(In reply to Mike Conley (:mconley) from comment #62)
> If the concern is that the button will still receive notifications in the
> panel when the panel isn't open, perhaps the button can simply know what
> area it was moved to last. If the area is the menu panel, then it
> deregisters itself from hearing the "browserupdate" event. And the opposite
> if it's in a toolbar.

Dão's concern was about this implementation crossing the chrome/content boundary for every page load and when switching tabs (see comment 54).
Flags: needinfo?(mconley)

    
    

    
  
Understood.

Ok, besides bug 873066, what are our options?
Flags: needinfo?(mconley)

    
    

    
  
Another options is to let this specific widget put itself back immediately after it was moved to a toolbar, by listening to the specific events triggered by CustomizeMode.jsm. I suggested this in comment 57, too.

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #65)
> Another options is to let this specific widget put itself back immediately
> after it was moved to a toolbar, by listening to the specific events
> triggered by CustomizeMode.jsm. I suggested this in comment 57, too.

Hm, that sounds worse than bug 873066.

Let me rephrase - are there any viable options that would allow this button to exist in the toolbar?

    
    

    
  
(In reply to Mike Conley (:mconley) from comment #66)
> Hm, that sounds worse than bug 873066.

Hehe, agreed, amerite!

> Let me rephrase - are there any viable options that would allow this button
> to exist in the toolbar?

Well put. In that case, no.

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #67)
> (In reply to Mike Conley (:mconley) from comment #66)
> > Hm, that sounds worse than bug 873066.
> 
> Hehe, agreed, amerite!
> 
> > Let me rephrase - are there any viable options that would allow this button
> > to exist in the toolbar?
> 
> Well put. In that case, no.

Alright, cool - we've got an Australis meeting today. Mind pitching this one? Maybe we can get some support behind bug 873066.

    
    

    
  
(In reply to Mike Conley (:mconley) from comment #66)
> Let me rephrase - are there any viable options that would allow this button
> to exist in the toolbar?

Why are we trying to give more UI visibility than it has at present to a feature that goes unused in 99.98% of Firefox sessions?

    
    

    
  
I agree with Henri. The assumptions from comment 0 have been proven false. The data points to obscuring this more.

    
    

    
  
So, how about just adding it to the web developer sub view?

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #71)
> So, how about just adding it to the web developer sub view?

That would be weird, since it's not a developer feature. It's a user feature for dealing with developers' mistakes.

    
    

    
  
During the Australis meeting yesterday, where UX team members were present (shorlander, madhava, fang), this bug was discussed. Decided was to not re-open bug 873066, but instead to keep the character encoding widget enabled when the user has decided to put it inside a toolbar, even for the case where everything inside it is disabled. That's a rare case, and acceptable.
This way I can check the state of the character encoding menu prior to opening the panel or subview. 

Anne, Henri, we're not going to give more UI visibility to this feature, but only the _option_ to the user to give this more visibility through customization.

The fact that this button/ subview might end up in the panel is justified when 
a) (auto)hiding the menubar, so no primary UI discloses this functionality
b) the user explicitly puts it there because she often uses this feature. Or really diggs the icon.

So the only concern that's left regarding the data, I think, is the rather depressing thought how much impact working on this feature will have on end users. Oh well, let's just get this done!

    
    

    
  


  

        Attachment #782503 -
        Attachment is obsolete: true

        Attachment #791260 -
        Flags: review?(dao)

    
    

    
  
Screen cast of the current UI/UX: https://vimeo.com/72488651

    
    

    
  
Comment on attachment 791260 [details] [diff] [review]
Patch v9: create a Character Encoding widget and subview

>+        onWidgetAdded: (aWidgetId, aArea) => {
>+          if (aWidgetId != this.id)
>+            return;
>+          if (aArea == CustomizableUI.AREA_PANEL) {
>+            updateButton();

Why call updateButton here?

>+    id: "characterencoding-panelmenu",

Why "panelmenu"? This is a button, rename it to characterencoding-button?

>+    viewId: "PanelUI-characterencoding",

and PanelUI-characterEncodingView?

>--- a/browser/themes/linux/customizableui/panelUIOverlay.css
>+++ b/browser/themes/linux/customizableui/panelUIOverlay.css

> %include ../../shared/customizableui/panelUIOverlay.inc.css
>+
>+.PanelUI-characterencoding-list > toolbarbutton[current] {
>+  -moz-padding-start: 2px;
>+}

>--- a/browser/themes/shared/customizableui/panelUIOverlay.inc.css
>+++ b/browser/themes/shared/customizableui/panelUIOverlay.inc.css

>+.PanelUI-characterencoding-list > toolbarbutton[current] {
>+  -moz-padding-start: 6px;
>+}

You shouldn't put this in shared/ if it's not consistent across themes.

    
    

    
  


  

        Attachment #791260 -
        Attachment is obsolete: true

        Attachment #791260 -
        Flags: review?(dao)

        Attachment #792150 -
        Flags: review?(dao)

    
    

    
  
Comment on attachment 792150 [details] [diff] [review]
Patch v10: create a Character Encoding widget and subview

>+  wrapWidgetEventHandler: function(aEventName, aWidget) {
>+    aWidget[aEventName] = null;
>+    if (typeof aWidget.implementation[aEventName] != "function") {
>+      return;
>+    }

Move aWidget[aEventName] = null; before 'return' into the if-block.

>+    aWidget[aEventName] = function(...aArgs) {
>+      try {
>+        // Don't copy the function to the normalized widget object, instead
>+        // keep it on the original object provided to the API so that
>+        // additional methods can be implemented and used by the event
>+        // handlers.
>+        return aWidget.implementation[aEventName].apply(aWidget.implementation,
>+                                                        aArgs);
>+      } catch (e) {
>+        Cu.reportError(e);
>+      }
>+    };

This function doesn't always return a value. (This might in fact trigger a strict warning.) What's the point of catching exceptions here? Can you stop doing that?

        Attachment #792150 -
        Flags: review?(dao) → review+

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #78)
> This function doesn't always return a value. (This might in fact trigger a
> strict warning.) What's the point of catching exceptions here? Can you stop
> doing that?

I think the point of doing this is to guard against add-ons that consume this API (through the SDK) destroying a build flow; onBuild is also wrapped like this that provides a hook for building custom widgets, for example.
But I'd have to ask Unfocused to be exactly sure (he wrote this part).

    
    

    
  
And thanks for the review! :)

    
    

    
  
Blair, please see comment 78 and comment 79: do we need to catch exceptions when calling event handlers?
Flags: needinfo?(bmcbride)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #81)
> Blair, please see comment 78 and comment 79: do we need to catch exceptions
> when calling event handlers?

Looking at it again, we don't *need* to. onBuild is already wrapped in try/catch where it's called. onViewShowing/onViewHiding isn't, but they're attached via addEventListener so they can throw without any side effect. However (IIRC), if nViewShowing/onViewHiding throw, their exceptions won't show up anywhere unless we manually catch them - so catching and logging inside wrapWidgetEventHandler() is useful from a development perspective (browser devs and add-on devs).
Flags: needinfo?(bmcbride)

    
    

    
  
(In reply to Blair McBride [:Unfocused] from comment #82)
> However (IIRC), if nViewShowing/onViewHiding throw, their exceptions won't
> show up anywhere unless we manually catch them

I'd be surprised if that's true. If it is, that sounds like a serious Gecko bug that we should file if it isn't already filed (like bug 895340).

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #83)
> (In reply to Blair McBride [:Unfocused] from comment #82)
> > However (IIRC), if nViewShowing/onViewHiding throw, their exceptions won't
> > show up anywhere unless we manually catch them
> 
> I'd be surprised if that's true. If it is, that sounds like a serious Gecko
> bug that we should file if it isn't already filed (like bug 895340).

Bug 503244.

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #84)
> (In reply to Dão Gottwald [:dao] from comment #83)
> > (In reply to Blair McBride [:Unfocused] from comment #82)
> > > However (IIRC), if nViewShowing/onViewHiding throw, their exceptions won't
> > > show up anywhere unless we manually catch them
> > 
> > I'd be surprised if that's true. If it is, that sounds like a serious Gecko
> > bug that we should file if it isn't already filed (like bug 895340).
> 
> Bug 503244.

As far as I can tell, we're not dealing with DOM events here. notifyListeners just calls JS functions directly.

    
    

    
  
Oh, I see ViewShowing and ViewHiding are events.

    
    

    
  
so can we reach a conclusion here to keep the try...catch then?

    
    

    
  
It seems like bug 862627 will happen eventually, so the workaround may not be worth it. But if you want to keep it, you should add a comment pointing to a bug so we'll eventually get rid of the workaround.

    
  
Depends on: 862627, 503244

    
  
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M?][Australis:P1][fixed-in-ux]

    
  
Depends on: 908954

    
    

    
  
A new bug just about removing the workaround is what we normally do. I filed bug 908954 for this.
No longer depends on: 503244, 862627
Whiteboard: [Australis:M?][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1][fixed-in-ux]

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #55)
> Do we have a way to query the telemetry data by locale?

https://bug906032.bugzilla.mozilla.org/attachment.cgi?id=796536

    
  
Depends on: 934415

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5a7f22213ce9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1]
Target Milestone: --- → Firefox 28

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: