Local NTP: Introduce OneGoogleBarService

chrome/browser/search/one_google_bar/one_google_bar_service.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SEARCH_ONE_GOOGLE_BAR_ONE_GOOGLE_BAR_SERVICE_H_
+#define CHROME_BROWSER_SEARCH_ONE_GOOGLE_BAR_ONE_GOOGLE_BAR_SERVICE_H_
+
+#include <memory>
+
+#include "base/observer_list.h"
+#include "base/optional.h"
+#include "chrome/browser/search/one_google_bar/one_google_bar_data.h"
+#include "chrome/browser/search/one_google_bar/one_google_bar_service_observer.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+class OneGoogleBarFetcher;
+class SigninManagerBase;
+
+// A service that downloads, caches, and hands out OneGoogleBarData.

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

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.

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

It never initiates fetches itself. Comment added.
It gets cleared, i.e. nullopt. Comment added.
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.

+class OneGoogleBarService : public KeyedService {

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

No expected need to split this into interface/impl?

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

Nope :)

+ public:
+  OneGoogleBarService(SigninManagerBase* signin_manager,
+                      std::unique_ptr<OneGoogleBarFetcher> fetcher);
+  ~OneGoogleBarService() override;
+
+  // KeyedService implementation.
+  void Shutdown() override;
+
+  // Returns the currently cached OneGoogleBarData, if any.
+  const base::Optional<OneGoogleBarData>& one_google_bar_data() const {
+    return one_google_bar_data_;
+  }
+
+  // Requests an asynchronous refresh from the network. After the update
+  // completes, the observers will be notified only if something changed.
+  void Refresh();

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

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.

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

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.

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

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.

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

That seems to be quite a comprehensive list; I can't think of other things right
now.
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!

+
+  // Add/remove observers. All observers must unregister themselves before the
+  // OneGoogleBarService is destroyed.
+  void AddObserver(OneGoogleBarServiceObserver* observer);
+  void RemoveObserver(OneGoogleBarServiceObserver* observer);
+
+ private:
+  class SigninObserver;
+
+  void SigninStatusChanged();
+
+  void SetOneGoogleBarData(const base::Optional<OneGoogleBarData>& data);
+
+  std::unique_ptr<OneGoogleBarFetcher> fetcher_;
+
+  std::unique_ptr<SigninObserver> signin_observer_;
+
+  base::ObserverList<OneGoogleBarServiceObserver, true> observers_;
+
+  base::Optional<OneGoogleBarData> one_google_bar_data_;
+};
+
+#endif  // CHROME_BROWSER_SEARCH_ONE_GOOGLE_BAR_ONE_GOOGLE_BAR_SERVICE_H_