[Offline Pages] Remove load state from public OfflinePageModel API.

[Offline Pages] Remove load state from public OfflinePageModel API.

The only consumer of this bit is in the New Tab Page, where it is
used to guard against delaying load of the NTP while the offline page
model is loaded.

Median time to load in stable is currently 200ms.  See
https://goto.google.com/ltwxu for full distribution.  If this is too
long we need to modify the NTP UI before landing this.

BUG=673412

[dewittj, 2016-12-09 21:03:41]

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

[commit-bot: I haz the power, 2016-12-09 21:04:30]

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-09 22:05:21]

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

[commit-bot: I haz the power, 2016-12-09 22:05:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dewittj, 2016-12-09 22:25:54]

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

[commit-bot: I haz the power, 2016-12-09 22:26:58]

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-10 01:38:36]

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

[commit-bot: I haz the power, 2016-12-10 01:38:37]

Dry run: This issue passed the CQ dry run.

[dewittj, 2016-12-12 18:52:39]

Description was changed from

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

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

to

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

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

[dewittj, 2016-12-12 18:52:40]

dewittj@chromium.org changed reviewers:
+ dimich@chromium.org, treib@chromium.org, vitaliii@chromium.org

[dewittj, 2016-12-12 18:54:21]

Description was changed from

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

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

to

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

BUG=673412
==========

[dewittj, 2016-12-12 19:05:05]

Description was changed from

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

BUG=673412
==========

to

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

The only consumer of this bit is in the New Tab Page, where it is used to guard
against delaying load of the NTP while the offline page model is loaded.

Median time to load in stable is currently 200ms.  See
https://goto.google.com/ltwxu for full distribution.  If this is too long we
need to modify the NTP UI before landing this.

BUG=673412
==========

[dewittj, 2016-12-12 19:09:57]

Description was changed from

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

The only consumer of this bit is in the New Tab Page, where it is used to guard
against delaying load of the NTP while the offline page model is loaded.

Median time to load in stable is currently 200ms.  See
https://goto.google.com/ltwxu for full distribution.  If this is too long we
need to modify the NTP UI before landing this.

BUG=673412
==========

to

==========
[Offline Pages] Remove load state from public OfflinePageModel API.

The only consumer of this bit is in the New Tab Page, where it is
used to guard against delaying load of the NTP while the offline page
model is loaded.

Median time to load in stable is currently 200ms.  See
https://goto.google.com/ltwxu for full distribution.  If this is too
long we need to modify the NTP UI before landing this.

BUG=673412
==========

[dewittj, 2016-12-12 19:41:53]

Hi all, PTAL.

vitalii/treib: NTP/Zine code
dimich: all

Thanks!

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(left):

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:151:
updateAllSnippetOfflineAvailability();
NTP folks: Do we need to call |updateAllSnippetOfflineAvailability| outside this
observer as well now?

[Marc Treib, 2016-12-13 10:11:01]

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

[Marc Treib, 2016-12-13 10:11:02]

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560: //
fully loaded.
Hm. I'd be more comfortable with resolving this first, i.e. making the badges
appear asynchronously. The median load time is okay, but there is quite a long
tail. +bauerb WDYT?

https://codereview.chromium.org/2564163002/diff/20001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/download_suggestions_provider.cc (left):

https://codereview.chromium.org/2564163002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/download_suggestions_provider.cc:306:
AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
I guess we don't need this call, because we'll get OfflinePageAdded
notifications anyway? Or should we now call this when the provider is created?

[Bernhard Bauer, 2016-12-14 15:55:07]

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560: //
fully loaded.
On 2016/12/13 10:11:02, Marc Treib wrote:
> Hm. I'd be more comfortable with resolving this first, i.e. making the badges
> appear asynchronously. The median load time is okay, but there is quite a long
> tail. +bauerb WDYT?

What is the current state exactly? Don't we already make the badges appears
asynchronously, just via explicitly listening for when the model is loaded?

[vitaliii, 2016-12-15 10:20:37]

Zine providers comments.

https://codereview.chromium.org/2564163002/diff/20001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/download_suggestions_provider.cc (left):

https://codereview.chromium.org/2564163002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/download_suggestions_provider.cc:306:
AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
On 2016/12/13 10:11:02, Marc Treib wrote:
> I guess we don't need this call, because we'll get OfflinePageAdded
> notifications anyway? Or should we now call this when the provider is created?

I believe that even if we get OfflinePageAdded, we may miss some of them at
startup.

https://codereview.chromium.org/2564163002/diff/20001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2564163002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:880:
ShouldLoadOfflinePagesIfMissesOnModelLoaded) {
This test must be renamed, since the OfflinePageModelLoaded notification has
been removed. I would call it "ShouldFetchOfflinePagesOnStartup".

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

https://codereview.chromium.org/2564163002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_pages_test_utils.cc:22:
FakeOfflinePageModel::FakeOfflinePageModel() {
 = default?

[Marc Treib, 2016-12-15 10:36:29]

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560: //
fully loaded.
On 2016/12/14 15:55:07, Bernhard Bauer wrote:
> On 2016/12/13 10:11:02, Marc Treib wrote:
> > Hm. I'd be more comfortable with resolving this first, i.e. making the
badges
> > appear asynchronously. The median load time is okay, but there is quite a
long
> > tail. +bauerb WDYT?
> 
> What is the current state exactly? Don't we already make the badges appears
> asynchronously, just via explicitly listening for when the model is loaded?

IIUC, the current state is: If, during NTP creation, the OfflinePageModel isn't
loaded yet, then we just don't show offline badges.
After this CL, we would instead block until it is loaded. (Probably the NTP
itself would still load, but the tiles wouldn't.)
We don't (yet) have the plumbing to make the offline badges appear after the
initial load.

[Bernhard Bauer, 2016-12-15 17:48:00]

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560: //
fully loaded.
On 2016/12/15 10:36:29, Marc Treib wrote:
> On 2016/12/14 15:55:07, Bernhard Bauer wrote:
> > On 2016/12/13 10:11:02, Marc Treib wrote:
> > > Hm. I'd be more comfortable with resolving this first, i.e. making the
> badges
> > > appear asynchronously. The median load time is okay, but there is quite a
> long
> > > tail. +bauerb WDYT?
> > 
> > What is the current state exactly? Don't we already make the badges appears
> > asynchronously, just via explicitly listening for when the model is loaded?
> 
> IIUC, the current state is: If, during NTP creation, the OfflinePageModel
isn't
> loaded yet, then we just don't show offline badges.
> After this CL, we would instead block until it is loaded. (Probably the NTP
> itself would still load, but the tiles wouldn't.)
> We don't (yet) have the plumbing to make the offline badges appear after the
> initial load.

What does the code in
https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
do then?

[vitaliii, 2016-12-16 08:28:04]

Drive-by comment.

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560: //
fully loaded.
On 2016/12/15 17:48:00, Bernhard Bauer wrote:
> On 2016/12/15 10:36:29, Marc Treib wrote:
> > On 2016/12/14 15:55:07, Bernhard Bauer wrote:
> > > On 2016/12/13 10:11:02, Marc Treib wrote:
> > > > Hm. I'd be more comfortable with resolving this first, i.e. making the
> > badges
> > > > appear asynchronously. The median load time is okay, but there is quite
a
> > long
> > > > tail. +bauerb WDYT?
> > > 
> > > What is the current state exactly? Don't we already make the badges
appears
> > > asynchronously, just via explicitly listening for when the model is
loaded?
> > 
> > IIUC, the current state is: If, during NTP creation, the OfflinePageModel
> isn't
> > loaded yet, then we just don't show offline badges.
> > After this CL, we would instead block until it is loaded. (Probably the NTP
> > itself would still load, but the tiles wouldn't.)
> > We don't (yet) have the plumbing to make the offline badges appear after the
> > initial load.
> 
> What does the code in
>
https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
> do then?

I agree with bauerb@. For example, if you download an article from NTP, its
offline badge appears on all open NTPs immediately.

[Dmitry Titov, 2016-12-17 02:57:35]

lgtm, from OfflinePages pov.

[Marc Treib, 2016-12-19 11:10:26]

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560: //
fully loaded.
On 2016/12/15 17:48:00, Bernhard Bauer wrote:
> On 2016/12/15 10:36:29, Marc Treib wrote:
> > On 2016/12/14 15:55:07, Bernhard Bauer wrote:
> > > On 2016/12/13 10:11:02, Marc Treib wrote:
> > > > Hm. I'd be more comfortable with resolving this first, i.e. making the
> > badges
> > > > appear asynchronously. The median load time is okay, but there is quite
a
> > long
> > > > tail. +bauerb WDYT?
> > > 
> > > What is the current state exactly? Don't we already make the badges
appears
> > > asynchronously, just via explicitly listening for when the model is
loaded?
> > 
> > IIUC, the current state is: If, during NTP creation, the OfflinePageModel
> isn't
> > loaded yet, then we just don't show offline badges.
> > After this CL, we would instead block until it is loaded. (Probably the NTP
> > itself would still load, but the tiles wouldn't.)
> > We don't (yet) have the plumbing to make the offline badges appear after the
> > initial load.
> 
> What does the code in
>
https://codereview.chromium.org/2564163002/diff/20001/chrome/android/java/src...
> do then?

That handles the offline badges on article suggestions. This here is for NTP
tiles (pka MostVisited). As far as I can tell, we still don't have the plumbing
here to add the badges after the tiles themselves.