1451795 - Store the devtools pages definitions into the instance of the ExtensionAPI class instead of a global Map.

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, enhancement, P3)

Description

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

This is a follow up originated from Bug 1394750 review comments:

::: browser/components/extensions/ext-devtools.js:483
(Diff revision 4)
> +
>      // Create and register a new devtools_page definition as specified in the
>      // "devtools_page" property in the extension manifest.
> -    let devtoolsPageDefinition = new DevToolsPageDefinition(extension, manifest.devtools_page);
> +    let devtoolsPageDefinition = new DevToolsPageDefinition(extension, manifest.devtools_page,
> +                                                            () => this.isDevToolsDisabled());
>      devtoolsPageDefinitionMap.set(extension, devtoolsPageDefinition);

>> It doesn't have to happen here, but can we just store this 
>> in the instance of the API class rather than in a global Map?

>Yeah, I think that we can do that if we also change the way we lazily
>initialize the devtools API (which is currently happening when we create
>the first devtools page definition from a devtools_page manifest entry, 
>from the global initDevTools function which is accessing that map: 
https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/browser/components/extensions/parent/ext-devtools.js#293)

(quoted from https://bugzilla.mozilla.org/show_bug.cgi?id=1394750#c36)

Comment 1

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

Attachment: Bug 1451795 - Move lazy devtools initialization helper into a static method of the ExtensionAPI.

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

Comment 2

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

Comment on attachment 8969733 [details]
Bug 1451795 - Move lazy devtools initialization helper into a static method of the ExtensionAPI.

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

Comment 3

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

Comment on attachment 8969733 [details]
Bug 1451795 - Move lazy devtools initialization helper into a static method of the ExtensionAPI.

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

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8969733 [details]
Bug 1451795 - Move lazy devtools initialization helper into a static method of the ExtensionAPI.

https://reviewboard.mozilla.org/r/238544/#review246742

This looks great!

::: browser/components/extensions/parent/ext-devtools.js:320
(Diff revision 3)
> -// Get the devtools preference given the extension id.
> -function getDevToolsPrefBranchName(extensionId) {
> -  return `devtools.webextensions.${extensionId}`;
> +this.devtools = class extends ExtensionAPI {
> +  constructor(extension) {
> +    super(extension);
> +
> +    // DevToolsPageDefinition instance (created in onManifestEntry).
> +    this.devtoolsPageDefinition = null;

nit: Since you're inside the devtools api, I think it would be clear to just call this something like pageDefinition and everything would be a little more concise.  This is a matter of taste though, feel free to leave it if you prefer it as is.

Comment 5

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

Comment on attachment 8969733 [details]
Bug 1451795 - Move lazy devtools initialization helper into a static method of the ExtensionAPI.

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

Comment 6

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

Comment on attachment 8969733 [details]
Bug 1451795 - Move lazy devtools initialization helper into a static method of the ExtensionAPI.

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

Comment 7

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

(In reply to Andrew Swan [:aswan] from comment #4)
> nit: Since you're inside the devtools api, I think it would be clear to just
> call this something like pageDefinition and everything would be a little
> more concise.  This is a matter of taste though, feel free to leave it if
> you prefer it as is.

Actually, I totally agree.

Change applied (and patch rebased on a recent mozilla-central tip).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e55646aad4a
Move lazy devtools initialization helper into a static method of the ExtensionAPI. r=aswan

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/6e55646aad4a