Closed
Bug 1436665
Opened 7 years ago
Closed 6 years ago
WE: onRequestFinished event should be sent even if the Netmonitor UI isn't initialized
Categories
(DevTools :: Netmonitor, enhancement, P2)
DevTools
Netmonitor
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Honza, Assigned: Honza, NeedInfo)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 3 obsolete files)
This is a follow-up for bug 1311171. onRequestFinished event is currently sent only if the Network panel UI is initialized (ie the panel is selected at least once). This requirement should be removed and the HTTP tracking back-end should not depend on the UI. First step towards this is fixing bug Bug 1416748 (or at least analysis should be done). Honza
Assignee | ||
Updated•7 years ago
|
Blocks: netmonitor-har
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
Work split into three parts: #1) Introduce Net monitor app object * The app object can be disconnected from the UI * The app object can be utilized by WebExtensions API #2) Use separate connection for HAR exporter * HAR exporter disables actions in connector and so needs its own connection #3) Use Net monitor app object for WebExtension API --- Net monitor app object created for WebExtension API isn't connected to the UI (net panel) since it doesn't have to exist at the time when extensions are using this API. We can also disconnect it from the store to make the HAR export process even faster. This is covered by bug 1445617 Honza
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
Try push green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b34d80e90ce7a3f830cfc9fe6149ea716c342531 Honza
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8958783 [details] Bug 1436665 - Expose Net panel API without the UI; https://reviewboard.mozilla.org/r/227688/#review234690 Sorry again for the delay in review. I started writing various review comments, but before going into the details, I was wondering why you choose to pull off NetMonitorApp? I thought that you would only need a Connector instance. There is many things that you don't need in NetMonitorApp, like all the code you prefix with `if (this.document)`. Also, I imagine we could simplify NetMonitorApp by moving all WebExtension code to a class specific to "panel-less" connector. Like `onRequestAdded`, `getHarExportConnector`, `fetchResponseContent`, `add/remove/hasRequestFinishedListener`. I'm not sure, but would such class fits HarExporter and WebExt needs? ``` function PanelLessStore() { // Configure store/state object. this.connector = new Connector(); this.store = configureStore(this.connector); } PanelLessStore.prototype = { bootstrap: function(toolbox) { // Initialize connection to the backend. const connection = { tabConnection: { tabTarget: toolbox.target, }, toolbox, panel: null, owner: null, }; return this.connector.connectFirefox(connection, null, this.store.getState); }, onRequestAdded: ..., getHarExportConnector: ..., fetchResponseContent: ..., add/remove/hasRequestFinishedListener: ..., } ``` And you would use that from toolbox instead of NetMonitorApp. Would that work? If that helps you may make this PanelLessStore class be a super class for NetMonitorApp if you want to keep all WebExt API on NetMonitorApp object. (PanelLessStore isn't a perfect name...) ::: commit-message-c56ef:3 (Diff revision 2) > +Bug 1436665 - Introduce Net monitor app object; r=ochameau > +* The app object can be disconnected from the UI > +* The app object can be utilized by WebExtensions API s/utilized/used/ ? ::: devtools/client/netmonitor/src/app.js:41 (Diff revision 2) > +} > + > +NetMonitorApp.prototype = { > + bootstrap({ toolbox, panel, window}) { > + this.window = window; > + this.document = this.window ? this.window.document : null; We don't use window in this class, so either: - directly pass document as bootstrap argument: bootstrap({ toolbox, panel, document}) { this.document = document; - or only store document on this: bootstrap({ toolbox, panel, window}) { this.document = window ? window.document : null; ::: devtools/client/netmonitor/src/app.js:50 (Diff revision 2) > + tabConnection: { > + tabTarget: toolbox.target, > + }, > + toolbox, > + panel, > + owner: this, I'm wondering if that would be clearer to name this attribute `app` rather than `owner`. With `app` would will more easily understand that refers to `NetMonitorApp`. ::: devtools/client/netmonitor/src/connector/firefox-connector.js:391 (Diff revision 2) > + } > + > + // Consumed mainly by tests. > + if (typeof window != "undefined") { > + window.emit(type, data); > + } I don't understand why tests are not listening for events on `owner`? (i.e. `window.Netmonitor` rather than `window`) It looks like unnecessary complexity from an obsolete reason. So may be you can replace all calls to `this.emit(...)` by `if (this.owner) { this.owner.emit(...) }`. Also, if `owner` can be null, you should algo guard all calls to `this.owner.on(...)` like what you do for `emit`: `if (this.owner) { this.owner.on(...); }` ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:628 (Diff revision 2) > + > + // Consumed mainly by tests. > - if (typeof window != "undefined") { > + if (typeof window != "undefined") { > - window.emit(type, data); > + window.emit(type, data); > - } > + } > -} > + } I have the same comment than FirefoxConnector.emit here. ::: devtools/client/netmonitor/src/har/har-exporter.js:132 (Diff revision 2) > */ > getHar: function(options) { > - return this.fetchHarData(options).then(JSON.parse); > + return this.fetchHarData(options).then(data => { > + return data ? JSON.parse(data) : null; > + }); > }, You never use async/await in any of these patches and I think you should. It would make the code more readable: ``` async getHar: function(options) { let data = await this.fetchHarData(options); if (data) { return JSON.parse(data); } else { return null; } } ```
Attachment #8958783 -
Flags: review?(poirot.alex)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•6 years ago
|
||
Thanks for the review Alex! (In reply to Alexandre Poirot [:ochameau] from comment #9) > I started writing various review comments, but before going into the > details, I was wondering why you choose to pull off NetMonitorApp? I thought > that you would only need a Connector instance. Yeah, I started with that, but it turned out that extract the code from initializer.js and intoroduce an App object is good move. It can contain: list of listeners (WE), connection to the backend and helper connection used for HAR export. And yes, the App contains even the code that needs document (e.g. React mounting and actual rendering). > There is many things that you don't need in NetMonitorApp, like all the code > you prefix with `if (this.document)`. Yes, rendering and `inspectRequest` -> that's an example of integration with another panel (Inspector). Read more in the next comment... > Also, I imagine we could simplify NetMonitorApp by moving all WebExtension > code to a class specific to "panel-less" connector. Like `onRequestAdded`, > `getHarExportConnector`, `fetchResponseContent`, > `add/remove/hasRequestFinishedListener`. > I'm not sure, but would such class fits HarExporter and WebExt needs? > ``` I've been thinking the suggestion with the super class, implementing only features independent of the panel. I was thinking about this a lot, but didn't do it in the end. It sounds like the right direction, but we can wait for another use case to learn more yet (e.g. for new WebExtension API, integration with another panel, etc.) I couldn't also find the right name for the super class, which might indicate luck of good concept... > function PanelLessStore() { > // Configure store/state object. > this.connector = new Connector(); > this.store = configureStore(this.connector); > } > PanelLessStore.prototype = { > bootstrap: function(toolbox) { > // Initialize connection to the backend. > const connection = { > tabConnection: { > tabTarget: toolbox.target, > }, > toolbox, > panel: null, > owner: null, > }; > return this.connector.connectFirefox(connection, null, > this.store.getState); > }, > onRequestAdded: ..., > getHarExportConnector: ..., > fetchResponseContent: ..., > add/remove/hasRequestFinishedListener: ..., > } > ``` Yes, I can imagine a super class like that. Let's prove such concept with yet another use case. > And you would use that from toolbox instead of NetMonitorApp. Would that > work? > If that helps you may make this PanelLessStore class be a super class for > NetMonitorApp if you want to keep all WebExt API on NetMonitorApp object. > (PanelLessStore isn't a perfect name...) > > ::: commit-message-c56ef:3 > (Diff revision 2) > > +Bug 1436665 - Introduce Net monitor app object; r=ochameau > > +* The app object can be disconnected from the UI > > +* The app object can be utilized by WebExtensions API > > s/utilized/used/ ? Fixed > > ::: devtools/client/netmonitor/src/app.js:41 > (Diff revision 2) > > +} > > + > > +NetMonitorApp.prototype = { > > + bootstrap({ toolbox, panel, window}) { > > + this.window = window; > > + this.document = this.window ? this.window.document : null; > > We don't use window in this class, so either: > - directly pass document as bootstrap argument: > bootstrap({ toolbox, panel, document}) { > this.document = document; > - or only store document on this: > bootstrap({ toolbox, panel, window}) { > this.document = window ? window.document : null; Also fixed. I am passing in: toolbox, panel and document. It's better > ::: devtools/client/netmonitor/src/app.js:50 > (Diff revision 2) > > + tabConnection: { > > + tabTarget: toolbox.target, > > + }, > > + toolbox, > > + panel, > > + owner: this, > > I'm wondering if that would be clearer to name this attribute `app` rather > than `owner`. With `app` would will more easily understand that refers to > `NetMonitorApp`. Yep, done. > ::: devtools/client/netmonitor/src/connector/firefox-connector.js:391 > (Diff revision 2) > > + } > > + > > + // Consumed mainly by tests. > > + if (typeof window != "undefined") { > > + window.emit(type, data); > > + } > > I don't understand why tests are not listening for events on `owner`? (i.e. > `window.Netmonitor` rather than `window`) > It looks like unnecessary complexity from an obsolete reason. Yep, agree, I'll file a follow-up bug to update all tests. > So may be you can replace all calls to `this.emit(...)` by `if (this.owner) > { this.owner.emit(...) }`. > Also, if `owner` can be null, you should algo guard all calls to > `this.owner.on(...)` like what you do for `emit`: > `if (this.owner) { this.owner.on(...); }` > > ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:628 > (Diff revision 2) > > + > > + // Consumed mainly by tests. > > - if (typeof window != "undefined") { > > + if (typeof window != "undefined") { > > - window.emit(type, data); > > + window.emit(type, data); > > - } > > + } > > -} > > + } > > I have the same comment than FirefoxConnector.emit here. Yep, we shouldn't depend on window. It doesn't exist if the App is used by WE API > > ::: devtools/client/netmonitor/src/har/har-exporter.js:132 > (Diff revision 2) > > */ > > getHar: function(options) { > > - return this.fetchHarData(options).then(JSON.parse); > > + return this.fetchHarData(options).then(data => { > > + return data ? JSON.parse(data) : null; > > + }); > > }, > > You never use async/await in any of these patches and I think you should. It > would make the code more readable: > ``` > async getHar: function(options) { > let data = await this.fetchHarData(options); > if (data) { > return JSON.parse(data); > } else { > return null; > } > } > ``` I updated some places in the patch (the old code stays the same, we should update it in another patch). Thanks! Honza
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #13) > I've been thinking the suggestion with the super class, implementing only > features independent of the panel. I was thinking about this a lot, > but didn't do it in the end. It sounds like the right direction, > but we can wait for another use case to learn more yet (e.g. for > new WebExtension API, integration with another panel, etc.) One thing I forgot to mention. The App object has now two ways how to be initialized: 1) `bootstrap()`: full initialization with panel/window included 2) `connect()`: only connection to the backend Honza
Assignee | ||
Comment 15•6 years ago
|
||
Try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae1879e7a41b7635e68d42cd8fb9608589c4e8f&selectedJob=169882697 Honza
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8958783 [details] Bug 1436665 - Expose Net panel API without the UI; https://reviewboard.mozilla.org/r/227688/#review236552 I didn't realized we would instanciate multiple connectors with the full patch queue, see my second comment. ::: devtools/client/netmonitor/src/app.js:20 (Diff revision 3) > +const { EVENTS } = require("./constants"); > +const Actions = require("./actions/index"); > +const { > + getDisplayedRequestById, > + getSortedRequests > +} = require("./selectors/index"); Ok so if you are keeping this class as-is, you probably want to use `loader.lazyRequireGetter` to prevent loading unecessary dependencies when using this from non-panel codepaths. For: all react deps, Provider, App. ::: devtools/client/netmonitor/src/app.js:34 (Diff revision 3) > +function NetMonitorApp() { > + EventEmitter.decorate(this); > + > + // Configure store/state object. > + this.connector = new Connector(); > + this.store = configureStore(this.connector); Can we instanciate connector and store only once and share between the panel and panel-less usages? 1) We would instanciate only one client/actor. Do the queries only once, even if the netmonitor is opened *after* the addon. 2) Your later modification in toolbox.js would be much simplier if you share them: ``` if (netPanel) { return netPanel.panelWin.Netmonitor.getHar(); } if (this._netMonitorApp) { return this._netMonitorApp.getHar(); ``` would become: ``` return toolbox.getNetmonitorConnector().getHar(); ``` 3) here, you would fetch the connector from the toolbox instead of instanciate it: ``` this.connector = toolbox.getNetmonitorConnector(); this.store = this.connector.store; // I think it would be easier to pass directly the `store` to `Connector` rather than just `getState` function. ``` toobox.getNetmonitorConnector would be something like: ``` if (this.connector) return this.connector; let connector = this.connector = new Connector(); let store = configureStore(this.connector); let actions = bindActionCreators(Actions, store.dispatch); const connection = { tabConnection: { tabTarget: toolbox.target, }, toolbox, } connector.connectFirefox(connection, actions, store); return connector; ``` * panel can be dropped, it is used only from here: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-connector.js#171 and can be replaced with: ``` let panel = this.toolbox.getPanel("netmonitor"); if (panel) panel.emit("reloaded"); ``` * you would revert back to emit events on `window` rather than `owner`. Or emit them on the connector rather than the app. 4) You would move all webextension helpers to connector. It is better hosted there rather than on the App. Connector is panel-less and already contains methods similar to the one we need for WebExt. ::: devtools/client/netmonitor/src/app.js:90 (Diff revision 3) > + toolbox, > + panel, > + app: this, > + }; > + > + return this.connectBackend(this.connector, connection, this.actions, Given that connectBackend if only called from here, I'm not sure it is worth having and would better inline it here.
Attachment #8958783 -
Flags: review?(poirot.alex)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #16) > ::: devtools/client/netmonitor/src/app.js:20 > (Diff revision 3) > > +const { EVENTS } = require("./constants"); > > +const Actions = require("./actions/index"); > > +const { > > + getDisplayedRequestById, > > + getSortedRequests > > +} = require("./selectors/index"); > > Ok so if you are keeping this class as-is, you probably want to use > `loader.lazyRequireGetter` to prevent loading unecessary dependencies when > using this from non-panel codepaths. For: all react deps, Provider, App. Done > ::: devtools/client/netmonitor/src/app.js:34 > (Diff revision 3) > > +function NetMonitorApp() { > > + EventEmitter.decorate(this); > > + > > + // Configure store/state object. > > + this.connector = new Connector(); > > + this.store = configureStore(this.connector); > > Can we instanciate connector and store only once and share between the panel > and panel-less usages? No, the connector for WebExtensions is *not* sending notifications while the connector used by the panel is. They are both used in parallel, so can't share the state. Yes, sharing the connector would make the code simpler. But, having separate connector for extensions, which is disconnected from the UI as well as from the store (bug 1445617) will make the measurements (HTTP Timings) more precise - less perf penalties (and the timing is what users mostly want to measure). > * panel can be dropped, it is used only from here: > https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/ > connector/firefox-connector.js#171 > and can be replaced with: > ``` > let panel = this.toolbox.getPanel("netmonitor"); > if (panel) panel.emit("reloaded"); > ``` Yeah, good point, agree, I'll do this anyway in an extra patch, so it's easy to review. > Given that connectBackend if only called from here, I'm not sure it is worth > having and would better inline it here. The method is called from two places and I also have an extra future plan with it. As soon as connecting to different backends is supported this method will be the point where a decision needs to be made. I updated the comment to make this more clear. Thanks for the review Alex! Honza
Comment 22•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #21) > > Can we instanciate connector and store only once and share between the panel > > and panel-less usages? > No, the connector for WebExtensions is *not* sending notifications while the > connector > used by the panel is. They are both used in parallel, so can't share the > state. What are notifications here? You mean redux actions/store updates? > Yes, sharing the connector would make the code simpler. But, having separate > connector for extensions, which is disconnected from the UI as well as > from the store (bug 1445617) will make the measurements (HTTP Timings) > more precise - less perf penalties (and the timing is what users mostly > want to measure). I see it very differently. * When the netmonitor panel is closed. It won't be any different. There will be only one connector, used by WebExt. * When the netmonitor panel is opened before the webext, it will be the same as well. We will be using one connector, the panel one, which will also fire redux updates. * When the netmonitor panel is opened after the WebExt, we will have two connectors with your current patch. The performance is going to be worse. The two netmonitor RDP clients are going to fetch requests data in parallel. bug 1445617 is independant improvement that doesn't directly relates to having one or more instances of connectors. It will only benefit to the first case, when the panel is closed. The store updates done by the duplicated connectors, used for netmonitor UI, will very likely pollute the data of the other one used by WebExt as they are running on the same process. But bug 1445617 is a good improvement, per comment 16 suggestion, it would allow to make toobox.getNetmonitorConnector create a connector without a store. The more I look at this code (connectors, and especially FirefoxDataProvider) the more it looks like a "Front" (per protocol.js architecture). A front is the client part of actors that helps querying actors data. And all fronts are singletons, we only instanciate one unique front per actor instance. Otherwise, our discussion and bug 1445617 seems to highlight that you only need the connector for WebExt purposes. I feel that you are focusing on NetmonitorApp just because it is the place where we have all WebExt helpers today (getHar, fetchResponseContent and onRequestAdded). But these helpers would be better hosted into the connector as it isn't related to netmonitor frontend after all.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8958784 -
Attachment is obsolete: true
Attachment #8958784 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•6 years ago
|
Attachment #8958785 -
Attachment is obsolete: true
Attachment #8958785 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•6 years ago
|
Attachment #8962428 -
Attachment is obsolete: true
Attachment #8962428 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 24•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #22) > What are notifications here? You mean redux actions/store updates? Yes > > Yes, sharing the connector would make the code simpler. But, having separate > > connector for extensions, which is disconnected from the UI as well as > > from the store (bug 1445617) will make the measurements (HTTP Timings) > > more precise - less perf penalties (and the timing is what users mostly > > want to measure). > > I see it very differently. > * When the netmonitor panel is closed. It won't be any different. There will > be only one connector, used by WebExt. > * When the netmonitor panel is opened before the webext, it will be the same > as well. We will be using one connector, the panel one, which will also fire > redux updates. > * When the netmonitor panel is opened after the WebExt, we will have two > connectors with your current patch. > The performance is going to be worse. The two netmonitor RDP clients are > going to fetch requests data in parallel. The new path is reusing an existing connection that can be created before the Net panel opens. There are other changes: - Only one patch sorry (all changes are quite interconnected) - Using `owner` again (the `App` is no longer the recipient and this name feels more appropriate). - The App object split into two: 1) API (not connected to the UI) 2) App (connected to the UI) The API object is used if the Net panel doesn't exist and reused when the Net panel is created later. Hope this will make my reviewer happy ;-) > The more I look at this code (connectors, and especially > FirefoxDataProvider) the more it looks like a "Front" (per protocol.js > architecture). > A front is the client part of actors that helps querying actors data. > And all fronts are singletons, we only instanciate one unique front per > actor instance. > Otherwise, our discussion and bug 1445617 seems to highlight that you only > need the connector for WebExt purposes. > I feel that you are focusing on NetmonitorApp just because it is the place > where we have all WebExt helpers today (getHar, fetchResponseContent and > onRequestAdded). But these helpers would be better hosted into the connector > as it isn't related to netmonitor frontend after all. All this sounds promising, but should be analyzed discussed as part of another bug. Try looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ecf7ff566d4ff0155ebaa5f537c65d54094f93 I am working on tests now. Honza
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8958783 [details] Bug 1436665 - Expose Net panel API without the UI; https://reviewboard.mozilla.org/r/227688/#review238786 Thanks a lot Jan for the new patch, it looks much better. If getHarExportConnector can be kept for a followup, I think the patch is almost good. ::: devtools/client/framework/toolbox.js:2793 (Diff revision 5) > deferred.resolve(settleAll(outstanding) > .catch(console.error) > .then(() => { > + let app = this._netMonitor; > + this._netMonitor = null; > + return app ? app.destroy() : null; s/app/api/? ::: devtools/client/framework/toolbox.js:3058 (Diff revision 5) > > /** > - * Returns data (HAR) collected by the Network panel. > + * Return Netmonitor API object. This object offers Network monitor > + * public API that can be consumed by other panels or WE API. > */ > - getHARFromNetMonitor: async function() { > + getNetMonitorWhenReady: async function() { May be it should be named getNetMonitorAPI? ::: devtools/client/framework/toolbox.js:3072 (Diff revision 5) > - let har = await netPanel.panelWin.Netmonitor.getHar(); > + return this._netMonitor; > + } > + > + // Create and initialize Network monitor API object. > + // This object is only connected to the backend - not to the UI. > + this._netMonitor = new NetMonitorAPI(); This attribute would be better named `_netMonitorAPI` now. ::: devtools/client/netmonitor/panel.js:20 (Diff revision 5) > await this.toolbox.target.makeRemote(); > } > - await this.panelWin.Netmonitor.bootstrap({ > + > + // Reuse an existing Network monitor API object if available. > + // It could have been created for WE API before Net panel opens. > + let app = this.panelWin.initialize(this.toolbox._netMonitor); Rather than using a private field like this, it would be better to do: let api = await this.toolbox.getNetmonitorAPI(); let app = this.panelWin.initialize(api); ::: devtools/client/netmonitor/panel.js:23 (Diff revision 5) > + // Reuse an existing Network monitor API object if available. > + // It could have been created for WE API before Net panel opens. > + let app = this.panelWin.initialize(this.toolbox._netMonitor); > + > + // Remove from the Toolbox. It belongs to the Network panel now. > + this.toolbox._netMonitor = null; I imagine you do that to prevent having the API destroyed when WebExt unregister its listener, but this will be hard to maintain. It would be slightly more explicit to call: api.addRequestFinishedListener(() => {}); in order to keep it alive. Or: * add an specific flag on NetMonitorAPI to make it always alive. And toggle this flag from here. * Have toolbox.getNetmonitorAPI accept a boolean and use it from toolbox.js to prevent destroying it from removeRequestFinishedListener. * ... ::: devtools/client/netmonitor/src/api.js:80 (Diff revision 5) > + > + await this.connector.disconnect(); > + > + if (this.harExportConnector) { > + await this.harExportConnector.disconnect(); > + } This looks like something used by a followup. ::: devtools/client/netmonitor/src/api.js:121 (Diff revision 5) > + return; > + } > + > + let { HarExporter } = require("devtools/client/netmonitor/src/har/har-exporter"); > + > + let connector = await this.getHarExportConnector(); WebExt should reuse the existing connector. It looks like getHarExportConnector is related to another followup patch? ::: devtools/client/netmonitor/src/api.js:188 (Diff revision 5) > + tabTarget: this.toolbox.target, > + }, > + toolbox: this.toolbox, > + }; > + > + this.harExportConnector = new Connector(); This method isn't necessary for this bug right? It defeats the suggestion I made to have only one connector. It would be great to followup on this in order to see if getState can be lazily set on Connector, so that you only use it when you need it. ::: devtools/client/netmonitor/src/app.js:20 (Diff revision 5) > + getDisplayedRequestById, > +} = require("./selectors/index"); > + > +/** > + * Global App object for Network panel. This object depends > + * on the UI and can't be created independently. I think the original comments was more helpful than this one: This object can be consumed by other panels (e.g. Console is using inspectRequest), by the Launchpad (bootstrap), etc. ::: devtools/client/netmonitor/src/app.js:25 (Diff revision 5) > + * on the UI and can't be created independently. > + * > + * @param {Object} api An existing API object to be reused. > + */ > +function NetMonitorApp(api) { > + this.api = api || new NetMonitorAPI(); Automatically creating the API when none is given as argument can easily hide errors where we were unable to pass toolbox's one. It would be safer to always expect one and have all the callsite pass one. So it would be better in initializer.js:91 was creating an API: const { NetMonitorAPI } = require("./api"); let api = new NetMonitorAPI(); let app = window.initialize(api); ::: devtools/client/netmonitor/src/app.js:64 (Diff revision 5) > + }); > + > + // Render the root Application component. > + render(Provider({ store: store }, app), this.mount); > + > + await this.api.connect(toolbox); toolbox.js:3073 should already have "connected" the API. You should not need to call this again from here. ::: devtools/client/netmonitor/src/app.js:73 (Diff revision 5) > + * Clean up (unmount from DOM, remove listeners, disconnect). > + */ > + async destroy() { > + unmountComponentAtNode(this.mount); > + > + await this.api.destroy(); I'm not sure you want to destroy it from here. There may still be WebExt listeners registered. "In theory", as Netmonitor's destroy is only ever called on toolbox destroy... ::: devtools/client/netmonitor/src/connector/firefox-connector.js:40 (Diff revision 5) > this.getNetworkRequest = this.getNetworkRequest.bind(this); > } > > async connect(connection, actions, getState) { > this.actions = actions; > this.getState = getState; It would be handy to mention here or in `connect` documentation that `getState` is optional. ::: devtools/client/netmonitor/src/connector/firefox-connector.js:44 (Diff revision 5) > this.getState = getState; > this.tabTarget = connection.tabConnection.tabTarget; > this.toolbox = connection.toolbox; > - this.panel = connection.panel; > > + // The owner object (NetPanelAPI) received all events. s/NetPanelAPI/NetMonitorAPI/
Attachment #8958783 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #25) > If getHarExportConnector can be kept for a followup, I think the patch is > almost good. Yes, I created a follow up Bug 1451304 > ::: devtools/client/framework/toolbox.js:2793 > (Diff revision 5) > > deferred.resolve(settleAll(outstanding) > > .catch(console.error) > > .then(() => { > > + let app = this._netMonitor; > > + this._netMonitor = null; > > + return app ? app.destroy() : null; > > s/app/api/? Done > ::: devtools/client/framework/toolbox.js:3058 > (Diff revision 5) > > > > /** > > - * Returns data (HAR) collected by the Network panel. > > + * Return Netmonitor API object. This object offers Network monitor > > + * public API that can be consumed by other panels or WE API. > > */ > > - getHARFromNetMonitor: async function() { > > + getNetMonitorWhenReady: async function() { > > May be it should be named getNetMonitorAPI? Done getNetMonitorWhenReady -> getNetMonitorAPI this._netMonitor -> this._netMonitorAPI > Rather than using a private field like this, it would be better to do: > let api = await this.toolbox.getNetmonitorAPI(); > let app = this.panelWin.initialize(api); Done > It would be slightly more explicit to call: > api.addRequestFinishedListener(() => {}); > in order to keep it alive. Done > I think the original comments was more helpful than this one: > This object can be consumed by other panels (e.g. Console is using > inspectRequest), by the Launchpad (bootstrap), etc. Done > Automatically creating the API when none is given as argument can easily > hide errors where we were unable to pass toolbox's one. > It would be safer to always expect one and have all the callsite pass one. > So it would be better in initializer.js:91 was creating an API: > const { NetMonitorAPI } = require("./api"); > let api = new NetMonitorAPI(); > let app = window.initialize(api); Yep, done > ::: devtools/client/netmonitor/src/app.js:64 > (Diff revision 5) > > + }); > > + > > + // Render the root Application component. > > + render(Provider({ store: store }, app), this.mount); > > + > > + await this.api.connect(toolbox); > > toolbox.js:3073 should already have "connected" the API. You should not need > to call this again from here. Done > > ::: devtools/client/netmonitor/src/app.js:73 > (Diff revision 5) > > + * Clean up (unmount from DOM, remove listeners, disconnect). > > + */ > > + async destroy() { > > + unmountComponentAtNode(this.mount); > > + > > + await this.api.destroy(); > > I'm not sure you want to destroy it from here. > There may still be WebExt listeners registered. > "In theory", as Netmonitor's destroy is only ever called on toolbox > destroy... It's for the case when the Network panel runs in a tab (there is no Toolbox in such case) I made a comment to explain that. > ::: devtools/client/netmonitor/src/connector/firefox-connector.js:40 > (Diff revision 5) > > this.getNetworkRequest = this.getNetworkRequest.bind(this); > > } > > > > async connect(connection, actions, getState) { > > this.actions = actions; > > this.getState = getState; > > It would be handy to mention here or in `connect` documentation that > `getState` is optional. Done > ::: devtools/client/netmonitor/src/connector/firefox-connector.js:44 > (Diff revision 5) > > this.getState = getState; > > this.tabTarget = connection.tabConnection.tabTarget; > > this.toolbox = connection.toolbox; > > - this.panel = connection.panel; > > > > + // The owner object (NetPanelAPI) received all events. > > s/NetPanelAPI/NetMonitorAPI/ Done Thanks Alex! Honza
Comment hidden (mozreview-request) |
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8958783 [details] Bug 1436665 - Expose Net panel API without the UI; https://reviewboard.mozilla.org/r/227688/#review239314 Thanks for addressing everything Jan! I still have a comment about getHarExportConnector. ::: devtools/client/netmonitor/src/api.js:129 (Diff revision 6) > + console.error("HAR: request not found " + requestId); > + return; > + } > + > + let options = { > + connector, You should use this.connector here, as before this patch and as all other methods of this class. And remove getHarExportConnector until you justify its needs. I understand you intend to avoid noise in har timings. This setup may be better when you end up instanciating only one of the two connectors (i.e when you only open WebExt -or- when you only open netmonitor). But it is going to be really bad if you have the two opened. You can make it fast in both cases by disabling getState calls if you only open WebExt or complely disable store interaction if you are only using it from Har automation.
Attachment #8958783 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•6 years ago
|
||
I yet updated the patch to fix some test failures. (1) I did remove the `api.addRequestFinishedListener(() => {})` since adding an empty listeners causes unnecessary overhead (HAR is generated for nothing). Instead the Toolbox is testing if the Net panel exists and if yes not destroying the API object. (2) Also connecting to backend in the API object (`connectBackend` method) is calling `target.makeRemote` to make sure the web client object is properly initialized. (3) Check values of props in FirefoxConnector to avoid NPE (e.g. for actions) (4) I created a new patch that stops firing events on the panel window object and updates all tests (we also discussed that). Tests are now listening for events on the API object. Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b490a5083e4a71e714d5b1fcaa50c37736431922 > I still have a comment about getHarExportConnector. > > ::: devtools/client/netmonitor/src/api.js:129 > (Diff revision 6) > > + console.error("HAR: request not found " + requestId); > > + return; > > + } > > + > > + let options = { > > + connector, > > You should use this.connector here, as before this patch and as all other > methods of this class. > And remove getHarExportConnector until you justify its needs. > > I understand you intend to avoid noise in har timings. This setup may be > better when you end up instanciating only one of the two connectors (i.e > when you only open WebExt -or- when you only open netmonitor). But it is > going to be really bad if you have the two opened. You can make it fast in > both cases by disabling getState calls if you only open WebExt or complely > disable store interaction if you are only using it from Har automation. I am aware of that and it'll be fixed in bug Bug 1451304 The thing is that standard connection fires actions (to updated the UI) while export har connection does not (to fix performance) and so the HAR export needs extra connection that knows not to fire actions. I'll find better way in Bug 1451304 --- The existing test: browser/components/extensions/test/browser/browser_ext_devtools_network.js ...needs to be updated yet (wip). Honza
Assignee | ||
Comment 32•6 years ago
|
||
@Luca: the existing test: browser/components/extensions/test/browser/browser_ext_devtools_network.js ... now fails for me since `addRequestListener` and `removeRequestListener` are async and the test doesn't wait for them to finish. Shutdown and disconnect is done too soon (in the middle of HAR generation) and the test fails. You might try it for yourself. Any tips how to fix this? Honza
Flags: needinfo?(lgreco)
Comment 33•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #31) > (2) Also connecting to backend in the API object (`connectBackend` method) > is calling `target.makeRemote` to make sure the web client object is > properly initialized. Did that ended up breaking a test? (I'm asking as I would like some day to simplify this makeRemote thing and have it called from only one place)
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8965343 [details] Bug 1436665 - Do not fire events on window, update tests; https://reviewboard.mozilla.org/r/234070/#review239728 Thanks for this cleanup! ::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:85 (Diff revision 1) > } > > function waitForRequestAdded(toolbox) { > return new Promise(resolve => { > let netPanel = toolbox.getPanel("netmonitor"); > - netPanel.panelWin.once("NetMonitor:RequestAdded", () => { > + netPanel.panelWin.api.once("NetMonitor:RequestAdded", () => { nit: may be exposing `api` onto panel object would help?
Attachment #8965343 -
Flags: review?(poirot.alex) → review+
Comment 35•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #32) > @Luca: the existing test: > browser/components/extensions/test/browser/browser_ext_devtools_network.js > > ... now fails for me since `addRequestListener` and `removeRequestListener` > are async and the test doesn't wait for them to finish. Shutdown and > disconnect is done too soon (in the middle of HAR generation) and the test > fails. You might try it for yourself. Any tips how to fix this? Hi Jan, I looked into it and the main reason for the failures on the devtools.network APIs tests seems to be related to the initial assumption under which we tested the initial implementation that "the onRequestFinished events were only going to be received when the netmonitor panel has been already selected by the user and loaded once", with the attached patch applied the events are actually received even without selecting the panel, which is great, and so we should basically fix the test, so that we are going to receive the event only when we are actually expecting it, otherwise the WebExtensions tests are going to fail automatically because of "test messages" received and never processed. Here is the diff related to the changes I applied locally to successfully pass the tests with these patches applied (the info("...") are only there to be able to investigate timeouts more easily from the failures logs of a push to try): diff --git a/browser/components/extensions/test/browser/browser_ext_devtools_network.js b/browser/components/extensions/test/browser/browser_ext_devtools_network.js --- a/browser/components/extensions/test/browser/browser_ext_devtools_network.js +++ b/browser/components/extensions/test/browser/browser_ext_devtools_network.js @@ -71,17 +71,22 @@ function devtools_page() { // Get response content using returned promise request.getContent().then(([content, encoding]) => { browser.test.sendMessage("onRequestFinished-promiseResolved", [content, encoding]); }); browser.devtools.network.onRequestFinished.removeListener(requestFinishedListener); }; - browser.devtools.network.onRequestFinished.addListener(requestFinishedListener); + + browser.test.onMessage.addListener(msg => { + if (msg === "addOnRequestFinishedListener") { + browser.devtools.network.onRequestFinished.addListener(requestFinishedListener); + } + }); } function waitForRequestAdded(toolbox) { return new Promise(resolve => { let netPanel = toolbox.getPanel("netmonitor"); netPanel.panelWin.api.once("NetMonitor:RequestAdded", () => { resolve(); }); @@ -181,19 +186,16 @@ add_task(async function test_devtools_ne // Reload the page to collect some HTTP requests. extension.sendMessage("navigate"); // Wait till the navigation is complete and request // added into the net panel. await Promise.all([ extension.awaitMessage("tabUpdated"), extension.awaitMessage("onNavigatedFired"), - extension.awaitMessage("onRequestFinished"), - extension.awaitMessage("onRequestFinished-callbackExecuted"), - extension.awaitMessage("onRequestFinished-promiseResolved"), waitForRequestAdded(toolbox), ]); // Get HAR, it should not be empty now. const getHARPromise = extension.awaitMessage("getHAR-result"); extension.sendMessage("getHAR"); const getHARResult = await getHARPromise; is(getHARResult.entries.length, 1, "HAR log should not be empty"); @@ -219,27 +221,33 @@ add_task(async function test_devtools_ne await extension.awaitMessage("ready"); let target = gDevTools.getTargetForTab(tab); // Open the Toolbox let toolbox = await gDevTools.showToolbox(target, "netmonitor"); info("Developer toolbox opened."); + // Wait the extension to subscribe the onRequestFinished listener. + await extension.sendMessage("addOnRequestFinishedListener"); + // Reload and wait for onRequestFinished event. extension.sendMessage("navigate"); + info("Wait for tab navigation to be completed"); await Promise.all([ extension.awaitMessage("tabUpdated"), extension.awaitMessage("onNavigatedFired"), waitForRequestAdded(toolbox), ]); + info("Wait for an onRequestFinished event"); await extension.awaitMessage("onRequestFinished"); + info("Wait for request.getBody results"); // Wait for response content being fetched. let [callbackRes, promiseRes] = await Promise.all([ extension.awaitMessage("onRequestFinished-callbackExecuted"), extension.awaitMessage("onRequestFinished-promiseResolved"), ]); ok(callbackRes[0].startsWith("<html>"), "The expected content has been retrieved.");
Flags: needinfo?(lgreco)
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8958783 [details] Bug 1436665 - Expose Net panel API without the UI; https://reviewboard.mozilla.org/r/227688/#review239776 The following comments seems unrelated to the WebExtensions tests failures (as described in comment 35), but while I was investigating those failures I noticed them and I thought they may be worth a mention. ::: devtools/client/netmonitor/src/api.js:185 (Diff revision 7) > + if (this.harExportConnector) { > + return this.harExportConnector; > + } > + > + const connection = { > + tabConnection: { This `connection` object doesn have `owner: this` as the one defined at line 58 (there it is part of the connection, at line 63). This would make `this.owner` undefined in the "firefox-connector.js" (which set it to the `connection.owner` at line 52 of "firefox-connector.js", inside the connect method), from what I saw it seems that it would be unexpected (e.g. the `navigate` method access `this.owner` without checking if it is defined at line 186 of that file). ::: devtools/client/netmonitor/src/connector/firefox-connector.js:88 (Diff revision 7) > + } > > await this.removeListeners(); > > if (this.tabTarget) { > this.tabTarget.off("will-navigate"); It looks that at line 70 and 71 we are subscribing a listener to both "will-navigate" and "navigate", I guess that we should unsubscribe also from "navigate" here, am I reading it wrong?
Assignee | ||
Comment 37•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #33) > (In reply to Jan Honza Odvarko [:Honza] from comment #31) > > (2) Also connecting to backend in the API object (`connectBackend` method) > > is calling `target.makeRemote` to make sure the web client object is > > properly initialized. > > Did that ended up breaking a test? > > (I'm asking as I would like some day to simplify this makeRemote thing and > have it called from only one place) Yes, this one: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_attach.js Here is a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82aa8598af312c7aa71c08b1b0f5e7da9d2aeb9d&selectedJob=172042285 (simply reproducible locally) Honza
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #34) > > function waitForRequestAdded(toolbox) { > > return new Promise(resolve => { > > let netPanel = toolbox.getPanel("netmonitor"); > > - netPanel.panelWin.once("NetMonitor:RequestAdded", () => { > > + netPanel.panelWin.api.once("NetMonitor:RequestAdded", () => { > > nit: may be exposing `api` onto panel object would help? Yep, sounds good to me. There are more globals exposed on the `window` (for tests) and if we change it to the `panel` it should be done for all, so it's consistent. I'll file a follow up as soon as this entire bug lands. Thanks for the review! Honza
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•6 years ago
|
||
(In reply to Luca Greco [:rpl] from comment #35) Thanks for the help! I am attaching a patch that fixes the test (In reply to Luca Greco [:rpl] from comment #36) > This `connection` object doesn have `owner: this` as the one defined at line > 58 (there it is part of the connection, at line 63). Good point, I added a condition to avoid NPE (part of the patch) > It looks that at line 70 and 71 we are subscribing a listener to both > "will-navigate" and "navigate", I guess that we should unsubscribe also from > "navigate" here, am I reading it wrong? Also fixed --- Try is green: (the linting failure is fixed in the patch) https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6f18ee749d76bb2d68134ab318d718de8271654 Honza
Comment 43•6 years ago
|
||
Remember to update the compat data when this is fixed -> https://github.com/mdn/browser-compat-data/pull/1769#issuecomment-380344825
Keywords: dev-doc-needed
Comment 44•6 years ago
|
||
mozreview-review |
Comment on attachment 8966567 [details] Bug 1436665 - Fix WebExtensions devtools.network tests; https://reviewboard.mozilla.org/r/235290/#review242192 Hi Jan, the changes applied to the tests looks good to me (I've only a couple of comments below about it, I've marked only one as an issue to fix, not a big deal to be fair, and a small optional nit, but I'm also marking the review as an r+ right now, because they are just minimal and/or optional changes). About the fixes applied on the firefox-connector.js module, they looks reasonable to me, but I don't know in very much detail all the behaviors expected from the `navigate` method and so it could be reasonable for Alex or someone who worked on this module before to double-check it and add his/her additional sign-off to it. Thanks again for working on the fix for the initial devtools.network API limitations so quickly! ::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:116 (Diff revision 1) > + resolve(); > + }); > + }); > +} > + > +async function navigate(extension, toolbox) { It is not a big deal (and so I'm not even marking it as an issue), but I'm wondering if renaming the helper to something like `navigateToolboxTarget(extension, toolbox)` would make it clearer why the helper gets the extension and toolbox as its parameters. How that would sound to you? ::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:236 (Diff revision 1) > > - // Reload and wait for onRequestFinished event. > - extension.sendMessage("navigate"); > + // Wait the extension to subscribe the onRequestFinished listener. > + await extension.sendMessage("addOnRequestFinishedListener"); > > - await Promise.all([ > - extension.awaitMessage("tabUpdated"), > + // Reload the page > + navigate(extension, toolbox); Even if there is a `await extension.awaitMessage("onRequestFinished");` at line 242, I think that it would still be better to `await` the returned promise of the navigate helper also here.
Attachment #8966567 -
Flags: review?(lgreco) → review+
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8966567 [details] Bug 1436665 - Fix WebExtensions devtools.network tests; https://reviewboard.mozilla.org/r/235290/#review242198 ::: commit-message-6db4e:1 (Diff revision 1) > +Bug 1436665 - Fix tests; r=rpl Do you mind to also tweak the bug summary message a bit? (e.g. Bug 1436665 - Fix WebExtensions devtools.network tests)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•6 years ago
|
||
@Alex: can you take a look at the changes in firefox-connector.js module (just a few lines), thanks. Honza
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 50•6 years ago
|
||
(In reply to Luca Greco [:rpl] from comment #44) > It is not a big deal (and so I'm not even marking it as an issue), but I'm > wondering if renaming the helper to something like > `navigateToolboxTarget(extension, toolbox)` would make it clearer why the Done > Even if there is a `await extension.awaitMessage("onRequestFinished");` at > line 242, I think that it would still be better to `await` the returned > promise of the navigate helper also here. Done > Do you mind to also tweak the bug summary message a bit? (e.g. Bug 1436665 - > Fix WebExtensions devtools.network tests) Done Thanks Luca, I appreciate your help on this bug! Honza
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8966567 [details] Bug 1436665 - Fix WebExtensions devtools.network tests; https://reviewboard.mozilla.org/r/235290/#review242648 Sorry for the delay, connector's change looks good!
Attachment #8966567 -
Flags: review?(poirot.alex) → review+
Updated•6 years ago
|
Flags: needinfo?(poirot.alex)
Comment 52•6 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4b8fe40857db Expose Net panel API without the UI; r=ochameau https://hg.mozilla.org/integration/autoland/rev/513c72b05382 Do not fire events on window, update tests; r=ochameau https://hg.mozilla.org/integration/autoland/rev/09030f59fbea Fix WebExtensions devtools.network tests; r=ochameau,rpl
Comment 53•6 years ago
|
||
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/00d4c3be380c Backed out 3 changesets for talos damp failures on netmonitor/simple.js. CLOSED TREE
Comment 54•6 years ago
|
||
Backed out for talos damp failures on netmonitor/simple.js Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&tochange=ab04b1ca218d7a0ce370ca9f2fd2028fee17567a&fromchange=09030f59fbeae4360be8c46d7e85ff32e9c00201&filter-searchStr=talos%20damp&selectedJob=174058200 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174058200&repo=autoland&lineNumber=1046 Backout link: https://hg.mozilla.org/integration/autoland/rev/00d4c3be380ce45d7ab113c9c668401ba3b301cd
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 58•6 years ago
|
||
Ah, sorry about that. The DAMP test fixed, trying to reland. Honza
Flags: needinfo?(odvarko)
Comment 59•6 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ddbbd0330a1 Expose Net panel API without the UI; r=ochameau https://hg.mozilla.org/integration/autoland/rev/b3702a775b16 Do not fire events on window, update tests; r=ochameau https://hg.mozilla.org/integration/autoland/rev/2120b4c84746 Fix WebExtensions devtools.network tests; r=ochameau,rpl
Comment 60•6 years ago
|
||
Backed out for talos damp failures on toolbox/panels-in-background.js Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2120b4c84746426c1313392f01ee6decb0948317 Backout: https://hg.mozilla.org/integration/autoland/rev/a00c7382b82f7fb9a86c3e9416225c3bf3a88013 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=174076689&repo=autoland&lineNumber=1283
Flags: needinfo?(odvarko)
Comment 61•6 years ago
|
||
Also, on the previous push, there were clipboard failures on devtools/client/netmonitor/src/har/test/browser_net_har_throttle_upload.js Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=09030f59fbeae4360be8c46d7e85ff32e9c00201 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=174070347&repo=autoland&lineNumber=4428
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 65•6 years ago
|
||
Test failures from previous logs fixed(In reply to Narcis Beleuzu [:NarcisB] from comment #61) > Also, on the previous push, there were clipboard failures on > devtools/client/netmonitor/src/har/test/browser_net_har_throttle_upload.js Fixed I'll try to land again. Honza
Flags: needinfo?(odvarko)
Comment 66•6 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f69489d9bb15 Expose Net panel API without the UI; r=ochameau https://hg.mozilla.org/integration/autoland/rev/de4c0e01289b Do not fire events on window, update tests; r=ochameau https://hg.mozilla.org/integration/autoland/rev/d22104932e7b Fix WebExtensions devtools.network tests; r=ochameau,rpl
Comment 67•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f69489d9bb15 https://hg.mozilla.org/mozilla-central/rev/de4c0e01289b https://hg.mozilla.org/mozilla-central/rev/d22104932e7b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Comment 68•6 years ago
|
||
@Will: This is follow up for bug 1311171 The Network panel doesn't have to be opened to make the `onRequestFinished` WebExtension API to work. The patch here is for 61 and no uplift planned. The doc should be updated to reflect it. Thanks! Honza
Flags: needinfo?(wbamberg)
Assignee | ||
Comment 69•6 years ago
|
||
Here is the MDN page that should be updated: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.network/onRequestFinished Honza
Comment 70•6 years ago
|
||
Thanks :Honza! I've updated the compat table to say that this restriction doesn't apply from 61 onwards : https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.network/onRequestFinished#Browser_compatibility I'm marking this dev-doc-complete, but please let me know if you need anything else.
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 71•6 years ago
|
||
Sorry for adding comments on a already done task. For me the HAR isn't correct see https://github.com/devtools-html/har-export-trigger/issues/17
Assignee | ||
Comment 72•6 years ago
|
||
(In reply to Peter Hedenskog from comment #71) > Sorry for adding comments on a already done task. For me the HAR isn't > correct see https://github.com/devtools-html/har-export-trigger/issues/17 Can you please file a bug covering this issue here in bugzilla? I'll look at it. Thanks! Honza
Flags: needinfo?(peter)
Comment 73•6 years ago
|
||
Thanks, I've added https://bugzilla.mozilla.org/show_bug.cgi?id=1471899
You need to log in
before you can comment on or make changes to this bug.
Description
•