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.

Sign up for GitHub

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

Jump to bottom

asyncBlocking in webRequest.onAuthRequired #34137

Merged
merged 10 commits into from
Jul 19, 2024
Merged

asyncBlocking in webRequest.onAuthRequired #34137

merged 10 commits into from
Jul 19, 2024
+21 −13

Conversation

rebloor
Copy link
Contributor

@rebloor rebloor commented Jun 14, 2024

Description

Addresses the documentation requirements of Bug 1889897 Implement asyncBlocking support for webRequest.onAuthRequired, including:

  • a release note
  • update to the webRequest.onAuthRequired event documentation. This also included some changes to avoid repetition in that documentation.
@rebloor rebloor added the Content:WebExt WebExtensions docs label Jun 14, 2024
@rebloor rebloor requested a review from dotproto June 14, 2024 03:02
@rebloor rebloor self-assigned this Jun 14, 2024
@rebloor rebloor requested review from a team as code owners June 14, 2024 03:02
@rebloor rebloor requested review from hamishwillee and removed request for a team June 14, 2024 03:02
@github-actions github-actions bot added Content:Firefox Content in the Mozilla/Firefox subtree size/s [PR only] 6-50 LoC changed labels Jun 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 14, 2024 •
edited
Loading

Preview URLs

External URLs (2)

URL: /en-US/docs/Mozilla/Firefox/Releases/128
Title: Firefox 128 for developers

(comment last updated: 2024-07-18 09:21:19)
@hamishwillee hamishwillee removed their request for review June 17, 2024 02:20
dotproto
dotproto requested changes Jun 18, 2024
View reviewed changes
Copy link
Collaborator

@dotproto dotproto left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there may have been a miscommunication around how async handling of onAuthRequired events work in Firefox 128+. The version of this PR I'm reviewing seems to say that Firefox 128 and later no longer support asynchronous response handling in "blocking" listeners. Based on my reading of Firefox source, it appears that blocking listeners can still return promises to asynchronously settle an onAuthRequired events. What has changed is that Firefox now supports registering onAuthRequired listeners with "asyncBlocking" and these listeners match the behavior seen in Chrome.

All changes suggested in this review are for illustrative purposes. I'm happy to defer to your judgement on how best to structure the related content.
files/en-us/mozilla/add-ons/webextensions/api/webrequest/onauthrequired/index.md Outdated Show resolved Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/webrequest/onauthrequired/index.md Outdated Show resolved Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/webrequest/onauthrequired/index.md Show resolved Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/webrequest/onauthrequired/index.md Show resolved Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/webrequest/onauthrequired/index.md Outdated Show resolved Hide resolved
@rebloor @dotproto
Suggestions from review
6d5142b 
Co-authored-by: Simeon Vincent <svincent@gmail.com>
@rebloor
Copy link
Contributor Author

rebloor commented Jun 19, 2024

@dotproto I need to check the language in the changes and fix some formatting issues but in the meanwhile, could you check my latest updates, do they address your feedback?
@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jun 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Jun 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.
@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jul 13, 2024
@rebloor rebloor requested a review from a team as a code owner July 13, 2024 17:27
@rebloor rebloor requested review from pepelsbey and removed request for a team July 13, 2024 17:27
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Jul 13, 2024
@rebloor rebloor requested a review from dotproto July 15, 2024 22:55
@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jul 17, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Jul 17, 2024
@dotproto
Copy link
Collaborator

@rebloor, I just created a test extension to flex the various ways that an onAuthRequired event handler can try to resolve an auth request. Hopefully this helps clear up the confusion about the behavior differences between Chrome and Firefox. https://gist.github.com/dotproto/43cba54752300484b69ca0d0ecc1c82c
@rebloor @dotproto @github-actions
Feedback and linter suggestions
505f60c 
Co-authored-by: Simeon Vincent <svincent@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions[bot]
github-actions bot reviewed Jul 18, 2024
View reviewed changes
files/en-us/mozilla/add-ons/webextensions/api/webrequest/onauthrequired/index.md Outdated
@@ -82,23 +84,28 @@ Events have three functions:

- `details`
- : `object`. Details about the request. See the [details](#details_2) section for more information.
- `asyncCallback` {{optional_inline}}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[mdn-linter] reported by reviewdog 🐶

Suggested change
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rebloor, any idea why mdn-linter is generating linter warnings that aren't showing up when doing a local yarn lint:md or know who we could tag about that?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dotproto sorry, no and no
@rebloor rebloor requested a review from dotproto July 18, 2024 09:22
dotproto
dotproto approved these changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last subjective suggestion. Thanks, @rebloor!
dotproto
dotproto reviewed Jul 18, 2024
View reviewed changes
files/en-us/mozilla/add-ons/webextensions/api/webrequest/onauthrequired/index.md
Comment on lines +34 to 38
- in addListener, pass `"asyncBlocking"` in Chrome and Firefox or `"blocking"` in Firefox in the `extraInfoSpec` parameter
- If `"blocking"` is provided, the extension can return a `webRequest.BlockingResponse` object or a Promise that resolves to a `webRequest.BlockingResponse` object
- If `"asyncBlocking"` is provided, the event listener function receives a `asyncCallback` function as its second parameter. `asyncCallback`can be called asynchronously and takes a`webRequest.BlockingResponse` object as its only parameter

> **Note:** Chrome does not support a Promise as a return value ([Chromium issue 1510405](https://crbug.com/1510405)). For alternatives, see [the return value of the `listener`](#listener).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this information is a bit redundant given the recent changes to the extraInfoSpec parameter documentation. Should we remove this block and link to that property for details? I'm happy to defer to your judgement.
@rebloor rebloor merged commit 3cb4e06 into mdn:main Jul 19, 2024
8 checks passed
@rebloor rebloor deleted the asyncBlocking-for-webRequest.onAuthRequired branch July 19, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Firefox Content in the Mozilla/Firefox subtree Content:WebExt WebExtensions docs size/s [PR only] 6-50 LoC changed
3 participants
@rebloor @dotproto @pepelsbey