-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
DNR: Clarify precedence order across browsers. #26793
Conversation
The currently documented order is not cross-browser, see WECG issue 280. Therefore, drop that part of the documentation and replace it with guidance on how to create stable rules.
Preview URLs Flaws (3)URL:
External URLs (1)URL:
(comment last updated: 2023-05-16 14:19:00) |
files/en-us/mozilla/add-ons/webextensions/api/declarativenetrequest/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/declarativenetrequest/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/declarativenetrequest/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/declarativenetrequest/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/declarativenetrequest/index.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rob--W I just took a quick look to this PR, the content looks good from a technical perspective, but I have a thought about the terminology currently used to describe the issue that I'd like to pass by you (described in the inline comment below), let me know wdyt.
4. the order of the rule in the ruleset, determined as the lowest value rule ID. | ||
6. "modifyHeaders" rewrites request and/or response headers. | ||
|
||
> **Note:** When there are multiple matching rules with the same rule priority and rule action type, a problem occurs when the matched action support additional properties. These can result in different outcomes that cannot be combined. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rob--W I'm wondering if instead of using the terms problem
/ no problem
we may consider using the terms ambiguous
/ non-ambiguous
instead, e.g.
Note: When there are multiple matching rules with the same rule priority and rule action type, the outcome of the rules evaluation is going to be potentially ambiguous when the matched action support additional properties. For example:
- "block" action rules: when multiple rules with the "block" action are matching, there wouldn't be any ambiguity because all "block" actions would result in the same outcome.
- "redirect" action rules: on the contrary when multiple with the "redirect" action are matching, the url each of the rules would be redirecting the request to may be different and that would make the outcome to become ambiguous, and so all but one of the matching "redirect" rules are going to be ignored. Note that it is still possible, ...
- "modifyHeaders" action rules: multiple rules with the "modifyHeaders" action are non-ambiguous if they all touch a different header and ambiguous if they touch the exact same header (as also explained at {{WebExtAPIRef("declarativeNetRequest.ModifyHeaderInfo")}}). The evaluation order of "modifyHeaders" is therefore important.
...
Not necessarily with this exact wording, but the idea was to make it more immediately clear what I meant with considering using the term "ambiguous/non-ambiguous" instead of "problem/no problem".
FYI: the dnr-redirect-url example at mdn/webextensions-examples#526 demonstrates the difference between Firefox and Chrome. They now behave identically, but when "priority" is removed from redirect-rules.json, the behavior differs. |
Co-authored-by: rebloor <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Description
The currently documented order is not cross-browser, see w3c/webextensions#280. Therefore, drop that part of the documentation and replace it with guidance on how to create stable rules.
Motivation
People following the documentation with the expectation of having the same browser across browsers will be disappointed. The documentation should offer guidance to have consistent behavior across browsers.
Additional details
The documentation currently lists 4 ways of affecting the outcome. Only the first two are kept, as these are also documented in Chrome's documentation (https://developer.chrome.com/docs/extensions/reference/declarativeNetRequest/#implementation-matching-algorithm). The other two (ruleset and rule ID) are not documented in Chrome, and cannot be relied upon. Therefore that part has been removed in favor of a note to emphasize the risk of having ambiguous outcomes, along with recommendations to resolve this.
Related issues and pull requests