Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221093003:
[Devtools] Backend support for setCookie (Closed)
|
Created:
4 years, 4 months ago by allada Modified:
4 years, 4 months ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, darin-cc_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Backend support for setCookie
Added the ability to set/update cookie through protocol. This was
modeled after extensions set cookie method found here:
https://developer.chrome.com/extensions/cookies#method-set
BUG=166637
R=dgozman
Committed: https://crrev.com/3bb7f20f6f0fab7dbd0fe0f3fcfe10024348c57d
Cr-Commit-Position: refs/heads/master@{#412625}
Patch Set 1 : backend changes #
Total comments: 14
Patch Set 2 : backend changes #Patch Set 3 : removed tests #Patch Set 4 : removed files #Patch Set 5 : done #
Total comments: 13
Patch Set 6 : Merge branch 'master' into EDIT_COOKIE_BACKEND #Patch Set 7 : Changes #
Total comments: 4
Patch Set 8 : Changes #Patch Set 9 : Forgot to rename variables #
Messages
Total messages: 30 (11 generated)
PTL, I ran my test a few thousand times and non seemed to be flaky. I think we should push it through and see what happens?
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.cc:130: void CookieSetOnIO(const base::Closure& callback, bool success) { Let's send an error over protocol if !success. https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.cc:296: if (sameSite && *sameSite == "lax") There are generated constants for all enums in protocol. https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/network_handler.h (right): https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.h:38: const std::string& url, const std::string& name, Shouldn't these be one-per-line per style guide? https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.h:40: const std::string* path, bool* secure, bool* httpOnly, style: 4-space continuation indent https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html (right): https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html:33: deleteAllCookies, Empty lines please. https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:1244: { "name": "url", "type": "string", "description": "The request-URI to associate with the setting of the cookie. This value can affect the default domain and path values of the created cookie. If host permissions for this URL are not specified in the manifest file, the API call will fail." }, Let's move descriptions to Cookie object, except for default values. https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:1251: { "name": "sameSite", "type": "string", "enum": ["no_restriction", "lax", "strict"], "optional": true, "description": "The cookie's same-site status: defaults to 'no_restriction'." }, We should extract SameSite now and use it both in Cookie and setCookie.
I am also going to need another patch/review to merge to M53 (per request by paulirish@)
On 2016/08/09 01:13:10, Blaise wrote: > I am also going to need another patch/review to merge to M53 (per request by > paulirish@) ***Ignore last comment***
PTL, however I removed the tests because of the timing issues, I'll make a follow up patch to fix the callback issues. https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.cc:130: void CookieSetOnIO(const base::Closure& callback, bool success) { On 2016/08/09 01:07:25, dgozman wrote: > Let's send an error over protocol if !success. Done. https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.cc:296: if (sameSite && *sameSite == "lax") On 2016/08/09 01:07:24, dgozman wrote: > There are generated constants for all enums in protocol. Done. https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/network_handler.h (right): https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.h:38: const std::string& url, const std::string& name, On 2016/08/09 01:07:25, dgozman wrote: > Shouldn't these be one-per-line per style guide? Done. https://codereview.chromium.org/2221093003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/network_handler.h:40: const std::string* path, bool* secure, bool* httpOnly, On 2016/08/09 01:07:25, dgozman wrote: > style: 4-space continuation indent Done. https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html (right): https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html:33: deleteAllCookies, On 2016/08/09 01:07:25, dgozman wrote: > Empty lines please. Done. https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:1244: { "name": "url", "type": "string", "description": "The request-URI to associate with the setting of the cookie. This value can affect the default domain and path values of the created cookie. If host permissions for this URL are not specified in the manifest file, the API call will fail." }, On 2016/08/09 01:07:25, dgozman wrote: > Let's move descriptions to Cookie object, except for default values. Done. https://codereview.chromium.org/2221093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:1251: { "name": "sameSite", "type": "string", "enum": ["no_restriction", "lax", "strict"], "optional": true, "description": "The cookie's same-site status: defaults to 'no_restriction'." }, On 2016/08/09 01:07:25, dgozman wrote: > We should extract SameSite now and use it both in Cookie and setCookie. Done.
On 2016/08/10 02:46:54, Blaise wrote: > PTL, however I removed the tests because of the timing issues, I'll make a > follow up patch to fix the callback issues. Most of the comments are not actually fixed. And what are we going to do with tests?
PTL
Patchset #5 (id:120001) has been deleted
PTL
looks pretty good https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.cc:187: BrowserThread::PostTask( Just inline this into SetCookie method. https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.cc:341: ->set_success(success)); nit: this fits previous line https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/network_handler.h (right): https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.h:23: typedef base::Callback<void(bool)> SetCookieResultCallback; Modern style says typedef -> using. https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.h:44: bool* httpOnly, snake_case everywhere please https://codereview.chromium.org/2221093003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2221093003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:929: "enum": ["NoRestriction", "Strict", "Lax"], We never send NoRestriction. Let's make "not present" === "no restriction" (as we did in Cookie object). https://codereview.chromium.org/2221093003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:1266: "hidden": true Hidden was renamed to experimental.
PTL https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.cc:187: BrowserThread::PostTask( On 2016/08/12 05:22:46, dgozman wrote: > Just inline this into SetCookie method. I am following the pattern of DeleteCookieOnUI(), I can inline this, but it'll break the pattern. I can also change DeleteCookieOnUI() to match our pattern in a different CL. https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.cc:341: ->set_success(success)); On 2016/08/12 05:22:46, dgozman wrote: > nit: this fits previous line Done. https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/network_handler.h (right): https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.h:23: typedef base::Callback<void(bool)> SetCookieResultCallback; On 2016/08/12 05:22:46, dgozman wrote: > Modern style says typedef -> using. Done. https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.h:44: bool* httpOnly, On 2016/08/12 05:22:46, dgozman wrote: > snake_case everywhere please Done. https://codereview.chromium.org/2221093003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2221093003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:929: "enum": ["NoRestriction", "Strict", "Lax"], On 2016/08/12 05:22:46, dgozman wrote: > We never send NoRestriction. Let's make "not present" === "no restriction" (as > we did in Cookie object). Done. https://codereview.chromium.org/2221093003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:1266: "hidden": true On 2016/08/12 05:22:46, dgozman wrote: > Hidden was renamed to experimental. Done.
lgtm https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2221093003/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/network_handler.cc:187: BrowserThread::PostTask( On 2016/08/16 01:10:39, Blaise wrote: > On 2016/08/12 05:22:46, dgozman wrote: > > Just inline this into SetCookie method. > > I am following the pattern of DeleteCookieOnUI(), I can inline this, but it'll > break the pattern. I can also change DeleteCookieOnUI() to match our pattern in > a different CL. I see, thanks. https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html (right): https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html:67: setCookie({url: "http://127.0.0.1", name: "foo\0\r\na", value: "bar9"}, done); Let's also exercise passing domain, path, secure, httpOnly and sameSite fields. https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:927: "id": "SameSite", Let's call this CookieSameSite.
https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html (right): https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/cookies-protocol-test.html:67: setCookie({url: "http://127.0.0.1", name: "foo\0\r\na", value: "bar9"}, done); On 2016/08/16 18:56:59, dgozman wrote: > Let's also exercise passing domain, path, secure, httpOnly and sameSite fields. Done. https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2221093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:927: "id": "SameSite", On 2016/08/16 18:56:59, dgozman wrote: > Let's call this CookieSameSite. Done.
The CQ bit was checked by allada@chromium.org
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/2221093003/#ps200001 (title: "Changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by allada@chromium.org
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/2221093003/#ps220001 (title: "Forgot to rename variables")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Backend support for setCookie Added the ability to set/update cookie through protocol. This was modeled after extensions set cookie method found here: https://developer.chrome.com/extensions/cookies#method-set BUG=166637 R=dgozman ========== to ========== [Devtools] Backend support for setCookie Added the ability to set/update cookie through protocol. This was modeled after extensions set cookie method found here: https://developer.chrome.com/extensions/cookies#method-set BUG=166637 R=dgozman Committed: https://crrev.com/3bb7f20f6f0fab7dbd0fe0f3fcfe10024348c57d Cr-Commit-Position: refs/heads/master@{#412625} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3bb7f20f6f0fab7dbd0fe0f3fcfe10024348c57d Cr-Commit-Position: refs/heads/master@{#412625} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
