Chromium Code Reviews
Help | Chromium Project | Sign in
(7)

Issue 2823523003: [Page Load Metrics] PageLoadMetrics Mojofication.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks ago by lpy
Modified:
2 days, 7 hours ago
CC:
Aaron Boodman, abarth-chromium, chromium-reviews, csharrison+watch_chromium.org, darin (slow to review), loading-reviews+metrics_chromium.org, qsr+mojo_chromium.org, speed-metrics-reviews_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Page Load Metrics] PageLoadMetrics Mojofication. This patch 1. Creates a PageLoadMetrics mojo interface; 2. Adds mojo pipeline behind the legacy IPC; 3. Adds Finch Trial check to toggle mojo IPC; 4. Adds browser tests for mojo IPC; The PageLoadMetrics mojo interface defines a UpdateTiming interface that is implemented by MetricsWebContentsObserver. PageLoadMetrics is sensitive to IPC ordering, thus MetricsWebContentsObserver holds a WebContentsFrameBindingSet that is used to bind associated interface together with RenderFrameHost, and it shares the legacy IPC channel so that we make sure we don't break IPC ordering. In PageLoadMetrics browser test, we mock PageLoadMetrics mojo interface, intercept mojo IPC to avoid logging internal error from mojo IPC. BUG=709259, 712915

Patch Set 1 #

Patch Set 2 : Mock PageLoadMetrics in browsertest to intercept mojo IPC. #

Patch Set 3 : Add Finch trial check and rebase #

Total comments: 10

Patch Set 4 : Update #

Total comments: 8

Patch Set 5 : [DO NOT COMMENT]FieldTrial on waterfall #

Patch Set 6 : Add browser test for mojofication. #

Total comments: 37

Patch Set 7 : Addressed comments, remove unnecessary RunUntilIdle, call OnTimingUpdated directly in unit tests #

Total comments: 21

Patch Set 8 : update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1654 lines, -17 lines) Patch
M chrome/browser/chrome_content_browser_manifest_overlay.json View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer.h View 1 2 3 4 5 6 4 chunks +20 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer.cc View 1 2 3 4 5 6 2 chunks +11 lines, -2 lines 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -3 lines 0 comments Download
A chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc View 1 2 3 4 5 6 7 1 chunk +923 lines, -0 lines 0 comments Download
M chrome/common/BUILD.gn View 1 2 3 4 5 3 chunks +13 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/common/page_load_metrics/OWNERS View 1 chunk +6 lines, -0 lines 0 comments Download
A chrome/common/page_load_metrics/page_load_metrics.mojom View 1 chunk +56 lines, -0 lines 0 comments Download
A chrome/common/page_load_metrics/page_load_metrics.typemap View 1 2 3 1 chunk +20 lines, -0 lines 0 comments Download
A chrome/common/page_load_metrics/page_load_metrics_struct_traits.h View 1 chunk +181 lines, -0 lines 0 comments Download
A chrome/common/page_load_metrics/page_load_metrics_struct_traits.cc View 1 chunk +110 lines, -0 lines 0 comments Download
M chrome/renderer/BUILD.gn View 1 2 3 4 5 2 chunks +2 lines, -0 lines 0 comments Download
A chrome/renderer/page_load_metrics/fake_page_load_metrics.h View 1 chunk +72 lines, -0 lines 0 comments Download
A chrome/renderer/page_load_metrics/fake_page_load_metrics.cc View 1 2 1 chunk +50 lines, -0 lines 0 comments Download
A chrome/renderer/page_load_metrics/fake_page_timing_sender.h View 1 chunk +37 lines, -0 lines 0 comments Download
A chrome/renderer/page_load_metrics/fake_page_timing_sender.cc View 1 chunk +23 lines, -0 lines 0 comments Download
M chrome/renderer/page_load_metrics/metrics_render_frame_observer.h View 1 2 3 4 5 6 7 4 chunks +14 lines, -4 lines 0 comments Download
M chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc View 3 chunks +14 lines, -2 lines 0 comments Download
M chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc View 5 chunks +23 lines, -0 lines 0 comments Download
M chrome/renderer/page_load_metrics/page_timing_metrics_sender.h View 3 chunks +4 lines, -0 lines 0 comments Download
M chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc View 1 2 3 chunks +9 lines, -0 lines 0 comments Download
M chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc View 8 chunks +26 lines, -1 line 0 comments Download
A chrome/renderer/page_load_metrics/page_timing_sender.h View 1 chunk +23 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/typemaps.gni View 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 100 (65 generated)
lpy
First round, ptal.
2 weeks ago (2017-04-15 00:26:24 UTC) #4
lpy
I updated the patch with Finch Trial changes that we are going to roll out. ...
1 week, 3 days ago (2017-04-19 00:28:29 UTC) #35
Zhen Wang
In the description, it mentioned that this CL adds the finch trial. I think it ...
1 week, 3 days ago (2017-04-19 18:49:54 UTC) #36
Zhen Wang
https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode589 chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:589: OnTimingUpdated(render_frame_host, timing, metadata); On 2017/04/19 18:49:54, Zhen Wang wrote: ...
1 week, 3 days ago (2017-04-19 18:56:31 UTC) #37
lpy
Talked offline. To clarify how Finch works here. If I understand correctly we don't need ...
1 week, 3 days ago (2017-04-19 21:05:05 UTC) #39
Zhen Wang
https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode49 chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:49: page_load_metrics_->UpdateTiming(timing, metadata); On 2017/04/19 21:05:05, lpy wrote: > On ...
1 week, 3 days ago (2017-04-19 21:52:47 UTC) #42
lpy
On 2017/04/19 21:52:47, Zhen Wang wrote: > Should this and the above IPC one be ...
1 week, 2 days ago (2017-04-20 01:49:24 UTC) #45
lpy
> https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc > File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right): > > https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc#newcode88 > chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88: > ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated( > ...
1 week, 2 days ago (2017-04-20 01:59:18 UTC) #46
lpy
On 2017/04/20 01:49:24, lpy wrote: > One thing I am not sure is that whether ...
1 week, 2 days ago (2017-04-20 05:25:21 UTC) #51
lpy
PTAL. I added extra browser tests for mojofication. The reason I didn't add it to ...
1 week, 2 days ago (2017-04-20 22:09:43 UTC) #59
Bryan McQuade
On 2017/04/20 at 22:09:43, lpy wrote: > PTAL. > > I added extra browser tests ...
5 days, 1 hour ago (2017-04-24 21:31:51 UTC) #63
Bryan McQuade
great, i see we have the feature support in place to finch this already. here ...
5 days, 1 hour ago (2017-04-24 21:50:56 UTC) #64
lpy
+rockot@ for mojo review and mojo related questions. Ken, could you please take a look ...
4 days, 14 hours ago (2017-04-25 09:03:38 UTC) #66
Bryan McQuade
Thanks again for taking this change on! A few more comments / questions. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json File ...
4 days, 7 hours ago (2017-04-25 15:41:14 UTC) #67
Ken Rockot
https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json#newcode34 chrome/browser/chrome_content_browser_manifest_overlay.json:34: "navigation:frame": { On 2017/04/25 at 15:41:13, Bryan McQuade wrote: ...
4 days, 3 hours ago (2017-04-25 19:31:46 UTC) #68
Bryan McQuade
https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode9 chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See comments in page_load_timing.h for details on each ...
3 days, 21 hours ago (2017-04-26 01:55:42 UTC) #69
Ken Rockot
https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode9 chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See comments in page_load_timing.h for details on each ...
3 days, 20 hours ago (2017-04-26 03:08:01 UTC) #70
lpy
Thanks for the feedback, I made a few updates, ptal. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (left): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#oldcode67 ...
3 days, 9 hours ago (2017-04-26 14:18:41 UTC) #73
Bryan McQuade
a couple more questions/comments, thanks! https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42 chrome/common/page_load_metrics/page_load_metrics.mojom:42: DocumentTiming document_timing; In looking ...
3 days, 8 hours ago (2017-04-26 15:24:23 UTC) #76
Ken Rockot
https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42 chrome/common/page_load_metrics/page_load_metrics.mojom:42: DocumentTiming document_timing; On 2017/04/26 at 15:24:22, Bryan McQuade wrote: ...
3 days, 7 hours ago (2017-04-26 15:28:45 UTC) #77
Bryan McQuade
On 2017/04/26 at 15:28:45, rockot wrote: > https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom > File chrome/common/page_load_metrics/page_load_metrics.mojom (right): > > https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42 ...
3 days, 7 hours ago (2017-04-26 15:50:06 UTC) #78
Ken Rockot
On 2017/04/26 at 15:50:06, bmcquade wrote: > On 2017/04/26 at 15:28:45, rockot wrote: > > ...
3 days, 7 hours ago (2017-04-26 15:55:03 UTC) #79
Bryan McQuade
https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode13 chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:13: #include "base/run_loop.h" can remove this include https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc ...
3 days, 7 hours ago (2017-04-26 16:04:34 UTC) #80
Ken Rockot
https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode178 chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178: void AddObserver(PageLoadTimingObserver* observer) { On 2017/04/26 at 16:04:34, Bryan ...
3 days, 7 hours ago (2017-04-26 16:11:42 UTC) #81
Bryan McQuade
On 2017/04/26 at 16:11:42, rockot wrote: > https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc > File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): > > https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode178 ...
3 days, 6 hours ago (2017-04-26 16:31:22 UTC) #82
Bryan McQuade
On 2017/04/26 at 16:31:22, Bryan McQuade wrote: > On 2017/04/26 at 16:11:42, rockot wrote: > ...
3 days, 6 hours ago (2017-04-26 16:32:03 UTC) #83
Ken Rockot
I wouldn't recommend it. Filters are applied pre-deserialization, so you'd have to manually inspect the ...
3 days, 6 hours ago (2017-04-26 16:33:49 UTC) #84
Bryan McQuade
Thanks! I appreciate your helping us to work through this. I'll spend a little time ...
3 days, 6 hours ago (2017-04-26 16:57:03 UTC) #85
Bryan McQuade
https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode178 chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178: void AddObserver(PageLoadTimingObserver* observer) { On 2017/04/26 at 16:11:41, Ken ...
3 days, 2 hours ago (2017-04-26 20:41:51 UTC) #86
lpy
I updated the patch, ptal. https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode13 chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:13: #include "base/run_loop.h" On 2017/04/26 ...
2 days, 12 hours ago (2017-04-27 10:58:35 UTC) #89
Bryan McQuade
Thanks! I landed the refactor to break test deps on IPC. Can you sync and ...
2 days, 7 hours ago (2017-04-27 15:42:32 UTC) #96
Bryan McQuade
On 2017/04/27 at 15:42:32, Bryan McQuade wrote: > Thanks! I landed the refactor to break ...
2 days, 7 hours ago (2017-04-27 16:01:39 UTC) #97
lpy
On 2017/04/27 15:42:32, Bryan McQuade wrote: > Thanks! I landed the refactor to break test ...
2 days, 7 hours ago (2017-04-27 16:07:00 UTC) #98
Ken Rockot
On 2017/04/27 at 15:42:32, bmcquade wrote: > Thanks! I landed the refactor to break test ...
2 days, 7 hours ago (2017-04-27 16:07:10 UTC) #99
Bryan McQuade
2 days, 7 hours ago (2017-04-27 16:25:32 UTC) #100
On 2017/04/27 at 16:07:10, rockot wrote:
> On 2017/04/27 at 15:42:32, bmcquade wrote:
> > Thanks! I landed the refactor to break test deps on IPC. Can you sync and
incorporate that into your change, and then we can continue reviewing? Thanks!
> > 
> >
https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_loa...
> > File chrome/common/page_load_metrics/page_load_metrics.mojom (right):
> > 
> >
https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_loa...
> > chrome/common/page_load_metrics/page_load_metrics.mojom:42: DocumentTiming
document_timing;
> > On 2017/04/27 at 10:58:35, lpy wrote:
> > > On 2017/04/26 20:41:51, Bryan McQuade wrote:
> > > > On 2017/04/26 at 15:28:45, Ken Rockot wrote:
> > > > > On 2017/04/26 at 15:24:22, Bryan McQuade wrote:
> > > > > > In looking at the generated code for this struct in the .mojom.h
file, it
> > > > appears that the DocumentTiming, PaintTiming, ParseTiming,
StyleSheetTiming are
> > > > declared as:
> > > > > > 
> > > > > >   page_load_metrics::DocumentTiming document_timing;
> > > > > >   page_load_metrics::PaintTiming paint_timing;
> > > > > >   page_load_metrics::ParseTiming parse_timing;
> > > > > >   page_load_metrics::StyleSheetTiming style_sheet_timing;
> > > > > > 
> > > > > > which makes me wonder - are we using the
> > > > page_load_metrics::mojom::DocumentTiming and other related structs at
all?
> > > > > 
> > > > > Yes, this is the point of typemapping. You typemap mojom::Foo to Foo,
and we
> > > > generate interfaces that use the more friendly Foo type where
appropriate (e.g.
> > > > struct fields and message arguments.)
> > > > > 
> > > > > The mojom still specifies the precise wire format of the data, and
> > > > StructTraits defines how to efficiently map between the friendly type
and the
> > > > wire format in a safe and reliable way.
> > > > 
> > > > Based on other discussions on the thread, I'm inclined to remove the
structs in
> > > > page_load_timing.h and switch all code over to using the structs that
come from
> > > > the mojom file. Peiyong, what do you think?
> > > 
> > > I prefer not to do it. I don't see clear benefit except saving some typing
and avoiding typemap, while the compiler discovers missing typemap for us. And
it is not scalable/extendable as far as I understand.
> > 
> > Ok. Just so I better understand, if we were writing this code from scratch
and had no legacy code, would you also use the typemapping solution? Or do you
only find it to be sensible because we already have existing structs?
> > 
> > In general I strongly prefer avoiding duplication as much as possible, so I
do worry about this a bit. If someone adds a new field to the mojo message they
would now have to:
> > 1. add it to the mojom file
> > 2. add it to the struct traits
> > 3. add it to the page_load_timing.h
> > 4. add it to the relevant parts of page_load_timing.cc
> > 5. update the IPC serialization code to include the field
> > 
> > That's a lot of code to have to update. If we removed the typemaps, we'd at
least get rid of 3-5.
> 
> I think you're overestimating the maintenance cost here.
> 
> If you were writing this from scratch and had no legacy code, you would not
add IPC serialization code in the first place, so step 5 is not there.
> 
> Meanwhile, without Mojo, if you were writing this from scratch and using
legacy IPC, you would still:
> 
> 1. add it to page_load_timing.h
> 2. add it to the relevant parts of page_load_timing.cc
> 3. update the IPC serialization code to include the field
> 
> Adding to the mojom file is a trivial extra step, and updating StructTraits is
roughly equivalent to updating IPC serialization code. As soon as these types
are no longer used in legacy IPC messages, updating IPC serialization code is no
longer required (i.e. that code will be deleted.)
> 

Sure, I'm not sure anything you said is in disagreement with what I said. If we
are in a world with just mojo, we have:

1. update mojom file
2. update struct traits

If we're in a world where we have canonical struct definitions in the mojom file
and use those in both mojo and legacy IPC, we have:

1. update mojom file
2. update struct traits
3. update IPC serialization (page_load_metrics_messages.h)

If we're in a world where we have mojom and also typemap to legacy structs with
identical definitions, we then have the 1-5 I mentioned above.

This patch as written puts us in the last world where we have 5 steps. I'd like
us to be in the world where we have just 3 steps, and ideally once we confirm
that IPC and Mojo have the same behavior, we move to the first world where we
have just the 2 steps.

FWIW we have had real bugs emerge in this code due to the need to update
multiple code sites (such as IsEmpty() and Equals() implementations) in the
existing legacy structs when adding new fields, so I'm keen to reduce rather
than increase duplication / number of steps needed to add new fields to these
protos. Mojo does make it possible for us to reduce the number of places to
update code once we kill the legacy IPC side, which I see as a win. But I see it
as a loss if we're also typemapping to identical structs - more duplication =
more opportunity for bugs to creep in.

Peiyong points out that we don't have a definition of IsEmpty() in the mojo
structs. I'd be totally happy writing static helpers for this one case though.
And we get Equal() for free in the mojo structs, rather than having to write it
by hand in our typemapped structs, which is another nice improvement.

> > 
> > I'm mildly opposed to the duplication but I think we can do this for now.
I'd like to remove the page_load_timing.h structs in a follow up CL if possible
though (I can do that, or you can). Do you see a reason not to do that?
> > 
> >
https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_l...
> > File chrome/renderer/page_load_metrics/page_timing_sender.h (right):
> > 
> >
https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_l...
> > chrome/renderer/page_load_metrics/page_timing_sender.h:15: class
PageTimingSender {
> > On 2017/04/27 at 10:58:35, lpy wrote:
> > > On 2017/04/26 16:04:34, Bryan McQuade wrote:
> > > > i like this abstraction, but let's do this slightly differently:
> > > > 
> > > > let's define an IPCPageTimingSender, which uses an IPC::Sender*
internally, and
> > > > a MojoPageTimingSender, which uses mojo.
> > > > 
> > > > When instantiating a PageTimingSender, we do something like:
> > > > 
> > > > std::unique_ptr<page_load_metrics::PageTimingSender>
CreateTimingSender() {
> > > >   if
(base::FeatureList::IsEnabled(features::kPageLoadMetricsMojofication))
> > > >     return new MojoPageTimingSender();
> > > >   else
> > > >     return new IPCPageTimingSender();
> > > > }
> > > > Additionally, you can use a MockPageTimingSender in unit tests, and none
of the
> > > > unit tests need to know about mojo or IPC.
> > > 
> > > I assume the owner of PageTimingSender should be
MetricsRenderFrameObserver, since it has all information needed to send two
types of IPC. We will need to intercept the creation of PageTimingSender in this
case, and I don't have a clean way to do it.
> > 
> > I'll have to look at the code more to understand why we can't do this
cleanly. Creating the sender once in MetricsRFO and then providing it to each
PageTimingMetricsSender as it is constructed seems very clean to me. Is that not
possible for some reason?
> > 
> > > 
> > > > then none of the downstream code needs to know anything about IPC or
Mojo. I
> > > > prefer this to having downstream code checking whether a feature is
enabled, as
> > > > you have things currently.
> > > > 
> > > > Over time if we kill the IPC impl I'd be fine with removing this class
and
> > > > inlining the mojo class.
> > > > 
> > > > My only concern is that the name of this class PageTimingSender is very
close to
> > > > the name of PageTimingMetricsSender. Perhaps we should move your new
class's
> > > > definition into PageTimingMetricsSender and call it
> > > > PageTimingMetricsSender::SenderInterface (inner class) or
> > > > PageTimingMetricsSenderEmbedderInterface, or something along those
lines?
> > > 
> > > IMHO, I think the name of PageTimingMetricsSender is misleading, it only
does batching.
> > 
> > I think you are right, with the factoring of the actual sending logic out
into your new interface, PageTimingMetricsSender is no longer responsible for
the sending. Its jobs are:
> > 1. track updates to timing data on a per page load basis (the render frame
observer doesn't understand page lifetimes)
> > 2. batch updates to avoid sending too frequently
> > 
> > Given these, what do you think we should rename PageTimingMetricsSender to?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46