-
Notifications
You must be signed in to change notification settings - Fork 55
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 spec for CookieManagement.md #463
Add spec for CookieManagement.md #463
Conversation
Thank you, looks good to me. |
So it's easier to handle your own downloads, can there be a function to convert to .Net cookie. |
There is HttpOnly property, so for sure we can create HttpOnly cookies, which is good thing. Will GetCookiesAsync return HttpOnly cookies as well? |
specs/CookieManagement.md
Outdated
|
||
void SetCookieCmdExecuted(object target, ExecutedRoutedEventArgs e) | ||
{ | ||
CoreWebView2Cookie cookie = _cookieManager.CreateCookie("CookieName", "CookieValue", ".bing.com", "/"); |
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.
Is the '.' at the start of .bing.com necessary or have special meaning? The parameter is named 'domain' and I wouldn't offhand expect I need to put a '.' at the start of the 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.
It is necessary for CDP
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.
But what does it mean? Does it just mean 'bing.com'? AFAIK the empty domain name label is only valid for the root and is not allowed elsewhere in a domain name. Is this syntax specific to CDP? I don't see anything about this in the cookie spec. I also don't see anything about it in CDP docs (but that doesn't necessarily mean anything)
specs/CookieManagement.md
Outdated
} | ||
|
||
/// A list of cookie objects. | ||
runtimeclass CoreWebView2CookieList |
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 COM I understand having our own list type, but for .NET and WinRT can we use existing types from those languages/runtimes?
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.
For .NET, CoreWebView2CookieList
should implement System.Collections.Generic.IList<CoreWebView2Cookie>
; for WinRT, CoreWebView2CookieList
should implement Windows.Foundation.Collections.IVector<CoreWebView2Cookie>
.
specs/CookieManagement.md
Outdated
/// The default is the host that this cookie has been received from. | ||
String Domain { get; }; | ||
|
||
/// The path for which the cookie is valid. If not specified, this cookie |
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.
'not specified' here means empty string or null I guess right?
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.
Correct. Should say it explicitly in the comment?
…tor<CoreWebView2Cookie>
specs/CookieManagement.md
Outdated
|
||
void AddOrUpdateCookieCmdExecuted(object target, ExecutedRoutedEventArgs e) | ||
{ | ||
CoreWebView2Cookie cookie = webView.CoreWebView2.CookieManager.CreateCookieWithDetails("CookieName", "CookieValue", ".bing.com", "/"); |
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.
What's with the prefix '.' on 'bing.com'? Is '.bing.com' different from 'bing.com'? If so, this needs a comment in the API documentation
|
@@ -461,7 +465,7 @@ namespace Microsoft.Web.WebView2.Core | |||
|
|||
/// The expiration date and time for the cookie as the number of seconds since the UNIX epoch. | |||
/// The default is -1.0, which means cookies are session cookies by default. | |||
Double Expires { get; set; }; | |||
Windows.Foundation.DateTime Expires { get; set; }; |
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.
Does this work in WPF/Winforms? I believe Windows.Foundation.DateTime
is UWP only, isn't it? In case it is, then we should revert back to Double
as the WebView2 control is available on WPF and on Winforms too.
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.
We use Windows.Foundation.DateTime
for WinRT API only, and we use Double for .NET API.
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.
There's a datetime type in .NET also right? Can't we use the appropriate datetime type for .NET?
Updated |
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.
Looks ready for api review. Please open PR to master. Thanks!
This is a review for the new Cookie Management APIs.