Closed Bug 1436665 Opened 6 years ago Closed 6 years ago

WE: onRequestFinished event should be sent even if the Netmonitor UI isn't initialized

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: Honza, Assigned: Honza, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 3 obsolete files)

This is a follow-up for bug 1311171.

onRequestFinished event is currently sent only if the Network panel UI is initialized (ie the panel is selected at least once). This requirement should be removed and the HTTP tracking back-end should not depend on the UI.

First step towards this is fixing bug Bug 1416748 (or at least analysis should be done).

Honza
Priority: -- → P2
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Work split into three parts:

#1) Introduce Net monitor app object
* The app object can be disconnected from the UI
* The app object can be utilized by WebExtensions API

#2) Use separate connection for HAR exporter
* HAR exporter disables actions in connector and so needs its own connection

#3) Use Net monitor app object for WebExtension API

---

Net monitor app object created for WebExtension API isn't connected to the UI (net panel) since it doesn't have to exist at the time when extensions are using this API. We can also disconnect it from the store to make the HAR export process even faster. This is covered by bug 1445617

Honza
Comment on attachment 8958783 [details]
Bug 1436665 - Expose Net panel API without the UI;

https://reviewboard.mozilla.org/r/227688/#review234690

Sorry again for the delay in review.

I started writing various review comments, but before going into the details, I was wondering why you choose to pull off NetMonitorApp? I thought that you would only need a Connector instance.
There is many things that you don't need in NetMonitorApp, like all the code you prefix with `if (this.document)`.
Also, I imagine we could simplify NetMonitorApp by moving all WebExtension code to a class specific to "panel-less" connector. Like `onRequestAdded`, `getHarExportConnector`, `fetchResponseContent`, `add/remove/hasRequestFinishedListener`.
I'm not sure, but would such class fits HarExporter and WebExt needs?
```
function PanelLessStore() {
  // Configure store/state object.
  this.connector = new Connector();
  this.store = configureStore(this.connector);
}
PanelLessStore.prototype = {
  bootstrap: function(toolbox) {
    // Initialize connection to the backend.
    const connection = {
      tabConnection: {
        tabTarget: toolbox.target,
      },
      toolbox,
      panel: null,
      owner: null,
    };
    return this.connector.connectFirefox(connection, null, this.store.getState);
  },
  onRequestAdded: ..., 
  getHarExportConnector: ...,
  fetchResponseContent: ...,
  add/remove/hasRequestFinishedListener: ...,
}
```
And you would use that from toolbox instead of NetMonitorApp. Would that work?
If that helps you may make this PanelLessStore class be a super class for NetMonitorApp if you want to keep all WebExt API on NetMonitorApp object. (PanelLessStore isn't a perfect name...)

::: commit-message-c56ef:3
(Diff revision 2)
> +Bug 1436665 - Introduce Net monitor app object; r=ochameau
> +* The app object can be disconnected from the UI
> +* The app object can be utilized by WebExtensions API

s/utilized/used/ ?

::: devtools/client/netmonitor/src/app.js:41
(Diff revision 2)
> +}
> +
> +NetMonitorApp.prototype = {
> +  bootstrap({ toolbox, panel, window}) {
> +    this.window = window;
> +    this.document = this.window ? this.window.document : null;

We don't use window in this class, so either:
- directly pass document as bootstrap argument:
  bootstrap({ toolbox, panel, document}) {
    this.document = document;
- or only store document on this:
  bootstrap({ toolbox, panel, window}) {
    this.document = window ? window.document : null;

::: devtools/client/netmonitor/src/app.js:50
(Diff revision 2)
> +      tabConnection: {
> +        tabTarget: toolbox.target,
> +      },
> +      toolbox,
> +      panel,
> +      owner: this,

I'm wondering if that would be clearer to name this attribute `app` rather than `owner`. With `app` would will more easily understand that refers to `NetMonitorApp`.

::: devtools/client/netmonitor/src/connector/firefox-connector.js:391
(Diff revision 2)
> +    }
> +
> +    // Consumed mainly by tests.
> +    if (typeof window != "undefined") {
> +      window.emit(type, data);
> +    }

I don't understand why tests are not listening for events on `owner`? (i.e. `window.Netmonitor` rather than `window`)
It looks like unnecessary complexity from an obsolete reason.

So may be you can replace all calls to `this.emit(...)` by `if (this.owner) { this.owner.emit(...) }`.
Also, if `owner` can be null, you should algo guard all calls to `this.owner.on(...)` like what you do for `emit`:
`if (this.owner) { this.owner.on(...); }`

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:628
(Diff revision 2)
> +
> +    // Consumed mainly by tests.
> -  if (typeof window != "undefined") {
> +    if (typeof window != "undefined") {
> -    window.emit(type, data);
> +      window.emit(type, data);
> -  }
> +    }
> -}
> +  }

I have the same comment than FirefoxConnector.emit here.

::: devtools/client/netmonitor/src/har/har-exporter.js:132
(Diff revision 2)
>     */
>    getHar: function(options) {
> -    return this.fetchHarData(options).then(JSON.parse);
> +    return this.fetchHarData(options).then(data => {
> +      return data ? JSON.parse(data) : null;
> +    });
>    },

You never use async/await in any of these patches and I think you should. It would make the code more readable:
```
async getHar: function(options) {
  let data = await this.fetchHarData(options);
  if (data) {
    return JSON.parse(data);
  } else {
    return null;
  }
}
```
Attachment #8958783 - Flags: review?(poirot.alex)
Thanks for the review Alex!

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> I started writing various review comments, but before going into the
> details, I was wondering why you choose to pull off NetMonitorApp? I thought
> that you would only need a Connector instance.
Yeah, I started with that, but it turned out that extract the code
from initializer.js and intoroduce an App object is good move. 
It can contain: list of listeners (WE), connection to the backend
and helper connection used for HAR export.

And yes, the App contains even the code that needs document
(e.g. React mounting and actual rendering).

> There is many things that you don't need in NetMonitorApp, like all the code
> you prefix with `if (this.document)`.
Yes, rendering and `inspectRequest` -> that's an example of integration
with another panel (Inspector). Read more in the next comment...

> Also, I imagine we could simplify NetMonitorApp by moving all WebExtension
> code to a class specific to "panel-less" connector. Like `onRequestAdded`,
> `getHarExportConnector`, `fetchResponseContent`,
> `add/remove/hasRequestFinishedListener`.
> I'm not sure, but would such class fits HarExporter and WebExt needs?
> ```
I've been thinking the suggestion with the super class, implementing only
features independent of the panel. I was thinking about this a lot,
but didn't do it in the end. It sounds like the right direction,
but we can wait for another use case to learn more yet (e.g. for
new WebExtension API, integration with another panel, etc.)

I couldn't also find the right name for the super class,
which might indicate luck of good concept...


> function PanelLessStore() {
>   // Configure store/state object.
>   this.connector = new Connector();
>   this.store = configureStore(this.connector);
> }
> PanelLessStore.prototype = {
>   bootstrap: function(toolbox) {
>     // Initialize connection to the backend.
>     const connection = {
>       tabConnection: {
>         tabTarget: toolbox.target,
>       },
>       toolbox,
>       panel: null,
>       owner: null,
>     };
>     return this.connector.connectFirefox(connection, null,
> this.store.getState);
>   },
>   onRequestAdded: ..., 
>   getHarExportConnector: ...,
>   fetchResponseContent: ...,
>   add/remove/hasRequestFinishedListener: ...,
> }
> ```
Yes, I can imagine a super class like that. Let's prove such concept
with yet another use case.

> And you would use that from toolbox instead of NetMonitorApp. Would that
> work?
> If that helps you may make this PanelLessStore class be a super class for
> NetMonitorApp if you want to keep all WebExt API on NetMonitorApp object.
> (PanelLessStore isn't a perfect name...)
> 
> ::: commit-message-c56ef:3
> (Diff revision 2)
> > +Bug 1436665 - Introduce Net monitor app object; r=ochameau
> > +* The app object can be disconnected from the UI
> > +* The app object can be utilized by WebExtensions API
> 
> s/utilized/used/ ?
Fixed

> 
> ::: devtools/client/netmonitor/src/app.js:41
> (Diff revision 2)
> > +}
> > +
> > +NetMonitorApp.prototype = {
> > +  bootstrap({ toolbox, panel, window}) {
> > +    this.window = window;
> > +    this.document = this.window ? this.window.document : null;
> 
> We don't use window in this class, so either:
> - directly pass document as bootstrap argument:
>   bootstrap({ toolbox, panel, document}) {
>     this.document = document;
> - or only store document on this:
>   bootstrap({ toolbox, panel, window}) {
>     this.document = window ? window.document : null;
Also fixed. I am passing in: toolbox, panel and document. It's better

> ::: devtools/client/netmonitor/src/app.js:50
> (Diff revision 2)
> > +      tabConnection: {
> > +        tabTarget: toolbox.target,
> > +      },
> > +      toolbox,
> > +      panel,
> > +      owner: this,
> 
> I'm wondering if that would be clearer to name this attribute `app` rather
> than `owner`. With `app` would will more easily understand that refers to
> `NetMonitorApp`.
Yep, done.

> ::: devtools/client/netmonitor/src/connector/firefox-connector.js:391
> (Diff revision 2)
> > +    }
> > +
> > +    // Consumed mainly by tests.
> > +    if (typeof window != "undefined") {
> > +      window.emit(type, data);
> > +    }
> 
> I don't understand why tests are not listening for events on `owner`? (i.e.
> `window.Netmonitor` rather than `window`)
> It looks like unnecessary complexity from an obsolete reason.
Yep, agree, I'll file a follow-up bug to update all tests.
 
> So may be you can replace all calls to `this.emit(...)` by `if (this.owner)
> { this.owner.emit(...) }`.
> Also, if `owner` can be null, you should algo guard all calls to
> `this.owner.on(...)` like what you do for `emit`:
> `if (this.owner) { this.owner.on(...); }`
> 
> ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:628
> (Diff revision 2)
> > +
> > +    // Consumed mainly by tests.
> > -  if (typeof window != "undefined") {
> > +    if (typeof window != "undefined") {
> > -    window.emit(type, data);
> > +      window.emit(type, data);
> > -  }
> > +    }
> > -}
> > +  }
> 
> I have the same comment than FirefoxConnector.emit here.
Yep, we shouldn't depend on window. It doesn't exist 
if the App is used by WE API

> 
> ::: devtools/client/netmonitor/src/har/har-exporter.js:132
> (Diff revision 2)
> >     */
> >    getHar: function(options) {
> > -    return this.fetchHarData(options).then(JSON.parse);
> > +    return this.fetchHarData(options).then(data => {
> > +      return data ? JSON.parse(data) : null;
> > +    });
> >    },
> 
> You never use async/await in any of these patches and I think you should. It
> would make the code more readable:
> ```
> async getHar: function(options) {
>   let data = await this.fetchHarData(options);
>   if (data) {
>     return JSON.parse(data);
>   } else {
>     return null;
>   }
> }
> ```
I updated some places in the patch (the old code stays the same,
we should update it in another patch).

Thanks!
Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> I've been thinking the suggestion with the super class, implementing only
> features independent of the panel. I was thinking about this a lot,
> but didn't do it in the end. It sounds like the right direction,
> but we can wait for another use case to learn more yet (e.g. for
> new WebExtension API, integration with another panel, etc.)

One thing I forgot to mention. The App object has now two ways how
to be initialized:

1) `bootstrap()`: full initialization with panel/window included
2) `connect()`: only connection to the backend

Honza
Comment on attachment 8958783 [details]
Bug 1436665 - Expose Net panel API without the UI;

https://reviewboard.mozilla.org/r/227688/#review236552

I didn't realized we would instanciate multiple connectors with the full patch queue, see my second comment.

::: devtools/client/netmonitor/src/app.js:20
(Diff revision 3)
> +const { EVENTS } = require("./constants");
> +const Actions = require("./actions/index");
> +const {
> +  getDisplayedRequestById,
> +  getSortedRequests
> +} = require("./selectors/index");

Ok so if you are keeping this class as-is, you probably want to use `loader.lazyRequireGetter` to prevent loading unecessary dependencies when using this from non-panel codepaths. For: all react deps, Provider, App.

::: devtools/client/netmonitor/src/app.js:34
(Diff revision 3)
> +function NetMonitorApp() {
> +  EventEmitter.decorate(this);
> +
> +  // Configure store/state object.
> +  this.connector = new Connector();
> +  this.store = configureStore(this.connector);

Can we instanciate connector and store only once and share between the panel and panel-less usages?

1) We would instanciate only one client/actor. Do the queries only once, even if the netmonitor is opened *after* the addon.
2) Your later modification in toolbox.js would be much simplier if you share them:
```
    if (netPanel) {
      return netPanel.panelWin.Netmonitor.getHar();
    } if (this._netMonitorApp) {
      return this._netMonitorApp.getHar();
```
would become:
```
   return toolbox.getNetmonitorConnector().getHar();
```
3) here, you would fetch the connector from the toolbox instead of instanciate it:
```
  this.connector = toolbox.getNetmonitorConnector();
  this.store = this.connector.store; // I think it would be easier to pass directly the `store` to `Connector` rather than just `getState` function.
```
toobox.getNetmonitorConnector would be something like:
```
if (this.connector) return this.connector;
let connector = this.connector = new Connector();
let store = configureStore(this.connector);
let actions = bindActionCreators(Actions, store.dispatch);
const connection = {
  tabConnection: {
    tabTarget: toolbox.target,
  },
  toolbox,
}
connector.connectFirefox(connection, actions, store);
return connector;
```
* panel can be dropped, it is used only from here:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-connector.js#171
and can be replaced with:
```
let panel = this.toolbox.getPanel("netmonitor");
if (panel) panel.emit("reloaded");
```
* you would revert back to emit events on `window` rather than `owner`. Or emit them on the connector rather than the app.
4) You would move all webextension helpers to connector. It is better hosted there rather than on the App. Connector is panel-less and already contains methods similar to the one we need for WebExt.

::: devtools/client/netmonitor/src/app.js:90
(Diff revision 3)
> +      toolbox,
> +      panel,
> +      app: this,
> +    };
> +
> +    return this.connectBackend(this.connector, connection, this.actions,

Given that connectBackend if only called from here, I'm not sure it is worth having and would better inline it here.
Attachment #8958783 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #16)
> ::: devtools/client/netmonitor/src/app.js:20
> (Diff revision 3)
> > +const { EVENTS } = require("./constants");
> > +const Actions = require("./actions/index");
> > +const {
> > +  getDisplayedRequestById,
> > +  getSortedRequests
> > +} = require("./selectors/index");
> 
> Ok so if you are keeping this class as-is, you probably want to use
> `loader.lazyRequireGetter` to prevent loading unecessary dependencies when
> using this from non-panel codepaths. For: all react deps, Provider, App.
Done

> ::: devtools/client/netmonitor/src/app.js:34
> (Diff revision 3)
> > +function NetMonitorApp() {
> > +  EventEmitter.decorate(this);
> > +
> > +  // Configure store/state object.
> > +  this.connector = new Connector();
> > +  this.store = configureStore(this.connector);
> 
> Can we instanciate connector and store only once and share between the panel
> and panel-less usages?
No, the connector for WebExtensions is *not* sending notifications while the connector
used by the panel is. They are both used in parallel, so can't share the state.

Yes, sharing the connector would make the code simpler. But, having separate
connector for extensions, which is disconnected from the UI as well as
from the store (bug 1445617) will make the measurements (HTTP Timings)
more precise - less perf penalties (and the timing is what users mostly
want to measure).

> * panel can be dropped, it is used only from here:
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> connector/firefox-connector.js#171
> and can be replaced with:
> ```
> let panel = this.toolbox.getPanel("netmonitor");
> if (panel) panel.emit("reloaded");
> ```
Yeah, good point, agree, I'll do this anyway
in an extra patch, so it's easy to review.

> Given that connectBackend if only called from here, I'm not sure it is worth
> having and would better inline it here.
The method is called from two places and I also have an extra future plan with it.
As soon as connecting to different backends is supported this method
will be the point where a decision needs to be made. I updated the comment
to make this more clear.

Thanks for the review Alex!

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #21)
> > Can we instanciate connector and store only once and share between the panel
> > and panel-less usages?
> No, the connector for WebExtensions is *not* sending notifications while the
> connector
> used by the panel is. They are both used in parallel, so can't share the
> state.

What are notifications here? You mean redux actions/store updates?

> Yes, sharing the connector would make the code simpler. But, having separate
> connector for extensions, which is disconnected from the UI as well as
> from the store (bug 1445617) will make the measurements (HTTP Timings)
> more precise - less perf penalties (and the timing is what users mostly
> want to measure).

I see it very differently.
* When the netmonitor panel is closed. It won't be any different. There will be only one connector, used by WebExt.
* When the netmonitor panel is opened before the webext, it will be the same as well. We will be using one connector, the panel one, which will also fire redux updates.
* When the netmonitor panel is opened after the WebExt, we will have two connectors with your current patch.
  The performance is going to be worse. The two netmonitor RDP clients are going to fetch requests data in parallel.

bug 1445617 is independant improvement that doesn't directly relates to having one or more instances of connectors.
It will only benefit to the first case, when the panel is closed. The store updates done by the duplicated connectors, used for netmonitor UI, will very likely pollute the data of the other one used by WebExt as they are running on the same process.
But bug 1445617 is a good improvement, per comment 16 suggestion, it would allow to make toobox.getNetmonitorConnector create a connector without a store.

The more I look at this code (connectors, and especially FirefoxDataProvider) the more it looks like a "Front" (per protocol.js architecture).
A front is the client part of actors that helps querying actors data.
And all fronts are singletons, we only instanciate one unique front per actor instance.

Otherwise, our discussion and bug 1445617 seems to highlight that you only need the connector for WebExt purposes.
I feel that you are focusing on NetmonitorApp just because it is the place where we have all WebExt helpers today (getHar, fetchResponseContent and onRequestAdded). But these helpers would be better hosted into the connector as it isn't related to netmonitor frontend after all.
Attachment #8958784 - Attachment is obsolete: true
Attachment #8958784 - Flags: review?(poirot.alex)
Attachment #8958785 - Attachment is obsolete: true
Attachment #8958785 - Flags: review?(poirot.alex)
Attachment #8962428 - Attachment is obsolete: true
Attachment #8962428 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #22)

> What are notifications here? You mean redux actions/store updates?
Yes 

> > Yes, sharing the connector would make the code simpler. But, having separate
> > connector for extensions, which is disconnected from the UI as well as
> > from the store (bug 1445617) will make the measurements (HTTP Timings)
> > more precise - less perf penalties (and the timing is what users mostly
> > want to measure).
> 
> I see it very differently.
> * When the netmonitor panel is closed. It won't be any different. There will
> be only one connector, used by WebExt.
> * When the netmonitor panel is opened before the webext, it will be the same
> as well. We will be using one connector, the panel one, which will also fire
> redux updates.
> * When the netmonitor panel is opened after the WebExt, we will have two
> connectors with your current patch.
>   The performance is going to be worse. The two netmonitor RDP clients are
> going to fetch requests data in parallel.
The new path is reusing an existing connection that can be created before
the Net panel opens.

There are other changes:
- Only one patch sorry (all changes are quite interconnected)
- Using `owner` again (the `App` is no longer the recipient
  and this name feels more appropriate).
- The App object split into two:
   1) API (not connected to the UI)
   2) App (connected to the UI)

The API object is used if the Net panel doesn't exist and reused when
the Net panel is created later. Hope this will make my reviewer happy ;-)


> The more I look at this code (connectors, and especially
> FirefoxDataProvider) the more it looks like a "Front" (per protocol.js
> architecture).
> A front is the client part of actors that helps querying actors data.
> And all fronts are singletons, we only instanciate one unique front per
> actor instance.
> Otherwise, our discussion and bug 1445617 seems to highlight that you only
> need the connector for WebExt purposes.
> I feel that you are focusing on NetmonitorApp just because it is the place
> where we have all WebExt helpers today (getHar, fetchResponseContent and
> onRequestAdded). But these helpers would be better hosted into the connector
> as it isn't related to netmonitor frontend after all.
All this sounds promising, but should be analyzed discussed as part of another bug.

Try looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ecf7ff566d4ff0155ebaa5f537c65d54094f93

I am working on tests now.

Honza
Comment on attachment 8958783 [details]
Bug 1436665 - Expose Net panel API without the UI;

https://reviewboard.mozilla.org/r/227688/#review238786

Thanks a lot Jan for the new patch, it looks much better.

If getHarExportConnector can be kept for a followup, I think the patch is almost good.

::: devtools/client/framework/toolbox.js:2793
(Diff revision 5)
>      deferred.resolve(settleAll(outstanding)
>          .catch(console.error)
>          .then(() => {
> +          let app = this._netMonitor;
> +          this._netMonitor = null;
> +          return app ? app.destroy() : null;

s/app/api/?

::: devtools/client/framework/toolbox.js:3058
(Diff revision 5)
>  
>    /**
> -   * Returns data (HAR) collected by the Network panel.
> +   * Return Netmonitor API object. This object offers Network monitor
> +   * public API that can be consumed by other panels or WE API.
>     */
> -  getHARFromNetMonitor: async function() {
> +  getNetMonitorWhenReady: async function() {

May be it should be named getNetMonitorAPI?

::: devtools/client/framework/toolbox.js:3072
(Diff revision 5)
> -    let har = await netPanel.panelWin.Netmonitor.getHar();
> +      return this._netMonitor;
> +    }
> +
> +    // Create and initialize Network monitor API object.
> +    // This object is only connected to the backend - not to the UI.
> +    this._netMonitor = new NetMonitorAPI();

This attribute would be better named `_netMonitorAPI` now.

::: devtools/client/netmonitor/panel.js:20
(Diff revision 5)
>        await this.toolbox.target.makeRemote();
>      }
> -    await this.panelWin.Netmonitor.bootstrap({
> +
> +    // Reuse an existing Network monitor API object if available.
> +    // It could have been created for WE API before Net panel opens.
> +    let app = this.panelWin.initialize(this.toolbox._netMonitor);

Rather than using a private field like this, it would be better to do:
  let api = await this.toolbox.getNetmonitorAPI();
  let app = this.panelWin.initialize(api);

::: devtools/client/netmonitor/panel.js:23
(Diff revision 5)
> +    // Reuse an existing Network monitor API object if available.
> +    // It could have been created for WE API before Net panel opens.
> +    let app = this.panelWin.initialize(this.toolbox._netMonitor);
> +
> +    // Remove from the Toolbox. It belongs to the Network panel now.
> +    this.toolbox._netMonitor = null;

I imagine you do that to prevent having the API destroyed when WebExt unregister its listener, but this will be hard to maintain.

It would be slightly more explicit to call:
 api.addRequestFinishedListener(() => {});
in order to keep it alive.

Or:
* add an specific flag on NetMonitorAPI to make it always alive. And toggle this flag from here.
* Have toolbox.getNetmonitorAPI accept a boolean and use it from toolbox.js to prevent destroying it from removeRequestFinishedListener.
* ...

::: devtools/client/netmonitor/src/api.js:80
(Diff revision 5)
> +
> +    await this.connector.disconnect();
> +
> +    if (this.harExportConnector) {
> +      await this.harExportConnector.disconnect();
> +    }

This looks like something used by a followup.

::: devtools/client/netmonitor/src/api.js:121
(Diff revision 5)
> +      return;
> +    }
> +
> +    let { HarExporter } = require("devtools/client/netmonitor/src/har/har-exporter");
> +
> +    let connector = await this.getHarExportConnector();

WebExt should reuse the existing connector.
It looks like getHarExportConnector is related to another followup patch?

::: devtools/client/netmonitor/src/api.js:188
(Diff revision 5)
> +        tabTarget: this.toolbox.target,
> +      },
> +      toolbox: this.toolbox,
> +    };
> +
> +    this.harExportConnector = new Connector();

This method isn't necessary for this bug right?
It defeats the suggestion I made to have only one connector. It would be great to followup on this in order to see if getState can be lazily set on Connector, so that you only use it when you need it.

::: devtools/client/netmonitor/src/app.js:20
(Diff revision 5)
> +  getDisplayedRequestById,
> +} = require("./selectors/index");
> +
> +/**
> + * Global App object for Network panel. This object depends
> + * on the UI and can't be created independently.

I think the original comments was more helpful than this one:
This object can be consumed by other panels (e.g. Console is using inspectRequest), by the Launchpad (bootstrap), etc.

::: devtools/client/netmonitor/src/app.js:25
(Diff revision 5)
> + * on the UI and can't be created independently.
> + *
> + * @param {Object} api An existing API object to be reused.
> + */
> +function NetMonitorApp(api) {
> +  this.api = api || new NetMonitorAPI();

Automatically creating the API when none is given as argument can easily hide errors where we were unable to pass toolbox's one.
It would be safer to always expect one and have all the callsite pass one.
So it would be better in initializer.js:91 was creating an API:
  const { NetMonitorAPI } = require("./api");
  let api = new NetMonitorAPI();
  let app = window.initialize(api);

::: devtools/client/netmonitor/src/app.js:64
(Diff revision 5)
> +    });
> +
> +    // Render the root Application component.
> +    render(Provider({ store: store }, app), this.mount);
> +
> +    await this.api.connect(toolbox);

toolbox.js:3073 should already have "connected" the API. You should not need to call this again from here.

::: devtools/client/netmonitor/src/app.js:73
(Diff revision 5)
> +   * Clean up (unmount from DOM, remove listeners, disconnect).
> +   */
> +  async destroy() {
> +    unmountComponentAtNode(this.mount);
> +
> +    await this.api.destroy();

I'm not sure you want to destroy it from here.
There may still be WebExt listeners registered.
"In theory", as Netmonitor's destroy is only ever called on toolbox destroy...

::: devtools/client/netmonitor/src/connector/firefox-connector.js:40
(Diff revision 5)
>      this.getNetworkRequest = this.getNetworkRequest.bind(this);
>    }
>  
>    async connect(connection, actions, getState) {
>      this.actions = actions;
>      this.getState = getState;

It would be handy to mention here or in `connect` documentation that `getState` is optional.

::: devtools/client/netmonitor/src/connector/firefox-connector.js:44
(Diff revision 5)
>      this.getState = getState;
>      this.tabTarget = connection.tabConnection.tabTarget;
>      this.toolbox = connection.toolbox;
> -    this.panel = connection.panel;
>  
> +    // The owner object (NetPanelAPI) received all events.

s/NetPanelAPI/NetMonitorAPI/
Attachment #8958783 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #25)
> If getHarExportConnector can be kept for a followup, I think the patch is
> almost good.
Yes, I created a follow up Bug 1451304

> ::: devtools/client/framework/toolbox.js:2793
> (Diff revision 5)
> >      deferred.resolve(settleAll(outstanding)
> >          .catch(console.error)
> >          .then(() => {
> > +          let app = this._netMonitor;
> > +          this._netMonitor = null;
> > +          return app ? app.destroy() : null;
> 
> s/app/api/?
Done

> ::: devtools/client/framework/toolbox.js:3058
> (Diff revision 5)
> >  
> >    /**
> > -   * Returns data (HAR) collected by the Network panel.
> > +   * Return Netmonitor API object. This object offers Network monitor
> > +   * public API that can be consumed by other panels or WE API.
> >     */
> > -  getHARFromNetMonitor: async function() {
> > +  getNetMonitorWhenReady: async function() {
> 
> May be it should be named getNetMonitorAPI?
Done
getNetMonitorWhenReady -> getNetMonitorAPI
this._netMonitor -> this._netMonitorAPI

> Rather than using a private field like this, it would be better to do:
>   let api = await this.toolbox.getNetmonitorAPI();
>   let app = this.panelWin.initialize(api);
Done

> It would be slightly more explicit to call:
>  api.addRequestFinishedListener(() => {});
> in order to keep it alive.
Done

> I think the original comments was more helpful than this one:
> This object can be consumed by other panels (e.g. Console is using
> inspectRequest), by the Launchpad (bootstrap), etc.
Done

> Automatically creating the API when none is given as argument can easily
> hide errors where we were unable to pass toolbox's one.
> It would be safer to always expect one and have all the callsite pass one.
> So it would be better in initializer.js:91 was creating an API:
>   const { NetMonitorAPI } = require("./api");
>   let api = new NetMonitorAPI();
>   let app = window.initialize(api);
Yep, done

> ::: devtools/client/netmonitor/src/app.js:64
> (Diff revision 5)
> > +    });
> > +
> > +    // Render the root Application component.
> > +    render(Provider({ store: store }, app), this.mount);
> > +
> > +    await this.api.connect(toolbox);
> 
> toolbox.js:3073 should already have "connected" the API. You should not need
> to call this again from here.
Done

> 
> ::: devtools/client/netmonitor/src/app.js:73
> (Diff revision 5)
> > +   * Clean up (unmount from DOM, remove listeners, disconnect).
> > +   */
> > +  async destroy() {
> > +    unmountComponentAtNode(this.mount);
> > +
> > +    await this.api.destroy();
> 
> I'm not sure you want to destroy it from here.
> There may still be WebExt listeners registered.
> "In theory", as Netmonitor's destroy is only ever called on toolbox
> destroy...
It's for the case when the Network panel runs in a tab (there is no Toolbox in such case)
I made a comment to explain that.


> ::: devtools/client/netmonitor/src/connector/firefox-connector.js:40
> (Diff revision 5)
> >      this.getNetworkRequest = this.getNetworkRequest.bind(this);
> >    }
> >  
> >    async connect(connection, actions, getState) {
> >      this.actions = actions;
> >      this.getState = getState;
> 
> It would be handy to mention here or in `connect` documentation that
> `getState` is optional.
Done

> ::: devtools/client/netmonitor/src/connector/firefox-connector.js:44
> (Diff revision 5)
> >      this.getState = getState;
> >      this.tabTarget = connection.tabConnection.tabTarget;
> >      this.toolbox = connection.toolbox;
> > -    this.panel = connection.panel;
> >  
> > +    // The owner object (NetPanelAPI) received all events.
> 
> s/NetPanelAPI/NetMonitorAPI/
Done

Thanks Alex!

Honza
Comment on attachment 8958783 [details]
Bug 1436665 - Expose Net panel API without the UI;

https://reviewboard.mozilla.org/r/227688/#review239314

Thanks for addressing everything Jan!

I still have a comment about getHarExportConnector.

::: devtools/client/netmonitor/src/api.js:129
(Diff revision 6)
> +      console.error("HAR: request not found " + requestId);
> +      return;
> +    }
> +
> +    let options = {
> +      connector,

You should use this.connector here, as before this patch and as all other methods of this class.
And remove getHarExportConnector until you justify its needs.

I understand you intend to avoid noise in har timings. This setup may be better when you end up instanciating only one of the two connectors (i.e when you only open WebExt -or- when you only open netmonitor). But it is going to be really bad if you have the two opened. You can make it fast in both cases by disabling getState calls if you only open WebExt or complely disable store interaction if you are only using it from Har automation.
Attachment #8958783 - Flags: review?(poirot.alex) → review+
I yet updated the patch to fix some test failures.

(1) I did remove the `api.addRequestFinishedListener(() => {})` since adding an empty listeners causes unnecessary overhead (HAR is generated for nothing). Instead the Toolbox is testing if the Net panel exists and if yes not destroying the API object.

(2) Also connecting to backend in the API object (`connectBackend` method) is calling `target.makeRemote` to make sure the web client object is properly initialized.

(3) Check values of props in FirefoxConnector to avoid NPE (e.g. for actions)


(4) I created a new patch that stops firing events on the panel window object and updates all tests (we also discussed that). Tests are now listening for events on the API object.

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b490a5083e4a71e714d5b1fcaa50c37736431922

> I still have a comment about getHarExportConnector.
> 
> ::: devtools/client/netmonitor/src/api.js:129
> (Diff revision 6)
> > +      console.error("HAR: request not found " + requestId);
> > +      return;
> > +    }
> > +
> > +    let options = {
> > +      connector,
> 
> You should use this.connector here, as before this patch and as all other
> methods of this class.
> And remove getHarExportConnector until you justify its needs.
> 
> I understand you intend to avoid noise in har timings. This setup may be
> better when you end up instanciating only one of the two connectors (i.e
> when you only open WebExt -or- when you only open netmonitor). But it is
> going to be really bad if you have the two opened. You can make it fast in
> both cases by disabling getState calls if you only open WebExt or complely
> disable store interaction if you are only using it from Har automation.
I am aware of that and it'll be fixed in bug Bug 1451304

The thing is that standard connection fires actions (to updated the UI)
while export har connection does not (to fix performance) and so the
HAR export needs extra connection that knows not to fire actions.
I'll find better way in Bug 1451304

---

The existing test:
browser/components/extensions/test/browser/browser_ext_devtools_network.js

...needs to be updated yet (wip).

Honza
@Luca: the existing test:
browser/components/extensions/test/browser/browser_ext_devtools_network.js

... now fails for me since `addRequestListener` and `removeRequestListener` are async and the test doesn't wait for them to finish. Shutdown and disconnect is done too soon (in the middle of HAR generation) and the test fails. You might try it for yourself. Any tips how to fix this?

Honza
Flags: needinfo?(lgreco)
(In reply to Jan Honza Odvarko [:Honza] from comment #31)
> (2) Also connecting to backend in the API object (`connectBackend` method)
> is calling `target.makeRemote` to make sure the web client object is
> properly initialized.

Did that ended up breaking a test?

(I'm asking as I would like some day to simplify this makeRemote thing and have it called from only one place)
Comment on attachment 8965343 [details]
Bug 1436665 - Do not fire events on window, update tests;

https://reviewboard.mozilla.org/r/234070/#review239728

Thanks for this cleanup!

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:85
(Diff revision 1)
>  }
>  
>  function waitForRequestAdded(toolbox) {
>    return new Promise(resolve => {
>      let netPanel = toolbox.getPanel("netmonitor");
> -    netPanel.panelWin.once("NetMonitor:RequestAdded", () => {
> +    netPanel.panelWin.api.once("NetMonitor:RequestAdded", () => {

nit: may be exposing `api` onto panel object would help?
Attachment #8965343 - Flags: review?(poirot.alex) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #32)
> @Luca: the existing test:
> browser/components/extensions/test/browser/browser_ext_devtools_network.js
> 
> ... now fails for me since `addRequestListener` and `removeRequestListener`
> are async and the test doesn't wait for them to finish. Shutdown and
> disconnect is done too soon (in the middle of HAR generation) and the test
> fails. You might try it for yourself. Any tips how to fix this?

Hi Jan,
I looked into it and the main reason for the failures on the devtools.network APIs tests
seems to be related to the initial assumption under which we tested the initial
implementation that "the onRequestFinished events were only going to be received
when the netmonitor panel has been already selected by the user and loaded once",
with the attached patch applied the events are actually received even without selecting
the panel, which is great, and so we should basically fix the test, so that we are
going to receive the event only when we are actually expecting it, otherwise the WebExtensions
tests are going to fail automatically because of "test messages" received and never processed.

Here is the diff related to the changes I applied locally to successfully pass the tests
with these patches applied (the info("...") are only there to be able to investigate timeouts
more easily from the failures logs of a push to try):

diff --git a/browser/components/extensions/test/browser/browser_ext_devtools_network.js b/browser/components/extensions/test/browser/browser_ext_devtools_network.js
--- a/browser/components/extensions/test/browser/browser_ext_devtools_network.js
+++ b/browser/components/extensions/test/browser/browser_ext_devtools_network.js
@@ -71,17 +71,22 @@ function devtools_page() {
     // Get response content using returned promise
     request.getContent().then(([content, encoding]) => {
       browser.test.sendMessage("onRequestFinished-promiseResolved",
                                [content, encoding]);
     });
 
     browser.devtools.network.onRequestFinished.removeListener(requestFinishedListener);
   };
-  browser.devtools.network.onRequestFinished.addListener(requestFinishedListener);
+
+  browser.test.onMessage.addListener(msg => {
+    if (msg === "addOnRequestFinishedListener") {
+      browser.devtools.network.onRequestFinished.addListener(requestFinishedListener);
+    }
+  });
 }
 
 function waitForRequestAdded(toolbox) {
   return new Promise(resolve => {
     let netPanel = toolbox.getPanel("netmonitor");
     netPanel.panelWin.api.once("NetMonitor:RequestAdded", () => {
       resolve();
     });
@@ -181,19 +186,16 @@ add_task(async function test_devtools_ne
   // Reload the page to collect some HTTP requests.
   extension.sendMessage("navigate");
 
   // Wait till the navigation is complete and request
   // added into the net panel.
   await Promise.all([
     extension.awaitMessage("tabUpdated"),
     extension.awaitMessage("onNavigatedFired"),
-    extension.awaitMessage("onRequestFinished"),
-    extension.awaitMessage("onRequestFinished-callbackExecuted"),
-    extension.awaitMessage("onRequestFinished-promiseResolved"),
     waitForRequestAdded(toolbox),
   ]);
 
   // Get HAR, it should not be empty now.
   const getHARPromise = extension.awaitMessage("getHAR-result");
   extension.sendMessage("getHAR");
   const getHARResult = await getHARPromise;
   is(getHARResult.entries.length, 1, "HAR log should not be empty");
@@ -219,27 +221,33 @@ add_task(async function test_devtools_ne
   await extension.awaitMessage("ready");
 
   let target = gDevTools.getTargetForTab(tab);
 
   // Open the Toolbox
   let toolbox = await gDevTools.showToolbox(target, "netmonitor");
   info("Developer toolbox opened.");
 
+  // Wait the extension to subscribe the onRequestFinished listener.
+  await extension.sendMessage("addOnRequestFinishedListener");
+
   // Reload and wait for onRequestFinished event.
   extension.sendMessage("navigate");
 
+  info("Wait for tab navigation to be completed");
   await Promise.all([
     extension.awaitMessage("tabUpdated"),
     extension.awaitMessage("onNavigatedFired"),
     waitForRequestAdded(toolbox),
   ]);
 
+  info("Wait for an onRequestFinished event");
   await extension.awaitMessage("onRequestFinished");
 
+  info("Wait for request.getBody results");
   // Wait for response content being fetched.
   let [callbackRes, promiseRes] = await Promise.all([
     extension.awaitMessage("onRequestFinished-callbackExecuted"),
     extension.awaitMessage("onRequestFinished-promiseResolved"),
   ]);
 
   ok(callbackRes[0].startsWith("<html>"),
      "The expected content has been retrieved.");
Flags: needinfo?(lgreco)
Comment on attachment 8958783 [details]
Bug 1436665 - Expose Net panel API without the UI;

https://reviewboard.mozilla.org/r/227688/#review239776

The following comments seems unrelated to the WebExtensions tests failures (as described in comment 35), but while I was investigating those failures I noticed them and I thought they may be worth a mention.

::: devtools/client/netmonitor/src/api.js:185
(Diff revision 7)
> +    if (this.harExportConnector) {
> +      return this.harExportConnector;
> +    }
> +
> +    const connection = {
> +      tabConnection: {

This `connection` object doesn have `owner: this` as the one defined at line 58 (there it is part of the connection, at line 63).

This would make `this.owner` undefined in the "firefox-connector.js" (which set it to the `connection.owner` at line 52 of "firefox-connector.js", inside the connect method), from what I saw it seems that it would be unexpected (e.g. the `navigate` method access `this.owner` without checking if it is defined at line 186 of that file).

::: devtools/client/netmonitor/src/connector/firefox-connector.js:88
(Diff revision 7)
> +    }
>  
>      await this.removeListeners();
>  
>      if (this.tabTarget) {
>        this.tabTarget.off("will-navigate");

It looks that at line 70 and 71 we are subscribing a listener to both "will-navigate" and "navigate", I guess that we should unsubscribe also from "navigate" here, am I reading it wrong?
(In reply to Alexandre Poirot [:ochameau] from comment #33)
> (In reply to Jan Honza Odvarko [:Honza] from comment #31)
> > (2) Also connecting to backend in the API object (`connectBackend` method)
> > is calling `target.makeRemote` to make sure the web client object is
> > properly initialized.
> 
> Did that ended up breaking a test?
> 
> (I'm asking as I would like some day to simplify this makeRemote thing and
> have it called from only one place)
Yes, this one:
devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_attach.js

Here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82aa8598af312c7aa71c08b1b0f5e7da9d2aeb9d&selectedJob=172042285

(simply reproducible locally)

Honza
(In reply to Alexandre Poirot [:ochameau] from comment #34)
> >  function waitForRequestAdded(toolbox) {
> >    return new Promise(resolve => {
> >      let netPanel = toolbox.getPanel("netmonitor");
> > -    netPanel.panelWin.once("NetMonitor:RequestAdded", () => {
> > +    netPanel.panelWin.api.once("NetMonitor:RequestAdded", () => {
> 
> nit: may be exposing `api` onto panel object would help?
Yep, sounds good to me. There are more globals exposed on the `window`
(for tests) and if we change it to the `panel` it should be done for all,
so it's consistent. I'll file a follow up as soon as this entire bug lands.

Thanks for the review!

Honza
(In reply to Luca Greco [:rpl] from comment #35)
Thanks for the help!

I am attaching a patch that fixes the test

(In reply to Luca Greco [:rpl] from comment #36)
> This `connection` object doesn have `owner: this` as the one defined at line
> 58 (there it is part of the connection, at line 63).
Good point, I added a condition to avoid NPE (part of the patch)

> It looks that at line 70 and 71 we are subscribing a listener to both
> "will-navigate" and "navigate", I guess that we should unsubscribe also from
> "navigate" here, am I reading it wrong?
Also fixed

---

Try is green:
(the linting failure is fixed in the patch)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6f18ee749d76bb2d68134ab318d718de8271654

Honza
Remember to update the compat data when this is fixed -> https://github.com/mdn/browser-compat-data/pull/1769#issuecomment-380344825
Keywords: dev-doc-needed
Comment on attachment 8966567 [details]
Bug 1436665 - Fix WebExtensions devtools.network tests;

https://reviewboard.mozilla.org/r/235290/#review242192

Hi Jan, the changes applied to the tests looks good to me (I've only a couple of comments below about it, I've marked only one as an issue to fix, not a big deal to be fair, and a small optional nit, but I'm also marking the review as an r+ right now, because they are just minimal and/or optional changes).

About the fixes applied on the firefox-connector.js module, they looks reasonable to me, but I don't know in very much detail all the behaviors expected from the `navigate` method and so it could be reasonable for Alex or someone who worked on this module before to double-check it and add his/her additional sign-off to it.

Thanks again for working on the fix for the initial devtools.network API limitations so quickly!

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:116
(Diff revision 1)
> +      resolve();
> +    });
> +  });
> +}
> +
> +async function navigate(extension, toolbox) {

It is not a big deal (and so I'm not even marking it as an issue), but I'm wondering if renaming the helper to something like `navigateToolboxTarget(extension, toolbox)` would make it clearer why the helper gets the extension and toolbox as its parameters.

How that would sound to you?

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:236
(Diff revision 1)
>  
> -  // Reload and wait for onRequestFinished event.
> -  extension.sendMessage("navigate");
> +  // Wait the extension to subscribe the onRequestFinished listener.
> +  await extension.sendMessage("addOnRequestFinishedListener");
>  
> -  await Promise.all([
> -    extension.awaitMessage("tabUpdated"),
> +  // Reload the page
> +  navigate(extension, toolbox);

Even if there is a `await extension.awaitMessage("onRequestFinished");` at line 242, I think that it would still be better to `await` the returned promise of the navigate helper also here.
Attachment #8966567 - Flags: review?(lgreco) → review+
Comment on attachment 8966567 [details]
Bug 1436665 - Fix WebExtensions devtools.network tests;

https://reviewboard.mozilla.org/r/235290/#review242198

::: commit-message-6db4e:1
(Diff revision 1)
> +Bug 1436665 - Fix tests; r=rpl

Do you mind to also tweak the bug summary message a bit? (e.g. Bug 1436665 - Fix WebExtensions devtools.network tests)
@Alex: can you take a look at the changes in firefox-connector.js
module (just a few lines), thanks.

Honza
Flags: needinfo?(poirot.alex)
(In reply to Luca Greco [:rpl] from comment #44)
> It is not a big deal (and so I'm not even marking it as an issue), but I'm
> wondering if renaming the helper to something like
> `navigateToolboxTarget(extension, toolbox)` would make it clearer why the
Done

> Even if there is a `await extension.awaitMessage("onRequestFinished");` at
> line 242, I think that it would still be better to `await` the returned
> promise of the navigate helper also here.
Done

> Do you mind to also tweak the bug summary message a bit? (e.g. Bug 1436665 -
> Fix WebExtensions devtools.network tests)
Done

Thanks Luca, I appreciate your help on this bug!

Honza
Comment on attachment 8966567 [details]
Bug 1436665 - Fix WebExtensions devtools.network tests;

https://reviewboard.mozilla.org/r/235290/#review242648

Sorry for the delay, connector's change looks good!
Attachment #8966567 - Flags: review?(poirot.alex) → review+
Flags: needinfo?(poirot.alex)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b8fe40857db
Expose Net panel API without the UI; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/513c72b05382
Do not fire events on window, update tests; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/09030f59fbea
Fix WebExtensions devtools.network tests; r=ochameau,rpl
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00d4c3be380c
Backed out 3 changesets for talos damp failures on netmonitor/simple.js. CLOSED TREE
Ah, sorry about that. The DAMP test fixed, trying to reland.

Honza
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ddbbd0330a1
Expose Net panel API without the UI; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/b3702a775b16
Do not fire events on window, update tests; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/2120b4c84746
Fix WebExtensions devtools.network tests; r=ochameau,rpl
Also, on the previous push, there were clipboard failures on devtools/client/netmonitor/src/har/test/browser_net_har_throttle_upload.js

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=09030f59fbeae4360be8c46d7e85ff32e9c00201
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=174070347&repo=autoland&lineNumber=4428
Test failures from previous logs fixed(In reply to Narcis Beleuzu [:NarcisB] from comment #61)

> Also, on the previous push, there were clipboard failures on
> devtools/client/netmonitor/src/har/test/browser_net_har_throttle_upload.js
Fixed

I'll try to land again.
Honza
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f69489d9bb15
Expose Net panel API without the UI; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/de4c0e01289b
Do not fire events on window, update tests; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d22104932e7b
Fix WebExtensions devtools.network tests; r=ochameau,rpl
https://hg.mozilla.org/mozilla-central/rev/f69489d9bb15
https://hg.mozilla.org/mozilla-central/rev/de4c0e01289b
https://hg.mozilla.org/mozilla-central/rev/d22104932e7b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1454948
@Will: This is follow up for bug 1311171

The Network panel doesn't have to be opened to make the `onRequestFinished` WebExtension API to work.
The patch here is for 61 and no uplift planned. The doc should be updated to reflect it.

Thanks!
Honza
Flags: needinfo?(wbamberg)
Thanks :Honza! I've updated the compat table to say that this restriction doesn't apply from 61 onwards : https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.network/onRequestFinished#Browser_compatibility

I'm marking this dev-doc-complete, but please let me know if you need anything else.
Flags: needinfo?(wbamberg)
Product: Firefox → DevTools
Sorry for adding comments on a already done task. For me the HAR isn't correct see https://github.com/devtools-html/har-export-trigger/issues/17
(In reply to Peter Hedenskog from comment #71)
> Sorry for adding comments on a already done task. For me the HAR isn't
> correct see https://github.com/devtools-html/har-export-trigger/issues/17
Can you please file a bug covering this issue here in bugzilla?
I'll look at it.
Thanks!
Honza
Flags: needinfo?(peter)
You need to log in before you can comment on or make changes to this bug.