-
Notifications
You must be signed in to change notification settings - Fork 64
API Review: Trusted Origin APIs #5438
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
base: user/chetanpandey/TrustedOriginSpecNew
Are you sure you want to change the base?
API Review: Trusted Origin APIs #5438
Conversation
220f538 to
d9fb9b3
Compare
|
|
||
| - AccentColor | ||
| - EnhancedSecurityMode | ||
| - PersistenceStorage |
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.
Grammar: You probably want "PersistentStorage" or "StoragePersistence"
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.
Will make the change. Will prefer PersistentStorage.
| /// | `*example/` | `https://app.example/`,`https://api.example/` | Matches any subdomain and top-level domain variations | | ||
| /// | `https://xn--qei.example/` | `https://â¤.example/`,`https://xn--qei.example/` | Normalized punycode matches with corresponding Non-ASCII hostnames | | ||
| /// | ||
| /// Note: `*` is not a valid value for OriginPattern. |
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 not? This seems like it should be valid.
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.
This means we are enabling/disabling for all the origins, which might not be true for all the features we are supporting. For example, persistence storage can't be enabled for all the origins. Because for all origins if we make storage persistent, in case of low disk space there would be no eviction and it might lead to issues.
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.
And if there is a need for toggle control, we'll add APIs as required.
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.
in case of low disk space there would be no eviction and it might lead to issues.
I'm pretty sure in most cases we allow developers to shoot themselves in the foot.
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.
And if there is a need for toggle control, we'll add APIs as required.
Is that the right path though? Maybe more of a question for @shrinaths / @david-risney but if we had this API (with just * allowed) when we were implementing something like CoreWebView2Profile.PreferredTrackingPreventionLevel, would we have implemented it? It seems like it could be cleaner and easier to have this API as the primary way to support enabling/disabling features for all origins in a profile for future additions.
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 today also we can make all the storage persistent. Using --unlimited-storage we can do that.
ref: #72
So thinking of removing this.
| - AccentColor | ||
| - EnhancedSecurityMode | ||
| - PersistenceStorage | ||
| - TrackingPrevention |
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.
You should explain how this interacts with CoreWebView2EnvironmentOptions.EnableTrackingPrevention and CoreWebView2Profile.PreferredTrackingPreventionLevel.
Same for ESM and the other API we're working on for that.
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.
Yeah sure will add it for tracking prevention and ESM
For Accent Color and Persistent Storage I have already mentioned the default state.
Draft API review for Trusted Origin APIs