Closed
Bug 1434333
Opened 7 years ago
Closed 7 years ago
Import an existing HAR file
Categories
(DevTools :: Netmonitor, enhancement, P2)
DevTools
Netmonitor
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: Honza, Assigned: Honza)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
It should be possible to import an existing *.har file into the Network monitor panel. I can be initially done by dragging and dropping har file onto the Net panel. Later when we work on Bug 1403530 - we might use the new button for export & import. Honza
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8948417 [details] Bug 1434333 - Import an existing HAR file; https://reviewboard.mozilla.org/r/217878/#review223632 Static analysis found 2 defects in this patch. - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint check path/to/file` (Python/Javascript/wpt) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/netmonitor/src/components/App.js:11 (Diff revision 1) > > -const { createFactory } = require("devtools/client/shared/vendor/react"); > +const { Component, createFactory } = require("devtools/client/shared/vendor/react"); > const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); > const dom = require("devtools/client/shared/vendor/react-dom-factories"); > const { connect } = require("devtools/client/shared/vendor/react-redux"); > +const Actions = require("../actions/index"); Error: 'Actions' is assigned a value but never used. [eslint: no-unused-vars] ::: devtools/client/netmonitor/src/components/App.js:139 (Diff revision 1) > + fileReader.onloadend = () => { > + callback(fileReader.result); > -}; > + }; > + fileReader.readAsText(file); > + } > + } Error: Missing semicolon. [eslint: semi]
Assignee | ||
Comment 3•7 years ago
|
||
@Dragana: can you remind me why bug 1384679 introduced `timing offsets`? HAR spec [1] (the spec for data collected by network panel) is defining 'timings', but there are no offsets. Why we had to add the set of offsets and send it from the backend instead of doing the calculation on the client side? The `timings` HAR section should be enough to get all timing data we need ... Honza [1] http://www.softwareishard.com/blog/har-12-spec/#timings
Flags: needinfo?(dd.mozilla)
Assignee | ||
Comment 4•7 years ago
|
||
Just to explain the context a bit more. I am exporting all the data from Net panel (as HAR) and importing it again (import is being implemented in this bug). But, since 'offsets' are not part of the HAR spec I am getting wrong timing info after import... Honza
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Patch from bug 1311171 is needed since there is a small collision (but it doesn't depend on the functionality). Honza
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8948417 [details] Bug 1434333 - Import an existing HAR file; https://reviewboard.mozilla.org/r/217878/#review224184 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/netmonitor/src/components/App.js:162 (Diff revision 2) > - // True if the stats panel is opened. > + callback(fileReader.result); > - statisticsOpen: PropTypes.bool.isRequired, > -}; > + }; > + fileReader.readAsText(file); > + } > + } Error: Missing semicolon. [eslint: semi]
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8948417 [details] Bug 1434333 - Import an existing HAR file; https://reviewboard.mozilla.org/r/217878/#review225502 Sorry for the delay in the review. It works great overall, but another review would be helpful I think. * If I record HAR with cached request and then import it, I get this exception when selecting the cached requests and opening their response sidebar: ResponsePanel.js, line 145: TypeError: mimeType is undefined Also their "transfered side" is -1B, may be we could assume this is cached request when it is -1? It could be worth looking at what other browsers do. ::: devtools/client/netmonitor/src/components/App.js:98 (Diff revision 3) > + if (har) { > + this.appendPreview(har); > + } > + }); > + reader(); > + } In HarImporter.doImport, you do: this.actions.clearRequests(); So I'm not sure there is any value in importing more than one HAR file at a time? ::: devtools/client/netmonitor/src/components/App.js:111 (Diff revision 3) > + try { > + let importer = new HarImporter(this.props.actions); > + importer.import(har); > + } catch (err) { > + if (openSplitConsole) { > + openSplitConsole(err); This is surprising, as you won't see any error if you don't have the split console opened. (I rarely have it opened outside of debugger) Shouldn't we use notification bar? If the error is one or two lines, that's a common way to report errors in toolbox. Like here in inspector: https://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#239-246 webconsole: https://searchfox.org/mozilla-central/source/devtools/client/webconsole/utils.js#257-265 storage: https://searchfox.org/mozilla-central/source/devtools/client/storage/ui.js#1268 styleeditor: https://searchfox.org/mozilla-central/source/devtools/client/styleeditor/styleeditor-panel.js#87 ::: devtools/client/netmonitor/src/components/App.js:145 (Diff revision 3) > - }) > + }), > + div({className: "dropHarFiles"}, > + div({}, DROP_HAR_FILES) > + ) > - ) > + ) > - ); > + ); When dropping over the StatisticsPanel, it looks broken. The pie chart and text all reset to gray/"loading", but never updates. Shouldn't you put all this drag'n drop/har importer code inside MonitorPanel? Also, wouldn't it be better to have a dedicated component, similar to VisibilityHandler, which would only implement this? ::: devtools/client/netmonitor/src/constants.js:321 (Diff revision 3) > + "connect", > + "ssl", > + "send", > + "wait", > + "receive", > +]; Do not hesitate to submit multiples patches to mozreview. This change could have been done in a dedicated changeset and help reviewing this particular change only once. ::: devtools/client/netmonitor/src/har/har-builder.js:115 (Diff revision 3) > > entry.request = await this.buildRequest(file); > entry.response = await this.buildResponse(file); > entry.cache = this.buildCache(file); > entry.timings = eventTimings ? eventTimings.timings : {}; > + entry._securityState = file.securityState; Why do you prefix this with `_`? Could you test this new field in existing har tests? It is not clear what should be this attribute. ::: devtools/client/netmonitor/src/har/har-builder.js:219 (Diff revision 3) > buildCookies: function (input) { > if (!input) { > return []; > } > > - return this.buildNameValuePairs(input.cookies); > + return this.buildNameValuePairs(input.cookies || input); This is really surprising, isn't that completely broken? I mean, is `input.cookies` ever defined? It would be great to have this tested. ::: devtools/client/netmonitor/src/har/har-importer.js:31 (Diff revision 3) > + this.doImport(json); > + }, > + > + doImport: function (har) { > + this.actions.clearRequests(); > + Should you warn when har.log.version < 1.2? ::: devtools/client/netmonitor/src/har/har-importer.js:127 (Diff revision 3) > + time: startedMillis + onContentLoad, > + }); > + this.actions.addTimingMarker({ > + name: "dom-complete", > + time: startedMillis + onLoad, > + }); Should you call addTimingMarker when pageTimings.onContentLoad / pageTimings.onLoad are equal to 0/-1? ::: devtools/client/netmonitor/src/har/test/browser_net_har_import.js:11 (Diff revision 3) > +/** > + * Tests for importing HAR data. > + */ > +add_task(function* () { > + let { tab, monitor } = yield initNetMonitor( > + HAR_EXAMPLE_URL + "html_har_post-data-test-page.html"); It would be great to complexify this test page or fork it to cover more usecases: * cookies * headers * cached requests
Attachment #8948417 -
Flags: review?(poirot.alex)
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8948417 [details] Bug 1434333 - Import an existing HAR file; https://reviewboard.mozilla.org/r/217878/#review226796 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/netmonitor/src/selectors/timing-markers.js:10 (Diff revision 4) > "use strict"; > > function getDisplayedTimingMarker(state, marker) { > - return state.timingMarkers[marker] - state.requests.firstStartedMillis; > + let value = state.timingMarkers[marker]; > + if (value == -1) { > + return value Error: Missing semicolon. [eslint: semi]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #9) > Comment on attachment 8948417 [details] > 1434333 - Import an existing HAR file; > > https://reviewboard.mozilla.org/r/217878/#review225502 > > Sorry for the delay in the review. Np, thanks for the review! > * If I record HAR with cached request and then import it, I get this > exception when selecting the cached requests and opening their response > sidebar: > ResponsePanel.js, line 145: TypeError: mimeType is undefined > Also their "transfered side" is -1B, may be we could assume this is cached > request when it is -1? > It could be worth looking at what other browsers do. This is not reproducible anymore (according to my testing and our chat). > In HarImporter.doImport, you do: > this.actions.clearRequests(); > So I'm not sure there is any value in importing more than one HAR file at a > time? This might be useful, but requires some UI/UX analysis. E.g. we might want to display list of pages like HAR Viewer [1] does and visually separate requests that belong to different pages. For now, It's enough to import just one HAR file (just like Chrome does). [1] http://www.softwareishard.com/har/viewer/?path=examples/inline-scripts-block.har In any case I filled bug 1438792, so it isn't forgotten. > This is surprising, as you won't see any error if you don't have the split > console opened. (I rarely have it opened outside of debugger) I can't reproduce this. But there are bugs I am seeing (so, perhaps these are what you are talking about): 1) The repeat number isn't displayed 2) The log should be an error (JSON parsing error) It's related to the Console panel. I crated a report for this - bug 1438482. Patch already attached. > Shouldn't we use notification bar? The log represents real JSON parsing error and it should be consistent with how to display other errors. Note that e.g. source maps also display warnings as real warnings. > When dropping over the StatisticsPanel, it looks broken. The pie chart and > text all reset to gray/"loading", but never updates. > Shouldn't you put all this drag'n drop/har importer code inside MonitorPanel? > Also, wouldn't it be better to have a dedicated component, similar to > VisibilityHandler, which would only implement this? Yes, good point, done. > > + entry._securityState = file.securityState; > > Why do you prefix this with `_`? _securityState is custom filed in the HAR format and those must use `_` prefix according to the spec. > Could you test this new field in existing har tests? Done > I mean, is `input.cookies` ever defined? Yes, but it's one of the things that should be cleaned up at some point (not related to import though) > It would be great to have this tested. Done > Should you warn when har.log.version < 1.2? No, we actually export HAR 1.1 at this point (we should upgrade to 1.2 at some point). > Should you call addTimingMarker when pageTimings.onContentLoad / > pageTimings.onLoad are equal to 0/-1? Good catch, adding only if time > 0 > It would be great to complexify this test page or fork it to cover more > usecases: > * cookies > * headers > * cached requests Done I also reported bug 1438831 for the total time issue. Honza
Comment 14•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #3) > @Dragana: can you remind me why bug 1384679 introduced `timing offsets`? > > HAR spec [1] (the spec for data collected by network panel) is defining > 'timings', but there are no offsets. Why we had to add the set of offsets > and send it from the backend instead of doing the calculation on the client > side? The `timings` HAR section should be enough to get all timing data we > need ... > > > Honza > > [1] http://www.softwareishard.com/blog/har-12-spec/#timings without TCP Fast Open and TLS early data each timing period starts when the previous one ends, e.g. sending data starts when connecting is finished. we did not need offsets with TCP Fast Open and TLS early data sending data can start at the same time as connect (we can send data on TCP syn packet). also tls handshake can carry application data thereby overlapping a sending data period and tls handshake period. Timing offsets are start time of each period.
Flags: needinfo?(dd.mozilla)
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8948417 [details] Bug 1434333 - Import an existing HAR file; https://reviewboard.mozilla.org/r/217878/#review227382 The drag'n drop overlay is not longer displayed? Could it be linux specific? Related to React 16? Otherwise it works great now! ::: devtools/client/netmonitor/src/components/DropHarHandler.js:82 (Diff revision 5) > + if (har) { > + this.appendPreview(har); > + } > + }); > + reader(); > + } Until you figure out the UX to handle more than one HAR file, you should process only one and ignore the others. Processing the others is only a waste of computation as only the latest will be displayed. ::: devtools/client/netmonitor/src/components/DropHarHandler.js:95 (Diff revision 5) > + try { > + let importer = new HarImporter(this.props.actions); > + importer.import(har); > + } catch (err) { > + if (openSplitConsole) { > + openSplitConsole(err); It may help to log a first line with a message like: "Error while processing HAR file: ..." Then the error here. ::: devtools/client/netmonitor/src/components/DropHarHandler.js:112 (Diff revision 5) > + onDragLeave: this.onDragLeave, > + onDrop: this.onDrop}, > + this.props.children, > + div({className: "dropHarFiles"}, > + div({}, DROP_HAR_FILES) > + ) The drag'n drop overlay isn't displayed anymore? Could it be broken only on linux? ::: devtools/client/netmonitor/src/components/DropHarHandler.js:131 (Diff revision 5) > + fileReader.onloadend = () => { > + callback(fileReader.result); > + }; > + fileReader.readAsText(file); > + } > + }; It would be easier to follow by using Promise here: return Promise.resolve(file.getAsText("")); return new Promise(resolve => { let fileReader = new FileReader(); fileReader.onloadend = () => { resolve(fileReader.result); }; }); Also, could you add a comment explaining why there is two codepaths here. Is it because of launchpad? ::: devtools/client/netmonitor/src/har/har-builder.js:115 (Diff revision 5) > > entry.request = await this.buildRequest(file); > entry.response = await this.buildResponse(file); > entry.cache = this.buildCache(file); > entry.timings = eventTimings ? eventTimings.timings : {}; > + entry._securityState = file.securityState; Please add a comment explaining why this particular field is "underscored". ::: devtools/client/netmonitor/src/har/har-builder.js:219 (Diff revision 5) > buildCookies: function (input) { > if (!input) { > return []; > } > > - return this.buildNameValuePairs(input.cookies); > + return this.buildNameValuePairs(input.cookies || input); Could you file a bug about this, it is still unclear what is going on here.
Attachment #8948417 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 16•7 years ago
|
||
Thanks for the review! (In reply to Alexandre Poirot [:ochameau] from comment #15) > Until you figure out the UX to handle more than one HAR file, you should > process only one and ignore the others. Processing the others is only a > waste of computation as only the latest will be displayed. Done > It may help to log a first line with a message like: > "Error while processing HAR file: ..." > Then the error here. Done > The drag'n drop overlay isn't displayed anymore? > Could it be broken only on linux? Fixed > It would be easier to follow by using Promise here: > return Promise.resolve(file.getAsText("")); > return new Promise(resolve => { > let fileReader = new FileReader(); > fileReader.onloadend = () => { > resolve(fileReader.result); > }; > }); Done > Also, could you add a comment explaining why there is two codepaths here. Is > it because of launchpad? Yes (when net panel runs within a tab on different browser) I tested again and it looks like FileReader is supported by both Firefox and Chrome, so I used only FileReader + new comment. > Please add a comment explaining why this particular field is "underscored". Done > > - return this.buildNameValuePairs(input.cookies); > > + return this.buildNameValuePairs(input.cookies || input); > > Could you file a bug about this, it is still unclear what is going on here. Bug 1440275 - Properly store cookies in internal Net panel data structure Honza
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8948417 [details] Bug 1434333 - Import an existing HAR file; https://reviewboard.mozilla.org/r/217878/#review228988 I have a couple of minor comments, but it works great now. Thanks Honza! ::: devtools/client/netmonitor/initializer.js:74 (Diff revision 6) > }; > > + const openSplitConsole = (err) => { > + toolbox.openSplitConsole().then(() => { > + toolbox.target.logErrorInPage("Error while processing HAR file: " > + + err.message, "har"); The error message prefix should rather be in DropHarHandler.js's appendPreview. You hand over `openSplitConsole` to many React components that may start using it now. If they do, they won't want to use this HAR import specific prefix. ::: devtools/client/netmonitor/src/components/DropHarHandler.js:83 (Diff revision 6) > + let reader = getFileReader(file).then(har => { > + if (har) { > + this.appendPreview(har); > + } > + }); > + reader(); This line looks like a lefover. Also, you may use async/await instead of using .then(). ::: devtools/client/netmonitor/src/components/DropHarHandler.js:109 (Diff revision 6) > + render() { > + return ( > + div({ > + onDragEnter: this.onDragEnter, > + onDragOver: this.onDragOver, > + onDragLeave: this.onDragLeave, With current patch, the drag'n drop overlay blinks as you move over netmonitor individual DOM elements, like column headers. I don't know much about drag/drop events, but firefox codebase seems to use dragexit instead of dragleave: https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#658-661 And if you replace leave by exit here, it no longer blinks. ::: devtools/client/netmonitor/src/components/DropHarHandler.js:122 (Diff revision 6) > + } > +} > + > +// Helpers > + > +function getFileReader(file) { nit: this function doesn't returns a FileReader, but the file content, so its name could be improved. ::: devtools/client/netmonitor/src/components/DropHarHandler.js:126 (Diff revision 6) > + > +function getFileReader(file) { > + // Test whether the browser supports FileReaders > + // (in case the Network panel is running in a tab) > + if (typeof FileReader == "undefined") { > + return Promise.resolve(null); Shouldn't we instead reject the promise with an explicit error message? But if FileReader is available in both launchpad and firefox, this check isn't really necessary.
Attachment #8948417 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #18) > The error message prefix should rather be in DropHarHandler.js's > appendPreview. Done > This line looks like a lefover. Yes, removed > With current patch, the drag'n drop overlay blinks as you move over > netmonitor individual DOM elements, like column headers. > I don't know much about drag/drop events, but firefox codebase seems to use > dragexit instead of dragleave: > https://searchfox.org/mozilla-central/source/browser/base/content/browser. > xul#658-661 > And if you replace leave by exit here, it no longer blinks. Nice! > nit: this function doesn't returns a FileReader, but the file content, so > its name could be improved. Done > Shouldn't we instead reject the promise with an explicit error message? > But if FileReader is available in both launchpad and firefox, this check > isn't really necessary. Done Honza
Comment 21•7 years ago
|
||
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 19 changes to 19 files remote: remote: remote: ************************** ERROR **************************** remote: Rev 1f95d943bf50 needs "Bug N" or "No bug" in the commit message. remote: Jan Odvarko <odvarko@gmail.com> remote: 1434333 - Import an existing HAR file; r=ochameau remote: remote: MozReview-Commit-ID: 1N0FIcWzbkc remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.c_commitmessage hook failed abort: push failed on remote
Comment 22•7 years ago
|
||
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 19 changes to 19 files remote: remote: remote: ************************** ERROR **************************** remote: Rev 1f95d943bf50 needs "Bug N" or "No bug" in the commit message. remote: Jan Odvarko <odvarko@gmail.com> remote: 1434333 - Import an existing HAR file; r=ochameau remote: remote: MozReview-Commit-ID: 1N0FIcWzbkc remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.c_commitmessage hook failed abort: push failed on remote
Comment 23•7 years ago
|
||
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 19 changes to 19 files remote: remote: remote: ************************** ERROR **************************** remote: Rev edaecc351479 needs "Bug N" or "No bug" in the commit message. remote: Jan Odvarko <odvarko@gmail.com> remote: 1434333 - Import an existing HAR file; r=ochameau remote: remote: MozReview-Commit-ID: 1N0FIcWzbkc remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.c_commitmessage hook failed abort: push failed on remote
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8dd669972fed Import an existing HAR file; r=ochameau
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8dd669972fed
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•