Closed Bug 948139 Opened 6 years ago Closed 6 years ago

Add telemetry for switching from Metro to Desktop

Categories

(Firefox for Metro Graveyard :: General, defect, P2)

x86_64
Windows 8
defect

Tracking

(firefox28 fixed, firefox29 fixed)

RESOLVED FIXED
Firefox 29
Tracking Status
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: emtwo, Assigned: sfoster)

References

Details

(Whiteboard: [beta28] [feature] p=1 [qa-])

Attachments

(1 file, 3 obsolete files)

We want to see how often a user clicks the button in Metro to switch to Desktop. This is a follow up from bug 944114.
Whiteboard: [release28]
Assignee: nobody → sfoster
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] → [beta28] p=1
Taking cues from BrowserUITelemetry.jsm, as far as I can tell this ought to do it. In about:telemetry I see an entry:

UITelemetry 	{"metro-appbar": {"countableEvents":{}}} 

..clicking on the view on desktop menu item, and just before we actually restart a "view-on-desktop" property should get incremented.
Attachment #8349064 - Flags: feedback?(mbrubeck)
Comment on attachment 8349064 [details] [diff] [review]
WIP: Use UITelemetry to capture clicks on the 'switch to desktop' appbar item

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

Looks fine to me, though I'm not really familiar with Telemetry code so I don't know whether there are any gotchas to look out for.
Attachment #8349064 - Flags: feedback?(mbrubeck) → feedback+
:mconley, see Matt's comment above. We haven't got plans to instrument all the Metro UI right now, so I'm striking a balance between one-off code and possible future needs. This should increment aUITelemetry.metro-appbar.countableEvents.switch-to-desktop-button, which AFAICT is parallel to the switch-to-metro-button on the desktop side. 

With your f+ I'll get mbrubeck or another metro peer to review for landing.
Attachment #8349064 - Attachment is obsolete: true
Attachment #8350403 - Flags: feedback?(mconley)
Quick update, fixing style in the _incrementCountableEvent method per your earlier comments on the other telemetry patch.
Attachment #8350403 - Attachment is obsolete: true
Attachment #8350403 - Flags: feedback?(mconley)
Attachment #8350404 - Flags: feedback?(mconley)
Comment on attachment 8350404 [details] [diff] [review]
Use UITelemetry to capture clicks on the 'switch to desktop' appbar item

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

It's hard for me to give good feedback here because metro is a little bit of a mystery to me, but assuming the addSimpleMeasureFunction is in a path that is guaranteed to only run once, then this is probably OK.

::: browser/metro/base/content/appbar.js
@@ +26,5 @@
>      // tilegroup selection events for all modules get bubbled up
>      window.addEventListener("selectionchange", this, false);
> +
> +    // gather appbar telemetry data
> +    UITelemetry.addSimpleMeasureFunction("metro-appbar",

Is this guaranteed to run once?
Its goes like this: 

browser.xul calls Browser.startup() from its onload handler, Browser.startup() calls singleton BrowserUI.init() 
which calls singleton ContextUI.init() which calls singleton Appbar.init();

We dont enforce singleton-ness, nor do we always guard against calling init() twice, i.e. it is *supposed* to run only once and other bad things would happen if it were called again. But I'll wrap it in a try/catch for good measure I think. 

Side-note: if UITelementy.jsm provided a hasSimpleMeasuresFunction, or a getSimpleMeasuresFunction we could guard against this case without resorting to try/catch. Was that a design decision?
oh, in Metro land we only ever have a single instance of the browser chrome. Thats probably the source of the confusion here.
Blocks: metrov1it22
No longer blocks: metrov1it21
(In reply to Sam Foster [:sfoster] from comment #7)
> oh, in Metro land we only ever have a single instance of the browser chrome.
> Thats probably the source of the confusion here.

Ah, OK, good to know. Then yes, this answers my question.

(In reply to Sam Foster [:sfoster] from comment #6)
> Side-note: if UITelementy.jsm provided a hasSimpleMeasuresFunction, or a
> getSimpleMeasuresFunction we could guard against this case without resorting
> to try/catch. Was that a design decision?

It wasn't so much a design decision as just the shortest path to getting something landed. Having a hasSimpleMeasuresFunction would be perfectly acceptable to me.
Added the try/catch to swallow the (shouldn't happen) exception that might occur if Appbar.init is run twice, per mconleys feedback above.
Attachment #8350404 - Attachment is obsolete: true
Attachment #8350404 - Flags: feedback?(mconley)
Attachment #8355364 - Flags: review?(mbrubeck)
I'm also taking the shortest path to landing here, by just swallowing the exception thrown if my simple measure is already registered. We can add hasSimpleMeasuresFunction or similar if that starts to smell bad, but its consistent with usage elsewhere.
Attachment #8355364 - Flags: review?(mbrubeck) → review+
Blocks: 956798
https://hg.mozilla.org/mozilla-central/rev/b3b7920dfbb9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment on attachment 8355364 [details] [diff] [review]
Use UITelemetry to capture clicks on the 'switch to desktop' appbar ite

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a (new feature)
User impact if declined: No visible impact to the user. The telemetry probe will help us understand and prioritize future development
Testing completed (on m-c, etc.): Tested
Risk to taking this patch (and alternatives if risky): Low-risk, metro-only patch
String or IDL/UUID changes made by this patch: None
Attachment #8355364 - Flags: approval-mozilla-aurora?
Attachment #8355364 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [beta28] p=1 → [beta28] [feature] p=1
Whiteboard: [beta28] [feature] p=1 → [beta28] [feature] p=1 [qa-]
You need to log in before you can comment on or make changes to this bug.