859058 - Implement "Copy as HAR" and "Save all as HAR"

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

Apparently they are "HTTP Archive" files. Is this a standardized format? Should we find something else instead?

Comment 1

[Mihai Sucan [:msucan]]

HAR spec:

http://www.softwareishard.com/blog/har-12-spec/

The network event actors are strongly inspired by the spec.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Or the more official:

http://w3c-test.org/webperf/specs/HAR/

Comment 3

[Victor Porof [:vporof][:vp]]

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 5

[Karl Dubost💡 :karlcow]

Having someone working on this or helping someone to work on this would be very useful. It would be useful for Bug 1026790 and many others in general, in Web Compatibility. 

How can we (Web Compatibility community) help to move this one forward?

Comment 6

[Tom]

Compressed HAR file (e.g. .zhar) option would be nice.

Not sure, if HAR 1.2 as of August 14, 2012, will maintain compatibility w/ HTTP 2.0, currently at draft 13 (see http://tools.ietf.org/html/draft-ietf-httpbis-http2-13).

Comment 7

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

(In reply to Tom from comment #6)
> Compressed HAR file (e.g. .zhar) option would be nice.
> 
> Not sure, if HAR 1.2 as of August 14, 2012, will maintain compatibility w/
> HTTP 2.0, currently at draft 13 (see
> http://tools.ietf.org/html/draft-ietf-httpbis-http2-13).
Do you have any specific compatibility issue in mind?
(I can release HAR 1.3 if knowing what's needed)

Honza

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Karl Dubost :karlcow from comment #5)
> How can we (Web Compatibility community) help to move this one forward?

If there is someone willing to work on this, then I'm sure between Victor, Honza and myself there will be a lot of guidance offered. Alternatively, you could make the case for this to the devtools EMs/PMs.

Comment 10

[Tim Nguyen :ntim]

The way firebug-next implements this ([0]) is worth checking for anyone who wants to work on this.

[0] : https://github.com/firebug/firebug.next/tree/master/lib/net/export

Comment 11

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

The plan (currently under discussion) is to integrate Firebug.next with m-c. This would also include the HAR export feature.

Honza

Comment 12

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

Attachment: bug859058-1.patch

Attaching first patch for this...

Some comments:

* Most of the code is inside devtools/netmonitor/har

* There are also two tests in netmonitor/har/test. I am expecting more tests coming in the future.

* FEATURE: Implemented: Copy As HAR (Net panel context menu)

* FEATURE: Implemented: Save As HAR (Net panel context menu)
** I have stripped out the word "All" from the label since copying just one entry isn't even implemented (and not that useful I think)
** These context menus are currently appended directly within netmonitor.xul, but I'd like to append them dynamically through an event (e.g. showContextMenu), so the entire feature is entirely encapsulated within the devtools/netmonitor/har directory. So, there wouldn't be any changes in netmonitor-view.js, netmonitor.dtd and netmonitor.xul files (outside of the dir). This is out of scope of this bug though.

* har-exporter.js: the public interface to the export feature, currently used by NetMonitorView
* har-builder.js: module responsible for building the result HAR log
* har-utils: helpers

* Two methods moved into toolkit/devtools/webconsole/network-helper, these are now shared.
** parseQueryString
** nsIURL

* There are some new preferences, detailed comment is in har-exporter.js at the top.
** All UI strings are localized (there is new har.properties file)

Try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a6d43f1721
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e8a6d43f1721

Honza

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8608691 [details] [diff] [review]
bug859058-1.patch

Review of attachment 8608691 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Few overall comments:

* Many methods could benefit a lot from some brief docs and arguments expected
* Some style nits compared to what I’m used to, more preference than required though :)
* Love the pulling out of parseQueryString and nsIURI methods — we have more dupes of these around the code base. Maybe it makes sense for this to be in toolkit/devtools/* as a utility, or browser/devtools/shared/* though, rather than web console? I don’t like requiring things from web console for URL parsing in the canvas debugger (and the perf tools can use this too, as we have the same function there). This is a pretty big patch, so if this sounds good, could you file a follow-up bug and CC me?
* Same with many of the HAR Utils — I wonder if we could pull out all the file utils (read, write, zip) into a shared utility (again, a few other tools could use this, which would be great) — same deal, if this sounds good, CC me on another bug
* Is it possible to test HAR stuff here without using the Netmonitor? Smaller and less moving parts, and could possibly be xpcshell, with also the wholistic net monitor tests for this, but could be useful/cleaner to keep the HAR tests small focusing on just the classes found there

::: browser/devtools/netmonitor/har/har-builder.js
@@ +20,5 @@
> +  return new ViewHelpers.L10N("chrome://browser/locale/devtools/har.properties");
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "NetworkHelper", function() {
> +  return devtools["require"]("devtools/toolkit/webconsole/network-helper");

Why bracket accessor instead of `devtools.require`?

@@ +23,5 @@
> +XPCOMUtils.defineLazyGetter(this, "NetworkHelper", function() {
> +  return devtools["require"]("devtools/toolkit/webconsole/network-helper");
> +});
> +
> +const harVersion = "1.1";

nit: HAR_VERSION

@@ +37,5 @@
> +}
> +
> +HarBuilder.prototype =
> +/** @lends HarBuilder */
> +{

Nit: curly should be on `HarBuilder.prototype = {` line

@@ +51,5 @@
> +      let file = items[i].attachment;
> +      log.entries.push(this.buildEntry(log, file));
> +    }
> +
> +    let deferred = defer();

nit: let { resolve, promise } = defer();

@@ +57,5 @@
> +    // Some data needs to be fetched from the backend during the
> +    // build process, so wait till all is done.
> +    all(this.promises).then(results => {
> +      deferred.resolve({ log: log });
> +    });

nit: all(this.promises).then(results => resolve({ log }));

@@ +63,5 @@
> +    return deferred.promise;
> +  },
> +
> +  buildLog: function() {
> +    let log = {};

nit: any reason this isn't done in one statement? `return {\nversion: harVersion,\n .... }`

@@ +92,5 @@
> +      return page;
> +    }
> +
> +    this.pageMap[id] = page = this.buildPage(file);
> +    log.pages.push(page); 

whitespace

::: browser/devtools/netmonitor/har/har-exporter.js
@@ +56,5 @@
> +   *
> +   * - defaultLogDir {String}: Default log directory for automated logs.
> +   *
> +   * - id {String}: ID of the page (used in the HAR file).
> +   * 

whitespace

::: browser/devtools/netmonitor/har/har-utils.js
@@ +146,5 @@
> +        .createInstance(Ci.nsIConverterOutputStream);
> +      convertor.init(foStream, "UTF-8", 0, 0);
> +
> +      // The entire jsonString can be huge so, write the data in chunks.
> +      let chunkLength = 1024 * 1204;

Should this be 1024 * 1024?

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +10,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "DevToolsUtils",
> +  "resource://gre/modules/devtools/DevToolsUtils.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "HarExporter", function() {
> +  return devtools["require"]("devtools/netmonitor/har/har-exporter.js").HarExporter;

nit: again why ["require"]?

Comment 14

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

Attachment: bug859058-2.patch

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13)
> Comment on attachment 8608691 [details] [diff] [review]
> bug859058-1.patch
> 
> Review of attachment 8608691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great! Few overall comments:
> 
> * Many methods could benefit a lot from some brief docs and arguments
> expected
I have put more comments into the code.

> * Some style nits compared to what I’m used to, more preference than
> required though :)
Feel free to create as many nit comment as you want, it's nice if the code base is using similar style.

> * Love the pulling out of parseQueryString and nsIURI methods — we have more
> dupes of these around the code base. Maybe it makes sense for this to be in
> toolkit/devtools/* as a utility, or browser/devtools/shared/* though, rather
> than web console? I don’t like requiring things from web console for URL
> parsing in the canvas debugger (and the perf tools can use this too, as we
> have the same function there). This is a pretty big patch, so if this sounds
> good, could you file a follow-up bug and CC me?
https://bugzilla.mozilla.org/show_bug.cgi?id=1168431


> * Same with many of the HAR Utils — I wonder if we could pull out all the
> file utils (read, write, zip) into a shared utility (again, a few other
> tools could use this, which would be great) — same deal, if this sounds
> good, CC me on another bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1168433

> * Is it possible to test HAR stuff here without using the Netmonitor?
> Smaller and less moving parts, and could possibly be xpcshell, with also the
> wholistic net monitor tests for this, but could be useful/cleaner to keep
> the HAR tests small focusing on just the classes found there
Yes, I like it and this should be possible.
(I'll keep the basic test in place so, we check out also the integration with the Network panel)


> 
> ::: browser/devtools/netmonitor/har/har-builder.js
> @@ +20,5 @@
> > +  return new ViewHelpers.L10N("chrome://browser/locale/devtools/har.properties");
> > +});
> > +
> > +XPCOMUtils.defineLazyGetter(this, "NetworkHelper", function() {
> > +  return devtools["require"]("devtools/toolkit/webconsole/network-helper");
> 
> Why bracket accessor instead of `devtools.require`?
Fixed (btw. it's a workaround for Add-on SDK parser)

> 
> @@ +23,5 @@
> > +XPCOMUtils.defineLazyGetter(this, "NetworkHelper", function() {
> > +  return devtools["require"]("devtools/toolkit/webconsole/network-helper");
> > +});
> > +
> > +const harVersion = "1.1";
> 
> nit: HAR_VERSION
Fixed

> 
> @@ +37,5 @@
> > +}
> > +
> > +HarBuilder.prototype =
> > +/** @lends HarBuilder */
> > +{
> 
> Nit: curly should be on `HarBuilder.prototype = {` line
But, the JSdoc @lends token is required to be befor the { character

> 
> @@ +51,5 @@
> > +      let file = items[i].attachment;
> > +      log.entries.push(this.buildEntry(log, file));
> > +    }
> > +
> > +    let deferred = defer();
> 
> nit: let { resolve, promise } = defer();
Fixed

> 
> @@ +57,5 @@
> > +    // Some data needs to be fetched from the backend during the
> > +    // build process, so wait till all is done.
> > +    all(this.promises).then(results => {
> > +      deferred.resolve({ log: log });
> > +    });
> 
> nit: all(this.promises).then(results => resolve({ log }));
Fixed

> 
> @@ +63,5 @@
> > +    return deferred.promise;
> > +  },
> > +
> > +  buildLog: function() {
> > +    let log = {};
> 
> nit: any reason this isn't done in one statement? `return {\nversion:
> harVersion,\n .... }`
Fixed

> 
> @@ +92,5 @@
> > +      return page;
> > +    }
> > +
> > +    this.pageMap[id] = page = this.buildPage(file);
> > +    log.pages.push(page); 
> 
> whitespace
Fixed

> 
> ::: browser/devtools/netmonitor/har/har-exporter.js
> @@ +56,5 @@
> > +   *
> > +   * - defaultLogDir {String}: Default log directory for automated logs.
> > +   *
> > +   * - id {String}: ID of the page (used in the HAR file).
> > +   * 
> 
> whitespace
Fixed

> 
> ::: browser/devtools/netmonitor/har/har-utils.js
> @@ +146,5 @@
> > +        .createInstance(Ci.nsIConverterOutputStream);
> > +      convertor.init(foStream, "UTF-8", 0, 0);
> > +
> > +      // The entire jsonString can be huge so, write the data in chunks.
> > +      let chunkLength = 1024 * 1204;
> 
> Should this be 1024 * 1024?
Fixed


> 
> ::: browser/devtools/netmonitor/netmonitor-view.js
> @@ +10,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this, "DevToolsUtils",
> > +  "resource://gre/modules/devtools/DevToolsUtils.jsm");
> > +
> > +XPCOMUtils.defineLazyGetter(this, "HarExporter", function() {
> > +  return devtools["require"]("devtools/netmonitor/har/har-exporter.js").HarExporter;
> 
> nit: again why ["require"]?
Fixed


I have also simplified the code a bit.


Thanks for the review Jordan!

Honza

Comment 15

[James Long (:jlongster)]

Comment on attachment 8610627 [details] [diff] [review]
bug859058-2.patch

Review of attachment 8610627 [details] [diff] [review]:
-----------------------------------------------------------------

Looks very nicely laid out with tests and everything, great job. Just saw a few small things that are mostly questions.

Sorry for the late review, I've been trying to find time to read through this more thoroughly and actually understand the code. Wasn't able to do that fully, but I looked through it best I could.

::: browser/devtools/netmonitor/har/har-builder.js
@@ +30,5 @@
> + * This object is responsible for building HAR file. See HAR spec:
> + * https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/HAR/Overview.html
> + * http://www.softwareishard.com/blog/har-12-spec/
> + *
> + * @param {Object} options configuration object

Do you see other doc styles like this in our codebase? In the code I work it would be `@param Object options` with an explanation on the line below. You used that style in `har-exporter.js`. I know we aren't too consistent about our doc styles, but we should probably pick one and go with it. If we are using this style in newer code, that's fine, but if everything use doesn't have the curly-brace might be good to remove that.

@@ +46,5 @@
> + *   bodies in the result data structure.
> + */
> +var HarBuilder = function(options) {
> +  this.options = options;
> +  this.pageMap = [];

nit: if these are private, prefix them with underscores. It's the style we use everywhere else.

@@ +50,5 @@
> +  this.pageMap = [];
> +}
> +
> +HarBuilder.prototype =
> +/** @lends HarBuilder */

What is the `@lends` comment? I've never seen that before (but I only work on the debugger and net monitor and sometimes console, all which are older code)

@@ +433,5 @@
> + * See also HAR Schema: http://janodvarko.cz/har/viewer/
> + *
> + * @param date {Date} The date object we want to convert.
> + */
> +function dateToJSON(date) {

How come `JSON.stringify(date)` doesn't work?

Comment 16

[Sebastian Zartner [:sebo]]

(In reply to James Long (:jlongster) from comment #15)
> ::: browser/devtools/netmonitor/har/har-builder.js
> @@ +30,5 @@
> > + * This object is responsible for building HAR file. See HAR spec:
> > + * https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/HAR/Overview.html
> > + * http://www.softwareishard.com/blog/har-12-spec/
> > + *
> > + * @param {Object} options configuration object
> 
> Do you see other doc styles like this in our codebase? In the code I work it
> would be `@param Object options` with an explanation on the line below. You
> used that style in `har-exporter.js`. I know we aren't too consistent about
> our doc styles, but we should probably pick one and go with it. If we are
> using this style in newer code, that's fine, but if everything use doesn't
> have the curly-brace might be good to remove that.

To answer this for Honza, this is the style prescribed by JSDoc[1].

> @@ +50,5 @@
> > +  this.pageMap = [];
> > +}
> > +
> > +HarBuilder.prototype =
> > +/** @lends HarBuilder */
> 
> What is the `@lends` comment? I've never seen that before (but I only work
> on the debugger and net monitor and sometimes console, all which are older
> code)

`@lends` allows you to document all the members of an object as if they were members of a symbol[2].

Sebastian

[1] http://usejsdoc.org/tags-param.html
[2] http://usejsdoc.org/tags-lends.html

Comment 17

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

Attachment: bug859058-3.patch

(In reply to James Long (:jlongster) from comment #15)
> Comment on attachment 8610627 [details] [diff] [review]
> bug859058-2.patch
> 
> Review of attachment 8610627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks very nicely laid out with tests and everything, great job. Just saw a
> few small things that are mostly questions.
Great, thanks!


> Sorry for the late review, I've been trying to find time to read through
> this more thoroughly and actually understand the code. Wasn't able to do
> that fully, but I looked through it best I could.
I know, it's relatively big patch, thanks for the effort James!

> ::: browser/devtools/netmonitor/har/har-builder.js
> @@ +30,5 @@
> > + * This object is responsible for building HAR file. See HAR spec:
> > + * https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/HAR/Overview.html
> > + * http://www.softwareishard.com/blog/har-12-spec/
> > + *
> > + * @param {Object} options configuration object
> 
> Do you see other doc styles like this in our codebase? In the code I work it
> would be `@param Object options` with an explanation on the line below. You
> used that style in `har-exporter.js`. I know we aren't too consistent about
> our doc styles, but we should probably pick one and go with it. If we are
> using this style in newer code, that's fine, but if everything use doesn't
> have the curly-brace might be good to remove that.
I was just chatting with Patrick on IRC

pbro	my suggestion: make it look like the other jsdoc comments in the same file
pbro	that's the most important
pbro	unless it's a new file, then use @param {Type} name Description.

So, I am keeping the curly brackets there.

> 
> @@ +46,5 @@
> > + *   bodies in the result data structure.
> > + */
> > +var HarBuilder = function(options) {
> > +  this.options = options;
> > +  this.pageMap = [];
> 
> nit: if these are private, prefix them with underscores. It's the style we
> use everywhere else.
Fixed

> 
> @@ +50,5 @@
> > +  this.pageMap = [];
> > +}
> > +
> > +HarBuilder.prototype =
> > +/** @lends HarBuilder */
> 
> What is the `@lends` comment? I've never seen that before (but I only work
> on the debugger and net monitor and sometimes console, all which are older
> code)
JSDocs recommended syntax (see also Sebastian's comment #16)


> @@ +433,5 @@
> > + * See also HAR Schema: http://janodvarko.cz/har/viewer/
> > + *
> > + * @param date {Date} The date object we want to convert.
> > + */
> > +function dateToJSON(date) {
> 
> How come `JSON.stringify(date)` doesn't work?
I wanted to use that but it doesn't return proper time zone offset
An example:

The helper dateToJSON() method returns:    2015-05-29T16:10:30.424+02:00
JSON.stringify() or Date.toJSON() returns: 2015-05-29T14:10:30.424Z

So, I am keeping the helper in place for now
(unless there is another better solution)

---

Finally, I have yet fixed the two Network context menu labels.
It's now "Save All As HAR" and "Copy All As HAR"
(included in the new patch)

Thanks for the review James!

Honza

Comment 18

[James Long (:jlongster)]

(In reply to Jan Honza Odvarko [:Honza] from comment #17)
> > How come `JSON.stringify(date)` doesn't work?
> I wanted to use that but it doesn't return proper time zone offset
> An example:
> 
> The helper dateToJSON() method returns:    2015-05-29T16:10:30.424+02:00
> JSON.stringify() or Date.toJSON() returns: 2015-05-29T14:10:30.424Z
> 
> So, I am keeping the helper in place for now
> (unless there is another better solution)

Ok cool, could you add a small comment to that function's docs with this? That would make it clear why the JSON method doesn't work.

Comment 19

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

Attachment: bug859058-4.patch

(In reply to James Long (:jlongster) from comment #18)
> Ok cool, could you add a small comment to that function's docs with this?
> That would make it clear why the JSON method doesn't work.
Done, updated patch attached.

Honza

Comment 20

[James Long (:jlongster)]

Comment on attachment 8613977 [details] [diff] [review]
bug859058-4.patch

Review of attachment 8613977 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, I skimmed each file and made just a few more comments. While I don't fully understand all the code, if it works you can land it. No need to ask for re-review unless you make bigger changes.

The comment about error handling applies to all this code in general: make sure errors are properly reported somehow it something fails.

::: browser/devtools/netmonitor/har/har-builder.js
@@ +50,5 @@
> +  this._pageMap = [];
> +}
> +
> +HarBuilder.prototype =
> +/** @lends HarBuilder */

nit: does this have to exists between the `=` and the `{`? It's against our style guide to put brackets on a new line.

::: browser/devtools/netmonitor/har/har-exporter.js
@@ +79,5 @@
> +      return resolve();
> +    }
> +
> +    return this.fetchHarData(options).then(jsonString => {
> +      HarUtils.saveToFile(file, jsonString, options.compress);

This function returns false if it wasn't successful, should we check that and show the error somewhere?

Comment 21

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

Attachment: bug859058-5.patch

(In reply to James Long (:jlongster) from comment #20)
> Comment on attachment 8613977 [details] [diff] [review]
> bug859058-4.patch
> 
> Review of attachment 8613977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great, I skimmed each file and made just a few more comments. While I
> don't fully understand all the code, if it works you can land it. No need to
> ask for re-review unless you make bigger changes.
ok

> The comment about error handling applies to all this code in general: make
> sure errors are properly reported somehow it something fails.
I fixed two more places

> ::: browser/devtools/netmonitor/har/har-builder.js
> @@ +50,5 @@
> > +  this._pageMap = [];
> > +}
> > +
> > +HarBuilder.prototype =
> > +/** @lends HarBuilder */
> 
> nit: does this have to exists between the `=` and the `{`? It's against our
> style guide to put brackets on a new line.
Since the tag isn't use anywhere else I've removed it. The brackets is not on the same line (following the style guide).
We can discuss using @lends at different place.

> ::: browser/devtools/netmonitor/har/har-exporter.js
> @@ +79,5 @@
> > +      return resolve();
> > +    }
> > +
> > +    return this.fetchHarData(options).then(jsonString => {
> > +      HarUtils.saveToFile(file, jsonString, options.compress);
> 
> This function returns false if it wasn't successful, should we check that
> and show the error somewhere?
Fixed, there is an error message now.

Honza

Comment 22

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

Attachment: bug859058-6.patch

Yet one more error handler added.

James, can you have a quick look yet please.

Honza

Comment 23

[James Long (:jlongster)]

(In reply to Jan Honza Odvarko [:Honza] from comment #22)
> Created attachment 8614720 [details] [diff] [review]
> bug859058-6.patch
> 
> Yet one more error handler added.
> 
> James, can you have a quick look yet please.
> 
> Honza

It's hard to review the entire patch again, could you either make a new patch with just the new changes or point out specific places to look?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #23)
> (In reply to Jan Honza Odvarko [:Honza] from comment #22)
> > Created attachment 8614720 [details] [diff] [review]
> > bug859058-6.patch
> > 
> > Yet one more error handler added.
> > 
> > James, can you have a quick look yet please.
> > 
> > Honza
> 
> It's hard to review the entire patch again, could you either make a new
> patch with just the new changes or point out specific places to look?

You can see the interdiff here: https://bugzilla.mozilla.org/attachment.cgi?oldid=8614595&action=interdiff&newid=8614720&headers=1

Comment 25

[James Long (:jlongster)]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> (In reply to James Long (:jlongster) from comment #23)
> > (In reply to Jan Honza Odvarko [:Honza] from comment #22)
> > > Created attachment 8614720 [details] [diff] [review]
> > > bug859058-6.patch
> > > 
> > > Yet one more error handler added.
> > > 
> > > James, can you have a quick look yet please.
> > > 
> > > Honza
> > 
> > It's hard to review the entire patch again, could you either make a new
> > patch with just the new changes or point out specific places to look?
> 
> You can see the interdiff here:
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8614595&action=interdiff&newid=8614720&headers=1

Oh, I didn't know you could do that.

I still like adding additional patches because I can do the review right in that diff. Thanks for letting me know about that though.

Comment 26

[James Long (:jlongster)]

Comment on attachment 8614720 [details] [diff] [review]
bug859058-6.patch

Review of attachment 8614720 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/netmonitor/har/har-exporter.js
@@ +152,5 @@
> +        if (!har.log.entries.length && !options.forceExport) {
> +          deferred.resolve();
> +        }
> +        deferred.resolve(har);
> +      });

this promise does not have a catch handler and is not returned. What is the point of the `try` statement? Seems like you are mainly trying to catch errors from this promise which try/catch is not going to do.

Comment 27

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

Attachment: bug859058-error-handling-1.patch

(In reply to James Long (:jlongster) from comment #26)
> this promise does not have a catch handler and is not returned. What is the
> point of the `try` statement? Seems like you are mainly trying to catch
> errors from this promise which try/catch is not going to do.
I simplified the code, see the new patch with changes.

Honza

Comment 28

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

Attachment: bug859058-error-handling-2.patch

Oops, attaching again (not empty patch).

Honza

Comment 29

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

Try push

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

Honza

Comment 31

[James Long (:jlongster)]

Comment on attachment 8615171 [details] [diff] [review]
bug859058-error-handling-2.patch

Review of attachment 8615171 [details] [diff] [review]:
-----------------------------------------------------------------

great!

Comment 32

[James Long (:jlongster)]

Comment on attachment 8614720 [details] [diff] [review]
bug859058-6.patch

Review of attachment 8614720 [details] [diff] [review]:
-----------------------------------------------------------------

This r? must have been leftover from before. It's mostly focusing on error handling which is in the other patch now.

Comment 33

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

Thanks for the review James!

Honza

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e708f2ce973c
https://hg.mozilla.org/integration/fx-team/rev/507407471469

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/e708f2ce973c
https://hg.mozilla.org/integration/fx-team/rev/507407471469

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e708f2ce973c
https://hg.mozilla.org/mozilla-central/rev/507407471469

Comment 37

[Josh Triplett]

What needs to be done to make sure this goes in the release notes for the next version?

Comment 38

[Sebastian Zartner [:sebo]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Allowing to export network data to use process it in external editors.
[Suggested wording]: Network requests can be exported in HAR format
[Links (documentation, blog post, etc)]: 'HAR' can be linked to http://www.softwareishard.com/blog/har-12-spec/; docs on MDN still need to be written.

Sebastian

Comment 39

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Context_menu to mention this.
Does that look all right, Sebastian?

Comment 40

[Sebastian Zartner [:sebo]]

Looks good to me. I didn't even know Honza put his HAR format into a W3C spec.

Just a little note aside: Please add comments to your revisions! It makes it easier to skim through the changes.

Sebastian

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Release note added to Firefox 41.0a2

Comment 42

[Saheda Reza [:Antora]]

I didn't found this feature on firefox nightly Version  23.0a1

It's fixed & I verified the implementation of this feature on Latest nightly

Build ID 	20150901030226
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0

Tested OS-- windows 32bit

Comment 43

[Saheda Reza [:Antora]]

One mistake ..... It should be

Tested OS --- windows 8.1 32bit