Split OfflinePageSuggestions into two categories

Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

Add more-button actions for Recent Tabs and Downloads to open the
respective native UIs.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c4fe8dd8b922142e6c3499369007688fd087bfcb
Cr-Commit-Position: refs/heads/master@{#411645}

[Philipp Keck, 2016-08-12 07:36:12]

Description was changed from

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

BUG=628198
==========

to

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

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

[Philipp Keck, 2016-08-12 07:38:55]

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

[Philipp Keck, 2016-08-12 07:38:55]

PTAL

[Philipp Keck, 2016-08-12 07:41:51]

Description was changed from

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

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

to

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2016-08-12 09:21:14]

https://codereview.chromium.org/2245583002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2245583002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15142: +        Show recent tabs on the New
Tab page
Should "offline" be somewhere in this string?

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:99: }
else if (category == downloads_category_) {
nit: No "else" after "return"

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:113:
} else if (category == downloads_category_) {
Same here

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:129:
DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
This is enforced in the ContentSuggestionsService?

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:138:
}
else NOTREACHED?

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:161:
std::set<std::string>* dismissed_ids = nullptr;
const?

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:169:
return suggestions;
NOTREACHED?

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:225:
bool needRecentTabs = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
need_recent_tabs, need_downloads

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:226:
bool needDownloads = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
downloads_status_

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:228:
NotifyStatusChanged(recent_tabs_category_, CategoryStatus::AVAILABLE);
Braces for single=line bodies or not? Either is fine with me, but please be
consistent at least within each file.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:303:
std::vector<const OfflinePageItem*>* offline_page_items) const {
Hrm, I'm not a fan of this kind of mutable input argument. Maybe pass it in by
value and use std::move at the call site? Or alternatively, split this method in
two: One that only does the sorting, and one that does the rest (and gets a
const ref).

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:92:
std::set<std::string> ReadDismissedIDsFromPrefs(const std::string& pref_name);
const?

[Philipp Keck, 2016-08-12 09:56:40]

Thank you

https://codereview.chromium.org/2245583002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2245583002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15142: +        Show recent tabs on the New
Tab page
On 2016/08/12 09:21:14, Marc Treib wrote:
> Should "offline" be somewhere in this string?

Ok, I guess since it's still more "internal" and we want to avoid confusion with
the other "recent tabs", it makes sense here.
Also adjusted the description for the message.

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:99: }
else if (category == downloads_category_) {
On 2016/08/12 09:21:14, Marc Treib wrote:
> nit: No "else" after "return"

Done. Don't really like this rule. Now the shape of this method is different
from the others (the voids), only because of its return value, but even though
they all just do different things depending on the category.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:113:
} else if (category == downloads_category_) {
On 2016/08/12 09:21:14, Marc Treib wrote:
> Same here

Done.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:129:
DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
On 2016/08/12 09:21:14, Marc Treib wrote:
> This is enforced in the ContentSuggestionsService?

Yes, but not only:
Firstly, if the status is NOT_PROVIDED, the mapping "category -> provider" is
deleted, so if a dismissed-call comes back, it doesn't know which provider to
call and just discards it.

In addition, the recent_tabs_category_ field can only ever have this value if it
was already set at startup (because it's a flag). So it's either NOT_PROVIDED
all the time, or never.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:138:
}
On 2016/08/12 09:21:14, Marc Treib wrote:
> else NOTREACHED?

Done.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:161:
std::set<std::string>* dismissed_ids = nullptr;
On 2016/08/12 09:21:14, Marc Treib wrote:
> const?

Done.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:169:
return suggestions;
On 2016/08/12 09:21:14, Marc Treib wrote:
> NOTREACHED?

Done.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:225:
bool needRecentTabs = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
On 2016/08/12 09:21:14, Marc Treib wrote:
> need_recent_tabs, need_downloads

Done.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:226:
bool needDownloads = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
On 2016/08/12 09:21:14, Marc Treib wrote:
> downloads_status_

Done. Good catch. The whole code duplication isn't an awesome solution ...

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:228:
NotifyStatusChanged(recent_tabs_category_, CategoryStatus::AVAILABLE);
On 2016/08/12 09:21:14, Marc Treib wrote:
> Braces for single=line bodies or not? Either is fine with me, but please be
> consistent at least within each file.

This file is supposed to be without. Changed two instances above.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:303:
std::vector<const OfflinePageItem*>* offline_page_items) const {
On 2016/08/12 09:21:14, Marc Treib wrote:
> Hrm, I'm not a fan of this kind of mutable input argument. Maybe pass it in by
> value and use std::move at the call site? Or alternatively, split this method
in
> two: One that only does the sorting, and one that does the rest (and gets a
> const ref).

Done.

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:92:
std::set<std::string> ReadDismissedIDsFromPrefs(const std::string& pref_name);
On 2016/08/12 09:21:14, Marc Treib wrote:
> const?

Done.

[Philipp Keck, 2016-08-12 09:57:45]

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

[Philipp Keck, 2016-08-12 09:57:45]

bauerb@chromium.org: Please review changes in 
chrome/browser/resources/snippets_internals.html
chrome/browser/ui/webui/snippets_internals_message_handler.cc

[Marc Treib, 2016-08-12 10:12:53]

lgtm

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:99: }
else if (category == downloads_category_) {
On 2016/08/12 09:56:40, Philipp Keck wrote:
> On 2016/08/12 09:21:14, Marc Treib wrote:
> > nit: No "else" after "return"
> 
> Done. Don't really like this rule. Now the shape of this method is different
> from the others (the voids), only because of its return value, but even though
> they all just do different things depending on the category.

Well, the rule is the rule... in general, I do like the rule (reduces
indentation), though I do see how it's a bit annoying in this instance. Oh well.

If you want things to be more consistent, you could add returns in the other
methods.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:129:
DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
On 2016/08/12 09:56:40, Philipp Keck wrote:
> On 2016/08/12 09:21:14, Marc Treib wrote:
> > This is enforced in the ContentSuggestionsService?
> 
> Yes, but not only:
> Firstly, if the status is NOT_PROVIDED, the mapping "category -> provider" is
> deleted, so if a dismissed-call comes back, it doesn't know which provider to
> call and just discards it.
> 
> In addition, the recent_tabs_category_ field can only ever have this value if
it
> was already set at startup (because it's a flag). So it's either NOT_PROVIDED
> all the time, or never.

Acknowledged.

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:226:
bool needDownloads = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
On 2016/08/12 09:56:40, Philipp Keck wrote:
> On 2016/08/12 09:21:14, Marc Treib wrote:
> > downloads_status_
> 
> Done. Good catch. The whole code duplication isn't an awesome solution ...

The alternative would be to have getters like GetStatus(category), that might
save at least some of the duplication. Though things will get kinda messy if
there are any differences between the sections (like the removing duplicates
thing). Your call :)

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

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:38: }
OrderByMostRecentlyVisited;
This instantiates one global instance of the (unnamed) comparator struct. Is
that on purpose?
(I don't think there's anything wrong with this as such, but it is uncommon...)

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

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:98:
const std::set<std::string>& dismissed_ids) const;
nit: I wouldn't make this one const, since it fundamentally does change
something, even if it's not strictly in this class.

[Philipp Keck, 2016-08-12 10:22:55]

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:226:
bool needDownloads = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
On 2016/08/12 10:12:53, Marc Treib wrote:
> On 2016/08/12 09:56:40, Philipp Keck wrote:
> > On 2016/08/12 09:21:14, Marc Treib wrote:
> > > downloads_status_
> > 
> > Done. Good catch. The whole code duplication isn't an awesome solution ...
> 
> The alternative would be to have getters like GetStatus(category), that might
> save at least some of the duplication. Though things will get kinda messy if
> there are any differences between the sections (like the removing duplicates
> thing). Your call :)

I'd refactor only after the provider has grown or changed a bit more, then it's
probably easier whether code duplication is a bad thing or necessary.

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

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:38: }
OrderByMostRecentlyVisited;
On 2016/08/12 10:12:53, Marc Treib wrote:
> This instantiates one global instance of the (unnamed) comparator struct. Is
> that on purpose?
> (I don't think there's anything wrong with this as such, but it is
uncommon...)

This is from example code of std::sort. Otherwise, we'd have to create a new
instance every time, right? So even if it's uncommon, it seems like a reasonable
choice here. Creating an instance of this doesn't really have a "meaning", it's
just a dummy.

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

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:98:
const std::set<std::string>& dismissed_ids) const;
On 2016/08/12 10:12:53, Marc Treib wrote:
> nit: I wouldn't make this one const, since it fundamentally does change
> something, even if it's not strictly in this class.

Done.

[Bernhard Bauer, 2016-08-12 10:28:31]

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:99: }
else if (category == downloads_category_) {
On 2016/08/12 10:12:53, Marc Treib wrote:
> On 2016/08/12 09:56:40, Philipp Keck wrote:
> > On 2016/08/12 09:21:14, Marc Treib wrote:
> > > nit: No "else" after "return"
> > 
> > Done. Don't really like this rule. Now the shape of this method is different
> > from the others (the voids), only because of its return value, but even
though
> > they all just do different things depending on the category.
> 
> Well, the rule is the rule... in general, I do like the rule (reduces
> indentation), though I do see how it's a bit annoying in this instance. Oh
well.
> 
> If you want things to be more consistent, you could add returns in the other
> methods.

Alternatively, use a switch statement?

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

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:38: }
OrderByMostRecentlyVisited;
On 2016/08/12 10:12:53, Marc Treib wrote:
> This instantiates one global instance of the (unnamed) comparator struct. Is
> that on purpose?
> (I don't think there's anything wrong with this as such, but it is
uncommon...)

Is that going to create a static initializer? If so, it's disallowed.

(It would probably be fine to name the struct OrderByMostRecentlyVisited and
construct an instance when needed for sorting.)

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:125:
DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
The (not 😉) expected value should go first for nicer error messages in case of
failure.

[Philipp Keck, 2016-08-12 11:02:26]

Please take a look at the changes in patch set 4. Currently, the offline pages
model uses the wrong namespace for downloads, so I adjusted that. The NTP will
now show exactly the downloads that appear in the Downloads Home in the section
"Pages" (not other files).

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

https://codereview.chromium.org/2245583002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:99: }
else if (category == downloads_category_) {
On 2016/08/12 10:28:31, Bernhard Bauer wrote:
> On 2016/08/12 10:12:53, Marc Treib wrote:
> > On 2016/08/12 09:56:40, Philipp Keck wrote:
> > > On 2016/08/12 09:21:14, Marc Treib wrote:
> > > > nit: No "else" after "return"
> > > 
> > > Done. Don't really like this rule. Now the shape of this method is
different
> > > from the others (the voids), only because of its return value, but even
> though
> > > they all just do different things depending on the category.
> > 
> > Well, the rule is the rule... in general, I do like the rule (reduces
> > indentation), though I do see how it's a bit annoying in this instance. Oh
> well.
> > 
> > If you want things to be more consistent, you could add returns in the other
> > methods.
> 
> Alternatively, use a switch statement?

Yeah, but that wouldn't get rid of the code duplicates. This method is okay, but
look at DismissSuggestion, for example. The whole "DCHECK_NE", "insert", "Store"
logic is there twice. On the other hand, Marc has a point: It might actually be
that we need different logic for those in the future.

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

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:38: }
OrderByMostRecentlyVisited;
On 2016/08/12 10:28:31, Bernhard Bauer wrote:
> On 2016/08/12 10:12:53, Marc Treib wrote:
> > This instantiates one global instance of the (unnamed) comparator struct. Is
> > that on purpose?
> > (I don't think there's anything wrong with this as such, but it is
> uncommon...)
> 
> Is that going to create a static initializer? If so, it's disallowed.
> 
> (It would probably be fine to name the struct OrderByMostRecentlyVisited and
> construct an instance when needed for sorting.)

Done.

https://codereview.chromium.org/2245583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:125:
DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
On 2016/08/12 10:28:31, Bernhard Bauer wrote:
> The (not 😉) expected value should go first for nicer error messages in case of
> failure.

Done.

[Philipp Keck, 2016-08-12 11:08:55]

Description was changed from

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Philipp Keck, 2016-08-12 11:09:57]

Made another small change (patch set 6) to read the new title field that the
OfflinePageModel now has. Since it's not always filled, the code still uses the
URL as a fallback.
Updated the issue description accordingly.

[Marc Treib, 2016-08-12 11:11:09]

On 2016/08/12 11:09:57, Philipp Keck wrote:
> Made another small change (patch set 6) to read the new title field that the
> OfflinePageModel now has. Since it's not always filled, the code still uses
the
> URL as a fallback.
> Updated the issue description accordingly.

Still lgtm

[Bernhard Bauer, 2016-08-12 11:31:28]

lgtm

[Philipp Keck, 2016-08-12 13:02:49]

Marc, Bernhard, PTAL at Patch Set 8.
After rebasing on the more-button CL (patch set 7), the more action was missing.
Since it's now available, I could simply integrate it.

However, it needs a metric. If it's okay for Bernhard (Marc already approved),
I'd land Patch Set 9 (coming soon) without the metric and put that on a separate
CL right after, for review with the OWNERS there (and with a proper owner and
description in the actions.xml).

[Marc Treib, 2016-08-12 13:05:43]

https://codereview.chromium.org/2245583002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2245583002/diff/140001/components/ntp_snippet...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:112:
/* has_more_button */ true);
Where would this "more" button lead - to the old "Recent Tabs" UI? Is that the
right thing?

[Bernhard Bauer, 2016-08-12 13:06:02]

On 2016/08/12 13:02:49, Philipp Keck wrote:
> Marc, Bernhard, PTAL at Patch Set 8.
> After rebasing on the more-button CL (patch set 7), the more action was
missing.
> Since it's now available, I could simply integrate it.
> 
> However, it needs a metric. If it's okay for Bernhard (Marc already approved),
> I'd land Patch Set 9 (coming soon) without the metric and put that on a
separate
> CL right after, for review with the OWNERS there (and with a proper owner and
> description in the actions.xml).

Landing without the metric is fine with me.

[Philipp Keck, 2016-08-12 13:06:06]

Description was changed from

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

Add more-button actions for Recent Tabs and Downloads to open the
respective native UIs.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Philipp Keck, 2016-08-12 13:09:44]

https://codereview.chromium.org/2245583002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2245583002/diff/140001/components/ntp_snippet...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:112:
/* has_more_button */ true);
On 2016/08/12 13:05:43, Marc Treib wrote:
> Where would this "more" button lead - to the old "Recent Tabs" UI? Is that the
> right thing?

You're right, that's not the right thing. I removed that one again, it won't
have a more-button. Confusingly, all the methods for that were already in place
(NewTabPage#nativeToRecentTabs()).

[Marc Treib, 2016-08-12 13:13:36]

LGTM (my third on this CL - land this already!! ;) )

https://codereview.chromium.org/2245583002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2245583002/diff/140001/components/ntp_snippet...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:112:
/* has_more_button */ true);
On 2016/08/12 13:09:44, Philipp Keck wrote:
> On 2016/08/12 13:05:43, Marc Treib wrote:
> > Where would this "more" button lead - to the old "Recent Tabs" UI? Is that
the
> > right thing?
> 
> You're right, that's not the right thing. I removed that one again, it won't
> have a more-button. Confusingly, all the methods for that were already in
place
> (NewTabPage#nativeToRecentTabs()).

Yes, for the old bottom toolbar that we're replacing. This is part of the reason
I'm always complaining about adding "Offline" somewhere in our "Recent Tabs"
strings :)

[Philipp Keck, 2016-08-12 13:14:59]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-12 13:15:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2245583002/#ps180001
(title: "Remove more-button for Recent Tabs")

[commit-bot: I haz the power, 2016-08-12 13:15:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Philipp Keck, 2016-08-12 13:16:39]

On 2016/08/12 13:13:36, Marc Treib wrote:
> LGTM (my third on this CL - land this already!! ;) )

:D Sorry for being slow with the landing. It helps getting rid of
dependencies/conflicts and I have to wait for the CQ less often if I just append
little changes to an existing Cl..

[commit-bot: I haz the power, 2016-08-12 14:34:55]

Description was changed from

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

Add more-button actions for Recent Tabs and Downloads to open the
respective native UIs.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

Add more-button actions for Recent Tabs and Downloads to open the
respective native UIs.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-12 14:34:56]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-12 14:38:57]

Description was changed from

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

Add more-button actions for Recent Tabs and Downloads to open the
respective native UIs.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Split OfflinePageSuggestions into two categories

Add separate flags for enabling the recent tabs and downloads sections.
Update flags shown on snippets-internals page. Modify the
OfflinePageSuggestionsProvider to handle the two categories, especially
to split the OfflinePageItems coming from the OfflinePageModel into two
distinct lists ordered by the last accessed date.

Use the title from the OfflinePageItem, if available; use the URL as
a fallback otherwise.

Add more-button actions for Recent Tabs and Downloads to open the
respective native UIs.

TBR=cpu@chromium.org (for generated_resources.grd)
BUG=628198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c4fe8dd8b922142e6c3499369007688fd087bfcb
Cr-Commit-Position: refs/heads/master@{#411645}
==========

[commit-bot: I haz the power, 2016-08-12 14:38:58]

Patchset 10 (id:??) landed as
https://crrev.com/c4fe8dd8b922142e6c3499369007688fd087bfcb
Cr-Commit-Position: refs/heads/master@{#411645}