Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

browser.devtools.network.onRequestFinished does not promisify getContent() #249

Closed
dgp1130 opened this issue Nov 14, 2020 · 4 comments · Fixed by #250 · May be fixed by ramzimalhas/refined-zapier#2
Closed

browser.devtools.network.onRequestFinished does not promisify getContent() #249

dgp1130 opened this issue Nov 14, 2020 · 4 comments · Fixed by #250 · May be fixed by ramzimalhas/refined-zapier#2

Comments

@dgp1130
Copy link
Contributor

dgp1130 commented Nov 14, 2020

On Chrome, chrome.devtools.network.onRequestFinished.addListener emits a chrome.devtools.network.Request object, which includes a getContent() method that invokes a callback with string content and its encoding. Correct usage looks like:

chrome.devtools.network.onRequestFinished.addListener((req) => {
  req.getContent((content, encoding) => {
    // ...
  });
});

With Web Extensions, getContent() should return a Promise rather than being callback based. For example:

browser.devtools.network.onRequestFinished.addListener(async (req) => {
  const [content, encoding] = await req.getContent();
});

Currently however, the Web Extensions polyfill simply passes through what it receives from Chrome, so getContent() is still callback based.

I wasn't able to find browser.devtools in the spec, however MDN does say that this method exists and does in fact return a Promise. firefox-webext-browser also says this returns Promise<object>. I'm not totally clear if this is an official part of the spec, or a Firefox-specific API.

My questions here are:

  1. Is browser.devtools.network.onRequestFinished actually defined in the Web Extensions spec somewhere?
  2. What exactly is the return type? Promise<object> is pretty vague and MDN doesn't provide specifics.
    • Chrome returns a tuple of [string, string], where the former is the content as a string or a base64 encoded string (if binary), and the latter is an empty string or 'base64' (if binary). I'm not sure what the expected behavior of Web Extensions would be for binary data?

I'm happy to write up a PR to implement this if it is appropriate to add to this polyfill. I'm not totally clear on where this API stands with respect to the spec, but if you're willing to merge it, I'm happy to write it.

@Rob--W
Copy link
Member

Rob--W commented Dec 7, 2020

  1. There is currently no maintained webext spec.
  2. MDN should be updated; the type definition is at https://searchfox.org/mozilla-central/rev/1843375acbbca68127713e402be222350ac99301/browser/components/extensions/schemas/devtools_network.json#11-44 and the implementation at https://searchfox.org/mozilla-central/rev/1843375acbbca68127713e402be222350ac99301/browser/components/extensions/parent/ext-devtools-network.js#65 . The schema and implementation are at odds with each other though... The schema says that the second parameter is the encoding ("base64" or none), but the actual implementation is the MIME type (e.g. text/plain). Here is a unit test that confirms the deviating behavior: https://searchfox.org/mozilla-central/rev/1843375acbbca68127713e402be222350ac99301/browser/components/extensions/test/browser/browser_ext_devtools_network.js#253,258,263,268

Regardless of the expected format, there are certainly two parameters. It's currently also the only API common to Firefox and Chrome where an API method resolves to an object that has an method with a callback. So it sounds reasonable to hard-code this API in the polyfill, without bothering to introduce support for a generic way of declaring the API in api-metadata.json. A wrapper can be added to

const staticWrappers = {
.

Patches are very welcome :)

@dgp1130
Copy link
Contributor Author

dgp1130 commented Dec 7, 2020

Made #250 to fix the implementation of the polyfill. I'm trying to make a patch for MDN as well, although that is a bit trickier for me as I've never worked with mozilla-central before, so I need to make an account and set up my workspace and everything. I'll try to update that separately.

@dgp1130
Copy link
Contributor Author

dgp1130 commented Dec 8, 2020

I updated the MDN page for browser.devtools.network.onRequestFinished to be more clear about the return value of the Promise. Since this is specifically for Web Extensions, I only mentioned that the second parameter is a MIME type rather than pointing out the distinction between Chrome and Firefox in this regard (since Chrome doesn't return a Promise in the first place).

Since MDN is editable directly through the site, I'm a little unclear on what devtools_network.json represents in mozilla-central. I left that alone for now as I don't understand what I would be updating or what the end result of the change should be.

@Rob--W
Copy link
Member

Rob--W commented Dec 8, 2020

I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1681085 for the Firefox side.

Rob--W pushed a commit that referenced this issue Dec 10, 2020
Fixes #249.

This updates `browser.devtools.network.onRequestFinished` to emit an
object with a promisified `getContent()` property. This brings the
polyfill implementation in line with Firefox's implementation, although
MDN documentation is still inaccurate at the moment.

Also updates some out of date documentation with `makeCallback()` and
`wrapAsyncFunction()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants