Add CategoryInfo for meta information of content suggestions Categories

components/ntp_snippets/content_suggestions_service.h

 // Copyright 2016 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_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_
 #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_
 
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/callback_forward.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ntp_snippets/category_factory.h"
 #include "components/ntp_snippets/category_status.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
 
 namespace gfx {
 class Image;
 }
 
 namespace ntp_snippets {
@@ skipping 46 matching lines @@
 
   State state() { return state_; }
 
   // Gets all categories for which a provider is registered. The categories
   // may or may not be available, see |GetCategoryStatus()|.
   const std::vector<Category>& GetCategories() const { return categories_; }
 
   // Gets the status of a category.
   CategoryStatus GetCategoryStatus(Category category) const;
 
+  // Gets the meta information of a category.
+  base::Optional<CategoryInfo> GetCategoryInfo(Category category) const;

[Bernhard Bauer, 2016/08/09 09:18:33]

Hum. You go to all these lengths to implement a move constructor for
CategoryInfo (presumably because copying it expensive), and then you return an
Optional here, which makes a copy of the value. Why not just return a pointer?
Really, that *is* the common idiom in Chromium C++ for returning optional "big"
values.

[Marc Treib, 2016/08/09 09:45:12]

CategoryInfo doesn't have a copy ctor, so it can't make a copy :)

Anyway, I'd argue "pointer" is the common idiom only because Optional hasn't
been around for long. It's certainly the clearer thing.

[Philipp Keck, 2016/08/09 10:03:06]

Indeed, the only relevant call we have
(https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...)
results in a call to the move constructor. I agree that base::Optional is more
readable, plus it works as intended.

[Bernhard Bauer, 2016/08/09 10:25:41]

If you are saying that over time Optional<> will replace pointers for passing
optional non-copyable things around, I'm fairly certain that will not be the
case. To me, Optional<T> signifies optional _values_, and T* signifies optional
(non-copyable) objects.
Ah right. It still feels a bit strange to construct a new string every time we
call GetCategoryInfo, even if it's not copied around...
Meh. It's very explicit about the fact that the lookup might fail, but at the
only current callsite we are iterating over the categories, so we should never
call it with a missing category. Really, this smacks a bit of handling
conditions that are not supposed to happen, which the style guide explicitly
argues against.

[Philipp Keck, 2016/08/09 11:24:27]

The callsites will change soon:
https://codereview.chromium.org/2230473002/diff/1/chrome/browser/android/ntp/...
https://codereview.chromium.org/2230473002/diff/1/chrome/android/java/src/org...
That could be called at any time, though currently it is still called right
after getCategories(), I think.

That being said, I'm also fine with using a pointer (and nullptr indicating
unavailability). That's also how it's treated behind the bridge: null means not
available.

+
   // Gets the available suggestions for a category. The result is empty if the
   // category is available and empty, but also if the category is unavailable
   // for any reason, see |GetCategoryStatus()|.
   const std::vector<ContentSuggestion>& GetSuggestionsForCategory(
       Category category) const;
 
   // Fetches the image for the suggestion with the given |suggestion_id| and
   // runs the |callback|. If that suggestion doesn't exist or the fetch fails,
   // the callback gets an empty image.
   void FetchSuggestionImage(const std::string& suggestion_id,
@@ skipping 102 matching lines @@
   // background fetching and debugging calls to it. If the NTPSnippetsService is
   // loaded, it is also present in |providers_|, otherwise this is a nullptr.
   NTPSnippetsService* ntp_snippets_service_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsService);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_