Implement first version of OfflinePageSuggestionsProvider

Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=628198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5044dec1802c333acca98268df660ccee3ddbd8d
Cr-Commit-Position: refs/heads/master@{#406028}

[Philipp Keck, 2016-07-14 11:37:32]

Description was changed from

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=619560
==========

to

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=619560
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Philipp Keck, 2016-07-14 11:41:24]

pke@google.com changed reviewers:
+ treib@chromium.org

[Philipp Keck, 2016-07-14 11:41:24]

PTAL

[Philipp Keck, 2016-07-14 11:44:17]

pke@google.com changed reviewers:
+ dewittj@chromium.org

[Philipp Keck, 2016-07-14 11:44:17]

dewittj@chromium.org: Please review changes in 
components/offline_pages/background/BUILD.gn

and please also have a look at the changes around
https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...

[Marc Treib, 2016-07-14 11:50:47]

Early comments:
- I think the Offline Pages integration deserves a separate bug from the generic
multi-provider one
- Have you tried building this on non-Android platforms? All the offline pages
code exists only on Android

[Philipp Keck, 2016-07-14 12:13:57]

Description was changed from

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=619560
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=628198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2016-07-14 12:16:54]

Mostly looks good!

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:66: "NTPOfflinePageSuggestions",
base::FEATURE_DISABLED_BY_DEFAULT};
nit: this is formatted differently than all the other entries

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.cc
(right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.cc:47:
DependsOn(ContentSuggestionsServiceFactory::GetInstance());
Add OfflinePageModelFactory?

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.cc:78: new
ntp_snippets::OfflinePageSuggestionsProvider(offline_page_model);
"ntp_snippets::" isn't needed; you have a "using" above

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.h
(right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.h:5:
#ifndef CHROME_BROWSER_NTP_SNIPPETS_OFFLINE_PAGE_SUGGESTIONS_PROVIDER_FACTORY_H_
Should this be in an offline_pages subfolder, for consistency with the code in
the component?

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gypi
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
Is this required?

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:7:
import("//build/config/android/rules.gni")
Required?

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:11:
static_library("offline_page_suggestions") {
nit: Should this have the same name as the folder?

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:21:
"//components/offline_pages:offline_pages",
nit: I think the ":offline_pages" etc isn't required, if it's the same as the
last path component

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:44:
// TODO(pke) Is this ok?
nit: "TODO(pke):" (add colon)

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:45:
return ContentSuggestionsCategoryStatus::AVAILABLE;
Why not "return category_status_;" ?

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:70:
// Ignore (when offline pages have been loaded, OnOfflinePagesLoaded will be
nit: DCHECK that model is the same that we have?

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:82:
// TODO(pke) Implement (does the ofp have to be cleared from UI immediately?)
It should be, yes (the card will link to the offline page directly, which won't
exist anymore).
(Of course, it's fine to leave this as a TODO for now)

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:106:
ContentSuggestion suggestion = ContentSuggestion(
nit: Just "ContentSuggestions suggestion(...)"

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h
(right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:17:
#include "components/offline_pages/offline_page_item.h"
nit: Not required I think

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:36:
base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
Is this required? It's already defined in ContentSuggestionsProvider.
(If a "using" is required at all, then at least make it "using
ImageFetchedCallback = ContentSuggestionsProvider::ImageFetchedCallback")

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:62: //
Queries the OfflinePageModel for offline pages
nit: period after comment

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:71:
ContentSuggestionsProvider::Observer* observer_ = nullptr;
Please let's keep all initialization in one place.

[Philipp Keck, 2016-07-14 12:28:20]

On 2016/07/14 11:50:47, Marc Treib wrote:
> Early comments:
> - I think the Offline Pages integration deserves a separate bug from the
generic
> multi-provider one
Created a new bug.

> - Have you tried building this on non-Android platforms? All the offline pages
> code exists only on Android
Good point. Used the trybots for that and, as you suspected, they failed for
non-Android. I fixed it so that it now compiles locally with the next Patch Set.
An alternative solution would be to move the entire
OfflinePageSuggestionsProviderFactory to c/b/android/ntp_snippets, though that
folder would need to be created.

[Marc Treib, 2016-07-14 12:32:50]

On 2016/07/14 12:28:20, Philipp Keck wrote:
> On 2016/07/14 11:50:47, Marc Treib wrote:
> > Early comments:
> > - I think the Offline Pages integration deserves a separate bug from the
> generic
> > multi-provider one
> Created a new bug.

Thanks!

> > - Have you tried building this on non-Android platforms? All the offline
pages
> > code exists only on Android
> Good point. Used the trybots for that and, as you suspected, they failed for
> non-Android. I fixed it so that it now compiles locally with the next Patch
Set.
> An alternative solution would be to move the entire
> OfflinePageSuggestionsProviderFactory to c/b/android/ntp_snippets, though that
> folder would need to be created.

But c/b/android/ntp exists, and the other ntp_snippets stuff lives there. Since
OfflinePages will probably never be implemented on non-Android platforms, it
might make sense to move the factory there. (But let's wait what Justin and the
Android people think)

[Philipp Keck, 2016-07-14 13:46:01]

pke@google.com changed reviewers:
+ bauerb@chromium.org

[Philipp Keck, 2016-07-14 13:46:03]

> > > - Have you tried building this on non-Android platforms? All the offline
> pages
> > > code exists only on Android
> > Good point. Used the trybots for that and, as you suspected, they failed for
> > non-Android. I fixed it so that it now compiles locally with the next Patch
> Set.
> > An alternative solution would be to move the entire
> > OfflinePageSuggestionsProviderFactory to c/b/android/ntp_snippets, though
that
> > folder would need to be created.
> 
> But c/b/android/ntp exists, and the other ntp_snippets stuff lives there.
Since
> OfflinePages will probably never be implemented on non-Android platforms, it
> might make sense to move the factory there. (But let's wait what Justin and
the
> Android people think)

Ok. I still uploaded a patch set with this change already made, but that's only
for testing with the trybots.
dewittj@ and bauerb@ what do you think?

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:66: "NTPOfflinePageSuggestions",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: this is formatted differently than all the other entries

This is "git cl" formatted. Both the seemingly missing space before "{" and the
missing newline before "}" are what the formatter does, so they are removed if
they're there. Something similar would happen to all the others, if I changed
them.

Actually I had a similar problem with about_flags.cc, where quite a few lines
that I never touched change every time I run "git cl format".

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.cc
(right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.cc:47:
DependsOn(ContentSuggestionsServiceFactory::GetInstance());
On 2016/07/14 12:16:53, Marc Treib wrote:
> Add OfflinePageModelFactory?

Done.

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.cc:78: new
ntp_snippets::OfflinePageSuggestionsProvider(offline_page_model);
On 2016/07/14 12:16:53, Marc Treib wrote:
> "ntp_snippets::" isn't needed; you have a "using" above

Done.

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.h
(right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/offline_page_suggestions_provider_factory.h:5:
#ifndef CHROME_BROWSER_NTP_SNIPPETS_OFFLINE_PAGE_SUGGESTIONS_PROVIDER_FACTORY_H_
On 2016/07/14 12:16:53, Marc Treib wrote:
> Should this be in an offline_pages subfolder, for consistency with the code in
> the component?

Yes. But now it's moving to c/b/a/ntp anyway.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gypi
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
On 2016/07/14 12:16:53, Marc Treib wrote:
> Is this required?

I don't know. It's inspired by this:
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
but I don't know how to run the GYP build to try it out. Someone recently said
that the GYP build wasn't supported for Android anyway?

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:7:
import("//build/config/android/rules.gni")
On 2016/07/14 12:16:53, Marc Treib wrote:
> Required?

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:11:
static_library("offline_page_suggestions") {
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: Should this have the same name as the folder?

It would then be confused more easily with the components/offline_pages one.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:21:
"//components/offline_pages:offline_pages",
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: I think the ":offline_pages" etc isn't required, if it's the same as the
> last path component

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:44:
// TODO(pke) Is this ok?
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: "TODO(pke):" (add colon)

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:45:
return ContentSuggestionsCategoryStatus::AVAILABLE;
On 2016/07/14 12:16:53, Marc Treib wrote:
> Why not "return category_status_;" ?

Done.

I had changed this entire method when I introduced category_status_, but that
must have gone missing during some rebase.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:70:
// Ignore (when offline pages have been loaded, OnOfflinePagesLoaded will be
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: DCHECK that model is the same that we have?

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:82:
// TODO(pke) Implement (does the ofp have to be cleared from UI immediately?)
On 2016/07/14 12:16:53, Marc Treib wrote:
> It should be, yes (the card will link to the offline page directly, which
won't
> exist anymore).
> (Of course, it's fine to leave this as a TODO for now)

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:106:
ContentSuggestion suggestion = ContentSuggestion(
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: Just "ContentSuggestions suggestion(...)"

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h
(right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:17:
#include "components/offline_pages/offline_page_item.h"
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: Not required I think

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:36:
base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
On 2016/07/14 12:16:53, Marc Treib wrote:
> Is this required? It's already defined in ContentSuggestionsProvider.
> (If a "using" is required at all, then at least make it "using
> ImageFetchedCallback = ContentSuggestionsProvider::ImageFetchedCallback")

Done. Also removed from NTPSnippetsService.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:62: //
Queries the OfflinePageModel for offline pages
On 2016/07/14 12:16:53, Marc Treib wrote:
> nit: period after comment

Done.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:71:
ContentSuggestionsProvider::Observer* observer_ = nullptr;
On 2016/07/14 12:16:53, Marc Treib wrote:
> Please let's keep all initialization in one place.

Done.

[Marc Treib, 2016-07-14 14:17:24]

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:7:
import("//build/config/android/rules.gni")
On 2016/07/14 13:46:02, Philipp Keck wrote:
> On 2016/07/14 12:16:53, Marc Treib wrote:
> > Required?
> 
> Done.

Hm, the config one should probably stay. Let's see what the more Android-savvy
folks say.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:11:
static_library("offline_page_suggestions") {
On 2016/07/14 13:46:02, Philipp Keck wrote:
> On 2016/07/14 12:16:53, Marc Treib wrote:
> > nit: Should this have the same name as the folder?
> 
> It would then be confused more easily with the components/offline_pages one.

Acknowledged.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:21:
"//components/offline_pages:offline_pages",
On 2016/07/14 13:46:02, Philipp Keck wrote:
> On 2016/07/14 12:16:53, Marc Treib wrote:
> > nit: I think the ":offline_pages" etc isn't required, if it's the same as
the
> > last path component
> 
> Done.

No, not done actually. There's a few places here what the comment applies :)

https://codereview.chromium.org/2149453004/diff/10001/chrome/browser/android/...
File chrome/browser/android/ntp/offline_page_suggestions_provider_factory.cc
(right):

https://codereview.chromium.org/2149453004/diff/10001/chrome/browser/android/...
chrome/browser/android/ntp/offline_page_suggestions_provider_factory.cc:62:
OfflinePageModel* offline_page_model = nullptr;
This can move down to where it's assigned.

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

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:71: 
nitty nit: also remove the empty line please :)

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/BUILD.gn:20: source_set("unit_tests") {
Do you have any unit tests for this yet? If not, leave this out until you do.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:23:
FetchOfflinePages();
Hm, this should probably happen in SetObserver, to make sure it doesn't return
before the observer exists? Otherwise, we'd lose the update.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:77:
FetchOfflinePages();
DCHECK also here :)

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:100:
category_status_ = ContentSuggestionsCategoryStatus::AVAILABLE;
nit: For readability, I'd prefer having this assignment unconditionally.. maybe
make a SetCategoryStatus helper that updates it and notifies if necessary, and
call that right at the top?

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:109:
std::to_string(item.offline_id)),
This isn't allowed yet, see https://chromium-cpp.appspot.com/
Use base::StringToInt instead.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h
(right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:61: //
Callback from the OfflinePageModel
Here too: period after comment

[Philipp Keck, 2016-07-14 15:30:06]

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:7:
import("//build/config/android/rules.gni")
On 2016/07/14 14:17:23, Marc Treib wrote:
> On 2016/07/14 13:46:02, Philipp Keck wrote:
> > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > Required?
> > 
> > Done.
> 
> Hm, the config one should probably stay. Let's see what the more Android-savvy
> folks say.

Acknowledged.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/BUILD.gn:21:
"//components/offline_pages:offline_pages",
On 2016/07/14 14:17:23, Marc Treib wrote:
> On 2016/07/14 13:46:02, Philipp Keck wrote:
> > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > nit: I think the ":offline_pages" etc isn't required, if it's the same as
> the
> > > last path component
> > 
> > Done.
> 
> No, not done actually. There's a few places here what the comment applies :)

Done. ...

https://codereview.chromium.org/2149453004/diff/10001/chrome/browser/android/...
File chrome/browser/android/ntp/offline_page_suggestions_provider_factory.cc
(right):

https://codereview.chromium.org/2149453004/diff/10001/chrome/browser/android/...
chrome/browser/android/ntp/offline_page_suggestions_provider_factory.cc:62:
OfflinePageModel* offline_page_model = nullptr;
On 2016/07/14 14:17:24, Marc Treib wrote:
> This can move down to where it's assigned.

Done.

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

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:71: 
On 2016/07/14 14:17:24, Marc Treib wrote:
> nitty nit: also remove the empty line please :)

Done.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/BUILD.gn:20: source_set("unit_tests") {
On 2016/07/14 14:17:24, Marc Treib wrote:
> Do you have any unit tests for this yet? If not, leave this out until you do.

Done.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:23:
FetchOfflinePages();
On 2016/07/14 14:17:24, Marc Treib wrote:
> Hm, this should probably happen in SetObserver, to make sure it doesn't return
> before the observer exists? Otherwise, we'd lose the update.

Done. The same goes for the NTPSnippetsService as soon as it doesn't talk to the
UI directly anymore -- left a TODO comment there.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:77:
FetchOfflinePages();
On 2016/07/14 14:17:24, Marc Treib wrote:
> DCHECK also here :)

Done.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:100:
category_status_ = ContentSuggestionsCategoryStatus::AVAILABLE;
On 2016/07/14 14:17:24, Marc Treib wrote:
> nit: For readability, I'd prefer having this assignment unconditionally..
maybe
> make a SetCategoryStatus helper that updates it and notifies if necessary, and
> call that right at the top?

Ok. Maybe that helper method should move to ContentSuggestionsProvider?

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:109:
std::to_string(item.offline_id)),
On 2016/07/14 14:17:24, Marc Treib wrote:
> This isn't allowed yet, see https://chromium-cpp.appspot.com/
> Use base::StringToInt instead.

Right. I'm sorry that things like this (or TODOs without colon) keep reappearing
-- these CLs are older Git branches that I've been working on for a while and I
don't spot everything when I revise them before uploading.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h
(right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:61: //
Callback from the OfflinePageModel
On 2016/07/14 14:17:24, Marc Treib wrote:
> Here too: period after comment

Done.

[Marc Treib, 2016-07-14 16:13:17]

lgtm

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:66: "NTPOfflinePageSuggestions",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/07/14 13:46:02, Philipp Keck wrote:
> On 2016/07/14 12:16:53, Marc Treib wrote:
> > nit: this is formatted differently than all the other entries
> 
> This is "git cl" formatted. Both the seemingly missing space before "{" and
the
> missing newline before "}" are what the formatter does, so they are removed if
> they're there. Something similar would happen to all the others, if I changed
> them.
> 
> Actually I had a similar problem with about_flags.cc, where quite a few lines
> that I never touched change every time I run "git cl format".

Can we stay consistent with how the rest of the entries are formatted?
Consistency over git cl format.
(Or alternatively, re-format everything...)

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gypi
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
On 2016/07/14 13:46:02, Philipp Keck wrote:
> On 2016/07/14 12:16:53, Marc Treib wrote:
> > Is this required?
> 
> I don't know. It's inspired by this:
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> but I don't know how to run the GYP build to try it out. Someone recently said
> that the GYP build wasn't supported for Android anyway?

I'm pretty sure gyp is still supported everywhere, though it won't be for much
longer. You can run gyp with build/gyp_chromium, see e.g.
http://dev.chromium.org/developers/gyp-environment-variables

Anyway, it doesn't really matter. If it builds, then it's fine.

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:100:
category_status_ = ContentSuggestionsCategoryStatus::AVAILABLE;
On 2016/07/14 15:30:06, Philipp Keck wrote:
> On 2016/07/14 14:17:24, Marc Treib wrote:
> > nit: For readability, I'd prefer having this assignment unconditionally..
> maybe
> > make a SetCategoryStatus helper that updates it and notifies if necessary,
and
> > call that right at the top?
> 
> Ok. Maybe that helper method should move to ContentSuggestionsProvider?

Hm, if so, then all the status handling needs to go there... not sure if that's
a good idea. (I'm not opposed, I'm actually not sure :) )

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:109:
std::to_string(item.offline_id)),
On 2016/07/14 15:30:06, Philipp Keck wrote:
> On 2016/07/14 14:17:24, Marc Treib wrote:
> > This isn't allowed yet, see https://chromium-cpp.appspot.com/
> > Use base::StringToInt instead.
> 
> Right. I'm sorry that things like this (or TODOs without colon) keep
reappearing
> -- these CLs are older Git branches that I've been working on for a while and
I
> don't spot everything when I revise them before uploading.

No worries!

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:39:
&kNTPOfflinePageSuggestionsFeature,
These used to be in alphabetical order, let's keep it like that.

[dewittj, 2016-07-14 18:13:29]

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:60:
// TODO(pke): Implement
Nit: Implement.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:64:
// Ignore
Nit: Ignored.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:68:
// Ignore
Ignored.

Or possibly put a TODO(pke): Implement when discarded suggestions are supported.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:75:
// called as a delayed callback, see ctor).
This comment seems out of date, there's nothing in the constructor that makes it
clear that SetObserver will asynchronous fetch the pages after load.

I think this comment could be just deleted.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:81:
FetchOfflinePages();
This is probably the best we can do for now, but seems like a lot of work to do
on every change (this will happen on every single navigation.)

[Bernhard Bauer, 2016-07-14 18:47:44]

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gypi
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
On 2016/07/14 16:13:16, Marc Treib wrote:
> On 2016/07/14 13:46:02, Philipp Keck wrote:
> > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > Is this required?
> > 
> > I don't know. It's inspired by this:
> >
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> > but I don't know how to run the GYP build to try it out. Someone recently
said
> > that the GYP build wasn't supported for Android anyway?
> 
> I'm pretty sure gyp is still supported everywhere, though it won't be for much
> longer. You can run gyp with build/gyp_chromium, see e.g.
> http://dev.chromium.org/developers/gyp-environment-variables
> 
> Anyway, it doesn't really matter. If it builds, then it's fine.

On Android there aren't any bits that still build with GYP, so it's effectively
unsupported. 

Also +1, the '..' include seems very strange, and the '../..' include (which is
the src root, right?) should already be there, I think. Why do we need a
separate target anyway?

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
File chrome/browser/android/ntp/offline_page_suggestions_provider_factory.h
(right):

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
chrome/browser/android/ntp/offline_page_suggestions_provider_factory.h:39: //
BrowserStateKeyedServiceFactory implementation.
Nit: This should be BrowserContextKeyedServiceFactory.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/BUILD.gn:6:
static_library("offline_page_suggestions") {
This should probably be a source_set.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:40:
if (observer) {
Nit: no braces for single-line bodies.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:73:
DCHECK_EQ(model, offline_page_model_);
Expected value goes first, for nicer error messages in case of failure.

[Philipp Keck, 2016-07-15 09:08:42]

Thank you Bernhard, Justin and Marc! Corrected almost all of the things you
commented on. Still one open question about the gyp file @bauerb.

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2149453004/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:66: "NTPOfflinePageSuggestions",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/07/14 16:13:16, Marc Treib wrote:
> On 2016/07/14 13:46:02, Philipp Keck wrote:
> > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > nit: this is formatted differently than all the other entries
> > 
> > This is "git cl" formatted. Both the seemingly missing space before "{" and
> the
> > missing newline before "}" are what the formatter does, so they are removed
if
> > they're there. Something similar would happen to all the others, if I
changed
> > them.
> > 
> > Actually I had a similar problem with about_flags.cc, where quite a few
lines
> > that I never touched change every time I run "git cl format".
> 
> Can we stay consistent with how the rest of the entries are formatted?
> Consistency over git cl format.
> (Or alternatively, re-format everything...)

Ok. For about_flags.cc, I don't want to reformat the whole file, so I will keep
undoing the auto-formatter when working on this CL, even though that's annoying.

For this file, I adjust everything the way the formatter does it.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gypi
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
On 2016/07/14 16:13:16, Marc Treib wrote:
> On 2016/07/14 13:46:02, Philipp Keck wrote:
> > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > Is this required?
> > 
> > I don't know. It's inspired by this:
> >
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> > but I don't know how to run the GYP build to try it out. Someone recently
said
> > that the GYP build wasn't supported for Android anyway?
> 
> I'm pretty sure gyp is still supported everywhere, though it won't be for much
> longer. You can run gyp with build/gyp_chromium, see e.g.
> http://dev.chromium.org/developers/gyp-environment-variables
> 
> Anyway, it doesn't really matter. If it builds, then it's fine.

Acknowledged.

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
On 2016/07/14 18:47:43, Bernhard Bauer wrote:
> On 2016/07/14 16:13:16, Marc Treib wrote:
> > On 2016/07/14 13:46:02, Philipp Keck wrote:
> > > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > > Is this required?
> > > 
> > > I don't know. It's inspired by this:
> > >
> >
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> > > but I don't know how to run the GYP build to try it out. Someone recently
> said
> > > that the GYP build wasn't supported for Android anyway?
> > 
> > I'm pretty sure gyp is still supported everywhere, though it won't be for
much
> > longer. You can run gyp with build/gyp_chromium, see e.g.
> > http://dev.chromium.org/developers/gyp-environment-variables
> > 
> > Anyway, it doesn't really matter. If it builds, then it's fine.
> 
> On Android there aren't any bits that still build with GYP, so it's
effectively
> unsupported. 
> 
> Also +1, the '..' include seems very strange, and the '../..' include (which
is
> the src root, right?) should already be there, I think. Why do we need a
> separate target anyway?

I'm not an expert on GYP build files. The reason why this is here is this:
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...

So you're saying I can remove the entire include_dirs, if not the entire target
here (and then include the files in the above target instead)?

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/10001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:100:
category_status_ = ContentSuggestionsCategoryStatus::AVAILABLE;
On 2016/07/14 16:13:16, Marc Treib wrote:
> On 2016/07/14 15:30:06, Philipp Keck wrote:
> > On 2016/07/14 14:17:24, Marc Treib wrote:
> > > nit: For readability, I'd prefer having this assignment unconditionally..
> > maybe
> > > make a SetCategoryStatus helper that updates it and notifies if necessary,
> and
> > > call that right at the top?
> > 
> > Ok. Maybe that helper method should move to ContentSuggestionsProvider?
> 
> Hm, if so, then all the status handling needs to go there... not sure if
that's
> a good idea. (I'm not opposed, I'm actually not sure :) )

Having all the status handling there would be difficult because a provider can
have multiple categories. So rather leave it here...

Created the helper method.

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:39:
&kNTPOfflinePageSuggestionsFeature,
On 2016/07/14 16:13:17, Marc Treib wrote:
> These used to be in alphabetical order, let's keep it like that.

Done.

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
File chrome/browser/android/ntp/offline_page_suggestions_provider_factory.h
(right):

https://codereview.chromium.org/2149453004/diff/50001/chrome/browser/android/...
chrome/browser/android/ntp/offline_page_suggestions_provider_factory.h:39: //
BrowserStateKeyedServiceFactory implementation.
On 2016/07/14 18:47:43, Bernhard Bauer wrote:
> Nit: This should be BrowserContextKeyedServiceFactory.

Done. Changed this in a couple of other places, too.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/BUILD.gn:6:
static_library("offline_page_suggestions") {
On 2016/07/14 18:47:43, Bernhard Bauer wrote:
> This should probably be a source_set.

Done.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:40:
if (observer) {
On 2016/07/14 18:47:43, Bernhard Bauer wrote:
> Nit: no braces for single-line bodies.

Done.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:60:
// TODO(pke): Implement
On 2016/07/14 18:13:28, dewittj wrote:
> Nit: Implement.

Done.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:64:
// Ignore
On 2016/07/14 18:13:29, dewittj wrote:
> Nit: Ignored.

Done.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:68:
// Ignore
On 2016/07/14 18:13:28, dewittj wrote:
> Ignored.
> 
> Or possibly put a TODO(pke): Implement when discarded suggestions are
supported.

Done.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:73:
DCHECK_EQ(model, offline_page_model_);
On 2016/07/14 18:47:44, Bernhard Bauer wrote:
> Expected value goes first, for nicer error messages in case of failure.

I know. Depends on whether you expect "ours" to be "theirs" or "theirs" to be
"ours". So you could ask "Is the passed parameter valid?" (but that's
unnecessary since we never use it anyway) or you could ask "Is our stored model
still up to date?".

I swapped it now.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:75:
// called as a delayed callback, see ctor).
On 2016/07/14 18:13:28, dewittj wrote:
> This comment seems out of date, there's nothing in the constructor that makes
it
> clear that SetObserver will asynchronous fetch the pages after load.
> 
> I think this comment could be just deleted.

Done.

https://codereview.chromium.org/2149453004/diff/50001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:81:
FetchOfflinePages();
On 2016/07/14 18:13:28, dewittj wrote:
> This is probably the best we can do for now, but seems like a lot of work to
do
> on every change (this will happen on every single navigation.)

The OfflinePageModel currently does not have a OfflinePageAdded event. Actually,
we are discussing these fine-grained events (added, changed, removed) instead of
the overall "something changed, reload everything" because the UI might want
such fine-grained updates for nicer animations, etc. So far, we haven't
considered the performance aspect of this.

[Philipp Keck, 2016-07-15 11:29:27]

pke@google.com changed reviewers:
+ thakis@chromium.org

[Philipp Keck, 2016-07-15 11:29:28]

thakis@chromium.org: Please review changes in 
chrome/browser/profiles/profile_manager.cc
and
chrome/browser/BUILD.gn

Thank you!

[Nico, 2016-07-15 17:47:21]

lgtm

[Philipp Keck, 2016-07-18 10:21:14]

dewittj@chromium.org: Please review changes in 
components/offline_pages/background/BUILD.gn
(need your approval for this file)

[Bernhard Bauer, 2016-07-18 11:09:33]

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gypi
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
On 2016/07/15 09:08:41, Philipp Keck wrote:
> On 2016/07/14 18:47:43, Bernhard Bauer wrote:
> > On 2016/07/14 16:13:16, Marc Treib wrote:
> > > On 2016/07/14 13:46:02, Philipp Keck wrote:
> > > > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > > > Is this required?
> > > > 
> > > > I don't know. It's inspired by this:
> > > >
> > >
> >
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> > > > but I don't know how to run the GYP build to try it out. Someone
recently
> > said
> > > > that the GYP build wasn't supported for Android anyway?
> > > 
> > > I'm pretty sure gyp is still supported everywhere, though it won't be for
> much
> > > longer. You can run gyp with build/gyp_chromium, see e.g.
> > > http://dev.chromium.org/developers/gyp-environment-variables
> > > 
> > > Anyway, it doesn't really matter. If it builds, then it's fine.
> > 
> > On Android there aren't any bits that still build with GYP, so it's
> effectively
> > unsupported. 
> > 
> > Also +1, the '..' include seems very strange, and the '../..' include (which
> is
> > the src root, right?) should already be there, I think. Why do we need a
> > separate target anyway?
> 
> I'm not an expert on GYP build files. The reason why this is here is this:
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> 
> So you're saying I can remove the entire include_dirs, if not the entire
target
> here (and then include the files in the above target instead)?

Yup. offline_pages_background_offliner is kind of a weird case anyway, as it
only seems to be a dependency of a test target...

https://codereview.chromium.org/2149453004/diff/70001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/70001/components/ntp_snippets...
components/ntp_snippets/offline_pages/BUILD.gn:6:
source_set("offline_page_suggestions") {
Analogously to the GYP part, we could probably merge this into the main target
as well.

[Philipp Keck, 2016-07-18 11:50:50]

Description was changed from

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=628198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=628198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[Philipp Keck, 2016-07-18 12:55:03]

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gypi
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/2149453004/diff/1/components/ntp_snippets.gyp...
components/ntp_snippets.gypi:71: '../..',
On 2016/07/18 11:09:32, Bernhard Bauer wrote:
> On 2016/07/15 09:08:41, Philipp Keck wrote:
> > On 2016/07/14 18:47:43, Bernhard Bauer wrote:
> > > On 2016/07/14 16:13:16, Marc Treib wrote:
> > > > On 2016/07/14 13:46:02, Philipp Keck wrote:
> > > > > On 2016/07/14 12:16:53, Marc Treib wrote:
> > > > > > Is this required?
> > > > > 
> > > > > I don't know. It's inspired by this:
> > > > >
> > > >
> > >
> >
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> > > > > but I don't know how to run the GYP build to try it out. Someone
> recently
> > > said
> > > > > that the GYP build wasn't supported for Android anyway?
> > > > 
> > > > I'm pretty sure gyp is still supported everywhere, though it won't be
for
> > much
> > > > longer. You can run gyp with build/gyp_chromium, see e.g.
> > > > http://dev.chromium.org/developers/gyp-environment-variables
> > > > 
> > > > Anyway, it doesn't really matter. If it builds, then it's fine.
> > > 
> > > On Android there aren't any bits that still build with GYP, so it's
> > effectively
> > > unsupported. 
> > > 
> > > Also +1, the '..' include seems very strange, and the '../..' include
(which
> > is
> > > the src root, right?) should already be there, I think. Why do we need a
> > > separate target anyway?
> > 
> > I'm not an expert on GYP build files. The reason why this is here is this:
> >
>
https://codesearch.chromium.org/chromium/src/components/offline_pages.gypi?q=...
> > 
> > So you're saying I can remove the entire include_dirs, if not the entire
> target
> > here (and then include the files in the above target instead)?
> 
> Yup. offline_pages_background_offliner is kind of a weird case anyway, as it
> only seems to be a dependency of a test target...

Done.

https://codereview.chromium.org/2149453004/diff/70001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2149453004/diff/70001/components/ntp_snippets...
components/ntp_snippets/offline_pages/BUILD.gn:6:
source_set("offline_page_suggestions") {
On 2016/07/18 11:09:33, Bernhard Bauer wrote:
> Analogously to the GYP part, we could probably merge this into the main target
> as well.

Done.

[dewittj, 2016-07-18 14:52:16]

lgtm

[Philipp Keck, 2016-07-18 14:54:33]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-07-18 14:54:33]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2149453004/#ps90001
(title: "Remove separate build target for offline_page_suggestions")

[commit-bot: I haz the power, 2016-07-18 14:55:03]

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, 2016-07-18 14:55:06]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-18 17:40:34]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-07-18 17:40:51]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 17:42:12]

Description was changed from

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=628198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Implement first version of OfflinePageSuggestionsProvider

Implement a ContentSuggestionsProvider that reads from the
OfflinePageModel, converts to ContentSuggestions and provides those to
the ContentSuggestionsService. Add a chrome-flag to enable the provider
and implement a factory that registers the provider with the service.

The implementation is not usable by users, yet, because extraction of
title, snippet text and image is not implemented. Those fields are
filled with dummy data or left empty. Deduplication, sorting and
discarding are also not implemented.
But the OfflinePageSuggestionsProvider serves as an example for other
ContentSuggestionsProvider implementations and it can be used to test
multiple-section UI implementations.

BUG=628198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5044dec1802c333acca98268df660ccee3ddbd8d
Cr-Commit-Position: refs/heads/master@{#406028}
==========

[commit-bot: I haz the power, 2016-07-18 17:42:14]

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