Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986883002:
blimp: Update page load status update indicator to use first paint. (Closed)
|
Created:
4 years, 7 months ago by Khushal Modified:
4 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nasko+codewatch_chromium.org, jam, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, creis+watch_chromium.org, piman+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, dtrainor+watch-blimp_chromium.org, David Trainor- moved to gerrit Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionblimp: Update page load status update indicator to use first paint.
The Blimp client uses the load progress status from the engine to
indicate page load progress for a navigation. While this signal informs
us whether the engine has downloaded all the resources for a page, it
can not be used reliably to indicate whether the client has received
any frames/compositor commits for this navigation.
This change uses 2 signals on the engine to indicate to the client
whether a navigation is complete:
1) When the engine is finished loading all the resources for the main
frame.
2) When the first paint has been performed for the main frame after a
navigation. The first paint implies a commit message has been sent
to the client.
The renderer uses compositor's SwapPromises to tie events, like the
first paint, with CompositorFrames and delivers them together to the
browser. Since Blimp uses the remote server LayerTreeHost on the engine,
which does not produce compositor frames and has no output surface, the
renderer now sends these messages after activation instead when using
remote compositing. Also, the remote compositor will activate these
promises immediately after sending the commit message.
BUG=603220
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/f3b62b6ed72d6405728899bf76aea9add6aee5fa
Cr-Commit-Position: refs/heads/master@{#396290}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments. #
Total comments: 6
Patch Set 3 : Addressed Wez's comments. #
Total comments: 5
Patch Set 4 : Update comment. #Patch Set 5 : Rebase. #Patch Set 6 : Fix test. #Messages
Total messages: 50 (16 generated)
Description was changed from ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message BUG=603220 ========== to ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message BUG=603220 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + sievers@chromium.org, vmpstr@chromium.org, wez@chromium.org
+wez for blimp/ +vmpstr for cc/ +sievers for content/ Need to add tests for the PageLoadTracker.
Description was changed from ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message BUG=603220 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message. BUG=603220 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + mdjones@chromium.org
+Matt to see how close this is to the progress bar for Clank.
https://codereview.chromium.org/1986883002/diff/1/cc/trees/remote_channel_mai... File cc/trees/remote_channel_main.cc (right): https://codereview.chromium.org/1986883002/diff/1/cc/trees/remote_channel_mai... cc/trees/remote_channel_main.cc:188: // Activate the swap promises after the commit is queued. Can you expand the comment to explain why this is necessary?
In general, this implementation isn't too different to what we now do for chrome on phones (see ToolbarManager.java). However, we do listen for a couple other events involving tab restoration and native pages; I'm not sure this is something you are worried about. I'm not perfectly clear on why first paint is a signal to stop the indicator; it seems like that would be too early. https://codereview.chromium.org/1986883002/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/1986883002/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:39: client_->SendPageLoadStatusUpdate(false); This call feels like you are switching the loading indicator off. Consider inverting the boolean value for this function (load_status doesn't tell me much).
https://codereview.chromium.org/1986883002/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/1986883002/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:39: client_->SendPageLoadStatusUpdate(false); On 2016/05/18 20:30:00, mdjones wrote: > This call feels like you are switching the loading indicator off. Consider > inverting the boolean value for this function (load_status doesn't tell me > much). Hmmm, I guess I was looking at it as the engine communicating the load status for a tab. And the client uses this information to drive the indicator. Added a comment about it. https://codereview.chromium.org/1986883002/diff/1/cc/trees/remote_channel_mai... File cc/trees/remote_channel_main.cc (right): https://codereview.chromium.org/1986883002/diff/1/cc/trees/remote_channel_mai... cc/trees/remote_channel_main.cc:188: // Activate the swap promises after the commit is queued. On 2016/05/18 18:05:59, vmpstr wrote: > Can you expand the comment to explain why this is necessary? Sure, Done.
On 2016/05/18 20:30:01, mdjones wrote: > In general, this implementation isn't too different to what we now do for chrome > on phones (see ToolbarManager.java). However, we do listen for a couple other > events involving tab restoration and native pages; I'm not sure this is Those are things we would have to take into account when this is integrated into Chrome. You can think of this as Chrome's content layer running on the engine, so we are piping signals that Chrome would generally receive from WebContents. > something you are worried about. I'm not perfectly clear on why first paint is a > signal to stop the indicator; it seems like that would be too early. First paint is used in addition to the load complete indication from content. The reason for this has to do with how scheduling of frame updates from the engine to client is structured. A first paint signal for Blimp implies that the first frame after a navigation has been sent to the client, which is important before we can actually say that the load was complete from the client's perspective.
friendly ping. This is for a critical bug. Would be great to get a quick review on this. Thanks!
On 2016/05/19 17:28:01, Khushal wrote: > friendly ping. > > This is for a critical bug. Would be great to get a quick review on this. > Thanks! > it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. But the client surely knows if it received a frame? Also the engine seems to be in the middle of all of this? I guess I just don't understand why this needs to be plumbed separately from the content layer.
On 2016/05/19 18:16:13, sievers wrote: > On 2016/05/19 17:28:01, Khushal wrote: > > friendly ping. > > > > This is for a critical bug. Would be great to get a quick review on this. > > Thanks! > > > it > can not be used reliably to indicate whether the client has received > any frames/compositor commits for this navigation. > > > But the client surely knows if it received a frame? > Also the engine seems to be in the middle of all of this? > > I guess I just don't understand why this needs to be plumbed separately from the > content layer. The client knows whether it has received a frame, but the engine knows about the navigation they are tied to. Content knows about both these events. I am trying to draw a parallel with how all the events in content which depend on compositor frames being sent to the browser will need to depend on the engine sending commits to the client anyway. My understanding of the way to do it on the client would be to inform the client when a provisional load is initiated, so it can show that a navigation has been started, and then when the load is committed, and have the client wait for at least one compositor commit after the load commit event? These load events will then need the render widget id attached to them so the client can make sure it listens to compositor commits from the corresponding compositor on the client. So we will basically be replicating the logic in the PageLoadTracker on the client while plumbing more granular information about the events on the engine to the client. Given that the engine already has knowledge of all the events, it seemed better to have this complexity stay on the engine rather than adding it to the protocol. If there is a better way to structure this, I am happy to change it.
On 2016/05/19 19:03:42, Khushal wrote: > On 2016/05/19 18:16:13, sievers wrote: > > On 2016/05/19 17:28:01, Khushal wrote: > > > friendly ping. > > > > > > This is for a critical bug. Would be great to get a quick review on this. > > > Thanks! > > > > > it > > can not be used reliably to indicate whether the client has received > > any frames/compositor commits for this navigation. > > > > > > But the client surely knows if it received a frame? > > Also the engine seems to be in the middle of all of this? > > > > I guess I just don't understand why this needs to be plumbed separately from > the > > content layer. > > The client knows whether it has received a frame, but the engine knows about the > navigation they are tied to. Content knows about both these events. I am trying > to draw a parallel with how all the events in content which depend on compositor > frames being sent to the browser will need to depend on the engine sending > commits to the client anyway. > > My understanding of the way to do it on the client would be to inform the client > when a provisional load is initiated, so it can show that a navigation has been > started, and then when the load is committed, and have the client wait for at > least one compositor commit after the load commit event? These load events will > then need the render widget id attached to them so the client can make sure it > listens to compositor commits from the corresponding compositor on the client. > So we will basically be replicating the logic in the PageLoadTracker on the > client while plumbing more granular information about the events on the engine > to the client. Given that the engine already has knowledge of all the events, it > seemed better to have this complexity stay on the engine rather than adding it > to the protocol. If there is a better way to structure this, I am happy to > change it. From a security perspective it seems like you really have to know what frame is for what site exactly. We very carefully deal with this so that you always show the right domain for example for the page that is currently displaying. So I'm not sure that some conservative logic is enough. I think you really need to be able to separate the frame streams somehow. In normal Chrome we do this by having separate 'windows' (whatever that means for the platform i.e. RWHV) for each domain and then carefully switching them out depending on the state of the navigation.
sievers@chromium.org changed reviewers: + nasko@chromium.org
cc lgtm
On 2016/05/19 19:33:47, sievers wrote: > On 2016/05/19 19:03:42, Khushal wrote: > > On 2016/05/19 18:16:13, sievers wrote: > > > On 2016/05/19 17:28:01, Khushal wrote: > > > > friendly ping. > > > > > > > > This is for a critical bug. Would be great to get a quick review on this. > > > > Thanks! > > > > > > > it > > > can not be used reliably to indicate whether the client has received > > > any frames/compositor commits for this navigation. > > > > > > > > > But the client surely knows if it received a frame? > > > Also the engine seems to be in the middle of all of this? > > > > > > I guess I just don't understand why this needs to be plumbed separately from > > the > > > content layer. > > > > The client knows whether it has received a frame, but the engine knows about > the > > navigation they are tied to. Content knows about both these events. I am > trying > > to draw a parallel with how all the events in content which depend on > compositor > > frames being sent to the browser will need to depend on the engine sending > > commits to the client anyway. > > > > My understanding of the way to do it on the client would be to inform the > client > > when a provisional load is initiated, so it can show that a navigation has > been > > started, and then when the load is committed, and have the client wait for at > > least one compositor commit after the load commit event? These load events > will > > then need the render widget id attached to them so the client can make sure it > > listens to compositor commits from the corresponding compositor on the client. > > So we will basically be replicating the logic in the PageLoadTracker on the > > client while plumbing more granular information about the events on the engine > > to the client. Given that the engine already has knowledge of all the events, > it > > seemed better to have this complexity stay on the engine rather than adding it > > to the protocol. If there is a better way to structure this, I am happy to > > change it. > > From a security perspective it seems like you really have to know what frame is > for what site exactly. > We very carefully deal with this so that you always show the right domain for > example for the page that is currently displaying. > So I'm not sure that some conservative logic is enough. I think you really need > to be able to separate the frame streams somehow. > In normal Chrome we do this by having separate 'windows' (whatever that means > for the platform i.e. RWHV) for each domain and then carefully switching them > out depending on the state of the navigation. Added offline, but as far as I know, Chrome on Android does not work like that at all. The omnibox shows the currently pending/committed navigation entry and that has no correspondence to the frames being delivered. For example, I can be on reddit and click to a link on a site that is extremely slow. The omnibox will show the url of the site i'm waiting for, but sometimes I can even interact with reddit and scroll it around. Granted, I haven't looked at this patch, but I know for certain the frames we paint do not always correspond with the URL that we are currently displaying.
On 2016/05/19 19:33:47, sievers wrote: > On 2016/05/19 19:03:42, Khushal wrote: > > On 2016/05/19 18:16:13, sievers wrote: > > > On 2016/05/19 17:28:01, Khushal wrote: > > > > friendly ping. > > > > > > > > This is for a critical bug. Would be great to get a quick review on this. > > > > Thanks! > > > > > > > it > > > can not be used reliably to indicate whether the client has received > > > any frames/compositor commits for this navigation. > > > > > > > > > But the client surely knows if it received a frame? > > > Also the engine seems to be in the middle of all of this? > > > > > > I guess I just don't understand why this needs to be plumbed separately from > > the > > > content layer. > > > > The client knows whether it has received a frame, but the engine knows about > the > > navigation they are tied to. Content knows about both these events. I am > trying > > to draw a parallel with how all the events in content which depend on > compositor > > frames being sent to the browser will need to depend on the engine sending > > commits to the client anyway. > > > > My understanding of the way to do it on the client would be to inform the > client > > when a provisional load is initiated, so it can show that a navigation has > been > > started, and then when the load is committed, and have the client wait for at > > least one compositor commit after the load commit event? These load events > will > > then need the render widget id attached to them so the client can make sure it > > listens to compositor commits from the corresponding compositor on the client. > > So we will basically be replicating the logic in the PageLoadTracker on the > > client while plumbing more granular information about the events on the engine > > to the client. Given that the engine already has knowledge of all the events, > it > > seemed better to have this complexity stay on the engine rather than adding it > > to the protocol. If there is a better way to structure this, I am happy to > > change it. > > From a security perspective it seems like you really have to know what frame is > for what site exactly. > We very carefully deal with this so that you always show the right domain for > example for the page that is currently displaying. The structure of the frame streams for Blimp follow the same structure that the compositor frame streams to the browser follow in Chrome. In Chrome this would be the case where a new document is loaded within the same RenderWidget, so the compositor is not torn down, it just receives a new LayerTree and starts producing frames for this document now. (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) If we are transitioning to a page which was loaded in a different RW and thus has a different RWHV, we would swap the frames visible to the user on the client when the RWHV is swapped on the engine. > So I'm not sure that some conservative logic is enough. I think you really need > to be able to separate the frame streams somehow. > In normal Chrome we do this by having separate 'windows' (whatever that means > for the platform i.e. RWHV) for each domain and then carefully switching them > out depending on the state of the navigation.
https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:174: content::RenderWidgetHost* render_widget_host) override; Why not make the PageLoadTracker a WebContentsObserver in its own right, and avoid these boilerplate methods? https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/pa... File blimp/engine/session/page_load_tracker.h (right): https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/pa... blimp/engine/session/page_load_tracker.h:24: // finishes or fails. nit: Feels like this should be an enum w/ values LOADING and LOADED, for clarity! https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/pa... blimp/engine/session/page_load_tracker.h:42: void DidFirstPaintAfterLoad(content::RenderWidgetHost* render_widget_host); nit: No need for blank lines between these. Suggest preceding them with a one-line comment explaining that they mirror calls in WebContentsObserver, or just make this class a WebContentsObserver? Is having multiple WebContentsObservers expensive? It does seem to be a very broad interface...
Thanks Wez! https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:174: content::RenderWidgetHost* render_widget_host) override; On 2016/05/24 01:42:17, Wez wrote: > Why not make the PageLoadTracker a WebContentsObserver in its own right, and > avoid these boilerplate methods? Thanks, that makes sense. Done. https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/pa... File blimp/engine/session/page_load_tracker.h (right): https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/pa... blimp/engine/session/page_load_tracker.h:24: // finishes or fails. On 2016/05/24 01:42:17, Wez wrote: > nit: Feels like this should be an enum w/ values LOADING and LOADED, for > clarity! Done. https://codereview.chromium.org/1986883002/diff/20001/blimp/engine/session/pa... blimp/engine/session/page_load_tracker.h:42: void DidFirstPaintAfterLoad(content::RenderWidgetHost* render_widget_host); On 2016/05/24 01:42:17, Wez wrote: > nit: No need for blank lines between these. > > Suggest preceding them with a one-line comment explaining that they mirror calls > in WebContentsObserver, or just make this class a WebContentsObserver? You're right, having this class as a WebContentsObserver makes purpose of these calls clearer. Done. > > Is having multiple WebContentsObservers expensive? It does seem to be a very > broad interface...
nasko@chromium.org changed reviewers: + kenrb@chromium.org
The content/ parts look fine to me. Adding kenrb@, since he wrote this code and I recall there was some trickiness there. I'll stamp the CL once he is happy with the code. https://codereview.chromium.org/1986883002/diff/40001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1986883002/diff/40001/content/public/browser/... content/public/browser/web_contents_observer.h:321: // This method is invoked when the main frame in the renderer performs the nit: s/renderer/renderer process/
lgtm https://codereview.chromium.org/1986883002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1986883002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:201: std::unique_ptr<PageLoadTracker> page_load_tracker_; nit: You could re-use the tracker across multiple WebContents by calling Observe() on it each time they change, if you want.
https://codereview.chromium.org/1986883002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1986883002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:201: std::unique_ptr<PageLoadTracker> page_load_tracker_; On 2016/05/25 03:33:10, Wez wrote: > nit: You could re-use the tracker across multiple WebContents by calling > Observe() on it each time they change, if you want. The PageLoadTracker has state tied to a WebContents, I was worried swapping would mean you start getting calls that don't respect the order it expects them in. Seemed safer to just tie it to a WebContents forever.
https://codereview.chromium.org/1986883002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1986883002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:201: std::unique_ptr<PageLoadTracker> page_load_tracker_; On 2016/05/25 04:24:23, Khushal wrote: > On 2016/05/25 03:33:10, Wez wrote: > > nit: You could re-use the tracker across multiple WebContents by calling > > Observe() on it each time they change, if you want. > > The PageLoadTracker has state tied to a WebContents, I was worried swapping > would mean you start getting calls that don't respect the order it expects them > in. Seemed safer to just tie it to a WebContents forever. +1 on having 1:1 objects instead of reuse. It is much easier to reason about and less likely to make a mistake.
Thanks nasko. Friendly ping to kenrb@ to take a look as well. :) https://codereview.chromium.org/1986883002/diff/40001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1986883002/diff/40001/content/public/browser/... content/public/browser/web_contents_observer.h:321: // This method is invoked when the main frame in the renderer performs the On 2016/05/24 21:39:22, nasko (slow) wrote: > nit: s/renderer/renderer process/ Done.
lgtm One thing to be cautious about is to not assume that you receive the commit message before the first paint notification, but AFAICT you are not assuming any particular ordering so this should be fine.
Thanks Ken! Stampity stamp LGTM.
On 2016/05/26 15:47:08, kenrb wrote: > lgtm > > One thing to be cautious about is to not assume that you receive the commit > message before the first paint notification, but AFAICT you are not assuming any > particular ordering so this should be fine. Oh, I was actually relying on the fact that the first commit message will be sent to the browser before the first paint notification. If we activate the swap promises after queuing the commit message, is there a reason why we can't assume ordering between these 2 events?
On 2016/05/26 16:12:41, Khushal wrote: > On 2016/05/26 15:47:08, kenrb wrote: > > lgtm > > > > One thing to be cautious about is to not assume that you receive the commit > > message before the first paint notification, but AFAICT you are not assuming > any > > particular ordering so this should be fine. > > Oh, I was actually relying on the fact that the first commit message will be > sent to the browser before the first paint notification. If we activate the swap > promises after queuing the commit message, is there a reason why we can't assume > ordering between these 2 events? I'm not sure, but I might have caused confusion by being loose with my terminology, so just to make sure we are on the same page with what we are talking about: by 'commit message' I was thinking of DidCommitProvisionalLoad, whereas you are watching for DidFinishLoad, and I was noting that messages queued from the main renderer thread aren't ordered relative to compositor frame swaps which happen on the compositor thread. (I wasn't talking about committing the compositor frame.) Looking at the handlers in PageLoadTracker, I don't see how you are assuming that ordering here, but I might be misunderstanding something about the patch. If we are indeed talking about DidFinishLoad vs DidFirstPaintAfterLoad ordering, then I am not clear on a basis for an ordering assumption.
On 2016/05/26 16:58:11, kenrb wrote: > On 2016/05/26 16:12:41, Khushal wrote: > > On 2016/05/26 15:47:08, kenrb wrote: > > > lgtm > > > > > > One thing to be cautious about is to not assume that you receive the commit > > > message before the first paint notification, but AFAICT you are not assuming > > any > > > particular ordering so this should be fine. > > > > Oh, I was actually relying on the fact that the first commit message will be > > sent to the browser before the first paint notification. If we activate the > swap > > promises after queuing the commit message, is there a reason why we can't > assume > > ordering between these 2 events? > > I'm not sure, but I might have caused confusion by being loose with my > terminology, so just to make sure we are on the same page with what we are > talking about: by 'commit message' I was thinking of DidCommitProvisionalLoad, > whereas you are watching for DidFinishLoad, and I was noting that messages > queued from the main renderer thread aren't ordered relative to compositor frame > swaps which happen on the compositor thread. (I wasn't talking about committing > the compositor frame.) Oh sorry, that's my bad too. When you said commit message I assumed we are talking about protobuf messages from the renderer's compositor to the browser. > > Looking at the handlers in PageLoadTracker, I don't see how you are assuming > that ordering here, but I might be misunderstanding something about the patch. > If we are indeed talking about DidFinishLoad vs DidFirstPaintAfterLoad ordering, > then I am not clear on a basis for an ordering assumption. You're right, I am not assuming any ordering between the DidFinishLoad and DidFirstPaintAfterLoad messages.
Thanks Ken!
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1986883002/#ps60001 (title: "Update comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986883002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986883002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, wez@chromium.org, kenrb@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1986883002/#ps80001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986883002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986883002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, wez@chromium.org, kenrb@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1986883002/#ps100001 (title: "Fix test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986883002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986883002/100001
Message was sent while issue was closed.
Description was changed from ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message. BUG=603220 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message. BUG=603220 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message. BUG=603220 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== blimp: Update page load status update indicator to use first paint. The Blimp client uses the load progress status from the engine to indicate page load progress for a navigation. While this signal informs us whether the engine has downloaded all the resources for a page, it can not be used reliably to indicate whether the client has received any frames/compositor commits for this navigation. This change uses 2 signals on the engine to indicate to the client whether a navigation is complete: 1) When the engine is finished loading all the resources for the main frame. 2) When the first paint has been performed for the main frame after a navigation. The first paint implies a commit message has been sent to the client. The renderer uses compositor's SwapPromises to tie events, like the first paint, with CompositorFrames and delivers them together to the browser. Since Blimp uses the remote server LayerTreeHost on the engine, which does not produce compositor frames and has no output surface, the renderer now sends these messages after activation instead when using remote compositing. Also, the remote compositor will activate these promises immediately after sending the commit message. BUG=603220 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/f3b62b6ed72d6405728899bf76aea9add6aee5fa Cr-Commit-Position: refs/heads/master@{#396290} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f3b62b6ed72d6405728899bf76aea9add6aee5fa Cr-Commit-Position: refs/heads/master@{#396290} |
