1360756 - Show the base url in about:debugging

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, enhancement, P5)

Description

[Andy McKay]

Attachment: Screenshot 2017-04-28 16.49.58.png

When a WebExtension is installed an internal UUID is assigned, but that value is hard to get at easily. Let's show that in about:debugging.

Comment 1

[Andy McKay]

Attachment: base_url.patch

Some questions:
* should the URL be clickable? 
* should we do a seperate line of say "Addon internal ID" of just the URL?
* I couldn't find any test coverage of https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webextension.js#90, so not sure where I should be testing that.

Comment 2

[Mark Striemer [:mstriemer]]

Comment on attachment 8863072 [details] [diff] [review]
base_url.patch

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

> Some questions:
>   * should the URL be clickable?

Having a clickable URL might make sense. Is there a way to tell if it's going to have meaningful content?

>   * should we do a seperate line of say "Addon internal ID" of just the URL?

This seems reasonable, I'm not sure what you'd need to know it for, though?

>   * I couldn't find any test coverage of https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webextension.js#90, so not sure where I should be testing that.

This I'm not sure of but it would be nice to test more than just `moz-extension://` as the value.

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ -310,5 @@
>  }
>  
>  /* We want the ellipsis on the left-hand-side, so make the parent RTL
>   * with an ellipsis and the child can be LTR. */
> -.file-path {

The .file-path CSS should be specific to the file path. I don't think you should need any extra CSS for the new row but if you do it should be in its own block and any duplicates here can be removed.

(This has some extra styles to get an ellipsis on the left-hand-side that shouldn't be needed for other fields)

::: devtools/client/aboutdebugging/components/addons/target.js
@@ +41,3 @@
>      // the ellipsis on the left.
>      dom.dd(
> +      { className: "addon-target-info-content addon-target-info-value " + className },

I think `addon-target-info-value` means the same thing as `addon-target-info-content`. I think I prefer the `-value` suffix so if you want to change it that works for me but any styles you need should be able to go with the `-content` block.

@@ -34,4 @@
>      // the ellipsis on the left.
>      dom.dd(
> -      { className: "addon-target-info-content file-path" },
> -      dom.span({ className: "file-path-inner", title: path }, path),

This span is specific to the file path for the LTR ellipsis, it can be passed in from `filePathForTarget()`. The comments about RTL/LTR should be moved there too.

@@ +97,5 @@
>        ),
>        dom.dl(
>          { className: "addon-target-info" },
>          ...filePathForTarget(target),
> +        ...targetInfoList("baseURL", target.baseURL, "base-url")

This should have a trailing comma.

::: devtools/client/aboutdebugging/test/browser_addons_debug_info.js
@@ +18,5 @@
>  
>    // Verify that the path to the install location is shown next to its label.
>    ok(filePath, "file path is in DOM");
>    ok(filePath.textContent.endsWith(expectedFilePath), "file path is set correctly");
>    is(filePath.previousElementSibling.textContent, "Location", "file path has label");

I'm guessing this extension doesn't have a base URL for some reason? If you want to just check the other extension instead I think that should work.

@@ +48,5 @@
> +  is(filePath.previousElementSibling.textContent, "Location", "file path has label");
> +
> +  let baseURL = container.querySelector(".base-url");
> +  ok(baseURL, "baseURL is present");
> +  ok(baseURL.textContent.startsWith("moz-extension://", "baseURL is correct"));

It would be nice to test the part after the protocol as well. Will it be a guid? Maybe checking its format with a regex would be a good idea.

@@ +49,5 @@
> +
> +  let baseURL = container.querySelector(".base-url");
> +  ok(baseURL, "baseURL is present");
> +  ok(baseURL.textContent.startsWith("moz-extension://", "baseURL is correct"));
> +  is(baseURL.previousElementSibling.textContent, "Base URL", "file path has label");

The description here is incorrect.

::: devtools/client/locales/en-US/aboutdebugging.properties
@@ +82,5 @@
>  # LOCALIZATION NOTE (location):
>  # This string is displayed as a label for the filesystem location of an extension.
>  location = Location
>  
> +# LOCALIZATION NOTE (location):

This should be

# LOCALIZATION NOTE (baseURL):

::: devtools/server/actors/webextension.js
@@ +101,5 @@
>      iconURL: this.addon.iconURL,
>      debuggable: this.addon.isDebuggable,
>      temporarilyInstalled: this.addon.temporarilyInstalled,
>      isWebExtension: this.addon.isWebExtension,
> +    baseURL: ExtensionManagement.getURLForExtension(this.id)

This should have a trailing comma.

Comment 3

[Andy McKay]

(In reply to Mark Striemer [:mstriemer] from comment #2)
> Having a clickable URL might make sense. Is there a way to tell if it's
> going to have meaningful content?

Not really, but lets try.

> >   * should we do a seperate line of say "Addon internal ID" of just the URL?
> 
> This seems reasonable, I'm not sure what you'd need to know it for, though?

We are using it in a couple of APIs to give an ID for the add-on. It's not that commonly used, but a pain to find when you need it.

> >   * I couldn't find any test coverage of https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webextension.js#90, so not sure where I should be testing that.
> 
> This I'm not sure of but it would be nice to test more than just
> `moz-extension://` as the value.

I've updated it to do check its a valid UUID.

> This span is specific to the file path for the LTR ellipsis, it can be
> passed in from `filePathForTarget()`. The comments about RTL/LTR should be
> moved there too.

I've cleaned up the CSS a bit and simplified it.

> I'm guessing this extension doesn't have a base URL for some reason? If you
> want to just check the other extension instead I think that should work.

It's a legacy URL, uses install.rdf, this splits into two tests which will be useful, one for old one for WebExtensions.

All the rest are done, new image and patch inbound.

Comment 4

[Andy McKay]

Attachment: Screenshot 2017-05-05 18.42.34.png

Comment 5

[Andy McKay]

Attachment: bug 1360756 show internal ID for about:debugging

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

Comment 6

[Mark Striemer [:mstriemer]]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review140238

r+wc

::: commit-message-37a5b:1
(Diff revision 1)
> +bug 1360756 show internal ID r?mstriemer

I'd mention this is for about:debugging but perhaps that isn't too important.

::: devtools/client/aboutdebugging/components/addons/target.js:48
(Diff revision 1)
> +  if (!target.baseURL) {
> +    return [];
> +  }
> +  let uuid = target.baseURL.slice(
> +    "moz-extension://".length,
> +    target.baseURL.length - 1

Is this stripping a trailing slash? As much as I don't like regexes maybe this would be better as a regex? Could do it in two operations with slice as well.

```
let uuid = /moz-extension:\/\/([^/]*)/.exec(target.baseURL)[1];
// or
let uuid = target.baseURL.slice("moz-extension://".length);
uuid = uuid.slice(0, uuid.indexOf("/"));
```

::: devtools/client/aboutdebugging/components/addons/target.js:61
(Diff revision 1)
> +      { className: "addon-target-info-content internal-id" },
> +      dom.span(
> +        { title: uuid },
> +        uuid
> +      ),
> +      dom.span(

You can include an explicit space rather than adding it using CSS.

```
dom.dd(
  { ... },
  dom.span({title: uuid}, uuid),
  " ",
  dom.span({}, "(", dom.a(...), ")"),
);
```

::: devtools/client/aboutdebugging/components/addons/target.js:63
(Diff revision 1)
> +        { title: uuid },
> +        uuid
> +      ),
> +      dom.span(
> +        { className: "addon-target-info-more" },
> +        "(",

There was an issue filed about not hard-coding parens. Maybe there's a better way to include the link.

Just "{abc123-...} Open base URL" might work. There's also an "Inspect" label in the mocks, I'm not sure what that will inspect but maybe it could be "Inspect    [Base URL]    [manifest.json]    ..."

Comment 7

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review140238

> Is this stripping a trailing slash? As much as I don't like regexes maybe this would be better as a regex? Could do it in two operations with slice as well.
> 
> ```
> let uuid = /moz-extension:\/\/([^/]*)/.exec(target.baseURL)[1];
> // or
> let uuid = target.baseURL.slice("moz-extension://".length);
> uuid = uuid.slice(0, uuid.indexOf("/"));
> ```

It is. I'm not keen on regex either, but its one line shorter. But really I should add in a comment to explain what I'm doing here to explain the regex for my future self.

> You can include an explicit space rather than adding it using CSS.
> 
> ```
> dom.dd(
>   { ... },
>   dom.span({title: uuid}, uuid),
>   " ",
>   dom.span({}, "(", dom.a(...), ")"),
> );
> ```

I could, but doing it as 1ch lines everything up and feels better to me.

> There was an issue filed about not hard-coding parens. Maybe there's a better way to include the link.
> 
> Just "{abc123-...} Open base URL" might work. There's also an "Inspect" label in the mocks, I'm not sure what that will inspect but maybe it could be "Inspect    [Base URL]    [manifest.json]    ..."

I've removed the parens now that jdescottes patch has landed so it's all consistent.

I think "Inspect" is based on Chrome which has this and essentially loads the Chrome devtools on a page. We have that through the debug button.

It might be nice to maybe figure out what URLs are available and make those links, but I'm not sure of the value of that, its probably a seperate bug.

Comment 8

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

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

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review142466

Looks great, thanks!

Let's just get someone from l10n to have a look at the localisation notes before landing.

::: devtools/client/locales/en-US/aboutdebugging.properties:70
(Diff revision 2)
>  # LOCALIZATION NOTE (temporaryExtensions):
>  # This string is displayed as a header above the list of temporarily loaded add-ons.
>  temporaryExtensions = Temporary Extensions
>  
> +# LOCALIZATION NOTE (internalUUID):
> +# This string is displayed as a label for the internal UUID of an extension.

Maybe the localization notes need to be expanded a bit for localizers. 

Is UUID understandable, should we explain how this id is "internal"?
For the second one, what is the base URL, what is it used for (maybe simply saying that clicking on it allows to see the root of the extension and its content or something like that).

I prefer to ask for feedback from l10n team here.

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

Francesco, can you review the localization notes in this patch ?

> # LOCALIZATION NOTE (internalUUID):
> # This string is displayed as a label for the internal UUID of an extension.
> internalUUID = Internal UUID
> 
> # LOCALIZATION NOTE (baseURL):
> # This string is displayed as a label for the base URL of an extension.
> baseURL = Base URL

Comment 11

[Francesco Lodolo [:flod]]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review142486

::: devtools/client/locales/en-US/aboutdebugging.properties:70
(Diff revision 2)
>  # LOCALIZATION NOTE (temporaryExtensions):
>  # This string is displayed as a header above the list of temporarily loaded add-ons.
>  temporaryExtensions = Temporary Extensions
>  
> +# LOCALIZATION NOTE (internalUUID):
> +# This string is displayed as a label for the internal UUID of an extension.

UUID should be OK, explaining the "internal" part would be useful, e.g.

# LOCALIZATION NOTE (internalUUID): label for the internal unique identifier
# (UUID) assigned to WebExtensions when installed

I confess I'm lost at what "Base URL" means in this context, so it definitely needs a localization comment. 

Is the screenshot obsolete? Because the text is displayed between parenthesis (not available in the string), and it's a link, not a label.

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: screenshot_bug_1360756.png

Here is an updated screenshot (parentheses are gone). And indeed it is a link and it should be mentioned in the localization notes.

Comment 13

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review142486

> UUID should be OK, explaining the "internal" part would be useful, e.g.
> 
> # LOCALIZATION NOTE (internalUUID): label for the internal unique identifier
> # (UUID) assigned to WebExtensions when installed
> 
> I confess I'm lost at what "Base URL" means in this context, so it definitely needs a localization comment. 
> 
> Is the screenshot obsolete? Because the text is displayed between parenthesis (not available in the string), and it's a link, not a label.

The screenshot is obsolete, it's updated on the bug. I've updated the localisation comments.

Comment 14

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review144210

Comment 15

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

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

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bc43645a0f20 -d 42ab019426c4: rebasing 396704:bc43645a0f20 "bug 1360756 show internal ID for about:debugging r=jdescottes,mstriemer" (tip)
merging devtools/server/actors/webextension.js
warning: conflicts while merging devtools/server/actors/webextension.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review144584

::: devtools/server/actors/webextension.js:19
(Diff revision 3)
>  
>  loader.lazyRequireGetter(this, "mapURIToAddonID", "devtools/server/actors/utils/map-uri-to-addon-id");
>  loader.lazyRequireGetter(this, "unwrapDebuggerObjectGlobal", "devtools/server/actors/script", true);
>  
>  loader.lazyImporter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
> +loader.lazyImporter(this, "ExtensionManagement", "resource://gre/modules/ExtensionManagement.jsm");

Sorry Andy, I broke your patch :-)

After Bug 1302702 has been landed, this file (webextension.js) is now splitted into two different files:

- webextension-parent.js: which provides the actor running in the main process (the one that can access the AddonManager and it provide the addon metadata to the about:debugging page), this file is loaded and used in the main process
- webextension.js: which provides a "tab actor" which is what is connected and used by the developer tools panels (e.g. the inspector, the webconsole, the debugger etc), this file is loaded and used in the child process.

Both the changes applied by this patch to 'webextension.js' (this one at line 19 and the one at line 105) are related to the addon metadata that is rendered in the about:debugging page, and so they now have to be applied to "webextension-parent.js", in the following places:

- http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/devtools/server/actors/webextension-parent.js#11-12
- http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/devtools/server/actors/webextension-parent.js#66-76

Let me know how it goes.

Comment 18

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review144584

> Sorry Andy, I broke your patch :-)
> 
> After Bug 1302702 has been landed, this file (webextension.js) is now splitted into two different files:
> 
> - webextension-parent.js: which provides the actor running in the main process (the one that can access the AddonManager and it provide the addon metadata to the about:debugging page), this file is loaded and used in the main process
> - webextension.js: which provides a "tab actor" which is what is connected and used by the developer tools panels (e.g. the inspector, the webconsole, the debugger etc), this file is loaded and used in the child process.
> 
> Both the changes applied by this patch to 'webextension.js' (this one at line 19 and the one at line 105) are related to the addon metadata that is rendered in the about:debugging page, and so they now have to be applied to "webextension-parent.js", in the following places:
> 
> - http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/devtools/server/actors/webextension-parent.js#11-12
> - http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/devtools/server/actors/webextension-parent.js#66-76
> 
> Let me know how it goes.

Thanks Luca!

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review144694

::: devtools/client/aboutdebugging/test/browser_addons_debug_info.js:44
(Diff revision 3)
> +  let container = document.querySelector(`[data-addon-id="${addonId}"]`);
> +  let filePath = container.querySelector(".file-path");
> +  let expectedFilePath = "/test/addons/test-devtools-webextension-nobg/";
> +
> +  ok(filePath, "file path is in DOM");
> +  ok(filePath.textContent.endsWith(expectedFilePath), "file path is set correctly");
> +  is(filePath.previousElementSibling.textContent, "Location", "file path has label");

Nit: it looks like this part of the assertions are a good candidate to be extracted into an helper function shared between the two tests (mostly to make it easier to update the test if we change the react components that renders the tested DOM)

Comment 20

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

https://reviewboard.mozilla.org/r/136752/#review144700

::: devtools/client/aboutdebugging/test/browser_addons_debug_info.js:44
(Diff revision 3)
> +  let container = document.querySelector(`[data-addon-id="${addonId}"]`);
> +  let filePath = container.querySelector(".file-path");
> +  let expectedFilePath = "/test/addons/test-devtools-webextension-nobg/";
> +
> +  ok(filePath, "file path is in DOM");
> +  ok(filePath.textContent.endsWith(expectedFilePath), "file path is set correctly");
> +  is(filePath.previousElementSibling.textContent, "Location", "file path has label");

Fair enough.

Comment 21

[Andy McKay]

Comment on attachment 8865069 [details]
bug 1360756 show internal ID for about:debugging

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

Comment 22

[Andy McKay]

Let's try that again, I rebased onto Luca's changes and he gave it a once over. Because of bug 1366342, I changed this to show the manifest instead of the base URL.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/147506bf6712
show internal ID for about:debugging r=jdescottes,mstriemer

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/147506bf6712