Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1376003002:
Make rendering timeouts work even if paint data received early (Closed)
|
Created:
5 years, 2 months ago by kenrb Modified:
5 years, 2 months ago CC:
esprehn, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cursor Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake rendering timeouts work even if paint data received early
The initial implementation of the browser paint timer for a new page
load assumed that the DidCommitProvisionalLoad IPC would be received
before any compositor frame generated from rendering the new page.
This turns out to be false. This CL adds a new IPC that is queued to
be sent when a new compositor frame is generated, and can be handled by
the browser even if it arrives too early. In that case, a flag is set
that tells RenderWidgetHost to not set the timer.
BUG=535375
Committed: https://crrev.com/d79de93d888aa78e3caeab8d5288915cdb4c7f29
Cr-Commit-Position: refs/heads/master@{#352028}
Patch Set 1 #Patch Set 2 : Issue with base resolved #Patch Set 3 : Rebase that maybe works this time #
Total comments: 18
Patch Set 4 : Review comments addressed #
Total comments: 8
Patch Set 5 : Changed method names in Blink #Patch Set 6 : More creis comments addressed #Patch Set 7 : Moved message dispatch mechanism from Blink to content #
Total comments: 5
Messages
Total messages: 29 (3 generated)
kenrb@chromium.org changed reviewers: + creis@chromium.org, dcheng@chromium.org
creis: Can you please review the content parts? dcheng: Can you please review the Blink parts?
Great. I'm a bit nervous about interactions between multiple navigations, but if we're guaranteed to get both IPCs for one navigation before hearing any for another navigation, we might be ok. Some comments below. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.cc:925: // is notified about the page have been committed, in which case nit: about the page being committed https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.cc:936: void RenderWidgetHostImpl::StopNewContentRenderingTimeout() { Can we remove this and just put this code within OnFirstPainAfterLoad? I don't think there's any other callers of it now, and the timer is tightly coupled with received_paint_after_load_. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... File content/browser/renderer_host/render_widget_host_impl.h (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:271: // a page load. This stops |new_content_rendering_timeout_|. nit: "...or prevents the timer from running if it hasn't started yet." https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:272: void OnFirstPaintAfterLoad(); Are we guaranteed to always get both this and the commit IPC before seeing any other instances? For example, I don't want to accidentally allow this: 1) OnFirstPaintAfterLoad for page 1. (No commit of page 1 for some reason) 2) OnDidCommitProvisionalLoad for page 2. (No timer starts because of #1, but page 2 can now be spoofed.) https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:817: bool received_paint_after_load_; Let's say when we expect this to be true, since it's not clear. I think it's exactly the time interval between the first paint and the navigation commit, if they happen in that order. It's never true if the commit happens first, and (hopefully?) we're guaranteed to always receive those IPCs in pairs without other messages from other navigations interleaved. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/web_contents/web_contents_impl.cc:3446: GetMainFrame()->GetRenderWidgetHost()->OnFirstPaintAfterLoad(); Why does this need to come through WebContents? Shouldn't we just handle it in RenderWidgetHostImpl? https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/common/v... File content/common/view_messages.h (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/common/v... content/common/view_messages.h:1336: // Send after a paint happens after any page commit, including a blank one. nit: Sent Also, we shouldn't really be adding new ViewHostMsgs at this point. But maybe we just need to rename all the widget ones to WidgetHostMsg. A TODO is probably fine.
+dglazkov and +esprehn as well: it seems kind of unfortunate to have so much
overlap in the m_shouldDispatch{various bools related to layout/painting}. Is
there anything else we can use as a signal?
https://codereview.chromium.org/1376003002/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
https://codereview.chromium.org/1376003002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3872:
m_shouldDispatchFirstPaintAfterLoad = false;
Why does this dispatch the didFirstPaintAfterLoad()? There's no painting
involved here, is there?
https://codereview.chromium.org/1376003002/diff/30001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):
https://codereview.chromium.org/1376003002/diff/30001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:94: // Signal that a new page
has loaded. This attaches a message to the
The second half of this comment seems out of place: this isn't really a layer
that's concerned with ipc messages.
Thanks for the review. Responses below. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.cc:925: // is notified about the page have been committed, in which case On 2015/09/30 20:36:19, Charlie Reis wrote: > nit: about the page being committed Done. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.cc:936: void RenderWidgetHostImpl::StopNewContentRenderingTimeout() { On 2015/09/30 20:36:19, Charlie Reis wrote: > Can we remove this and just put this code within OnFirstPainAfterLoad? I don't > think there's any other callers of it now, and the timer is tightly coupled with > received_paint_after_load_. Done. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... File content/browser/renderer_host/render_widget_host_impl.h (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:271: // a page load. This stops |new_content_rendering_timeout_|. On 2015/09/30 20:36:19, Charlie Reis wrote: > nit: "...or prevents the timer from running if it hasn't started yet." Done. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:272: void OnFirstPaintAfterLoad(); On 2015/09/30 20:36:19, Charlie Reis wrote: > Are we guaranteed to always get both this and the commit IPC before seeing any > other instances? > > For example, I don't want to accidentally allow this: > 1) OnFirstPaintAfterLoad for page 1. > (No commit of page 1 for some reason) > 2) OnDidCommitProvisionalLoad for page 2. > (No timer starts because of #1, but page 2 can now be spoofed.) It shouldn't be possible to get a ViewHostMsg_DidFirstPaintAfterLoad without a FrameHostMsg_DidCommitProvisionalLoad. Both are triggered in the renderer by FrameLoaderClientImpl::dispatchDidCommitLoad. The only bailout I see is that FrameHostMsg_DidCommitProvisionalLoad won't be sent if the WebFrame has no client (i.e. no RenderFrameImpl). In this case I imagine the frame has been destroyed so we don't have to worry about any spoofing problems, but I would be interested if Daniel can think of a way that could cause a problem. It's tricky to reason through this, but does that guarantee we won't run into problems? I would assume that if the load commit for page 1 is sent, that it isn't possible for a load commit for page 2 to arrive before it (this would seem really bad). https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:817: bool received_paint_after_load_; On 2015/09/30 20:36:19, Charlie Reis wrote: > Let's say when we expect this to be true, since it's not clear. I think it's > exactly the time interval between the first paint and the navigation commit, if > they happen in that order. It's never true if the commit happens first, and > (hopefully?) we're guaranteed to always receive those IPCs in pairs without > other messages from other navigations interleaved. Done. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... content/browser/web_contents/web_contents_impl.cc:3446: GetMainFrame()->GetRenderWidgetHost()->OnFirstPaintAfterLoad(); On 2015/09/30 20:36:19, Charlie Reis wrote: > Why does this need to come through WebContents? Shouldn't we just handle it in > RenderWidgetHostImpl? Done. I was copying how the other QueueMessage() messages were set up, but now that I look at it, there is no reason. https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/common/v... File content/common/view_messages.h (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/common/v... content/common/view_messages.h:1336: // Send after a paint happens after any page commit, including a blank one. On 2015/09/30 20:36:19, Charlie Reis wrote: > nit: Sent > > Also, we shouldn't really be adding new ViewHostMsgs at this point. But maybe > we just need to rename all the widget ones to WidgetHostMsg. A TODO is probably > fine. Done. Bug 537793 filed for that.
This isn't the right place for a paint related message. On Sep 30, 2015 2:25 PM, <kenrb@chromium.org> wrote: > Thanks for the review. Responses below. > > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > content/browser/renderer_host/render_widget_host_impl.cc:925: // is > notified about the page have been committed, in which case > On 2015/09/30 20:36:19, Charlie Reis wrote: > >> nit: about the page being committed >> > > Done. > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > content/browser/renderer_host/render_widget_host_impl.cc:936: void > RenderWidgetHostImpl::StopNewContentRenderingTimeout() { > On 2015/09/30 20:36:19, Charlie Reis wrote: > >> Can we remove this and just put this code within OnFirstPainAfterLoad? >> > I don't > >> think there's any other callers of it now, and the timer is tightly >> > coupled with > >> received_paint_after_load_. >> > > Done. > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > File content/browser/renderer_host/render_widget_host_impl.h (right): > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > content/browser/renderer_host/render_widget_host_impl.h:271: // a page > load. This stops |new_content_rendering_timeout_|. > On 2015/09/30 20:36:19, Charlie Reis wrote: > >> nit: "...or prevents the timer from running if it hasn't started yet." >> > > Done. > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > content/browser/renderer_host/render_widget_host_impl.h:272: void > OnFirstPaintAfterLoad(); > On 2015/09/30 20:36:19, Charlie Reis wrote: > >> Are we guaranteed to always get both this and the commit IPC before >> > seeing any > >> other instances? >> > > For example, I don't want to accidentally allow this: >> 1) OnFirstPaintAfterLoad for page 1. >> (No commit of page 1 for some reason) >> 2) OnDidCommitProvisionalLoad for page 2. >> (No timer starts because of #1, but page 2 can now be spoofed.) >> > > It shouldn't be possible to get a ViewHostMsg_DidFirstPaintAfterLoad > without a FrameHostMsg_DidCommitProvisionalLoad. Both are triggered in > the renderer by FrameLoaderClientImpl::dispatchDidCommitLoad. The only > bailout I see is that FrameHostMsg_DidCommitProvisionalLoad won't be > sent if the WebFrame has no client (i.e. no RenderFrameImpl). In this > case I imagine the frame has been destroyed so we don't have to worry > about any spoofing problems, but I would be interested if Daniel can > think of a way that could cause a problem. > > It's tricky to reason through this, but does that guarantee we won't run > into problems? I would assume that if the load commit for page 1 is > sent, that it isn't possible for a load commit for page 2 to arrive > before it (this would seem really bad). > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > content/browser/renderer_host/render_widget_host_impl.h:817: bool > received_paint_after_load_; > On 2015/09/30 20:36:19, Charlie Reis wrote: > >> Let's say when we expect this to be true, since it's not clear. I >> > think it's > >> exactly the time interval between the first paint and the navigation >> > commit, if > >> they happen in that order. It's never true if the commit happens >> > first, and > >> (hopefully?) we're guaranteed to always receive those IPCs in pairs >> > without > >> other messages from other navigations interleaved. >> > > Done. > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > File content/browser/web_contents/web_contents_impl.cc (right): > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/browser/... > content/browser/web_contents/web_contents_impl.cc:3446: > GetMainFrame()->GetRenderWidgetHost()->OnFirstPaintAfterLoad(); > On 2015/09/30 20:36:19, Charlie Reis wrote: > >> Why does this need to come through WebContents? Shouldn't we just >> > handle it in > >> RenderWidgetHostImpl? >> > > Done. > > I was copying how the other QueueMessage() messages were set up, but now > that I look at it, there is no reason. > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/common/v... > File content/common/view_messages.h (right): > > > https://chromiumcodereview.appspot.com/1376003002/diff/30001/content/common/v... > content/common/view_messages.h:1336: // Send after a paint happens after > any page commit, including a blank one. > On 2015/09/30 20:36:19, Charlie Reis wrote: > >> nit: Sent >> > > Also, we shouldn't really be adding new ViewHostMsgs at this point. >> > But maybe > >> we just need to rename all the widget ones to WidgetHostMsg. A TODO >> > is probably > >> fine. >> > > Done. Bug 537793 filed for that. > > https://chromiumcodereview.appspot.com/1376003002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://chromiumcodereview.appspot.com/1376003002/diff/30001/third_party/WebK... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/third_party/WebK... third_party/WebKit/Source/web/WebViewImpl.cpp:3872: m_shouldDispatchFirstPaintAfterLoad = false; On 2015/09/30 21:16:03, dcheng wrote: > Why does this dispatch the didFirstPaintAfterLoad()? There's no painting > involved here, is there? I've changed the name. The fact that it will queue a message to be sent with the next paint isn't really relevant here. https://chromiumcodereview.appspot.com/1376003002/diff/30001/third_party/WebK... File third_party/WebKit/public/web/WebWidgetClient.h (right): https://chromiumcodereview.appspot.com/1376003002/diff/30001/third_party/WebK... third_party/WebKit/public/web/WebWidgetClient.h:94: // Signal that a new page has loaded. This attaches a message to the On 2015/09/30 21:16:03, dcheng wrote: > The second half of this comment seems out of place: this isn't really a layer > that's concerned with ipc messages. Done.
Thanks. I'm happy with content/ (with nits below), assuming it doesn't need large changes to correspond with the Blink questions. https://chromiumcodereview.appspot.com/1376003002/diff/50001/content/browser/... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://chromiumcodereview.appspot.com/1376003002/diff/50001/content/browser/... content/browser/renderer_host/render_widget_host_impl.cc:934: if (new_content_rendering_timeout_) Tangentially related: Why do we need the null check? https://chromiumcodereview.appspot.com/1376003002/diff/50001/content/browser/... File content/browser/renderer_host/render_widget_host_impl.h (right): https://chromiumcodereview.appspot.com/1376003002/diff/50001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:815: // This boolean is true if RenderWidgetHostImpl receives a compositor frame nit: Add blank line above. https://chromiumcodereview.appspot.com/1376003002/diff/50001/content/browser/... content/browser/renderer_host/render_widget_host_impl.h:816: // from a newly loaded page before OnFirstPaintAfterLoad() is called. In Wait, that's not right. It's true if the compositor frame is received (i.e., OnFirstPaintAfterLoad()) before commit (i.e., StartNewContentRenderingTimeout()). Also, there's more things from my comment worth including here, like the fact it gets reset at commit time and that these two IPCs arrive in a pair, not interleaved with other navigations. https://chromiumcodereview.appspot.com/1376003002/diff/50001/content/browser/... File content/browser/web_contents/web_contents_impl.h (right): https://chromiumcodereview.appspot.com/1376003002/diff/50001/content/browser/... content/browser/web_contents/web_contents_impl.h:890: void OnFirstPaintAfterLoad(); Don't forget to remove this.
Sorry, more dumb questions... why isn't WebFrameClient::didCommitProvisionalLoad sufficient for our purposes here?
Why not just model it exactly how I plumbed didFirstLayoutAfterFinishedParsing?
On 2015/09/30 21:44:37, dcheng wrote: > Sorry, more dumb questions... why isn't WebFrameClient::didCommitProvisionalLoad > sufficient for our purposes here? It's possible I could make that work, but it felt a bit more risky because I really don't want the message sent for all navigations, just where new pages have been loaded, and also I only want it for top-level frames, not subframes. The way it currently works seems to offer a stronger guarantee of those things, than sending the message from didCommitProvisionalLoad after checking some set of conditions. I could be wrong, maybe it's simpler than I imagined.
On 2015/09/30 21:47:43, dglazkov wrote: > Why not just model it exactly how I plumbed didFirstLayoutAfterFinishedParsing? I think it is? The only difference being that the message is dispatched from WebViewImpl::didCommitLoad(), which is the signal we are looking for here.
On 2015/09/30 at 21:54:38, kenrb wrote: > On 2015/09/30 21:44:37, dcheng wrote: > > Sorry, more dumb questions... why isn't WebFrameClient::didCommitProvisionalLoad > > sufficient for our purposes here? > > It's possible I could make that work, but it felt a bit more risky because I really don't want the message sent for all navigations, just where new pages have been loaded, and also I only want it for top-level frames, not subframes. The way it currently works seems to offer a stronger guarantee of those things, than sending the message from didCommitProvisionalLoad after checking some set of conditions. I could be wrong, maybe it's simpler than I imagined. I guess I don't understand enough: when does a WebView get a new root graphics layer to set the bool to true? It seems like this is partially to work around the fact that didCommitLoad() is also called for in-page navigations. Why can't we just always queue the IPC message every time a provisional load commits at the top-level?
On 2015/09/30 22:03:45, dcheng wrote: > On 2015/09/30 at 21:54:38, kenrb wrote: > > On 2015/09/30 21:44:37, dcheng wrote: > > > Sorry, more dumb questions... why isn't > WebFrameClient::didCommitProvisionalLoad > > > sufficient for our purposes here? > > > > It's possible I could make that work, but it felt a bit more risky because I > really don't want the message sent for all navigations, just where new pages > have been loaded, and also I only want it for top-level frames, not subframes. > The way it currently works seems to offer a stronger guarantee of those things, > than sending the message from didCommitProvisionalLoad after checking some set > of conditions. I could be wrong, maybe it's simpler than I imagined. > > I guess I don't understand enough: when does a WebView get a new root graphics > layer to set the bool to true? It seems like this is partially to work around > the fact that didCommitLoad() is also called for in-page navigations. > Why can't we just always queue the IPC message every time a provisional load > commits at the top-level? In-page navigations don't guarantee a new paint will be sent, in which case the timer would fire and the page would be blanked. Accordingly, the browser doesn't set the timer on in-page commits, and since these need to be paired, we can't send a FirstPaintAfterLoad notification. The root graphics layer is destroyed when the top-level Document is destroyed, which seems to be a suitable signal that a full navigation is occurring and we will do a paint after the next load commit.
https://codereview.chromium.org/1376003002/diff/50001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1376003002/diff/50001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:934: if (new_content_rendering_timeout_) On 2015/09/30 21:42:09, Charlie Reis wrote: > Tangentially related: Why do we need the null check? I guess we don't. I had originally wanted it in the callback, but I've removed it here. https://codereview.chromium.org/1376003002/diff/50001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1376003002/diff/50001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:815: // This boolean is true if RenderWidgetHostImpl receives a compositor frame On 2015/09/30 21:42:09, Charlie Reis wrote: > nit: Add blank line above. Done. https://codereview.chromium.org/1376003002/diff/50001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:816: // from a newly loaded page before OnFirstPaintAfterLoad() is called. In On 2015/09/30 21:42:09, Charlie Reis wrote: > Wait, that's not right. It's true if the compositor frame is received (i.e., > OnFirstPaintAfterLoad()) before commit (i.e., > StartNewContentRenderingTimeout()). > > Also, there's more things from my comment worth including here, like the fact it > gets reset at commit time and that these two IPCs arrive in a pair, not > interleaved with other navigations. Rewritten and expanded. https://codereview.chromium.org/1376003002/diff/50001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1376003002/diff/50001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:890: void OnFirstPaintAfterLoad(); On 2015/09/30 21:42:09, Charlie Reis wrote: > Don't forget to remove this. Done.
content/ LGTM
On 2015/10/01 at 12:43:10, kenrb wrote: > On 2015/09/30 22:03:45, dcheng wrote: > > On 2015/09/30 at 21:54:38, kenrb wrote: > > > On 2015/09/30 21:44:37, dcheng wrote: > > > > Sorry, more dumb questions... why isn't > > WebFrameClient::didCommitProvisionalLoad > > > > sufficient for our purposes here? > > > > > > It's possible I could make that work, but it felt a bit more risky because I > > really don't want the message sent for all navigations, just where new pages > > have been loaded, and also I only want it for top-level frames, not subframes. > > The way it currently works seems to offer a stronger guarantee of those things, > > than sending the message from didCommitProvisionalLoad after checking some set > > of conditions. I could be wrong, maybe it's simpler than I imagined. > > > > I guess I don't understand enough: when does a WebView get a new root graphics > > layer to set the bool to true? It seems like this is partially to work around > > the fact that didCommitLoad() is also called for in-page navigations. > > Why can't we just always queue the IPC message every time a provisional load > > commits at the top-level? > > In-page navigations don't guarantee a new paint will be sent, in which case the timer would fire and the page would be blanked. Accordingly, the browser doesn't set the timer on in-page commits, and since these need to be paired, we can't send a FirstPaintAfterLoad notification. > > The root graphics layer is destroyed when the top-level Document is destroyed, which seems to be a suitable signal that a full navigation is occurring and we will do a paint after the next load commit. I don't think we send didCommitProvisonalLoad() for an in-page navigation though. Thus, the only difference between the new WebWidgetClient callback and didCommitProvisonalLoad() is that the former is only sent for top-level navigations. But it doesn't seem like it'd be hard to add the logic to queue the IPC message to RenderFrameImpl::didCommitProvisonalLoad(): what am I missing?
On 2015/10/01 17:36:30, dcheng wrote: > I don't think we send didCommitProvisonalLoad() for an in-page navigation > though. Thus, the only difference between the new WebWidgetClient callback and > didCommitProvisonalLoad() is that the former is only sent for top-level > navigations. But it doesn't seem like it'd be hard to add the logic to queue the > IPC message to RenderFrameImpl::didCommitProvisonalLoad(): what am I missing? We do send didCommitProvisionalLoad for in-page navigations. That was https://crbug.com/532428. That said, we could possibly filter out both subframe navigations and in-page navigations in content with appropriate checks. I did it this way because it felt cleaner and was consistent with how the other QueueMessage notifications are generated, but if you feel strongly about not modifying the Blink API layer then I could change it.
On 2015/10/01 17:45:46, kenrb wrote: > On 2015/10/01 17:36:30, dcheng wrote: > > I don't think we send didCommitProvisonalLoad() for an in-page navigation > > though. Thus, the only difference between the new WebWidgetClient callback and > > didCommitProvisonalLoad() is that the former is only sent for top-level > > navigations. But it doesn't seem like it'd be hard to add the logic to queue > the > > IPC message to RenderFrameImpl::didCommitProvisonalLoad(): what am I missing? > > We do send didCommitProvisionalLoad for in-page navigations. That was > https://crbug.com/532428. We definitely send didCommitProvisionalLoad for in-page navigations. We set params.was_within_same_page to true in that case (in RenderFrameImpl::SendDidCommitProvisionalLoad). > That said, we could possibly filter out both subframe navigations and in-page > navigations in content with appropriate checks. I did it this way because it > felt cleaner and was consistent with how the other QueueMessage notifications > are generated, but if you feel strongly about not modifying the Blink API layer > then I could change it. I'm not opposed to doing the filtering in content/.
On 2015/10/01 at 17:45:46, kenrb wrote: > On 2015/10/01 17:36:30, dcheng wrote: > > I don't think we send didCommitProvisonalLoad() for an in-page navigation > > though. Thus, the only difference between the new WebWidgetClient callback and > > didCommitProvisonalLoad() is that the former is only sent for top-level > > navigations. But it doesn't seem like it'd be hard to add the logic to queue the > > IPC message to RenderFrameImpl::didCommitProvisonalLoad(): what am I missing? > > We do send didCommitProvisionalLoad for in-page navigations. That was https://crbug.com/532428. Actually... Blink doesn't. In-page navigations use didNavigateWithinPage(). However, the problem is that //content's implementation of didNavigateWithinPage() ends up delegating to didCommitProvisionalLoad(). This seems like something that should be addressed in //content, rather than adding a new Blink callback. > > That said, we could possibly filter out both subframe navigations and in-page navigations in content with appropriate checks. I did it this way because it felt cleaner and was consistent with how the other QueueMessage notifications are generated, but if you feel strongly about not modifying the Blink API layer then I could change it. I'd prefer to fix the embedder. WebFrameClient/WebViewClient already have a huge callback surface, and when we have a bunch of callbacks that are closely related like this, it makes it harder to reason about the interface (for example, we have didCreateNewDocument, didClearWindowObject, and didCreateDocumentElement... what's the difference?).
New approach in RenderFrameImpl LGTM with nit. https://codereview.chromium.org/1376003002/diff/110001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1376003002/diff/110001/content/renderer/rende... content/renderer/render_frame_impl.cc:2739: // first paint of that page, so it can cancel the timer that waits for it. nit: (or skip the timer if the paint IPC is handled before the navigation commit IPC). https://codereview.chromium.org/1376003002/diff/110001/content/renderer/rende... content/renderer/render_frame_impl.cc:2741: render_view_->QueueMessage( There's some magic here I don't fully understand, but the comments on it in RenderWidget make it sound like the right thing.
lgtm
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
yay, this is much better :) https://codereview.chromium.org/1376003002/diff/110001/content/browser/render... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/1376003002/diff/110001/content/browser/render... content/browser/renderer_host/render_widget_host_unittest.cc:1112: TimeDelta::FromMicroseconds(20)); Why wait 20 microseconds? This usually produces flake in tests
https://codereview.chromium.org/1376003002/diff/110001/content/browser/render... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/1376003002/diff/110001/content/browser/render... content/browser/renderer_host/render_widget_host_unittest.cc:1112: TimeDelta::FromMicroseconds(20)); On 2015/10/02 05:58:15, esprehn wrote: > Why wait 20 microseconds? This usually produces flake in tests I used the same values as the hung renderer test, but I've changed it to 200. https://codereview.chromium.org/1376003002/diff/110001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1376003002/diff/110001/content/renderer/rende... content/renderer/render_frame_impl.cc:2739: // first paint of that page, so it can cancel the timer that waits for it. On 2015/10/01 21:26:39, Charlie Reis wrote: > nit: (or skip the timer if the paint IPC is handled before the navigation commit > IPC). Done.
The CQ bit was checked by kenrb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376003002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376003002/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d79de93d888aa78e3caeab8d5288915cdb4c7f29 Cr-Commit-Position: refs/heads/master@{#352028} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
