Headers for resource tracking.

Headers for resource tracking.

The BackgroundLoaderOffliner needs signals to do a better job of knowing
when a page is loaded enough to take a snapshot.  One such signal is to
track the precentages of resources that have been requested that actually
get loaded.

This change adds the headers in the design documented below.

Design doc here:
https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0


BUG=699313
Review-Url: https://codereview.chromium.org/2933193003
Cr-Commit-Position: refs/heads/master@{#480210}
Committed: https://chromium.googlesource.com/chromium/src/+/7c8537fbd2ed880fd61e765bcdcc1d57b864853d

[Pete Williamson, 2017-06-13 17:03:15]

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

[commit-bot: I haz the power, 2017-06-13 17:03:32]

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-06-13 17:07:30]

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

[commit-bot: I haz the power, 2017-06-13 17:07:31]

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

[Pete Williamson, 2017-06-13 19:29:35]

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

[commit-bot: I haz the power, 2017-06-13 19:30:31]

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-06-13 21:15:43]

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

[commit-bot: I haz the power, 2017-06-13 21:15:45]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-06-13 21:36:46]

petewil@chromium.org changed reviewers:
+ csharrison@chromium.org, dimich@chromium.org, ryansturm@chromium.org

[Dmitry Titov, 2017-06-14 19:56:37]

Initial comments:

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:812: "offline_pages/offliner_user_data.cc",
These should be under build flag enable_offline_pages

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
File chrome/browser/loader/resource_tracker_observer.h (right):

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
chrome/browser/loader/resource_tracker_observer.h:12: class
ResourceTrackerObserver {
Since this is going to be used by OfflinePages, should it live in c/o_p/core or
crome/b/o_p?

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
chrome/browser/loader/resource_tracker_observer.h:12: class
ResourceTrackerObserver {
Naming: This is an observer of resource loading. Maybe
offline_pages::ResourceLoadingObserver?

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
chrome/browser/loader/resource_tracker_observer.h:20: MODEL,
Similarly, lets just have the types we are going to process in immediate future.
I guess it's TEXT_CSS and IMAGE?

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/offline_...
File chrome/browser/offline_pages/offliner_user_data.cc (right):

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:36: #if
BUILDFLAG(ENABLE_OFFLINE_PAGES)
It's strange to have the flag check in offline_pages code itself. It should be
checked in the code that invokes ResourceTrackerObserverFromWebContents().

[Pete Williamson, 2017-06-14 21:59:09]

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

[commit-bot: I haz the power, 2017-06-14 21:59:37]

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-06-14 23:31:26]

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

[commit-bot: I haz the power, 2017-06-14 23:31:26]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-06-15 01:03:16]

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

[Pete Williamson, 2017-06-15 01:03:27]

CR fixes per Dimich

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:812: "offline_pages/offliner_user_data.cc",
On 2017/06/14 19:56:37, Dmitry Titov wrote:
> These should be under build flag enable_offline_pages

Done.

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
File chrome/browser/loader/resource_tracker_observer.h (right):

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
chrome/browser/loader/resource_tracker_observer.h:12: class
ResourceTrackerObserver {
On 2017/06/14 19:56:37, Dmitry Titov wrote:
> Since this is going to be used by OfflinePages, should it live in c/o_p/core
or
> crome/b/o_p?

Done.

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
chrome/browser/loader/resource_tracker_observer.h:12: class
ResourceTrackerObserver {
On 2017/06/14 19:56:37, Dmitry Titov wrote:
> Naming: This is an observer of resource loading. Maybe
> offline_pages::ResourceLoadingObserver?

Done.

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/loader/r...
chrome/browser/loader/resource_tracker_observer.h:20: MODEL,
On 2017/06/14 19:56:37, Dmitry Titov wrote:
> Similarly, lets just have the types we are going to process in immediate
future.
> I guess it's TEXT_CSS and IMAGE?

Done.

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/offline_...
File chrome/browser/offline_pages/offliner_user_data.cc (right):

https://codereview.chromium.org/2933193003/diff/40001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:36: #if
BUILDFLAG(ENABLE_OFFLINE_PAGES)
On 2017/06/14 19:56:37, Dmitry Titov wrote:
> It's strange to have the flag check in offline_pages code itself. It should be
> checked in the code that invokes ResourceTrackerObserverFromWebContents().

Removed.

[commit-bot: I haz the power, 2017-06-15 01:03:42]

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-06-15 02:19:44]

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

[commit-bot: I haz the power, 2017-06-15 02:19:45]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-06-16 17:04:45]

bunch of nits, but lgtm on the whole. I'll let dimich review the rest.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
File chrome/browser/offline_pages/background_loader_offliner.h (right):

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/background_loader_offliner.h:69: //
ResourceTrackerObserver implemenation
s/ResourceTrackerObserver/ResourceLoadingObserver/

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/background_loader_offliner.h:70: void
ObserveResourceLoading(ResourceLoadingObserver::ResourceDataType type,
Is this called from anywhere? If not, would it make more sense to add it in the
CL that calls it?

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
File chrome/browser/offline_pages/offliner_user_data.cc (right):

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:18:
webcontents->SetUserData(UserDataKey(), std::unique_ptr<OfflinerUserData>(
nit, feel free to leave it as is:

Would it make more sense to call CreateForWebContents and then add a method
(Init) for adding |offliner| to the OfflinerUserData?

You could do that directly in BackgroundLoaderOffliner::AttachObservers since
it's only a couple lines as well.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:32: // static - gets the data
pointer as a ResourceTrackerObserver
s/ResourceTrackerObserver/ResourceLoadingObserver/

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:38: return
static_cast<ResourceLoadingObserver*>(
nit: Not sure this first static_case makes things clearer or more confusing.
Since BackgroundLoaderOffliner is a sub class of ResourceLoadingObserver, you
might want to just let the method/compiler downcast it.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
File chrome/browser/offline_pages/offliner_user_data.h (right):

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.h:22: static
ResourceLoadingObserver* ResourceTrackerObserverFromWebContents(
s/ResourceTrackerObserverFromWebContents/ResourceLoadingObserverFromWebContents/

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.h:26: Offliner* offliner() {
return offliner_; }
Why the switch from BackgroundOffliner to Offliner?

[Pete Williamson, 2017-06-16 21:16:16]

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
File chrome/browser/offline_pages/background_loader_offliner.h (right):

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/background_loader_offliner.h:69: //
ResourceTrackerObserver implemenation
On 2017/06/16 17:04:45, RyanSturm wrote:
> s/ResourceTrackerObserver/ResourceLoadingObserver/

Done.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/background_loader_offliner.h:70: void
ObserveResourceLoading(ResourceLoadingObserver::ResourceDataType type,
On 2017/06/16 17:04:44, RyanSturm wrote:
> Is this called from anywhere? If not, would it make more sense to add it in
the
> CL that calls it?

Not done.  The intent of this changelist is to give form to the upcoming
implementation change.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
File chrome/browser/offline_pages/offliner_user_data.cc (right):

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:18:
webcontents->SetUserData(UserDataKey(), std::unique_ptr<OfflinerUserData>(
On 2017/06/16 17:04:45, RyanSturm wrote:
> nit, feel free to leave it as is:
> 
> Would it make more sense to call CreateForWebContents and then add a method
> (Init) for adding |offliner| to the OfflinerUserData?
> 
> You could do that directly in BackgroundLoaderOffliner::AttachObservers since
> it's only a couple lines as well.

I could make that change, but it feels like one more step I could forget to do,
since this method does both creation and setting the offliner, and we always
need to do both.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:32: // static - gets the data
pointer as a ResourceTrackerObserver
On 2017/06/16 17:04:45, RyanSturm wrote:
> s/ResourceTrackerObserver/ResourceLoadingObserver/

Done.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.cc:38: return
static_cast<ResourceLoadingObserver*>(
On 2017/06/16 17:04:45, RyanSturm wrote:
> nit: Not sure this first static_case makes things clearer or more confusing.
> Since BackgroundLoaderOffliner is a sub class of ResourceLoadingObserver, you
> might want to just let the method/compiler downcast it.

I was able to get rid of both casts by changing the observer* to a
BackgroundLoadingObserver*

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
File chrome/browser/offline_pages/offliner_user_data.h (right):

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.h:22: static
ResourceLoadingObserver* ResourceTrackerObserverFromWebContents(
On 2017/06/16 17:04:45, RyanSturm wrote:
>
s/ResourceTrackerObserverFromWebContents/ResourceLoadingObserverFromWebContents/

Done.

https://codereview.chromium.org/2933193003/diff/80001/chrome/browser/offline_...
chrome/browser/offline_pages/offliner_user_data.h:26: Offliner* offliner() {
return offliner_; }
On 2017/06/16 17:04:45, RyanSturm wrote:
> Why the switch from BackgroundOffliner to Offliner?

Done (or rather undone, in this case ^_^)

The original intent was to allow this interface to be used in more places in the
code, such as common or components.  However, afterwards, we moved it back to
browser, and this didn't get put back.  While I like the Offliner* interface a
bit better, we can eliminate some casts with BackgroundLoaderOffliner, so I put
it back the way it used to be.

[Pete Williamson, 2017-06-16 21:16:23]

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

[commit-bot: I haz the power, 2017-06-16 21:16:37]

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-06-16 22:45:08]

lgtm, thanks!

[commit-bot: I haz the power, 2017-06-16 22:53:56]

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

[commit-bot: I haz the power, 2017-06-16 22:53:57]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-06-16 22:57:43]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2017-06-16 22:57:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2933193003/#ps100001
(title: "CR feedback from RyanSturm")

[commit-bot: I haz the power, 2017-06-16 22:58:10]

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-06-16 23:03:18]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497653863572090,
"parent_rev": "ce0e628972d6f0473c760da5851add36e13ceb49", "commit_rev":
"7c8537fbd2ed880fd61e765bcdcc1d57b864853d"}

[commit-bot: I haz the power, 2017-06-16 23:03:33]

Description was changed from

==========
Headers for resource tracking.

The BackgroundLoaderOffliner needs signals to do a better job of knowing
when a page is loaded enough to take a snapshot.  One such signal is to
track the precentages of resources that have been requested that actually
get loaded.

This change adds the headers in the design documented below.

Design doc here:
https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0


BUG=699313
==========

to

==========
Headers for resource tracking.

The BackgroundLoaderOffliner needs signals to do a better job of knowing
when a page is loaded enough to take a snapshot.  One such signal is to
track the precentages of resources that have been requested that actually
get loaded.

This change adds the headers in the design documented below.

Design doc here:
https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0


BUG=699313

Review-Url: https://codereview.chromium.org/2933193003
Cr-Commit-Position: refs/heads/master@{#480210}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c8537fbd2ed880fd61e765bcdcc...
==========

[commit-bot: I haz the power, 2017-06-16 23:03:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7c8537fbd2ed880fd61e765bcdcc...