Closed Bug 952585 Opened 7 years ago Closed 7 years ago

Collect UITelemetry on customization events and durations with Australis

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Australis:P-])

Attachments

(1 file, 1 obsolete file)

This is the Australis version of bug 942274.

We want to collect counts on the following events:

1) Customization start
2) Item is moved
3) Item is removed to palette
4) Item is added from palette
I'm gonna pack duration in here as well for kicks.
Summary: Collect UITelemetry on customization events with Australis → Collect UITelemetry on customization events and durations with Australis
Attached patch Patch v1 (obsolete) — Splinter Review
This was extremely easy thanks to CustomizableUI. :D
Attachment #8350763 - Flags: review?(jaws)
Comment on attachment 8350763 [details] [diff] [review]
Patch v1

Hm, I've just realized that this cannot differentiate between the user adding, moving or removing widgets, and an add-on adding, moving or removing widgets programmatically...
Attachment #8350763 - Flags: review?(jaws)
Attached patch Patch v2Splinter Review
Ok, going from the other direction here - instead of detecting widget movements via the listener (which might include programmatic movements), we call BrowserUITelemetry.countCustomizationEvent within CustomizeMode.jsm on drop events.
Attachment #8350763 - Attachment is obsolete: true
Comment on attachment 8359250 [details] [diff] [review]
Patch v2

What do you think of this, Gijs?
Attachment #8359250 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8359250 [details] [diff] [review]
Patch v2

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

I don't need to see another patch here but please note the comments; there's some bits that definitely want adjusting before landing.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1154,5 @@
>      // Is the target the customization area itself? If so, we just add the
>      // widget to the end of the area.
>      if (aTargetNode == aTargetArea.customizationTarget) {
>        CustomizableUI.addWidgetToArea(aDraggedItemId, aTargetArea.id);
> +      let custEventType = aTargetArea == aOriginArea ? "move" : "add";

This isn't the same as the thing below. You presumably want to check aOriginArea.id !== kPaletteId again, and have the same comment.

@@ +1195,5 @@
> +
> +    // For the purposes of BrowserUITelemetry, we consider both moving a widget
> +    // within the same area, and adding a widget from one area to another area
> +    // as a "move". An "add" is only when we move an item from the palette into
> +    // an area.

Nit: shortened version of this comment above where you want to use the same logic?

::: browser/modules/BrowserUITelemetry.jsm
@@ +202,5 @@
>      return parent;
>    },
>  
>    _countableEvents: {},
> +  _countEvent: function(aCategory, aAction) {

Nit: if you make this _countEvent: function(aKeyArray) {
you can use it from _countMouseUpEvent and consolidate.

On the other hand, only two lines of code, not sure the abstraction necessarily makes sense.

@@ +431,5 @@
>  
>    getToolbarMeasures: function() {
>      let result = this._firstWindowMeasurements || {};
>      result.countableEvents = this._countableEvents;
> +    result.durations = this._durations;

So this file is called BrowserUITelemetry.jsm, and while this is about toolbars and will get logged in telemetry as such, I wonder if we should name this variable customizationDurations or something else equally descriptive. Thoughts?

@@ +445,5 @@
> +  },
> +
> +  onCustomizeStart: function(aWindow) {
> +    this._countEvent("customize", "start");
> +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);

So you only set this in the callback above. And here you just assume it exists. While that is likely, my paranoia would be more at ease if you just set it upon use if it's null here:

if (!durationMap) {
  durationMap = {};
  WINDOW_DURATION_MAP.set(aWindow, durationMap);
}

@@ +450,5 @@
> +    durationMap.customization = aWindow.performance.now();
> +  },
> +
> +  onCustomizeEnd: function(aWindow) {
> +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);

Ditto, except here you should probably just bail if durationMap is null.
Attachment #8359250 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8359250 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8359250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't need to see another patch here but please note the comments; there's
> some bits that definitely want adjusting before landing.
> 
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +1154,5 @@
> >      // Is the target the customization area itself? If so, we just add the
> >      // widget to the end of the area.
> >      if (aTargetNode == aTargetArea.customizationTarget) {
> >        CustomizableUI.addWidgetToArea(aDraggedItemId, aTargetArea.id);
> > +      let custEventType = aTargetArea == aOriginArea ? "move" : "add";
> 
> This isn't the same as the thing below. You presumably want to check
> aOriginArea.id !== kPaletteId again, and have the same comment.
> 

Yeah, you're absolutely right. Thanks for catching that.

> @@ +1195,5 @@
> > +
> > +    // For the purposes of BrowserUITelemetry, we consider both moving a widget
> > +    // within the same area, and adding a widget from one area to another area
> > +    // as a "move". An "add" is only when we move an item from the palette into
> > +    // an area.
> 
> Nit: shortened version of this comment above where you want to use the same
> logic?

Yep, good plan.

> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +202,5 @@
> >      return parent;
> >    },
> >  
> >    _countableEvents: {},
> > +  _countEvent: function(aCategory, aAction) {
> 
> Nit: if you make this _countEvent: function(aKeyArray) {
> you can use it from _countMouseUpEvent and consolidate.
> 
> On the other hand, only two lines of code, not sure the abstraction
> necessarily makes sense.
>

I'll give it a shot and see what we get.
 
> @@ +431,5 @@
> >  
> >    getToolbarMeasures: function() {
> >      let result = this._firstWindowMeasurements || {};
> >      result.countableEvents = this._countableEvents;
> > +    result.durations = this._durations;
> 
> So this file is called BrowserUITelemetry.jsm, and while this is about
> toolbars and will get logged in telemetry as such, I wonder if we should
> name this variable customizationDurations or something else equally
> descriptive. Thoughts?
> 

Well, what we're going to get is a durations object, and inside that a "customization" array of the durations on that. I guess this keeps us open for other durations (I know UX was interested in eventually getting durations on things like...window drag sessions and stuff), so I think this should probably be OK. Lemme know if there's anything else that concerns you about this.

> @@ +445,5 @@
> > +  },
> > +
> > +  onCustomizeStart: function(aWindow) {
> > +    this._countEvent("customize", "start");
> > +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);
> 
> So you only set this in the callback above. And here you just assume it
> exists. While that is likely, my paranoia would be more at ease if you just
> set it upon use if it's null here:
> 
> if (!durationMap) {
>   durationMap = {};
>   WINDOW_DURATION_MAP.set(aWindow, durationMap);
> }
> 

Yeah OK - I can see a possible (but highly unlikely) scenario in which the durationMap could be queried for before it exists. I'll put in the safeguard.

> @@ +450,5 @@
> > +    durationMap.customization = aWindow.performance.now();
> > +  },
> > +
> > +  onCustomizeEnd: function(aWindow) {
> > +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);
> 
> Ditto, except here you should probably just bail if durationMap is null.

Good call.
Thanks! Landed alterations as:

https://hg.mozilla.org/integration/fx-team/rev/a021dc363852
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a021dc363852
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P-][fixed-in-fx-team] → [Australis:P-]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.