1311177 - Implement the devtools.network.getHAR API method

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, defect, P3)

Description

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

The devtools.network.getHAR API method retrieves all the collected network requests generated by the target window as a HAR log (e.g. mostly like currently collected by the devtools and exportable from the network panel).

References:
- https://developer.chrome.com/extensions/devtools_network#method-getHAR

Comment 1

[Andrei Petcu]

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.

Comment 2

[Peter Hedenskog]

This is needed to make https://github.com/firebug/har-export-trigger work in the future (you can use the trigger from Selenium to get the HAR file).

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Luca: I am working on better support for HAR this quarter and `getHAR` API is an important piece of the effort. Is it ok to work on this?
Honza

Comment 4

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

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> @Luca: I am working on better support for HAR this quarter and `getHAR` API
> is an important piece of the effort. Is it ok to work on this?

yes, it is absolutely ok.

Bug 1311171 is also relevant (basically an extension can subscribe a listener to devtools.network.onRequestFinished and receive a single HAR entry per request finished, and it can also retrieve the full HAR log for every requests already collected by calling devtools.network.getHAR).

Some relevant WebExtensions internals on m-c related to the devtools.network API namespace:

- API JSON schema file for the devtools.network namespace (where the getHAR method and the onRequestFinished event are currently marked as unsupported, and so they are currently going to be skipped when the devtools.network namespace API object is being created by the WE internals):

  https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/browser/components/extensions/schemas/devtools_network.json

- API implementation module:

  https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/browser/components/extensions/ext-devtools-network.js

:aswan has recently landed some docs on m-c related to the creation of new WebExtensions API:

- https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/index.html

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1311177 - Implement the devtools.network.getHAR API method;

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

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Patch submitted

@Luca please review WebExtensions API parts
@Ricky please review Network monitor parts

Thanks!
Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Btw. I am testing the getHAR() API using HARExportTrigger extension. 
Preliminary version is available here:
https://github.com/devtools-html/har-export-trigger

Honza

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211714/#review217710

::: devtools/client/netmonitor/initializer.js:81
(Diff revision 1)
> +   * Returns list of requests currently available in the panel.
> +   */
> +  getHar() {
> +    let { HarExporter } = require("devtools/client/netmonitor/src/har/har-exporter");
> +    let { getLongString, getTabTarget, requestData } = connector;
> +    let { form: { title, url } } = getTabTarget();
> +    let state = store.getState();
> +
> +    let options = {
> +      getString: getLongString,
> +      items: getSortedRequests(state),
> +      requestData,
> +      title: title || url,
> +    };
> +
> +    return HarExporter.getHar(options);
> +  },
> +
> +  /**

We should separate WebExt APIs from our client implementation if possible. IMO, this step of opening the netmonitor panel is unnecessary when accessing getHAR() API.

I suggest this, the getHAR() in ext-devtools-network.js should do

`require("devtools/client/netmonitor/src/har/har-exporter");`

and access har API directly.

Does it makes sense to you?

::: devtools/client/netmonitor/src/har/har-builder-utils.js:4
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/* eslint no-unused-vars: [2, {"vars": "local"}] */

Is this eslint config necessary?

::: devtools/client/netmonitor/src/har/har-builder-utils.js:8
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/* eslint no-unused-vars: [2, {"vars": "local"}] */
> +
> +"use strict";
> +
> +const HAR_VERSION = "1.1";

I guess this is using for extension upgrade. Please add a comment about the purpose of HAR_VERSION.

::: devtools/client/netmonitor/src/har/har-builder-utils.js:32
(Diff revision 1)
> +var EXPORTED_SYMBOLS = [
> +  "buildHarLog"
> +];
> +
> +// Consumed by DevTools
> +if (typeof exports !== "undefined") {

nit: if (exports) { ... }

::: devtools/client/netmonitor/src/har/har-exporter.js:129
(Diff revision 1)
> +   *
> +   * @param Object options
> +   *        Configuration object, see save() for detailed description.
> +   */
> +  getHar: function (options) {
> +    return this.fetchHarData(options).then(jsonString => {

nit: I believe this code can be simplified into

```
return this.fetchHarData(options).then(JSON.parse);
```

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #8)
> We should separate WebExt APIs from our client implementation if possible.
> IMO, this step of opening the netmonitor panel is unnecessary when accessing
> getHAR() API.
Not simple, see below.

> ::: devtools/client/netmonitor/src/har/har-builder-utils.js:4
> (Diff revision 1)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +/* eslint no-unused-vars: [2, {"vars": "local"}] */
> 
> Is this eslint config necessary?
It's for EXPORTED_SYMBOLS and I changed that to:
/*eslint no-unused-vars: "EXPORTED_SYMBOLS"*/

> > +const HAR_VERSION = "1.1";
> 
> I guess this is using for extension upgrade. Please add a comment about the
> purpose of HAR_VERSION.
It's just the current supported HAR spec version (we need to upgrade to 1.2, but
it's out of scope of this bug). Comment appended.

> > +if (typeof exports !== "undefined") {
> 
> nit: if (exports) { ... }
Done

> nit: I believe this code can be simplified into
> 
> ```
> return this.fetchHarData(options).then(JSON.parse);
> ```
Done

---

Some analysis/comments related to "HAR export without the Toolbox"

1) The WebExtensions architecture introduces a new scope (besides content and background) called devtools. This scope is automatically created for every opened DevTools Toolbox (so, there can more intances of the scope) and all DevTools WebExt API are available in it. This means that the Toolbox has be to opened in order to make the `chrome.network.getHAR()` available. I don't think there is a workaround. Note that Chrome works the same way.
 
@Luca: is this correct assumption?


2) Another thing is that the Network panel needs to be selected at least once after the Toolbox is opened. This ensures that the panel is actually created and starts listening for and collecting all the HTTP details. It would be wrong to create the panel by default immediately after the Toolbox is opened since collecting HTTP details represents performance penalty and many users are not even interested in that. Note that Chrome works the same way.

We might want to introduce a new pref (false by default) that could be set to true in order to force the Network panel to be created by default. However, I am not sure if this is needed.

@Peter, do you know if Chrome has something like that?

---

The patch is almost ready. The last missing thing is forcing lazily loaded data (e.g. response body, etc.) to be fetched automatically when `getHAR` is called. I am working on this and I'll attach a new patch for it.


Honza

Comment 10

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

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211714/#review218192

Thanks for getting this started!

Follows an initial round of review comments (and some questions).

Besides the inline comments, The following are some pointers related to the additional test case that we are going to add for the newly supported API method:

- the following is the test file for the existent devtools.network.onNavigated API event:
  https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/test/browser/browser_ext_devtools_network.js
  
- the entire test extension is defined inside the test case itself:
  https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/test/browser/browser_ext_devtools_network.js#43-60
  
- the extension object returned by ExtensionTestUtils.loadExtension allows the test to startup and unload
  the test extension (and await these asynchronous calls to be completed)
  https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/test/browser/browser_ext_devtools_network.js#63,89

- the test case privileged code and the test extension code can exchange messages using
  extension.awaitMessage/extension.sendMessage methods on the 'test case' side and browser.test.sendMessage
  and browser.test.onMessage on the 'test extension' side:
  
  - https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/test/browser/browser_ext_devtools_network.js#27,64
  - https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/test/browser/browser_ext_devtools_network.js#12,71

- both the test case privileged code and the test extension code can make assertions, the extension code
  can use the browser.test.assert* methods (side note: unfortunately is(...) and browser.test.assertEq(...)
  use the opposite convention for the expected and actual parameter):
  
  - https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/test/browser/browser_ext_devtools_network.js#34,74

::: browser/components/extensions/ext-devtools-network.js:8
(Diff revision 1)
>  "use strict";
>  
>  // The ext-* files are imported into the same scopes.
>  /* import-globals-from ext-devtools.js */
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "buildHarLog",

Initially we were importing the needed devtools pieces lazily when needed in the WebExtensions API implementation modules, but we are now always using the DevToolsShim as the entrypoint to the code exported by the devtools modules.

E.g. The following link points to the some code where we use the DevToolsShim in the ext-devtools.js module:

- https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/ext-devtools.js#16-17,57,96

The following are some useful pointers to the DevToolsShim implementation:

- https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/shim/DevToolsShim.jsm#226-238
- https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/framework/devtools.js#574-580

:jdescottes is a great pick to review that part of the changes.

::: browser/components/extensions/ext-devtools-network.js:35
(Diff revision 1)
> +              let netPanel = context.devToolsToolbox.getPanel("netmonitor");
> +
> +              // The panel doesn't have to exist (it must be selected
> +              // by the user at least once to be created).
> +              // Return default empty HAR file in such case.
> +              if (!netPanel) {
> +                return buildHarLog(Services.appinfo);
> +              }
> +
> +              // Use Netmonitor object to get the current HAR log.
> +              return netPanel.panelWin.Netmonitor.getHar();

How about extracting all of this part into the method exported by the DevToolsShim?

So that this could become something like:

```
getHAR: function() {
  return DevToolsShim.getHARFromToolbox(toolbox);
}
```

::: browser/components/extensions/ext-devtools-network.js:39
(Diff revision 1)
> +            return targetPromise.then(target => {
> +              let netPanel = context.devToolsToolbox.getPanel("netmonitor");
> +
> +              // The panel doesn't have to exist (it must be selected
> +              // by the user at least once to be created).
> +              // Return default empty HAR file in such case.

It looks that the requests are logged (and filtered out by default) also when the webconsole has been opened (even when the network panel has never been opened yet), am I right?

I'm wondering if it would be possible to generate the HAR logs (using the same internals used by the net monitor) without the need to open the Network panel first?

(as a side note: how does it behave on Chrome? does the method return the HAR log if called without opening the Network panel?)

Comment 11

[Peter Hedenskog]

> @Peter, do you know if Chrome has something like that?

No Chrome has nothing like that what I know about. You need to open the devtools panel manually. The thing with the HAR in devtools in Chrome is that it isn't actually as good (it misses parts), so we use the Chrome tracelog instead to parse our own version. The HAR generated in FF is general much better than it is in Chrome (at least a couple of versions ago when I looked deeper into it).

But it would be super if we could avoid having it open for Firefox, or at least not visible since we use it the browser screen to record a video and analyze it. Since FF 55 we've been able to do that together with WebPageReplay (so we can replay content locally) and the only missing part now is to get the HAR.

Comment 12

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

(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> Some analysis/comments related to "HAR export without the Toolbox"
> 
> 1) The WebExtensions architecture introduces a new scope (besides content
> and background) called devtools. This scope is automatically created for
> every opened DevTools Toolbox (so, there can more intances of the scope) and
> all DevTools WebExt API are available in it. This means that the Toolbox has
> be to opened in order to make the `chrome.network.getHAR()` available. I
> don't think there is a workaround. Note that Chrome works the same way.
>  
> @Luca: is this correct assumption?

yes, this is absolutely correct, the entire devtools API namespace exists only
in devtools contexts, and no devtools context exists until a toolbox is opened.

The currently available devtools extensions contexts are:

 - the devtools page, which is invisible and it is created when the toolbox is created
   (on toolbox-created: https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/components/extensions/ext-devtools.js#305)
 
 - further devtools contexts are created from the devtools page itself
   (e.g. devtools panels registered from the devtools page, but it is actually created only once the panel
   has been selected by the user once)

 - a sub-frame of a devtools page (or a devtools panel) which is pointed to an extension url 
   is also a DevTools Extension context (and it is allowed to use the devtools API namespace)

> 
> 2) Another thing is that the Network panel needs to be selected at least
> once after the Toolbox is opened. This ensures that the panel is actually
> created and starts listening for and collecting all the HTTP details. It
> would be wrong to create the panel by default immediately after the Toolbox
> is opened since collecting HTTP details represents performance penalty and
> many users are not even interested in that. Note that Chrome works the same
> way.

This seems to be the answer to one of my question: so in Chrome getHAR returns
and empty HAR log if the network panel has not been opened yet?

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

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

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #8)
> > +// Consumed by DevTools
> > +if (typeof exports !== "undefined") {
> 
> nit: if (exports) { ... }
typeof is needed here since the var doesn't have to exist (undefined) and can't be used directly in a condition.


(In reply to Luca Greco [:rpl] from comment #10)
> ::: browser/components/extensions/ext-devtools-network.js:35
> (Diff revision 1)
> > +              let netPanel = context.devToolsToolbox.getPanel("netmonitor");
> > +
> > +              // The panel doesn't have to exist (it must be selected
> > +              // by the user at least once to be created).
> > +              // Return default empty HAR file in such case.
> > +              if (!netPanel) {
> > +                return buildHarLog(Services.appinfo);
> > +              }
> > +
> > +              // Use Netmonitor object to get the current HAR log.
> > +              return netPanel.panelWin.Netmonitor.getHar();
> 
> How about extracting all of this part into the method exported by the
> DevToolsShim?
> 
> So that this could become something like:
> 
> ```
> getHAR: function() {
>   return DevToolsShim.getHARFromToolbox(toolbox);
> }
> ```
Done


> 
> It looks that the requests are logged (and filtered out by default) also
> when the webconsole has been opened (even when the network panel has never
> been opened yet), am I right?
The network panel needs to be selected.

> I'm wondering if it would be possible to generate the HAR logs (using the
> same internals used by the net monitor) without the need to open the Network
> panel first?
Collecting HTTP data represents perf penalty and so having the Net panel created only if the user is actually interested (ie opens the panel at least once) is good thing. Since most of the users won't use `getHAR` I think it's good to keep this behavior and don't enable the panel by default. Our tools will be faster.

> (as a side note: how does it behave on Chrome? does the method return the
> HAR log if called without opening the Network panel?)
`getHAR` in Chrome returns content of the Panel. AFAIK the panel starts collecting the data immediately after the toolbox is opened.

Test now included in the patch

Thanks Luca for all the pointers!

Honza

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211714/#review218712

LGTM for the netmonitor part and thanks for the detailed explaination and update.

::: devtools/client/netmonitor/initializer.js:27
(Diff revision 2)
>  const Provider = createFactory(require("devtools/client/shared/vendor/react-redux").Provider);
>  const { bindActionCreators } = require("devtools/client/shared/vendor/redux");
>  const { Connector } = require("./src/connector/index");
>  const { configureStore } = require("./src/utils/create-store");
>  const App = createFactory(require("./src/components/App"));
> -const { getDisplayedRequestById } = require("./src/selectors/index");
> +const { getDisplayedRequestById, getSortedRequests } = require("./src/selectors/index");

nit: using multi-line here

::: devtools/client/netmonitor/src/har/har-builder-utils.js:4
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/*eslint no-unused-vars: "EXPORTED_SYMBOLS"*/

nit: add an empty line between license and eslint config

::: devtools/client/netmonitor/src/har/har-builder.js:16
(Diff revision 2)
>  const {
>    getFormDataSections,
>    getUrlQuery,
>    parseQueryString,
>  } = require("../utils/request-utils");
>  

nit: remove empty line

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

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

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #15)
> Comment on attachment 8941393 [details]
> Bug 1311177 - Implement the devtools.network.getHAR API method;
All done

Thanks Ricky,
Honza

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1664a4a223d894df6f7520223fa51fb0ed609b00
(one failure, but seems unrelated)

Honza

Comment 19

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

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211712/#review219224

Thanks Honza, the WebExtensions API implementation (and testing) pieces looks almost ready (there are just a couple of additional comments about it, once we handle those, I'm going to my r+ on this patch), but I'd also like to have jdescottes give a look at the DevToolsShim pieces (I'm adding him as an additional reviewer on this patch).

::: browser/components/extensions/ext-devtools-network.js:32
(Diff revision 3)
>                  target.off("navigate", listener);
>                });
>              };
>            }).api(),
> +
> +          getHAR: function(callback) {

We should remove the callback parameter here,
it is unused and it is not received by the API methods implemented here (a callback parameter can only be accessed in the API methods implemented in the same process where the extension context is running, e.g. in the ext-c-APINAME.js files).

::: browser/components/extensions/ext-devtools-network.js:33
(Diff revision 3)
>                });
>              };
>            }).api(),
> +
> +          getHAR: function(callback) {
> +            let targetPromise = getDevToolsTargetForContext(context);

And so I'm wondering if this could be just:

```
getHAR: function() {
  return DevToolsShim.getHARFromToolbox(
    context.devToolsToolbox
  );
}
```

getDevToolsTargetForContext is used when we need a remote debugging connect (separate from the connection used by the integrated developer tools) to exchange messages with the RDP actors, and it does't seem that we need it here (on the contrary the context.devToolsToolbox property is set automatically to the toolbox related to the extension context when it is being created and it doesn't need the getDevToolsTargetForContext to be called first).

::: devtools/shim/DevToolsShim.jsm:232
(Diff revision 3)
> +  },
> +
> +  /**
> +   * Returns data (HAR) collected by the Network panel.
> +   */
> +  getHARFromToolbox: function (toolbox) {

It looks that most of the methods exposed by DevToolsShim as a "compatibility layer with the webextensions" are not implemented directly in the DevToolsShim, they seems to live in `devtools/client/framework/devtools.js` and being exposed in DevToolsShim by the methods wrappers created near the end of this file.

I'm going to ask :jdescottes to take a look at this part of the patch.

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211714/#review219232

Thanks for pinging me here. 
Only looked at the DevToolsShim part, this should be done in toolbox.js instead. 
At least the entrypoint should be there, feel free to move the implementation in another file if needed.

::: browser/components/extensions/ext-devtools-network.js:35
(Diff revision 3)
>            }).api(),
> +
> +          getHAR: function(callback) {
> +            let targetPromise = getDevToolsTargetForContext(context);
> +            return targetPromise.then(target => {
> +              return DevToolsShim.getHARFromToolbox(context.devToolsToolbox);

DevToolsShim is intended for callers that don't have a reference to devtools and are not sure if devtools are already loaded. If DevTools are not available/loaded, it should trigger the onboarding flow, or fallback to a shim implementation.

Here it looks like you already have a reference on a toolbox. The entry point (and possibly implementation) should be in toolbox.js. DevToolsShim should not be modified here.

Comment 21

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

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211714/#review219232

> DevToolsShim is intended for callers that don't have a reference to devtools and are not sure if devtools are already loaded. If DevTools are not available/loaded, it should trigger the onboarding flow, or fallback to a shim implementation.
> 
> Here it looks like you already have a reference on a toolbox. The entry point (and possibly implementation) should be in toolbox.js. DevToolsShim should not be modified here.

Thanks jdescottes! That's a very good cal, and it should also help to make this method even simpler, e.g. something like:

```
getHAR: function () {
  return context.devToolsToolbox.getHAR();
}
```

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

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

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

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

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Luca Greco [:rpl] from comment #19)
> Comment on attachment 8941393 [details]
> Bug 1311177 - Implement the devtools.network.getHAR API method;
> 
> https://reviewboard.mozilla.org/r/211712/#review219224
> 
> Thanks Honza, the WebExtensions API implementation (and testing) pieces
> looks almost ready (there are just a couple of additional comments about it,
> once we handle those, I'm going to my r+ on this patch),
All done.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #20)
> Comment on attachment 8941393 [details]
> Bug 1311177 - Implement the devtools.network.getHAR API method;
> 
> https://reviewboard.mozilla.org/r/211714/#review219232
> 
> Thanks for pinging me here. 
> Only looked at the DevToolsShim part, this should be done in toolbox.js
> instead. 
Done

(In reply to Luca Greco [:rpl] from comment #21)
> Thanks jdescottes! That's a very good cal, and it should also help to make
> this method even simpler, e.g. something like:
> 
> ```
> getHAR: function () {
>   return context.devToolsToolbox.getHAR();
> }
> ```
Yes, that's exactly what I've done (just the name is `getHARFromNetPanel`)

Thanks Luca & Julian for the review!

Honza

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211714/#review219254

Looks good to me, thanks!

::: devtools/client/framework/toolbox.js:81
(Diff revision 5)
>  
>  loader.lazyGetter(this, "registerHarOverlay", () => {
>    return require("devtools/client/netmonitor/src/har/toolbox-overlay").register;
>  });
>  
> +loader.lazyGetter(this, "buildHarLog", () => {

nit: here we could use 

loader.lazyRequireGetter(this, "buildHarLog",
  "devtools/client/netmonitor/src/har/har-builder-utils", true);

Comment 26

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

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

https://reviewboard.mozilla.org/r/211714/#review219258

Thanks! r=me with the "harLogCount fix" described below and successfull push to try.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:42
(Diff revision 5)
> -      browser.test.sendMessage("onNavigatedFired", eventCount);
> -    };
> +  };
> -    browser.devtools.network.onNavigated.addListener(listener);
> +  browser.devtools.network.onNavigated.addListener(listener);
> +
> +  let harLogCount = 0;
> +  let harListener = msg => {

it looks that harLogCount should be incremented after we checked that the msg is "getHAR".

Also, we can make this an async arrow function and use await on the browser.devtools.network.getHAR API call and early exit the listener when the msg parameter is not "getHAR":

```
let harListener = async (msg) => {
  if (msg !== "getHAR") {
    return;
  }
  
  harLogCount++;
  
  const harLog = await browser.devtools.network.getHAR();
  browser.test.sendMessage("getHAR-result", harLog);
  
  ...
}
```

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:51
(Diff revision 5)
> +        browser.test.sendMessage("getHAR-result", harLog);
> +      });
> +    }
> +    if (harLogCount === 2) {
> +      harLogCount = 0;
> +      browser.test.onMessage.removeListener(harListener);

Unfortunately, `browser.test.onMessage.removeListener` is not currently removing the listener (Bug 1428213),
it is clearly a bug on `test.onMessage.removeListener` and so I would leave the call here even if it is not currently working as intended (but I thought that was important to mention the issue to you so that you know about it, eg. in case the test fails intermittently)

::: devtools/client/netmonitor/src/har/har-builder-utils.js:30
(Diff revision 5)
> +// Consumed by WebExtension API
> +// eslint-disable-next-line no-unused-vars
> +var EXPORTED_SYMBOLS = [
> +  "buildHarLog"
> +];
> +
> +// Consumed by DevTools
> +if (typeof exports !== "undefined") {

Nit, now that this module is accessed only from "devtools consumers" which are using `require` instead of `Cu.import`, this doesn't seem to be needed anymore.

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

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

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #25)
> nit: here we could use 
> 
> loader.lazyRequireGetter(this, "buildHarLog",
>   "devtools/client/netmonitor/src/har/har-builder-utils", true);
Done

(In reply to Luca Greco [:rpl] from comment #26)
> Thanks! r=me with the "harLogCount fix" described below and successfull push
> to try.
Done

> > +// Consumed by DevTools
> > +if (typeof exports !== "undefined") {
> 
> Nit, now that this module is accessed only from "devtools consumers" which
> are using `require` instead of `Cu.import`, this doesn't seem to be needed
> anymore.
Nice catch, fixed!

Thanks,
Honza

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14e354b22ca6f0238f5052769669dbf25c26fb0

Honza

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32daec7fd5b6
Implement the devtools.network.getHAR API method; r=jdescottes,rickychien,rpl

Comment 31

[Eliza Balazs [:ebalazs_]]

Backed out changeset 32daec7fd5b6 (bug 1311177) for bc5 failures in browser/components/extensions/test/browser/test-oop-extensions/browser_ext_devtools_network.js for frequently failing

Push where the failures started: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=32daec7fd5b6bae0302ff3aa802ac5aca9d84663&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=157385417

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=157385417&repo=autoland&lineNumber=2377

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=db4cf9d2c11d38c6be1c3dc8697a3912c0315e29&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Luca: do you know why the tests fails?
Btw. I don't even see `browser/components/extensions/test/browser/test-oop-extensions/browser_ext_devtools_network.js` in the tree. Is this generated dynamically somehow (for oop testing)?

Honza

Comment 33

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

(In reply to Jan Honza Odvarko [:Honza] from comment #32)
> @Luca: do you know why the tests fails?

I took a brief a look to some of the failures from comment 31, and it looks that in all of them
the test is timing out on the second getHAR API call that is happening right after the network.onNavigated
event has been fired, and the following error is logged right before the timeout:

```
Console message: [JavaScript Error: "JSON.parse: unexpected character at line 1 column 1 of the JSON data"]
08:53:59     INFO -  normalizeError@resource://gre/modules/ExtensionCommon.jsm:403:7
08:53:59     INFO -  call/<@resource://gre/modules/ExtensionParent.jsm:777:19
08:53:59     INFO -  promise callback*call@resource://gre/modules/ExtensionParent.jsm:770:9
08:53:59     INFO -  async*receiveMessage@resource://gre/modules/ExtensionParent.jsm:676:11
```
e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=157385417&repo=autoland&lineNumber=2369

Another thing that I noticed is that in all the failures, the split webconsole seems to be opened
while the network panel is selected, e.g.:

- https://public-artifacts.taskcluster.net/FC0f326MRNapCqsI6ubd-w/0/public/test_info/mozilla-test-fail-screenshot_veple6.png

and we are now opening the split webconsole in this test (could it be because we are opening the webconsole on the toolbox initially and then we quickly switch to the network console without waiting the webconsole to load?)

Also, the tests seems to be failing consistently on windows 7 pgo builds, are you being able to reproduce the failure locally? 
(I'm also going to try to see if I can reproduce it locally using ./mach mochitest --verify)

> Btw. I don't even see
> `browser/components/extensions/test/browser/test-oop-extensions/
> browser_ext_devtools_network.js` in the tree. Is this generated dynamically
> somehow (for oop testing)?

Yes, most of the WebExtensions tests runs with the webextensions in oop mode and in single process mode,
and the test-oop-extensions directories are generated dinamically for the tests that runs also in oop mode.

As an example, when running the test with:
```
./mach mochitest browser/components/extensions/test/browser/browser_ext_devtools_network.js
```

the test runs automatically twice, because it is a mochitest-browser test file included in the browser-common.ini file

- https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser-common.ini#82

which is included in both browser.ini and browser-remote.ini:

- https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser.ini
- https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser-remote.ini

To execute only in one of the mode (oop and no oop), we can use the --tag options on the ./mach mochitest command,
e.g.

- only run with oop extensions mode disabled: ./mach mochitest --tag in-process-webextensions ...
- only run with oop extensions mode enabled: ./mach mochitest --tag remote-webextensions ...

Comment 34

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941393 [details]
Bug 1311177 - Implement the devtools.network.getHAR API method;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211714/diff/6-7/

Comment 35

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Luca!

I think I fixed the problem, so will try to land again...

Honza

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4631839df8c
Implement the devtools.network.getHAR API method; r=jdescottes,rickychien,rpl

Comment 37

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e4631839df8c

Comment 38

[Will Bamberg [:wbamberg]]

This seems to be incompatible with the Chrome implementation.

In Chrome, the object returned from getHAR is a HAR "log" (http://www.softwareishard.com/blog/har-12-spec/#log), so you could do something like:

function gotHar(har) {
  document.querySelector("#har").textContent = `Har: ${har.version}`;
  console.log(har);
}

document.querySelector("#gethar").addEventListener("click", () => {
  chrome.devtools.network.getHAR(gotHar);
});

In Firefox, the object is an object with a single property "log", which is the HAR log. So you'd have to do something like:

function gotHar(har) {
  document.querySelector("#har").textContent = `Har: ${har.log.version}`;
  console.log(har);
}

document.querySelector("#gethar").addEventListener("click", () => {
  chrome.devtools.network.getHAR(gotHar);
});

Is this incompatibility intentional?

Comment 39

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Will Bamberg [:wbamberg] from comment #38)
> This seems to be incompatible with the Chrome implementation.
> Is this incompatibility intentional?
Thanks for looking at this. The API should definitely be compatible.
I created a follow up bug 1448288

Honza

Comment 40

[Will Bamberg [:wbamberg]]

Thanks for fixing the compat issue and getting it uplifted!

I've added a page for this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.network/getHAR - please let me know if that covers it.

There is the restriction that the Network Panel must have been opened, otherwise the log is empty. Is this a Firefox-only thing? If so, I think we should capture it in a compat note.

Comment 41

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Will Bamberg [:wbamberg] from comment #40)
> Thanks for fixing the compat issue and getting it uplifted!
> 
> I've added a page for this:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.
> network/getHAR - please let me know if that covers it.
Looks great to me, thanks!

> There is the restriction that the Network Panel must have been opened,
> otherwise the log is empty. Is this a Firefox-only thing? If so, I think we
> should capture it in a compat note.
Yes, it's known limitation. It's being fixed in bug 1436665
Honza

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Release Note Request (optional, but appreciated)
[Why is this notable]: There is a new WebExtension API for DevTools extension developers
[Affects Firefox for Android]: no
[Suggested wording]: DevTools supports new WebExtension API `devtools.network.getHAR` that can be used to get data collected by the Network panel as HAR. The API is compatible with Chrome browser implementation.
[Links (documentation, blog post, etc)]:
MDN doc: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.network/getHAR

Honza

Comment 43

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This landed in Fx60, removing the release note flag.
Honza