Kris Maglione [:kmag] Reporter
	Description • 8 years ago

+++ This bug was initially created as a clone of Bug #1259093 +++

In bug 1259093, we began pre-loading browserAction popups on mousedown, and showing them only when they fully load, or if they fail to load after a delay. pageAction popups currently do not pre-load, and are not shown until they fully load, no matter how long it takes.

We should gather telemetry on the timing of various points of the opening cycle:

- How long the browser has to pre-load between the mousedown event and the command event that opens it
- How long it takes between the command event and the popup actually opening
- How long it takes between the command event and the popup's load and DOMContentLoaded events.
- How often the popup is pre-loaded on mousedown, but canceled because the click does not trigger a command event.
- How often the popup is pre-loaded, but canceled because it is closed by the extension before it is shown.

I think it may be enough to collect this data in non-release populations, but it may be useful to collect some more basic data in the long term from release populations.

	Markus Jaritz [:designakt] (UX)
	Comment 1 • 8 years ago

Great Idea.

(In reply to Kris Maglione [:kmag] from comment #0)
> ... we began pre-loading browserAction popups on mousedown, 
> ... pageAction popups currently do not pre-load, ...

Why are browserActions and pageActions treated differently?

	Kris Maglione [:kmag] Reporter
	Comment 2 • 8 years ago

(In reply to Markus Jaritz [:maritz] (UX) from comment #1)
> Great Idea.
> 
> (In reply to Kris Maglione [:kmag] from comment #0)
> > ... we began pre-loading browserAction popups on mousedown, 
> > ... pageAction popups currently do not pre-load, ...
> 
> Why are browserActions and pageActions treated differently?

Initially, it was just because of differences in the underlying implementation. At this point, given the PanelUI changes in bug 1259093, we can probably change them to behave pretty much the same way. But I think that having the two, slightly different behaviors may be a useful experiment in the short term. I'd like to see if people notice the difference, and prefer one behavior over the other.

	Markus Jaritz [:designakt] (UX)
	Comment 3 • 8 years ago

I was wondering why the pageAction in your test-extension felt faster than the browserAction. Now I know. Thx.

	Kris Maglione [:kmag] Reporter
	Comment 4 • 8 years ago

(In reply to Markus Jaritz [:maritz] (UX) from comment #3)
> I was wondering why the pageAction in your test-extension felt faster than
> the browserAction. Now I know. Thx.

Does it? Interesting. Based on this difference, the opposite should be true, actually. But there are a lot of other differences, too. The browserAction popups are a lot more complicated, under the hood.

Can you be a bit more specific about how they feel faster?

	Markus Jaritz [:designakt] (UX)
	Comment 5 • 8 years ago

Looking at the gif I made for the other bug, it seams that between click and panel fully visible the browser action needed 10frames (6 before first sign of panel), where as the pageAction only needed 8 frames (4 before first sign of panel).
https://bugzilla.mozilla.org/attachment.cgi?id=8781200&action=edit

	Kris Maglione [:kmag] Reporter
	Comment 6 • 8 years ago

(In reply to Markus Jaritz [:maritz] (UX) from comment #5)
> Looking at the gif I made for the other bug, it seams that between click and
> panel fully visible the browser action needed 10frames (6 before first sign
> of panel), where as the pageAction only needed 8 frames (4 before first sign
> of panel).

Hm. Maybe we're looking at this differently, but for for both, I see the first sign of the panel two frames after the mouse click circle disappears, but it's somewhat farther along for the browserAction. And I see the full browserAction after 4 frames, and the full pageAction after 5.

	Markus Jaritz [:designakt] (UX)
	Comment 7 • 8 years ago

(In reply to Kris Maglione [:kmag] from comment #6)
> Hm. Maybe we're looking at this differently

I did look at it using Photoshop Timeline - Frame Animation converted to Video Timeline (using the button in the bottom left of the Timeline Panel). This results in a timeline in 30fps for exact timing, whereas in the Frame Animation view each frame has a different time it is visible. (In Frame Animation I see the same number of frames you mention.) - But time-wise I think the Video Timeline is more reliable.

	Kris Maglione [:kmag] Reporter
	Comment 8 • 8 years ago

Looking at the animation timeline, I still see pretty much the same thing. 60ms before there are any signs of the panel for both browserAction and pageAction, with the browserAction more visible in its first frame. Then an additional 110ms for the browserAction to fully open, and 160ms for the pageAction.

	Markus Jaritz [:designakt] (UX)
	Comment 9 • 8 years ago

Sorry. You are right. They both take 10frames before they are fully visible.
Only difference seams to be that the pageAction is visible a little earlier, but still very much transparent.

	Kris Maglione [:kmag] Reporter
	Comment 10 • 7 years ago

I believe Bob wants to work on this, so reassigning.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 11 • 7 years ago

As per the quality planning discussion, this should include both popups and sidebars.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 12 • 7 years ago

Kris, when we discussed this in Vancouver my notes indicate that we said we wanted to track:
- browserAction creation
- Open and then fully load

But in the description of this bug more detail is requested, namely:
- How long the browser has to pre-load between the mousedown event and the command event that opens it
- How long it takes between the command event and the popup actually opening
- How long it takes between the command event and the popup's load and DOMContentLoaded events.
- How often the popup is pre-loaded on mousedown, but canceled because the click does not trigger a command event.
- How often the popup is pre-loaded, but canceled because it is closed by the extension before it is shown.

Can you please update the bug to list exactly what we want to track for this iteration of metrics collection?

	Kris Maglione [:kmag] Reporter
	Comment 13 • 7 years ago

Well, what we talked about was basically a summary of what I already wrote in this bug.

Except that I've added hover preload since then (bug 1346607), which we should probably factor in somehow or other...

	Bob Silverberg [:bsilverberg] Assignee
	Comment 14 • 7 years ago

Kris and I discussed this some more today and removed sidebar information from what we plan on collecting at this time.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 17 • 7 years ago

Updating this bug to reflect what we described in the Analysis Plan. We plan to include:

- Exponential Histogram: Time from mousedown to PageAction popup loaded
- Exponential Histogram: Time from command event to BrowserAction popup opened
- Exponential Histogram: Time from command event to BrowserAction popup load event
- Exponential Histogram: Time from command event to BrowserAction popup DOMContentLoaded event
- Enumerated Histogram: BrowserAction popup preloading success. Popup is preloaded on hover and results in:
  - Successful load
  - Load cancelled after hover
  - Load cancelled after mousedown

	Bob Silverberg [:bsilverberg] Assignee
	Comment 22 • 7 years ago

I have implemented some of what is being requested via this bug. What I have not done thus far is:

Time from command event to BrowserAction popup DOMContentLoaded event
- I believe that in order to capture the event we need to add a listener for DOMContentLoaded to the document that is created in ViewPopup in ExtensionPopups.jsm [1]. I’m not sure, however, at what point we would start the timer (or start keeping track of time) that is waiting for that DOMContentLoaded event. It seems like it would have to be somewhere in ext-browserAction.js (maybe the mouseover handler), but I’m not sure how to wire that all up.

Time from command event to BrowserAction popup load event
- I cannot figure out to what we need to attach a listener for this load event. I’ve tried a bunch of different objects, but none of them seem to fire when I expect.

If you have any advice about either of these that would be appreciated.

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/ExtensionPopups.jsm#420

	Kris Maglione [:kmag] Reporter
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8864139 [details]
Bug 1297167 - Part 1: Add telemetry probe for PageAction open timing,

https://reviewboard.mozilla.org/r/135830/#review147164

This also needs data review.

::: toolkit/components/telemetry/Histograms.json:13039
(Diff revision 4)
>      "high": 50000,
>      "n_buckets": 100,
>      "description": "The amount of time it takes for a WebExtension to start up, from when the startup function is called to when the startup promise resolves."
> +  },
> +  "WEBEXT_PAGEACTION_POPUP_OPEN_MS": {
> +    "record_in_processes": ["main", "content"],

Only main.

	Kris Maglione [:kmag] Reporter
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8868232 [details]
Bug 1297167 - Part 2: Add telemetry probes for BrowserAction popups,

https://reviewboard.mozilla.org/r/139806/#review147162

r=me with the open timing issue fixed.

This also needs data review.

::: browser/components/extensions/ext-browserAction.js:169
(Diff revision 1)
>          // Google Chrome onClicked extension API.
>          if (popupURL) {
>            try {
>              let popup = this.getPopup(document.defaultView, popupURL);
>              event.detail.addBlocker(popup.attach(event.target));
> +            TelemetryStopwatch.finish(POPUP_OPEN_MS_HISTOGRAM, this);

This won't do. We need to save the promise returned by `popup.attach` and await on it before calling finish().

::: toolkit/components/telemetry/Histograms.json:13017
(Diff revision 1)
>      "high": 60000,
>      "n_buckets": 100,
>      "description": "The amount of time it takes to load a WebExtensions background page, from when the build function is called to when the page has finished processing the onload event."
>    },
> +  "WEBEXT_BROWSERACTION_POPUP_OPEN_MS": {
> +    "record_in_processes": ["main", "content"],

Only main.

::: toolkit/components/telemetry/Histograms.json:13028
(Diff revision 1)
> +    "high": 50000,
> +    "n_buckets": 100,
> +    "description": "The amount of time it takes for a BrowserAction popup to open."
> +  },
> +  "WEBEXT_BROWSERACTION_POPUP_PRELOAD_RESULT_COUNT": {
> +    "record_in_processes": ["main", "content"],

Only main.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 29 • 7 years ago
mozreview-review-reply

Comment on attachment 8868232 [details]
Bug 1297167 - Part 2: Add telemetry probes for BrowserAction popups,

https://reviewboard.mozilla.org/r/139806/#review147162

> This won't do. We need to save the promise returned by `popup.attach` and await on it before calling finish().

I didn't save the promise, per se, I simply added an `await` before the call to `popup.attach`. Is that sufficient?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 30 • 7 years ago

Comment on attachment 8868232 [details]
Bug 1297167 - Part 2: Add telemetry probes for BrowserAction popups,

Please take a look at how I addressed the popup.attach issue and let me know if it's okay.

	Kris Maglione [:kmag] Reporter
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8868232 [details]
Bug 1297167 - Part 2: Add telemetry probes for BrowserAction popups,

https://reviewboard.mozilla.org/r/139806/#review152072

::: browser/components/extensions/ext-browserAction.js:168
(Diff revision 2)
>          // a "click" event is dispatched. This is done for compatibility with the
>          // Google Chrome onClicked extension API.
>          if (popupURL) {
>            try {
>              let popup = this.getPopup(document.defaultView, popupURL);
> -            event.detail.addBlocker(popup.attach(event.target));
> +            event.detail.addBlocker(await popup.attach(event.target));

No, this won't work. We need to pass the promise to addBlocker immediately, and *also* await on it before ending the timer. This change will cause us to not add a blocker at all.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 34 • 7 years ago

Comment on attachment 8864139 [details]
Bug 1297167 - Part 1: Add telemetry probe for PageAction open timing,

Requesting data review for this patch, which adds a temporary probe to track the time it takes to open a pageAction popup. This data will be used to make decisions about the design and caching of this function.

	Benjamin Smedberg
	Comment 35 • 7 years ago
mozreview-review

Comment on attachment 8864139 [details]
Bug 1297167 - Part 1: Add telemetry probe for PageAction open timing,

https://reviewboard.mozilla.org/r/135830/#review153620

data-r=me

	Kris Maglione [:kmag] Reporter
	Comment 36 • 7 years ago
mozreview-review

Comment on attachment 8868232 [details]
Bug 1297167 - Part 2: Add telemetry probes for BrowserAction popups,

https://reviewboard.mozilla.org/r/139806/#review154132

	Bob Silverberg [:bsilverberg] Assignee
	Comment 37 • 7 years ago

Comment on attachment 8868232 [details]
Bug 1297167 - Part 2: Add telemetry probes for BrowserAction popups,

Requesting data review for the second patch, which also adds a couple of temporary telemetry probes to collect data to help us decide what, if anything we need to change about the design of browserAction popups.

	Benjamin Smedberg
	Comment 38 • 7 years ago
mozreview-review

Comment on attachment 8868232 [details]
Bug 1297167 - Part 2: Add telemetry probes for BrowserAction popups,

https://reviewboard.mozilla.org/r/139806/#review154654

data-r=me

	Pulsebot
	Comment 41 • 7 years ago

Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4641fc51e28
Part 1: Add telemetry probe for PageAction open timing, r=bsmedberg,kmag
https://hg.mozilla.org/integration/autoland/rev/63c2552a27d7
Part 2: Add telemetry probes for BrowserAction popups, r=bsmedberg,kmag

	Carsten Book [:Tomcat]
	Comment 42 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b4641fc51e28
https://hg.mozilla.org/mozilla-central/rev/63c2552a27d7