This is a follow-up for bug 942915. We want to collect data on the usage of these buttons to decide on placement of the 'Metro Mode' button, possibly replacing 'Fullscreen'.

- flag histogram won't work, it's only one field that can only set once, and we want to count how many times people use the switch button. - throwing it up in case some one wants to poke at it while Im blocked on building.

This ports quite a bit of work we've been doing on Holly, but focuses it entirely on the fullscreen-button and switch-to-metro-button.

Hey ally, I recall you mentioning that you wanted this in before the uplift - I'm curious though why the uplift matters... Australis (and the Metro-switch button) will not be uplifted to Aurora for Firefox 28. So the probe that I land for that button will only be exposed to Nightly uses for the next ~6 weeks at least. I do already have a probe on Aurora that counts clicks on the default button set (including the fullscreen button). Anyhow, I just wanted to know how the Aurora merge deadline fits into all of this, -Mike

Comment on attachment 8344124 [details] [diff] [review] Patch 1 This ports some of the patches that have been landing on Holly, but only tracks clicks on the fullscreen and metro buttons for now. Gijs, what do you think of this?

Comment on attachment 8344124 [details] [diff] [review] Patch 1 Review of attachment 8344124 [details] [diff] [review]: ----------------------------------------------------------------- Looks good generally, but again I think we need slightly more work to figure out if we're in one of the default buttons, and I just realized that we kind of need to figure out if it was a left mouse click rather than a context/middle-click? ::: browser/modules/BrowserUITelemetry.jsm @@ +19,5 @@ > + "resource:///modules/CustomizableUI.jsm"); > + > +XPCOMUtils.defineLazyGetter(this, "AREA_IDS", function() { > + return CustomizableUI.areas; > +}); Might as well just use this directly, I think? @@ +24,5 @@ > + > +const ALL_BUILTIN_ITEMS = [ > + "fullscreen-button", > + "switch-to-metro-button", > +]; Nit: trailing whitespace @@ +59,5 @@ > + let document = aWindow.document; > + > + for (let areaID of AREA_IDS) { > + let areaNode = document.getElementById(areaID); > + areaNode.addEventListener("mouseup", this); Nit: I believe you want (areaNode.customizationTarget || areaNode).add/removeEventListener(...) (also below) @@ +88,5 @@ > + _handleMouseUp: function(aEvent) { > + let item = aEvent.originalTarget; > + // Perhaps we're seeing one of the default toolbar items > + // being clicked. > + if (ALL_BUILTIN_ITEMS.indexOf(item.id) != -1) { Again, I believe we need to check somehow that we're inside these items, so stuff works for e.g. the bookmarks-menu-button if you click the button inside it. The latter might be somewhat tricky because if the inner star button is added through a XBL binding the original button might not be in the parentNode chain, and you'd have to use node.ownerDocument.getBindingParent(node). Also, and I only just realized this, I think both here and on holly (!) we kind of want to check that aEvent.button === 0, I think.

(In reply to :Gijs Kruitbosch from comment #5) > Comment on attachment 8344124 [details] [diff] [review] > Patch 1 > > Review of attachment 8344124 [details] [diff] [review]: > ----------------------------------------------------------------- > > > ::: browser/modules/BrowserUITelemetry.jsm > @@ +19,5 @@ > > + "resource:///modules/CustomizableUI.jsm"); > > + > > +XPCOMUtils.defineLazyGetter(this, "AREA_IDS", function() { > > + return CustomizableUI.areas; > > +}); > > Might as well just use this directly, I think? > Good call - yeah, I guess we're really not saving anything substantial here. > @@ +24,5 @@ > > + > > +const ALL_BUILTIN_ITEMS = [ > > + "fullscreen-button", > > + "switch-to-metro-button", > > +]; > > Nit: trailing whitespace Fixed. > > @@ +59,5 @@ > > + let document = aWindow.document; > > + > > + for (let areaID of AREA_IDS) { > > + let areaNode = document.getElementById(areaID); > > + areaNode.addEventListener("mouseup", this); > > Nit: I believe you want (areaNode.customizationTarget || > areaNode).add/removeEventListener(...) > Ah, right! Forgot about those. Looking at the toolbar.xml binding and CustomizableUI.jsm, it looks like we should be OK in assuming that these nodes have customizationTargets, so I dropped the || areaNode. Let me know if you have concerns about that. > (also below) > > @@ +88,5 @@ > > + _handleMouseUp: function(aEvent) { > > + let item = aEvent.originalTarget; > > + // Perhaps we're seeing one of the default toolbar items > > + // being clicked. > > + if (ALL_BUILTIN_ITEMS.indexOf(item.id) != -1) { > > Again, I believe we need to check somehow that we're inside these items, so > stuff works for e.g. the bookmarks-menu-button if you click the button > inside it. The latter might be somewhat tricky because if the inner star > button is added through a XBL binding the original button might not be in > the parentNode chain, and you'd have to use > node.ownerDocument.getBindingParent(node). That's true. :/ I am, however trying to get this landed ASAP for the Metro folk. Not *entirely* sure why there's such a rush to get this landed on m-c before the uplift, but that seems to be important to them. I've pinged them for more information, but until I hear more, I'll assume the deadline is legit. I'm happy to deal with the deep and wacky widgets in a follow-up, since both the fullscreen and metro buttons are structured quite simply. > > Also, and I only just realized this, I think both here and on holly (!) we > kind of want to check that aEvent.button === 0, I think. Ah, good call! Holy crap, can't believe I missed that. Added a check for that before passing to the handler, and I'll file a bug to fix this on Holly too.

Self-reviewing, going to do a little bit of testing on my Win 8 VM, and then I'll request review again.

Because metro is(afaik) riding up to Aurora. but you raise a good point that probably didn't factor into the last minute ask from product. I'll ask product today about it.

Aha. "There's an item in the Firefox menu in the non-Australis UI" and the draft I submitted was tied to the handling function, so we'd still get telemetry data from that.

After talking with emtwo and ally, here's what we've determined that we need: 1) A patch that modifies the BrowserUITelemetry on Holly to allow monitoring click events on the "switch-to-metro" menu item. This will need to be uplifted to Aurora once the merge is complete. 2) A patch for mozilla-central that counts the number of click events on the switch-to-metro widget, as well as the fullscreen button. Separately, emtwo will work on a patch that counts how many times we exist Metro mode, outside of this bug. Of these two tasks, #1 is the higher priority, since Holly just merged to Aurora.

This one is pretty straight-forward, since we were already monitoring the appmenu.

Comment on attachment 8344883 [details] [diff] [review] [non-Australis] Patch v1 Review of attachment 8344883 [details] [diff] [review]: ----------------------------------------------------------------- Yuppers.

Ok, this addresses Gijs's original feedback, and also adds support for tracking which button was used to click an item (which can be ported over to Holly/Aurora in bug 947991).

Comment on attachment 8344986 [details] [diff] [review] [Australis] Patch v1.3 Thoughts, Gijs?

Comment on attachment 8344883 [details] [diff] [review] [non-Australis] Patch v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): None - gives Aurora the ability to track switch-to-metro menuitem clicks in the Appmenu via UITelemetry. User impact if declined: None. Testing completed (on m-c, etc.): Manual only. Risk to taking this patch (and alternatives if risky): Very, very little. String or IDL/UUID changes made by this patch: None.

Thanks Gijs - Holly patch landed: https://hg.mozilla.org/projects/holly/rev/4ce6a734a9e2

Comment on attachment 8344986 [details] [diff] [review] [Australis] Patch v1.3 Review of attachment 8344986 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but see below for some nits ::: browser/modules/BrowserUITelemetry.jsm @@ +93,5 @@ > + let countObject = this._ensureObjectChain([aCategory, aAction], { > + "left": 0, > + "middle": 0, > + "right": 0, > + }); Interesting. Why not: let countObject = this._ensureObjectChain([aCategory, aAction, buttonKey], 0); countObject[buttonKey]++; ? @@ +133,5 @@ > + _handleMouseUp: function(aEvent) { > + let item = aEvent.originalTarget; > + // Perhaps we're seeing one of the default toolbar items > + // being clicked. > + if (ALL_BUILTIN_ITEMS.indexOf(item.id) != -1) { I don't think you've addressed the feedback regarding nested items, but I suppose for these two things it doesn't really matter, so r+ either way. Will need to deal with that when expanding this code to work on other stuff, though.

Comment on attachment 8344986 [details] [diff] [review] [Australis] Patch v1.3 Oh, bugzilla. See review comments in the previous comment. :-\

(In reply to :Gijs Kruitbosch from comment #19) > Comment on attachment 8344986 [details] [diff] [review] > [Australis] Patch v1.3 > > Review of attachment 8344986 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, but see below for some nits > > ::: browser/modules/BrowserUITelemetry.jsm > @@ +93,5 @@ > > + let countObject = this._ensureObjectChain([aCategory, aAction], { > > + "left": 0, > > + "middle": 0, > > + "right": 0, > > + }); > > Interesting. Why not: > > let countObject = this._ensureObjectChain([aCategory, aAction, buttonKey], > 0); > countObject[buttonKey]++; > > ? Ah, yes, that's better. > > @@ +133,5 @@ > > + _handleMouseUp: function(aEvent) { > > + let item = aEvent.originalTarget; > > + // Perhaps we're seeing one of the default toolbar items > > + // being clicked. > > + if (ALL_BUILTIN_ITEMS.indexOf(item.id) != -1) { > > I don't think you've addressed the feedback regarding nested items, but I > suppose for these two things it doesn't really matter, so r+ either way. > Will need to deal with that when expanding this code to work on other stuff, > though. Correct - yeah, I hope to fix that when we add the other buttons.

Switched to Gijs's suggestion with countObject. Landed on fx-team as: https://hg.mozilla.org/integration/fx-team/rev/71524d9e89f0

Landed follow-up that passes the MouseEvent.button directly instead of passing MouseEvent, as requested by Gijs in bug 947991. Landed on fx-team as https://hg.mozilla.org/integration/fx-team/rev/c083d7a68fd2

Backed out that follow-up due to missing Australis in commit message: https://hg.mozilla.org/integration/fx-team/rev/564dc76ddc09 Re-landed as: https://hg.mozilla.org/integration/fx-team/rev/860f0ad9bb33 What a gong show.

Landed on mozilla-aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/e11be0659194