Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814753002:
Local NTP: Introduce OneGoogleBarFetcher (Closed)
|
Created:
3 years, 8 months ago by Marc Treib Modified:
3 years, 8 months ago Reviewers:
sfiera CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLocal NTP: Introduce OneGoogleBarFetcher
BUG=583292
Review-Url: https://codereview.chromium.org/2814753002
Cr-Commit-Position: refs/heads/master@{#464393}
Committed: https://chromium.googlesource.com/chromium/src/+/50ddc321d12aa3b47c5e39331e5a2d4a4466226c
Patch Set 1 #Patch Set 2 : copy ctor #
Total comments: 14
Patch Set 3 : review1 #
Total comments: 17
Patch Set 4 : fix tests #Patch Set 5 : review2 #Patch Set 6 : safe_json #Dependent Patchsets: Messages
Total messages: 34 (23 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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: + sfiera@chromium.org
Let's try to get some of the actual code landed in parallel to all the CSP yak shaving rabbit hole... PTAL! https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h:26: class OneGoogleBarFetcherImpl : public OneGoogleBarFetcher { So far, this is the only implementation, but we'll want a fake implementation for tests soon enough, so better to split into interface+impl in the first place.
Not finished but flushing for today https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_data.h (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_data.h:18: std::string in_head_noscript; Comments? I can guess what most of these are, but in_head_noscript lost me. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher.h (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher.h:18: virtual void Fetch(OneGoogleCallback callback) = 0; Comments? In particular, I would wonder if the caller, callee, or neither should be caching the result. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:33: const char kApiKeyFormat[] = "?key=%s"; This is at least the second time that we've written this exact code, right? Maybe it's time for a google_api_client component. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:130: SigninManagerBase* signin_manager_; I'm in favor of "(*) const" whenever possible because the compiler will warn you if you're missing initializers for const members. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:156: default; Inline with the class? https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:227: url_fetcher_->SetExtraRequestHeaders(GetExtraRequestHeaders(access_token)); Should we send variations headers too? https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h:26: class OneGoogleBarFetcherImpl : public OneGoogleBarFetcher { On 2017/04/12 10:19:48, Marc Treib wrote: > So far, this is the only implementation, but we'll want a fake implementation > for tests soon enough, so better to split into interface+impl in the first > place. Acknowledged.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Thanks! Comments addressed. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_data.h (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_data.h:18: std::string in_head_noscript; On 2017/04/12 17:25:47, sfiera wrote: > Comments? I can guess what most of these are, but in_head_noscript lost me. It's for a <noscript> tag :) https://www.w3schools.com/tags/tag_noscript.asp However, a) there are no plans to actually use it (the remote NTP doesn't, and it makes no sense on the local one either), and b) it seems to be empty in practice anyway, so I've just removed it. I've added comments for the rest, though it's mostly "this is what the API spits out". https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher.h (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher.h:18: virtual void Fetch(OneGoogleCallback callback) = 0; On 2017/04/12 17:25:47, sfiera wrote: > Comments? > > In particular, I would wonder if the caller, callee, or neither should be > caching the result. Added a comment which hopefully makes things clear - the fetcher just fetches and doesn't know or care about caching. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:33: const char kApiKeyFormat[] = "?key=%s"; On 2017/04/12 17:25:47, sfiera wrote: > This is at least the second time that we've written this exact code, right? > Maybe it's time for a google_api_client component. Agreed - that's essentially crbug.com/609084, which I've started on with the AccessTokenFetcher class, but of course it's hard to find the time. Feel free to take it over ;) https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:130: SigninManagerBase* signin_manager_; On 2017/04/12 17:25:47, sfiera wrote: > I'm in favor of "(*) const" whenever possible because the compiler will warn you > if you're missing initializers for const members. Done. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:156: default; On 2017/04/12 17:25:47, sfiera wrote: > Inline with the class? Sure, done. https://codereview.chromium.org/2814753002/diff/20001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:227: url_fetcher_->SetExtraRequestHeaders(GetExtraRequestHeaders(access_token)); On 2017/04/12 17:25:47, sfiera wrote: > Should we send variations headers too? Good idea! There are no concrete plans to use them, but it can't hurt to have the option. Done.
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:193: return base::StringPrintf(kRequestBodyFormat, GetUserAgent().c_str(), Do we know for sure that these values don't need any escaping? https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:212: /*incognito=*/false, /*uma_enabled*/ false, missing an = https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:309: base::JSONReader::ReadAndReturnError(response_sp, Sorry, but… safe_json? https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:312: if (!value) { Would be nice to have the remainder of the function be just Respond(JSONToOGBData()). https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:353: Respond(result); This is going to make a copy of |result| to create the Optional, right? https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc:169: } Looks like we're leaving a request hanging. Is that state cleared up at the end of the unittest? https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc:232: } Test with )]}' ? Test that verifies JSON -> OneGoogleBarData translation?
More comments addressed :) https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:193: return base::StringPrintf(kRequestBodyFormat, GetUserAgent().c_str(), On 2017/04/13 09:42:51, sfiera wrote: > Do we know for sure that these values don't need any escaping? Good point - not sure, no. It's probably better anyway to build this up through base::Values. https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:212: /*incognito=*/false, /*uma_enabled*/ false, On 2017/04/13 09:42:51, sfiera wrote: > missing an = Done. https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:309: base::JSONReader::ReadAndReturnError(response_sp, On 2017/04/13 09:42:51, sfiera wrote: > Sorry, but… safe_json? Hm, not sure if that's required here: We're getting json from a trusted Google server, via https. Seems highly unlikely anything bad would happen here, so I'm not sure it's worth the cost of the extra process. Do we have any guidance on when to use safe_json? https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:312: if (!value) { On 2017/04/13 09:42:51, sfiera wrote: > Would be nice to have the remainder of the function be just > Respond(JSONToOGBData()). Good idea! Done. https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:353: Respond(result); On 2017/04/13 09:42:51, sfiera wrote: > This is going to make a copy of |result| to create the Optional, right? I think so, yes. I've made OneGoogleBarData movable; together with the new JsonToOGBData that should get rid of the copy. Is that what you had in mind? https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc:169: } On 2017/04/13 09:42:51, sfiera wrote: > Looks like we're leaving a request hanging. Is that state cleared up at the end > of the unittest? You mean, so that it won't mess up other tests? Yeah, pretty sure. It's fairly hard to make things survive across tests; basically only global variables survive, and I'd very much hope that the test URL fetcher stuff doesn't use those. https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc:232: } On 2017/04/13 09:42:51, sfiera wrote: > Test with )]}' ? Done. > Test that verifies JSON -> OneGoogleBarData translation? Done.
LGTM https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:309: base::JSONReader::ReadAndReturnError(response_sp, On 2017/04/13 10:23:53, Marc Treib wrote: > On 2017/04/13 09:42:51, sfiera wrote: > > Sorry, but… safe_json? > > Hm, not sure if that's required here: We're getting json from a trusted Google > server, via https. Seems highly unlikely anything bad would happen here, so I'm > not sure it's worth the cost of the extra process. Do we have any guidance on > when to use safe_json? That has been true of all the other cases I know too (popular sites, content suggestions API). I'm not aware of any answer to the question "Should I use safe_json?" more nuanced than "yes" :) https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:353: Respond(result); On 2017/04/13 10:23:53, Marc Treib wrote: > On 2017/04/13 09:42:51, sfiera wrote: > > This is going to make a copy of |result| to create the Optional, right? > > I think so, yes. I've made OneGoogleBarData movable; together with the new > JsonToOGBData that should get rid of the copy. Is that what you had in mind? Yep, looks good.
Patchset #6 (id:80002) has been deleted
The CQ bit was checked by treib@chromium.org to run a CQ dry run
https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2814753002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:309: base::JSONReader::ReadAndReturnError(response_sp, On 2017/04/13 10:38:38, sfiera wrote: > On 2017/04/13 10:23:53, Marc Treib wrote: > > On 2017/04/13 09:42:51, sfiera wrote: > > > Sorry, but… safe_json? > > > > Hm, not sure if that's required here: We're getting json from a trusted Google > > server, via https. Seems highly unlikely anything bad would happen here, so > I'm > > not sure it's worth the cost of the extra process. Do we have any guidance on > > when to use safe_json? > > That has been true of all the other cases I know too (popular sites, content > suggestions API). I'm not aware of any answer to the question "Should I use > safe_json?" more nuanced than "yes" :) It was not true in the original case that triggered the whole thing (supervised user whitelists), but alright. safe_json it is.
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.
Did you want to look at this again, or can I land?
Punch it.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2814753002/#ps110001 (title: "safe_json")
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": 110001, "attempt_start_ts": 1492091058149340,
"parent_rev": "a356ba7b790ece519e5d162a80813505d5f62449", "commit_rev":
"50ddc321d12aa3b47c5e39331e5a2d4a4466226c"}
CQ is committing da patch.
Bot data: {"patchset_id": 110001, "attempt_start_ts": 1492091058149340,
"parent_rev": "a356ba7b790ece519e5d162a80813505d5f62449", "commit_rev":
"50ddc321d12aa3b47c5e39331e5a2d4a4466226c"}
Message was sent while issue was closed.
Description was changed from ========== Local NTP: Introduce OneGoogleBarFetcher BUG=583292 ========== to ========== Local NTP: Introduce OneGoogleBarFetcher BUG=583292 Review-Url: https://codereview.chromium.org/2814753002 Cr-Commit-Position: refs/heads/master@{#464393} Committed: https://chromium.googlesource.com/chromium/src/+/50ddc321d12aa3b47c5e39331e5a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:110001) as https://chromium.googlesource.com/chromium/src/+/50ddc321d12aa3b47c5e39331e5a... |
