Factor management of metrics updates into its own class.

Factor management of metrics updates into its own class.

With the recent changes to merge paint timings across all frames in a page, we
have learned that first paint timings can arrive out of order.

This change factors IPC update management into its own class, which will
make it easier to refactor this new class to also support buffering of
cross frame paints, should we decide to implement that in a subsequent change.

BUG=725610
Review-Url: https://codereview.chromium.org/2904533002
Cr-Commit-Position: refs/heads/master@{#474635}
Committed: https://chromium.googlesource.com/chromium/src/+/000cea89869e0276d886fe541b3ff050aba9527f

[Bryan McQuade, 2017-05-23 14:52:52]

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

[commit-bot: I haz the power, 2017-05-23 14:53:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-23 14:55:02]

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

[commit-bot: I haz the power, 2017-05-23 14:55:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-23 15:00:47]

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

[commit-bot: I haz the power, 2017-05-23 15:00:53]

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-05-23 16:20:32]

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

[commit-bot: I haz the power, 2017-05-23 16:20:33]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-23 19:21:06]

Description was changed from

==========
Factor management of metrics updates into its own class.

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

to

==========
Factor management of metrics updates into its own class.

With the recent changes to merge paint timings across all frames in a page, we
have learned that first paint timings can arrive out of order.

This change factors IPC update management into its own class, which will
make it easier to refactor this new class to also support buffering of
cross frame paints, should we decide to implement that in a subsequent change.

BUG=725610
==========

[Bryan McQuade, 2017-05-23 19:21:17]

bmcquade@chromium.org changed reviewers:
+ csharrison@chromium.org

[Bryan McQuade, 2017-05-23 19:21:37]

PTAL, thanks!

Not totally sure what to call this new object. Let me know if you have
suggestions for renaming it.

Thanks,
Bryan

[Charlie Harrison, 2017-05-23 20:32:10]

This is a nice refactor, just left a few comments.

Overall I like this but it feels weird that the PageLoadTracker is both
controlling the dispatcher (i.e. sending UpdateTiming and DidFinishSubframeNav)
as well as being its client.

Maybe that's just the naming though. A pattern like this is maybe better
expressed as an "aggregator". Either that or there may be a way to re-jigger
such that the dispatcher is notified directly of timing updates / subframe navs
and forwards them to the PLT. Not sure.

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:9:
#include <memory>
Is <memory> for the mojo ptrs?

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:77: //
The Client interface informs a client of the
This is awkwardly worded. Maybe something like: "The Client class is updated
when metrics managed by the dispatcher have changed. Typically it owns the
dispatcher."

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:107:
const mojom::PageLoadMetadata& main_frame_metadata() const {
Does it need fwd declaration / #include?

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:132:
Client* const client_;
Can you comment on the lifetime of client_?

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:143:
mojom::PageLoadTimingPtr current_merged_page_timing_;
Can you comment what is the difference between current and pending?

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:603: if
(last_dispatched_merged_page_timing_->Equals(
Can we DCHECK this somehow? It seems like something that the dispatcher should
be doing for us.

[Bryan McQuade, 2017-05-23 20:50:36]

Thanks! PTAL.

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:77: //
The Client interface informs a client of the
On 2017/05/23 at 20:32:10, Charlie Harrison wrote:
> This is awkwardly worded. Maybe something like: "The Client class is updated
when metrics managed by the dispatcher have changed. Typically it owns the
dispatcher."

Done

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:107:
const mojom::PageLoadMetadata& main_frame_metadata() const {
On 2017/05/23 at 20:32:10, Charlie Harrison wrote:
> Does it need fwd declaration / #include?

I was including page_load_timing.h which includes the mojo header, but switched
to include mojo header directly, thanks!

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:132:
Client* const client_;
On 2017/05/23 at 20:32:10, Charlie Harrison wrote:
> Can you comment on the lifetime of client_?

Done

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:143:
mojom::PageLoadTimingPtr current_merged_page_timing_;
On 2017/05/23 at 20:32:10, Charlie Harrison wrote:
> Can you comment what is the difference between current and pending?

Done

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:603: if
(last_dispatched_merged_page_timing_->Equals(
On 2017/05/23 at 20:32:10, Charlie Harrison wrote:
> Can we DCHECK this somehow? It seems like something that the dispatcher should
be doing for us.

Agree, done.

[Bryan McQuade, 2017-05-23 20:51:09]

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:9:
#include <memory>
On 2017/05/23 at 20:32:10, Charlie Harrison wrote:
> Is <memory> for the mojo ptrs?

This isn't needed. Removed.

[Bryan McQuade, 2017-05-23 21:04:02]

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

[commit-bot: I haz the power, 2017-05-23 21:05:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-24 00:16:21]

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

[commit-bot: I haz the power, 2017-05-24 00:17:02]

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-05-24 02:51:41]

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

[commit-bot: I haz the power, 2017-05-24 02:51:42]

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...)

[Bryan McQuade, 2017-05-24 11:53:10]

On 2017/05/23 at 20:51:09, Bryan McQuade wrote:
>
https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
> File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):
> 
>
https://codereview.chromium.org/2904533002/diff/40001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:9:
#include <memory>
> On 2017/05/23 at 20:32:10, Charlie Harrison wrote:
> > Is <memory> for the mojo ptrs?
> 
> This isn't needed. Removed.

Updated this change based on our conversation yesterday. PTAL, thanks!

[Charlie Harrison, 2017-05-24 12:30:16]

Yeah this is looking a lot nicer to me.

This API also seems nicely testable. It would be cool to add a new unit test
suite for just this component in a followup.

I'll try to do a more detailed final pass today, but I may not get to it until
tomorrow since today is packed with meetings.

[Charlie Harrison, 2017-05-24 21:02:52]

This generally LGTM % some nits

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(left):

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:8:
#include <ostream>
shouldn't be removed

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:12:
#include "base/logging.h"
shouldn't be removed

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:14:
#include "base/metrics/histogram_macros.h"
We're still using UMA_HISTOGRAM_ENUMERATION

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(right):

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:31:
const base::Optional<base::TimeDelta>& opt) {
include optional

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:141: //
main frame document. current_merged_page_timing_ contains the most recent
|current_merged_page_timing_| and elsewhere in this comment.

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:144: //
updated. pending_merged_page_timing_ will be copied to
"updated to the client" maybe? There are multiple notions of updating here (this
class gets updates but also dispatches updates).

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:157:
DISALLOW_COPY_AND_ASSIGN(PageLoadMetricsUpdateDispatcher);
#include "base/macros.h"

https://codereview.chromium.org/2904533002/diff/80001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_metrics_util.h (right):

https://codereview.chromium.org/2904533002/diff/80001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_metrics_util.h:2: // Use of this
source code is governed by a BSD-style license that can be
It's kinda weird to have two page_load_metrics_util files in the tree, even if
they are under different directories. Can we rename this one?
page_load_metrics_constants could work.

[Bryan McQuade, 2017-05-25 01:02:37]

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

[commit-bot: I haz the power, 2017-05-25 01:03:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-25 02:30:21]

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(left):

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:8:
#include <ostream>
On 2017/05/24 at 21:02:51, Charlie Harrison wrote:
> shouldn't be removed

done

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:12:
#include "base/logging.h"
On 2017/05/24 at 21:02:51, Charlie Harrison wrote:
> shouldn't be removed

done

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:14:
#include "base/metrics/histogram_macros.h"
On 2017/05/24 at 21:02:51, Charlie Harrison wrote:
> We're still using UMA_HISTOGRAM_ENUMERATION

done

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:141: //
main frame document. current_merged_page_timing_ contains the most recent
On 2017/05/24 at 21:02:51, Charlie Harrison wrote:
> |current_merged_page_timing_| and elsewhere in this comment.

done

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:144: //
updated. pending_merged_page_timing_ will be copied to
On 2017/05/24 at 21:02:52, Charlie Harrison wrote:
> "updated to the client" maybe? There are multiple notions of updating here
(this class gets updates but also dispatches updates).

used 'dispatched to the client'

https://codereview.chromium.org/2904533002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:157:
DISALLOW_COPY_AND_ASSIGN(PageLoadMetricsUpdateDispatcher);
On 2017/05/24 at 21:02:51, Charlie Harrison wrote:
> #include "base/macros.h"

done

https://codereview.chromium.org/2904533002/diff/80001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_metrics_util.h (right):

https://codereview.chromium.org/2904533002/diff/80001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_metrics_util.h:2: // Use of this
source code is governed by a BSD-style license that can be
On 2017/05/24 at 21:02:52, Charlie Harrison wrote:
> It's kinda weird to have two page_load_metrics_util files in the tree, even if
they are under different directories. Can we rename this one?
page_load_metrics_constants could work.

done

[Charlie Harrison, 2017-05-25 03:06:02]

slgtm!

[commit-bot: I haz the power, 2017-05-25 04:34:08]

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

[commit-bot: I haz the power, 2017-05-25 04:34:09]

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...)

[Bryan McQuade, 2017-05-25 12:56:44]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-05-25 12:56:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2904533002/#ps100001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-25 12:56:56]

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-05-25 13:00:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495717004800970,
"parent_rev": "ed245d6ac7b483dc433a44b025d15dd870163a0a", "commit_rev":
"000cea89869e0276d886fe541b3ff050aba9527f"}

[commit-bot: I haz the power, 2017-05-25 13:01:10]

Description was changed from

==========
Factor management of metrics updates into its own class.

With the recent changes to merge paint timings across all frames in a page, we
have learned that first paint timings can arrive out of order.

This change factors IPC update management into its own class, which will
make it easier to refactor this new class to also support buffering of
cross frame paints, should we decide to implement that in a subsequent change.

BUG=725610
==========

to

==========
Factor management of metrics updates into its own class.

With the recent changes to merge paint timings across all frames in a page, we
have learned that first paint timings can arrive out of order.

This change factors IPC update management into its own class, which will
make it easier to refactor this new class to also support buffering of
cross frame paints, should we decide to implement that in a subsequent change.

BUG=725610

Review-Url: https://codereview.chromium.org/2904533002
Cr-Commit-Position: refs/heads/master@{#474635}
Committed:
https://chromium.googlesource.com/chromium/src/+/000cea89869e0276d886fe541b3f...
==========

[commit-bot: I haz the power, 2017-05-25 13:01:11]

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