Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261483002:
headless: Add a test for setting cookies via DevTools (Closed)
Descriptionheadless: Add a test for setting cookies via DevTools
This patch adds a headless test for setting cookies using the recently
added DevTools cookie editing commands.
BUG=546953
Committed: https://crrev.com/da38573f708f7725faaab8fe3dec7fcf722f54c8
Cr-Commit-Position: refs/heads/master@{#412863}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review comments #Messages
Total messages: 18 (10 generated)
Description was changed from ========== headless: Add a test for setting cookies via DevTools This patch adds a headless test for setting cookies using the recently added DevTools cookie editing commands. BUG=546953 ========== to ========== headless: Add a test for setting cookies via DevTools This patch adds a headless test for setting cookies using the recently added DevTools cookie editing commands. BUG=546953 ==========
skyostil@chromium.org changed reviewers: + alexclarke@chromium.org, eseckler@chromium.org
The CQ bit was checked by skyostil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2261483002/diff/1/headless/lib/headless_brows... File headless/lib/headless_browser_browsertest.cc (right): https://codereview.chromium.org/2261483002/diff/1/headless/lib/headless_brows... headless/lib/headless_browser_browsertest.cc:549: std::unique_ptr<network::SetCookieParams> params = nit: set_cookie_params and below too? https://codereview.chromium.org/2261483002/diff/1/headless/lib/headless_brows... headless/lib/headless_browser_browsertest.cc:568: .SetPath("") Maybe add a comment explaining why you need to set path etc?
lgtm. Should we also add tests for getCookies / deleteCookie, or are they less important for headless?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/18 15:09:58, Eric Seckler wrote: > Should we also add tests for getCookies / deleteCookie, or are they less > important for headless? Good question. I think setCookies is the most interesting one for now. Let's add tests for the others if we start relying on them. https://codereview.chromium.org/2261483002/diff/1/headless/lib/headless_brows... File headless/lib/headless_browser_browsertest.cc (right): https://codereview.chromium.org/2261483002/diff/1/headless/lib/headless_brows... headless/lib/headless_browser_browsertest.cc:549: std::unique_ptr<network::SetCookieParams> params = On 2016/08/18 15:05:12, alex clarke wrote: > nit: set_cookie_params and below too? Done. https://codereview.chromium.org/2261483002/diff/1/headless/lib/headless_brows... headless/lib/headless_browser_browsertest.cc:568: .SetPath("") On 2016/08/18 15:05:12, alex clarke wrote: > Maybe add a comment explaining why you need to set path etc? Good point, done.
lgtm
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org Link to the patchset: https://codereview.chromium.org/2261483002/#ps20001 (title: "Review comments")
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.
Description was changed from ========== headless: Add a test for setting cookies via DevTools This patch adds a headless test for setting cookies using the recently added DevTools cookie editing commands. BUG=546953 ========== to ========== headless: Add a test for setting cookies via DevTools This patch adds a headless test for setting cookies using the recently added DevTools cookie editing commands. BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== headless: Add a test for setting cookies via DevTools This patch adds a headless test for setting cookies using the recently added DevTools cookie editing commands. BUG=546953 ========== to ========== headless: Add a test for setting cookies via DevTools This patch adds a headless test for setting cookies using the recently added DevTools cookie editing commands. BUG=546953 Committed: https://crrev.com/da38573f708f7725faaab8fe3dec7fcf722f54c8 Cr-Commit-Position: refs/heads/master@{#412863} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/da38573f708f7725faaab8fe3dec7fcf722f54c8 Cr-Commit-Position: refs/heads/master@{#412863} |
