-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add declarativeNetRequest #18403
Add declarativeNetRequest #18403
Conversation
Please take a look at #14942 and merge the feedback from the other PR with this one. In particular, pay attention to the support state of Safari for the individual APIs. |
@Rob--W "In particular, pay attention to the support state of Safari for the individual APIs." Where can I find more details? |
The files in the linked PR itself, at |
@Rob--W "The files in the linked PR itself, at |
I and @rpl are planning to review both PRs. I think that just mine suffices here, and Luca and I will take care of the one in the MDN repo. |
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.
I need to confirm with Apple folks about the support state of individual APIs, methods and events. The PR here lists much more than just https://github.com/w3c/webextensions/blob/main/interfaces/safari/WebExtensionAPIDeclarativeNetRequest.idl
The DNR API is declarative, and therefore it's probably worth mentioning each property of rule.condition
and rule.action
. This is because we should not assume a property to be supported unless explicitly confirmed to be supported.
For example, I would consider adding a note to isUrlFilterCaseInsensitive
and/or regexFilter
, because of their volatile status (see mdn/content#22644 (comment) and mdn/content#22644 (comment) ).
"__compat": { | ||
"support": { | ||
"chrome": { | ||
"version_added": "86" |
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.
Add an entry or notes
that clarify that the "append"
operation was only supported (for requestHeaders) starting from Chrome 108. For details, see https://bugs.chromium.org/p/chromium/issues/detail?id=1117475#c7
"safari_ios": "mirror" | ||
} | ||
}, | ||
"callback": { |
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.
Let's drop the callback
part. The onRuleMatchedDebug
key here specifies support for the event, which implies that there is some listener. Individual event properties can be specified directly under it, without another dummy "callback".
"safari_ios": "mirror" | ||
} | ||
}, | ||
"options": { |
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.
Why did you add an entry for options
? The options
parameter is required and it is not possible to implement an updateDynamicRules
API without an options
parameter.
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 because the way the Chrome documentation appears to work is that everything that doesn't have a specific release identified was released in 84 and the documentation for updateDynamicRules
shows that options
with added in 87.
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 given your comments, I have assumed the information about adding options
in 87 is incorrect and have removed it from the BCD, retaining 84 as the release that introduced updateDynamicRules
.
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.
The API was introduced in 84, but a breaking change to the syntax was introduced in 87.
https://bugs.chromium.org/p/chromium/issues/detail?id=1131746 has details. If you'd like to be complete, you could add that it was supported from 84 - 86 with a different syntax, and 87+ with the current format.
Since it has been over 2 years now, I think that it's fine to mention 87+ as supported version.
"safari_ios": "mirror" | ||
} | ||
}, | ||
"options": { |
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.
Similarly as above: Why is options
documented for updateEnabledRulesets
? If there is no reason, delete it.
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.
Also as above, deleted and retained 84 as the release version for updateEnabledRulesets
.
Co-authored-by: Rob Wu <rob@robwu.nl>
- requestHeaders note about the "append" operation in Chrome 108 - removed callback from onRuleMatchedDebug - added options and includeOtherExtensions to testMatchOutcome - removed options from updateDynamicRules and updateEnabledRulesets - added extensionId to MatchedRule
}, | ||
"request": { | ||
"__compat": { | ||
"support": { | ||
"chrome": { | ||
"version_added": "103" | ||
}, | ||
"edge": "mirror", | ||
"firefox": { | ||
"version_added": false | ||
}, | ||
"firefox_android": "mirror", | ||
"opera": "mirror", | ||
"safari": { | ||
"version_added": false | ||
}, | ||
"safari_ios": "mirror" | ||
} | ||
} | ||
} |
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.
The positional request
parameter is a required part of the API and does not need to be documented separately.
}, | |
"request": { | |
"__compat": { | |
"support": { | |
"chrome": { | |
"version_added": "103" | |
}, | |
"edge": "mirror", | |
"firefox": { | |
"version_added": false | |
}, | |
"firefox_android": "mirror", | |
"opera": "mirror", | |
"safari": { | |
"version_added": false | |
}, | |
"safari_ios": "mirror" | |
} | |
} | |
} |
"safari_ios": "mirror" | ||
} | ||
}, | ||
"includeOtherExtensions": { |
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.
Let's put this includeOtherExtensions
option under options
, since it is part of it.
In another API I asked for the options
to be removed, but in this case not all implementations support options
, so we have to keep options
to be able to show when the positonal parameter is supported and when not. And since the options
entry then exists, for clarity we have to put the members of the options
dictionary under it.
"safari_ios": "mirror" | ||
} | ||
}, | ||
"extensionActionOptions": { |
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.
Remove the separate entry for this. The setExtensionActionOptions
method takes one dictionary, and any options underneath are part of the positional parameter that is documented as options
(not extensionActionOptions
) at https://developer.chrome.com/docs/extensions/reference/declarativeNetRequest/#method-setExtensionActionOptions
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.
I've renamed this option
as we need to retain the entry to note that tabUpdate
was added in 89.
- removed `request` from `testMatchOutcome` - moved `includeOtherExtensions` under `options` in `testMatchOutcome` - renamed `extensionActionOptions` to `option` under `setExtensionActionOptions`
"firefox_android": "mirror", | ||
"opera": "mirror", | ||
"safari": { | ||
"version_added": "15" |
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.
onRuleMatchedDebug
and related are not supported by Safari. We do support this, along with timeStamp
, in getMatchedRules
results, but I don't see that above.
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.
"version_added": "15" | |
"version_added": false |
@rebloor By the way, when Timothy says "supported in Safari Technology Preview", write "preview" in the version, per https://github.com/mdn/browser-compat-data/blob/main/docs/data-guidelines/index.md#choosing-preview-values |
Co-authored-by: Rob Wu <rob@robwu.nl>
…aders supported from 15)
@xeenon thanks for providing these details, when you have a moment could you check and see whether I've missed anything |
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.
The dfa193b commit should be reverted. preview
is correct for ModifyHeaderInfo
.
@rebloor Looks good. One thing that was missed in We do support But I'm seeing now we have a bug, because we put Here is an example of what shipping Safari returns for [{
"request": {
"url": "https://example.com",
"tabId": 1,
"timeStamp": 1676055129451
}
}] It should look more like {
"rulesMatchedInfo": [{
"request": {
"url": "https://example.com"
},
"tabId": 1,
"timeStamp": 1676055129451
}]
} I'm not sure how this deviation between Chrome and Safari should be handled (on top of the bad bug). It might just be best to say |
@xeenon for the |
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.
I think that the PR is good to merge.
All Firefox APIs are currently marked as "version_added": false
despite there being an implementation behind a preference. We can change their status in a follow-up PR. I'm considering to use "version_added": "preview"
for that, according to the guidelines at https://github.com/mdn/browser-compat-data/blob/main/docs/data-guidelines/index.md#choosing-preview-values . Does that sound reasonable?
@Rob--W my understanding is that we document features behind a preference using |
Adding the declarativeNetRequest API.
Complement the content addition in #22644