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

Add declarativeNetRequest #18403

Merged
merged 21 commits into from
Feb 14, 2023
Merged

Add declarativeNetRequest #18403

merged 21 commits into from
Feb 14, 2023

Conversation

rebloor
Copy link
Collaborator

@rebloor rebloor commented Dec 9, 2022

Adding the declarativeNetRequest API.

Complement the content addition in #22644

  • core content
  • confirm core content release versions
  • add detail for additions and deprecations

@rebloor rebloor added the data:webext 🎲 Compat data for Browser Extensions. https://developer.mozilla.org/Add-ons/WebExtensions label Dec 9, 2022
@rebloor rebloor self-assigned this Dec 9, 2022
@Rob--W
Copy link
Member

Rob--W commented Dec 9, 2022

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.

@rebloor
Copy link
Collaborator Author

rebloor commented Dec 12, 2022

@Rob--W "In particular, pay attention to the support state of Safari for the individual APIs." Where can I find more details?

@Rob--W
Copy link
Member

Rob--W commented Dec 12, 2022

@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 safari and safari_ios.

@rebloor
Copy link
Collaborator Author

rebloor commented Dec 12, 2022

@Rob--W "The files in the linked PR itself, at safari and safari_ios." They all say "false", but the 15 release notes say this was added there.

@rebloor rebloor marked this pull request as ready for review December 12, 2022 22:23
@Elchi3
Copy link
Member

Elchi3 commented Jan 11, 2023

There is this PR and then there is #14942. Should someone review this PR or what is the plan here? Do you need help with this by BCD maintainers or are you sorting this out yourself? @rebloor @Rob--W

@Rob--W
Copy link
Member

Rob--W commented Jan 11, 2023

There is this PR and then there is #14942. Should someone review this PR or what is the plan here? Do you need help with this by BCD maintainers or are you sorting this out yourself? @rebloor @Rob--W

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.

Copy link
Member

@Rob--W Rob--W left a 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) ).

webextensions/api/declarativeNetRequest.json Outdated Show resolved Hide resolved
"__compat": {
"support": {
"chrome": {
"version_added": "86"
Copy link
Member

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

webextensions/api/declarativeNetRequest.json Outdated Show resolved Hide resolved
webextensions/api/declarativeNetRequest.json Outdated Show resolved Hide resolved
"safari_ios": "mirror"
}
},
"callback": {
Copy link
Member

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".

webextensions/api/declarativeNetRequest.json Show resolved Hide resolved
webextensions/api/declarativeNetRequest.json Outdated Show resolved Hide resolved
"safari_ios": "mirror"
}
},
"options": {
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Member

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": {
Copy link
Member

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.

Copy link
Collaborator Author

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.

webextensions/api/declarativeNetRequest.json Outdated Show resolved Hide resolved
rebloor and others added 3 commits January 27, 2023 05:57
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
@rebloor rebloor requested a review from Rob--W January 27, 2023 17:23
Comment on lines 1002 to 1021
},
"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"
}
}
}
Copy link
Member

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.

Suggested change
},
"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": {
Copy link
Member

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": {
Copy link
Member

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

Copy link
Collaborator Author

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`
@rebloor rebloor requested a review from Rob--W January 29, 2023 15:33
"firefox_android": "mirror",
"opera": "mirror",
"safari": {
"version_added": "15"
Copy link
Contributor

@xeenon xeenon Feb 9, 2023

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
"version_added": "15"
"version_added": false

@Rob--W
Copy link
Member

Rob--W commented Feb 9, 2023

@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

@rebloor
Copy link
Collaborator Author

rebloor commented Feb 10, 2023

@xeenon thanks for providing these details, when you have a moment could you check and see whether I've missed anything

Copy link
Contributor

@xeenon xeenon left a 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.

@xeenon
Copy link
Contributor

xeenon commented Feb 10, 2023

@rebloor Looks good. One thing that was missed in MatchedRule, Safari does not support that.

We do support getMatchesRules() which in Chrome returns MatchedRuleInfo, but in Safari is returning a result more like hybrid of MatchedRuleInfo and MatchedRuleInfoDebug with a subset of RequestDetails as request that just includes url. We also only include the match in the result array if the extension has host permissions for the domain of the matched URL.

But I'm seeing now we have a bug, because we put tabId, timeStamp and url inside request when only url should be inside and tabId and timeStamp should be top level. On top of missing the wrapper object RulesMatchedDetails. I'll file a bug to get that fixed.

Here is an example of what shipping Safari returns for getMatchesRules():

[{
    "request": {
        "url": "https://example.com",
        "tabId": 1,
        "timeStamp": 1676055129451
    }
}]

It should look more like RulesMatchedDetails with MatchedRuleInfo plus MatchedRuleInfoDebug, for example:

{
    "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 "preview" for getMatchedRules() or add a Note and mark it as partial implementation in 15.4 until this bug is fixed?

@rebloor
Copy link
Collaborator Author

rebloor commented Feb 12, 2023

@xeenon

The dfa193b commit should be reverted. preview is correct for ModifyHeaderInfo.

Following advice from @Rob--W BCD for ModifyHeaderInfo has been removed and it's BCD will be represented by BCD for RuleAction.requestHeaders & RuleAction.responseHeaders BCD on MDN.

@rebloor
Copy link
Collaborator Author

rebloor commented Feb 12, 2023

@xeenon

One thing that was missed in MatchedRule, Safari does not support that.

Updated in c41c06a

@rebloor
Copy link
Collaborator Author

rebloor commented Feb 12, 2023

@xeenon for the getMatchesRules() issue, do you have a public bug reference?

Copy link
Member

@Rob--W Rob--W left a 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?

@rebloor rebloor merged commit e3eb956 into mdn:main Feb 14, 2023
@rebloor
Copy link
Collaborator Author

rebloor commented Feb 14, 2023

@Rob--W my understanding is that we document features behind a preference using flag, like this:
image

@rebloor rebloor deleted the add-declarativeNetRequest branch February 14, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:webext 🎲 Compat data for Browser Extensions. https://developer.mozilla.org/Add-ons/WebExtensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants