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

RESOLVED FIXED in Firefox 41

Status

P2
normal
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: vporof, Assigned: Honza)

Tracking

(Depends on: 2 bugs, Blocks: 4 bugs, {dev-doc-complete})

unspecified
Firefox 41
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, relnote-firefox 41+)

Details

(Whiteboard: [polish-backlog])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
Apparently they are "HTTP Archive" files. Is this a standardized format? Should we find something else instead?
HAR spec:

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

The network event actors are strongly inspired by the spec.
(Reporter)

Comment 3

6 years ago
Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.
Component: Developer Tools → Developer Tools: Netmonitor
Summary: [netmonitor] Implement "Copy as HAR" and "Save all as HAR" → Implement "Copy as HAR" and "Save all as HAR"
(Reporter)

Updated

6 years ago
Priority: -- → P2
(Reporter)

Updated

5 years ago
Duplicate of this bug: 992604

Comment 5

4 years ago
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

4 years ago
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).
(Assignee)

Comment 7

4 years ago
(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
(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.
Duplicate of this bug: 1034137
Whiteboard: [devedition-40]
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
(Assignee)

Comment 11

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

Honza
(Assignee)

Updated

4 years ago
Assignee: nobody → odvarko
(Assignee)

Updated

4 years ago
Blocks: 1167080
(Assignee)

Comment 12

4 years ago
Created attachment 8608691 [details] [diff] [review]
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/jodvarko@mozilla.com-e8a6d43f1721

Honza
Attachment #8608691 - Flags: review?(jsantell)
Attachment #8608691 - Flags: review?(jlong)
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"]?
Attachment #8608691 - Flags: review?(jsantell) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8610627 [details] [diff] [review]
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
Attachment #8608691 - Attachment is obsolete: true
Attachment #8608691 - Flags: review?(jlong)
Attachment #8610627 - Flags: review?(jlong)
(Assignee)

Updated

3 years ago
Blocks: 1168431
(Assignee)

Updated

3 years ago
Blocks: 1168433
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?
Attachment #8610627 - Flags: review?(jlong) → review+
(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
(Assignee)

Comment 17

3 years ago
Created attachment 8612871 [details] [diff] [review]
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
Attachment #8610627 - Attachment is obsolete: true
Attachment #8612871 - Flags: review?(jlong)
(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.
(Assignee)

Comment 19

3 years ago
Created attachment 8613977 [details] [diff] [review]
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
Attachment #8612871 - Attachment is obsolete: true
Attachment #8612871 - Flags: review?(jlong)
Attachment #8613977 - Flags: review?(jlong)
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?
Attachment #8613977 - Flags: review?(jlong) → review+
(Assignee)

Comment 21

3 years ago
Created attachment 8614595 [details] [diff] [review]
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
Attachment #8613977 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
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
Attachment #8614595 - Attachment is obsolete: true
Attachment #8614720 - Flags: review?(jlong)
(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?
(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
Status: NEW → ASSIGNED
(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 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.
(Assignee)

Comment 27

3 years ago
Created attachment 8615169 [details] [diff] [review]
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
Attachment #8615169 - Flags: review?(jlong)
(Assignee)

Comment 28

3 years ago
Created attachment 8615171 [details] [diff] [review]
bug859058-error-handling-2.patch

Oops, attaching again (not empty patch).

Honza
Attachment #8615169 - Attachment is obsolete: true
Attachment #8615169 - Flags: review?(jlong)
Attachment #8615171 - Flags: review?(jlong)
Duplicate of this bug: 1172078
Comment on attachment 8615171 [details] [diff] [review]
bug859058-error-handling-2.patch

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

great!
Attachment #8615171 - Flags: review?(jlong) → review+
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.
Attachment #8614720 - Flags: review?(jlong) → review+
(Assignee)

Comment 33

3 years ago
Thanks for the review James!

Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e708f2ce973c
https://hg.mozilla.org/mozilla-central/rev/507407471469
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [devedition-40][fixed-in-fx-team] → [devedition-40]
Target Milestone: --- → Firefox 41
See Also: → bug 1174091
Depends on: 1174095
Depends on: 1174100

Comment 37

3 years ago
What needs to be done to make sure this goes in the release notes for the next version?
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
relnote-firefox: --- → ?
Keywords: dev-doc-needed
I've updated https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Context_menu to mention this.
Does that look all right, Sebastian?
Flags: needinfo?(sebastianzartner)
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
Flags: needinfo?(sebastianzartner)
Keywords: dev-doc-needed → dev-doc-complete
Release note added to Firefox 41.0a2
relnote-firefox: ? → 41+

Updated

3 years ago
Depends on: 1185293
Whiteboard: [devedition-40] → [polish-backlog]
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
QA Whiteboard: [bugday-20150902]
One mistake ..... It should be

Tested OS --- windows 8.1 32bit
Duplicate of this bug: 588751

Updated

2 years ago
Depends on: 1337864

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.