|
|
Created:
3 years, 8 months ago by Marc Treib Modified:
3 years, 8 months ago Reviewers:
fhorschig CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Doodle] Allow cookie authentication
This enables birthday doodles.
BUG=715154
Review-Url: https://codereview.chromium.org/2838713003
Cr-Commit-Position: refs/heads/master@{#467290}
Committed: https://chromium.googlesource.com/chromium/src/+/29b3b5cf99713f88fd9b5739dcbb2f476e2721e0
Patch Set 1 #
Total comments: 2
Patch Set 2 : review #Patch Set 3 : review2 #Messages
Total messages: 23 (15 generated)
The CQ bit was checked by treib@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
treib@chromium.org changed reviewers: + fhorschig@chromium.org
+rhalavati FYI
lgtm w/ nit https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl.cc (right): https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl.cc:92: "appropriate times." nit: pretty hard to read. How about: The user's Google cookies. Used [for example] to show birthday doodles at appropriate times. ... is there any other use case than birthday doodles? If not, drop the [for example].
The CQ bit was checked by treib@chromium.org to run a CQ dry run
https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl.cc (right): https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl.cc:92: "appropriate times." On 2017/04/26 08:08:53, fhorschig wrote: > nit: pretty hard to read. How about: > > The user's Google cookies. Used [for example] to show birthday doodles at > appropriate times. Done. > ... is there any other use case than birthday doodles? > If not, drop the [for example]. I'm not aware of any other use case, but a) it's hard to be sure, and b) it might change in the future, and when that happens, this string will almost certainly not get updated. So I'd like to leave a bit of wiggle room.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/26 08:32:00, Marc Treib wrote: > https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... > File components/doodle/doodle_fetcher_impl.cc (right): > > https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... > components/doodle/doodle_fetcher_impl.cc:92: "appropriate times." > On 2017/04/26 08:08:53, fhorschig wrote: > > nit: pretty hard to read. How about: > > > > The user's Google cookies. Used [for example] to show birthday doodles at > > appropriate times. > > Done. > > > ... is there any other use case than birthday doodles? > > If not, drop the [for example]. > > I'm not aware of any other use case, but a) it's hard to be sure, and b) it > might change in the future, and when that happens, this string will almost > certainly not get updated. So I'd like to leave a bit of wiggle room. We will soon add presubmit checker for annotations to prevent syntax problems. When you add "cookies_allowed: true", you also need to add 'cookies_store: "something"' as well, as in: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/devtools_ui.cc?r...
On 2017/04/26 09:13:05, Ramin Halavati wrote: > On 2017/04/26 08:32:00, Marc Treib wrote: > > > https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... > > File components/doodle/doodle_fetcher_impl.cc (right): > > > > > https://codereview.chromium.org/2838713003/diff/1/components/doodle/doodle_fe... > > components/doodle/doodle_fetcher_impl.cc:92: "appropriate times." > > On 2017/04/26 08:08:53, fhorschig wrote: > > > nit: pretty hard to read. How about: > > > > > > The user's Google cookies. Used [for example] to show birthday doodles at > > > appropriate times. > > > > Done. > > > > > ... is there any other use case than birthday doodles? > > > If not, drop the [for example]. > > > > I'm not aware of any other use case, but a) it's hard to be sure, and b) it > > might change in the future, and when that happens, this string will almost > > certainly not get updated. So I'd like to leave a bit of wiggle room. > > We will soon add presubmit checker for annotations to prevent syntax problems. > When you add "cookies_allowed: true", you also need to add 'cookies_store: > "something"' as well, as in: > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/devtools_ui.cc?r... Thanks, and sorry for missing this! Done.
The CQ bit was checked by treib@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fhorschig@chromium.org Link to the patchset: https://codereview.chromium.org/2838713003/#ps40001 (title: "review2")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493202439149680, "parent_rev": "b9a42ab6f073b057c8c05d2bfc799dcc8082e2a6", "commit_rev": "29b3b5cf99713f88fd9b5739dcbb2f476e2721e0"}
Message was sent while issue was closed.
Description was changed from ========== [Doodle] Allow cookie authentication This enables birthday doodles. BUG=715154 ========== to ========== [Doodle] Allow cookie authentication This enables birthday doodles. BUG=715154 Review-Url: https://codereview.chromium.org/2838713003 Cr-Commit-Position: refs/heads/master@{#467290} Committed: https://chromium.googlesource.com/chromium/src/+/29b3b5cf99713f88fd9b5739dcbb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/29b3b5cf99713f88fd9b5739dcbb... |