Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2823523003:
[Page Load Metrics] PageLoadMetrics Mojofication. (Closed)
|
Created:
3 years, 8 months ago by lpy Modified:
3 years, 7 months 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. Adds mojo pipeline;
2. Adds Finch Trial check to toggle mojo IPC;
3. Modifies browser tests to be parameterized 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.
BUG=709259, 712915
Review-Url: https://codereview.chromium.org/2823523003
Cr-Commit-Position: refs/heads/master@{#473663}
Committed: https://chromium.googlesource.com/chromium/src/+/a44af8f558766ab8c6b65fff8be4b1ef1a5f903b
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: 23
Patch Set 8 : update #Patch Set 9 : rebase #
Total comments: 8
Patch Set 10 : Add unit test for page_load_metrics_struct_traits #
Total comments: 17
Patch Set 11 : Addressed comments #
Total comments: 2
Patch Set 12 : Update PageTimingSender API #Patch Set 13 : Fix tests #Patch Set 14 : Fix tests #Patch Set 15 : Rebase and merge browsertest. #Patch Set 16 : rebase #
Total comments: 19
Patch Set 17 : addressed comments #
Total comments: 4
Patch Set 18 : Addressed comments and rebased #
Total comments: 1
Patch Set 19 : rebase #Messages
Total messages: 197 (142 generated)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpy@chromium.org changed reviewers: + bmcquade@chromium.org, zhenw@chromium.org
First round, ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Page Load Metrics] PageLoadMetrics Mojofication. This patch 1. Creates a PageLoadMetrics mojo interface; 2. Adds mojo pipeline behind the legacy 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. BUG=709259 ========== to ========== [Page Load Metrics] PageLoadMetrics Mojofication. This patch 1. Creates a PageLoadMetrics mojo interface; 2. Adds mojo pipeline behind the legacy 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Page Load Metrics] PageLoadMetrics Mojofication. This patch 1. Creates a PageLoadMetrics mojo interface; 2. Adds mojo pipeline behind the legacy 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 ========== to ========== [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. 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 ==========
Patchset #3 (id:60001) has been deleted
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...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [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. 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 ========== to ========== [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. 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 ==========
I updated the patch with Finch Trial changes that we are going to roll out. I also added a bug to track Finch Trial progress and updated the design doc. PTAL
In the description, it mentioned that this CL adds the finch trial. I think it is adding the flag to control the feature instead of actually adding the finch trial. I think the complete finch trial will include adding the reporting logic to UMA, etc. In this CL, you can just clarify that in the description. The actual reporting part can be in a followup CL. https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:589: OnTimingUpdated(render_frame_host, timing, metadata); This is using the same timing update function as the old IPC, right? How do we distinguish which data comes from which? https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.typemap (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.typemap:16: "page_load_metrics.mojom.PageLoadMetadata=page_load_metrics::PageLoadMetadata", Why are DocumentTiming, PaintTiming, ParseTiming not included here? https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:49: page_load_metrics_->UpdateTiming(timing, metadata); Why should MetricsRenderFrameObserver care about sending the data? I think this should still be done by PageTimingMetricsSender. Ideally, MetricsRenderFrameObserver does not need to worry about whether it is using old IPC or mojo. PageTimingMetricsSender will take care of it. https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:85: page_timing_sender_->SendTiming(last_timing_, metadata_); So this actually calls to MetricsRenderFrameObserver::SendTiming(). Can we just do the following? page_load_metrics_->UpdateTiming(timing, metadata); We can also just get the mojo end point here. https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_sender.h (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.h:15: class PageTimingSender { Why do we need this interface? SendTiming can be just a function in PageTimingMetricsSender. Or you can just do it in PageTimingMetricsSender::SendNow(). This is related to the above comment about MetricsRenderFrameObserver does not need to do the actual sending.
https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_lo... 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: > This is using the same timing update function as the old IPC, right? How do we > distinguish which data comes from which? I just realized that you may not need to do any special thing on the browser side, if you use the flag to toggle between IPC and Mojo. See the renderer side comment for details. https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88: ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated( We do not need to always send this via IPC. The flag can just toggle this: either send via IPC or Mojo, but not both. So the IPC one is control group and the Mojo one is experiment group. This can simplify the code on the browser side.
lpy@chromium.org changed reviewers: + rkaplow@chromium.org
Talked offline. To clarify how Finch works here. If I understand correctly we don't need to add more histograms. Histograms sent to UMA will be a different configuration, that is to say, when we verify histograms, we basically choose between whether mojo IPC is turnt on or not, and histograms will be grouped by IPC channel. +rkaplow@ for further clarification if needed. https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.typemap (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.typemap:16: "page_load_metrics.mojom.PageLoadMetadata=page_load_metrics::PageLoadMetadata", On 2017/04/19 18:49:54, Zhen Wang wrote: > Why are DocumentTiming, PaintTiming, ParseTiming not included here? Done. https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:49: page_load_metrics_->UpdateTiming(timing, metadata); On 2017/04/19 18:49:54, Zhen Wang wrote: > Why should MetricsRenderFrameObserver care about sending the data? I think this > should still be done by PageTimingMetricsSender. > > Ideally, MetricsRenderFrameObserver does not need to worry about whether it is > using old IPC or mojo. PageTimingMetricsSender will take care of it. MetricsRenderFrameObserver is IPC::Sender, PageTimingMetricsSender only batches page timing, and call MetricsRenderFrameObserver itself to send the timing over IPC. I maintain this logic here by making MetricsRenderFrameObserver to implement a PageTimingSender API. And as it turns out, this is easier to test/verify by separating the logic of sending and batching. If PageTimingMetricsSender is doing the actually sending, we need to fake a sender in MetricsRenderFrameObserver test, and intercept the creation of the sender. See Patch Set #13 in the prototype CL as an example: https://codereview.chromium.org/2731583002/#ps380001 In my opinion, the name of PageTimingMetricsSender is misleading, as it only batches the timing and calls IPC::Sender to send the timing.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_l... 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 2017/04/19 18:49:54, Zhen Wang wrote: > > Why should MetricsRenderFrameObserver care about sending the data? I think > this > > should still be done by PageTimingMetricsSender. > > > > Ideally, MetricsRenderFrameObserver does not need to worry about whether it is > > using old IPC or mojo. PageTimingMetricsSender will take care of it. > > MetricsRenderFrameObserver is IPC::Sender, PageTimingMetricsSender only batches > page timing, and call MetricsRenderFrameObserver itself to send the timing over > IPC. I maintain this logic here by making MetricsRenderFrameObserver to > implement a PageTimingSender API. > > And as it turns out, this is easier to test/verify by separating the logic of > sending and batching. If PageTimingMetricsSender is doing the actually sending, > we need to fake a sender in MetricsRenderFrameObserver test, and intercept the > creation of the sender. See Patch Set #13 in the prototype CL as an example: > https://codereview.chromium.org/2731583002/#ps380001 > > In my opinion, the name of PageTimingMetricsSender is misleading, as it only > batches the timing and calls IPC::Sender to send the timing. Oh, I see. https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:584: void MetricsWebContentsObserver::UpdateTiming( nit: Can you put this close to OnMessageReceived? https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:182: ptr->UpdateTiming(timing, PageLoadMetadata()); Should this and the above IPC one be controlled by the finch trial flag? Is it possible to control that in the test? If that is possible, you may not need to change the expected timing error number in the tests. In addition, why do we get duplicated errors, but not duplicated timing updates (CountUpdatedTimingReported())? So I guess somewhere in the code determines that the 2nd timing update is not reported. Can you give me a pointer on that? It feels strange that we have only one report count when we actually send twice. https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:703: base::TimeDelta::FromMilliseconds(1100); Is there any reason to change this? https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:157: ptr->UpdateTiming(timing, metadata); Same comment as the one in metrics_web_contents_observer_unittest.cc. https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88: ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated( This should not be sent over IPC if the feature is enabled, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/19 21:52:47, Zhen Wang wrote: > Should this and the above IPC one be controlled by the finch trial flag? Is it > possible to control that in the test? Good suggestion, there's a testing config for Finch on waterfall in https://cs.chromium.org/chromium/src/testing/variations/fieldtrial_testing_co..., for details see the doc: https://chromium.googlesource.com/chromium/src/+/master/testing/variations/. Basically it will run perf tests, browser tests with Finch configuration, but not unit tests when I run the tests locally. One thing I am not sure is that whether waterfall will run the tests for both cases, or just for mojo-enabled case, I will upload a patch set to verify it. If waterfall doesn't run the testing for both cases, then we will lose test coverage for legacy IPC on browser test. > If that is possible, you may not need to change the expected timing error number > in the tests. > > In addition, why do we get duplicated errors, but not duplicated timing updates > (CountUpdatedTimingReported())? So I guess somewhere in the code determines that > the 2nd timing update is not reported. Can you give me a pointer on that? It > feels strange that we have only one report count when we actually send twice. The 2nd timing update is dropped here: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... This is expected, only when two page timing are the same(which they should be), will the latter one be dropped. So if we receive two page timing for each simulation, then I think it is wrong. https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:584: void MetricsWebContentsObserver::UpdateTiming( On 2017/04/19 21:52:47, Zhen Wang wrote: > nit: Can you put this close to OnMessageReceived? It's much closer to OnTimingUpdated I think. OnMessageReceived receives raw IPC signal, but UpdateTiming doesn't, think about it as just a function call. https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:703: base::TimeDelta::FromMilliseconds(1100); On 2017/04/19 21:52:47, Zhen Wang wrote: > Is there any reason to change this? The overhead of SimulateTimingUpdate increases a little bit. https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right): https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88: ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated( On 2017/04/19 21:52:47, Zhen Wang wrote: > This should not be sent over IPC if the feature is enabled, right? Yes.
> https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_l... > File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right): > > https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_l... > chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88: > ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated( > On 2017/04/19 21:52:47, Zhen Wang wrote: > > This should not be sent over IPC if the feature is enabled, right? > > Yes. I mean, no, it should not be sent.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/20 01:49:24, lpy wrote: > One thing I am not sure is that whether waterfall will run the tests for both > cases, or just for mojo-enabled case, I will upload a patch set to verify it. If > waterfall doesn't run the testing for both cases, then we will lose test > coverage for legacy IPC on browser test. Looks like it doesn't run the tests for both config(enable mojo IPC and disable mojo IPC)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. I added extra browser tests for mojofication. The reason I didn't add it to exist browser test set, is because we need to set the feature state before the render process is created, in order for it to inherit the feature state from the browser process. That is to say, we can't config Finch Trial for individual test cases, we can only turn it on for a browser test set. And please ignore Patch Set #5.
Description was changed from ========== [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. 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 ========== to ========== [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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/20 at 22:09:43, lpy wrote: > PTAL. > > I added extra browser tests for mojofication. > > The reason I didn't add it to exist browser test set, is because we need to set the feature state before the render process is created, in order for it to inherit the feature state from the browser process. That is to say, we can't config Finch Trial for individual test cases, we can only turn it on for a browser test set. > > And please ignore Patch Set #5. Thanks! I will take a look at this soon. Are we planning to factor the field trial logic into this change as well?
great, i see we have the feature support in place to finch this already. here are some early comments / questions, thanks! https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (left): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:67: // static thanks! not sure how this crept in here... https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.h:189: extern const base::Feature kPageLoadMetricsMojofication; does this need to be declared in chrome_features.h? Can it instead be declared in the class that wants to detect whether this feature is enabled/disabled? that's the more typical way I've seen features like this declared. see for example kDelayNavigationFeature in https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experim... https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See comments in page_load_timing.h for details on each field. interesting, do all types sent over mojo have to be declared in a mojom file? we can't for instance include page_load_timing.h? or convert page_load_timing.h to a mojom file which just declares structs, and then include it here, to avoid the duplication? https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:54: interface PageLoadMetrics { though I don't have a better name for this service, I find the name PageLoadMetrics a little confusing. That name doesn't sound like a service name that you execute actions on but rather a passive data structure that holds metrics data. I'm slightly inclined to call this something like PageLoadMetricsChannel to better describe that it's a communication channel that the PageLoadMetrics are sent over, but I'm not sure if that's the best name or not. Can you think of a name to better describe that this is a service interface that page load metrics data is sent over? https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17: struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView, interesting, this is a lot of boilerplate that seems like maybe could be automatically emitted by a mojo compiler. i guess that's not currently possible but it does seem like this is a fine area for bugs to be introduced (e.g. we copy/paste a struct trait and reference the wrong field). do you know if there's any current way or future plan to trim down the amount of StructTraits code that needs to be written? i notice in this much older iteration to move this system to mojo https://codereview.chromium.org/2056153002/ there was no such StructTraits. is it possible to avoid StructTraits as in that change for simple types that don't need any special behavior?
lpy@chromium.org changed reviewers: + rockot@chromium.org
+rockot@ for mojo review and mojo related questions. Ken, could you please take a look at the patch and mojo questions raised by Bryan? Thank you. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.h:189: extern const base::Feature kPageLoadMetricsMojofication; On 2017/04/24 21:50:55, Bryan McQuade wrote: > does this need to be declared in chrome_features.h? Can it instead be declared > in the class that wants to detect whether this feature is enabled/disabled? > that's the more typical way I've seen features like this declared. see for > example kDelayNavigationFeature in > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experim... It's declared here because both renderer/ and browser/ need to use it, renderer checks to decide which IPC it should use to send data, and browser uses it for browser test only. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See comments in page_load_timing.h for details on each field. On 2017/04/24 21:50:55, Bryan McQuade wrote: > interesting, do all types sent over mojo have to be declared in a mojom file? we > can't for instance include page_load_timing.h? or convert page_load_timing.h to > a mojom file which just declares structs, and then include it here, to avoid the > duplication? I think that's how mojo works since I am following mojo wiki, that's also what legacy IPC used to require. +rockot@ for further answer. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17: struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView, On 2017/04/24 21:50:56, Bryan McQuade wrote: > interesting, this is a lot of boilerplate that seems like maybe could be > automatically emitted by a mojo compiler. i guess that's not currently possible > but it does seem like this is a fine area for bugs to be introduced (e.g. we > copy/paste a struct trait and reference the wrong field). do you know if there's > any current way or future plan to trim down the amount of StructTraits code that > needs to be written? > > i notice in this much older iteration to move this system to mojo > https://codereview.chromium.org/2056153002/ there was no such StructTraits. is > it possible to avoid StructTraits as in that change for simple types that don't > need any special behavior? I was following the mojo wiki so I am not sure if there's any better approach or future plan. + rockot@ for further answer.
Thanks again for taking this change on! A few more comments / questions. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_manifest_overlay.json:34: "navigation:frame": { so i can better understand & review the code, can you explain why we're under 'navigation:frame'? What does that mean & why does page load metrics belong underneath it? https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:114: friend class FakePageLoadMetrics; i'm a bit uncomfortable about this - i need to see why the helper browser test class needs access to private state, but i'd like to try to avoid this need if at all possible. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:176: observer_->page_load_metrics_binding_for_testing() I think I'm inclined to change the SimulateTimingUpdate impl to just directly invoke OnTimingUpdated, rather than going through the IPC dispatch patch, and not having to have the test invoke / know about both IPC and Mojo. This is a unit test so it shouldn't really have to know about IPC or Mojo dispatch. While it's nice to exercise slightly more code with IPC dispatch, it adds the complexity of now also having to invoke the mojo path here, having to expose the page_load_metrics_binding_for_testing() method on the observer, and we do also have the browsertest for IPC/Mojo dispatch coverage so I feel it is a little redundant to also do that here, given the complexity it now adds. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:148: observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated( same thing here - let's invoke OnTimingUpdated directly and avoid the need to know about mojo in these unit tests. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. on first glance i'd really like to see if we can have a single browsertest file for both mojo and ipc flows, rather than 2 separate. with them split, we have to fix bugs in both places, add new tests in both places, etc. ideally the duplication goes away soonish but it's best not to have the duplication at all if possible. i'll spend some more time thinking about this & see what we can do here. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:137: base::RunLoop().RunUntilIdle(); can we add a comment to explain why this is needed? i usually like to add a comment explaining why calls to RunUntilIdle() are needed, since it can often be subtle & helps future modifiers of the code to understand the specific reason. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:173: metrics_observer_->page_load_metrics_binding_for_testing() it seems odd that we need to wire this frame mapping up in a browser test. a browser test should be managing the message routing between frames correctly on its own. why is this needed? https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:179: void AddObserver(PageLoadTimingObserver* observer) { hmm, this doesn't seem like it should be here. i'm concerned that we're having to fake out so much behavior here. browser tests are supposed to exercise the real production codepath. by stubbing this out we're mitigating much of the value of the browser test. can we spend some time to see how to get the browser test working w/o the need for a FakePageLoadMetrics object? https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.h:189: extern const base::Feature kPageLoadMetricsMojofication; On 2017/04/25 at 09:03:37, lpy wrote: > On 2017/04/24 21:50:55, Bryan McQuade wrote: > > does this need to be declared in chrome_features.h? Can it instead be declared > > in the class that wants to detect whether this feature is enabled/disabled? > > that's the more typical way I've seen features like this declared. see for > > example kDelayNavigationFeature in > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experim... > > It's declared here because both renderer/ and browser/ need to use it, renderer checks to decide which IPC it should use to send data, and browser uses it for browser test only. Ah, I see, thanks! I'm ok with putting this here, though I'm also open to moving it somewhere under chrome/common/page_load_metrics/ if there's a logical place there.
https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_manifest_overlay.json:34: "navigation:frame": { On 2017/04/25 at 15:41:13, Bryan McQuade wrote: > so i can better understand & review the code, can you explain why we're under 'navigation:frame'? What does that mean & why does page load metrics belong underneath it? These are capabilities exposed to render frames specifically. i.e. there are interfaces which can be requested by individual frames, and interfaces which can be requested by the render process without any frame context. navigation:frame capabilities constitute the former. It's a (IMHO somewhat confusing) detail of the way we scope capabilities in service manifests today, but this is correct. We're looking into making this less confusing and subsequently better documented. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:176: observer_->page_load_metrics_binding_for_testing() On 2017/04/25 at 15:41:14, Bryan McQuade wrote: > I think I'm inclined to change the SimulateTimingUpdate impl to just directly invoke OnTimingUpdated, rather than going through the IPC dispatch patch, and not having to have the test invoke / know about both IPC and Mojo. This is a unit test so it shouldn't really have to know about IPC or Mojo dispatch. While it's nice to exercise slightly more code with IPC dispatch, it adds the complexity of now also having to invoke the mojo path here, having to expose the page_load_metrics_binding_for_testing() method on the observer, and we do also have the browsertest for IPC/Mojo dispatch coverage so I feel it is a little redundant to also do that here, given the complexity it now adds. +1 - I prefer to avoid going through IPC boundaries when unit-testing if possible. Seems like unnecessary complexity. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:137: base::RunLoop().RunUntilIdle(); On 2017/04/25 at 15:41:14, Bryan McQuade wrote: > can we add a comment to explain why this is needed? i usually like to add a comment explaining why calls to RunUntilIdle() are needed, since it can often be subtle & helps future modifiers of the code to understand the specific reason. I would in fact prefer to never use RunUntilIdle(). It's **almost always** wrong. The correct thing to do is to capture a RunLoop's QuitClosure() and explicitly invoke it when a specific, interesting event occurs, and simply Run() the loop. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:173: metrics_observer_->page_load_metrics_binding_for_testing() On 2017/04/25 at 15:41:14, Bryan McQuade wrote: > it seems odd that we need to wire this frame mapping up in a browser test. a browser test should be managing the message routing between frames correctly on its own. why is this needed? This is true. It should only be necessary to inject a target frame in unit test code when a message isn't actually arriving from a renderer. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See comments in page_load_timing.h for details on each field. On 2017/04/25 at 09:03:37, lpy wrote: > On 2017/04/24 21:50:55, Bryan McQuade wrote: > > interesting, do all types sent over mojo have to be declared in a mojom file? we > > can't for instance include page_load_timing.h? or convert page_load_timing.h to > > a mojom file which just declares structs, and then include it here, to avoid the > > duplication? > > I think that's how mojo works since I am following mojo wiki, that's also what legacy IPC used to require. > > +rockot@ for further answer. Absolutely yes, everything sent over Mojo has to be defined in a mojom file. Otherwise bindings would be limited to C++. We do support C++ typemapping[1] which allows you to teach the bindings generator how to serialize some arbitrary C++ type as a mojom struct and then generate bindings which use that type in any relevant generated C++ interfaces. We also support some trickery[2] for exploiting existing legacy IPC traits definitions to avoid duplicate mojom definitions for existing IPC structs. Use of this feature should generally be discouraged though and will not be supported indefinitely. [1] https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi... [2] https://chromium.googlesource.com/chromium/src/+/master/ipc/README.md#Using-L... https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:54: interface PageLoadMetrics { On 2017/04/24 at 21:50:55, Bryan McQuade wrote: > though I don't have a better name for this service, I find the name PageLoadMetrics a little confusing. That name doesn't sound like a service name that you execute actions on but rather a passive data structure that holds metrics data. I'm slightly inclined to call this something like PageLoadMetricsChannel to better describe that it's a communication channel that the PageLoadMetrics are sent over, but I'm not sure if that's the best name or not. Can you think of a name to better describe that this is a service interface that page load metrics data is sent over? terminology nit: it's not a service. All mojo interfaces are by convention in a mojom subnamespace, so this will always be read as mojom::PageLoadMetrics in C++ code. That should be sufficient to convey the detail that it's an IPC interface. I do not think it is correct for people to start adding nouns like "Channel" or "Pipe" or whatever to interface names. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17: struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView, On 2017/04/25 at 09:03:37, lpy wrote: > On 2017/04/24 21:50:56, Bryan McQuade wrote: > > interesting, this is a lot of boilerplate that seems like maybe could be > > automatically emitted by a mojo compiler. i guess that's not currently possible > > but it does seem like this is a fine area for bugs to be introduced (e.g. we > > copy/paste a struct trait and reference the wrong field). do you know if there's > > any current way or future plan to trim down the amount of StructTraits code that > > needs to be written? > > > > i notice in this much older iteration to move this system to mojo > > https://codereview.chromium.org/2056153002/ there was no such StructTraits. is > > it possible to avoid StructTraits as in that change for simple types that don't > > need any special behavior? > > I was following the mojo wiki so I am not sure if there's any better approach or future plan. > + rockot@ for further answer. I don't think there is any reasonable way to automate StructTraits generation. It's almost certainly not worth the complexity to do it well and cover all the weird possible edge cases.
https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See comments in page_load_timing.h for details on each field. On 2017/04/25 at 19:31:46, Ken Rockot wrote: > On 2017/04/25 at 09:03:37, lpy wrote: > > On 2017/04/24 21:50:55, Bryan McQuade wrote: > > > interesting, do all types sent over mojo have to be declared in a mojom file? we > > > can't for instance include page_load_timing.h? or convert page_load_timing.h to > > > a mojom file which just declares structs, and then include it here, to avoid the > > > duplication? > > > > I think that's how mojo works since I am following mojo wiki, that's also what legacy IPC used to require. > > > > +rockot@ for further answer. > > Absolutely yes, everything sent over Mojo has to be defined in a mojom file. Otherwise bindings would be limited to C++. > > We do support C++ typemapping[1] which allows you to teach the bindings generator how to serialize some arbitrary C++ type as a mojom struct and then generate bindings which use that type in any relevant generated C++ interfaces. > > We also support some trickery[2] for exploiting existing legacy IPC traits definitions to avoid duplicate mojom definitions for existing IPC structs. Use of this feature should generally be discouraged though and will not be supported indefinitely. > > [1] https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi... > [2] https://chromium.googlesource.com/chromium/src/+/master/ipc/README.md#Using-L... Ah, ok, thanks! Is it possible to declare the structs in a mojom file and then use them for the IPC code flow as well? That way we have only one canonical place that we define the structs (mojom file) rather than having a mojom file and a separate header for the IPC variant. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:54: interface PageLoadMetrics { On 2017/04/25 at 19:31:46, Ken Rockot wrote: > On 2017/04/24 at 21:50:55, Bryan McQuade wrote: > > though I don't have a better name for this service, I find the name PageLoadMetrics a little confusing. That name doesn't sound like a service name that you execute actions on but rather a passive data structure that holds metrics data. I'm slightly inclined to call this something like PageLoadMetricsChannel to better describe that it's a communication channel that the PageLoadMetrics are sent over, but I'm not sure if that's the best name or not. Can you think of a name to better describe that this is a service interface that page load metrics data is sent over? > > terminology nit: it's not a service. > > All mojo interfaces are by convention in a mojom subnamespace, so this will always be read as mojom::PageLoadMetrics in C++ code. That should be sufficient to convey the detail that it's an IPC interface. I do not think it is correct for people to start adding nouns like "Channel" or "Pipe" or whatever to interface names. the mojom:: namespace does help to clarify the class's purpose here - sounds fine to me thanks. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17: struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView, On 2017/04/25 at 19:31:46, Ken Rockot wrote: > On 2017/04/25 at 09:03:37, lpy wrote: > > On 2017/04/24 21:50:56, Bryan McQuade wrote: > > > interesting, this is a lot of boilerplate that seems like maybe could be > > > automatically emitted by a mojo compiler. i guess that's not currently possible > > > but it does seem like this is a fine area for bugs to be introduced (e.g. we > > > copy/paste a struct trait and reference the wrong field). do you know if there's > > > any current way or future plan to trim down the amount of StructTraits code that > > > needs to be written? > > > > > > i notice in this much older iteration to move this system to mojo > > > https://codereview.chromium.org/2056153002/ there was no such StructTraits. is > > > it possible to avoid StructTraits as in that change for simple types that don't > > > need any special behavior? > > > > I was following the mojo wiki so I am not sure if there's any better approach or future plan. > > + rockot@ for further answer. > > I don't think there is any reasonable way to automate StructTraits generation. It's almost certainly not worth the complexity to do it well and cover all the weird possible edge cases. ah, ok. what are all the static methods that just return the member variable needed for? those do seem like they could be autogenerated. the Read() methods seem like they probably need someone to implement them though.
https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See comments in page_load_timing.h for details on each field. On 2017/04/26 at 01:55:42, Bryan McQuade wrote: > On 2017/04/25 at 19:31:46, Ken Rockot wrote: > > On 2017/04/25 at 09:03:37, lpy wrote: > > > On 2017/04/24 21:50:55, Bryan McQuade wrote: > > > > interesting, do all types sent over mojo have to be declared in a mojom file? we > > > > can't for instance include page_load_timing.h? or convert page_load_timing.h to > > > > a mojom file which just declares structs, and then include it here, to avoid the > > > > duplication? > > > > > > I think that's how mojo works since I am following mojo wiki, that's also what legacy IPC used to require. > > > > > > +rockot@ for further answer. > > > > Absolutely yes, everything sent over Mojo has to be defined in a mojom file. Otherwise bindings would be limited to C++. > > > > We do support C++ typemapping[1] which allows you to teach the bindings generator how to serialize some arbitrary C++ type as a mojom struct and then generate bindings which use that type in any relevant generated C++ interfaces. > > > > We also support some trickery[2] for exploiting existing legacy IPC traits definitions to avoid duplicate mojom definitions for existing IPC structs. Use of this feature should generally be discouraged though and will not be supported indefinitely. > > > > [1] https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi... > > [2] https://chromium.googlesource.com/chromium/src/+/master/ipc/README.md#Using-L... > > Ah, ok, thanks! Is it possible to declare the structs in a mojom file and then use them for the IPC code flow as well? That way we have only one canonical place that we define the structs (mojom file) rather than having a mojom file and a separate header for the IPC variant. No, and we won't be supporting that. I mean, you could always call mojom::Foo::Serialize(foo) to get a std::vector<uint8_t> of serialized bytes and send that over legacy IPC, but that's pretty awful. If you have a structure which is already used by legacy IPC code though, you can define a mojom alias (no duplicate definition in Mojom beyond a simple name declaration) and the bindings will repurpose the existing serialization format. Of course this limits the type to use with C++ bindings, but that kind of goes without saying if you plan to use legacy IPC serialization. This is the feature outlined by [2] in my previous reply. Of course if we're talking about a data structure which is not currently used in any legacy IPC messages, the point is moot because we should not be introducing any new legacy IPC code without very compelling reasons. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17: struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView, On 2017/04/26 at 01:55:42, Bryan McQuade wrote: > On 2017/04/25 at 19:31:46, Ken Rockot wrote: > > On 2017/04/25 at 09:03:37, lpy wrote: > > > On 2017/04/24 21:50:56, Bryan McQuade wrote: > > > > interesting, this is a lot of boilerplate that seems like maybe could be > > > > automatically emitted by a mojo compiler. i guess that's not currently possible > > > > but it does seem like this is a fine area for bugs to be introduced (e.g. we > > > > copy/paste a struct trait and reference the wrong field). do you know if there's > > > > any current way or future plan to trim down the amount of StructTraits code that > > > > needs to be written? > > > > > > > > i notice in this much older iteration to move this system to mojo > > > > https://codereview.chromium.org/2056153002/ there was no such StructTraits. is > > > > it possible to avoid StructTraits as in that change for simple types that don't > > > > need any special behavior? > > > > > > I was following the mojo wiki so I am not sure if there's any better approach or future plan. > > > + rockot@ for further answer. > > > > I don't think there is any reasonable way to automate StructTraits generation. It's almost certainly not worth the complexity to do it well and cover all the weird possible edge cases. > > ah, ok. what are all the static methods that just return the member variable needed for? those do seem like they could be autogenerated. the Read() methods seem like they probably need someone to implement them though. This is documented at [1], but the gist is, you have a static method per mojom field which defines a (fast, i.e., copy-free) way of mapping the object's data into something the serializer knows how to serialize. They cannot be autogenerated. It might seem that way for the simplest cases, but only for the simplest cases. For fields that are POD types for example this is trivial, and you could imagine us adding a helper macro like: #define DEFINE_TRIVIAL_STRUCT_TRAITS_FIELD(type, name) \ static decltype(std::declval<type>().name) name(const type& value) { \ return value.name; } so then you could write DEFINE_TRIVIAL_STRUCT_TRAITS_FIELD(page_load_metrics::DocumentTiming, first_layout); DEFINE_TRIVIAL_STRUCT_TRAITS_FIELD(page_load_metrics::DocumentTiming, load_event_start); etc, instead of static base::Optional<base::TimeDelta> first_layout( const page_load_metrics::DocumentTiming& value) { return value.first_layout; } but it saves very little typing and hides behind a magic macro what would otherwise be easily understandable code. Keep in mind that many field accessors are not trivial, and do not return the exact same type as the native type's field (e.g. static std::string accessors typically return a base::StringPiece); or they don't correspond verbatim to an existing field on the object (e.g. maybe they're accessed by some other public accessor method or for various reasons don't have the same name as the mojom field). In some cases they need to be mapped to some other serializer-compatible view type (e.g. maybe you use mojo::CArray<uint8_t> to serialize some specialized buffer type as a mojom array<uint8>), and so on. [1] https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi...
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the feedback, I made a few updates, ptal. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (left): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:67: // static On 2017/04/24 21:50:55, Bryan McQuade wrote: > thanks! not sure how this crept in here... Acknowledged. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:176: observer_->page_load_metrics_binding_for_testing() On 2017/04/25 19:31:46, Ken Rockot wrote: > On 2017/04/25 at 15:41:14, Bryan McQuade wrote: > > I think I'm inclined to change the SimulateTimingUpdate impl to just directly > invoke OnTimingUpdated, rather than going through the IPC dispatch patch, and > not having to have the test invoke / know about both IPC and Mojo. This is a > unit test so it shouldn't really have to know about IPC or Mojo dispatch. While > it's nice to exercise slightly more code with IPC dispatch, it adds the > complexity of now also having to invoke the mojo path here, having to expose the > page_load_metrics_binding_for_testing() method on the observer, and we do also > have the browsertest for IPC/Mojo dispatch coverage so I feel it is a little > redundant to also do that here, given the complexity it now adds. > > +1 - I prefer to avoid going through IPC boundaries when unit-testing if > possible. Seems like unnecessary complexity. Done. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:148: observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated( On 2017/04/25 15:41:14, Bryan McQuade wrote: > same thing here - let's invoke OnTimingUpdated directly and avoid the need to > know about mojo in these unit tests. Done. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/04/25 15:41:14, Bryan McQuade wrote: > on first glance i'd really like to see if we can have a single browsertest file > for both mojo and ipc flows, rather than 2 separate. with them split, we have to > fix bugs in both places, add new tests in both places, etc. ideally the > duplication goes away soonish but it's best not to have the duplication at all > if possible. i'll spend some more time thinking about this & see what we can do > here. The reason I didn't add it to exist browser test set, is because we need to set the feature state before the render process is created, in order for it to inherit the feature state from the browser process. That is to say, we can't config Finch Trial for individual test cases, we can only turn it on for a browser test set. Please also see SetUpCommandLine in PageLoadMetricsMojoficationBrowserTest. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:137: base::RunLoop().RunUntilIdle(); On 2017/04/25 19:31:46, Ken Rockot wrote: > On 2017/04/25 at 15:41:14, Bryan McQuade wrote: > > can we add a comment to explain why this is needed? i usually like to add a > comment explaining why calls to RunUntilIdle() are needed, since it can often be > subtle & helps future modifiers of the code to understand the specific reason. > > I would in fact prefer to never use RunUntilIdle(). It's **almost always** > wrong. > > The correct thing to do is to capture a RunLoop's QuitClosure() and explicitly > invoke it when a specific, interesting event occurs, and simply Run() the loop. Done. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:173: metrics_observer_->page_load_metrics_binding_for_testing() On 2017/04/25 19:31:46, Ken Rockot wrote: > On 2017/04/25 at 15:41:14, Bryan McQuade wrote: > > it seems odd that we need to wire this frame mapping up in a browser test. a > browser test should be managing the message routing between frames correctly on > its own. why is this needed? > > This is true. It should only be necessary to inject a target frame in unit test > code when a message isn't actually arriving from a renderer. We are re-directing the mojo incoming request to the FakePageLoadMetrics by using FakePageLoadMetricsBinder, so the MetricsWebContentsObserver won't get called, but we still want messages to be delivered fully in order to do histogram check, so we need to manually bind render frame host context to the web contents frame binding set. And call it manually. https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:179: void AddObserver(PageLoadTimingObserver* observer) { On 2017/04/25 15:41:14, Bryan McQuade wrote: > hmm, this doesn't seem like it should be here. i'm concerned that we're having > to fake out so much behavior here. browser tests are supposed to exercise the > real production codepath. by stubbing this out we're mitigating much of the > value of the browser test. can we spend some time to see how to get the browser > test working w/o the need for a FakePageLoadMetrics object? The basic idea is the same as the old browser test, which is blocking code execution until IPC is fully delivered. Unlike legacy IPC which we can use filter to filter out specific IPC messages, mojo doesn't have similar mechanism as far as I understand. Thus we fake an implementation so that mojo IPC sent from renderer would arrive to the fake implementation, and we manually dispatch to the original one and do whatever we want, which is unblocking execution here. https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.h:189: extern const base::Feature kPageLoadMetricsMojofication; On 2017/04/25 15:41:14, Bryan McQuade wrote: > On 2017/04/25 at 09:03:37, lpy wrote: > > On 2017/04/24 21:50:55, Bryan McQuade wrote: > > > does this need to be declared in chrome_features.h? Can it instead be > declared > > > in the class that wants to detect whether this feature is enabled/disabled? > > > that's the more typical way I've seen features like this declared. see for > > > example kDelayNavigationFeature in > > > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experim... > > > > It's declared here because both renderer/ and browser/ need to use it, > renderer checks to decide which IPC it should use to send data, and browser uses > it for browser test only. > > Ah, I see, thanks! I'm ok with putting this here, though I'm also open to moving > it somewhere under chrome/common/page_load_metrics/ if there's a logical place > there. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
a couple more questions/comments, 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; 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? https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:55: UpdateTiming(PageLoadTiming page_load_timing, PageLoadMetadata page_load_metadata); similarly, the generated code appears to be using page_load_metrics::PageLoadTiming rather than the mojom version you declare in this file. It may just be that I don't fully understand how mojo works, but on first glance it doesn't seem like we're using the types you declared in this mojom file. If I'm misunderstanding please let me know - I'm still trying to learn how mojo works. 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/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.
On 2017/04/26 at 15:28:45, rockot wrote: > 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/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. Got it, makes sense, thanks! In our case, since the page_load_metrics::mojom:: versions of the structs are identical to the typemapped structs, is there any reason for us to not use the mojom versions directly, and get rid of the structs we are typemapping to? Or are teams encouraged to always typemap even if the struct definitions are identical?
On 2017/04/26 at 15:50:06, bmcquade wrote: > On 2017/04/26 at 15:28:45, rockot wrote: > > 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/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. > > Got it, makes sense, thanks! In our case, since the page_load_metrics::mojom:: versions of the structs are identical to the typemapped structs, is there any reason for us to not use the mojom versions directly, and get rid of the structs we are typemapping to? Or are teams encouraged to always typemap even if the struct definitions are identical? It's entirely a matter of preference IMHO. I would love it if more code just used mojom types directly, but it's often not feasible for one of two reasons: 1. You want your common types to have helper methods which cannot be autogenerated - e.g., gfx::Rect is a very simple data structure but also has tons of useful helper methods for common rectangle operations. 2. You want any kind of universal custom validation when deserializing the type - e.g., GURL rejects in its StructTraits' Read() if the incoming string data exceeds a maximum length.
https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... 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_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:149: observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated( let's switch this to invoke OnTimingUpdated directly, rather than invoking the IPC and Mojo codepaths. https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:227: if (!mock_page_load_metrics_) { just so i better understand, why is this needed in this browsertest if we've written a separate browsertest for exercising the mojo codepath? fwiw i am still hopeful we can merge the 2 browsertests into one, but in the absence of a merge it seems odd that we have to do so much mojo setup for a browsertest that isn't actually doing anything with mojo https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178: void AddObserver(PageLoadTimingObserver* observer) { ah, I see now why you have this class and this method. I still worry about having to stub/wrap so much of the mojo code in a browser test. Ideally we'd be able to accomplish the goal of waiting for a mojo message to arrive without having to wrap the mojom::PageLoadMetrics like this. Ken, the test cases in this file to wait until a message received over the mojo channel meets a certain constraint before allowing the test to continue. Is there a canonical/best way for browser tests to do this, without having to wrap the mojom::PageLoadMetrics object like this? https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:62: mojom::PageLoadMetricsAssociatedPtr page_load_metrics_; Let's add a short comment explaining why we use a PageLoadMetricsAssociatedPtr here (rather than a plain PageLoadMetrics). 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 { 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. 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?
https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... 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 McQuade wrote: > ah, I see now why you have this class and this method. I still worry about having to stub/wrap so much of the mojo code in a browser test. Ideally we'd be able to accomplish the goal of waiting for a mojo message to arrive without having to wrap the mojom::PageLoadMetrics like this. > > Ken, the test cases in this file to wait until a message received over the mojo channel meets a certain constraint before allowing the test to continue. Is there a canonical/best way for browser tests to do this, without having to wrap the mojom::PageLoadMetrics object like this? The messages are always going to be dispatched to some implementation of mojom::PageLoadMetrics. You can either do this (dispatch to a test-only impl) or add testing hooks to the non-test impl. I don't see much room for other options if a test needs to inspect incoming messages.
On 2017/04/26 at 16:11:42, rockot wrote: > https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): > > https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... > 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 McQuade wrote: > > ah, I see now why you have this class and this method. I still worry about having to stub/wrap so much of the mojo code in a browser test. Ideally we'd be able to accomplish the goal of waiting for a mojo message to arrive without having to wrap the mojom::PageLoadMetrics like this. > > > > Ken, the test cases in this file to wait until a message received over the mojo channel meets a certain constraint before allowing the test to continue. Is there a canonical/best way for browser tests to do this, without having to wrap the mojom::PageLoadMetrics object like this? > > The messages are always going to be dispatched to some implementation of mojom::PageLoadMetrics. You can either do this (dispatch to a test-only impl) or add testing hooks to the non-test impl. I don't see much room for other options if a test needs to inspect incoming messages. Makes sense, thanks! Looking around the mojo code a bit, I see the MessageReceiver interface. Is it possible to use Binding::AddFilter() or something similar to observe messages as they are being processed? That would allow us to avoid wrapping the PageLoadMetrics itself, which seems desirable to avoid if possible.
On 2017/04/26 at 16:31:22, Bryan McQuade wrote: > On 2017/04/26 at 16:11:42, rockot wrote: > > https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... > > File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): > > > > https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... > > 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 McQuade wrote: > > > ah, I see now why you have this class and this method. I still worry about having to stub/wrap so much of the mojo code in a browser test. Ideally we'd be able to accomplish the goal of waiting for a mojo message to arrive without having to wrap the mojom::PageLoadMetrics like this. > > > > > > Ken, the test cases in this file to wait until a message received over the mojo channel meets a certain constraint before allowing the test to continue. Is there a canonical/best way for browser tests to do this, without having to wrap the mojom::PageLoadMetrics object like this? > > > > The messages are always going to be dispatched to some implementation of mojom::PageLoadMetrics. You can either do this (dispatch to a test-only impl) or add testing hooks to the non-test impl. I don't see much room for other options if a test needs to inspect incoming messages. > > Makes sense, thanks! Looking around the mojo code a bit, I see the MessageReceiver interface. Is it possible to use Binding::AddFilter() or something similar to observe messages as they are being processed? That would allow us to avoid wrapping the PageLoadMetrics itself, which seems desirable to avoid if possible. (and just to clarify, we'd only ever do this in test code - never in real production code)
I wouldn't recommend it. Filters are applied pre-deserialization, so you'd have to manually inspect the message using DataView objects. I don't think this will end up being better than what you're doing now. On Wed, Apr 26, 2017 at 9:32 AM, <bmcquade@chromium.org> wrote: > On 2017/04/26 at 16:31:22, Bryan McQuade wrote: > > 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 > > > > 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 McQuade wrote: > > > > ah, I see now why you have this class and this method. I still worry > about > having to stub/wrap so much of the mojo code in a browser test. Ideally > we'd be > able to accomplish the goal of waiting for a mojo message to arrive without > having to wrap the mojom::PageLoadMetrics like this. > > > > > > > > Ken, the test cases in this file to wait until a message received > over the > mojo channel meets a certain constraint before allowing the test to > continue. Is > there a canonical/best way for browser tests to do this, without having to > wrap > the mojom::PageLoadMetrics object like this? > > > > > > The messages are always going to be dispatched to some implementation > of > mojom::PageLoadMetrics. You can either do this (dispatch to a test-only > impl) or > add testing hooks to the non-test impl. I don't see much room for other > options > if a test needs to inspect incoming messages. > > > > Makes sense, thanks! Looking around the mojo code a bit, I see the > MessageReceiver interface. Is it possible to use Binding::AddFilter() or > something similar to observe messages as they are being processed? That > would > allow us to avoid wrapping the PageLoadMetrics itself, which seems > desirable to > avoid if possible. > > (and just to clarify, we'd only ever do this in test code - never in real > production code) > > https://codereview.chromium.org/2823523003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks! I appreciate your helping us to work through this. I'll spend a little time thinking through whether wrapping the interface vs adding hooks to the non test class is the better path. I currently have a slight pref for wrapping as done currently but will think through a bit more. Ken, if you do see any opportunities to simplify the way lpy@ is wrapping things currently, please do let us know. Generally speaking, the simpler the wrapping code the better for things like this. On Wed, Apr 26, 2017 at 12:33 PM Ken Rockot <rockot@chromium.org> wrote: > I wouldn't recommend it. Filters are applied pre-deserialization, so you'd > have to manually inspect the message using DataView objects. I don't think > this will end up being better than what you're doing now. > > On Wed, Apr 26, 2017 at 9:32 AM, <bmcquade@chromium.org> wrote: > >> On 2017/04/26 at 16:31:22, Bryan McQuade wrote: >> > On 2017/04/26 at 16:11:42, rockot wrote: >> > > >> >> https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... >> > > File >> >> chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc >> (right): >> > > >> > > >> >> https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... >> > > >> >> 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 McQuade wrote: >> > > > ah, I see now why you have this class and this method. I still >> worry about >> having to stub/wrap so much of the mojo code in a browser test. Ideally >> we'd be >> able to accomplish the goal of waiting for a mojo message to arrive >> without >> having to wrap the mojom::PageLoadMetrics like this. >> > > > >> > > > Ken, the test cases in this file to wait until a message received >> over the >> mojo channel meets a certain constraint before allowing the test to >> continue. Is >> there a canonical/best way for browser tests to do this, without having >> to wrap >> the mojom::PageLoadMetrics object like this? >> > > >> > > The messages are always going to be dispatched to some implementation >> of >> mojom::PageLoadMetrics. You can either do this (dispatch to a test-only >> impl) or >> add testing hooks to the non-test impl. I don't see much room for other >> options >> if a test needs to inspect incoming messages. >> > >> > Makes sense, thanks! Looking around the mojo code a bit, I see the >> MessageReceiver interface. Is it possible to use Binding::AddFilter() or >> something similar to observe messages as they are being processed? That >> would >> allow us to avoid wrapping the PageLoadMetrics itself, which seems >> desirable to >> avoid if possible. >> >> (and just to clarify, we'd only ever do this in test code - never in real >> production code) >> >> https://codereview.chromium.org/2823523003/ >> > > -- > You received this message because you are subscribed to the Google Groups > "loading-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to loading-reviews+unsubscribe@chromium.org. > To post to this group, send email to loading-reviews@chromium.org. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/CA%2BapAgHWf... > <https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/CA%2BapAgHWf...> > . > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... 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 Rockot wrote: > On 2017/04/26 at 16:04:34, Bryan McQuade wrote: > > ah, I see now why you have this class and this method. I still worry about having to stub/wrap so much of the mojo code in a browser test. Ideally we'd be able to accomplish the goal of waiting for a mojo message to arrive without having to wrap the mojom::PageLoadMetrics like this. > > > > Ken, the test cases in this file to wait until a message received over the mojo channel meets a certain constraint before allowing the test to continue. Is there a canonical/best way for browser tests to do this, without having to wrap the mojom::PageLoadMetrics object like this? > > The messages are always going to be dispatched to some implementation of mojom::PageLoadMetrics. You can either do this (dispatch to a test-only impl) or add testing hooks to the non-test impl. I don't see much room for other options if a test needs to inspect incoming messages. I decided that adding testing hooks to the non-test impl was better overall. It has no real cost to non-test code, and allows us to avoid having to wrap code in tests, which I think is desirable. Peiyong, I put together a change for this here: https://codereview.chromium.org/2847513002 which I hope eases the rest of your work to migrate from IPC to mojo. Let me know what you think. Thanks, Bryan 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/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?
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I updated the patch, ptal. https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:13: #include "base/run_loop.h" On 2017/04/26 16:04:34, Bryan McQuade wrote: > can remove this include Done. https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:149: observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated( On 2017/04/26 16:04:34, Bryan McQuade wrote: > let's switch this to invoke OnTimingUpdated directly, rather than invoking the > IPC and Mojo codepaths. Done. Forgot to change this part https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:227: if (!mock_page_load_metrics_) { On 2017/04/26 16:04:34, Bryan McQuade wrote: > just so i better understand, why is this needed in this browsertest if we've > written a separate browsertest for exercising the mojo codepath? > > fwiw i am still hopeful we can merge the 2 browsertests into one, but in the > absence of a merge it seems odd that we have to do so much mojo setup for a > browsertest that isn't actually doing anything with mojo Done, I forgot to remove this part. As I mentioned before, the reason I didn't add it to exist browser test set, is because we need to set the feature state before the render process is created, in order for it to inherit the feature state from the browser process. That is to say, we can't config Finch Trial for individual test case, we can only turn it on for a browser test set. https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178: void AddObserver(PageLoadTimingObserver* observer) { On 2017/04/26 20:41:51, Bryan McQuade wrote: > On 2017/04/26 at 16:11:41, Ken Rockot wrote: > > On 2017/04/26 at 16:04:34, Bryan McQuade wrote: > > > ah, I see now why you have this class and this method. I still worry about > having to stub/wrap so much of the mojo code in a browser test. Ideally we'd be > able to accomplish the goal of waiting for a mojo message to arrive without > having to wrap the mojom::PageLoadMetrics like this. > > > > > > Ken, the test cases in this file to wait until a message received over the > mojo channel meets a certain constraint before allowing the test to continue. Is > there a canonical/best way for browser tests to do this, without having to wrap > the mojom::PageLoadMetrics object like this? > > > > The messages are always going to be dispatched to some implementation of > mojom::PageLoadMetrics. You can either do this (dispatch to a test-only impl) or > add testing hooks to the non-test impl. I don't see much room for other options > if a test needs to inspect incoming messages. > > I decided that adding testing hooks to the non-test impl was better overall. It > has no real cost to non-test code, and allows us to avoid having to wrap code in > tests, which I think is desirable. > > Peiyong, I put together a change for this here: > https://codereview.chromium.org/2847513002 which I hope eases the rest of your > work to migrate from IPC to mojo. Let me know what you think. > > Thanks, > Bryan It looks good. 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/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. https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h (right): https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:62: mojom::PageLoadMetricsAssociatedPtr page_load_metrics_; On 2017/04/26 16:04:34, Bryan McQuade wrote: > Let's add a short comment explaining why we use a PageLoadMetricsAssociatedPtr > here (rather than a plain PageLoadMetrics). Done. They are two different concepts. A plain PageLoadMetrics means a reference to a PageLoadMetrics implementation, a PageLoadMetricsAssociatedPtr is one end of a mojo channel. 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/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. > 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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'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?
On 2017/04/27 at 15:42:32, Bryan McQuade 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'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? Given that the lifetime of PageTimingMetricsSender is scoped to a committed document's lifetime, perhaps we call it CommittedDocumentMetricsCollector/Accumulator (or just DocumentMetricsCollector/Accumulator/etc)?
On 2017/04/27 15:42:32, Bryan McQuade 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! Great! Thanks! Will update the patch. > 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. What is the IPC serialization code we need to update? > 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? It's not scalable/extendable. And we also need to have operations like IsEmpty standalone in a separate file. But I don't object here, a follow-up patch sounds good to me, and we can always go back to typemap if we encounter problems. > 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? The creation is very clear, the interception of creation is not clean. We have `page_timing_sender_ = PageTimgingSender::CreatePageTimingSender(this, routing_id(), render_frame());` somewhere, and in MetricsRFO::DidCommitProvisionalLoad, we have: page_timing_metrics_sender_.reset( new PageTimingMetricsSender(page_timing_sender_.get(), CreateTimer(), timing)); So I think we either wrap around `page_timing_sender_ = PageTimgingSender::CreatePageTimingSender(this, routing_id(), render_frame());` into a method and override it in test, or expose page_timing_sender_ through a setter and set it to a mocked version in test. Both ways are not clean to me though. WDYT?
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.) > > 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?
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?
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #10 (id:260001) has been deleted
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [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 ========== to ========== [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. NOPRESUBMIT=true BUG=709259,712915 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I rebased the patch, ptal.
I have nothing new to add so FWIW this LGTM
Thanks! Here are a couple comments. Looks like some of my comments from earlier rounds of reviews didn't get addressed. Can you go through & reply to each so I know you didn't miss them, and if you decided not to address them can you give an explanation why? https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:19: #include "chrome/browser/ui/browser.h" looks like you can revert this file https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:175: ASSERT_TRUE( i think you can get rid of these assertions in each test. if you want you could override SetUp() and verify that this assertion holds in that method (SetUp is invoked once at the start of each test fixture) https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:49: int32 behavior_flags; will this get a default value of zero? or do we need to write custom init code to set it to zero by default? https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. i see unittests for other struct traits elsewhere in chromium code. could we add some unittest coverage for these as well? ideally, we'd have tests that do something like: 1. create a PageLoadTiming, where every field is set and each has a distinct value 2. serialize it using mojo 3. unserialize it using mojo 4. verify that the resulting output is equal to the original
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I addressed the latest comments and updated the patch. I need to think about the PageTimingSender API again. 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 15:42:32, Bryan McQuade wrote: > 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'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? Acknowledged. https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:55: UpdateTiming(PageLoadTiming page_load_timing, PageLoadMetadata page_load_metadata); On 2017/04/26 15:24:22, Bryan McQuade wrote: > similarly, the generated code appears to be using > page_load_metrics::PageLoadTiming rather than the mojom version you declare in > this file. It may just be that I don't fully understand how mojo works, but on > first glance it doesn't seem like we're using the types you declared in this > mojom file. If I'm misunderstanding please let me know - I'm still trying to > learn how mojo works. Thanks! Acknowledged. https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:19: #include "chrome/browser/ui/browser.h" On 2017/05/03 19:03:05, Bryan McQuade wrote: > looks like you can revert this file We have two FailAllNetworkTransactions in browsertests, so I move them to anonymous namespace. https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:175: ASSERT_TRUE( On 2017/05/03 19:03:05, Bryan McQuade wrote: > i think you can get rid of these assertions in each test. if you want you could > override SetUp() and verify that this assertion holds in that method (SetUp is > invoked once at the start of each test fixture) Done. https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics.mojom (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics.mojom:49: int32 behavior_flags; On 2017/05/03 19:03:05, Bryan McQuade wrote: > will this get a default value of zero? or do we need to write custom init code > to set it to zero by default? yes https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h (right): https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/03 19:03:05, Bryan McQuade wrote: > i see unittests for other struct traits elsewhere in chromium code. could we add > some unittest coverage for these as well? > > ideally, we'd have tests that do something like: > 1. create a PageLoadTiming, where every field is set and each has a distinct > value > 2. serialize it using mojo > 3. unserialize it using mojo > 4. verify that the resulting output is equal to the original Done.
Thanks! Here are a few more comments. RE: the PageTimingSender refactor, I think that'll help to simplify renderer tests quite a bit - you should be able to get rid of both FakePageLoadMetricsImpl and FakePageTimingSender if you provide an interface for PageTimingSender with implementations for IPC and Mojo (as well as a mock version for use in the unittest). https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:150: page_load_metrics_binding_for_testing() { looks like this is no longer used. remove? https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:156: friend class FakePageLoadMetrics; can remove - this class doesn't appear to exist anymore https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157: friend class MetricsWebContentsObserverTest; can remove - don't need friend anymore https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:158: friend class PageLoadMetricsObserverTestHarness; can remove - don't need friend anymore https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:9: looks like this file can be reverted https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:9: looks like this file can be reverted https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:177: ASSERT_TRUE( looks like you can remove this one as well https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc:16: TEST(DocumentTimingStructTraitsTest, Basic) { Let's make all of these part of PageLoadMetricsStructTraitsTest with more descriptive names for each than 'Basic', for example TEST(PageLoadMetricsStructTraitsTest, DocumentTiming) { ... } TEST(PageLoadMetricsStructTraitsTest, PaintTiming) { ... }
https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:617: content::RenderFrameHost* render_frame_host = can we DCHECK that the mojofication feature is enabled here? and likewise, dcheck that it's not enabled when the IPC codepath is hit? that probably means adding an extra method besides OnTimingUpdated so you have an IPC-specific method to dcheck in.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:617: content::RenderFrameHost* render_frame_host = On 2017/05/07 19:48:13, Bryan McQuade wrote: > can we DCHECK that the mojofication feature is enabled here? and likewise, > dcheck that it's not enabled when the IPC codepath is hit? that probably means > adding an extra method besides OnTimingUpdated so you have an IPC-specific > method to dcheck in. Done. https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:150: page_load_metrics_binding_for_testing() { On 2017/05/07 19:44:41, Bryan McQuade wrote: > looks like this is no longer used. remove? Done. https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:156: friend class FakePageLoadMetrics; On 2017/05/07 19:44:41, Bryan McQuade wrote: > can remove - this class doesn't appear to exist anymore Done. https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157: friend class MetricsWebContentsObserverTest; On 2017/05/07 19:44:41, Bryan McQuade wrote: > can remove - don't need friend anymore Done. https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:158: friend class PageLoadMetricsObserverTestHarness; On 2017/05/07 19:44:41, Bryan McQuade wrote: > can remove - don't need friend anymore Done. https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:9: On 2017/05/07 19:44:41, Bryan McQuade wrote: > looks like this file can be reverted Done. https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:177: ASSERT_TRUE( On 2017/05/07 19:44:41, Bryan McQuade wrote: > looks like you can remove this one as well Done. https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_loa... File chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_loa... chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc:16: TEST(DocumentTimingStructTraitsTest, Basic) { On 2017/05/07 19:44:41, Bryan McQuade wrote: > Let's make all of these part of PageLoadMetricsStructTraitsTest with more > descriptive names for each than 'Basic', for example > TEST(PageLoadMetricsStructTraitsTest, DocumentTiming) { > ... > } > TEST(PageLoadMetricsStructTraitsTest, PaintTiming) { > ... > } Done. I don't think these names are more descriptive, so I made them to TEST(PageLoadMetricsStructTraitsTest, DocumentTimingBasic).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Thanks! I think all that's left here is to refactor to have implementations of PageTimingSender for IPC and for Mojo (and a gmock version in the unit test), which should also allow you to get rid of FakePageLoadMetrics and FakePageTimingSender. https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157: void OnUpdateTiming(content::RenderFrameHost* render_frame_host, nit: just to clarify things for readers of the code, let's call this OnUpdateTimingOverIPC. otherwise the name OnUpdateTiming is very similar to OnTimingUpdated & could be confusing
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #12 (id:340001) has been deleted
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
I updated the PageTimingSender API, ptal. https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157: void OnUpdateTiming(content::RenderFrameHost* render_frame_host, On 2017/05/08 21:13:29, Bryan McQuade wrote: > nit: just to clarify things for readers of the code, let's call this > OnUpdateTimingOverIPC. otherwise the name OnUpdateTiming is very similar to > OnTimingUpdated & could be confusing Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ping for review.
The CQ bit was unchecked by commit-bot@chromium.org
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...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [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. NOPRESUBMIT=true BUG=709259,712915 ========== to ========== [Page Load Metrics] PageLoadMetrics Mojofication. This patch 1. Adds mojo pipeline; 2. Adds Finch Trial check to toggle mojo IPC; 3. Modifies browser tests to be parameterized 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. BUG=709259,712915 ==========
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Patchset #16 (id:440001) has been deleted
I rebased the patch, ptal.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Thanks! This is much easier to review with the initial mojo change landed. Basically looks good - just some small things. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/fake_page_timing_sender.h (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/fake_page_timing_sender.h:29: // soon use base::Optional which uses aligned memory, so we're forced to roll this comment can be updated - it's out of date - to say "PageLoadTiming uses base::Optional" https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/fake_page_timing_sender.h:35: class PageTimingValidator { thanks for factoring this out into its own class - cleaner this way. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/fake_page_timing_sender.h:69: PageTimingValidator* validator_; nit: since this is a constructor arg, can use const PageTimingValidator* validator_; https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:173: return PageTimingSender::CreatePageTimingSender(render_frame(), routing_id()); given that the static PageTimingSender::CreatePageTimingSender method is only called in this one place, I'd rather that we inline it into this method, and move the IPC and Mojo sender impls to an anonymous namespace in this file. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc:36: base::WrapUnique<PageTimingSender>( nit: we should use base::MakeUnique here instead. there's supposed to be a linter warning about this. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_sender.cc (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:18: class LegacyIPCPageTimingSender : public IPC::Sender, public PageTimingSender { does this need to implement IPC::Sender anymore? seems like the Send() method can just be inlined into SendTiming now https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:22: : render_frame_(render_frame), routing_id_(routing_id) {} can the render frame ever be null? iirc we had some checks for this when we used the concrete sender class in unit tests, but with your refactor, maybe we can be certain that we never receive a null renderframe here? you do dcheck this condition in the mojo sender so it seems like it's probably a valid assumption for this class as well. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:32: bool Send(IPC::Message* message) override { let's inline this up into SendTiming() above https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:35: delete message; do we want to delete here? I was under the impression that the RenderFrame takes ownership and manages the object lifetime https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:51: if (!page_load_metrics_) { I'd rather initialize this in the constructor (instead of lazily in the send method). That also allow us to avoid keeping render_frame_ as a member.
lpy@chromium.org changed reviewers: + tsepez@chromium.org
Thanks, I updated the patch. +Tom@ for security review on mojo, please take a look. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/fake_page_timing_sender.h (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/fake_page_timing_sender.h:29: // soon use base::Optional which uses aligned memory, so we're forced to roll On 2017/05/19 14:06:18, Bryan McQuade wrote: > this comment can be updated - it's out of date - to say "PageLoadTiming uses > base::Optional" Done. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/fake_page_timing_sender.h:69: PageTimingValidator* validator_; On 2017/05/19 14:06:18, Bryan McQuade wrote: > nit: since this is a constructor arg, can use > const PageTimingValidator* validator_; Done. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:173: return PageTimingSender::CreatePageTimingSender(render_frame(), routing_id()); On 2017/05/19 14:06:18, Bryan McQuade wrote: > given that the static PageTimingSender::CreatePageTimingSender method is only > called in this one place, I'd rather that we inline it into this method, and > move the IPC and Mojo sender impls to an anonymous namespace in this file. Done. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc:36: base::WrapUnique<PageTimingSender>( On 2017/05/19 14:06:18, Bryan McQuade wrote: > nit: we should use base::MakeUnique here instead. there's supposed to be a > linter warning about this. Done. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/page_timing_sender.cc (right): https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:18: class LegacyIPCPageTimingSender : public IPC::Sender, public PageTimingSender { On 2017/05/19 14:06:18, Bryan McQuade wrote: > does this need to implement IPC::Sender anymore? seems like the Send() method > can just be inlined into SendTiming now Done. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:22: : render_frame_(render_frame), routing_id_(routing_id) {} On 2017/05/19 14:06:18, Bryan McQuade wrote: > can the render frame ever be null? iirc we had some checks for this when we used > the concrete sender class in unit tests, but with your refactor, maybe we can be > certain that we never receive a null renderframe here? you do dcheck this > condition in the mojo sender so it seems like it's probably a valid assumption > for this class as well. Done. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:32: bool Send(IPC::Message* message) override { On 2017/05/19 14:06:18, Bryan McQuade wrote: > let's inline this up into SendTiming() above Done. https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:35: delete message; On 2017/05/19 14:06:19, Bryan McQuade wrote: > do we want to delete here? I was under the impression that the RenderFrame takes > ownership and manages the object lifetime https://cs.chromium.org/chromium/src/content/public/renderer/render_frame_obs... https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/page_timing_sender.cc:51: if (!page_load_metrics_) { On 2017/05/19 14:06:18, Bryan McQuade wrote: > I'd rather initialize this in the constructor (instead of lazily in the send > method). That also allow us to avoid keeping render_frame_ as a member. Done.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm RS - I saw only cosmetic changes in .mojom plus one manifest.
LGTM, thanks! Just a couple small things. https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:52: content::RenderFrame* render_frame_ = nullptr; since this is set in the constructor, you can make it const content::RenderFrame* render_frame_; (and thus no need to set it to nullptr by default) https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:59: DCHECK(render_frame && render_frame->GetWebFrame()); we can omit the render_frame->GetWebFrame() part of the DCHECK since we don't appear to depend on that anywhere.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I addressed your comments, will land this after trybot goes green. https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:52: content::RenderFrame* render_frame_ = nullptr; On 2017/05/20 00:09:53, Bryan McQuade wrote: > since this is set in the constructor, you can make it > const content::RenderFrame* render_frame_; > (and thus no need to set it to nullptr by default) Done. https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:59: DCHECK(render_frame && render_frame->GetWebFrame()); On 2017/05/20 00:09:53, Bryan McQuade wrote: > we can omit the render_frame->GetWebFrame() part of the DCHECK since we don't > appear to depend on that anywhere. Done.
https://codereview.chromium.org/2823523003/diff/500001/chrome/renderer/page_l... File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/2823523003/diff/500001/chrome/renderer/page_l... chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:52: content::RenderFrame* const render_frame_; ah, thanks, it's been a while - you got the const placement right here. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, tsepez@chromium.org, bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/2823523003/#ps520001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 520001, "attempt_start_ts": 1495482484547220,
"parent_rev": "d99444dbb09468ef53fc699708c0b68afb451baa", "commit_rev":
"a44af8f558766ab8c6b65fff8be4b1ef1a5f903b"}
Message was sent while issue was closed.
Description was changed from ========== [Page Load Metrics] PageLoadMetrics Mojofication. This patch 1. Adds mojo pipeline; 2. Adds Finch Trial check to toggle mojo IPC; 3. Modifies browser tests to be parameterized 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. BUG=709259,712915 ========== to ========== [Page Load Metrics] PageLoadMetrics Mojofication. This patch 1. Adds mojo pipeline; 2. Adds Finch Trial check to toggle mojo IPC; 3. Modifies browser tests to be parameterized 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. BUG=709259,712915 Review-Url: https://codereview.chromium.org/2823523003 Cr-Commit-Position: refs/heads/master@{#473663} Committed: https://chromium.googlesource.com/chromium/src/+/a44af8f558766ab8c6b65fff8be4... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:520001) as https://chromium.googlesource.com/chromium/src/+/a44af8f558766ab8c6b65fff8be4...
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:520001) has been created in https://codereview.chromium.org/2897243002/ by lpy@chromium.org. The reason for reverting is: Revert this patch due to windows crash and we are at the corner of branch day.. |
