[Offline Pages] Make prerenderer notify us of network progress.

[Offline Pages] Make prerenderer notify us of network progress.

Today, Prerenderer is made aware of bytes that are loaded when each
resource finishes, but does not expose it to observers of
PrerenderContents and friends.  This change allows us to track
coarse-grained progress on background downloads.

BUG=681269
Review-Url: https://codereview.chromium.org/2711693002
Cr-Commit-Position: refs/heads/master@{#452934}
Committed: https://chromium.googlesource.com/chromium/src/+/e932ed672d61ea3e2e4675aa17715bb2899af788

[Dmitry Titov, 2017-02-22 00:14:23]

dimich@chromium.org changed reviewers:
+ pasko@chromium.org, petewil@chromium.org

[Dmitry Titov, 2017-02-22 00:14:24]

This is a continuation of Justin's patch at
https://codereview.chromium.org/2697963002/

I've rebased the patch to remove the merge conflict.

[Dmitry Titov, 2017-02-22 00:14:38]

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

[commit-bot: I haz the power, 2017-02-22 00:16:26]

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

[Dmitry Titov, 2017-02-22 01:40:57]

Peter and Egor - PTAL please, this is Justin's patch, I resolved conflicts so it
passes on bots, it looks good to me - but please take a look!

Thanks! 
Dmitry

[commit-bot: I haz the power, 2017-02-22 02:19:47]

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

[commit-bot: I haz the power, 2017-02-22 02:19:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[pasko-google - do not use, 2017-02-22 13:32:43]

pasko@google.com changed reviewers:
+ pasko@google.com

[pasko-google - do not use, 2017-02-22 13:32:45]

as previously, changes in chrome/browser/prerender/ looks mostly good, left a
few comments.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:781: for (Observer& observer :
observer_list_)
nit: auto&?

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:245: int64_t network_bytes() {
return network_bytes_; }
I think it would be useful to link from here to some other description about
precision/limitations of the way we count network bytes. But the reference
itself might be long or ambiguous. If you like this comment, please do whatever
looks appropriate to you.

Very nit. Much rant. Wow.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:135: void
OnPrerenderNetworkBytesChanged(PrerenderContents* launcher) override {}
This ignores data from <link rel=prerender> that is coming from the page
currently being offlined. These prerenders are being removed, but we still can
potentially issue a NoStatePrefetch from this entry point until we factor it out
of Prerender code.

This accounting is OK to me, just wanted you to confirm that it is what you
want.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:99: class
TestPrerenderHandleObserver
Can we have a more specialized name like NetworkBytesChangedObserver? This would
make it more obvious what the class does down in the test.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:109: network_bytes_changed =
true;
even though it is a simple class, let's follow the coding style: make members
private, name them appropriately, DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:112: bool network_bytes_changed =
false;
member initializers outside constructors are allowed, but not common in
prerender code. Please move to constructor for consistency.

[pasko, 2017-02-22 13:34:36]

Description was changed from

==========
[Offline Pages] Make prerenderer notify us of network progress.

Today, Prerenderer is made aware of bytes that are loaded when each
resource finishes, but does not expose it to observers of
PrerenderContents and friends.  This change allows us to track
coarse-grained progress on background downloads.

BUG=681269
==========

to

==========
[Offline Pages] Make prerenderer notify us of network progress.

Today, Prerenderer is made aware of bytes that are loaded when each
resource finishes, but does not expose it to observers of
PrerenderContents and friends.  This change allows us to track
coarse-grained progress on background downloads.

BUG=681269
==========

[pasko, 2017-02-22 13:34:36]

pasko@chromium.org changed reviewers:
- pasko@google.com

[pasko, 2017-02-22 13:35:22]

wrote from the wrong account, sry, but all comments should still reflect what I
tried to say

[Pete Williamson, 2017-02-22 18:46:30]

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:172: void
PrerenderAdapterTest::OnPrerenderNetworkBytesChanged(int64_t bytes) {}
Might be good to add an actual test here.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc
(right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:172:
base::Bind([](int64_t bytes) {})));
What does this line do?  Is this a "do-nothing" lambda in C++ 11/14?  Are we
allowing those in the code yet?

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:781: for (Observer& observer :
observer_list_)
On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> nit: auto&?

I think this is better as Observer&, auto is helpful when the type is really
long (like a const iterator over some huge type).

[Dmitry Titov, 2017-02-23 04:23:18]

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

[commit-bot: I haz the power, 2017-02-23 04:23:40]

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

[Dmitry Titov, 2017-02-23 04:27:55]

Thanks much for good comments!
PTAL.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:172: void
PrerenderAdapterTest::OnPrerenderNetworkBytesChanged(int64_t bytes) {}
On 2017/02/22 18:46:30, Pete Williamson wrote:
> Might be good to add an actual test here.

Done.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc
(right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:172:
base::Bind([](int64_t bytes) {})));
On 2017/02/22 18:46:30, Pete Williamson wrote:
> What does this line do?  Is this a "do-nothing" lambda in C++ 11/14?  Are we
> allowing those in the code yet?

Yep, lambda. 
https://cs.chromium.org/search/?q=Bind%5C(%5C%5B%5C%5D&type=cs
Also, https://chromium-cpp.appspot.com/ says they are allowed.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:781: for (Observer& observer :
observer_list_)
On 2017/02/22 18:46:30, Pete Williamson wrote:
> On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> > nit: auto&?
> 
> I think this is better as Observer&, auto is helpful when the type is really
> long (like a const iterator over some huge type).

Oh, opposite guidance! :)
I've looked through this file and for consistency with existing here code, the
Observer& seems to win... SO leaving it for now.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:245: int64_t network_bytes() {
return network_bytes_; }
On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> I think it would be useful to link from here to some other description about
> precision/limitations of the way we count network bytes. But the reference
> itself might be long or ambiguous. If you like this comment, please do
whatever
> looks appropriate to you.
> 
> Very nit. Much rant. Wow.

Done. I tried.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:135: void
OnPrerenderNetworkBytesChanged(PrerenderContents* launcher) override {}
On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> This ignores data from <link rel=prerender> that is coming from the page
> currently being offlined. These prerenders are being removed, but we still can
> potentially issue a NoStatePrefetch from this entry point until we factor it
out
> of Prerender code.
> 
> This accounting is OK to me, just wanted you to confirm that it is what you
> want.

Acknowledged.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:99: class
TestPrerenderHandleObserver
On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> Can we have a more specialized name like NetworkBytesChangedObserver? This
would
> make it more obvious what the class does down in the test.

Done.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:109: network_bytes_changed =
true;
On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> even though it is a simple class, let's follow the coding style: make members
> private, name them appropriately, DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:112: bool network_bytes_changed =
false;
On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> member initializers outside constructors are allowed, but not common in
> prerender code. Please move to constructor for consistency.

Done.

[commit-bot: I haz the power, 2017-02-23 04:34:41]

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

[commit-bot: I haz the power, 2017-02-23 04:34:42]

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

[Dmitry Titov, 2017-02-23 04:40:06]

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

[commit-bot: I haz the power, 2017-02-23 04:40:20]

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, 2017-02-23 05:19:36]

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

[commit-bot: I haz the power, 2017-02-23 05:19:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Pete Williamson, 2017-02-23 18:26:20]

lgtm

[pasko, 2017-02-24 15:10:28]

chrome/browser/prerender/ lgtm

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc
(right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:172:
base::Bind([](int64_t bytes) {})));
On 2017/02/23 04:27:54, Dmitry Titov wrote:
> On 2017/02/22 18:46:30, Pete Williamson wrote:
> > What does this line do?  Is this a "do-nothing" lambda in C++ 11/14?  Are we
> > allowing those in the code yet?
> 
> Yep, lambda. 
> https://cs.chromium.org/search/?q=Bind%5C(%5C%5B%5C%5D&type=cs
> Also, https://chromium-cpp.appspot.com/ says they are allowed.

Interestingly, Bind allows capture-less lambda, but should reject others:

https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:781: for (Observer& observer :
observer_list_)
On 2017/02/23 04:27:54, Dmitry Titov wrote:
> On 2017/02/22 18:46:30, Pete Williamson wrote:
> > On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> > > nit: auto&?
> > 
> > I think this is better as Observer&, auto is helpful when the type is really
> > long (like a const iterator over some huge type).
> 
> Oh, opposite guidance! :)
> I've looked through this file and for consistency with existing here code, the
> Observer& seems to win... SO leaving it for now.

sgtm

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:245: int64_t network_bytes() {
return network_bytes_; }
On 2017/02/23 04:27:54, Dmitry Titov wrote:
> On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> > I think it would be useful to link from here to some other description about
> > precision/limitations of the way we count network bytes. But the reference
> > itself might be long or ambiguous. If you like this comment, please do
> whatever
> > looks appropriate to you.
> > 
> > Very nit. Much rant. Wow.
> 
> Done. I tried.

the new description is OK. I asked for a more detailed comment because I did not
know a central place where caveats like "we don't count TLS overhead" are
mentioned. If there is no easy place, let's keep it as you last suggested.

https://codereview.chromium.org/2711693002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2711693002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:115: bool network_bytes_changed()
{ return network_bytes_changed_; }
const

[Dmitry Titov, 2017-02-24 19:09:13]

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2711693002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:245: int64_t network_bytes() {
return network_bytes_; }
On 2017/02/24 15:10:28, pasko wrote:
> On 2017/02/23 04:27:54, Dmitry Titov wrote:
> > On 2017/02/22 13:32:44, pasko-google - do not use wrote:
> > > I think it would be useful to link from here to some other description
about
> > > precision/limitations of the way we count network bytes. But the reference
> > > itself might be long or ambiguous. If you like this comment, please do
> > whatever
> > > looks appropriate to you.
> > > 
> > > Very nit. Much rant. Wow.
> > 
> > Done. I tried.
> 
> the new description is OK. I asked for a more detailed comment because I did
not
> know a central place where caveats like "we don't count TLS overhead" are
> mentioned. If there is no easy place, let's keep it as you last suggested.

Acknowledged.

https://codereview.chromium.org/2711693002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2711693002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:115: bool network_bytes_changed()
{ return network_bytes_changed_; }
On 2017/02/24 15:10:28, pasko wrote:
> const

Done.

[Dmitry Titov, 2017-02-24 19:09:19]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2017-02-24 19:09:19]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2711693002/#ps60001
(title: "last minute fixes")

[commit-bot: I haz the power, 2017-02-24 19:09:53]

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, 2017-02-24 21:38:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487963359028140,
"parent_rev": "c1b9731d28d5fbf0dc02bc49665f4f38d5a26fac", "commit_rev":
"e932ed672d61ea3e2e4675aa17715bb2899af788"}

[commit-bot: I haz the power, 2017-02-24 21:39:19]

Description was changed from

==========
[Offline Pages] Make prerenderer notify us of network progress.

Today, Prerenderer is made aware of bytes that are loaded when each
resource finishes, but does not expose it to observers of
PrerenderContents and friends.  This change allows us to track
coarse-grained progress on background downloads.

BUG=681269
==========

to

==========
[Offline Pages] Make prerenderer notify us of network progress.

Today, Prerenderer is made aware of bytes that are loaded when each
resource finishes, but does not expose it to observers of
PrerenderContents and friends.  This change allows us to track
coarse-grained progress on background downloads.

BUG=681269

Review-Url: https://codereview.chromium.org/2711693002
Cr-Commit-Position: refs/heads/master@{#452934}
Committed:
https://chromium.googlesource.com/chromium/src/+/e932ed672d61ea3e2e4675aa1771...
==========

[commit-bot: I haz the power, 2017-02-24 21:39:20]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e932ed672d61ea3e2e4675aa1771...

[Dmitry Titov, 2017-02-25 00:17:20]

https://codereview.chromium.org/2711693002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2711693002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:22: const char
kDownloadUIAdapterKey[] = "download-ui-adapter";
Apparently, this doesn't work since the GetUserData does not use the value of
those strings, it only is interested in an address passed in. So even if you
have 2 equal strings but located in different files, using them as 'keys' does
not work...
I'll fix it in follow-up.