[Doodle] Introduce a DoodleService

components/doodle/doodle_service.h

Index: components/doodle/doodle_service.h
diff --git a/components/doodle/doodle_service.h b/components/doodle/doodle_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..2c16d0df3dd91bddb0f503e405b911c84fa99b48
--- /dev/null
+++ b/components/doodle/doodle_service.h
@@ -0,0 +1,60 @@
+// 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 COMPONENTS_DOODLE_DOODLE_SERVICE_H_
+#define COMPONENTS_DOODLE_DOODLE_SERVICE_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/observer_list.h"
+#include "base/optional.h"
+#include "components/doodle/doodle_fetcher.h"
+#include "components/doodle/doodle_types.h"
+
+namespace doodle {
+
+class DoodleService {
+ public:
+  class Observer {

[fhorschig, 2017/02/24 15:33:14]

Is there a special reason that we use observers instead of callbacks?
I am not sure how many clients would care about multiple updates of
the doodle (and OnceCallbacks would be nice then).
But I guess you have some use case in mind?

E.g. you have NTPs and the Halloween doodle. 
1. You open an NTP and get a Doodle.
2. You open a second NTP (or trigger some Refresh in any way).
3. The fetched doodle is different because there are multiple 
version of the Halloween doodle.
4. The perfectly fine doodle on the first NTP would change.
(An unnecessary UI update that would make UX unhappy)

Does that sound reasonable?

[Marc Treib, 2017/02/24 16:18:51]

The current NTP integration is based on an observer. I think the main use case
is startup: The NTP asks for a Doodle, but there isn't one yet (and we can't
wait for it, because we don't want to slow down the page load). Instead, it gets
called back as soon as the Doodle has been downloaded, and updates in-place.

Now, we could implement the same thing without observers of course, but I think
that'd make things more complex - clients would have to care about whether the
Doodle comes from the cache or not.
It's by definition an update to a currently-not-visible UI though, so I don't
think it's a big deal.

Maybe we want to remove Doodles immediately when they expire? Not sure about
that.
See above :)

[fhorschig, 2017/02/24 16:38:20]

Consideration of caching is a good argument.
Hmmm, Remove expired Doodlesmakes sense. That alone would be argument 
enough for an Observer.

Probably the Doodle is an edge case for UI updates. I cannot assess how
important a
persistent UI would be here.

[Marc Treib, 2017/02/27 13:55:41]

We're also keeping the status quo.
I'm open for discussions about changing it, but let's do that separately from
the API switch. One step at a time!

+   public:
+    virtual void OnDoodleConfigUpdated(const base::Optional<DoodleConfig>&) = 0;
+  };
+
+  DoodleService(std::unique_ptr<DoodleFetcher> fetcher);
+  ~DoodleService();
+
+  // Adds a new observer to the service. It'll be called immediately with the
+  // current DoodleConfig, or nullopt if there is no doodle, or the config
+  // hasn't been downloaded yet.

[Marc Treib, 2017/02/24 14:04:11]

This (immediately notifying the just-added observer) is maybe a bit
controversial. The alternative would be to add a "GetCurrentConfig", but I like
the simplicity of having just one single way of passing the config around,
namely through the observer.
WDYT?

[fhorschig, 2017/02/24 15:33:14]

This side-effect can quickly slip one's attention and I expect very few people
to read the documentation for an AddObserver method. So I would definitely
prefer the second method.

[Marc Treib, 2017/02/24 16:18:51]

How about if we name it AddObserverAndNotify or something like that?

[fhorschig, 2017/02/24 16:38:20]

Definitely an improvement but I still prefer very clear communication.
I don't think it is too much of a hassle to call a GetDoodleConfig function
and subscribe afterwards for changes.
Maybe I can roll along with this one if we find a catchy name. If I read the
"AndNotify" part, I would intuitively assume to notify the service.

... but at least I would read the documentation in that case.

[Marc Treib, 2017/02/27 13:55:41]

Alright, auto-notifications on AddObserver are gone.

+  void AddObserver(Observer* observer);
+
+  // Prevents |observer| from receiving future updates. This is safe to call
+  // even when the observer is being notified of an update.
+  void RemoveObserver(Observer* observer);
+
+  // Requests an asynchronous refresh of the DoodleConfig from the network.
+  // After the update completes, the observers will be notified only if the
+  // config changed.
+  void Refresh();
+
+ private:
+  void DoodleFetched(DoodleState state,
+                     const base::Optional<DoodleConfig>& doodle_config);
+
+  // The fetcher for getting fresh DoodleConfigs from the network.
+  std::unique_ptr<DoodleFetcher> fetcher_;
+
+  // The result of the last network fetch.
+  base::Optional<DoodleConfig> cached_config_;
+
+  // The list of observers to be notified when the DoodleConfig changes.
+  base::ObserverList<Observer> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(DoodleService);
+};
+
+}  // namespace doodle
+
+#endif  // COMPONENTS_DOODLE_DOODLE_SERVICE_H_