Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=565575
Committed: https://crrev.com/011a526e4b9923dd648df7b2d6914ea7e6f109fe
Cr-Commit-Position: refs/heads/master@{#437114}

[dewittj, 2016-11-01 21:37:05]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 21:37:53]

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

[dewittj, 2016-11-01 21:40:57]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 21:41:39]

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

[dewittj, 2016-11-01 21:53:45]

Description was changed from

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=
==========

to

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=
==========

[dewittj, 2016-11-01 21:53:45]

dewittj@chromium.org changed reviewers:
+ dimich@chromium.org

[commit-bot: I haz the power, 2016-11-01 22:47:55]

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

[commit-bot: I haz the power, 2016-11-01 22:47:56]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2016-11-02 07:15:37]

https://codereview.chromium.org/2469933002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/2469933002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:293: //
TODO(dewittj): Change the Java side to use OfflinePageAdded.
Please make it next CL.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:54: virtual void
OfflinePageModelLoaded(OfflinePageModel* model);
What prompted you to make this a non-abstract interface? I think typically these
are abstract, and if there is one with gazillion methods that there is a
separate class which is an impl of interface with empty methods provided, for
convenience... But in this case, it doesn't seem to save you much.

If you'd like to consider an actual class rather than an abstract interface, I'd
left empty methods definitions in .h file (so it's clearly visible they are
empty) and moved "model" parameter out of them, instead having
offline_page_model() accessor on the Observer class (somewhat like
WebContentsObserver).

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:59: const OfflinePageItem* added);
added -> added_page?

Also. needs a comment on when it can be a nullptr.

[fgorski, 2016-11-02 15:51:06]

fgorski@chromium.org changed reviewers:
+ fgorski@chromium.org

[fgorski, 2016-11-02 15:51:07]

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

https://codereview.chromium.org/2469933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_proxy.cc:46: void
OfflinePageProxy::OfflinePageAdded(OfflinePageModel* model,
Please ping the owners of this class, as they may no longer need to fetch
offline pages, as they are getting direct access to a newly added item.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_ui_adapter.cc (right):

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/downloads/download_ui_adapter.cc:87: if
(items_.find(guid) != items_.end())  // Item already exists.
If item already exists this should probably never happen, therefore at least a
DCHECK? Or am I missing something?

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:51: class Observer {
https://bugs.chromium.org/p/chromium/issues/detail?id=565575 use this bug
please.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:57: virtual void
OfflinePageAdded(OfflinePageModel* model,
This hits me as problematic. I would expect another method for notifying
observers that an offline page has been changed/updated.

A short design document would be great for that.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:58: SavePageResult result,
Is this necessary?

I think you should not be informing anyone about failed add attempts. Therefore,
if you report page added, you are always not null and there is no need to pass
results, as SUCCESS is implied.

Also, I haven't seen it used in this patch.

[dewittj, 2016-11-02 16:09:58]

Description was changed from

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=
==========

to

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=565575
==========

[dewittj, 2016-11-02 16:09:58]

Description was changed from

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=
==========

to

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=565575
==========

[dewittj, 2016-11-02 16:33:16]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 16:33:40]

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

[dewittj, 2016-11-02 16:34:37]

https://codereview.chromium.org/2469933002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/2469933002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:293: //
TODO(dewittj): Change the Java side to use OfflinePageAdded.
On 2016/11/02 07:15:37, Dmitry Titov wrote:
> Please make it next CL.

Acknowledged.

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

https://codereview.chromium.org/2469933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_proxy.cc:46: void
OfflinePageProxy::OfflinePageAdded(OfflinePageModel* model,
On 2016/11/02 15:51:06, fgorski wrote:
> Please ping the owners of this class, as they may no longer need to fetch
> offline pages, as they are getting direct access to a newly added item.

That's the plan

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_ui_adapter.cc (right):

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/downloads/download_ui_adapter.cc:87: if
(items_.find(guid) != items_.end())  // Item already exists.
On 2016/11/02 15:51:06, fgorski wrote:
> If item already exists this should probably never happen, therefore at least a
> DCHECK? Or am I missing something?

I was trying not to change the logic from OnOfflinePagesChanged, but this could
indeed be a DCHECK.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:51: class Observer {
On 2016/11/02 15:51:06, fgorski wrote:
> https://bugs.chromium.org/p/chromium/issues/detail?id=565575 use this bug
> please.

Done.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:54: virtual void
OfflinePageModelLoaded(OfflinePageModel* model);
On 2016/11/02 07:15:37, Dmitry Titov wrote:
> What prompted you to make this a non-abstract interface?
Most observers don't implement all the methods, it was leading to a
proliferation of empty code.  But since there are still only 3 methods, I
suppose we could leave it abstract.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:57: virtual void
OfflinePageAdded(OfflinePageModel* model,
On 2016/11/02 15:51:06, fgorski wrote:
> This hits me as problematic. I would expect another method for notifying
> observers that an offline page has been changed/updated.
> 
> A short design document would be great for that.

This patch is not trying to change any behavior of the model.  I agree that we
lack an updated event, but today it is not possible to update a page except by
marking it as accessed.

Future patch could add support for update and an updated method on the observer.
 I'll make a doc for that change.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:58: SavePageResult result,
On 2016/11/02 15:51:06, fgorski wrote:
> Is this necessary?
> 
> I think you should not be informing anyone about failed add attempts.
Therefore,
> if you report page added, you are always not null and there is no need to pass
> results, as SUCCESS is implied.
> 
> Also, I haven't seen it used in this patch.

Not necessary now, I agree that observers won't care about failure except
possibly for the event log.  Removed.

https://codereview.chromium.org/2469933002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:59: const OfflinePageItem* added);
On 2016/11/02 07:15:37, Dmitry Titov wrote:
> added -> added_page?
> 
> Also. needs a comment on when it can be a nullptr.

Done.

[commit-bot: I haz the power, 2016-11-02 16:48:38]

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

[commit-bot: I haz the power, 2016-11-02 16:48:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dewittj, 2016-11-10 01:11:21]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 01:11:47]

Dry run: 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-11-10 01:37:03]

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

[commit-bot: I haz the power, 2016-11-10 01:37:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dewittj, 2016-11-10 18:40:04]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 18:40:43]

Dry run: 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-11-10 18:43:35]

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

[commit-bot: I haz the power, 2016-11-10 18:43:41]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dewittj, 2016-11-10 18:57:11]

Patchset #6 (id:100001) has been deleted

[dewittj, 2016-11-11 23:00:25]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 23:00:54]

Dry run: 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-11-11 23:15:29]

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

[commit-bot: I haz the power, 2016-11-11 23:15:29]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dewittj, 2016-11-11 23:39:59]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 23:40:30]

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

[dewittj, 2016-11-11 23:43:05]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 23:43:46]

Dry run: 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-11-12 00:20:07]

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

[commit-bot: I haz the power, 2016-11-12 00:20:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dewittj, 2016-11-12 00:25:07]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 00:25:59]

Dry run: 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-11-12 01:09:45]

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

[commit-bot: I haz the power, 2016-11-12 01:09:46]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-11-15 00:25:29]

lgtm

[Dmitry Titov, 2016-11-15 20:02:30]

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
File components/offline_pages/downloads/download_ui_adapter.cc (right):

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
components/offline_pages/downloads/download_ui_adapter.cc:86:
model_->GetAllPages(
Now the impl of this should change to do a direct add instead of re-get all.
Also the OnOfflinePagesChanged should change (likely by being removed?)

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.h:58: virtual void
OfflinePageAdded(OfflinePageModel* model,
Might be not this patch, but please consider removing "*model" parameter. There
is no practical case for an observer to observe multiple page models, right?

[dewittj, 2016-11-15 23:54:19]

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
File components/offline_pages/downloads/download_ui_adapter.cc (right):

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
components/offline_pages/downloads/download_ui_adapter.cc:86:
model_->GetAllPages(
On 2016/11/15 20:02:30, Dmitry Titov wrote:
> Now the impl of this should change to do a direct add instead of re-get all.
> Also the OnOfflinePagesChanged should change (likely by being removed?)

I had that in here before (see patchset #2), but in the interest of compact,
focused patches I decided to put it in an upcoming CL.

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2469933002/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.h:58: virtual void
OfflinePageAdded(OfflinePageModel* model,
On 2016/11/15 20:02:30, Dmitry Titov wrote:
> Might be not this patch, but please consider removing "*model" parameter.
There
> is no practical case for an observer to observe multiple page models, right?

Ack, will do later.

[dewittj, 2016-11-16 17:54:05]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 17:54:47]

Dry run: 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-11-16 17:58:04]

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

[commit-bot: I haz the power, 2016-11-16 17:58:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dmitry Titov, 2016-11-16 22:33:57]

LGTM, but please do follow up with the other parts of this.

[dewittj, 2016-12-02 22:47:19]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 22:47:47]

Dry run: 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-12-03 00:00:51]

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

[commit-bot: I haz the power, 2016-12-03 00:00:52]

Dry run: This issue passed the CQ dry run.

[dewittj, 2016-12-06 00:25:33]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-12-06 00:25:34]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2469933002/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-12-06 00:25:58]

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-12-06 00:33:40]

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

[commit-bot: I haz the power, 2016-12-06 00:33:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dewittj, 2016-12-06 00:37:30]

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

[dewittj, 2016-12-06 00:37:31]

bauerb@chromium.org: Please review changes in 

//components/ntp_snippets
//chrome/browser/ntp_snippets
SuggestionsSection.java

Thanks!

[Bernhard Bauer, 2016-12-06 17:17:17]

https://codereview.chromium.org/2469933002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2469933002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:152:
public void offlinePageAdded(final OfflinePageItem addedPage) {
Nit: The `final` isn't really necessary.

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
File
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:83:
void AddOfflinePageToModel(const OfflinePageItem item) {
Should this take a (const) reference?

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:91:
auto end = model_.mutable_items()->end();
Why do you erase everything until the end?

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:120:
for (auto& recent_tab : recent_tabs_list)
Could you use the actual type of |recent_tab|?

[dewittj, 2016-12-06 17:49:56]

https://codereview.chromium.org/2469933002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2469933002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:152:
public void offlinePageAdded(final OfflinePageItem addedPage) {
On 2016/12/06 17:17:17, Bernhard Bauer wrote:
> Nit: The `final` isn't really necessary.

Done.

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
File
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:83:
void AddOfflinePageToModel(const OfflinePageItem item) {
On 2016/12/06 17:17:17, Bernhard Bauer wrote:
> Should this take a (const) reference?

Done.

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:91:
auto end = model_.mutable_items()->end();
On 2016/12/06 17:17:17, Bernhard Bauer wrote:
> Why do you erase everything until the end?

std::remove is somewhat misleading; all it does is move the matching item to the
end of the list; after that happens you need to erase everything from the
returned iterator to the end.  From cppreference:

Removing is done by shifting (by means of move assignment) the elements in the
range in such a way that the elements that are not to be removed appear in the
beginning of the range. Relative order of the elements that remain is preserved
and the physical size of the container is unchanged. [...]  A call to remove is
typically followed by a call to a container's erase method, which erases the
unspecified values and reduces the physical size of the container to match its
new logical size.

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:120:
for (auto& recent_tab : recent_tabs_list)
On 2016/12/06 17:17:17, Bernhard Bauer wrote:
> Could you use the actual type of |recent_tab|?

Done.

[dewittj, 2016-12-06 22:03:00]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:03:38]

Dry run: 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-12-06 23:39:51]

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

[commit-bot: I haz the power, 2016-12-06 23:39:52]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-12-07 15:44:41]

Thanks! LGTM with some nits.

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
File
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2469933002/diff/200001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:91:
auto end = model_.mutable_items()->end();
On 2016/12/06 17:49:56, dewittj wrote:
> On 2016/12/06 17:17:17, Bernhard Bauer wrote:
> > Why do you erase everything until the end?
> 
> std::remove is somewhat misleading; all it does is move the matching item to
the
> end of the list; after that happens you need to erase everything from the
> returned iterator to the end.  From cppreference:
> 
> Removing is done by shifting (by means of move assignment) the elements in the
> range in such a way that the elements that are not to be removed appear in the
> beginning of the range. Relative order of the elements that remain is
preserved
> and the physical size of the container is unchanged. [...]  A call to remove
is
> typically followed by a call to a container's erase method, which erases the
> unspecified values and reduces the physical size of the container to match its
> new logical size.

Oh right, I thought std::remove would just create a hole. Thanks for the
explanation!

(Possible yak-shave: Add a helper method with a more descriptive name to
base/stl_util.h).

https://codereview.chromium.org/2469933002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/2469933002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:81:
public void offlinePageAdded(final OfflinePageItem addedPage) {}
Can you remove this `final` too?

https://codereview.chromium.org/2469933002/diff/240001/components/ntp_snippet...
File
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2469933002/diff/240001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:190:
for (auto& recent_tab : recent_tabs_list)
Can you use OfflinePageItem here and below as well?

[dewittj, 2016-12-07 23:14:02]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 23:14:44]

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

[dewittj, 2016-12-07 23:28:14]

hm, might hold off on the yak shaving :)

Thanks!

https://codereview.chromium.org/2469933002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/2469933002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:81:
public void offlinePageAdded(final OfflinePageItem addedPage) {}
On 2016/12/07 15:44:41, Bernhard Bauer wrote:
> Can you remove this `final` too?

Done.

https://codereview.chromium.org/2469933002/diff/240001/components/ntp_snippet...
File
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2469933002/diff/240001/components/ntp_snippet...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:190:
for (auto& recent_tab : recent_tabs_list)
On 2016/12/07 15:44:41, Bernhard Bauer wrote:
> Can you use OfflinePageItem here and below as well?

Done.

[dewittj, 2016-12-07 23:28:20]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-12-07 23:28:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
fgorski@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2469933002/#ps280001
(title: "nits")

[commit-bot: I haz the power, 2016-12-07 23:29:00]

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-12-08 01:27:28]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1481153300150350,
"parent_rev": "82494e61c72f6ab461a5281928ad29ed83862f98", "commit_rev":
"dd8d62cc056cc7ce5221e65448d26eff12bc5b43"}

[commit-bot: I haz the power, 2016-12-08 01:27:55]

Description was changed from

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=565575
==========

to

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=565575
==========

[commit-bot: I haz the power, 2016-12-08 01:27:56]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-12-08 01:33:21]

Description was changed from

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=565575
==========

to

==========
Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

The Observer API is somewhat misleading because the OfflinePageModelChanged
function is not called for all model changes.  This renames the function and
passes the actual page that was added, which can reduce the query load for
observers.

BUG=565575

Committed: https://crrev.com/011a526e4b9923dd648df7b2d6914ea7e6f109fe
Cr-Commit-Position: refs/heads/master@{#437114}
==========

[commit-bot: I haz the power, 2016-12-08 01:33:22]

Patchset 14 (id:??) landed as
https://crrev.com/011a526e4b9923dd648df7b2d6914ea7e6f109fe
Cr-Commit-Position: refs/heads/master@{#437114}