Closed Bug 1311171 Opened 8 years ago Closed 6 years ago

Implement the devtools.network.onRequestFinished API event

Categories

(WebExtensions :: Developer Tools, defect, P3)

Product:
WebExtensions
Component:
Developer Tools
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: rpl, Assigned: Honza, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [devtools][devtools.network][devtools.network.onRequestFinished] triaged)

Attachments

(1 file)

Bug 1311171 - Implement the devtools.network.onRequestFinished API event;
59 bytes, text/x-review-board-request
rpl
: review+
jdescottes
: review+
Details 
The devtools.network.onRequestFinished API event, is an event that provides access to the network requests generated by the target window (e.g. as collected by the devtools and logged in the webconsole).

The Request object provides the fields specified in the HAR spec (besides the content that seems to be accessible by using the Request.getContent asynchronous method).

References:
- https://developer.chrome.com/extensions/devtools_network#event-onRequestFinished
- https://developer.chrome.com/extensions/devtools_network#type-Request
- http://www.softwareishard.com/blog/har-12-spec/

NOTE: This API methods should be possible without the creation of a new actor, by subscribing the existent Remote Debugging events sent from the console actor.
Whiteboard: [devtools][devtools.network][devtools.network.onRequestFinished] triaged
Priority: -- → P3
Blocks: 1211859
I'm giving a beer to the dev that implements this. 5$ on Bountysource :) I know it's not much but I can't wait for Chrome DevTools extensions to work in Firefox also :) I might increase the amount in the future.
Summary: Implements the devtools.network.onRequestFinished API event → Implement the devtools.network.onRequestFinished API event
Blocks: 1370525
No longer blocks: 1211859
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
I have been testing `chrome.network.onRequestFinished` in Chrome and it looks like that the method isn't executed until I call `chrome.network.getHAR` it looks like a feature.

@andrei, Peter: do you know more about this?


Honza
Flags: needinfo?(peter)
Flags: needinfo?(andrei)
> Peter: do you know more about this?

Sorry I didn't get around to test/check this. I probably will not next week either, I'm off to our team offsite. I thought that the new WebPageTest agent was using it (https://github.com/WPO-Foundation/wptagent) but seems like I was wrong.
I don't know how this API works but I can show you an extension that uses it. I was planning to nag the dev of this https://chrome.google.com/webstore/detail/saml-chrome-panel/paijfdbeoenhembfhkhllainmocckace to port it to Firefox. This is my motivation for seeing this API implemented.

Here is where the invocation is used. https://github.com/milton-lai/saml-chrome-panel/search?utf8=%E2%9C%93&q=chrome.network.onRequestFinished&type=

You can test the extension in Chrome to see how it works on this demo web site https://simulator.etoegang.nl/simulator-1.9/hm/create/AuthnRequest/ad just click "Create message > Post request > Post request". to see a SAML message in the DevTools extension.
Flags: needinfo?(andrei)
(In reply to andrei from comment #4)
> You can test the extension in Chrome to see how it works on this demo web
> site https://simulator.etoegang.nl/simulator-1.9/hm/create/AuthnRequest/ad
> just click "Create message > Post request > Post request". to see a SAML
> message in the DevTools extension.
I followed your instructions, but I don't see anything in the SAML panel.
What exactly should I see? Can you please post a video/screenshot?

Honza
Flags: needinfo?(andrei)
Some comments/questions/analysis:

1) Just to make sure everyone understands the API (as it's implemented in Chrome):
`onRequestFinished` is fired when a network request is finished and all request data are available. 
Argument passed into the callback represents description of a network request in the form of a HAR entry. See HAR specification for details. 

An example:

chrome.devtools.network.onRequestFinished.addListener(request => {
  console.log(request);
};


2) The object passed into `onRequestFinished` callback: Request.
has `getContent` method, which is used to explicitly fetch response body.
 
An example:

chrome.devtools.network.onRequestFinished.addListener(request => {
  request.getContent(function (content, encoding) {
    request.response.content.text = content;
    request.response.content.encoding = encoding;
    console.log(request);
  })
};


3) Firefox implementation
Firefox Network panel is doing a lot of 'data lazy loading' to improve performance.
This means fetching HTTP request related data only when the user actually wants to see it.
Not only the `response body` is lazy loaded, but also headers, cookies, etc.

Important consequence of `onRequestFinished` API (as it's designed in Chrome) is that 
when a listener is registered for `onRequestFinished` event - the Network panel
has to fetch all data immediately, so it can be passed into the callback
(except of the response body since there is an extra API for it).
This effectively disables the lazy loading strategy and significantly
slows down the Network panel.


4) Options how to solve the 'lazy loading' problem.

a) Fetch all data immediately
Users who install an extension that registers `onRequestFinished` callback
will experience slower tools. This would have to be properly documented and
users need to know what they are doing. 

Pros: we are compatible with Chrome

b) Introduce new API
We could introduce new API for the `Request` object allowing to fetch lazy
loaded data (ie mimic `getContent` for response body).

An example:

chrome.devtools.network.onRequestFinished.addListener(request => {
  request.getContent(...);
  request.getCookies(...);
  request.getHeaders(...);
};

Cons: we are not compatible with Chrome

c) Any other option?

Suggestions?

Honza
Flags: needinfo?(poirot.alex)
Flags: needinfo?(lgreco)
addListener implementation could listen for networkEvent messages and all networkEventUpdate messages to wait for all fields to be ready, like payload_ready used to do in FirefoxDataProvider. Then force fetching all fields before calling addListener callback. We may share some code with netmonitor provider?
This would fetch all data only if addListener is called.

I don't have strong opinion on exposing an alternative API, it sounds more like a product decision. How many existing addons use current chrome API? Are we interested in seeing them work as-is in Firefox? How typicaly react Addon authors to firefox specific APIs?
Flags: needinfo?(poirot.alex)
I'm not in a position to put any effort toward implementing this API, but I thought I would share why I am interested in it.  The only reason I keep Chrome around currently is for the Django Debug Panel extension (https://github.com/recamshak/chrome-django-panel).  I made a brief attempt to port it to Firefox, but stopped due to the lack of the devtools.network.onRequestFinished API.  That extension uses the API in only one place, for the purpose of extracting a response header (https://github.com/recamshak/chrome-django-panel/blob/823f160ec2908090698ee09c0b48d8aef3e108d4/panel.js#L59).  If an API is provided that will let me access the response headers, that would be sufficient for me to continue working to get the extension functioning with Firefox.
Mike,

since I know you did some analytics, do you have data on how many and maybe even which chrome extensions use chrome.devtools.network.onRequestFinished?
(see above)
Flags: needinfo?(mconca)
There are currently 101 extensions on the Chrome store that use this API (out of a total of approximately 75,000 extensions). It is the 239th most popular API used by extensions. The top extensions that use this API (and have at least 10,000 users) are:

Tamper Chrome (64,168 users)
WASP.inspector: Analytics Solution Profiler (38,178 users)
SAML Chrome Panel (33,513 users)
AJAX Debugger (28,756 users)
dataslayer (27,910 users)
RailsPanel (17,408 users)
SAML DevTools extension (13,287 users)
FirePHP4Chrome (10,140 users)
Flags: needinfo?(mconca)
Not sure but I think the data from the table is populated with this API. Here is a screencast, Honza https://vimeo.com/252078595.
Flags: needinfo?(andrei)
Based on stats from Mike, I believe that this API is important, should be supported in Firefox and the implementation compatible with Chrome.

So, I am planning to fetch all the necessary data that must be passed into the `onRequestFinished` callback immediately. As suggested in comment #6  point 4a.

Let me know if you think otherwise.

Honza
I also agree that we should keep the API that we share with Chrome to be compatible with Chrome if possible (because it makes it easier to port existent extensions and because if make it easier to write extension that can wrong across the browsers that supports the API with less efforts for the addon developer).

The implementation strategy suggested by Alex in comment 6 sounds like a reasonable approach to me too.
Flags: needinfo?(lgreco)
> Based on stats from Mike, I believe that this API is important, should be supported in Firefox and the implementation compatible with Chrome.

SGTM, the list has framework panels that would probably be interested porting to Firefox.
@Luca: I am seeing "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" error when calling `fire.async(data);` in my `onRequestFinished` handler, see the attached patch (wip).

Do you know why?

Honza
Flags: needinfo?(lgreco)
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review220844


Static analysis found 3 defects in this patch.
 - 3 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


::: browser/components/extensions/ext-devtools-network.js:44
(Diff revision 1)
> +                    return [
> +                      "content",
> +                      "utf-8",
> +                    ];
> +                  });
> +                }

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-devtools-network.js:45
(Diff revision 1)
> +                      "content",
> +                      "utf-8",
> +                    ];
> +                  });
> +                }
> +              }

Error: Missing semicolon. [eslint: semi]

::: devtools/client/netmonitor/initializer.js:109
(Diff revision 1)
> +   */
> +  registerOnRequestFinishedListeners(listeners) {
> +    let onRequestAdded = (requestId) => {
> +      let request = getDisplayedRequestById(store.getState(), requestId);
> +      listeners.forEach(listener => listener(request));
> +    }

Error: Missing semicolon. [eslint: semi]
(In reply to Jan Honza Odvarko [:Honza] from comment #17)
> @Luca: I am seeing "Sending message that cannot be cloned. Are you trying to
> send an XPCOM object?" error when calling `fire.async(data);` in my
> `onRequestFinished` handler, see the attached patch (wip).
> 
> Do you know why?

Yes, the reason for that error is that inside the data object that we are "firing"
there is a getContent method (defined at line 37 of the ext-devtools-network.js file
in the attached patch), and an object with methods cannot be "structured cloned" to be
sent across processes.

When a WebExtensions API has to pass to the caller an API object (I mean "an object
that cannot be "structured cloned" and passed across processes)
we have to implement it into a module that is going to run in the same process of the caller
(and eventually split the API implementation in two parts if there is part of it that has to run in the
main process).

Follows some additional details, with links to some docs and code that may be helpful to get
a better picture, about how we could split this event implementation into the two parts:

- one part has to run in the same process of the caller (which is the main process on linux and osx,
  but it is a separate extension process on windows, and the implementation has to be agnostic
  to this difference and always consider it as it is running in a separate process),
  and to achieve it we have to create a new ext-c-devtools-network.js file
  and register it in the ext-c-browser.js file (e.g. as we do for the ext-c-devtools-panels.js file
  here: https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/components/extensions/ext-c-browser.js#18-24)

- in ext-c-devtools-network.js we only need to define the onRequestFinished event,
  and its implementation has to register a listener to the same event defined in the
  main process by ext-devtools-network.js file (which always runs in the main Firefox process),
  this is briefly described in this doc page: https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/events.html#implementing-an-event-in-the-child-process
  (e.g. and ext-c-storage.js does something similar for the onChanged event here:
  https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/toolkit/components/extensions/ext-c-storage.js#162-166)

- in the onRequestFinished implemented in ext-devtools-network.js we have to "fire" an object
  which contains: all the data that is going to be synchronously accessible from the object
  that we pass to the caller (the extension code) and an unique id that can be used by the
  getContent method (which has to be implemented in ext-c-devtools-network.js) to lazily retrieve
  the content of the request, if/when the caller uses the getContent method
  (so that we will never have to retrieve the content and send it across processes 
  if the caller doesn't need it, which is good for performance and memory usage reasons)

- the event fired from the onRequestFinished defined in ext-devtools-network.js is going to
  be received by the onRequestFinished defined in ext-c-devtools-network.js, which
  creates the API object which wraps the data received and provides the getContent API method,
  and then it fires this API object to the extension code
Flags: needinfo?(lgreco)
Let me also use another existent devtools API for an additional example of this kind of issues
and how we solve it in other APIs:

The devtools.panels.create method has to pass to the caller the panel API object,
but to register a panel to the devtools we have to register it on the toolbox from
the main process.

For this reason devtools.panels.create is splitted into two parts:

- the part from ext-c-devtools-panels.js (https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/components/extensions/ext-c-devtools-panels.js#265-281) which:
  - always run in the process of the caller
  - it received the parameters from the caller, it calls the parent process to register
    a panel, and then it receives back an unique id, this unique id is used to pair the panel representation
    in the extension process with its representation in the main process
  - it creates a panel representation and return it as an api object to the caller:
    https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/components/extensions/ext-c-devtools-panels.js#191-236,257

- the part from ext-devtools-panels.js (https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-devtools-panels.js#531-550) which:
  - always run in the main process
  - it is called from the method implementation that lives in ext-c-devtools-panels.js,
    it creates the panel representation in the main process and return an unique id
    to the caller that lives in the extension process (and the unique id is a string
    and can be sent across processes without any issue)
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review221646


Static analysis found 3 defects in this patch.
 - 3 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


::: browser/components/extensions/ext-devtools-network.js:37
(Diff revision 2)
> +
> +          onRequestFinished: new EventManager(context, "devtools.network.onRequestFinished", fire => {
> +            let listener = (request) => {
> +              let data = {
> +                request: {
> +                  test: "test data"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-devtools-network.js:39
(Diff revision 2)
> +            let listener = (request) => {
> +              let data = {
> +                request: {
> +                  test: "test data"
> +                },
> +              }

Error: Missing semicolon. [eslint: semi]

::: devtools/client/netmonitor/initializer.js:109
(Diff revision 2)
> +   */
> +  registerOnRequestFinishedListeners(listeners) {
> +    let onRequestAdded = (requestId) => {
> +      let request = getDisplayedRequestById(store.getState(), requestId);
> +      listeners.forEach(listener => listener(request));
> +    }

Error: Missing semicolon. [eslint: semi]
Luca, I made good progress and the latest attached version is properly exposing `onRequestFinished` API. My test extension [1] is able to receive request data. However, when I am calling `getContent` callback (to fetch response content lazily) I am seeing:

XrayWrapper denied access to property "getContent" (reason: value is callable). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported.

The return value passed from the main process is somehow wrong. 

// The following method is used internally to allow the request API
// piece that is running in the child process to ask the parent process
// to fetch response content lazily from the back-end.
Request: {
  async getContent(requestId) {
    return getNetmonitor(context).then(Netmonitor => {
      return context.cloneScope.Promise.resolve().then(() => {
        console.log("===> main getContent " + requestId);
        return Netmonitor.fetchResponseContent(requestId);
      });
    });
  },
},

The other side (extension process) code looks as follows:

return {
  request: request,
  getContent() {
    console.log("==> ext-c-devtools-network.getContent " + request.id);

    return context.cloneScope.Promise.resolve().then(() => {
      return context.childManager.callParentAsyncFunction(
        "devtools.network.Request.getContent",
        [request.id]
      );
    });
  },
};


Any tips how to fix that?

Honza


[1] https://github.com/devtools-html/har-export-trigger/tree/onrequestfinished
Flags: needinfo?(lgreco)
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review221692


Static analysis found 3 defects in this patch.
 - 3 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


::: browser/components/extensions/ext-devtools-network.js:52
(Diff revision 3)
>              return context.devToolsToolbox.getHARFromNetMonitor();
>            },
> +
> +          onRequestFinished: new EventManager(context, "devtools.network.onRequestFinished", fire => {
> +            let listener = (request) => {
> +              const result = Cu.cloneInto(request, context.cloneScope)

Error: Missing semicolon. [eslint: semi]

::: browser/components/extensions/ext-devtools-network.js:74
(Diff revision 3)
> +          // to fetch response content from the back-end.
> +          Request: {
> +            async getContent(requestId) {
> +              return getNetmonitor(context).then(Netmonitor => {
> +                return context.cloneScope.Promise.resolve().then(() => {
> +                  console.log("===> main getContent " + requestId);

Error: Unexpected console statement. [eslint: no-console]

::: devtools/client/netmonitor/initializer.js:109
(Diff revision 3)
> +   */
> +  registerOnRequestFinishedListeners(listeners) {
> +    let onRequestAdded = (event, requestId) => {
> +      let request = getDisplayedRequestById(store.getState(), requestId);
> +      listeners.forEach(listener => listener(request));
> +    }

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review221700


Static analysis found 1 defect in this patch.
 - 1 defect 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


::: browser/components/extensions/ext-devtools-network.js:74
(Diff revision 4)
> +          // to fetch response content from the back-end.
> +          Request: {
> +            async getContent(requestId) {
> +              return getNetmonitor(context).then(Netmonitor => {
> +                return context.cloneScope.Promise.resolve().then(() => {
> +                  console.log("===> main getContent " + requestId);

Error: Unexpected console statement. [eslint: no-console]
(In reply to Jan Honza Odvarko [:Honza] from comment #24)
> Luca, I made good progress and the latest attached version is properly
> exposing `onRequestFinished` API. My test extension [1] is able to receive
> request data. However, when I am calling `getContent` callback (to fetch
> response content lazily) I am seeing:
> 
> XrayWrapper denied access to property "getContent" (reason: value is
> callable). See https://developer.mozilla.org/en-US/docs/Xray_vision for more
> information. Note that only the first denied property access from a given
> global object will be reported.
Problem solved, I was returning wrong promise from the API.

Honza
Flags: needinfo?(lgreco)
I just briefly looked the attached patches, and also pulled the changes locally to try the API on the test extension, and I've not been able to reproduce the issue described in 24 (the 'XrayWrapper denied access to property "getContent"' error message logged when the extension code calls `request.getContent(...)`), this kind of error message usually means that the API object has not been "cloned" into the target extension scope using `Cu.cloneInto(apiObject, context.cloneScope, {cloneFunctions: true});`, but I see that Cu.cloneInto is currently used as expected in the ext-c-devtools-network.js module, so I guess that you had already fixed the issue in one of the last updates.

I'm going to add some additional comment on the patch asap (e.g. you don't need to use Cu.cloneInto in ext-devtools-network.js, because the object is not going to be accessed by extension code yet from there, it will be sent to the privileged code that lives in ext-c-devtools-network.js and then sent to the extension code, and so you only need Cu.cloneInto in ext-c-devtools-network.js).

Also, I noticed that the `getContent` method is not handling the "callback"-based variant of the API method
(currently the API objects returned, or resolved / passed as a parameter, from an API method or an API event are not wrapped automatically by the schema wrappers, and so they have to handle the callback version of the API call on their own), e.g. using something like:

      getContent(cb) {
        return context.cloneScope.Promise.resolve().then(async () => {
          const result = await context.childManager.callParentAsyncFunction(
            "devtools.network.Request.getContent",
            [request.id]
          );

          if (cb) {
            // If the API method has been called with a callback,
            // call it with the request content as its parameter. 
            context.runSafe(cb, result);
          }

          // Resolves the returned promise to the retrieved request content. 
          return result;
        });
      },
(In reply to Luca Greco [:rpl] from comment #30)
> I just briefly looked the attached patches, and also pulled the changes
> locally to try the API on the test extension, and I've not been able to
> reproduce the issue described in 24 (the 'XrayWrapper denied access to
> property "getContent"' error message logged when the extension code calls
> `request.getContent(...)`),

Interestingly I am still seeing the exception, but it doesn't seem to break
the behavior.

XrayWrapper denied access to property "disconnect" (reason: value is callable). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported.

 
> Also, I noticed that the `getContent` method is not handling the
> "callback"-based variant of the API method
Yep, done now.

Thanks!
Honza
The last missing pieces:
- provide data to the `onRequestFinished` callback as valid HAR structure
- fetch all lazy loaded data automatically.

Otherwise, the basic structure seems to be ready
(except the exception shouldn't appear)

Honza
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review222326


Static analysis found 1 defect in this patch.
 - 1 defect 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


::: browser/components/extensions/ext-devtools-network.js:52
(Diff revision 7)
>              return context.devToolsToolbox.getHARFromNetMonitor();
>            },
> +
> +          onRequestFinished: new EventManager(context, "devtools.network.onRequestFinished", fire => {
> +            let listener = (data) => {
> +              const result = Cu.cloneInto(data, context.cloneScope);

Error: 'result' is assigned a value but never used. Allowed unused vars must match /^(Cc|Ci|Cr|Cu|EXPORTED_SYMBOLS)$/. [eslint: no-unused-vars]
Luca, this new version is ready for first review.

I am using HARExportTrigger extension for manual testing.

HARExportTrigger extension repo:
https://github.com/devtools-html/har-export-trigger/tree/master

Test page with detailed instructions:
http://softwareishard.com/test/harexporttrigger/
* It can be used to test getHAR as well as onRequestFinished API

Honza
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review222328


Static analysis found 1 defect in this patch.
 - 1 defect 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


::: browser/components/extensions/ext-devtools-network.js:73
(Diff revision 8)
> +          // to fetch response content from the back-end.
> +          Request: {
> +            async getContent(requestId) {
> +              let Netmonitor = await getNetmonitor(context);
> +              return context.cloneScope.Promise.resolve().then(async () => {
> +                return await Netmonitor.fetchResponseContent(requestId);

Error: Redundant use of `await` on a return value. [eslint: no-return-await]
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review222428

Hi Jan,
sorry for the let you waiting for a more detailed review of this patch, I collected some initial review comments last week but I wanted to also take a deeper look to the part that subscribes the listeners.

Besides some minor tweaks, in my opinion, the following are currently the main missing pieces:

- fix the issues around the onRequestFinished listeners (described in more detail in the comments below)
- add an additional test case for the newly implemented API event

Another detail that I think we should evaluate is what kind of behavior getHAR and onRequestFinished should have when the toolbox target is not a web page (e.g. if it is a privileged tab, like about:addons, or an extension page, especially if related to a different webpage), my guess is that we should return an "empty HAR logs object" or even better we could return a rejected promise from getHAR, and that the onRequestFinished listener should not be fired for this kind of requests.

Follows more detailed comments.

::: browser/components/extensions/ext-c-devtools-network.js:12
(Diff revision 9)
> + *
> + * @param {DevtoolsExtensionContext}
> + *   A devtools extension context running in a child process.
> + * @param {object} options
> + */
> +class ChildNetworkResponseLoader extends ExtensionUtils.EventEmitter {

It looks that we never subscribe or fire events from this class, and so it seems that we don't need to extend EventEmitter here.

::: browser/components/extensions/ext-c-devtools-network.js:17
(Diff revision 9)
> +class ChildNetworkResponseLoader extends ExtensionUtils.EventEmitter {
> +  constructor(context, requestId) {
> +    super();
> +
> +    this.context = context;
> +    this.context.callOnClose(this);

This class doesn't seem to have a `close` method (and it seems that currently the only thing that the `close` method can actually do is to nullify `this.context` and `this.requestId`).

::: browser/components/extensions/ext-c-devtools-network.js:37
(Diff revision 9)
> +            // If the API method has been called with a callback,
> +            // call it with the request content as its parameter.
> +            context.runSafe(callback, result.content.text, result.content.mimeType);
> +          }
> +
> +          return Cu.cloneInto(result.content, context.cloneScope);

We should try to keep consistent the way we pass the result as the parameters of the callback function or as the resolved value of the returned promise.

And so this looks that should resolve to an array of two elements (the content and the mimetype):

```
return Cu.cloneInto([result.content.text, result.content.mimetype], context.cloneScope);
```

::: browser/components/extensions/ext-c-devtools-network.js:37
(Diff revision 9)
> +            // If the API method has been called with a callback,
> +            // call it with the request content as its parameter.
> +            context.runSafe(callback, result.content.text, result.content.mimeType);
> +          }
> +
> +          return Cu.cloneInto(result.content, context.cloneScope);

Given that we have to always clone the values for the returned promise, it looks that we could clone the result only once and use the cloned values for both (`context.runSafe` is going to clone the parameters internally), e.g. something like:

```
...

const values = Cu.cloneInto([...], context.cloneScope);

if (callback) {
  context.runSafeWithoutClone(callback, ...values)
}

return values;
```

::: browser/components/extensions/ext-devtools-network.js:8
(Diff revision 9)
>  "use strict";
>  
>  // The ext-* files are imported into the same scopes.
>  /* import-globals-from ext-devtools.js */
>  
> +var listeners = null;

How about using a more specific for the set of listeners? (eg. something like `netmonitorListeners`)

Besides its name, if I'm not reading the Netmonitor part wrong, we cannot assume that the listeners subscribed by the installed webextensions that are using this API are all related to the same Netmonitor instance, e.g. because the Netmonitor instance is related to a particular toolbox and even a single extension can have multiple active extension contexts related to different active toolboxes.

e.g. if we install the har-trigger-export extension (https://github.com/devtools-html/har-export-trigger/tree/onrequestfinished), we open the test page in two different tabs （http://softwareishard.com/test/harexporttrigger/）, and then we open the DevTools toolbox on both the tabs, 
I would expect the onRequestFinished listeners to be only fired for the first tab which registers a listener (because when the second one is going to register its listeners, `listener` is not going to be null anymore and `registerListeners` would not be called for it).

I'm wondering if a strategy similar to the following may be reasonable:

- the toolbox instance could be where the Set of the subscribed onRequestFinished listeners is actually stored
- the onRequestFinished subscribes/unsubscribe its listeners to the toolbox object (e.g. `context.devToolsToolbox.registerRequestFinishedListener(listener)`) as soon as it called (without waiting the Netmonitor to be ready)
- the Netmonitor panel receives the reference to the set of the subscribed listeners from the toolbox when it is being created (and then it knows that it has to create and send the requests data if there are listeners subscribed)
- when all the onRequestFinished listeners are unregistered from the toolbox object, the toolbox object may tell the Netmonitor panel (if there is a Netmonitor panel currently active in the toolbox) that there is not need to collect the request data to be sent to these listener anymore.

How that sounds to you?

By using a strategy similar to the above, we also don't need to wait the netmonitor panel to be ready to be able to subscribe a listener to it (which would also help to handle another scenario that I think that it is a bit tricky with the current implementation: if an extension is waiting for the netmonitor to be ready to subscribe its listener, and then the addon is uninstalled, `Netmonitor.registerOnRequestFinishedListeners` would still be called as soon as the network panel is selected by the user).

Follows some additional information about the lifecycle of the module, the ExtensionAPI sub-class and its getAPI method:

The ext-devtools-network.js module will be loaded once, as soon as an installed extension is going to use it, and then the class that extends ExtensionAPI is used internally to create an instance of it per extension, and the getAPI method will be called once per context related to the same extension (e.g. every extension page is represented by an extension context instance, and every subframe of an extension page that is still pointed to an extension url is also represented as an extension context instance, and in the main process in particular an extension context instance is created to proxy every extension context which is running in the child processes and needs to execute some code in the main process, e.g. like for the request.getContent API method).

::: browser/components/extensions/ext-devtools-network.js:23
(Diff revision 9)
> +
> +    function registerListeners(context) {
> +      listeners = new Set();
> +
> +      getNetmonitor(context).then(Netmonitor => {
> +        Netmonitor.registerOnRequestFinishedListeners(listeners);

I'm wondering if we should also unregister the set of listeners from the Netmonitor at some point (e.g. once the set becomes empty because all the listeners have been unsubscribed, e.g. because all the extension has been disabled while the toolbox was still open).

::: browser/components/extensions/ext-devtools-network.js:72
(Diff revision 9)
> +          // piece that is running in the child process to ask the parent process
> +          // to fetch response content from the back-end.
> +          Request: {
> +            async getContent(requestId) {
> +              let Netmonitor = await getNetmonitor(context);
> +              return context.cloneScope.Promise.resolve().then(async () => {

This code is running in the main process and the returned promise in not received by the extension code, it is used by the WebExtensions internals to send the result to the code running in the extension process, and so there is no need to return a Promise instance coming from the context.cloneScope and we can just return the result:

```
async getContent(requestId) {
  const netmonitor = await getNetmonitor(context);
  return netmonitor.fetchResponseContent(requestId);
}
```

::: devtools/client/netmonitor/initializer.js:141
(Diff revision 9)
> +
> +  /**
> +   * Support for `Request.getContent` (lazy load response body)
> +   */
> +  fetchResponseContent(requestId) {
> +    let request = getDisplayedRequestById(store.getState(), requestId);

I'm wondering if/how we should handle a bit more explictly what it should happen here (and for the extension code that has subscribed the event) if the request object with the given requestId doesn't exist (or doesn't exist anymore).

e.g. given that the extension pages, as an extension devtools panel, and the target inspected page have different lifecycles, the request API object that we give to the extension may be related to a request related to the previous page load, which I guess that are still alive only if the "persistent logs" option is enabled on the network panel, am I right?

Also, how this API works in this scenario on Chrome?
Attachment #8945062 - Flags: review?(lgreco) → review-
(In reply to Luca Greco [:rpl] from comment #40)
> Comment on attachment 8945062 [details]
> Bug 1311171 - Implement the devtools.network.onRequestFinished API event;
> 
> https://reviewboard.mozilla.org/r/215284/#review222428
> 
> Hi Jan,
> sorry for the let you waiting for a more detailed review of this patch, I
> collected some initial review comments last week but I wanted to also take a
> deeper look to the part that subscribes the listeners.
Thanks for great feedback!

> Besides some minor tweaks, in my opinion, the following are currently the
> main missing pieces:
> 
> - fix the issues around the onRequestFinished listeners (described in more
> detail in the comments below)
> - add an additional test case for the newly implemented API event
Is there any good example I could get some inspiration from?

> Another detail that I think we should evaluate is what kind of behavior
> getHAR and onRequestFinished should have when the toolbox target is not a
> web page (e.g. if it is a privileged tab, like about:addons, or an extension
> page, especially if related to a different webpage), my guess is that we
> should return an "empty HAR logs object" or even better we could return a
> rejected promise from getHAR, and that the onRequestFinished listener should
> not be fired for this kind of requests.
The network panel doesn't collect requests from these pages (no content),
so there is nothing to export. But also, is there any security implication?

> 
> Follows more detailed comments.
> 
> ::: browser/components/extensions/ext-c-devtools-network.js:12
> (Diff revision 9)
> > + *
> > + * @param {DevtoolsExtensionContext}
> > + *   A devtools extension context running in a child process.
> > + * @param {object} options
> > + */
> > +class ChildNetworkResponseLoader extends ExtensionUtils.EventEmitter {
> 
> It looks that we never subscribe or fire events from this class, and so it
> seems that we don't need to extend EventEmitter here.
Removed

> ::: browser/components/extensions/ext-c-devtools-network.js:17
> (Diff revision 9)
> > +class ChildNetworkResponseLoader extends ExtensionUtils.EventEmitter {
> > +  constructor(context, requestId) {
> > +    super();
> > +
> > +    this.context = context;
> > +    this.context.callOnClose(this);
> 
> This class doesn't seem to have a `close` method (and it seems that
> currently the only thing that the `close` method can actually do is to
> nullify `this.context` and `this.requestId`).
`callOnClose` Removed


> ```
> ...
> 
> const values = Cu.cloneInto([...], context.cloneScope);
> 
> if (callback) {
>   context.runSafeWithoutClone(callback, ...values)
> }
> 
> return values;
> ```
The method returns the following, but it doesn't feel right.

return {
  content: values[0],
  encoding: values[1],
}

... so, the client can do:

request.getContent().then(({content, encoding}) => {
});



But, perhaps we should return void? I am not seeing anything
about return value in Chrome doc and we should probably do the same?

> I'm wondering if a strategy similar to the following may be reasonable:
> 
> - the toolbox instance could be where the Set of the subscribed
> onRequestFinished listeners is actually stored
> - the onRequestFinished subscribes/unsubscribe its listeners to the toolbox
> object (e.g.
> `context.devToolsToolbox.registerRequestFinishedListener(listener)`) as soon
> as it called (without waiting the Netmonitor to be ready)
> - the Netmonitor panel receives the reference to the set of the subscribed
> listeners from the toolbox when it is being created (and then it knows that
> it has to create and send the requests data if there are listeners
> subscribed)
> - when all the onRequestFinished listeners are unregistered from the toolbox
> object, the toolbox object may tell the Netmonitor panel (if there is a
> Netmonitor panel currently active in the toolbox) that there is not need to
> collect the request data to be sent to these listener anymore.
Sounds good and I implemented that this way.

> The ext-devtools-network.js module will be loaded once, as soon as an
> installed extension is going to use it, and then the class that extends
> ExtensionAPI is used internally to create an instance of it per extension,
> and the getAPI method will be called once per context related to the same
> extension (e.g. every extension page is represented by an extension context
> instance, and every subframe of an extension page that is still pointed to
> an extension url is also represented as an extension context instance, and
> in the main process in particular an extension context instance is created
> to proxy every extension context which is running in the child processes and
> needs to execute some code in the main process, e.g. like for the
> request.getContent API method).
Is there any MDN doc about this? What exactly do you refer to by "extension page"?

> and we can just return the result:
> 
> ```
> async getContent(requestId) {
>   const netmonitor = await getNetmonitor(context);
>   return netmonitor.fetchResponseContent(requestId);
> }
> ```
Done

> > +  fetchResponseContent(requestId) {
> > +    let request = getDisplayedRequestById(store.getState(), requestId);
> 
> I'm wondering if/how we should handle a bit more explictly what it should
> happen here (and for the extension code that has subscribed the event) if
> the request object with the given requestId doesn't exist (or doesn't exist
> anymore).
Done

> e.g. given that the extension pages, as an extension devtools panel, and the
> target inspected page have different lifecycles, the request API object that
> we give to the extension may be related to a request related to the previous
> page load, which I guess that are still alive only if the "persistent logs"
> option is enabled on the network panel, am I right?
> 
> Also, how this API works in this scenario on Chrome?
It returns the response-content even if you clear the Network panel content
Our impl works the same now.

Honza
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review222960

Hi Honza, follows some further suggested tweaks on the API implementation (e.g. to handle the error scenario, support both the "callback"-based and "promise"-based API modes, and cleanup/simplify the code a bit) and to the way the toolbox is going to expose the registering/unregistering of the onRequestFinished listeners (which should also evaluated with and reviewed by jdescottes).

About the new test case, the test case you added for getHAR in Bug 1311177 [1] seems a good starting point / source of inspiration for the new test case needed for onRequestFinished, what we want is basically testing that:

- the onRequestFinished event is fired when expected
- the request object contains the expected properties 
- the request.getContent() API method support both the callback and promise API modes
  and that it provides the expected content and mimeType

[1]: https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/browser/components/extensions/test/browser/browser_ext_devtools_network.js#130

::: browser/components/extensions/ext-c-devtools-network.js:23
(Diff revision 10)
> +  api() {
> +    const {context, requestId} = this;
> +    return {
> +      getContent(callback) {
> +        return context.cloneScope.Promise.resolve().then(async () => {
> +          const result = await context.childManager.callParentAsyncFunction(

I just noticed that we should also take care of the error scenario here (which means that chrome.runtime.lastError should be set to the rejected value when the API has been called with a callback, otherwise the returned promise is supposed to be rejected, and in both cases the error object that is provided to the extension code should be an instance of context.cloneScope.Error, so that it can be accessed by the extension code).

The `context.childManager.callParentAsyncFunction` helper is already calling `context.wrapPromise` (which is where the above behavior is already implemented) internally and it supports an optional callback parameter, and so we can refactor this API method to cover this cases and also simplify it in the process:

- in ext-devtools-network.js we need to apply the following changes to the getContent method:

```
var {
  SpreadArgs
} = ExtensionCommon;

...
          Request: {
            async getContent(requestId) {
              const netmonitor = await getNetmonitor(context);

              return netmonitor.fetchResponseContent(requestId)
                .then(({content}) => new SpreadArgs([content.text, content.mimeType]))
                .catch(err => {
                  const errorMsg = "Unexpected error while fetching response content";
                  Cu.reportError(`${errorMsg} for ${requestId}: ${err}`);

                  throw new ExtensionError(errorMsg);
                });
            },
          },
```

- and in ext-c-devtools-network the following ones:

```
  api() {
    return {
      getContent: (callback) => {
        return this.context.childManager.callParentAsyncFunction(
          "devtools.network.Request.getContent",
          [this.requestId],
          callback);
      },
    };
  }
```

`SpreadArgs` is used to tell to the WebExtension internals that the results should be passed as the two arguments of the callback (if the Chrome-compatible/callback-based API form is being used) or as a single array object parameter of the returned promise if the callback is missing (which means that we are using the promise-based API, which is currently natively supported only on Firefox), as in the following example:

```
// In the "callback"-based mode:

response.getContent((contentText, mimeType) => {
  if (chrome.runtime.lastError) {
    // chrome.runtime.lastError contains an error raised from request.getContent(...).
  }
  
  // otherwise contextText and mimeType contains the expected results.
});

// In the "promise"-based mode:

response.getContent()
  .then(([contentText, mimeType]) => {
    // The two callback parameters becomes a single array parameter
    // of the resolved promise (because a promise can be resolved only with a single value.
  })
  .catch(err => {
    // err is the same error that can be retrieved from chrome.runtime.lastError
    // in the "callback"-based mode.
  });
```

::: browser/components/extensions/ext-devtools-network.js:12
(Diff revision 10)
>  
>  this.devtools_network = class extends ExtensionAPI {
>    getAPI(context) {
> +    function getNetmonitor(context) {
> +      let toolbox = context.devToolsToolbox;
> +      return toolbox.getPanelWhenReady("netmonitor").then(panel => {

In this new version of the patch, getNetmonitor seems to be used only by the Response's getContent API method, and so I'm wondering if `getPanelWhenReady` is still necessary or if we could just use getPanel to retrieve the panel, by assuming that the netmonitor panel should already be ready because the request finished event couldn't be fired otherwise (and that it the panel is not available we can raise an error because it is unexpected).

::: browser/components/extensions/ext-devtools-network.js:45
(Diff revision 10)
> +          onRequestFinished: new EventManager(context, "devtools.network.onRequestFinished", fire => {
> +            let listener = (data) => {
> +              fire.async(data);
> +            };
> +
> +            let listeners = context.devToolsToolbox.onRequestFinishedListeners;

Retrieveing the Set of the onRequestFinished listeners from the toolbox and manipulating it directly from here doesn't feel right, personally I would prefer if the WebExtensions API implementation code could use a couple of toolbox methods to abstract the manipulation of the registered listeners, something like:


```
const toolbox = context.devToolsToolbox;

toolbox.addRequestedFinishedListener(listener);

return () => {
  toolbox.removeRequestFinishedListener(listener);
};
```

The set of the proposed changes to toolbox.js should also be reviewed by jdescottes, so that he can provide his feedback on the currently proposed strategy (and then give his final sign-off on the strategy that we are going to agree on).

::: browser/components/extensions/ext-devtools-network.js:59
(Diff revision 10)
> +          // piece that is running in the child process to ask the parent process
> +          // to fetch response content from the back-end.
> +          Request: {
> +            async getContent(requestId) {
> +              let netmonitor = await getNetmonitor(context);
> +              return netmonitor.fetchResponseContent(requestId);

From the fetchResponseContent method is not immediately clear which kind of error message is going to be raised for a non existent request, to prevent it from producing an error that doesn't make sense for the user how about using something like the following approach:

- catch the featchResponseContent rejection
- log the original error (e.g. using Cu.reportError)
- throw a new ExtensionError which contains the explicit error that we want to provide to the extension code

(I've include am implementation of this behavior in one of the previous comment attached to ext-c-devtools-network.js)
Attachment #8945062 - Flags: review?(lgreco) → review-
(In reply to Jan Honza Odvarko [:Honza] from comment #41)
> Is there any MDN doc about this? What exactly do you refer to by "extension page"?

Not on MDN because these are concepts related to the internals of the WebExtensions API implementation,
(and so they should be part of the docs https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/
but it seems that we don't have these concepts explicitly described in these docs yet).

We basically use "Extension Context" to refer to the "globals" (windows or sandboxes) where the extension code is going to run (and has the WebExtensions APIs available), and "Extension Page" to refer to the abstraction that provides the XUL browser element that contains a tree of "Extension Contexts" that share the same top level "Extension Context".

e.g. the background page is an "Extension Page" and it contains at least an "Extension Context" that is the top level window for the background page (but there could be more "Extension Contexts", e.g. if the background page contains an iframe that is also pointed to an extension url), other extension pages are the browserAction and pageActions popups, the sidebarActon page, the devtools page (the invisible one), the devtools panel, as well as a browser tab which has loaded an extension url.
(In reply to Luca Greco [:rpl] from comment #44)
> Comment on attachment 8945062 [details]
> Bug 1311171 - Implement the devtools.network.onRequestFinished API event;
> 
All comments resolved, thanks Luca!

Julian, can you please look at the changes related to the Toolbox?

Honza
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review223212


Static analysis found 3 defects in this patch.
 - 3 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


::: browser/components/extensions/ext-devtools-network.js:9
(Diff revision 12)
>  
>  // The ext-* files are imported into the same scopes.
>  /* import-globals-from ext-devtools.js */
>  
> +var {
> +  SpreadArgs

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:70
(Diff revision 12)
> +    browser.test.sendMessage("onRequestFinished", requestFinishedCount);
> +
> +    // Get response content using callback
> +    request.getContent((content, encoding) => {
> +      browser.test.sendMessage("onRequestFinished-callbackExecuted",
> +        [content, encoding]);

Error: Expected indentation of 31 spaces but found 8. [eslint: indent]

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:76
(Diff revision 12)
> +    });
> +
> +    // Get response content using returned promise
> +    request.getContent().then(([content, encoding]) => {
> +      browser.test.sendMessage("onRequestFinished-promiseResolved",
> +        [content, encoding]);

Error: Expected indentation of 31 spaces but found 8. [eslint: indent]
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review223230

Thanks Honza!

The changes to toolbox.js look good. I have a side question about calling the listeners from netmonitor/initializer.js.

::: devtools/client/framework/toolbox.js:3022
(Diff revision 13)
>  
>      // Use Netmonitor object to get the current HAR log.
>      return netPanel.panelWin.Netmonitor.getHar();
> -  }
> +  },
> +
> +  addRequestFinishedListener: function (listener) {

Add a small JSDoc about the signature of the listener (listener is expected to be a function that takes ({harEntry, requestId}) as first argument.

::: devtools/client/netmonitor/initializer.js:71
(Diff revision 13)
>        let top = iframe.ownerDocument.defaultView.top;
>        top.openUILinkIn(link, "tab");
>      };
>  
> +    this.onRequestAdded = this.onRequestAdded.bind(this);
> +    window.on(EVENTS.REQUEST_ADDED, this.onRequestAdded);

Usually this code is only executed when the netmonitor panel is loaded (ie when the tool is selected).

Is there a something that forces the netmonitor UI to be initialized when a webextension wants to be notified onRequestFinished? 
I can't see this in the current patch but I might have missed something.

If we ensure that the netmonitor UI is started before webextensions use this API
=> then we need a comment to explain why it's safe to rely on netmonitor's UI for an API which is not related to the UI.
Attachment #8945062 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #50)
> Add a small JSDoc about the signature of the listener (listener is expected
> to be a function that takes ({harEntry, requestId}) as first argument.
Done

> Is there a something that forces the netmonitor UI to be initialized when a
> webextension wants to be notified onRequestFinished? 
> I can't see this in the current patch but I might have missed something.
I added a `this.loadTool("netmonitor")` call in the `addListener` method.

I'll be yet investigating if it's possible to start HTTP intercepting and
fire `onRequestFinished` events (and also use `getHAR`) without the Network
panel being loaded. This means creating the store + connectors for the
backend with no UI and consequently using it when the panel opens.
I'll work on this in bug 1416748. 

It could also simply help if the Network panel is not doing React UI update
if running in the background - should be fixed in bug 1419350.

Thanks!
Honza
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review223298

Thanks! In my opinion this is an acceptable compromise. 

For now, if a webextension needs to be notified about requests, DevTools will need to start the netmonitor UI as soon as a onRequestFinished listener is added.
Attachment #8945062 - Flags: review?(jdescottes) → review+
@Luca, the test if failing now - after adding `this.loadTool("netmonitor")` call in the `addListener` method. I am not sure why yet, but I am worried if this isn't a bit hacky and if there is some fundamental problem with async panel initialization in the test.

Honza
Forcing the load of netmonitor feels hacky as the usage of onRequestFinished doesn't relate to netmonitor usage.
I mean that, when a webextension uses this API, it doesn't mean the user will open/use the netmonitor.
If you look at the two most popular chrome addons (tamper and wasp), they both open their own toolbox panel or tab.
(By the way, is there any way to integrate *into* netmonitor UI, like having a new sidebar implemented by a webext?)

I would discourage force-loading the netmonitor, if possible.
It sounds even more important until bug 1419350 is fixed.

Also, this is not the only one bug/feature we are actively working on that would benefit from having the firefox-data-provider/provider/connector logic pulled off of netmonitor UI.
(In reply to Alexandre Poirot [:ochameau] from comment #55)
> Forcing the load of netmonitor feels hacky as the usage of onRequestFinished
> doesn't relate to netmonitor usage.
> I mean that, when a webextension uses this API, it doesn't mean the user
> will open/use the netmonitor.
> If you look at the two most popular chrome addons (tamper and wasp), they
> both open their own toolbox panel or tab.
> (By the way, is there any way to integrate *into* netmonitor UI, like having
> a new sidebar implemented by a webext?)
> 
> I would discourage force-loading the netmonitor, if possible.
> It sounds even more important until bug 1419350 is fixed.
> 
> Also, this is not the only one bug/feature we are actively working on that
> would benefit from having the firefox-data-provider/provider/connector logic
> pulled off of netmonitor UI.
Agree with all and I'll be working on this in bug 1416748. 

Let's yet see what Luca says (he's traveling today, so his response
might take some time)

Honza
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review223646

Hi Honza, follows another round of review comments.

The WE API implementation side of the patch looks almost ready, e.g. I think that part of the Request.getContent method implementation could be refactored out into another toolbox method, which can better abstract the "devtools internals"-side of fetching the response content given a requestId.

This can also help to fix in a follow up the current limitations of getHAR and onRequestFinished (related to the netmonitor panel that has to be loaded before any of this API could start to provide the expected results) without have further change to the API implementation itself.

::: browser/components/extensions/ext-devtools-network.js:56
(Diff revision 15)
> +              const panel = toolbox.getPanel("netmonitor");
> +              if (!panel) {
> +                const errorMsg = "Unexpected error: Network monitor panel doesn't exist!";
> +                Cu.reportError(errorMsg);
> +                throw new ExtensionError(errorMsg);
> +              }
> +
> +              return panel.panelWin.Netmonitor.fetchResponseContent(requestId)

I'm wondering if it would not be better to move this part into a new toolbox method (mostly because this seems to be very specific implementation details internals of the toolbox and the particular toolbox panel, that could change indipendently from the API implementation, if we abstract it from the internal devtools implementation details).

e.g. this method could become (from a "WE API implementation" point of view) something like:

```
Request: {
  async getContent(requestId) {
    return context.devToolsToolbox
      .fetchResponseContent(requestId)
      .catch(error => {
         ...
      });
  }
}
```

(and, once the toolbox would be able to retrieve the requests without the netmonitor panel to be loaded, it is very likely that we don't need to change this API implementation)

::: browser/components/extensions/ext-devtools-network.js:59
(Diff revision 15)
> +            async getContent(requestId) {
> +              const toolbox = context.devToolsToolbox;
> +              const panel = toolbox.getPanel("netmonitor");
> +              if (!panel) {
> +                const errorMsg = "Unexpected error: Network monitor panel doesn't exist!";
> +                Cu.reportError(errorMsg);

Nit, it seems to me that we don't need to report this error twice (because the error that we are including in the rejection has exactly the same error message, on the contrary the one from line 7 an error that contains the internal error is part of the message logged using Cu.reportError, and a more friendly one is rejected, to be received by the extension code).

::: browser/components/extensions/ext-devtools-network.js:67
(Diff revision 15)
> +
> +              return panel.panelWin.Netmonitor.fetchResponseContent(requestId)
> +                .then(({content}) => new SpreadArgs([content.text, content.mimeType]))
> +                .catch(err => {
> +                  const errorMsg = "Unexpected error while fetching response content";
> +                  Cu.reportError(`${errorMsg} for ${requestId}: ${err}`);

Nit, we could also include `extension.policy.debugName` as part of this error message (only in the string logged using Cu.reportError, so that it would also include enough information to identify which extension has originated the issue)

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:79
(Diff revision 15)
> +    request.getContent().then(([content, encoding]) => {
> +      browser.test.sendMessage("onRequestFinished-promiseResolved",
> +                               [content, encoding]);
> +    });
> +
> +    requestFinishedCount = 0;

Is this counter needed? it seems to be set to 0 right after we receive an onRequestFinished event.

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:234
(Diff revision 15)
> +
> +  let eventCount = await extension.awaitMessage("onRequestFinished");
> +  is(eventCount, 1, "The expected number of events were fired.");
> +
> +  // Wait for response content being fetched.
> +  let [result1, result2] = await Promise.all([

Nit, how about renaming these two into something like callbackRes and promiseRes? (or a different pair of names that immediately identify which one is related to the two different API mode).

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:239
(Diff revision 15)
> +  let [result1, result2] = await Promise.all([
> +    extension.awaitMessage("onRequestFinished-callbackExecuted"),
> +    extension.awaitMessage("onRequestFinished-promiseResolved"),
> +  ]);
> +
> +  ok(result1[0].startsWith("<html>"), "The expected content has been retrieved.");

Nit, we could check that the first one contains the expected value and the second one is exactly equal to the first one, eg. something like:

```
ok(result1[0].startsWith("<html>"), "...");
ok(result1[1], "...", "...");

is(result2[0], result1[0], "The resolved value is equal to the one received in the callback API mode")  
is(result2[1], result1[1], "The resolved value is equal to the one received in the callback API mode")  
```

::: devtools/client/framework/toolbox.js:3036
(Diff revision 15)
> +    this._requestFinishedListeners.add(listener);
> +
> +    // HTTP traffic is intercepted only if the Network panel exists,
> +    // so make sure the "netmonitor" tool is loaded when there is
> +    // a listener for `onRequestFinished` events.
> +    this.loadTool("netmonitor");

I agree with Alex on this: 
implicitly loading the netmonitor is going to introduce a performance penalty on the toolbox loading time if any extension that uses this event has been installed by the user. 

Besides that, the devtools_page is executed when the toolbox is being created (toolbox-created) but not yet ready (toolbox-ready), and so calling `this.loadTool("netmonitor")` is likely to fail when the toolbox is not ready yet (and my guess is that this is the reason why the test started to fail when this has been added to the addRequestFinishedListener method).

I think that it also worth a mention that currently getHAR has a similar limitation:
it returns an empty HAR log if the netmonitor panel has never been loaded yet.

But, once onRequestFinished events are going to be fired even when the netmonitor panel has not been opened yet, getHAR should also be able to return the expected　ＨＡＲ log as well, or it would be even more confusing.

I would be fine if the initial version of these APIs (both getHAR and onRequestFinished) are limited (and require the user to have activated the netmonitor panel first), and then we remove this limitation in a follow up, once we have fixed the underlying issues (so that we can receive the events and generate the related HAR log indipendently from the netmonitor panel).
Attachment #8945062 - Flags: review?(lgreco) → review-
(In reply to Luca Greco [:rpl] from comment #58)
> Comment on attachment 8945062 [details]
> Bug 1311171 - Implement the devtools.network.onRequestFinished API event;
> 
> https://reviewboard.mozilla.org/r/215284/#review223646

Thanks Luca for the review, all comments resolved.

Julian, re-requesting review as well, there have been some
changes and mainly the Network panel is not loaded automatically
anymore.

Honza
Attachment #8945062 - Flags: review+ → review?(jdescottes)
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review223836


Static analysis found 5 defects in this patch.
 - 5 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


::: browser/components/extensions/ext-devtools-network.js:58
(Diff revision 16)
> +          Request: {
> +            async getContent(requestId) {
> +              return context.devToolsToolbox.fetchResponseContent(requestId)
> +                .then(({content}) => new SpreadArgs([content.text, content.mimeType]))
> +                .catch(err => {
> +                  const debugName = extension.policy.debugName;

Error: 'extension' is not defined. [eslint: no-undef]

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:236
(Diff revision 16)
> +    extension.awaitMessage("onRequestFinished-callbackExecuted"),
> +    extension.awaitMessage("onRequestFinished-promiseResolved"),
> +  ]);
> +
> +  ok(callbackRes[0].startsWith("<html>"),
> +    "The expected content has been retrieved.");

Error: Expected indentation of 5 spaces but found 4. [eslint: indent]

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:238
(Diff revision 16)
> +  ]);
> +
> +  ok(callbackRes[0].startsWith("<html>"),
> +    "The expected content has been retrieved.");
> +  is(callbackRes[1], "text/html; charset=utf-8",
> +    "The expected content has been retrieved.");

Error: Expected indentation of 5 spaces but found 4. [eslint: indent]

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:241
(Diff revision 16)
> +    "The expected content has been retrieved.");
> +  is(callbackRes[1], "text/html; charset=utf-8",
> +    "The expected content has been retrieved.");
> +
> +  is(promiseRes[0], callbackRes[0],
> +    "The resolved value is equal to the one received in the callback API mode");

Error: Expected indentation of 5 spaces but found 4. [eslint: indent]

::: browser/components/extensions/test/browser/browser_ext_devtools_network.js:243
(Diff revision 16)
> +    "The expected content has been retrieved.");
> +
> +  is(promiseRes[0], callbackRes[0],
> +    "The resolved value is equal to the one received in the callback API mode");
> +  is(promiseRes[1], callbackRes[1],
> +    "The resolved value is equal to the one received in the callback API mode");

Error: Expected indentation of 5 spaces but found 4. [eslint: indent]
Attachment #8945062 - Flags: review?(jdescottes)
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review224098

Code wise this is good, but I'm not comfortable with the new approach. I'm clearing the review flag because I need to understand better what we are landing here.

I don't think we should ship a feature which only works under some unclear conditions. Wouldn't it be frustrating for extension developers to work on an extension and release it only to discover later that it doesn't work unless users start the netmonitor first? In my opinion, the previous approach at least had a predictable behaviour. Here we need to explain to developers what is the impact of using this feature, and in turn they also need to explain it to their own users. If forcing the netmonitor UI to start is not an option, I personally think we should block this on Bug 1416748 and not land it alone. Or have really strong documentation and warning messages. 

Luca: "I would be fine if the initial version of these APIs (both getHAR and onRequestFinished) are limited (and require the user to have activated the netmonitor panel first)"
Do you have anything in mind to communicate this to developers who will use the APIs?

::: devtools/client/framework/toolbox.js:3031
(Diff revision 17)
> +   *        The listener to be called it's expected to be
> +   *        a function that takes ({harEntry, requestId})
> +   *        as first argument.
> +   */
> +  addRequestFinishedListener: function (listener) {
> +    this._requestFinishedListeners.add(listener);

We need to warn developers that their extension will not catch any request unless netmonitor has been started I think.

I'm not sure where the best spot is for such a warning. Maybe here, if they try to add a listener, log a message to the console (and I would do so regardless of the netmonitor being started or not, developers need to know that they are using a very flaky feature that will only work if users have started the netmonitor before).
Attachment #8945062 - Flags: review?(jdescottes)
Luca: what is your opinion about my comment above?
Flags: needinfo?(lgreco)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #63)
> Comment on attachment 8945062 [details]
> Bug 1311171 - Implement the devtools.network.onRequestFinished API event;
> 
> https://reviewboard.mozilla.org/r/215284/#review224098
> 
> Code wise this is good, but I'm not comfortable with the new approach. I'm
> clearing the review flag because I need to understand better what we are
> landing here.
> 
> I don't think we should ship a feature which only works under some unclear
> conditions. Wouldn't it be frustrating for extension developers to work on
> an extension and release it only to discover later that it doesn't work
> unless users start the netmonitor first? In my opinion, the previous
> approach at least had a predictable behaviour. Here we need to explain to
> developers what is the impact of using this feature, and in turn they also
> need to explain it to their own users. If forcing the netmonitor UI to start
> is not an option, I personally think we should block this on Bug 1416748 and
> not land it alone. Or have really strong documentation and warning messages. 

I definitely agree that this would be an annoying limitation of this implementation of the API.

The main reason why I've been ok with a limitation of the first provided version of these APIs (as long as it doesn't
introduce a performance penalty for the entire toolbox) is that once the legacy extension have been deprecated there is no way to export the HAR logs on Firefox >= 57 yet (at least as far as I know, and my guess was that this is also the main reason why Jan wanted to provide these APIs asap, so that it can be used in a WebExtensions version of the har-export-trigger extension).
 
> Luca: "I would be fine if the initial version of these APIs (both getHAR and
> onRequestFinished) are limited (and require the user to have activated the
> netmonitor panel first)"
> Do you have anything in mind to communicate this to developers who will use
> the APIs?

We should definitely mention this initial limitation explicitly in the MDN API docs, where we already have this kind of information for other APIs which behaves differently from Chrome or have some limitations.

> 
> ::: devtools/client/framework/toolbox.js:3031
> (Diff revision 17)
> > +   *        The listener to be called it's expected to be
> > +   *        a function that takes ({harEntry, requestId})
> > +   *        as first argument.
> > +   */
> > +  addRequestFinishedListener: function (listener) {
> > +    this._requestFinishedListeners.add(listener);
> 
> We need to warn developers that their extension will not catch any request
> unless netmonitor has been started I think.
> 
> I'm not sure where the best spot is for such a warning. Maybe here, if they
> try to add a listener, log a message to the console (and I would do so
> regardless of the netmonitor being started or not, developers need to know
> that they are using a very flaky feature that will only work if users have
> started the netmonitor before).

Logging a warning when an onRequestFinished listener is added sounds like a good idea,
it could help the developer to notice the reason of the apparently missing events
(and we could also include a link to the section of the MDN doc page where we are going to
describe this limitation).

On the user side, an extension that uses this APIs also needs to have a reasonable way 
to warn the user about it as well, e.g. I guess that an extension would have to do something like:

- the extension subscribes its listener to onRequestFinished
- then the extension calls getHAR and if it receives an empty HAR log
  it may use the notification API (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/notifications)
  to warn the user that no network events will be received until the network panel has been loaded 
  (and the page reloaded if there are no requests currently collected by the netmonitor).

Having said that, especially if the devtools.network.getHAR API method is enough to allow har-export-trigger to provide its
main feature ("exporting the full HAR logs archive"), I would also prefer (as Julian) to land this API
once we can avoid these limitations (without introducing a performance penalty on the entire toolbox),  
and basically block this bugzilla issue (onRequestFinished) on the issue that is going to refactor out the 
part of the netmonitor panel that would allow to listen and retrieve the requests (and their responses) 
without loading the netmonitor panel first (and then also fix the behavior of devtools.network.getHAR
accordingly).
Flags: needinfo?(lgreco)
Separating backend from the Network panel front-end is definitely on my TODO list, but it isn't straightforward to have it all at once. Not even the Bug 1416748 alone is enough, it's required step towards this goal and more is needed. It's easier to land step by step.

So, I am suggesting the following: Let's log a message to the console explaining the API requirement and let's also update MDN as soon as it is *not* necessary to open the Net panel. Finish work on this patch, land it, and make sure it doesn't become obsolete and other work can be done on top of that.
Sounds reasonable?
Honza
Flags: needinfo?(lgreco)
Flags: needinfo?(jdescottes)
I'm also in favor of landing this work. Would have preferred starting the netmonitor UI as a first step rather than this, but I'm fine with logging a message + adding documentation.
Flags: needinfo?(jdescottes)
Ok for me too, with "logging a message" + "describe limitation in the API docs" + "followup issue that fix this limitation filed and linked to its blocking issues".
Flags: needinfo?(lgreco)
Great!

* The message is added
* Follow up Bug 1436665

Honza
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review224488

LGTM for the devtools part. We can see if the message needs to be improved in a follow up.

::: devtools/client/framework/toolbox.js:3035
(Diff revision 18)
> +  addRequestFinishedListener: function (listener) {
> +    // Log console message informing the extension developer
> +    // that the Network panel needs to be selected at least
> +    // once in order to receive `onRequestFinished` events.
> +    if (!this._requestFinishedListeners.size) {
> +      let message = "The Network panel needs to be selected at least" +

Given the tone of the message, maybe follow-up with a second message giving the current status:

"(Network panel is already started: 'onRequestFinished' events will be received)"

"(Network panel is not started yet: no 'onRequestFinished' event will be received)"

Otherwise, as a developer, the message here would make think something is wrong *right now*.

Alternatively we could go for a lengthier neutral message 

"You are using the onRequestFinished API. Note that onRequestFinished events are currently only forwarded if the Network panel has been opened."

But we can also land as is right now and touch the message later (thinking Sole or others could provide good advice on how to make the message easy to understand).
Attachment #8945062 - Flags: review?(jdescottes) → review+
Comment on attachment 8945062 [details]
Bug 1311171 - Implement the devtools.network.onRequestFinished API event;

https://reviewboard.mozilla.org/r/215284/#review225044

Hi Honza,
I took another look to the "WebExtensions API" part of this patch and it looks good to me.

I've added some additional review comments related to small changes needed to prevent some eslint errors, and a couple of additional questions around the current limitations of this API (in particular one about the warning message that we are going to log in the console when an extension is subscribing the onRequestFinished event and the netmonitor panel has not been loaded yet, and one related to the behavior of the API on fetching the response content for a request collected before a page reload when the netmonitor persistent logs are not enabled).

Did Julian also reviewed the changes applied to the netmonitor internals, right?
They looks reasonable to me, but I haven't looked into that panel before.

r=me (with eslint issues fixed, green try, and with the additional changes applied to the behavior and details included in the warning message logged when onRequestFinished has been subscribed if you agree with my comments below).

::: browser/components/extensions/ext-c-devtools-network.js:5
(Diff revision 18)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/**

We currently have to add the following eslint comment in this file:

```
/* import-globals-from ext-c-toolkit.js */
```

otherwise eslint will complain that `EventManager` is not defined (and `EventManager` is available as a global because if it is defined in ext-c-toolkit.js).

::: browser/components/extensions/ext-c-devtools-network.js:33
(Diff revision 18)
> +  }
> +}
> +
> +this.devtools_network = class extends ExtensionAPI {
> +  getAPI(context) {
> +    let {extension} = context;

it looks that `extension` is unused here, it is likely a leftover of a previous version of this patch.

::: browser/components/extensions/ext-c-devtools-network.js:42
(Diff revision 18)
> +          onRequestFinished: new EventManager(context, "devtools.network.onRequestFinished", fire => {
> +            let onFinished = (data) => {
> +              const loader = new ChildNetworkResponseLoader(context, data.requestId);
> +              const harEntry = {...data.harEntry, ...loader.api()};
> +              const result = Cu.cloneInto(harEntry, context.cloneScope, {
> +                cloneFunctions: true

There is a missing trailing comma here.

::: browser/components/extensions/ext-devtools-network.js:55
(Diff revision 18)
> +          // The following method is used internally to allow the request API
> +          // piece that is running in the child process to ask the parent process
> +          // to fetch response content from the back-end.
> +          Request: {
> +            async getContent(requestId) {
> +              return context.devToolsToolbox.fetchResponseContent(requestId)

If an extension subscribed the onRequestFinished events fetching the response content related to a request after a page reload, then fetching the response content is going to fail (at least if the persistent log flag has not been turned on in the network panel), am I right?

I'm wondering what is the behavior on Chrome in the same scenario (so that we can document the incompatibility/limitation and evaluate if we could/should remove it in a follow up).

::: devtools/client/framework/toolbox.js:3034
(Diff revision 18)
> +    if (!this._requestFinishedListeners.size) {
> +      let message = "The Network panel needs to be selected at least" +
> +        " once in order to receive 'onRequestFinished' events.";
> +      this.target.logErrorInPage(message, "har");

It looks that this message is going to be printed in the console panel when the first listener is subscribed, and so if two extensions are subscribed this event, the message will be logged only for the first one, and it doesn't include any information about what is subscribing the event (and so it may not be clear for the user what it means).

I'm wondering if it would be better to include some information about the extension that is adding a listener (e.g. by including the name of the extension in the logged message), and in that case we cannot log the message only once (we could probably log the message once per extension).

::: devtools/client/framework/toolbox.js:3034
(Diff revision 18)
> +   */
> +  addRequestFinishedListener: function (listener) {
> +    // Log console message informing the extension developer
> +    // that the Network panel needs to be selected at least
> +    // once in order to receive `onRequestFinished` events.
> +    if (!this._requestFinishedListeners.size) {

I'm wondering if we could also check if the netmonitor panel has been already loaded here, and then log the warning only if the netmonitor panel has not been loaded.
Attachment #8945062 - Flags: review?(lgreco) → review+
(In reply to Luca Greco [:rpl] from comment #72)
> Comment on attachment 8945062 [details]
> ::: devtools/client/framework/toolbox.js:3034
> (Diff revision 18)
> > +   */
> > +  addRequestFinishedListener: function (listener) {
> > +    // Log console message informing the extension developer
> > +    // that the Network panel needs to be selected at least
> > +    // once in order to receive `onRequestFinished` events.
> > +    if (!this._requestFinishedListeners.size) {
> 
> I'm wondering if we could also check if the netmonitor panel has been
> already loaded here, and then log the warning only if the netmonitor panel
> has not been loaded.

I'm not sure about that, I wouldn't like developers to miss this important piece of information while developing just because they had the netmonitor opened by chance.
(In reply to Luca Greco [:rpl] from comment #72)
> We currently have to add the following eslint comment in this file:
> 
> ```
> /* import-globals-from ext-c-toolkit.js */
> ```
Fixed, I used relative patch: 
/* import-globals-from ../../../toolkit/components/extensions/ext-c-toolkit.js */

> it looks that `extension` is unused here, it is likely a leftover of a
> previous version of this patch.
Fixed

> There is a missing trailing comma here.
Fixed

> If an extension subscribed the onRequestFinished events fetching the
> response content related to a request after a page reload, then fetching the
> response content is going to fail (at least if the persistent log flag has
> not been turned on in the network panel), am I right?
The response body fetching works no matter if before/after page load event.

> I'm wondering what is the behavior on Chrome in the same scenario (so that
> we can document the incompatibility/limitation and evaluate if we
> could/should remove it in a follow up).
Chrome seems to be quite inconsistent since `getHAR` doesn't return response
bodies (it's ok from HAR spec validation) and `Request.getContent()` does
return it.

> It looks that this message is going to be printed in the console panel when
> the first listener is subscribed, and so if two extensions are subscribed
> this event, the message will be logged only for the first one,
Good point, so the message is now printed every time an extension appends
a listener.

> I'm wondering if we could also check if the netmonitor panel has been
> already loaded here, and then log the warning only if the netmonitor panel
> has not been loaded.
Agree with Julian, let's keep it as is (plus as mentioned, print the message
every time a listener is appended).

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=648b66df36d84c192217dc8cbbe2d92b3924dd3a

Thanks!
Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/026401920e32
Implement the devtools.network.onRequestFinished API event; r=jdescottes,rpl
https://hg.mozilla.org/mozilla-central/rev/026401920e32
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Thank you for working on this! The API seems to work fine in the extension I mentioned. Some other stuff seem to be broken in the extension but not API related :)
I've added a page on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.network/onRequestFinished

Please let me know if this covers it.
Flags: needinfo?(odvarko)
(In reply to Will Bamberg [:wbamberg] from comment #79)
> I've added a page on this:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.
> network/onRequestFinished
> 
> Please let me know if this covers it.

Do the other browser have the same limitation as firefox (having to open the network panel)? If not, perhaps the compatibility table should indicate partial support?
(In reply to Joris van der Wel [:JoWie] from comment #80)
> (In reply to Will Bamberg [:wbamberg] from comment #79)
> > I've added a page on this:
> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.
> > network/onRequestFinished
> > 
> > Please let me know if this covers it.
> 
> Do the other browser have the same limitation as firefox (having to open the
> network panel)? If not, perhaps the compatibility table should indicate
> partial support?
This should be fixed in bug 1436665 (currently patch under review).
So, perhaps we can wait for it?

Honza
Flags: needinfo?(odvarko)
(In reply to Joris van der Wel [:JoWie] from comment #80)
> (In reply to Will Bamberg [:wbamberg] from comment #79)
> > I've added a page on this:
> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.
> > network/onRequestFinished
> > 
> > Please let me know if this covers it.
> 
> Do the other browser have the same limitation as firefox (having to open the
> network panel)? If not, perhaps the compatibility table should indicate
> partial support?

Good point! I've filed https://github.com/mdn/browser-compat-data/pull/1675 for this.

> This should be fixed in bug 1436665 (currently patch under review).
> So, perhaps we can wait for it?

Will that patch be uplifted to Firefox 60? If not, we should note this incompatibility for that release.
Flags: needinfo?(odvarko)
(In reply to Will Bamberg [:wbamberg] from comment #82)
> Will that patch be uplifted to Firefox 60? If not, we should note this
> incompatibility for that release.
No uplift planned. So, yes agree, we should note this for that release.

Thanks Will!
Honza
Flags: needinfo?(odvarko)
Release Note Request (optional, but appreciated)
[Why is this notable]: New WebExtension API for DevTools extension developers.
[Affects Firefox for Android]: no
[Suggested wording]: DevTools supports a new WebExtension API `devtools.network.onRequestFinished` that allows registering a listener for request finished event. The API is compatible with Chrome browser implementation.

[Links (documentation, blog post, etc)]:
MDN: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.network/onRequestFinished
Chrome browser docs: https://developer.chrome.com/extensions/devtools_network
relnote-firefox: --- → ?
This landed in Fx60, removing the release note flag.
Honza
relnote-firefox: ? → ---
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: