Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770873002:
Remove ViewHostMsg_DidFirstPaintAfterLoad (Closed)
|
Created:
3 years, 9 months ago by Saman Sami Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, mlamouri+watch-content_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove ViewHostMsg_DidFirstPaintAfterLoad
ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside
ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame
is mojoified, we don't want to have IPC messages sent using mojo calls.
Fortunately ViewHostMsg_DidFirstPaintAfterLoad can go away after the
introduction of content_source_id in CompositorFrameMetadata a few
weeks ago.
BUG=704972
Review-Url: https://codereview.chromium.org/2770873002
Cr-Commit-Position: refs/heads/master@{#459514}
Committed: https://chromium.googlesource.com/chromium/src/+/087035f0efd4da20592c02b64fddaa7bfee761c6
Patch Set 1 #Patch Set 2 : Fixed unittests #Patch Set 3 : Updated unit tests #
Total comments: 4
Patch Set 4 : Added comments #Patch Set 5 : Rebased #
Messages
Total messages: 42 (34 generated)
The CQ bit was checked by samans@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by samans@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...)
The CQ bit was checked by samans@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 ========== Remove ViewHostMsg_DidFirstPaintAfterLoad BUG=697864 ========== to ========== Remove ViewHostMsg_DidFirstPaintAfterLoad BUG=697864 ==========
samans@chromium.org changed reviewers: + fsamuel@chromium.org
PTAL
PTAL
Description was changed from ========== Remove ViewHostMsg_DidFirstPaintAfterLoad BUG=697864 ========== to ========== Remove ViewHostMsg_DidFirstPaintAfterLoad ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame is mojoified, we don't want to have IPC messages sent using mojo calls. BUG=697864 ==========
Description was changed from ========== Remove ViewHostMsg_DidFirstPaintAfterLoad ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame is mojoified, we don't want to have IPC messages sent using mojo calls. BUG=697864 ========== to ========== Remove ViewHostMsg_DidFirstPaintAfterLoad ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame is mojoified, we don't want to have IPC messages sent using mojo calls. Fortunately ViewHostMsg_DidFirstPaintAfterLoad can go away after the introduction of content_source_id in CompositorFrameMetadata a few weeks ago. BUG=697864 ==========
samans@chromium.org changed reviewers: + kenrb@chromium.org
kenrb PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 samans@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.
kenrb@chromium.org changed reviewers: + creis@chromium.org
lgtm, this is great, thanks! When I added the source ID field to CompositorFrameMetadata, I had been thinking a bit about how that could be used to fix https://bugs.chromium.org/p/chromium/issues/detail?id=551338, which reflects a flaw in the existing timer system when there multiple DidCommitProvisionalLoad messages and compositor frames get interleaved in strange orders. I suspect that your patch might address that problem, because it means we are now relying on a clear signal for stale vs non-stale frames. We should check that out and (if I am right) close that bug when this CL lands. Adding creis@ for content/ OWNER review, since he has reviewed previous CLs on this topic.
Thanks for the simplification! LGTM with nits. https://codereview.chromium.org/2770873002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2770873002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:895: uint32_t last_received_content_source_id_ = 0; Can you add a comment here about it, or point to where content source ID is documented (e.g., in RenderWidget::content_source_id_)? https://codereview.chromium.org/2770873002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2770873002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:3696: if (is_main_frame_ && !navigation_state->WasWithinSameDocument()) Let's leave some comment here so that it's clear why this is needed. Something like: Navigations that change the document represent a new content source. Keep track of that on the widget to help the browser process detect when stale compositor frames are being shown after a commit.
The CQ bit was checked by samans@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-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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 samans@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 LGTMs! I added some comments. https://codereview.chromium.org/2770873002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2770873002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:895: uint32_t last_received_content_source_id_ = 0; On 2017/03/24 16:52:01, Charlie Reis (OOO soon) wrote: > Can you add a comment here about it, or point to where content source ID is > documented (e.g., in RenderWidget::content_source_id_)? Done. https://codereview.chromium.org/2770873002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2770873002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:3696: if (is_main_frame_ && !navigation_state->WasWithinSameDocument()) On 2017/03/24 16:52:01, Charlie Reis (OOO soon) wrote: > Let's leave some comment here so that it's clear why this is needed. Something > like: > > Navigations that change the document represent a new content source. Keep track > of that on the widget to help the browser process detect when stale compositor > frames are being shown after a commit. That's a good comment. I'll add it verbatim.
Description was changed from ========== Remove ViewHostMsg_DidFirstPaintAfterLoad ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame is mojoified, we don't want to have IPC messages sent using mojo calls. Fortunately ViewHostMsg_DidFirstPaintAfterLoad can go away after the introduction of content_source_id in CompositorFrameMetadata a few weeks ago. BUG=697864 ========== to ========== Remove ViewHostMsg_DidFirstPaintAfterLoad ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame is mojoified, we don't want to have IPC messages sent using mojo calls. Fortunately ViewHostMsg_DidFirstPaintAfterLoad can go away after the introduction of content_source_id in CompositorFrameMetadata a few weeks ago. BUG=704972 ==========
The CQ bit was unchecked by samans@chromium.org
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2770873002/#ps80001 (title: "Rebased")
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": 80001, "attempt_start_ts": 1490382494465920,
"parent_rev": "98b4d6275a72758a1cb97e7cbee1fefc3a15192a", "commit_rev":
"087035f0efd4da20592c02b64fddaa7bfee761c6"}
Message was sent while issue was closed.
Description was changed from ========== Remove ViewHostMsg_DidFirstPaintAfterLoad ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame is mojoified, we don't want to have IPC messages sent using mojo calls. Fortunately ViewHostMsg_DidFirstPaintAfterLoad can go away after the introduction of content_source_id in CompositorFrameMetadata a few weeks ago. BUG=704972 ========== to ========== Remove ViewHostMsg_DidFirstPaintAfterLoad ViewHostMsg_DidFirstPaintAfterLoad is currently stored inside ViewHostMsg_SwapCompositorFrame, but once ViewHostMsg_SwapCompositorFrame is mojoified, we don't want to have IPC messages sent using mojo calls. Fortunately ViewHostMsg_DidFirstPaintAfterLoad can go away after the introduction of content_source_id in CompositorFrameMetadata a few weeks ago. BUG=704972 Review-Url: https://codereview.chromium.org/2770873002 Cr-Commit-Position: refs/heads/master@{#459514} Committed: https://chromium.googlesource.com/chromium/src/+/087035f0efd4da20592c02b64fdd... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/087035f0efd4da20592c02b64fdd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
