Closed Bug 1300588 Opened 8 years ago Closed 7 years ago

Implements the devtools.network.onNavigated API event

Categories

(WebExtensions :: Developer Tools, defect, P2)

Product:
WebExtensions
Component:
Developer Tools
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: rpl, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [devtools][devtools.network][devtools.network.onNavigated] triaged)

Attachments

(1 file)

Bug 1300588 - Implements the devtools.network.onNavigated API event,
58 bytes, text/x-review-board-request
kmag
: review+
Details 
The goal of this issue is to provide an initial subset of the devtools.network API, in particular the devtools.network.onNavigated event, that is often used by addons that wants to react to a navigation happening of the target tab (e.g. to refresh the addon devtools panel when the target tab navigates to a new url)

NOTE: This API methods should be possible without the creation of a new actor, by subscribing one of the existent Remote Debugging events sent from the tab actor.
Blocks: 1211859
Depends on: 1291737, 1300584
Priority: -- → P2
Whiteboard: [devtools][devtools.network]pdevtools.network.onNavigated] triaged
Whiteboard: [devtools][devtools.network]pdevtools.network.onNavigated] triaged → [devtools][devtools.network][devtools.network.onNavigated] triaged
Component: WebExtensions → WebExtensions: Developer tools
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

Luca, all I have put in place is the plumbing to allow the devtools.network API to function. None of the code to actually implement onNavigated is included. I am experimenting with some of that, and we can discuss it in our meeting tomorrow.
Attachment #8791043 - Flags: feedback?(lgreco)
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

The goal of this preliminary version of this patch was:
- ensuring that it is clear enough how to add additional sub-namespaces to the devtools APIs
- having a base API skeleton that could be used to discuss in more details the actual implementation of this API
- evaluating the changes planned in Bug 1291737 and Bug 1300584, to ensure that they provide the internals needed by the other pieces of the devtools APIs implementation (and discuss how to make it clearer and nicer, where needed)

As briefly discussed during the last planning meeting, this version of the patch has already reached the above initial goals and we are ready to move to the next version of this patch that is going to contain the actual implementation of this initial subset of the devtools.network API.

f+
Attachment #8791043 - Flags: feedback?(lgreco) → feedback+
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

I have implemented onNavigated and written a single test for it. Everything seems to work, but I know we should have more tests. Submitting this for more feedback from Luca.
Attachment #8791043 - Flags: feedback?(lgreco)
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

Luca, I have made all of the changes we discussed in our last meeting. The only thing missing is a test for changing into an iframe.
Attachment #8791043 - Flags: feedback?(lgreco)
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review81368

Hi Bob,
Thanks for applying all the changes that we discussed in our last meeting.

Follows some new feedback comments on the updated patch.

::: browser/components/extensions/ext-devtools-network.js:17
(Diff revision 3)
> +
> +// Map[context => observer]
> +let observerMap = new Map();
> +
> +function getObserver(context) {
> +  let _observer = observerMap.get(context);

is there any reason to name this `_observer` instead of just `observer`?

::: browser/components/extensions/ext-devtools-network.js:29
(Diff revision 3)
> +      },
> +    };
> +    EventEmitter.decorate(_observer);
> +    const toolbox = getDeveloperToolboxForContext(context);
> +    if (!toolbox || !toolbox.target.isLocalTab) {
> +      throw new Error("unable to subscribe to the tabNavigated event for the current inspected tab");

We can probably move this check at line 17, so that we don't even have to create an observer object and decorate it as an EventEmitter when we are going to raise an exception.

::: browser/components/extensions/ext-devtools-network.js:33
(Diff revision 3)
> +    if (!toolbox || !toolbox.target.isLocalTab) {
> +      throw new Error("unable to subscribe to the tabNavigated event for the current inspected tab");
> +    }
> +    getLocalTabTargetForContext(context, toolbox.target).then(clonedTarget => {
> +      clonedTarget.client.addListener("tabNavigated", _observer.onNavigated);
> +      observerMap.set(context, _observer);

how about moving this `observerMap.set(...)` to line 27 (just after the `EventEmitter.decorate(...)` line)? 

so that we can be sure that the next `devtools.network.onNavigated.addListener` calls are going to receive the some observer object even if the promise returned by `getLocalTabTargetForContext` is not yet resolved.
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review81368

> is there any reason to name this `_observer` instead of just `observer`?

No. ;)

> We can probably move this check at line 17, so that we don't even have to create an observer object and decorate it as an EventEmitter when we are going to raise an exception.

Good idea. Done.

> how about moving this `observerMap.set(...)` to line 27 (just after the `EventEmitter.decorate(...)` line)? 
> 
> so that we can be sure that the next `devtools.network.onNavigated.addListener` calls are going to receive the some observer object even if the promise returned by `getLocalTabTargetForContext` is not yet resolved.

Yes, very good point. Fixed.
I'm giving a beer to the dev that implements this. 5$ on Bountysource :) I know it's not much but I can't wait for Chrome DevTools extensions to work in Firefox also :) I might increase the amount in the future.
webextensions: --- → +
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review107120

Hi Bob,
I'm so sorry for the long time needed to ask you the final update on this patch,
but given that Bug 1291737 (Implement devtools_page and devtools.inspectedWindow.tabId) review has been completed and the attached patches have been landed today, we can now re-look at this patch and makes the final touches.

Follows some comments related to:

- some small updates related to the helpers shared by ext-devtoools.js (in particular `getDeveloperToolboxForContext` doesn't exist anymore and `getLocalTabTargetForContext` has been renamed)

- some minor nits (and some ideas related to the naming of some of the globals, which is  pretty subjective most of the times, but that worth mentioning it now)

- some additional feedback on the tests (and a proposed addition to it)

::: browser/components/extensions/ext-devtools-network.js:14
(Diff revision 4)
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +
> +const {SingletonEventManager} = ExtensionUtils;
> +
> +// Map[context => observer]
> +let observerMap = new Map();

How about renaming this from `observer` to something more specific? 
(e.g. `devToolsEventObserver` or something similar)

::: browser/components/extensions/ext-devtools-network.js:16
(Diff revision 4)
> +const {SingletonEventManager} = ExtensionUtils;
> +
> +// Map[context => observer]
> +let observerMap = new Map();
> +
> +function getObserver(context) {

Nits: How about adding some empty lines in this function body to make it more readable? e.g.

- move the `if (!toolbox || ...) { ...} ` to the head of this function (so that the `let observer` is alongside to the `if (!observer) { ... }) and add an empty line between it and the following `let observer = ...`
 
- add an empty line between `EventEmitter.decorate(...)` and `observerMap.set(...)` and between this last one and the `getDevToolsTargetForContext` call

- add an empty line between the end of the `if (!observer) { ... }` block and the final `return observer;`

::: browser/components/extensions/ext-devtools-network.js:18
(Diff revision 4)
> +// Map[context => observer]
> +let observerMap = new Map();
> +
> +function getObserver(context) {
> +  let observer = observerMap.get(context);
> +  const toolbox = getDeveloperToolboxForContext(context);

The toolbox is now stored in the context instance, and so this is going to be:

```
const toolbox = context.devToolsToolbox;
```

::: browser/components/extensions/ext-devtools-network.js:32
(Diff revision 4)
> +        }
> +      },
> +    };
> +    EventEmitter.decorate(observer);
> +    observerMap.set(context, observer);
> +    getLocalTabTargetForContext(context, toolbox.target).then(clonedTarget => {

`getLocalTabTargetForContext` is now named `getDevToolsTargetForContext` in the version that has landed on mozilla-central.

It only takes the context (because the toolbox target is retrieved automatically from the one stored in the context instance itsel)

::: browser/components/extensions/schemas/devtools_network.json:18
(Diff revision 4)
> +        "id": "Request",
> +        "type": "object",
> +        "description": "Represents a network request for a document resource (script, image and so on). See HAR Specification for reference.",
> +        "functions": [
> +          {
> +            "name": "getContent",

`getContent` is not yet supported but, given that we are already doing it for getHAR, it could be nice to add the `"async": "callback"` here as well.

(as a side note, if I recall correctly these types are not currently wrapped automatically with the schema wrappers, but anyway, when we are going to implement it, its API will have to be consistent with the general approach of supporting both the explicit callback parameter and the implicitly returned promise)

::: browser/components/extensions/test/browser/browser.ini:42
(Diff revision 4)
>  [browser_ext_contextMenus_radioGroups.js]
>  [browser_ext_contextMenus_uninstall.js]
>  [browser_ext_contextMenus_urlPatterns.js]
>  [browser_ext_currentWindow.js]
>  [browser_ext_devtools_inspectedWindow.js]
> +[browser_ext_devtools_network.js]

This change to browser.ini should be moved to browser-common.ini now.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:75
(Diff revision 4)
> +  extension.sendMessage("reload");
> +  eventCount = yield extension.awaitMessage("onNavigated-fired");
> +  is(eventCount, 2, "the expected number of events were fired");
> +
> +  // do a reload after the listener has been removed, do not expect a message to be sent
> +  extension.sendMessage("reload");

If I get it correctly this second reload is meant to test that `devtools.network.onTabNavigated.removeEventListener` has been unregistered.

but I think that it is not yet enough because there is nothing that checks that is really what is happening.

am I reading it wrong?

We can be more sure about it with some changes to this test, e.g.:

- wait that the tab has been fully reloaded by subscribing a different event (that has it's own tests and that is still there after we remove the `onNavigated` listener from the devtools_page) from the background page (e.g. we can probably choose to use `tabs.onUpdated` or `webNavigation.onCompleted` event)

- add a `test.onMessage` listener subscribed by the devtools_page that provides the current `eventCount` value

- check that the `eventCount` has not changes on the second reload (the one that happens after the `devtools.network.onNavigated.removeListener`

Let me know if the above approach makes sense to you or if I'm reading something wrong.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:77
(Diff revision 4)
> +  is(eventCount, 2, "the expected number of events were fired");
> +
> +  // do a reload after the listener has been removed, do not expect a message to be sent
> +  extension.sendMessage("reload");
> +
> +  yield gDevTools.closeToolbox(target);

In the last version of the other tests related to the devtools API that has been already landed, I added an additional `yield target.destroy()` (e.g. in the ["browser_ext_devtools_page.js" test](https://hg.mozilla.org/mozilla-central/file/tip/browser/components/extensions/test/browser/browser_ext_devtools_page.js#l79)) to explicitly destroy the target after the toolbox has been closed (given that we are creating it manually using the `TargetFactory`, it is a responsability of our test to clean it once done).
No longer depends on: 1300584
I removed Bug 1300584 from the dependencies, this patches only needs the devtools_page support for its tests, which has been already provided by Bug 1291737, and so it doesn't depend from Bug 1300584 that introduces `inspectedWindow.eval` and `inspectedWindow.reload`.
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

Alexandre, could you please take a look at this to verify that what I'm doing with the data and events from devtools makes sense? Thanks.
Attachment #8791043 - Flags: feedback?(poirot.alex)
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review107644

getDevToolsEventObserver could possibly be simplier.
Otherwise I think you want to watch tabNavigated with state == "start" to match chrome's behavior.

::: browser/components/extensions/ext-devtools-network.js:26
(Diff revision 6)
> +
> +  let observer = devToolsEventObserverMap.get(context);
> +  if (!observer) {
> +    observer = {
> +      onNavigated: function(event, data) {
> +        if (event == "tabNavigated" && data.state == "stop") {

I imagine you want to fire navigated only when state == start.

https://developer.chrome.com/extensions/devtools_network#event-onNavigated
Fired when the inspected window navigates to a new page. 

state = start is fired very early when we just heard about the location change

state = stop is fired only when the new url is fully loaded, it is fired on DOM "load" event.

::: browser/components/extensions/ext-devtools-network.js:31
(Diff revision 6)
> +        if (event == "tabNavigated" && data.state == "stop") {
> +          observer.emit("navigated", data.url);
> +        }
> +      },
> +    };
> +    EventEmitter.decorate(observer);

This code is hard to process. It would be easier to read by not making observer a multi-purpose object:

observer = new EventEmitter();
let onNavigated = function (event, data) {
  if (...) {
    observer.emit(...)
  }
}
...
cloneTarget.client.addListener("tabNavigated", onNavigated);

::: browser/components/extensions/ext-devtools-network.js:43
(Diff revision 6)
> +      context.callOnClose({
> +        close: () => {
> +          clonedTarget.client.removeListener("tabNavigated", observer.onNavigated);
> +          devToolsEventObserverMap.delete(context);
> +        },
> +      });

nit: Instead of caching, it may be better to spawn multiple listeners on tabNavigated rather than caching...
1/ this code would be much simplier. Easier to maitain.
2/ we wouldn't keep listening until end of context I imagine is most cases it would be firefox shutdown?? Instead we would unregister the tabNavigated listener from line 61, once the addon explicitely stop listening.

Consider that as a nit as it isn't that important.
Attachment #8791043 - Flags: review+
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review107644

Thanks for the feedback, Alexandre. It is very helpful.
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review107854

::: browser/components/extensions/ext-devtools-network.js:27
(Diff revision 7)
> +      observer.emit("navigated", data.url);
> +    }
> +  };
> +
> +  getDevToolsTargetForContext(context).then(clonedTarget => {
> +    clonedTarget.client.addListener("tabNavigated", onNavigated);

Actually I just remind the "will-navigate" event on target object. It is exactly equivalent to what you do here.
So here, you can do: 
  let onNavigated = function (event) {
    observer.emit("navigated", event.url);
  };
  cloneTarget.on("will-navigate", onNavigated);
  ...
  cloneTarget.off("will-navigate", onNavigated);

::: browser/components/extensions/ext-devtools-network.js:51
(Diff revision 7)
> +            context.runSafe(fire, url);
> +          };
> +
> +          getDevToolsEventObserver(context).on("navigated", listener);
> +          return () => {
> +            getDevToolsEventObserver(context).off("navigated", listener);

Here you may want to also cleanup the object and unregister the tabNavigated/will-navigate listener.
Attachment #8791043 - Flags: review+
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review111732

::: browser/components/extensions/ext-devtools-network.js:5
(Diff revision 8)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* global getDevToolsTargetForContext */

This should be added to `.eslintrc.js`

::: browser/components/extensions/ext-devtools-network.js:11
(Diff revision 8)
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +
> +const {SingletonEventManager} = ExtensionUtils;

Please split this destructuring into multiple lines as we do elsewhere.

::: browser/components/extensions/ext-devtools-network.js:13
(Diff revision 8)
> +
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +
> +const {SingletonEventManager} = ExtensionUtils;
> +
> +function getDevToolsEventObserver(context) {

Shorter function name would be nice. Also a doc comment.

::: browser/components/extensions/ext-devtools-network.js:16
(Diff revision 8)
> +const {SingletonEventManager} = ExtensionUtils;
> +
> +function getDevToolsEventObserver(context) {
> +  const toolbox = context.devToolsToolbox;
> +  if (!toolbox || !toolbox.target.isLocalTab) {
> +    throw new Error("Unable to subscribe to the tabNavigated event for the current inspected tab.");

Please use an ExtensionError for this.

::: browser/components/extensions/ext-devtools-network.js:19
(Diff revision 8)
> +  let observer = new EventEmitter();
> +  let onNavigated = (event, data) => {
> +    observer.emit("navigated", data.url);
> +  };
> +  observer.cleanUp = listener => {
> +    observer.off("navigated", listener);
> +    getDevToolsTargetForContext(context).then(clonedTarget => {
> +      clonedTarget.off("will-navigate", onNavigated);
> +    });
> +  };
> +
> +  getDevToolsTargetForContext(context).then(clonedTarget => {
> +    clonedTarget.on("will-navigate", onNavigated);
> +
> +    // Remove the listener when the context is closed.
> +    context.callOnClose({
> +      close: () => {
> +        clonedTarget.off("will-navigate", onNavigated);
> +      },
> +    });
> +  });
> +

All of this seems like quite a lot of unnecessary overhead. Why not just return the promise for the target and add/remove the listeners directly from the event manager?

::: browser/components/extensions/ext-devtools-network.js:23
(Diff revision 8)
> +
> +  let observer = new EventEmitter();
> +  let onNavigated = (event, data) => {
> +    observer.emit("navigated", data.url);
> +  };
> +  observer.cleanUp = listener => {

`s/cleanUp/cleanup/` please.

::: browser/components/extensions/ext-devtools-network.js:25
(Diff revision 8)
> +  let onNavigated = (event, data) => {
> +    observer.emit("navigated", data.url);
> +  };
> +  observer.cleanUp = listener => {
> +    observer.off("navigated", listener);
> +    getDevToolsTargetForContext(context).then(clonedTarget => {

Please call this once and store the promise rather than calling it once at startup and once at cleanup.

::: browser/components/extensions/ext-devtools-network.js:33
(Diff revision 8)
> +    // Remove the listener when the context is closed.
> +    context.callOnClose({
> +      close: () => {
> +        clonedTarget.off("will-navigate", onNavigated);
> +      },
> +    });

This isn't necessary. The event listener already handles this by calling `.cleanUp`

::: browser/components/extensions/ext-devtools-network.js:50
(Diff revision 8)
> +  return {
> +    devtools: {
> +      network: {
> +        onNavigated: new SingletonEventManager(context, "devtools.onNavigated", fire => {
> +          let listener = (event, url) => {
> +            context.runSafe(fire, url);

This needs to be changed to `fire.async(url)`

::: browser/components/extensions/ext-devtools-network.js:53
(Diff revision 8)
> +        onNavigated: new SingletonEventManager(context, "devtools.onNavigated", fire => {
> +          let listener = (event, url) => {
> +            context.runSafe(fire, url);
> +          };
> +
> +          getDevToolsEventObserver(context).on("navigated", listener);

Please store the result of `getDevToolsEventObserver` rather than calling it again on cleanup. Especially since the second call will return a completely different object.

::: browser/components/extensions/schemas/devtools_network.json:19
(Diff revision 8)
> +        "type": "object",
> +        "description": "Represents a network request for a document resource (script, image and so on). See HAR Specification for reference.",
> +        "functions": [
> +          {
> +            "name": "getContent",
> +            "unsupported": true,

The entire type is unsupported, so please move this property to the type itself.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:5
(Diff revision 8)
> +XPCOMUtils.defineLazyModuleGetter(this, "gDevTools",
> +                                  "resource://devtools/client/framework/gDevTools.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "devtools",
> +                                  "resource://devtools/shared/Loader.jsm");

Please keep imports sorted.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:16
(Diff revision 8)
> +      if (msg == "navigate") {
> +        code = "window.location.href = 'http://example.com/';";
> +        browser.tabs.executeScript({code});
> +      } else if (msg == "reload") {
> +        code = "window.location.reload(true);";
> +        browser.tabs.executeScript({code});
> +      }

We should probably use `window.wrappedJSObject.location` for these.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:37
(Diff revision 8)
> +  function devtools_page() {
> +    let eventCount = 0;
> +    let listener = url => {
> +      eventCount++;
> +      browser.test.assertEq("http://example.com/", url, "onNavigated received the expected url.");
> +      if (eventCount == 2) {

Nit: s/==/===/

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:44
(Diff revision 8)
> +      }
> +      browser.test.sendMessage("onNavigatedFired", eventCount);
> +    };
> +    browser.devtools.network.onNavigated.addListener(listener);
> +    browser.test.onMessage.addListener(msg => {
> +      if (msg == "getCount") {

Nit: s/==/===/

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:58
(Diff revision 8)
> +      <html>
> +       <head>
> +         <meta charset="utf-8">
> +       </head>
> +       <body>
> +         <script src="devtools_page.js"></script>
> +       </body>
> +      </html>`,

Nit: Please increase indentation level.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:63
(Diff revision 8)
> +      <html>
> +       <head>
> +         <meta charset="utf-8">
> +       </head>
> +       <body>
> +         <script src="devtools_page.js"></script>

Please move to <head>.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:94
(Diff revision 8)
> +  extension.sendMessage("reload");
> +  await extension.awaitMessage("tabUpdated");
> +
> +  extension.sendMessage("getCount");
> +  eventCount = await extension.awaitMessage("onNavigatedFiredCount");
> +  is(eventCount, 2, "The expected number of events were fired.");

There's no point in checking this here. You remove the listener when the event count hits two, so it will never go higher.
Attachment #8791043 - Flags: review?(kmaglione+bmo)
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review111732

> All of this seems like quite a lot of unnecessary overhead. Why not just return the promise for the target and add/remove the listeners directly from the event manager?

Good suggestion! This cleans up a lot of the code and ends up removing a bunch of stuff that you opened issues for, so I am marking all of those as fixed as well.

> The entire type is unsupported, so please move this property to the type itself.

It doesn't seem like we can mark a type as `unsupported`. I tried moving the `"unsupported": true,` into the type definition but that yielded the following error:

[JavaScript Error: "Internal error: Namespace devtools.network has invalid type property "unsupported" in type "Request"" {file: "resource://gre/modules/Schemas.jsm" line: 716}]

Is that a bug?

> Nit: s/==/===/

I have changed this, but out of interest, why? I thought I had discussed this with aswan before and he suggested we always use `==` unless `===` is necessary. Are you suggesting that the opposite is true, or is there another reason for wanting to use `===` explicitly in this case?

> Nit: s/==/===/

As above, there are numerous places in this test where I have used `==` that are similar to this line. Are you suggesting that I change all uses of `==` to `===` (where that would work) or is there something specific about this one? Looking through our code I see far more uses of `==` than `===`, so I am confused as to what is preferred.

> There's no point in checking this here. You remove the listener when the event count hits two, so it will never go higher.

I think the point of this was to check that removing the listener actually works.
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review111732

> It doesn't seem like we can mark a type as `unsupported`. I tried moving the `"unsupported": true,` into the type definition but that yielded the following error:
> 
> [JavaScript Error: "Internal error: Namespace devtools.network has invalid type property "unsupported" in type "Request"" {file: "resource://gre/modules/Schemas.jsm" line: 716}]
> 
> Is that a bug?

Probably. But we don't actually use the type, so you may as well just remove the "unsupported" annotation entirely.

> I have changed this, but out of interest, why? I thought I had discussed this with aswan before and he suggested we always use `==` unless `===` is necessary. Are you suggesting that the opposite is true, or is there another reason for wanting to use `===` explicitly in this case?

WebExtension code prefers strict equality operators. Most of the rest of toolkit code prefers `==`.

> I think the point of this was to check that removing the listener actually works.

The SingletonEventManager implementation already guarantees that events are not fired after a listener is removed. That behavior is already tested elsewhere.
Comment on attachment 8791043 [details]
Bug 1300588 - Implements the devtools.network.onNavigated API event,

https://reviewboard.mozilla.org/r/78600/#review112122

r=me with remaining issues addressed.

::: browser/components/extensions/ext-devtools-network.js:27
(Diff revision 10)
> +          return () => {
> +            // getDevToolsTargetForContext above could have rejected.
> +            if (devToolsTarget) {
> +              devToolsTarget.off("will-navigate", listener);
> +            }
> +          };

There's a race here. The listener will be added and never removed if the extension adds and removes a listener in quick succession. Please just store the promise, and remove the listener from a promise handler in the cleanup function.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:43
(Diff revision 10)
> +    browser.test.onMessage.addListener(msg => {
> +      if (msg == "getCount") {
> +        browser.test.sendMessage("onNavigatedFiredCount", eventCount);
> +      }
> +    });

Still not necessary.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:88
(Diff revision 10)
> +  // do a reload after the listener has been removed, do not expect a message to be sent
> +  extension.sendMessage("reload");
> +  await extension.awaitMessage("tabUpdated");
> +
> +  extension.sendMessage("getCount");
> +  eventCount = await extension.awaitMessage("onNavigatedFiredCount");
> +  is(eventCount, 2, "The expected number of events were fired.");

Not necessary.
Attachment #8791043 - Flags: review?(kmaglione+bmo) → review+
This looks good on try. Requesting check-in.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6de1955b65e
Implements the devtools.network.onNavigated API event, r=kmag,ochameau
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b6de1955b65e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: