SameSite: Teach devtools about the new 'samesite' syntax.

SameSite: Teach devtools about the new 'samesite' syntax.

We used to treat 'samesite' as a binary flag, displaying a checkmark
just like 'secure' or 'httponly'. The attribute is now a trinary value in
the table: "", "Lax", or "Strict".

The protocol shifted to align with this change, switching from a boolean
value to an enum ("NoRestriction", "Lax", and "Strict", respectively).

BUG=596520
Committed: https://crrev.com/4e0bddf6d2123979b95860fae65429014828b33f
Cr-Commit-Position: refs/heads/master@{#385393}

[Mike West, 2016-03-23 10:15:19]

mkwst@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Mike West, 2016-03-23 10:15:19]

Hey Pavel! What do you think of this patch? It ends up changing the protocol
format, and I'm not really sure what the process is there. Is this a reasonable
approach, or am I missing things?

[Mike West, 2016-03-24 07:20:23]

mkwst@chromium.org changed reviewers:
+ dgozman@chromium.org

[Mike West, 2016-03-24 07:20:24]

+dgozman@, as I realize now that Pavel might be buried in perf. :/

[dgozman, 2016-03-24 18:59:26]

https://codereview.chromium.org/1827743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1827743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1408: { "name": "sameSite",
"type": "string", "description": "'Lax', 'Strict', or the empty string." }
Use enum:
"type": "string", "enum": ["Lax", "Strict", "Default"]

I think empty string is a bad value, let's have Default or something instead.
Or if it is actually optional in the spec, make this property optional as well.

[Mike West, 2016-04-01 13:21:07]

On 2016/03/24 at 18:59:26, dgozman wrote:
>
https://codereview.chromium.org/1827743002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/protocol.json (right):
> 
>
https://codereview.chromium.org/1827743002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/protocol.json:1408: { "name": "sameSite",
"type": "string", "description": "'Lax', 'Strict', or the empty string." }
> Use enum:
> "type": "string", "enum": ["Lax", "Strict", "Default"]
> 
> I think empty string is a bad value, let's have Default or something instead.
> Or if it is actually optional in the spec, make this property optional as
well.

Thanks for the comments, and sorry for the delay on my end getting back to you.
I've poked a bit at this, WDYT?

[Mike West, 2016-04-01 13:22:32]

Description was changed from

==========
SameSite: Teach devtools about the new 'samesite' syntax.

We used to treat 'samesite' as a binary flag, displaying a checkmark
just like 'secure' or 'httponly'. The attribute is now a trinary value:
"", "Lax", or "Strict". This requires changes to the protocol and to
the UI code displaying the data.

BUG=596520
==========

to

==========
SameSite: Teach devtools about the new 'samesite' syntax.

We used to treat 'samesite' as a binary flag, displaying a checkmark
just like 'secure' or 'httponly'. The attribute is now a trinary value in
the table: "", "Lax", or "Strict".

The protocol shifted to align with this change, switching from a boolean
value to an enum ("NoRestriction", "Lax", and "Strict", respectively).

BUG=596520
==========

[pfeldman, 2016-04-01 22:04:09]

https://codereview.chromium.org/1827743002/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/1827743002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:258:
->set_same_site(same_site));
What is the value in case SameSite is not specified? Can we make this protocol
parameter optional?

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt
(right):

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt:63:
sameSite : undefined
Then we would omit default value here altogether.

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/cookie-resource-match.html
(right):

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/cookie-resource-match.html:76:
sameSite: "",
And you would not need this

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1398: { "name": "sameSite",
"type": "string", "enum": ["Strict", "Lax", "NoRestriction"], "description":
"Represents the cookies' 'SameSite' status:
https://tools.ietf.org/html/draft-west-first-party-cookies" }
What dgozman suggests is that you add "optional": true into this and only set it
when the value is non-default.

[Mike West, 2016-04-05 08:07:08]

Thanks, I've taken another pass. WDYT?

https://codereview.chromium.org/1827743002/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/1827743002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:258:
->set_same_site(same_site));
On 2016/04/01 at 22:04:08, pfeldman wrote:
> What is the value in case SameSite is not specified? Can we make this protocol
parameter optional?

Done.

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt
(right):

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt:63:
sameSite : undefined
On 2016/04/01 at 22:04:08, pfeldman wrote:
> Then we would omit default value here altogether.

Done.

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/components/cookie-parser-expected.txt
(right):

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/components/cookie-parser-expected.txt:4:
name: cookie, value: value, httpOnly: false, sameSite: undefined, secure: false,
session: true, path: undefined, domain: undefined, port: undefined, expires:
n/a, size: 12
We still have "undefined" here because the test writes the value regardless of
whether it exists as a string.

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/cookie-resource-match.html
(right):

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/cookie-resource-match.html:76:
sameSite: "",
On 2016/04/01 at 22:04:09, pfeldman wrote:
> And you would not need this

Done.

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1827743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1398: { "name": "sameSite",
"type": "string", "enum": ["Strict", "Lax", "NoRestriction"], "description":
"Represents the cookies' 'SameSite' status:
https://tools.ietf.org/html/draft-west-first-party-cookies" }
On 2016/04/01 at 22:04:09, pfeldman wrote:
> What dgozman suggests is that you add "optional": true into this and only set
it when the value is non-default.

Done.

[dgozman, 2016-04-05 17:34:54]

lgtm with comments

https://codereview.chromium.org/1827743002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/1827743002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:250:
devtools_cookie->set_same_site("Strict");
There should be generated constants for these (based on protocol.json enum):

(content::devtools::network::)cookie::kSameSiteStrict
(content::devtools::network::)cookie::kSameSiteLax

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js
(right):

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:264:
data.sameSite = (cookie.sameSite() ? cookie.sameSite() : "");
cookie.sameSite() || ""

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right):

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:388:
WebInspector.Cookie.SameSiteMode = {
There should be auto-generated NetworkAgent.CookieSameSite enum with
NetworkAgent.CookieSameSite.Lax and NetworkAgent.CookieSameSite.Strict values.

Also, I don't see you using it anywhere.

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:439: if
(protocolCookie["sameSite"] != undefined)
Omit comparison with undefined, just |if (protocolCookie[...])|

[Mike West, 2016-04-05 17:55:10]

Thanks!

https://codereview.chromium.org/1827743002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/1827743002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:250:
devtools_cookie->set_same_site("Strict");
On 2016/04/05 at 17:34:53, dgozman wrote:
> There should be generated constants for these (based on protocol.json enum):
> 
> (content::devtools::network::)cookie::kSameSiteStrict
> (content::devtools::network::)cookie::kSameSiteLax

Oh, great! I'll use them!

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js
(right):

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:264:
data.sameSite = (cookie.sameSite() ? cookie.sameSite() : "");
On 2016/04/05 at 17:34:53, dgozman wrote:
> cookie.sameSite() || ""

Should I change `secure` and `httpOnly` as well? I only went with this format
because those were already there. :)

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right):

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:388:
WebInspector.Cookie.SameSiteMode = {
On 2016/04/05 at 17:34:53, dgozman wrote:
> There should be auto-generated NetworkAgent.CookieSameSite enum with
NetworkAgent.CookieSameSite.Lax and NetworkAgent.CookieSameSite.Strict values.
> 
> Also, I don't see you using it anywhere.

Yeah. I'll just drop this entirely. Thanks!

[dgozman, 2016-04-05 17:58:24]

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js
(right):

https://codereview.chromium.org/1827743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:264:
data.sameSite = (cookie.sameSite() ? cookie.sameSite() : "");
On 2016/04/05 17:55:10, Mike West (OOO through 31st) wrote:
> On 2016/04/05 at 17:34:53, dgozman wrote:
> > cookie.sameSite() || ""
> 
> Should I change `secure` and `httpOnly` as well? I only went with this format
> because those were already there. :)

I think secure and httpOnly are booleans, while sameSite is a string now. Thus
the difference.

[Mike West, 2016-04-06 05:58:36]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-04-06 05:58:36]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1827743002/#ps80001
(title: "Feedback")

[commit-bot: I haz the power, 2016-04-06 05:59:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1827743002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1827743002/80001

[commit-bot: I haz the power, 2016-04-06 07:16:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-06 07:17:10]

Description was changed from

==========
SameSite: Teach devtools about the new 'samesite' syntax.

We used to treat 'samesite' as a binary flag, displaying a checkmark
just like 'secure' or 'httponly'. The attribute is now a trinary value in
the table: "", "Lax", or "Strict".

The protocol shifted to align with this change, switching from a boolean
value to an enum ("NoRestriction", "Lax", and "Strict", respectively).

BUG=596520
==========

to

==========
SameSite: Teach devtools about the new 'samesite' syntax.

We used to treat 'samesite' as a binary flag, displaying a checkmark
just like 'secure' or 'httponly'. The attribute is now a trinary value in
the table: "", "Lax", or "Strict".

The protocol shifted to align with this change, switching from a boolean
value to an enum ("NoRestriction", "Lax", and "Strict", respectively).

BUG=596520

Committed: https://crrev.com/4e0bddf6d2123979b95860fae65429014828b33f
Cr-Commit-Position: refs/heads/master@{#385393}
==========

[commit-bot: I haz the power, 2016-04-06 07:17:11]

Patchset 5 (id:??) landed as
https://crrev.com/4e0bddf6d2123979b95860fae65429014828b33f
Cr-Commit-Position: refs/heads/master@{#385393}