1474356 - Most "other" panel openings without "panel_name"

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[:Harald Kirschner :digitarald]

The expectation would that we should have a panel_name most of the time.

In a quick test, I only got "other" as name for all additional panels I have active, including "Accessibility", "Application" and "Highlighter" (https://addons.mozilla.org/en-US/firefox/addon/devtools-highlighter/)

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This is because people can use any ID they would like when they create a panel... there is no pattern.

Of course, we can easily map "Accessibility", "Application" and "Highlighter." But other new panels would need to me added manually.

Unless you want us to also log every GUID but your charts would quickly become polluted by temporary addon GUIDs.

Comment 2

[:Harald Kirschner :digitarald]

> Of course, we can easily map "Accessibility", "Application" and "Highlighter." But other new panels would need to me added manually.

Let's make sure all build-in panels are mapped.

> Unless you want us to also log every GUID but your charts would quickly become polluted by temporary addon GUIDs.

The assumption from the extension-name-matching effort was that we can get a useful name for most extension panels (tested with some of the panel names we extracted) and right now it doesn't seem to work for any.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to :Harald Kirschner :digitarald from comment #2)
> > Of course, we can easily map "Accessibility", "Application" and "Highlighter." But other new panels would need to me added manually.
> 
> Let's make sure all build-in panels are mapped.
> 
> > Unless you want us to also log every GUID but your charts would quickly become polluted by temporary addon GUIDs.
> 
> The assumption from the extension-name-matching effort was that we can get a
> useful name for most extension panels (tested with some of the panel names
> we extracted) and right now it doesn't seem to work for any.

Because the way that information was stored changed almost immediately after we landed extension-name-matching. Only built in panels will work because the ID can be literally anything.

We can add "Accessibility", "Application" and "Highlighter"... when new panels are added we will need to remember to add them though... this didn't happen for the "Accessibility" and "Application" panels.

Comment 4

[:Harald Kirschner :digitarald]

> Because the way that information was stored changed almost immediately after we landed extension-name-matching.

Can we make it work against the new format?

> We can add "Accessibility", "Application" and "Highlighter"... when new panels are added we will need to remember to add them though... this didn't happen for the "Accessibility" and "Application" panels.

Let's make sure all build-in panels are reported. Can we ensure that for the future with tests?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to :Harald Kirschner :digitarald from comment #4)
> > Because the way that information was stored changed almost immediately after we landed extension-name-matching.
> 
> Can we make it work against the new format?
> 

No, because the ID can be anything... I discussed this at length with Luca Greco.

As an example, the highlighter extension has a tab id of "webext-devtools-panel-_ff0cf743-dcf3-4097-ae4c-d872c18f7b4e_-18-0".

If you really want support for external panels you should log a new bug so we can decide how it can be done reliably.

> > We can add "Accessibility", "Application" and "Highlighter"... when new panels are added we will need to remember to add them though... this didn't happen for the "Accessibility" and "Application" panels.
> 
> Let's make sure all build-in panels are reported. Can we ensure that for the
> future with tests?

Having a test for this makes perfect sense, great idea.

The following already exist:

- inspector
- jsdebugger
- netmonitor
- storage
- styleeditor
- webconsole

So I will add:

- accessibility
- canvasdebugger
- dom
- memory
- performance
- scratchpad
- shadereditor
- webaudioeditor

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I will also add application... in fact, I can just read the panel names from definitions.js.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1474356 - Most other panel openings without panel_name

Review commit: https://reviewboard.mozilla.org/r/256618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256618/

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8991684 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256618/diff/1-2/

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8991684 [details]
Bug 1474356 - Most other panel openings without panel_name

https://reviewboard.mozilla.org/r/256618/#review263626

The important change here is at the bottom of `devtools/client/framework/toolbox.js`, where we now automatically get all built-in panel names.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: data-review.txt

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8991684 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256618/diff/2-3/

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8991684 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256618/diff/3-4/

Comment 13

[Yulia Startsev [:yulia]]

Comment on attachment 8991684 [details]
Bug 1474356 - Most other panel openings without panel_name

https://reviewboard.mozilla.org/r/256618/#review263656

I am not sure I have enough context with this review, is there a list somewhere of tools being registered?

::: devtools/client/framework/toolbox.js:3383
(Diff revision 4)
>      return extInfo && Services.prefs.getBoolPref(extInfo.pref, false);
>    },
>  
>    getTelemetryPanelNameOrOther: function(id) {
> -    if (!REGEX_PANEL.test(id)) {
> +    if (!this._toolNames) {
> +      const definitions = gDevTools.getToolDefinitionArray();

Can you point me to where this is defined? I am not familiar with this part of the code base.

Comment 14

[François Marier [:francois]]

CCing Chris because this is a new Event telemetry probe.

> The devtools.main.enter::objects field has been changed from a list of hard-coded tool names to ["tools"]. this allows us to automatically add new tools to this event without requesting a data review (the panel name is already included in extras).

That's fine for all of the built-in panels, but if you are collecting data about user-defined panels, please ask for a data review (you can link to this one and include include the changes, no need to repeat everything) since we need to make sure it doesn't include user data (which could make it a different data category).

Comment 15

[François Marier [:francois]]

Comment on attachment 8991827 [details]
data-review.txt

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Events.yaml.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Harald Kirschner.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 2.

5) Is the data collection request for default-on or default-off?

Default ON.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent.

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to François Marier [:francois] from comment #14)
> CCing Chris because this is a new Event telemetry probe.
> 
> > The devtools.main.enter::objects field has been changed from a list of hard-coded tool names to ["tools"]. this allows us to automatically add new tools to this event without requesting a data review (the panel name is already included in extras).
> 
> That's fine for all of the built-in panels, but if you are collecting data
> about user-defined panels, please ask for a data review (you can link to
> this one and include include the changes, no need to repeat everything)
> since we need to make sure it doesn't include user data (which could make it
> a different data category).

DevTools Addons are all logged as "other" so no worries there.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1474356 - Most other panel openings without panel_name

Changes:

Pretty much all files contain changes to the order of evdents and properties... these are to stop subtle issues with events not being logged because not all properties were sent and some events were sent in the wrong order.

- devtools/client/framework/toolbox.js
  - Removed unneeded const REGEX_PANEL.
  - Changed telemetry event `"devtools.main", "enter", panelName` to (`"devtools.main", "enter", "tools"`). This means that we no longer need to hard code the panel name inside of Events.yaml.
  - Now that we no longer need to hardcode the panel name in Events.yaml we can use `gDevTools.getToolDefinitionArray()` to get a list of all built-in tools. DevTools extensions are not included in this list. This means we can automatically include new tool names in the telemetry output or include "other" for devtools extensions.

- toolkit/components/telemetry/Events.yaml
  - Changed telemetry event `"devtools.main", "enter", panelName` to (`"devtools.main", "enter", "tools"`). This means that we no longer need to hard code the panel name inside of Events.yaml.

Review commit: https://reviewboard.mozilla.org/r/257322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257322/

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/1-2/

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/2-3/

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/3-4/

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/4-5/

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

https://reviewboard.mozilla.org/r/257322/#review266404

I only have a few nits and questions, but this looks good to go since try is green
thanks Mike !

::: devtools/client/framework/test/browser_devtools_api.js:10
(Diff revision 5)
> -const toolId1 = "test-tool-1";
> -const toolId2 = "test-tool-2";
> +const toolId1 = "testtool1";
> +const toolId2 = "testtool2";

uber-nit: we use testTool in the next test. Could we have similar casing here (i.e. "testTool1" and "testTool2") ?

::: devtools/client/framework/test/browser_toolbox_textbox_context_menu.js:13
(Diff revision 5)
>  // HTML inputs don't automatically get the 'edit' context menu, so we have
>  // a helper on the toolbox to do so. Make sure that shows menu items in the
>  // right state, and that it works for an input inside of a panel.
>  
>  const URL = "data:text/html;charset=utf8,test for textbox context menu";
> -const textboxToolId = "test-tool-1";
> +const textboxToolId = "testtool1";

same nit about using camelCase

::: devtools/client/framework/toolbox.js:3383
(Diff revision 5)
> -    if (!REGEX_PANEL.test(id)) {
> +    if (!this._toolNames) {
> +      const definitions = gDevTools.getToolDefinitionArray();
> +      const definitionIds = definitions.map(definition => definition.id);
> +
> +      this._toolNames = new Set(definitionIds);
> +    }
> +    if (!this._toolNames.has(id)) {
>        return "other";
>      }

great

::: devtools/client/webconsole/webconsole-output-wrapper.js:423
(Diff revision 5)
>  
>          store.dispatch(actions.messagesAdd(this.queuedMessageAdds));
>  
>          const length = this.queuedMessageAdds.length;
> +
> +        // This telemetry event is only useful when we have a toolbox so only

is it useless in browser console because we show all the messages from all the tabs ?
If so, it would be good to indicate it here.

::: toolkit/components/telemetry/Events.yaml:450
(Diff revision 5)
>      extra_keys:
>        host: "Toolbox host (positioning): bottom, side, window or other."
>        width: Toolbox width rounded up to the nearest 50px.
>        session_id: The start time of the session in milliseconds since epoch (Unix Timestamp) e.g. 1396381378123.
>    enter:
> -    objects: ["webconsole", "inspector", "jsdebugger", "styleeditor", "netmonitor", "storage", "other"]
> +    objects: ["accessibility", "application", "canvasdebugger", "dom", "fakeTool4242", "inspector", "jsdebugger", "memory", "netmonitor", "options", "other", "performance", "scratchpad", "shadereditor", "storage", "styleeditor", "testTool", "testtool1", "testTool1072208", "testtool2", "webaudioeditor", "webconsole"]

could we put all the real tools first, then "other" then fake id used by tests ?
if we could format it with one item by line, and then add a comment before all the fake ids that would be nice

::: toolkit/components/telemetry/Events.yaml:466
(Diff revision 5)
>        start_state: debuggerStatement, breakpoint, exception, tab_switch, toolbox_show, initial_panel, toggle_settings_off, toggle_settings_on, key_shortcut, select_next_key, select_prev_key, tool_unloaded, inspect_dom, unknown etc.
> -      panel_name: The name of the panel opened, webconsole, inspector, jsdebugger, styleeditor, netmonitor, storage or other
> +      panel_name: The name of the panel opened or other
>        cold: Is this the first time the current panel has been opened in this toolbox?
>        session_id: The start time of the session in milliseconds since epoch (Unix Timestamp) e.g. 1396381378123.
>    exit:
> -    objects: ["webconsole", "inspector", "jsdebugger", "styleeditor", "netmonitor", "storage", "other"]
> +    objects: ["accessibility", "application", "canvasdebugger", "dom", "fakeTool4242", "inspector", "jsdebugger", "memory", "netmonitor", "options", "other", "performance", "scratchpad", "shadereditor", "storage", "styleeditor", "testTool", "testtool1", "testTool1072208", "testtool2", "webaudioeditor", "webconsole"]

same question here

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/5-6/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55739a1e4de9
Most other panel openings without panel_name r=nchevobbe

Comment 25

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest bc failures on browser_ext_devtools_panel.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/4370c5bc1b6913a178fbe9ab0108c9152dc55803
Push link: https://hg.mozilla.org/integration/autoland/rev/55739a1e4de9fd0691e54baf04d55b4edad8a902
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=190225955&repo=autoland&lineNumber=2792

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/6-7/

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/7-8/

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8992464 [details]
Bug 1474356 - Most other panel openings without panel_name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257322/diff/8-9/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aea9d627039e
Most other panel openings without panel_name r=nchevobbe

Comment 30

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/aea9d627039e