Refactor OfflinePageSuggestionsProvider dismissed ID handling

Refactor OfflinePageSuggestionsProvider dismissed ID handling

Remove the members storing the IDs; access prefs directly instead.
Change the read and store methods to take the category as a parameter
to simplify the calling code.
Move suggestion invalidation out of OfflinePageDeleted into a
dedicated helper method to avoid code duplication.

BUG=628198
Committed: https://crrev.com/0ad1a7e948c22a60d3f640205c2da2efffcd6113
Cr-Commit-Position: refs/heads/master@{#412823}

[Philipp Keck, 2016-08-16 11:16:28]

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

[Philipp Keck, 2016-08-16 11:16:28]

Your suggestion not to use the member vectors for dismissed_ids has greatly
simplified the code. PTAL

[Philipp Keck, 2016-08-16 11:17:03]

Description was changed from

==========
Refactor OfflinePageSuggestionsProvider dismissed ID handling

Remove the members storing the IDs; access prefs directly instead.
Change the read and store methods to take the category as a parameter
to simplify the calling code.

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

to

==========
Refactor OfflinePageSuggestionsProvider dismissed ID handling

Remove the members storing the IDs; access prefs directly instead.
Change the read and store methods to take the category as a parameter
to simplify the calling code.
Move suggestion invalidation out of OfflinePageDeleted into a
dedicated helper method to avoid code duplication.

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

[Philipp Keck, 2016-08-16 11:42:35]

Please also note patch sets 2 and 3.

[Marc Treib, 2016-08-16 11:45:17]

lgtm

Nice!

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

https://codereview.chromium.org/2251743002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:324:
} else if (category == downloads_category_) {
no else after return

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

https://codereview.chromium.org/2251743002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:94: //
doesn't exist anymore), otherwise the |OnSuggestionInvalidated| event is
"otherwise", really? Shouldn't it be fired in any case?

[Marc Treib, 2016-08-16 11:48:37]

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

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:219:
if (dismissed_recent_tab_ids.count(offline_page_id)) {
nit: no braces required

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:220:
cleaned_recent_tab_ids.insert(offline_page_id);
Hm. This depends on us getting *all* offline pages here, which is true for now,
but will probably change soon. Maybe at least worth a comment?

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:232:
if (dismissed_download_ids.count(offline_page_id)) {
also here, no braces required

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:249:
if (cleaned_recent_tab_ids.size() != dismissed_recent_tab_ids.size()) {
and here

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:257:
if (cleaned_download_ids.size() != dismissed_download_ids.size()) {
and here :)

[Philipp Keck, 2016-08-16 12:04:02]

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

https://codereview.chromium.org/2251743002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:324:
} else if (category == downloads_category_) {
On 2016/08/16 11:45:17, Marc Treib wrote:
> no else after return

Done.

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

https://codereview.chromium.org/2251743002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:94: //
doesn't exist anymore), otherwise the |OnSuggestionInvalidated| event is
On 2016/08/16 11:45:17, Marc Treib wrote:
> "otherwise", really? Shouldn't it be fired in any case?

Done.

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

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:219:
if (dismissed_recent_tab_ids.count(offline_page_id)) {
On 2016/08/16 11:48:37, Marc Treib wrote:
> nit: no braces required

Done.

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:220:
cleaned_recent_tab_ids.insert(offline_page_id);
On 2016/08/16 11:48:37, Marc Treib wrote:
> Hm. This depends on us getting *all* offline pages here, which is true for
now,
> but will probably change soon. Maybe at least worth a comment?

Added comment in FetchOfflinePages, so that Justin's CL gets a merge conflict.

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:232:
if (dismissed_download_ids.count(offline_page_id)) {
On 2016/08/16 11:48:37, Marc Treib wrote:
> also here, no braces required

Done.

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:249:
if (cleaned_recent_tab_ids.size() != dismissed_recent_tab_ids.size()) {
On 2016/08/16 11:48:37, Marc Treib wrote:
> and here

Done.

https://codereview.chromium.org/2251743002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:257:
if (cleaned_download_ids.size() != dismissed_download_ids.size()) {
On 2016/08/16 11:48:37, Marc Treib wrote:
> and here :)

Done.

[Philipp Keck, 2016-08-16 12:04:22]

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

[Philipp Keck, 2016-08-16 12:04:23]

bauerb@chromium.org: Please review changes in 
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java

[Bernhard Bauer, 2016-08-16 16:49:53]

https://codereview.chromium.org/2251743002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2251743002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:31:
* immediately. This event may be fired for a category or suggestion that does
not
Couldn't you fold this into https://codereview.chromium.org/2244793002/#ps80001?

[Philipp Keck, 2016-08-18 08:28:02]

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

[Philipp Keck, 2016-08-18 08:28:03]

https://codereview.chromium.org/2251743002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2251743002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:31:
* immediately. This event may be fired for a category or suggestion that does
not
On 2016/08/16 16:49:53, Bernhard Bauer wrote:
> Couldn't you fold this into
https://codereview.chromium.org/2244793002/#ps80001?

Done, therefore removing you as a reviewer for this CL.

[Philipp Keck, 2016-08-18 13:36:18]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-18 13:36:19]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2251743002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-18 13:36:45]

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-08-18 14:49:43]

Description was changed from

==========
Refactor OfflinePageSuggestionsProvider dismissed ID handling

Remove the members storing the IDs; access prefs directly instead.
Change the read and store methods to take the category as a parameter
to simplify the calling code.
Move suggestion invalidation out of OfflinePageDeleted into a
dedicated helper method to avoid code duplication.

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

to

==========
Refactor OfflinePageSuggestionsProvider dismissed ID handling

Remove the members storing the IDs; access prefs directly instead.
Change the read and store methods to take the category as a parameter
to simplify the calling code.
Move suggestion invalidation out of OfflinePageDeleted into a
dedicated helper method to avoid code duplication.

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

[commit-bot: I haz the power, 2016-08-18 14:49:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-18 14:51:16]

Description was changed from

==========
Refactor OfflinePageSuggestionsProvider dismissed ID handling

Remove the members storing the IDs; access prefs directly instead.
Change the read and store methods to take the category as a parameter
to simplify the calling code.
Move suggestion invalidation out of OfflinePageDeleted into a
dedicated helper method to avoid code duplication.

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

to

==========
Refactor OfflinePageSuggestionsProvider dismissed ID handling

Remove the members storing the IDs; access prefs directly instead.
Change the read and store methods to take the category as a parameter
to simplify the calling code.
Move suggestion invalidation out of OfflinePageDeleted into a
dedicated helper method to avoid code duplication.

BUG=628198

Committed: https://crrev.com/0ad1a7e948c22a60d3f640205c2da2efffcd6113
Cr-Commit-Position: refs/heads/master@{#412823}
==========

[commit-bot: I haz the power, 2016-08-18 14:51:17]

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