Closed
Bug 859058
Opened 12 years ago
Closed 10 years ago
Implement "Copy as HAR" and "Save all as HAR"
Categories
(DevTools :: Netmonitor, defect, P2)
DevTools
Netmonitor
Tracking
(firefox41 fixed, relnote-firefox 41+)
RESOLVED
FIXED
Firefox 41
People
(Reporter: vporof, Assigned: Honza)
References
(Depends on 1 open bug, Blocks 3 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: [polish-backlog])
Attachments
(2 files, 6 obsolete files)
58.76 KB,
patch
|
jlong
:
review+
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
jlong
:
review+
|
Details | Diff | Splinter Review |
Apparently they are "HTTP Archive" files. Is this a standardized format? Should we find something else instead?
Comment 1•12 years ago
|
||
HAR spec:
http://www.softwareishard.com/blog/har-12-spec/
The network event actors are strongly inspired by the spec.
Comment 2•12 years ago
|
||
Or the more official:
http://w3c-test.org/webperf/specs/HAR/
Reporter | ||
Comment 3•12 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•12 years ago
|
Priority: -- → P2
Comment 5•10 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?
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•10 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
Comment 8•10 years ago
|
||
(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.
Updated•10 years ago
|
Blocks: firebug-gaps
Updated•10 years ago
|
Whiteboard: [devedition-40]
Comment 10•10 years ago
|
||
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•10 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•10 years ago
|
Assignee: nobody → odvarko
Assignee | ||
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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•10 years ago
|
||
(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)
Comment 15•10 years ago
|
||
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+
Comment 16•10 years ago
|
||
(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•10 years ago
|
||
(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)
Comment 18•10 years ago
|
||
(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•10 years ago
|
||
(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 20•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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)
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
(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
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 25•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
Oops, attaching again (not empty patch).
Honza
Attachment #8615169 -
Attachment is obsolete: true
Attachment #8615169 -
Flags: review?(jlong)
Attachment #8615171 -
Flags: review?(jlong)
Assignee | ||
Comment 29•10 years ago
|
||
Comment 31•10 years ago
|
||
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 32•10 years ago
|
||
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+
Comment 34•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e708f2ce973c
https://hg.mozilla.org/integration/fx-team/rev/507407471469
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e708f2ce973c
https://hg.mozilla.org/integration/fx-team/rev/507407471469
Whiteboard: [devedition-40] → [devedition-40][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e708f2ce973c
https://hg.mozilla.org/mozilla-central/rev/507407471469
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [devedition-40][fixed-in-fx-team] → [devedition-40]
Target Milestone: --- → Firefox 41
Comment 37•9 years ago
|
||
What needs to be done to make sure this goes in the release notes for the next version?
Comment 38•9 years ago
|
||
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
Comment 39•9 years ago
|
||
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)
Comment 40•9 years ago
|
||
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)
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Release note added to Firefox 41.0a2
Updated•9 years ago
|
Whiteboard: [devedition-40] → [polish-backlog]
Comment 42•9 years ago
|
||
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]
Comment 43•9 years ago
|
||
One mistake ..... It should be
Tested OS --- windows 8.1 32bit
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•