[NTP Snippets] Refresh the displayed snippets less frequently

[NTP Snippets] Refresh the displayed snippets less frequently

The fetch frequence of the snippet service does not change, but the
snippets are now refreshed less frequently on the UI. They are pulled
when the NTP is opened, or when the user switches back to it from
another tab or another app.

Additionally, we also change the way the list is updated, adding only
new entries instead of clearing the old list and replacing it with
the new one.

BUG=601757
Committed: https://crrev.com/104124354787e845e02db283185d499232518dd6
Cr-Commit-Position: refs/heads/master@{#388195}

[dgn, 2016-04-14 18:54:14]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org, treib@chromium.org

[dgn, 2016-04-14 18:54:15]

PTAL

[Marc Treib, 2016-04-15 09:06:49]

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:85:
// Fetch new snippets when swiching back to the NTP, from another tab or another
app.
s/swiching/switching/

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:107:
if (mNewTabPageListItems.size() == 1) { // The above-the-fold item is always
there.
Err wait, that's there even if we don't have any snippets?! That seems super
frustrating for the user: "Hey look at these cool snippets - haha no fooled you,
actually there aren't any!"
(IIUC this is a pre-existing problem, so it doesn't need to get fixed in this
CL, but please file a bug for it and add a TODO.)

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111:
// The service always adds new snippets at the end of the list
This worries me. It likely won't stay like this, and there are already cases
where it isn't true: Snippets can also get removed, e.g. because they expired.
IMO if we update the snippets at all, then we do it properly, i.e. either
replace the whole list (as we did before, but with this CL it should happen much
less often), or have actual merging logic.

[dgn, 2016-04-15 09:56:06]

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:107:
if (mNewTabPageListItems.size() == 1) { // The above-the-fold item is always
there.
On 2016/04/15 09:06:49, Marc Treib wrote:
> Err wait, that's there even if we don't have any snippets?! That seems super
> frustrating for the user: "Hey look at these cool snippets - haha no fooled
you,
> actually there aren't any!"
> (IIUC this is a pre-existing problem, so it doesn't need to get fixed in this
> CL, but please file a bug for it and add a TODO.)

The above-the-fold item is the one containing the search bar and the ML tiles. I
think you're thinking about SnippetHeaderListItem, which we add only when we
have snippets, right below.

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111:
// The service always adds new snippets at the end of the list
On 2016/04/15 09:06:49, Marc Treib wrote:
> This worries me. It likely won't stay like this, and there are already cases
> where it isn't true: Snippets can also get removed, e.g. because they expired.
> IMO if we update the snippets at all, then we do it properly, i.e. either
> replace the whole list (as we did before, but with this CL it should happen
much
> less often), or have actual merging logic.

I wanted to remove the annoyance of having snippets randomly disappear under
your finger, but in the cases where they are refreshed, there is a context
switch so it should not be too annoying. There is also the fact that when
clearing, we lose downloaded thumbnails, but that can be worked around.

[Marc Treib, 2016-04-15 10:00:58]

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:107:
if (mNewTabPageListItems.size() == 1) { // The above-the-fold item is always
there.
On 2016/04/15 09:56:06, dgn wrote:
> On 2016/04/15 09:06:49, Marc Treib wrote:
> > Err wait, that's there even if we don't have any snippets?! That seems super
> > frustrating for the user: "Hey look at these cool snippets - haha no fooled
> you,
> > actually there aren't any!"
> > (IIUC this is a pre-existing problem, so it doesn't need to get fixed in
this
> > CL, but please file a bug for it and add a TODO.)
> 
> The above-the-fold item is the one containing the search bar and the ML tiles.
I
> think you're thinking about SnippetHeaderListItem, which we add only when we
> have snippets, right below.

Ah yes, I interpreted "the above-the-fold item" as "the peeking card thing".
Maybe worth expanding the comment slightly to make this clearer for the next
person who reads it?

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111:
// The service always adds new snippets at the end of the list
On 2016/04/15 09:56:06, dgn wrote:
> On 2016/04/15 09:06:49, Marc Treib wrote:
> > This worries me. It likely won't stay like this, and there are already cases
> > where it isn't true: Snippets can also get removed, e.g. because they
expired.
> > IMO if we update the snippets at all, then we do it properly, i.e. either
> > replace the whole list (as we did before, but with this CL it should happen
> much
> > less often), or have actual merging logic.
> 
> I wanted to remove the annoyance of having snippets randomly disappear under
> your finger, but in the cases where they are refreshed, there is a context
> switch so it should not be too annoying. There is also the fact that when
> clearing, we lose downloaded thumbnails, but that can be worked around.

The thumbnails downloading/caching should soon be moved to the native side
anyway (crbug.com/584429), so I wouldn't worry too much about that.

[dgn, 2016-04-15 14:04:56]

PTAL

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:85:
// Fetch new snippets when swiching back to the NTP, from another tab or another
app.
On 2016/04/15 09:06:49, Marc Treib wrote:
> s/swiching/switching/

Thanks. I keep making typos, I really need to pay better attention while
proofreading -_-

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:107:
if (mNewTabPageListItems.size() == 1) { // The above-the-fold item is always
there.
On 2016/04/15 10:00:58, Marc Treib wrote:
> On 2016/04/15 09:56:06, dgn wrote:
> > On 2016/04/15 09:06:49, Marc Treib wrote:
> > > Err wait, that's there even if we don't have any snippets?! That seems
super
> > > frustrating for the user: "Hey look at these cool snippets - haha no
fooled
> > you,
> > > actually there aren't any!"
> > > (IIUC this is a pre-existing problem, so it doesn't need to get fixed in
> this
> > > CL, but please file a bug for it and add a TODO.)
> > 
> > The above-the-fold item is the one containing the search bar and the ML
tiles.
> I
> > think you're thinking about SnippetHeaderListItem, which we add only when we
> > have snippets, right below.
> 
> Ah yes, I interpreted "the above-the-fold item" as "the peeking card thing".
> Maybe worth expanding the comment slightly to make this clearer for the next
> person who reads it?

Obsolete, that code is gone. The documentation for AboveTheFoldListItem also
explains it nicely I think.

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111:
// The service always adds new snippets at the end of the list
On 2016/04/15 10:00:58, Marc Treib wrote:
> On 2016/04/15 09:56:06, dgn wrote:
> > On 2016/04/15 09:06:49, Marc Treib wrote:
> > > This worries me. It likely won't stay like this, and there are already
cases
> > > where it isn't true: Snippets can also get removed, e.g. because they
> expired.
> > > IMO if we update the snippets at all, then we do it properly, i.e. either
> > > replace the whole list (as we did before, but with this CL it should
happen
> > much
> > > less often), or have actual merging logic.
> > 
> > I wanted to remove the annoyance of having snippets randomly disappear under
> > your finger, but in the cases where they are refreshed, there is a context
> > switch so it should not be too annoying. There is also the fact that when
> > clearing, we lose downloaded thumbnails, but that can be worked around.
> 
> The thumbnails downloading/caching should soon be moved to the native side
> anyway (crbug.com/584429), so I wouldn't worry too much about that.

Done, the whole list is refreshed now.
Thumbnails: Fairly simple to support and helps trendemously on 3g, wdyt?

[Marc Treib, 2016-04-15 15:33:18]

LGTM, thanks!

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111:
// The service always adds new snippets at the end of the list
On 2016/04/15 14:04:56, dgn wrote:
> On 2016/04/15 10:00:58, Marc Treib wrote:
> > On 2016/04/15 09:56:06, dgn wrote:
> > > On 2016/04/15 09:06:49, Marc Treib wrote:
> > > > This worries me. It likely won't stay like this, and there are already
> cases
> > > > where it isn't true: Snippets can also get removed, e.g. because they
> > expired.
> > > > IMO if we update the snippets at all, then we do it properly, i.e.
either
> > > > replace the whole list (as we did before, but with this CL it should
> happen
> > > much
> > > > less often), or have actual merging logic.
> > > 
> > > I wanted to remove the annoyance of having snippets randomly disappear
under
> > > your finger, but in the cases where they are refreshed, there is a context
> > > switch so it should not be too annoying. There is also the fact that when
> > > clearing, we lose downloaded thumbnails, but that can be worked around.
> > 
> > The thumbnails downloading/caching should soon be moved to the native side
> > anyway (crbug.com/584429), so I wouldn't worry too much about that.
> 
> Done, the whole list is refreshed now.
> Thumbnails: Fairly simple to support and helps trendemously on 3g, wdyt?

Yeah, this seems like a good idea! Even in the future when the thumbnails will
be cached, there's no need to reload them when we already have them right here.

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:117:
mNewTabPageListItems.remove(existingSnippetIdx);
This is an ArrayList, so remove() is O(n). I don't think this will speed things
up overall.

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:27:
* @param snippets latest state of the snippet list from the native service.
nit: I think "Latest" should be capitalized? Also "Whether" below.

[dgn, 2016-04-15 15:42:57]

PTAL, I added some tests.

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:117:
mNewTabPageListItems.remove(existingSnippetIdx);
On 2016/04/15 15:33:18, Marc Treib wrote:
> This is an ArrayList, so remove() is O(n). I don't think this will speed
things
> up overall.

I was thinking about indexOf()

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:27:
* @param snippets latest state of the snippet list from the native service.
On 2016/04/15 15:33:18, Marc Treib wrote:
> nit: I think "Latest" should be capitalized? Also "Whether" below.

Done.

[Marc Treib, 2016-04-15 16:08:39]

Thanks for adding the tests!

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:117:
mNewTabPageListItems.remove(existingSnippetIdx);
On 2016/04/15 15:42:57, dgn wrote:
> On 2016/04/15 15:33:18, Marc Treib wrote:
> > This is an ArrayList, so remove() is O(n). I don't think this will speed
> things
> > up overall.
> 
> I was thinking about indexOf()

...which is also O(n), but doesn't modify things, so is probably faster than
remove().

https://codereview.chromium.org/1888913002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/1888913002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:124:
new NewTabPageAdapter(mNewTabPageManager, null);
What does this do? Create and then more or less immediately destroy the object?!

[dgn, 2016-04-15 16:54:13]

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1888913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:117:
mNewTabPageListItems.remove(existingSnippetIdx);
On 2016/04/15 16:08:39, Marc Treib wrote:
> On 2016/04/15 15:42:57, dgn wrote:
> > On 2016/04/15 15:33:18, Marc Treib wrote:
> > > This is an ArrayList, so remove() is O(n). I don't think this will speed
> > things
> > > up overall.
> > 
> > I was thinking about indexOf()
> 
> ...which is also O(n), but doesn't modify things, so is probably faster than
> remove().

Ah right, my bad. Removed.

https://codereview.chromium.org/1888913002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/1888913002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:124:
new NewTabPageAdapter(mNewTabPageManager, null);
On 2016/04/15 16:08:39, Marc Treib wrote:
> What does this do? Create and then more or less immediately destroy the
object?!

The reference is still kept, as mSnippetsObserver. It's a bit obscure, true.
Added a variable.

[Marc Treib, 2016-04-15 17:19:57]

Still LGTM :)

[Bernhard Bauer, 2016-04-18 08:19:57]

Could you update the CL title to explain that the UI is updated less frequently,
not the stored list of snippets? :)

https://codereview.chromium.org/1888913002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1888913002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:28:
* @return Whether this observer should be notified of further changes.
This is a bit uncommon... It would be more straightforward for the observer to
unregister itself when decides it doesn't want to observe snippets anymore.

[dgn, 2016-04-18 10:49:04]

Description was changed from

==========
[NTP Snippets] Refresh the snippets less frequently

The fetch frequence of the snippet service does not change, but the
snippets are now refreshed less frequently on the UI. They are pulled
when the NTP is opened, or when the user switches back to it from
another tab or another app.

Additionally, we also change the way the list is updated, adding only
new entries instead of clearing the old list and replacing it with
the new one.

BUG=601757
==========

to

==========
[NTP Snippets] Refresh the displayed snippets less frequently

The fetch frequence of the snippet service does not change, but the
snippets are now refreshed less frequently on the UI. They are pulled
when the NTP is opened, or when the user switches back to it from
another tab or another app.

Additionally, we also change the way the list is updated, adding only
new entries instead of clearing the old list and replacing it with
the new one.

BUG=601757
==========

[dgn, 2016-04-18 11:27:05]

PTAL

https://codereview.chromium.org/1888913002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1888913002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:28:
* @return Whether this observer should be notified of further changes.
On 2016/04/18 08:19:57, Bernhard Bauer wrote:
> This is a bit uncommon... It would be more straightforward for the observer to
> unregister itself when decides it doesn't want to observe snippets anymore.

Done. Used setObserver(null) instead.

[Bernhard Bauer, 2016-04-19 10:10:06]

lgtm

[dgn, 2016-04-19 10:11:40]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-04-19 10:11:41]

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/1888913002/#ps100001
(title: "explicitly unregister the observer instead of using the return value")

[commit-bot: I haz the power, 2016-04-19 10:11:48]

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

[commit-bot: I haz the power, 2016-04-19 10:34:13]

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

[commit-bot: I haz the power, 2016-04-19 10:34:14]

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

[dgn, 2016-04-19 12:29:42]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-04-19 12:29:43]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1888913002/#ps120001
(title: "rebase and fix tests")

[commit-bot: I haz the power, 2016-04-19 12:29:57]

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

[commit-bot: I haz the power, 2016-04-19 13:00:17]

Description was changed from

==========
[NTP Snippets] Refresh the displayed snippets less frequently

The fetch frequence of the snippet service does not change, but the
snippets are now refreshed less frequently on the UI. They are pulled
when the NTP is opened, or when the user switches back to it from
another tab or another app.

Additionally, we also change the way the list is updated, adding only
new entries instead of clearing the old list and replacing it with
the new one.

BUG=601757
==========

to

==========
[NTP Snippets] Refresh the displayed snippets less frequently

The fetch frequence of the snippet service does not change, but the
snippets are now refreshed less frequently on the UI. They are pulled
when the NTP is opened, or when the user switches back to it from
another tab or another app.

Additionally, we also change the way the list is updated, adding only
new entries instead of clearing the old list and replacing it with
the new one.

BUG=601757
==========

[commit-bot: I haz the power, 2016-04-19 13:00:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-19 13:01:52]

Description was changed from

==========
[NTP Snippets] Refresh the displayed snippets less frequently

The fetch frequence of the snippet service does not change, but the
snippets are now refreshed less frequently on the UI. They are pulled
when the NTP is opened, or when the user switches back to it from
another tab or another app.

Additionally, we also change the way the list is updated, adding only
new entries instead of clearing the old list and replacing it with
the new one.

BUG=601757
==========

to

==========
[NTP Snippets] Refresh the displayed snippets less frequently

The fetch frequence of the snippet service does not change, but the
snippets are now refreshed less frequently on the UI. They are pulled
when the NTP is opened, or when the user switches back to it from
another tab or another app.

Additionally, we also change the way the list is updated, adding only
new entries instead of clearing the old list and replacing it with
the new one.

BUG=601757

Committed: https://crrev.com/104124354787e845e02db283185d499232518dd6
Cr-Commit-Position: refs/heads/master@{#388195}
==========

[commit-bot: I haz the power, 2016-04-19 13:01:53]

Patchset 7 (id:??) landed as
https://crrev.com/104124354787e845e02db283185d499232518dd6
Cr-Commit-Position: refs/heads/master@{#388195}

[cmumford, 2016-04-19 16:22:39]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1897313002/ by cmumford@chromium.org.

The reason for reverting is: testSnippetLoading is failing. Opened new bug:
crbug.com/604763.