[NTP Snippets] Refactor home-grown container API

[NTP Snippets] Refactor home-grown container API

The service was originally a much simpler class but it nowadays doesn't
make sense to expose home-grown begin/end/size signatures.

The only downside is that constness is lost for exposed snippets due
to std::unique_ptr exposing non-const raw pointers. The alternative is
std::reference_wrapper which is not yet allowed by the style guide, but
the issue is not important enough to maintain inner_iterator.h.

BUG=584428
Committed: https://crrev.com/8c7889b1c478cbe766a57cd14845ba0bbd16a936
Cr-Commit-Position: refs/heads/master@{#392631}

[mastiz, 2016-05-09 15:36:54]

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

[Marc Treib, 2016-05-10 07:28:50]

LGTM with one semi-related optional nit below. My second comment (front instead
of begin) you already guessed :D

https://codereview.chromium.org/1958163002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1958163002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:123: const NTPSnippetStorage&
snippets() const { return snippets_; }
Not your doing, but the ordering is a bit weird here. Could you move this and
discarded_snippets() up, say between Shutdown() and FetchSnippets()?

[mastiz, 2016-05-10 07:35:16]

https://codereview.chromium.org/1958163002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1958163002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:123: const NTPSnippetStorage&
snippets() const { return snippets_; }
On 2016/05/10 07:28:50, Marc Treib wrote:
> Not your doing, but the ordering is a bit weird here. Could you move this and
> discarded_snippets() up, say between Shutdown() and FetchSnippets()?

Done, moved *after* FetchSnippetsFromHosts(), to stay together with other simple
getters like last_status().

[mastiz, 2016-05-10 07:35:55]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-05-10 07:35:56]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1958163002/#ps60001
(title: "Moved getters up as suggested.")

[commit-bot: I haz the power, 2016-05-10 07:36:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1958163002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1958163002/60001

[commit-bot: I haz the power, 2016-05-10 07:51:00]

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

[commit-bot: I haz the power, 2016-05-10 07:51:01]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[mastiz, 2016-05-10 08:24:08]

mastiz@chromium.org changed reviewers:
+ bauerb@chromium.org

[mastiz, 2016-05-10 08:24:09]

bauerb@chromium.org: Please review changes in 

chrome/browser/ui/webui/snippets_internals_message_handler.cc

[Bernhard Bauer, 2016-05-10 14:48:56]

lgtm

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:140: for (const auto& snippet
: ntp_snippets_service_->snippets()) {
Could you keep the original type? I find it's much more helpful to be able to
see it rather than having to infer it from the container type.

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:180: for (auto&
snippet : ntp_snippets_service_->snippets())
Make this const as well? Also, use the actual type?

[mastiz, 2016-05-10 15:59:52]

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:140: for (const auto& snippet
: ntp_snippets_service_->snippets()) {
On 2016/05/10 14:48:56, Bernhard Bauer wrote:
> Could you keep the original type? I find it's much more helpful to be able to
> see it rather than having to infer it from the container type.

Done.

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1958163002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:180: for (auto&
snippet : ntp_snippets_service_->snippets())
On 2016/05/10 14:48:56, Bernhard Bauer wrote:
> Make this const as well? Also, use the actual type?

Done.

[mastiz, 2016-05-10 16:00:01]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-05-10 16:00:01]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1958163002/#ps100001
(title: "Avoided auto as suggested.")

[commit-bot: I haz the power, 2016-05-10 16:00:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1958163002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1958163002/100001

[commit-bot: I haz the power, 2016-05-10 17:08:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-10 17:09:54]

Description was changed from

==========
[NTP Snippets] Refactor home-grown container API

The service was originally a much simpler class but it nowadays doesn't
make sense to expose home-grown begin/end/size signatures.

The only downside is that constness is lost for exposed snippets due
to std::unique_ptr exposing non-const raw pointers. The alternative is
std::reference_wrapper which is not yet allowed by the style guide, but
the issue is not important enough to maintain inner_iterator.h.

BUG=584428
==========

to

==========
[NTP Snippets] Refactor home-grown container API

The service was originally a much simpler class but it nowadays doesn't
make sense to expose home-grown begin/end/size signatures.

The only downside is that constness is lost for exposed snippets due
to std::unique_ptr exposing non-const raw pointers. The alternative is
std::reference_wrapper which is not yet allowed by the style guide, but
the issue is not important enough to maintain inner_iterator.h.

BUG=584428

Committed: https://crrev.com/8c7889b1c478cbe766a57cd14845ba0bbd16a936
Cr-Commit-Position: refs/heads/master@{#392631}
==========

[commit-bot: I haz the power, 2016-05-10 17:09:55]

Patchset 6 (id:??) landed as
https://crrev.com/8c7889b1c478cbe766a57cd14845ba0bbd16a936
Cr-Commit-Position: refs/heads/master@{#392631}