Local NTP: Introduce OneGoogleBarService

Local NTP: Introduce OneGoogleBarService

BUG=583292
Review-Url: https://codereview.chromium.org/2811353002
Cr-Commit-Position: refs/heads/master@{#467286}
Committed: https://chromium.googlesource.com/chromium/src/+/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e

[Marc Treib, 2017-04-12 17:24:01]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 17:24:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 17:45:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 17:45:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2017-04-13 09:14:40]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 09:15:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-04-13 09:38:57]

Patchset #3 (id:40001) has been deleted

[Marc Treib, 2017-04-13 14:01:01]

treib@chromium.org changed reviewers:
+ sfiera@chromium.org

[Marc Treib, 2017-04-13 14:01:02]

As threatened, here's the next part! PTAL!

[sfiera, 2017-04-13 15:18:31]

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service.cc (right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.cc:48:
signin_observer_ = base::MakeUnique<SigninObserver>(
Any reason not to do this in an initializer and make it const?

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service.h (right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:19: // A service
that downloads, caches, and hands out OneGoogleBarData.
I'd like a little detail about what is expected to happen in unusual or
transitional states, like:
- Service is created but we don't have an initial value yet.
- Service is created while Chrome is offline. When can we expect a value?
- User signs in/out. What's o_g_b_d() in the meantime?
- User's password changes, so we're no longer authenticated.

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:20: class
OneGoogleBarService : public KeyedService {
No expected need to split this into interface/impl?

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:36: void
Refresh();
For my curiosity, who are you expecting to call this and when?

Since sign-in/out doesn't trigger a refresh, and there's no periodic refresh, it
seems like the answer would be "often", but then it's difficult to rely on the
cached value.

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service_observer.h
(right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service_observer.h:17: const
base::Optional<OneGoogleBarData>& data) = 0;
Since the service provides an accessor, I would be inclined to provide either
the service or nothing to the observer here, so that it's clear that there is
one correct way to get the value.

[Marc Treib, 2017-04-13 15:44:02]

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service.cc (right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.cc:48:
signin_observer_ = base::MakeUnique<SigninObserver>(
On 2017/04/13 15:18:31, sfiera wrote:
> Any reason not to do this in an initializer and make it const?

It can't be const because I call .reset() on it below. Initializer works though.

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service.h (right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:19: // A service
that downloads, caches, and hands out OneGoogleBarData.
On 2017/04/13 15:18:31, sfiera wrote:
> I'd like a little detail about what is expected to happen in unusual or
> transitional states, like:
> - Service is created but we don't have an initial value yet.
> - Service is created while Chrome is offline. When can we expect a value?

It never initiates fetches itself. Comment added.

> - User signs in/out. What's o_g_b_d() in the meantime?

It gets cleared, i.e. nullopt. Comment added.

> - User's password changes, so we're no longer authenticated.

I'm not completely sure about this one. If the credentials are out of date,
we'll fail to get an access token, so we'll download the non-personalized bar,
which makes sense. But the service probably wouldn't notice by itself, so any
cached data might remain. I'll investigate if there's a notification it could
listen to.

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:20: class
OneGoogleBarService : public KeyedService {
On 2017/04/13 15:18:31, sfiera wrote:
> No expected need to split this into interface/impl?

Nope :)

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:36: void
Refresh();
On 2017/04/13 15:18:31, sfiera wrote:
> For my curiosity, who are you expecting to call this and when?
> 
> Since sign-in/out doesn't trigger a refresh, and there's no periodic refresh,
it
> seems like the answer would be "often", but then it's difficult to rely on the
> cached value.

The plan was: Open NTP, immediately serve the cached value, and also trigger a
refresh, for the next NTP. I might add a minimum time delay between refreshes,
e.g. 15 minutes.

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service_observer.h
(right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service_observer.h:17: const
base::Optional<OneGoogleBarData>& data) = 0;
On 2017/04/13 15:18:31, sfiera wrote:
> Since the service provides an accessor, I would be inclined to provide either
> the service or nothing to the observer here, so that it's clear that there is
> one correct way to get the value.

Hm. I do like the idea of "one way to get the value", but it's also somewhat
less convenient for clients...

[sfiera, 2017-04-13 15:58:33]

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service.h (right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:36: void
Refresh();
On 2017/04/13 15:44:02, Marc Treib (OOO till April 25) wrote:
> On 2017/04/13 15:18:31, sfiera wrote:
> > For my curiosity, who are you expecting to call this and when?
> > 
> > Since sign-in/out doesn't trigger a refresh, and there's no periodic
refresh,
> it
> > seems like the answer would be "often", but then it's difficult to rely on
the
> > cached value.
> 
> The plan was: Open NTP, immediately serve the cached value, and also trigger a
> refresh, for the next NTP. I might add a minimum time delay between refreshes,
> e.g. 15 minutes.

Do we have a list of all of the places we have this kind of logic? (NTP displays
out-of-date information, but starts an update for the next NTP)

It would be cool at some point to bundle refresh logic for all of these things
(Top Sites, Most Likely, Popular Sites, Snippets, One Google Bar, Doodles, more
to come?) into one NTPFreshnessService or something that's tries to be smarter
about when to refresh and how often. It would seem to be a natural extension of
Jan's work for snippets freshness.

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service_observer.h
(right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service_observer.h:17: const
base::Optional<OneGoogleBarData>& data) = 0;
On 2017/04/13 15:44:02, Marc Treib (OOO till April 25) wrote:
> On 2017/04/13 15:18:31, sfiera wrote:
> > Since the service provides an accessor, I would be inclined to provide
either
> > the service or nothing to the observer here, so that it's clear that there
is
> > one correct way to get the value.
> 
> Hm. I do like the idea of "one way to get the value", but it's also somewhat
> less convenient for clients...

Passing nothing would be a lot less convenient, since you'd have to save a
pointer to the service. Passing the service seems OK to me; use
service->one_google_bar_data() instead of data.

[sfiera, 2017-04-13 16:28:51]

LGTM

[Marc Treib, 2017-04-13 16:45:15]

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service.h (right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service.h:36: void
Refresh();
On 2017/04/13 15:58:33, sfiera wrote:
> On 2017/04/13 15:44:02, Marc Treib (OOO till April 25) wrote:
> > On 2017/04/13 15:18:31, sfiera wrote:
> > > For my curiosity, who are you expecting to call this and when?
> > > 
> > > Since sign-in/out doesn't trigger a refresh, and there's no periodic
> refresh,
> > it
> > > seems like the answer would be "often", but then it's difficult to rely on
> the
> > > cached value.
> > 
> > The plan was: Open NTP, immediately serve the cached value, and also trigger
a
> > refresh, for the next NTP. I might add a minimum time delay between
refreshes,
> > e.g. 15 minutes.
> 
> Do we have a list of all of the places we have this kind of logic? (NTP
displays
> out-of-date information, but starts an update for the next NTP)

That seems to be quite a comprehensive list; I can't think of other things right
now.

> It would be cool at some point to bundle refresh logic for all of these things
> (Top Sites, Most Likely, Popular Sites, Snippets, One Google Bar, Doodles,
more
> to come?) into one NTPFreshnessService or something that's tries to be smarter
> about when to refresh and how often. It would seem to be a natural extension
of
> Jan's work for snippets freshness.

Maybe. I expect that things might get a bit messy since different things have
different requirements for refreshing: e.g. clearing on sign-in/out, clearing on
history deletions, live-updatable or not (doodles are, tiles are but maybe
shouldn't be, the OGB isn't), ...
Definitely worth looking into though!

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
File chrome/browser/search/one_google_bar/one_google_bar_service_observer.h
(right):

https://codereview.chromium.org/2811353002/diff/80001/chrome/browser/search/o...
chrome/browser/search/one_google_bar/one_google_bar_service_observer.h:17: const
base::Optional<OneGoogleBarData>& data) = 0;
On 2017/04/13 15:58:33, sfiera wrote:
> On 2017/04/13 15:44:02, Marc Treib (OOO till April 25) wrote:
> > On 2017/04/13 15:18:31, sfiera wrote:
> > > Since the service provides an accessor, I would be inclined to provide
> either
> > > the service or nothing to the observer here, so that it's clear that there
> is
> > > one correct way to get the value.
> > 
> > Hm. I do like the idea of "one way to get the value", but it's also somewhat
> > less convenient for clients...
> 
> Passing nothing would be a lot less convenient, since you'd have to save a
> pointer to the service. Passing the service seems OK to me; use
> service->one_google_bar_data() instead of data.

Eeh, in my experience most observers need to keep a pointer to the service
around anyway. Here in particular, the one pending implementation does that.
I've removed the data param.

[Marc Treib, 2017-04-13 16:45:42]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 16:46:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-04-13 17:00:13]

The CQ bit was unchecked by treib@chromium.org

[Marc Treib, 2017-04-13 17:00:14]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-04-13 17:00:14]

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/2811353002/#ps120001
(title: "review2")

[commit-bot: I haz the power, 2017-04-13 17:00:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-04-13 19:47:33]

The CQ bit was unchecked by treib@chromium.org

[Marc Treib, 2017-04-25 16:37:45]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 16:37:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 19:06:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 19:06:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2017-04-26 09:20:30]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 09:20:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 10:22:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 10:22:58]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-04-26 10:25:26]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-04-26 10:25:26]

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/2811353002/#ps180001
(title: "fix memleak")

[commit-bot: I haz the power, 2017-04-26 10:25:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 10:29:43]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493202326638380,
"parent_rev": "e08ac360faf4df6fc67b419d549ad682f8e56962", "commit_rev":
"2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e"}

[commit-bot: I haz the power, 2017-04-26 10:29:51]

Description was changed from

==========
Local NTP: Introduce OneGoogleBarService

BUG=583292
==========

to

==========
Local NTP: Introduce OneGoogleBarService

BUG=583292

Review-Url: https://codereview.chromium.org/2811353002
Cr-Commit-Position: refs/heads/master@{#467286}
Committed:
https://chromium.googlesource.com/chromium/src/+/2edac8fe131cb58ae2bc37c007cc...
==========

[commit-bot: I haz the power, 2017-04-26 10:29:52]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/2edac8fe131cb58ae2bc37c007cc...