Closed Bug 1285557 Opened 8 years ago Closed 8 years ago

Create a WebExtensionActor based on the ChromeActor and TabActor

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(4 files)

Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.
58 bytes, text/x-review-board-request
ochameau
: review+
jryans
: review+
Details
Bug 1285557 - Fix eslint errors on webbrowser devtools actor.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1285557 - eslint should lint the selected devtools actors.
58 bytes, text/x-review-board-request
jryans
: review+
Details 
The goal of this issue is the creation of a new Addon Actor for the WebExtensions add-ons, this new actor is going to inherit most of its features from ChromeActor (which inherits most of its own from TabActor) and on top of them it applies add-on specific features (e.g. filter visible sources and the listed frames, handle the addon reload etc.) 

Given that all the contexts that compose a WebExtensions add-on are provided with their own window and their own HTML documents (besides the content scripts which runs in a sandbox paired with the target content window), ChromeActor/TabActor are a perfect match and gives us the ability of providing all the existent DevTools panels and features (e.g. the DOM inspector, the Picker tool, the Style panel, the Storage panel etc.)   

The patches attached to this issue are the core of the proposed changes to achieve the above goal.
Depends on: 1285556
Depends on: 1268773
Blocks: 1226743
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

This patch contains the core of the proposed changes, pushed for an initial feedback.

It currently depends from the patches attached to the following bugs:

- Bug 1285556 (for the isWebExtension getter on the AddonWrapper)
- Bug 1268773 (for issues related to the addon reload, which depends from the patches attached to Bug 1285289 for the new addTemporaryAddon test helper)

On top of this patch, I've a bunch of other patches related to further tweaks and fixes to propose/discuss (that I'm going to push in separate bugzilla issues):

- Fix (webextension specific): ignore the background pages in the Highlighters' markup helper (because accessing the special anonymous content in that context, that is loaded inside a browser tag in a browserless window, raises exceptions and it is not needed because that context is never visible)

- Follow up (webextension specific): Enable the picker button for the WebExtensionAddonActor

- Fix (general, exists on the browser toolbox too): reset the `_lastConsoleInputEvaluationResult` when the debugger changes, to prevent "Debugger.Object belongs to a different Debugger" exceptions to be raised in evalWithDebugger (because of the `$_` binding)

- Fix (probably general, it seems to happen on the browser toolbox too): listed sources not updated on switch to a different frame
Attachment #8769213 - Flags: feedback?(poirot.alex)
Follows a link to a google document which contains a description of the proposal and a summary of the issues that compose this proposed feature:

- https://docs.google.com/document/d/1-irEoJ5Ri6MKG708Q_WvyaOaDfyYoHjg4cR6-Fvj0wk
https://reviewboard.mozilla.org/r/63180/#review60306

Looks good overall!
Do not hesitate to introduce small tweaks to TabActor to better support your new actor.

::: devtools/client/framework/attach-thread.js:103
(Diff revision 1)
> -    // Attaching a normal thread
> +    // Attaching a tab, a browser process or a WebExtensions add-on.
>      target.activeTab.attachThread(threadOptions, handleResponse);
>    } else {
> -    // Attaching the browser debugger
> +    // Attaching an old browser debugger or a content process.
>      target.client.attachThread(chromeDebugger, handleResponse);
>    }

I don't think you need this target.isWebExtension check.
I imagine:
if (target.isTabActor) {

} else if (target.isAddon) {

} else {
  
}
Should work?

::: devtools/server/actors/webextension.js:67
(Diff revision 1)
> -
> -  // Defines the default docshell selected for the tab actor
> -  let window = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> +  // TODO: destroy the chromeWebNav, if any.
> +  let chromeWebNav = Services.appShell.createWindowlessBrowser(true);
> +  this.chromeWebNav = chromeWebNav;
> +  this.fallbackDocShell = chromeWebNav.QueryInterface(Ci.nsIInterfaceRequestor)
> +    .getInterface(Ci.nsIDocShell);
> +  chromeWebNav.loadURI("about:blank", 0, null, null, null);

Can't you fetch the hidden window?
I understand it won't necessarely be available,
but can't you at least try to fetch it if it's already around?

::: devtools/server/actors/webextension.js:80
(Diff revision 1)
>  }
> -exports.ChromeActor = ChromeActor;
> +exports.WebExtensionAddonActor = WebExtensionAddonActor;
> +
> +WebExtensionAddonActor.prototype = Object.create(ChromeActor.prototype);
> +
> +update(WebExtensionAddonActor.prototype, {

I'm not sure `update()` brings much value here and in requestTypes. It mosly make this particular actor looks different from all others.

::: devtools/server/actors/webextension.js:111
(Diff revision 1)
>  
> -ChromeActor.prototype = Object.create(TabActor.prototype);
> +  onAttach: function () {
> +    let res = ChromeActor.prototype.onAttach.apply(this, arguments);
>  
> -ChromeActor.prototype.constructor = ChromeActor;
> +    // TODO: the console and error messages, network requests, storages etc.
> +    // should be filtered by addonId.

Oh. I haven't thought about that. That may be a significant work? Or may be it's mostly the console?
Everything should work fine if you are expecting things from the document you are currently selecting.
It will be harder if you want console from any addon document to appear in the addon console.

I don't know if you saw that, but most tools filters by frame hierarchy. That means that, by default, you will see all console messages or scripts in the debugger for all documents living in your tab. Everything from the top level document or any iframes within it, and everything from iframes's of iframes and so on.

::: devtools/server/actors/webextension.js:141
(Diff revision 1)
> +   */
> +  get sources() {
> +    if (!this._sources) {
> +      this._sources = new TabSources(this.threadActor, (source) => {
> +        return WebExtensionAddonActor.prototype._allowSource.call(this, source);
> +      });

Instead of overloading sources, you may tweak webbrowser.js to accept passing a customized filter function passed to TabSources from a derived class.

::: devtools/server/actors/webextension.js:217
(Diff revision 1)
> -  }
> +    }
> -};
>  
> -ChromeActor.prototype._detach = function () {
> -  if (!this.attached) {
> -    return false;
> +    if (this.attached) {
> +       this.onDetach();
> +       this.conn.send({ from: this.actorID, type: "tabDetached" });

Note that I do have a patch to simplify this "tabDetached" event. bug 1281726.

::: devtools/server/actors/webextension.js:220
(Diff revision 1)
> -ChromeActor.prototype._detach = function () {
> -  if (!this.attached) {
> -    return false;
> +    if (this.attached) {
> +       this.onDetach();
> +       this.conn.send({ from: this.actorID, type: "tabDetached" });
> -  }
> +    }
>  
> -  Services.obs.removeObserver(this, "chrome-webnavigation-create");
> +    this.disconnect();

I think you want to call this.exit() here.

::: devtools/server/actors/webextension.js:282
(Diff revision 1)
>  
> -/* ThreadActor hooks. */
> +      if (addonID == this.id) {
> +        return true;
> +      }
> +
> +      return false;

`return addonID == this.id`

::: devtools/server/actors/webextension.js:322
(Diff revision 1)
> -  let e = Services.wm.getEnumerator(null);
> -  while (e.hasMoreElements()) {
> -    let win = e.getNext();
> -    let windowUtils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> -                         .getInterface(Ci.nsIDOMWindowUtils);
> -    windowUtils.resumeTimeouts();
> +    this.conn.send({
> +      from: this.actorID,
> +      type: "frameUpdate",
> +      frames: windows.filter(frame => {
> +        return frame.addonID == this.id;
> +      }),

You have to filter first before sending the message, otherwise you are going to notify with an ampty `frames` array.
Also, I'm not sure you have to override this method.
You may only override `_docShellsToWindows()`
to make it return only windows related to the targetted addon. Then tweak webbrowser.js to dispatch a frameUpdate if windows isn't empty.
Severity: normal → enhancement
Depends on: 1286526
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

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

> I don't think you need this target.isWebExtension check.
> I imagine:
> if (target.isTabActor) {
> 
> } else if (target.isAddon) {
> 
> } else {
>   
> }
> Should work?

sure, I initially did this way and I changed it to make it more explicit, but I'm totally ok to use this version instead.

fixed in the updated version.

> Can't you fetch the hidden window?
> I understand it won't necessarely be available,
> but can't you at least try to fetch it if it's already around?

Yep, and I added it in the updated version (based on the work on Bug 1268773), and the devtools are switched to it if it exists,
but I'm still creating the fallback window for two reasons:

- to have a fallback context to switch to if the addon has been disabled and then re-enabled.
- to have a fallback when the background page doesn't exist.

> I'm not sure `update()` brings much value here and in requestTypes. It mosly make this particular actor looks different from all others.

sounds good, in the updated version it has been removed and I changed the prototype methods overriding to looks like the other actors, but I'm using Object.assign to make the "override the requestTypes definition" less verbose (I'm ok with removing Object.assign usage if we prefer)

> Oh. I haven't thought about that. That may be a significant work? Or may be it's mostly the console?
> Everything should work fine if you are expecting things from the document you are currently selecting.
> It will be harder if you want console from any addon document to appear in the addon console.
> 
> I don't know if you saw that, but most tools filters by frame hierarchy. That means that, by default, you will see all console messages or scripts in the debugger for all documents living in your tab. Everything from the top level document or any iframes within it, and everything from iframes's of iframes and so on.

In the updated version, I'm applying the filter to the console messages, but I'm waiting to filter the errors (and for this reason isRootActor is currently set to true on this actor) because most of the errors raised by the WebExtensions internals are currently not associated to any window and the console actor will filter out important informations (e.g. manifest errors, or the runtime.lastError when it is not checked, exception that are catched when listeners registered to the WebExtensions API are called).

I'll change it (and I'll look at how to filter the other panels) in follow ups, if it sounds ok from your point of view.

> Instead of overloading sources, you may tweak webbrowser.js to accept passing a customized filter function passed to TabSources from a derived class.

I like the idea, in the updated version I've tweaked webbrowser.js to optionally use `this._allowSource` as a tab sources filter if defined (and pushed it to try to check if it breaks any of the existent tests)

> Note that I do have a patch to simplify this "tabDetached" event. bug 1281726.

Added a comment (on both the addon actors, because currently they are both using this approach), so that I can update it once that patch is landed.

> I think you want to call this.exit() here.

I'm not actually sure of what is the differences between disconnect and exit :-(

Currently I'm using the same cleanup approach that is currently used in the existent addon actor:

- the addon actor is an addon listener and it calls this.disconnect() on itself when it is uninstalled
- disconnect is implemented by this actor and it calls the parent disconnect and 
- the parent disconnect calls this.exit (that is implemented by the tab actor)
- after the parent disconnect has been executed, it cleanups its own properties (e.g. it closes the fallback browserless window and it unregister itself as a listener of the AddonManager events)

> You have to filter first before sending the message, otherwise you are going to notify with an ampty `frames` array.
> Also, I'm not sure you have to override this method.
> You may only override `_docShellsToWindows()`
> to make it return only windows related to the targetted addon. Then tweak webbrowser.js to dispatch a frameUpdate if windows isn't empty.

yep, I would prefer it too, I applied the above suggestions in the updated patch.
In the last push of the attached patches, besides the updated version of the first patch, I added a new patch which introduce the first of the new tests:

https://reviewboard.mozilla.org/r/63930/diff/1#index_header

The new patch adds a new high level test in the about:debugging tests, which install a webextension, click the debug button and checks that the opened toolbox's webconsole execute the function defined in the test addon background page.
Attachment #8770523 - Flags: review?(poirot.alex) → review+
Comment on attachment 8770523 [details]
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.

https://reviewboard.mozilla.org/r/63930/#review60992

::: devtools/client/aboutdebugging/test/head.js:153
(Diff revision 1)
>  function getTabList(document) {
>    return document.querySelector("#tabs .target-list") ||
>      document.querySelector("#tabs.targets");
>  }
>  
> -function* installAddon(document, path, name, evt) {
> +function* installAddon({document, addonPath, addonName, isWebExtension}) {

There is no need to prefix everything by "addon", this method is already calling `installAddon` and makes it clear all this is related to addons.

::: devtools/client/aboutdebugging/test/head.js:175
(Diff revision 1)
> +        }
> +
> +        Management.off("startup", listener);
> +        done();
> +      });
> +    });

I imagine this is no `once()` method on Management?

::: devtools/client/aboutdebugging/test/head.js:200
(Diff revision 1)
>    names = names.map(element => element.textContent);
> -  ok(names.includes(name),
> +  ok(names.includes(addonName),
>      "The addon name appears in the list of addons: " + names);
>  }
>  
> -function* uninstallAddon(document, addonId, addonName) {
> +function* uninstallAddon({document, addonId, addonName}) {

Same comment here about addon prefix.
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

https://reviewboard.mozilla.org/r/63180/#review60986

Here is another set of comments, thanks for addressing everything!
I won't have time for another review, but I think you can ask :jryans to do the final review while I'm off.

::: devtools/server/actors/webextension.js:73
(Diff revision 2)
> -  this.listenForNewDocShells = true;
> +  // not defined for the target add-on or not yet when the actor instance has been created).
> +  let chromeWebNav = Services.appShell.createWindowlessBrowser(true);
> +  this.chromeWebNav = chromeWebNav;
> +  this.fallbackDocShell = chromeWebNav.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                      .getInterface(Ci.nsIDocShell);
> +  let baseURI = Services.io.newURI("about:blank", null, null);

It would be great to have feedback from your team about that fallback document, as it can easily be misleading.
For example, shouldn't we load a document saying what it is?
Something like "data:text/html,You addon doesn't has any document opened yet.".
Or make some tweaks in the client to detect that we are on the fallback document and display some hints to say there is no doc? Or disable document tools when there is no read document?

::: devtools/server/actors/webextension.js:77
(Diff revision 2)
> +                                      .getInterface(Ci.nsIDocShell);
> +  let baseURI = Services.io.newURI("about:blank", null, null);
> +  let originAttributes = {addonId: this.id};
> +  let addonPrincipal = Services.scriptSecurityManager
> +                               .createCodebasePrincipal(baseURI, originAttributes);
> +  this.fallbackDocShell.createAboutBlankContentViewer(addonPrincipal);

Can you try to create the fallbackDocshell lazily?
So that it will only be created if `_findAddonPreferedTargetWindow()` failed to retrieve a window.

::: devtools/server/actors/webextension.js:99
(Diff revision 2)
> +WebExtensionAddonActor.prototype.constructor = WebExtensionAddonActor;
> +
> +// NOTE: This is needed to catch in the webextension webconsole all the
> +// errors raised by the WebExtension internals that are not currently
> +// associated with any window.
> +WebExtensionAddonActor.prototype.isRootActor = true;

See bug 1266561. It may be a great time to finally fix this magical isRootActor flag.

::: devtools/server/actors/webextension.js:128
(Diff revision 2)
> +  // TODO: the console and error messages, network requests, storages etc.
> +  // should be filtered by addonId.
> +  if (this.preferredTargetWindow) {
> +    DevToolsUtils.executeSoon(() => {
> +      this._changeTopLevelDocument(this.preferredTargetWindow);
> +    });

Why do you change the top level on attach??
The comment seems unrelated to that.

::: devtools/server/actors/webextension.js:142
(Diff revision 2)
> +WebExtensionAddonActor.prototype.disconnect = function () {
> +  let res = ChromeActor.prototype.disconnect.apply(this, arguments);
> +  AddonManager.removeAddonListener(this);
> +
> +  if (this.chromeWebNav) {
> +    this.chromeWebNav.loadURI("about:blank", 0, null, null, null);

If we are sure we don't keep any reference to chromeWebNav, it will be garbaged collected and destroyed. May be it is safer to at least call close in case we leak it. But I don't see why we would load about:blank AND close it? Does it fixes anything?
Please comment about the why or remove unecessary calls.

::: devtools/server/actors/webextension.js:164
(Diff revision 2)
> -    // Iterate over all top-level windows and all their docshells.
> -    let docShells = [];
> -    let e = Services.ww.getWindowEnumerator();
> -    while (e.hasMoreElements()) {
> -      let window = e.getNext();
> -      let docShell = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +  return {
> +    frames: windows.filter(frame => {
> +      return frame.addonID == this.id;
> +    }),
> +  };
> +};

You shouldn't need to overload this now that you overloaded _docShellsToWindows.

::: devtools/server/actors/webextension.js:184
(Diff revision 2)
> -  TabActor.prototype.observe.call(this, aSubject, aTopic, aData);
> -  if (!this.attached) {
> + * Set the preferred global for the add-on (called from the AddonManager).
> + */
> +WebExtensionAddonActor.prototype.setOptions = function (aOptions) {
> +  if ("global" in aOptions) {
> +    // Set the proposed debug global as the preferred target window.
> +    this.preferredTargetWindow = aOptions.global;

Shouldn't you call changeTopLevelDocument from here?

::: devtools/server/actors/webextension.js:199
(Diff revision 2)
>    }
> -  if (aTopic == "chrome-webnavigation-create") {
> -    aSubject.QueryInterface(Ci.nsIDocShell);
> -    this._onDocShellCreated(aSubject);
> -  } else if (aTopic == "chrome-webnavigation-destroy") {
> -    this._onDocShellDestroy(aSubject);
> +
> +  this.addon = aAddon;
> +
> +  // TODO: propagate the update add-on metadata to the connected client.
> +};

updateAddonWrapper looks like a leftover.

::: devtools/server/actors/webextension.js:224
(Diff revision 2)
> +WebExtensionAddonActor.prototype.onInstalled = function(aAddon) {
> +  if (aAddon.id != this.id) {
> +    return;
> +  }
>  
> -  // Listen for any new/destroyed chrome docshell
> +  this.addon = aAddon;

Could be worth having the same comment than in addon.js

::: devtools/server/actors/webextension.js:263
(Diff revision 2)
> -    let docShell = window.QueryInterface(Ci.nsIInterfaceRequestor)
> -                         .getInterface(Ci.nsIWebNavigation)
> -                         .QueryInterface(Ci.nsIDocShell);
> -    if (docShell == this.docShell) {
> -      continue;
> +    let activeAddon = XPIProvider.activeAddons.get(this.id);
> +
> +    if (!activeAddon) {
> +      // the background page is going to be closed,
> +      // navigate to the original window (which is the fallback window).
> +      resolve(this._originalWindow);

Shouldn't we use this.chromeWebNav instead of relying on this TabActor attribute?
Same comment for the other usage few lines after (line 271)

::: devtools/server/actors/webextension.js:310
(Diff revision 2)
> +    }
> +
> +    let addonID = undefined;
> +    if (window.document.documentURIObject.schemeIs("moz-extension")) {
> +      addonID = window.document.nodePrincipal.originAttributes.addonId;
> +    }

Shouldn't we just use let addonID = window.document.nodePrincipal.originAttributes.addonId?
Sounds more flexible. Shouldn't we trust the originAttributes?

::: devtools/server/actors/webextension.js:351
(Diff revision 2)
> -  while (e.hasMoreElements()) {
> -    let win = e.getNext();
> -    let windowUtils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> -                         .getInterface(Ci.nsIDOMWindowUtils);
> -    windowUtils.resumeTimeouts();
> -    windowUtils.suppressEventHandling(false);
> +    // This will fail for non-Sandbox objects, hence the try-catch block.
> +    let metadata = Cu.getSandboxMetadata(global);
> +    if (metadata) {
> +      return metadata.addonID === this.id;
> +    }
> +  } catch (e) {}

Is the sandbox codepath ever going to be used for web extensions? I imagine we only have content scripts using sandboxes and that should be visible to this actor living in the parent process.

Oh but wait, there is still non-e10s mode where you will see them.
May be you could at least do the nsIDOMWindow check first as it should be true more frequently.

::: devtools/server/actors/webextension.js:355
(Diff revision 2)
> -    windowUtils.resumeTimeouts();
> -    windowUtils.suppressEventHandling(false);
> +    }
> +  } catch (e) {}
> +
> +  if (global instanceof Ci.nsIDOMWindow) {
> +    return global.document.documentURIObject.schemeIs("moz-extension") &&
> +      mapURIToAddonID(global.document.documentURIObject) == this.id;

Can we use `window.document.nodePrincipal.originAttributes.addonId == this.id` instead?

::: devtools/server/actors/webbrowser.js:1059
(Diff revision 2)
>      if (!this._sources) {
> -      this._sources = new TabSources(this.threadActor);
> +      // Optionally use `this._allowSource` as a source filter.
> +      let allowSource = this._allowSource ?
> +            this._allowSource.bind(this) : null;
> +
> +      this._sources = new TabSources(this.threadActor, allowSource);

Let the caller do bind if necessary and just do:
this._sources = new TabSources(this.threadActor, this._allowSource);

Also explicitely set `_sources` to `null` somewhere and put a comment about this attribute there.

::: devtools/server/actors/webconsole.js:601
(Diff revision 2)
>            if (!this.consoleAPIListener) {
> +            // Create the consoleAPIListener (and apply the filtering options defined
> +            // in the parent actor).
>              this.consoleAPIListener =
> -              new ConsoleAPIListener(window, this);
> +              new ConsoleAPIListener(window, this,
> +                                     this.parentActor.consoleAPIListenerOptions);

Please set a `consoleAPIListenerOptions` attribute on TabActor with a default value and a comment documenting it.
Attachment #8769213 - Flags: review?(poirot.alex)
Assignee: nobody → lgreco
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63180/diff/2-3/
Attachment #8769213 - Flags: review?(jryans)
Attachment #8770523 - Flags: review?(jryans)
Comment on attachment 8770523 [details]
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63930/diff/1-2/
https://reviewboard.mozilla.org/r/63180/#review60986

> It would be great to have feedback from your team about that fallback document, as it can easily be misleading.
> For example, shouldn't we load a document saying what it is?
> Something like "data:text/html,You addon doesn't has any document opened yet.".
> Or make some tweaks in the client to detect that we are on the fallback document and display some hints to say there is no doc? Or disable document tools when there is no read document?

In the updated version I opted for using a browserless window that doesn't have any addon id (in any case, no APIs are injected in it without creating an ExtensionContext, which it is more complex and has more side-effects of what we probably want from a fallback page)

The fallback page is now created lazily when the actor has been attached for the first time (instead of being created when the actor instance is created, which happens even when the "about:debugging" request the list of addons, but the debugger is not really attached), and only if there isn't a background page to use as a initial target context.

I think that this approach is nice enough, at least as the initial (and simplest possible) solution/workaround, but I'd like to be more explicit about it in follow ups (e.g. logging a message in the webconsole that tells the addon developer that the current document is a fallback and it can select a frame from the frame list menu once they are available)

On webextension addon reload/upgrade, the tools are automatically switched to the preferred target context once available (when the webextension is fully started again and it has a background page to set as the preferred debug global)

In any case, I will ask to Andrew and Kris to give me a feedback about it.

> Can you try to create the fallbackDocshell lazily?
> So that it will only be created if `_findAddonPreferedTargetWindow()` failed to retrieve a window.

Yep, done in the updated version (as described in more detail in the above comment), but it is important that it is set before the TabActor.prototype._attach method is called, or the actors will not subscribe the events related to the creation/destroying of the child doc shells and it will fail to detect them (e.g. the webextension popup pages),
I added a inline comment about this in the onAttach method.

> See bug 1266561. It may be a great time to finally fix this magical isRootActor flag.

just added myself in the CC of that bug.

I want to point out that this actor needs to be identified as a root actor mostly because we currently need to catch all the errors or most of the current webextensions errors will not be visible, because they are not currently assigned to any inner window id.

This solution is not perfect (it is actually more a short term workaround than a real long term solution), but I'm pretty sure (mostly from what we are currently experiencing with the "addon debugging with Addon Debugger vs. Browser Toolbox" situation) that it is better to let the addon developers to see everything in the meantime, than forcing them to check the errors in a different tool, e.g. the Browser Console.

I've some ideas about how I would like to change it in more follow ups (and it will need some changes on the WebExtension internals side as well).

> Why do you change the top level on attach??
> The comment seems unrelated to that.

Fixed in the updated version (which set the target window, or create the fallback, in this method), added an inline comment to explain why the current context needs to be set before `ChromeActor.prototype.onAttach.apply(this, arguments);` has been called.

> If we are sure we don't keep any reference to chromeWebNav, it will be garbaged collected and destroyed. May be it is safer to at least call close in case we leak it. But I don't see why we would load about:blank AND close it? Does it fixes anything?
> Please comment about the why or remove unecessary calls.

When the windowless browser was initialized with the `createAboutBlankContentViewer`, one of the xpcshell tests was getting stuck on the debugger server cleanup, it doesn't happen if the `createAboutBlankContentViewer` is not used (as in the updated version), but I prefer to close the windowless browser explicitly on disconnect, because it is something that we currently do internally to prevent leaks (e.g. https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js#129).

> You shouldn't need to overload this now that you overloaded _docShellsToWindows.

Agree, removed in the last version.

> Shouldn't you call changeTopLevelDocument from here?

setOptions is called right after the toolbox process is started, but before the actor is actually attached.

For the above reason, this method just save, for later usage, the preferred addon debug global into this.preferredTargetWindow, if any.

> updateAddonWrapper looks like a leftover.

That's true, removed in the last version of this patch.

> Could be worth having the same comment than in addon.js

Added same inline comment added in addon.js.

> Shouldn't we use this.chromeWebNav instead of relying on this TabActor attribute?
> Same comment for the other usage few lines after (line 271)

Fixed in the last version of this patch, where any usage of this._originalWindow has been removed from this actor.

> Shouldn't we just use let addonID = window.document.nodePrincipal.originAttributes.addonId?
> Sounds more flexible. Shouldn't we trust the originAttributes?

The originAttributes is what we use internally to identify which is the addon id of a WebExtensions page, and so it should be trusted, in the last version of this patch the redudant check on the moz-extension scheme has been removed.

> Is the sandbox codepath ever going to be used for web extensions? I imagine we only have content scripts using sandboxes and that should be visible to this actor living in the parent process.
> 
> Oh but wait, there is still non-e10s mode where you will see them.
> May be you could at least do the nsIDOMWindow check first as it should be true more frequently.

Check order reversed in the last updated version of this patch.

> Can we use `window.document.nodePrincipal.originAttributes.addonId == this.id` instead?

Yep, fixed in the last version.

> Let the caller do bind if necessary and just do:
> this._sources = new TabSources(this.threadActor, this._allowSource);
> 
> Also explicitely set `_sources` to `null` somewhere and put a comment about this attribute there.

The above suggested changes have been applied to the last version of this patch.

> Please set a `consoleAPIListenerOptions` attribute on TabActor with a default value and a comment documenting it.

Fixed in the last version as suggested by the above review comment.
https://reviewboard.mozilla.org/r/63180/#review60306

> sounds good, in the updated version it has been removed and I changed the prototype methods overriding to looks like the other actors, but I'm using Object.assign to make the "override the requestTypes definition" less verbose (I'm ok with removing Object.assign usage if we prefer)

This has been fixed in the last version (and the last version doesn't use Object.assign to override the requestTypes anymore)
https://reviewboard.mozilla.org/r/63930/#review60992

> There is no need to prefix everything by "addon", this method is already calling `installAddon` and makes it clear all this is related to addons.

Agree, removed the redundant "addon" prefix from installAddon/uninstallAddon test helpers.

> I imagine this is no `once()` method on Management?

Confirmed, not yet at least.
In the last version of the second patch, I've started to add more tests related to the Addon Developer Toolbox and the WebExtensions add-ons (and I'm going to split them into multiple file, because it looks like that, at least on try, they can have timeout issues if they are all running from the same file).

Follows a summary of the test cases that I'm currently working, which are opening the developer toolbox on a simple webextension addon and once opened:

- the webconsole panel should be able to interact with the background page

- the inspector panel should be able to inspect the background page

- the inspector panel shows the fallback document if the webextension addon does not have a background page to use as the default initial target

- the nohide button and list frames can be used to switch to a webextension popup page, and once switched to that frame by selecting it from the frame list menu, the webconsole panel should be able to interact with the popup page
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63180/diff/3-4/
Comment on attachment 8770523 [details]
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63930/diff/2-3/
Pushed an update with the following changes:

- split part of the test cases added to the new "browser_addons_debug_webextension.js" test file into the following new test files:
  - browser_addons_debug_webextension.js: base test, open the toolbox and test the webconsole is attached to the background page and works correctly
  - browser_addons_debug_webextension_inspector.js: test the inspection of the WebExtension background page
  - browser_addons_debug_webextension_nobg.js: test the fallback windowless browser context created when a webextension add-on doesn't have any background page
  - browser_addons_debug_webextension_popup.js: test the "no auto hide popup" toggle button and switch the toolbox to a WebExtensions popup page from the frame list menu (and that the webconsole is attached to the popup page and works correctly)
- added a new patch with a proposed change to be able to skip the WebExtensions background page in the Highlighter markup helpers (attachment 8772849 [details])

To get the inspector to work correctly on a WebExtensions background page, the Highlighter markup helpers have to identify and skip the WebExtensions bakground pages (because inserting the anonymous nodes used by the highlighter currently raises an exception on the background page and prevent the inspector to work correctly).

the test file browser_addons_debug_webextension_inspector.js verifies that the above issue can be fixed with the proposed fix added (as the new attachment 8772849 [details]), and the test case shows how to programmatically reproduce the issue when no fix is not present in the highlighter helpers.
Push to try (still running) of the last version of these patches (including the patches from the bugzilla issues linked as dependencies of this one):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=70cb7be48bf2
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

https://reviewboard.mozilla.org/r/63180/#review63024

Overall, I think it's coming together well, but a few changes I'd still like to see before r+.

Thanks for working on this!

::: devtools/client/framework/attach-thread.js:93
(Diff revision 4)
>        deferred.resolve(threadClient);
>      });
>    };
>  
> -  if (target.isAddon) {
> -    // Attaching an addon
> +  if (target.isTabActor) {
> +    // Attaching a tab, a browser process or a WebExtensions add-on.

Nit: comma before "or"

::: devtools/server/actors/addon.js:136
(Diff revision 4)
>      if (aAddon != this._addon) {
>        return;
>      }
>  
>      if (this.attached) {
> +      // This can be simplified once Bug 1281726 has been landed.

What does this have to do with bug 1281726?

::: devtools/server/actors/webextension.js:1
(Diff revision 4)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Since you are creating a new file, please add an ESLint ignore exclusion[1] so it will be linted (currently the actors directory is ignored).

[1]: https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/.eslintignore#118

::: devtools/server/actors/webextension.js:23
(Diff revision 4)
> +loader.lazyRequireGetter(this, "unwrapDebuggerObjectGlobal", "devtools/server/actors/script", true);
> +
> +loader.lazyImporter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
> +loader.lazyImporter(this, "XPIProvider", "resource://gre/modules/addons/XPIProvider.jsm");
> +
> +const FALLBACK_DOC_MESSAGE = "You addon does note have any document opened yet.";

"Your add-on does not have any document opened yet."

::: devtools/server/actors/webextension.js:49
(Diff revision 4)
>   * @param aConnection DebuggerServerConnection
>   *        The connection to the client.
> + * @param aAddon AddonWrapper
> + *        The target addon.
>   */
> -function ChromeActor(aConnection) {
> +function WebExtensionAddonActor(aConnection, aAddon) {

Don't use aArgs style, ESLint will tell you once you enable it for this file.

For the name, what about just `WebExtensionActor`, since they are always add-ons?

::: devtools/server/actors/webextension.js:55
(Diff revision 4)
> -  TabActor.call(this, aConnection);
> +  ChromeActor.call(this, aConnection);
> +
> +  this.id = aAddon.id;
> +  this.addon = aAddon;
>  
> -  // This creates a Debugger instance for chrome debugging all globals.
> +  // bind the _allowSource helper to this, it is used in the

Nit: Start the line comments with capitals, like "Bind..."

::: devtools/server/actors/webextension.js:96
(Diff revision 4)
> +WebExtensionAddonActor.prototype.form = function () {
> +  assert(this.actorID, "addon should have an actorID.");
> +
> +  let baseForm = ChromeActor.prototype.form.call(this);
> +
> +  let url = this.addon.sourceURI ? this.addon.sourceURI.spec : undefined;

Seems like you could inline this into the object below.

::: devtools/server/actors/webextension.js:110
(Diff revision 4)
> +    temporarilyInstalled: this.addon.temporarilyInstalled,
> +    isWebExtension: this.addon.isWebExtension,
>    });
> +};
> +
> +WebExtensionAddonActor.prototype.onAttach = function () {

ChromeActor overrides `_attach` for its details instead of the outer method `onAttach` which is left for TabActor.  Can you use this approach as well?

::: devtools/server/actors/webextension.js:123
(Diff revision 4)
> -exports.ChromeActor = ChromeActor;
>  
> -ChromeActor.prototype = Object.create(TabActor.prototype);
> +  let res = ChromeActor.prototype.onAttach.apply(this, arguments);
>  
> -ChromeActor.prototype.constructor = ChromeActor;
> +  // TODO: the console and error messages, network requests, storages etc.
> +  // should be filtered by addonId.

Is there a bug filed to do this?  Please add a bug number.

::: devtools/server/actors/webextension.js:134
(Diff revision 4)
>  
>  /**
> - * Getter for the list of all docshells in this tabActor
> + * Called when the actor is removed from the connection.
> - * @return {Array}
>   */
> -Object.defineProperty(ChromeActor.prototype, "docShells", {
> +WebExtensionAddonActor.prototype.disconnect = function () {

You should override `exit` instead of `disconnect` for final cleanup of the actor.  `exit` is called from more code paths.

::: devtools/server/actors/webextension.js:138
(Diff revision 4)
>   */
> -Object.defineProperty(ChromeActor.prototype, "docShells", {
> -  get: function () {
> -    // Iterate over all top-level windows and all their docshells.
> -    let docShells = [];
> -    let e = Services.ww.getWindowEnumerator();
> +WebExtensionAddonActor.prototype.disconnect = function () {
> +  let res = ChromeActor.prototype.disconnect.apply(this, arguments);
> +  AddonManager.removeAddonListener(this);
> +
> +  this._destroyFallbackWindow();

Since you create the fallback window during attaching, it seems like it should be destroyed during detach instead of disconnect.

Or, conversely, you should create the window in the actor's constructor if you want to destroy it during disconnect.

There are 2 init / destroy pairs for the actor:
1. constructor & disconnect
2. attach & detach

Something that's created in one of these inits should generally be cleaned up in the matching destroy.

Looking at TabActor, it seems to assume there is a window / docShell right after actor construction, so perhaps moving the fallback window creation to the constructor is best?

::: devtools/server/actors/webextension.js:172
(Diff revision 4)
> -    }
> +  }
> +};
> +
> +// AddonManagerListener callbacks.
>  
> -    return docShells;
> +WebExtensionAddonActor.prototype.onEnabled = function (aAddon) {

Seems like these can just be empty entirely?

::: devtools/server/actors/webextension.js:177
(Diff revision 4)
> +  if (aAddon != this.addon) {
> +    return;
>    }
> -});
>  
> -ChromeActor.prototype.observe = function (aSubject, aTopic, aData) {
> +  // TODO: Print a message to the console?

Either file a bug for this and mention here or remove the TODO.

::: devtools/server/actors/webextension.js:204
(Diff revision 4)
> -    return false;
> +    return;
>    }
>  
> -  TabActor.prototype._attach.call(this);
> +  if (this.attached) {
> +    // This can be simplified once Bug 1281726 has been landed.
> +    this.onDetach();

Seems like calling `exit` here is more appropriate.

Also, it will send `tabDetached` for you, so the next line is not needed.

::: devtools/server/actors/webextension.js:208
(Diff revision 4)
> +    // This can be simplified once Bug 1281726 has been landed.
> +    this.onDetach();
> +    this.conn.send({ from: this.actorID, type: "tabDetached" });
> +  }
>  
> -  // Listen for any new/destroyed chrome docshell
> +  this.disconnect();

Actually, this should also be `exit`, so it seems like all this method needs to do is call `exit` after the add-on check.

::: devtools/server/actors/webextension.js:268
(Diff revision 4)
> +
> +/**
> + * Discover the preferred debug global and switch to it if the addon has been attached.
> + */
> +WebExtensionAddonActor.prototype._findAddonPreferredTargetWindow = function() {
> +  return new Promise((resolve) => {

Don't need parens for single arg => functions:

`return new Promise(resolve => {`

::: devtools/server/actors/webextension.js:288
(Diff revision 4)
> +          }
> +
> +          resolve(targetWindow);
> +        });
> -  }
> +    }
> +  }).then((preferredTargetWindow) => {

Remove extra parens

::: devtools/server/actors/webbrowser.js:905
(Diff revision 4)
>  
>  TabActor.prototype = {
>    traits: null,
>  
> +  // Optional console API listener options (e.g. used by the WebExtensionAddonActor to
> +  // filter console messages by adonID), set to an empty (no options) object by default.

Nit: addonID
Comment on attachment 8770523 [details]
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.

https://reviewboard.mozilla.org/r/63930/#review63058
Attachment #8770523 - Flags: review?(jryans) → review+
Blocks: 1289126
Blocks: 1289125
Depends on: 1274775
Good news, Bug 1274775 (which is recently landed on fx-team) has the nice side-effect of fixing the exception raised by the highlighter helpers when they are initializing on a WebExtensions background page (which makes not anymore necessary any special handling of the WebExtensions pages, which is great!).
Review commit: https://reviewboard.mozilla.org/r/66832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66832/
Attachment #8772849 - Attachment description: Bug 1285557 - Skip WebExtensions background pages in DevTools Markup highlighers helpers. → Bug 1285557 - Fix eslint errors on webbrowser devtools actor.
Attachment #8769213 - Flags: review- → review?(jryans)
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63180/diff/4-5/
Comment on attachment 8770523 [details]
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63930/diff/3-4/
Comment on attachment 8772849 [details]
Bug 1285557 - Fix eslint errors on webbrowser devtools actor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65562/diff/1-2/
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63180/diff/5-6/
Comment on attachment 8770523 [details]
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63930/diff/4-5/
Comment on attachment 8772849 [details]
Bug 1285557 - Fix eslint errors on webbrowser devtools actor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65562/diff/2-3/
Comment on attachment 8774354 [details]
Bug 1285557 - eslint should lint the selected devtools actors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66832/diff/1-2/
https://reviewboard.mozilla.org/r/63180/#review63024

> What does this have to do with bug 1281726?

it was mentioned by Alex in one of the previous review comment, but I probably misunderstood how it was connected to this issue (in the last version of this patch the onUninstalled method has been simplified as suggested in one of the other review comments, and this comment is gone away).

> Since you are creating a new file, please add an ESLint ignore exclusion[1] so it will be linted (currently the actors directory is ignored).
> 
> [1]: https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/.eslintignore#118

I added this file to the file to "exclude it from the ignored files", but then I noticed that eslint was ignoring it even after the change.

I have fixed the eslint errors on the WebExtensionActor module in the first patch, but I added two more patches to this queue:

- the first patch fixes some eslint errors on the webbrowser actor (which seems that it was wrongly skipped by eslint until now)
- the second patch applies a change to the ignore list (so that the devtools files that are supposed to be linted are not ignored)

I've added these patches to this issue because the webbrowser actor has been changed in this Bug (and some eslint error had to, but let me know if you prefer to handle these new issues (fix the eslint ignored list  and fix the webbrowser actor eslint errors) in a follow up issue, and I will create the bugzilla issue and move the two patches there.

> Don't use aArgs style, ESLint will tell you once you enable it for this file.
> 
> For the name, what about just `WebExtensionActor`, since they are always add-ons?

eslint errors fixed (with additional notes about the eslint ignore list added to the comments above) and actor renamed to WebExtensionActor.

> Seems like you could inline this into the object below.

inlined to the object as suggested.

> ChromeActor overrides `_attach` for its details instead of the outer method `onAttach` which is left for TabActor.  Can you use this approach as well?

Yes, absolutely, the last version of the patch overrides `_attach` (which has the nice side-effect of reducing the overridden requestTypes to just 1).

> Is there a bug filed to do this?  Please add a bug number.

Filed the following new follow ups:

- Bug 1289125 - Addon Developer Toolbox should only shows the network requests of the target addon
- Bug 1289126 - Addon Developer Toolbox should alert the user when the addon has been enabled/disable/reloaded/upgraded

> You should override `exit` instead of `disconnect` for final cleanup of the actor.  `exit` is called from more code paths.

Thanks for the additional details, this was a point that I was discussing with Alex as well.

The last version of this patch overrides `exit` instead of `disconnect` as suggested.

> Since you create the fallback window during attaching, it seems like it should be destroyed during detach instead of disconnect.
> 
> Or, conversely, you should create the window in the actor's constructor if you want to destroy it during disconnect.
> 
> There are 2 init / destroy pairs for the actor:
> 1. constructor & disconnect
> 2. attach & detach
> 
> Something that's created in one of these inits should generally be cleaned up in the matching destroy.
> 
> Looking at TabActor, it seems to assume there is a window / docShell right after actor construction, so perhaps moving the fallback window creation to the constructor is best?

I would like to keep the fallback window creation during the attaching, mostly because an instance of this actor is created for every installed addon by the webbrowser actor when the "about:debugging" page is opened (and it will create a lot of not really necessary fallback windows).

The main issue of "not creating the fallback window in the constructor" that I found (at least from my digging) is that it needs to be defined before the [`TabActor.prototype._attach` method has been called](http://searchfox.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1203), or the actor wrongly assumes that it is running in a xpcshell test and the debug progress listener will not be registered and the actor does not register any listener for the new/destroyed docshells, and this is the reason why we check if we already have a background page (and we create a fallback page if it's not) in the `WebExtensionActor.prototype._attach` method before calling the related method on the base class (which is ChromeActor).

I moved the "destroy any existent fallback page" into the `WebExtensionAddon.prototype._detach` method (but I'm checking if the fallback page needs to be destroyed during the `exit` as well)

> Seems like these can just be empty entirely?

Removed (and follow up issue filed)

> Either file a bug for this and mention here or remove the TODO.

Filed as follow up (as Bug 1289126)

> Seems like calling `exit` here is more appropriate.
> 
> Also, it will send `tabDetached` for you, so the next line is not needed.

suggestion applied.

> Actually, this should also be `exit`, so it seems like all this method needs to do is call `exit` after the add-on check.

applied.
Comment on attachment 8772849 [details]
Bug 1285557 - Fix eslint errors on webbrowser devtools actor.

This patch fixes some eslint error on the webbrowser actor (probably because it was wrongly skipped by the eslint ignore list)
Attachment #8772849 - Flags: feedback?(jryans)
Comment on attachment 8774354 [details]
Bug 1285557 - eslint should lint the selected devtools actors.

This patch changes the .eslintignore file to prevent eslint from skipping all the devtools actors, even if they are added as explicitly enabled in the ignore list.
Attachment #8774354 - Flags: feedback?(jryans)
Comment on attachment 8774354 [details]
Bug 1285557 - eslint should lint the selected devtools actors.

https://reviewboard.mozilla.org/r/66832/#review63790

::: .eslintignore:114
(Diff revision 2)
>  !devtools/client/webconsole/console-commands.js
>  devtools/client/webide/**
>  !devtools/client/webide/components/webideCli.js
> -devtools/server/**
> +devtools/server/*.js
> +devtools/server/*.jsm
> +devtools/server/*/**

Huh, interesting!  I guess it appears that `**` means ESLint won't even look into subdirectories at all, even if there are ignore exlcuded files.

Rather than using `devtools/server/*/**` which is a bit mysterious, and would also ignore any new directories in server, let's explcitly list the directories.

So, something like:

```
devtools/server/*.js
devtools/server/*.jsm
<ignore exclusions for server>
devtools/server/actors/**
<ignore exclusions for actors>
devtools/server/performance/**
devtools/server/tests/**
```
Attachment #8774354 - Flags: review+
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

https://reviewboard.mozilla.org/r/63180/#review63810

A few small tweaks, but I think it looks ready to go.

Thanks for working on this!

::: devtools/server/actors/webextension.js:21
(Diff revisions 4 - 6)
>  loader.lazyRequireGetter(this, "unwrapDebuggerObjectGlobal", "devtools/server/actors/script", true);
>  
>  loader.lazyImporter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
>  loader.lazyImporter(this, "XPIProvider", "resource://gre/modules/addons/XPIProvider.jsm");
>  
> -const FALLBACK_DOC_MESSAGE = "You addon does note have any document opened yet.";
> +const FALLBACK_DOC_MESSAGE = "You addon does not have any document opened yet.";

Nit: You -> Your

::: devtools/server/actors/webextension.js:82
(Diff revisions 4 - 6)
>  }
> -exports.WebExtensionAddonActor = WebExtensionAddonActor;
> +exports.WebExtensionActor = WebExtensionActor;
>  
> -WebExtensionAddonActor.prototype = Object.create(ChromeActor.prototype);
> +WebExtensionActor.prototype = Object.create(ChromeActor.prototype);
>  
> -WebExtensionAddonActor.prototype.actorPrefix = "webext";
> +WebExtensionActor.prototype.actorPrefix = "webext";

No reason to use a short name here, so either "webextension" or "webExtension" (there's no clear case style for these).

::: devtools/server/actors/webextension.js:137
(Diff revisions 4 - 6)
>   */
> -WebExtensionAddonActor.prototype.disconnect = function () {
> +WebExtensionActor.prototype.exit = function () {
> -  let res = ChromeActor.prototype.disconnect.apply(this, arguments);
>    AddonManager.removeAddonListener(this);
>  
>    this._destroyFallbackWindow();

TabActor's `exit` calls `_detach` as well, so there should not be a need to repeat `_destroyFallbackWindow` in this `exit`.
Attachment #8769213 - Flags: review?(jryans) → review+
Comment on attachment 8769213 [details]
Bug 1285557 - Create a WebExtensionAddonActor based on ChromeActor and TabActor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63180/diff/6-7/
Comment on attachment 8770523 [details]
Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63930/diff/5-6/
Comment on attachment 8772849 [details]
Bug 1285557 - Fix eslint errors on webbrowser devtools actor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65562/diff/3-4/
Comment on attachment 8774354 [details]
Bug 1285557 - eslint should lint the selected devtools actors.

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

> Huh, interesting!  I guess it appears that `**` means ESLint won't even look into subdirectories at all, even if there are ignore exlcuded files.
> 
> Rather than using `devtools/server/*/**` which is a bit mysterious, and would also ignore any new directories in server, let's explcitly list the directories.
> 
> So, something like:
> 
> ```
> devtools/server/*.js
> devtools/server/*.jsm
> <ignore exclusions for server>
> devtools/server/actors/**
> <ignore exclusions for actors>
> devtools/server/performance/**
> devtools/server/tests/**
> ```

You should have seen my face while I was looking at the change that finally fixed the weird "eslint ignored list issue" :-)

Agree, explicitly excluding the subdirs is better to prevent new dirs to be magically excluded so easily.

Suggested change applied in the last update of this patch.
(In reply to Luca Greco [:rpl] from comment #40)
> Comment on attachment 8770523 [details]
> Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/63930/diff/5-6/

I'm not sure why reviewboard is not able to render this change properly,
my guess is that the combination of "hg cp" and changes applied to the copied source file in the same patch is not its "favorite kind of patch".

I tried to reapply this patch manually in a different hg bookmark and it doesn't seem to have any merging issue, should I dig more?

I'm thinking that adding a note about this in the comment related to the checkin-needed could be a good idea.
Flags: needinfo?(jryans)
https://reviewboard.mozilla.org/r/63180/#review63810

> Nit: You -> Your

oh my :-(... sorry about that (in my defense it was a very "test driven" typo, 'cause both the times I did the same typo in both the code and the test without the any help from "copy & paste")

> No reason to use a short name here, so either "webextension" or "webExtension" (there's no clear case style for these).

I opted for "webExtension".

> TabActor's `exit` calls `_detach` as well, so there should not be a need to repeat `_destroyFallbackWindow` in this `exit`.

Removed.
https://reviewboard.mozilla.org/r/63180/#review64062

::: devtools/server/actors/addon.js:130
(Diff revision 7)
>    onUninstalled: function BAA_onUninstalled(aAddon) {
>      if (aAddon != this._addon) {
>        return;
>      }
>  
>      if (this.attached) {
>        this.onDetach();
> +
> +      // The BrowserAddonActor is not a TabActor and it has to send
> +      // "tabDetached" directly to close the devtools toolbox window.
>        this.conn.send({ from: this.actorID, type: "tabDetached" });
>      }
>  
>      this.disconnect();
>    },

I thought that adding a comment here to explain that \`BrowserAddonActor.prototype.onUninstalled\` is different from \`WebExtensionActor.prototype.onUninstalled\` for very good reasons could be a nice thing to leave for the next one who looks at these two actors.

Highlighted to double-check that the comment content is right and clear.
(In reply to Luca Greco [:rpl] from comment #44)
> (In reply to Luca Greco [:rpl] from comment #40)
> > Comment on attachment 8770523 [details]
> > Bug 1285557 - Add a new WebExtension debug tests into about:debugging tests.
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/63930/diff/5-6/
> 
> I'm not sure why reviewboard is not able to render this change properly,
> my guess is that the combination of "hg cp" and changes applied to the
> copied source file in the same patch is not its "favorite kind of patch".
> 
> I tried to reapply this patch manually in a different hg bookmark and it
> doesn't seem to have any merging issue, should I dig more?
> 
> I'm thinking that adding a note about this in the comment related to the
> checkin-needed could be a good idea.

I believe your patch is fine.  MozReview currently doesn't behave well if you rebase your patches between revisions, which is what it seemed like you had done, since bits of other commits appeared for webbrowser.js.

Your patch itself should be okay, it's purely a bug in the reviewing tool.

For the future, it's probably best to avoid rebasing until after you've gotten r+ if possible to avoid confusion.  Hopefully MozReview can be improved to better handle this case.  See various issues under bug 1255654.
Flags: needinfo?(jryans)
I should add... if you do need to rebase during a review and you want MozReview to work reasonably, you can "close" the review request:

1. Go to review in MozReview
2. Click "Review Summary"
3. Close header tab -> Discarded

This will throw away past review comments and diffs and then the next review submission will start fresh, showing the correct patch after rebasing.
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6054c876d1e

(In this try build, the patches attached to this issue are pushed together with the minimum dependency patches from Bug 1285556, for the AddonWrapper isWebExtension getter or the minimal Addon Debugger Toolbox is going to be opened in tests and the tests will fail with timeouts in the inspector test, and Bug 1268773, for the new getDebugGlobal helper in the PrivateAddonWrapper used in the new WebExtensionActor)
Summary: Create a WebExtensionAddonActor based on the ChromeActor and TabActor → Create a WebExtensionActor based on the ChromeActor and TabActor
All the dependencies are now landed and their related bugzilla issue fixed.

Added checkin-needed to proceed with landing these patches.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a6679fb98d01
Create a WebExtensionAddonActor based on ChromeActor and TabActor. r=jryans
https://hg.mozilla.org/integration/fx-team/rev/2ccdff083889
Add a new WebExtension debug tests into about:debugging tests. r=ochameau, r=jryans
https://hg.mozilla.org/integration/fx-team/rev/dea8a60d4ba9
Fix eslint errors on webbrowser devtools actor. r=jryans
https://hg.mozilla.org/integration/fx-team/rev/527045290210
eslint should lint the selected devtools actors. r=jryans
Keywords: checkin-needed
Depends on: 1365775
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: