Closed Bug 1012526 Opened 5 years ago Closed 5 years ago

UITour.jsm only registers with UITelemetry when it's lazily imported on-demand

Categories

(Firefox :: General, defect)

29 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox29 --- affected
firefox30 --- fixed
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: Unfocused, Assigned: Unfocused)

References

Details

(Whiteboard: p=3 s=it-32c-31a-30b.2 [qa-])

Attachments

(1 file, 1 obsolete file)

So, UITour.jsm is lazily imported on-demand whenever:
* A page uses the mozUITour API
* The person uses the Tour menuitem from the Help menu

And as part of the initialization process of UITour, it registers with UITelemetry.jsm a simple measure function. This is a problem, because we *always* want that to be registered with UITelemetry, so the data from UITour can be collected.

Note: This *only* affects seenPageIDs, as that's the only telemetry data collected by UITour.jsm

I think this would explain why we may not be seeing seenPageIDs not show up in telemetry as often as expected.
Gavin: Will need this fixed ASAP, as it affects telemetry collection of data that we're trying to analyse *now*. Can you add this to the priority backlog?
Flags: needinfo?(gavin.sharp)
Flags: firefox-backlog+
Whiteboard: p=3
Marco, can you please add this to the current iteration?
Flags: needinfo?(gavin.sharp) → needinfo?(mmucci)
Added to Iteration 32.2
Flags: needinfo?(mmucci)
Whiteboard: p=3 → p=3 s=it-32c-31a-30b.2 [qa?]
Attached patch Patch v1 (obsolete) — Splinter Review
This also lets UITour.jsm remain being lazy-loaded.

Sadly, I don't think we can test this. If it were testable via xpcshell, it'd be easy - as we get a new environment for each test. But we can only test this via mochitest... in which case we can't guarantee that some other test hasn't already caused UITour.jsm to be loaded beforehand :\
Attachment #8425170 - Flags: review?(mconley)
Comment on attachment 8425170 [details] [diff] [review]
Patch v1

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

Bah, sorry for not catching this the first time. :( This looks good - just some minor things.

::: browser/modules/BrowserUITelemetry.jsm
@@ +171,5 @@
>    init: function() {
>      UITelemetry.addSimpleMeasureFunction("toolbars",
>                                           this.getToolbarMeasures.bind(this));
> +    // Ensure that UITour.jsm remains lazy-loaded, yet always registers its
> +    // simple measure functiojn with UITelemetry.

Nit - typo: "functiojn" -> "function"

::: browser/modules/UITour.jsm
@@ +7,5 @@
>  this.EXPORTED_SYMBOLS = ["UITour"];
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
> +Cu.reportError("UITour.jsm loaded");

Please remove before landing.
Attachment #8425170 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/f1bd861d8acf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa?] → p=3 s=it-32c-31a-30b.2 [qa-]
Status: RESOLVED → VERIFIED
Attached patch Patch v1.1Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 952568
User impact if declined: No user impact. Impact is lack of/inconsistent Telemetry data. Unable to analyse anything but immediate (within 1 hour) impact of whats-new tour page on usage of UI, via Telemetry.
Testing completed (on m-c, etc.): Unable to automate test. Manually tested.
Risk to taking this patch (and alternatives if risky): Minimal - can only impact telemetry collection of what is already broken. Change to fix that is minimal low-risk change.
String or IDL/UUID changes made by this patch: None
Attachment #8425170 - Attachment is obsolete: true
Attachment #8426728 - Flags: review+
Attachment #8426728 - Flags: approval-mozilla-beta?
Attachment #8426728 - Flags: approval-mozilla-aurora?
Attachment #8426728 - Flags: approval-mozilla-beta?
Attachment #8426728 - Flags: approval-mozilla-beta+
Attachment #8426728 - Flags: approval-mozilla-aurora?
Attachment #8426728 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.