Deliver IPC messages together with SwapCompositorFrame

Deliver IPC messages together with SwapCompositorFrame

This makes it possible to send IPC messages in such a way
that they're delivered together with the SwapCompositorFrame
corresponding to the next commit.

BUG=364612
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284712

[mkosiba (inactive), 2014-04-17 18:44:10]

I'm submitting this for feedback on the overall solution/plumbing/naming - in
other words - does this kind of make sense or do you think I'm crazy?

[mkosiba (inactive), 2014-04-17 18:46:39]

On 2014/04/17 18:44:10, mkosiba wrote:
> I'm submitting this for feedback on the overall solution/plumbing/naming - in
> other words - does this kind of make sense or do you think I'm crazy?

also, for extra context there is a design doc (Google only):
https://docs.google.com/a/google.com/document/d/1TIcmXJnL5l8eHqAxZ17b8sGM27zI...

[danakj, 2014-04-17 18:55:03]

There is LatencyInfo tracking for determining when a change caused by an input
events gets to the screen. Does this overlap with it, by how much?

[piman, 2014-04-18 01:32:33]

On Thu, Apr 17, 2014 at 11:55 AM, <danakj@chromium.org> wrote:

> There is LatencyInfo tracking for determining when a change caused by an
> input
> events gets to the screen. Does this overlap with it, by how much?
>

Although LatencyInfo is wired in all the right places, and used for
measuring/testing I'm not so hot on using it for actual functional
behavior, because of the very loose semantics associated with things like
latency components and stuff. Namespacing issues, loose typing issues, etc.
In fact those issues creep up in subtle ways (e.g. layering violations
in LatencyComponentType, incorrect support for <webview> / out-of-process
iframes, implicit squashing of latency components, etc.).

At the same time there is definitely the need to associate data with things
as they flow through the compositor, as well as between browser and
renderers. Maybe we could come up with a more generic / safe system, that
would flow through the same places LatencyInfo does, with LatencyInfo being
a component of this system?
Other things I'm thinking of is the plugin moves (for NPAPI), if, say, we
wanted to get rid of the ViewHostMsg_UpdateRect, and the various things we
have in CompositorFrameMetadata.

I don't have anything concrete beyond that, but some area to think towards.


Regarding this CL, I would in particular like the information to flow with
existing messages, in particular ViewHostMsg_SwapCompositorFrame. In your
CL you're sending a message after commit, which doesn't mean the data "hit
the screen", because the actual draw with that data may not have happened.


> https://codereview.chromium.org/240163005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkosiba (inactive), 2014-04-23 13:52:48]

On 2014/04/18 01:32:33, piman wrote:
> On Thu, Apr 17, 2014 at 11:55 AM, <mailto:danakj@chromium.org> wrote:
> 
> > There is LatencyInfo tracking for determining when a change caused by an
> > input
> > events gets to the screen. Does this overlap with it, by how much?
> >
> 
> Although LatencyInfo is wired in all the right places, and used for
> measuring/testing I'm not so hot on using it for actual functional
> behavior, because of the very loose semantics associated with things like
> latency components and stuff. Namespacing issues, loose typing issues, etc.
> In fact those issues creep up in subtle ways (e.g. layering violations
> in LatencyComponentType, incorrect support for <webview> / out-of-process
> iframes, implicit squashing of latency components, etc.).
> 
> At the same time there is definitely the need to associate data with things
> as they flow through the compositor, as well as between browser and
> renderers. Maybe we could come up with a more generic / safe system, that
> would flow through the same places LatencyInfo does, with LatencyInfo being
> a component of this system?

Agree. I assumed LatencyInfo would not flow the same way as the load progress
reporting (it seemed like LatencyInfo is 100% tied to swaps) but having a more
general mechanism for this kind of think sounds like a good idea.
I was thinking we could do it as a specialized Send operation, something that
you would use like:

RenderWidget::Send(message, DELIVER_WITH_SWAP);
RenderWidget::Send(message, DELIVER_WITH_COMMIT);

under the covers this would batch the messages up till the next relevant
swap/commit. The swap/commit IPC would need to be expanded to carry this
extra payload which the RenderWidgetHost would unpack and deliver at
the right time.
Getting the information on the 'context' might be a bit tricky. I was thinking
exposing that as state on RWH would be the simplest. A better option would be
to expand the IPC delivery macros to allow us to specify an extra param, that
would nicely differentiate the methods that are called in this special way
from regular IPC.


> Other things I'm thinking of is the plugin moves (for NPAPI), if, say, we
> wanted to get rid of the ViewHostMsg_UpdateRect, and the various things we
> have in CompositorFrameMetadata.
> 
> I don't have anything concrete beyond that, but some area to think towards.
>

Do you have a preference on how we approach this? Should I start a discussion
on graphics-dev@chromium.org or should I try and implement a draft of the
above and we can discuss over the review?

> 
> Regarding this CL, I would in particular like the information to flow with
> existing messages, in particular ViewHostMsg_SwapCompositorFrame. In your
> CL you're sending a message after commit, which doesn't mean the data "hit
> the screen", because the actual draw with that data may not have happened.
> 

Ah, so, a couple of things:
 - I may be mixing things up, but the main reason this mechanism works that
   way can be explained by an example. Say I want to know when the page
   reached the final DID_FINISH_DOCUMENT_LOAD state. Possible combinations:
   a) no commit pending, no tree activation pending
      In this case the contents didn't change from the when the previous
      load progress notification was delivered, so we can send a 'regular'
      IPC to the browser and deliver it immediately since the content on
      screen already corresponds to the state the renderer is in now
   b) no commit pending, tree activation pending
      In this case the contents on the renderer side didn't change since
      the time the previous load progress notification was sent. We can
      send the IPC to the browser now, however the RVH will delay the
      actual callback till the pending tree activates.
      (in terms of source_frame_id's let's say the renderer's current
       s_f_id is 3, the active tree's is 2 and pending tree's is 3).
   c) commit pending
      In this case the events that had occurred before the renderer-side
      load progress notification had caused the visible contents to
      change. We need to delay sending the IPC till after the commit
      because we need to find out what the new source_frame_id is
      (we can't just +1 the current value since the commit might
       'early-out' and not increment the s_f_id).
 - for the webview I'm not really interested in the fact that the
   data is actually displayed on the screen. What I care about is
   whether the draw that occurs after the callback is delivered
   has the new data. Sorry for not being clear on that.
   This is mostly because the WebView might not be visible on the
   screen right up until a load progress with the right values is
   delivered, so making progress depend on swaps might cause a
   deadlock. 
   

> 
> > https://codereview.chromium.org/240163005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-24 20:58:20]

On Wed, Apr 23, 2014 at 6:52 AM, <mkosiba@chromium.org> wrote:

> On 2014/04/18 01:32:33, piman wrote:
>
>  On Thu, Apr 17, 2014 at 11:55 AM, <mailto:danakj@chromium.org> wrote:
>>
>
>  > There is LatencyInfo tracking for determining when a change caused by an
>> > input
>> > events gets to the screen. Does this overlap with it, by how much?
>> >
>>
>
>  Although LatencyInfo is wired in all the right places, and used for
>> measuring/testing I'm not so hot on using it for actual functional
>> behavior, because of the very loose semantics associated with things like
>> latency components and stuff. Namespacing issues, loose typing issues,
>> etc.
>> In fact those issues creep up in subtle ways (e.g. layering violations
>> in LatencyComponentType, incorrect support for <webview> / out-of-process
>> iframes, implicit squashing of latency components, etc.).
>>
>
>  At the same time there is definitely the need to associate data with
>> things
>> as they flow through the compositor, as well as between browser and
>> renderers. Maybe we could come up with a more generic / safe system, that
>> would flow through the same places LatencyInfo does, with LatencyInfo
>> being
>> a component of this system?
>>
>
> Agree. I assumed LatencyInfo would not flow the same way as the load
> progress
> reporting (it seemed like LatencyInfo is 100% tied to swaps) but having a
> more
> general mechanism for this kind of think sounds like a good idea.
> I was thinking we could do it as a specialized Send operation, something
> that
> you would use like:
>
> RenderWidget::Send(message, DELIVER_WITH_SWAP);
> RenderWidget::Send(message, DELIVER_WITH_COMMIT);
>
> under the covers this would batch the messages up till the next relevant
> swap/commit. The swap/commit IPC would need to be expanded to carry this
> extra payload which the RenderWidgetHost would unpack and deliver at
> the right time.
> Getting the information on the 'context' might be a bit tricky. I was
> thinking
> exposing that as state on RWH would be the simplest. A better option would
> be
> to expand the IPC delivery macros to allow us to specify an extra param,
> that
> would nicely differentiate the methods that are called in this special way
> from regular IPC.
>
>
>
>  Other things I'm thinking of is the plugin moves (for NPAPI), if, say, we
>> wanted to get rid of the ViewHostMsg_UpdateRect, and the various things we
>> have in CompositorFrameMetadata.
>>
>
>  I don't have anything concrete beyond that, but some area to think
>> towards.
>>
>
>
> Do you have a preference on how we approach this? Should I start a
> discussion
> on graphics-dev@chromium.org or should I try and implement a draft of the
> above and we can discuss over the review?
>
>
>
>  Regarding this CL, I would in particular like the information to flow with
>> existing messages, in particular ViewHostMsg_SwapCompositorFrame. In your
>> CL you're sending a message after commit, which doesn't mean the data "hit
>> the screen", because the actual draw with that data may not have happened.
>>
>
>
> Ah, so, a couple of things:
>  - I may be mixing things up, but the main reason this mechanism works that
>    way can be explained by an example. Say I want to know when the page
>    reached the final DID_FINISH_DOCUMENT_LOAD state. Possible combinations:
>    a) no commit pending, no tree activation pending
>       In this case the contents didn't change from the when the previous
>       load progress notification was delivered, so we can send a 'regular'
>       IPC to the browser and deliver it immediately since the content on
>       screen already corresponds to the state the renderer is in now
>    b) no commit pending, tree activation pending
>       In this case the contents on the renderer side didn't change since
>       the time the previous load progress notification was sent. We can
>       send the IPC to the browser now, however the RVH will delay the
>       actual callback till the pending tree activates.
>       (in terms of source_frame_id's let's say the renderer's current
>        s_f_id is 3, the active tree's is 2 and pending tree's is 3).
>    c) commit pending
>       In this case the events that had occurred before the renderer-side
>       load progress notification had caused the visible contents to
>       change. We need to delay sending the IPC till after the commit
>       because we need to find out what the new source_frame_id is
>       (we can't just +1 the current value since the commit might
>        'early-out' and not increment the s_f_id).
>  - for the webview I'm not really interested in the fact that the
>    data is actually displayed on the screen. What I care about is
>    whether the draw that occurs after the callback is delivered
>    has the new data. Sorry for not being clear on that.
>    This is mostly because the WebView might not be visible on the
>    screen right up until a load progress with the right values is
>    delivered, so making progress depend on swaps might cause a
>    deadlock.
>
>
>
Ok, sorry, I missed part of your CL originally, looking closer I understand
how it works a bit better.
So basically you're trying to reconcile state at commit time with state at
draw time, by matching the frame numbers. That's great, and close to what
we want, but I think we want to do that entirely on the renderer side. I
think we can do it. The browser shouldn't need to be aware of the behavior
on the renderer side wrt threads, or even source frame number.
Essentially you want the main thread to associate the data with a frame
number, and have the output surface pick that association on swap, by
matching the frame number, before sending the SwapCompositorFrame message.
A shared hash_map (+ a lock) should do it if done right. Then the
SwapCompositorFrame would directly include the data we need. I'll also make
sure we have fewer transient states in the browser, by having atomically
the frame and all the metadata associated with the frame in one set.
If we need a little bit more consistency wrt the source frame numbers to be
able to do that, we should do it.


>
>  > https://codereview.chromium.org/240163005/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/240163005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkosiba (inactive), 2014-05-06 22:30:33]

ok, got a super rough version of the patch up. I put in a comment in all of the
relevant files (the rest are just leftovers from the previous patch, so please
ignore). Just wanted to know if this is headed in the right direction, thanks!

https://codereview.chromium.org/240163005/diff/20001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/240163005/diff/20001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:45: std::vector<IPC::Message> payload;
metadata seems to be the most convenient place for hacking but I imagine we want
this to liv in SwapCompositorFrame directly?

https://codereview.chromium.org/240163005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/240163005/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1491:
OnDeliverPayload(frame->metadata.payload[i]);
the receiving end on browser side

https://codereview.chromium.org/240163005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/240163005/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1297: void
WebContentsImpl::DispatchMessageWithSwapFrame(const IPC::Message&) {
the web contents would just re-broadcast to all of the observers

https://codereview.chromium.org/240163005/diff/20001/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/240163005/diff/20001/content/public/browser/w...
content/public/browser/web_contents_observer.h:336: virtual bool
DispatchMessageWithSwapFrame(const IPC::Message& message) {}
this is where subclasses from different layers (think chrome/ vs
android_webview/) would hook in.

https://codereview.chromium.org/240163005/diff/20001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/240163005/diff/20001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:388: void
RenderWidgetCompositor::QueueSwapPromise(
we'd need a way to track whether the commit actually took place

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1369: void RenderWidget::QueueMessage(const
IPC::Message& msg) {
so this would be the entry point

[piman, 2014-05-07 01:21:46]

Couple of high-level comments.
Beware that LTH::QueueSwapPromise may fail in case of queue overflow, it'll call
DidNotSwap, but that means it'll be out-of-order wrt the swap, so as is it can't
be used as a reliable way of pipelining the state
OTOH, I'm not sure I agree with having such arbitrary limit, that may be
something to reconsider.

https://codereview.chromium.org/240163005/diff/20001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/240163005/diff/20001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:45: std::vector<IPC::Message> payload;
On 2014/05/06 22:30:33, mkosiba wrote:
> metadata seems to be the most convenient place for hacking but I imagine we
want
> this to liv in SwapCompositorFrame directly?

I don't think we want to make cc depend on IPC.

I think we can relatively easily have it on the message, if the RWSwapPromise
has a way to get to the CompositorOutputSurface, since DidSwap is called before
the actual swap (the CompositorOutputSurface can stash whatever info from the
RWSwapPromise, and include it with the SwapCompositorFrame IPC)

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1359: new
ViewHostMsg_DeliverPayload(fallback_routing_id_, msg));
How about fallback_sender_->Send(msg)? Is there a reason why we would want this
message to be treated specially?

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1359: new
ViewHostMsg_DeliverPayload(fallback_routing_id_, msg));
Also, beware, DidNotSwap can be called on either thread. It's not ok to call
RenderWidget::Send from the compositor thread.

You can use RenderThreadImpl::current()->sync_message_filter() to send from the
compositor thread, and RenderThreadImpl::current() to send from the main thread.

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1370: if (!compositor_->commitRequested()) {
I'm not sure if that isn't racy.

I think the contract of the SwapPromise is that it will call DidNotSwap if it
doesn't commit?

[mkosiba (inactive), 2014-05-07 18:27:07]

On 2014/05/07 01:21:46, piman wrote:
> Couple of high-level comments.
Thanks!

> Beware that LTH::QueueSwapPromise may fail in case of queue overflow, it'll
call
> DidNotSwap, but that means it'll be out-of-order wrt the swap, so as is it
can't
> be used as a reliable way of pipelining the state
> OTOH, I'm not sure I agree with having such arbitrary limit, that may be
> something to reconsider.
Yeath, I was thinking about that - the problem extends to the entire mechanism
I guess - you could imagine that if the commit is stalled for some reason
(possible in WebView where the cc impl is merged with the UI thread) we could
queue up a lot of data to be sent with the next commit.
Maybe a 'soft' limit would be helpful? For the API I'm adding it would be fine
to send messages out-of-sync with the swap if we're too backed up on the
renderer
thread (not that they'd get through, we'd just be filling up the IPC channel
till it starts blocking the renderer).

[mkosiba (inactive), 2014-05-07 18:27:20]

https://codereview.chromium.org/240163005/diff/20001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/240163005/diff/20001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:45: std::vector<IPC::Message> payload;
On 2014/05/07 01:21:46, piman wrote:
> On 2014/05/06 22:30:33, mkosiba wrote:
> > metadata seems to be the most convenient place for hacking but I imagine we
> want
> > this to liv in SwapCompositorFrame directly?
> 
> I don't think we want to make cc depend on IPC.
> 
> I think we can relatively easily have it on the message, if the RWSwapPromise
> has a way to get to the CompositorOutputSurface, since DidSwap is called
before
> the actual swap (the CompositorOutputSurface can stash whatever info from the
> RWSwapPromise, and include it with the SwapCompositorFrame IPC)

true. The other reason I'm gravitating towards metadata is because the WebView
synchronous compositor doesn't use SwapCompositorFrame (and does use metadata)
but I could equally well add a bit of extra plumbing to the synchronous CC that
replicates whatever CompositorOutputSurface does).

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1359: new
ViewHostMsg_DeliverPayload(fallback_routing_id_, msg));
On 2014/05/07 01:21:46, piman wrote:
> Also, beware, DidNotSwap can be called on either thread. It's not ok to call
> RenderWidget::Send from the compositor thread.
thanks for pointing that out!

> How about fallback_sender_->Send(msg)? Is there a reason why we would want
this
> message to be treated specially?

yes. The way I envisioned this was that you'd only ever call QueueMessage with
control IPCs (that is I don't want whoever's calling QueueMessage to be able to
specify a routing id). I was planning on using IPC for their serialization
ability and the dispatch mechanism, not for the routing.
Ideally I'd use a Pickle that can be dispatched, and I'm not sure which is worse
- fiddling with the ipc_messages_macros file or the current solution :P

The other reason for wrapping msg in ViewHostMsg_DeliverPayload is to make it
easier to deliver the message to the designated WebContentsObserver callback.
Otherwise it would be confusing, I think:
1) you wouldn't be able to distinguish whether the message can be delivered in
this special way,
2) It would be cumbersome to plumb through a way for the receiver to know
whether the message was sent from DidNotSwap or DidSwap.
3) Re-injecting the messages from RenderWidgetHost into the RenderProcessHost's
message dispatcher feels hacky.

https://codereview.chromium.org/240163005/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1370: if (!compositor_->commitRequested()) {
On 2014/05/07 01:21:46, piman wrote:
> I'm not sure if that isn't racy.
> 
> I think the contract of the SwapPromise is that it will call DidNotSwap if it
> doesn't commit?

oh.. I assumed that this tracks whether the Blink thread requested a commit. Is
that not the case?

[piman, 2014-05-07 21:17:33]

On Wed, May 7, 2014 at 11:27 AM, <mkosiba@chromium.org> wrote:

>
> https://codereview.chromium.org/240163005/diff/20001/cc/
> output/compositor_frame_metadata.h
> File cc/output/compositor_frame_metadata.h (right):
>
> https://codereview.chromium.org/240163005/diff/20001/cc/
> output/compositor_frame_metadata.h#newcode45
> cc/output/compositor_frame_metadata.h:45: std::vector<IPC::Message>
> payload;
> On 2014/05/07 01:21:46, piman wrote:
>
>> On 2014/05/06 22:30:33, mkosiba wrote:
>> > metadata seems to be the most convenient place for hacking but I
>>
> imagine we
>
>> want
>> > this to liv in SwapCompositorFrame directly?
>>
>
>  I don't think we want to make cc depend on IPC.
>>
>
>  I think we can relatively easily have it on the message, if the
>>
> RWSwapPromise
>
>> has a way to get to the CompositorOutputSurface, since DidSwap is
>>
> called before
>
>> the actual swap (the CompositorOutputSurface can stash whatever info
>>
> from the
>
>> RWSwapPromise, and include it with the SwapCompositorFrame IPC)
>>
>
> true. The other reason I'm gravitating towards metadata is because the
> WebView synchronous compositor doesn't use SwapCompositorFrame (and does
> use metadata) but I could equally well add a bit of extra plumbing to
> the synchronous CC that replicates whatever CompositorOutputSurface
> does).


We want to use cc/ in a non-content world (think pepper, mojo), we
shouldn't add a dependency on ipc.
So I'd prefer if we handle messages in the embedder. Maybe it makes sense
to have a common class to be able to do that in
both CompositorOutputSurface and WebView.


>
> https://codereview.chromium.org/240163005/diff/20001/
> content/renderer/render_widget.cc
> File content/renderer/render_widget.cc (right):
>
> https://codereview.chromium.org/240163005/diff/20001/
> content/renderer/render_widget.cc#newcode1359
> content/renderer/render_widget.cc:1359: new
> ViewHostMsg_DeliverPayload(fallback_routing_id_, msg));
> On 2014/05/07 01:21:46, piman wrote:
>
>> Also, beware, DidNotSwap can be called on either thread. It's not ok
>>
> to call
>
>> RenderWidget::Send from the compositor thread.
>>
> thanks for pointing that out!
>
>
>  How about fallback_sender_->Send(msg)? Is there a reason why we would
>>
> want this
>
>> message to be treated specially?
>>
>
> yes. The way I envisioned this was that you'd only ever call
> QueueMessage with control IPCs (that is I don't want whoever's calling
> QueueMessage to be able to specify a routing id).


Why?
I think I'd like to use the mechanism to handle the existing UpdateRect
message, that's routed.


> I was planning on
> using IPC for their serialization ability and the dispatch mechanism,
> not for the routing.
> Ideally I'd use a Pickle that can be dispatched, and I'm not sure which
> is worse - fiddling with the ipc_messages_macros file or the current
> solution :P
>
> The other reason for wrapping msg in ViewHostMsg_DeliverPayload is to
> make it easier to deliver the message to the designated
> WebContentsObserver callback. Otherwise it would be confusing, I think:
> 1) you wouldn't be able to distinguish whether the message can be
> delivered in this special way,
>

That's a bit of circular logic :)


> 2) It would be cumbersome to plumb through a way for the receiver to
> know whether the message was sent from DidNotSwap or DidSwap.
>

Sounds like just a bool would do.


> 3) Re-injecting the messages from RenderWidgetHost into the
> RenderProcessHost's message dispatcher feels hacky.


I don't think you'd need to do that.
Any message you want to send together with the frame should be routed the
same way.
If the handlers of those messages dispatch the message to
WebContentsObserver, all the power to it, but there's no fundamental reason
to force this mechanism?


>
>
> https://codereview.chromium.org/240163005/diff/20001/
> content/renderer/render_widget.cc#newcode1370
> content/renderer/render_widget.cc:1370: if
> (!compositor_->commitRequested()) {
> On 2014/05/07 01:21:46, piman wrote:
>
>> I'm not sure if that isn't racy.
>>
>
>  I think the contract of the SwapPromise is that it will call
>>
> DidNotSwap if it
>
>> doesn't commit?
>>
>
> oh.. I assumed that this tracks whether the Blink thread requested a
> commit. Is that not the case?
>

Not exactly... it's set when something SetNeedsCommit, but it's also set on
a BeginMainFrame coming from the compositor thread.


>
> https://codereview.chromium.org/240163005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-05-07 21:23:41]

On Wed, May 7, 2014 at 11:27 AM, <mkosiba@chromium.org> wrote:

> On 2014/05/07 01:21:46, piman wrote:
>
>> Couple of high-level comments.
>>
> Thanks!
>
>
>  Beware that LTH::QueueSwapPromise may fail in case of queue overflow,
>> it'll
>>
> call
>
>> DidNotSwap, but that means it'll be out-of-order wrt the swap, so as is it
>>
> can't
>
>> be used as a reliable way of pipelining the state
>> OTOH, I'm not sure I agree with having such arbitrary limit, that may be
>> something to reconsider.
>>
> Yeath, I was thinking about that - the problem extends to the entire
> mechanism
> I guess - you could imagine that if the commit is stalled for some reason
> (possible in WebView where the cc impl is merged with the UI thread) we
> could
> queue up a lot of data to be sent with the next commit.

Maybe a 'soft' limit would be helpful?


What would the 'soft' limit mean when you reach it?


> For the API I'm adding it would be fine
> to send messages out-of-sync with the swap if we're too backed up on the
> renderer
> thread (not that they'd get through, we'd just be filling up the IPC
> channel
> till it starts blocking the renderer).
>

Actually, you won't fill up the IPC channel, you'll just allocate more
memory, because Send posts a task to the IO thread and keeps it in a queue
until the pipe is available for writing - and actually the messages will go
through to the browser IO thread since that's not blocked.

Are there cases that would cause QueueMssage that are not either initiated
by or throttled by the UI thread?


>
>
> https://codereview.chromium.org/240163005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkosiba (inactive), 2014-05-08 19:04:18]

Thanks for the great comments Antoine!
I think I have a better idea of how this would work, new patch coming soon.

On 2014/05/07 21:17:33, piman wrote:
> > 3) Re-injecting the messages from RenderWidgetHost into the
> > RenderProcessHost's message dispatcher feels hacky.
>
> I don't think you'd need to do that.
> Any message you want to send together with the frame should be routed the
> same way.

ok, so I was assuming we want the IPC messages we want to deliver with
the SwapCompositorFrame message to be embedded in that message.
I don't think the IPC layer allows you to send messages in batches
that are guaranteed to be dispatched together?

> If the handlers of those messages dispatch the message to
> WebContentsObserver, all the power to it, but there's no fundamental reason
> to force this mechanism?

not really, no. I'll do 'regular' routing in the next iteration.

[piman, 2014-05-08 20:49:12]

On Thu, May 8, 2014 at 12:04 PM, <mkosiba@chromium.org> wrote:

> Thanks for the great comments Antoine!
> I think I have a better idea of how this would work, new patch coming soon.
>
>
> On 2014/05/07 21:17:33, piman wrote:
>
>> > 3) Re-injecting the messages from RenderWidgetHost into the
>> > RenderProcessHost's message dispatcher feels hacky.
>>
>
>  I don't think you'd need to do that.
>> Any message you want to send together with the frame should be routed the
>> same way.
>>
>
> ok, so I was assuming we want the IPC messages we want to deliver with
> the SwapCompositorFrame message to be embedded in that message.
> I don't think the IPC layer allows you to send messages in batches
> that are guaranteed to be dispatched together?


I was thinking, still send it via the SwapCompositorFrame IPC, and when
received, dispatch via, say, RPH::OnMessageReceived. On the fallback path,
if they're just sent regularly, they will be dispatched that way too.


>
>  If the handlers of those messages dispatch the message to
>> WebContentsObserver, all the power to it, but there's no fundamental
>> reason
>> to force this mechanism?
>>
>
> not really, no. I'll do 'regular' routing in the next iteration.
>
>
>
> https://codereview.chromium.org/240163005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkosiba (inactive), 2014-05-13 19:15:25]

hoping this is closer to what you were thinking of, PTAL. Still need to
implement the bit where this is hooked up to the synchronous compositor. Treat
the TODOs as things I'll do in the next patch set.

https://codereview.chromium.org/240163005/diff/40001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/240163005/diff/40001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:38: int source_frame_number;
this is only ever used by the SwapPromise. You think it would be better to pass
this in as an extra param to DidSwap?

https://codereview.chromium.org/240163005/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/240163005/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1494:
GetProcess()->OnMessageReceived(*i);
reentrant IPC dispatch seems shady to me.

https://codereview.chromium.org/240163005/diff/40001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/240163005/diff/40001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:428: int
RenderWidgetCompositor::source_frame_number() {
probably don't need this.

https://codereview.chromium.org/240163005/diff/40001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/40001/content/renderer/render_...
content/renderer/render_widget.cc:1246: // TODO(mkosiba): move most of this to
RenderWidgetCompositor, although
..although it might be best to have this as a separate .cc/.h file.

[mkosiba (inactive), 2014-05-14 10:08:07]

https://codereview.chromium.org/240163005/diff/40001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/40001/content/renderer/render_...
content/renderer/render_widget.cc:1266: if
(!message_queue_->TryQueueMessage(metadata->source_frame_number, msg_)) {
the interesting case is how to handle messages that are being scheduled on the
impl thread after DidSwap is called but before the actual message is sent.
We'd need to postpone sending those messages till after the OutputSurface had
sent the IPC.
I was thinking of something along the lines of having two queues in the
FrameMessageQueue. We'd add to this queue instead of making the call to Send in
line 1287 and drain it from RenderWidget::didCompleteSwapBuffers.

I was also thinking that waiting for didSwapBuffers instead of
didCompleteSwapBuffers would be better in this context. WDYT?

[mkosiba (inactive), 2014-05-15 17:11:48]

https://codereview.chromium.org/240163005/diff/40001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/40001/content/renderer/render_...
content/renderer/render_widget.cc:1266: if
(!message_queue_->TryQueueMessage(metadata->source_frame_number, msg_)) {
On 2014/05/14 10:08:08, mkosiba wrote:
> the interesting case is how to handle messages that are being scheduled on the
> impl thread after DidSwap is called but before the actual message is sent.

Simpler option: acquire lock on FrameSwapMessageQueue before calling Drain,
release lock after calling Send on the IPC. :)

[mkosiba (inactive), 2014-05-20 13:19:47]

[this issue used to be called [draft] Improve renderer load progress reporting.]

I've reduced the change to only introduce this new mechanism and updated the
title to reflect that.

[piman, 2014-05-20 19:54:04]

https://codereview.chromium.org/240163005/diff/80001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/240163005/diff/80001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:38: int source_frame_number;
This has no meaning in the parent compositor (browser), so I don't think it's
worth having in here and IPC over.

You need it in the OutputSurface, but you could very well add a
GetSourceFrameNumber() to OutputSurfaceClient

https://codereview.chromium.org/240163005/diff/80001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/240163005/diff/80001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:42:
}
You may want to check with boliu@ if this is the right thing to do.
Another possibility could be to pass an IPC::Listener (the RenderProcessHost)
explicitly to the SynchronousCompositorOutputSurface.

https://codereview.chromium.org/240163005/diff/80001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:164:
std::vector<IPC::Message> messages_to_deliver_with_frame;
nit: indent

https://codereview.chromium.org/240163005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1484:
messages_to_deliver_with_frame.clear();
Do you need these to be delivered before the OnSwapCompositorFrame?
For the other things I would like to use this for (e.g. the UpdateRect message),
it would make more sense to deliver them after the frame.
It would also slightly alleviate re-entrency concerns if it's at the end of the
method, for example since these messages could be ViewHostMsg_Close or
ViewHostMsg_RenderProcessGone which would end up deleting |this|. For that
reason too, you may want to grab the RenderProcessHost before the loop by the
way (the RPH deletes itself with a DeleteSoon, so there is no risk of it going
away in the loop).

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/com...
content/renderer/gpu/compositor_output_surface.cc:171:
FrameSwapMessageQueue::SendMessageScope send_message_scope =
nit: indent.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/fra...
File content/renderer/gpu/frame_swap_message_queue.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/fra...
content/renderer/gpu/frame_swap_message_queue.cc:20:
FrameSwapMessageQueue::~FrameSwapMessageQueue() {
There may be still messages in the map here. Delete them?
Alternatively, use a linked_ptr in the map.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/fra...
content/renderer/gpu/frame_swap_message_queue.cc:49: i !=
queued_messages_.upper_bound(source_frame_number);
You likely want to extract the upper_bound out of the loop since it's O(log n).

https://codereview.chromium.org/240163005/diff/80001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:1262: if (!compositor_ ||
!compositor_->commitRequested()) {
I think I would move the commitRequested() logic to the caller.
Some other use cases (I'm thinking, ViewHostMsg_UpdateRect) would want to wait
for the commit, even if one is not requested yet.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:1269:
compositor_->QueueSwapPromise(promise.Pass());
I still have concerns about the arbitrary limit in LTH::QueueSwapPromise.
Looking back at the CL, I think it was added "just in case" but is not well
founded.

[mkosiba (inactive), 2014-05-22 17:40:23]

https://codereview.chromium.org/240163005/diff/80001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/240163005/diff/80001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:38: int source_frame_number;
On 2014/05/20 19:54:05, piman wrote:
> This has no meaning in the parent compositor (browser), so I don't think it's
> worth having in here and IPC over.
> 
> You need it in the OutputSurface, but you could very well add a
> GetSourceFrameNumber() to OutputSurfaceClient

Done.

https://codereview.chromium.org/240163005/diff/80001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/240163005/diff/80001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:42:
}
On 2014/05/20 19:54:05, piman wrote:
> You may want to check with boliu@ if this is the right thing to do.
> Another possibility could be to pass an IPC::Listener (the RenderProcessHost)
> explicitly to the SynchronousCompositorOutputSurface.

Bo's already on the review. I moved this to SynchronousCompositorImpl since that
class already has the needed information. Adding DispatchMessages to the
delegate still feels a bit weird but probably better than this.

https://codereview.chromium.org/240163005/diff/80001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:164:
std::vector<IPC::Message> messages_to_deliver_with_frame;
On 2014/05/20 19:54:05, piman wrote:
> nit: indent

Done.

https://codereview.chromium.org/240163005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1484:
messages_to_deliver_with_frame.clear();
On 2014/05/20 19:54:05, piman wrote:
> Do you need these to be delivered before the OnSwapCompositorFrame?

nope.

> For the other things I would like to use this for (e.g. the UpdateRect
message),
> it would make more sense to deliver them after the frame.
> It would also slightly alleviate re-entrency concerns if it's at the end of
the
> method, for example since these messages could be ViewHostMsg_Close or
> ViewHostMsg_RenderProcessGone which would end up deleting |this|. For that
> reason too, you may want to grab the RenderProcessHost before the loop by the
> way (the RPH deletes itself with a DeleteSoon, so there is no risk of it going
> away in the loop).

Those are some good points. I'll move the code to the bottom of the method.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/com...
content/renderer/gpu/compositor_output_surface.cc:171:
FrameSwapMessageQueue::SendMessageScope send_message_scope =
On 2014/05/20 19:54:05, piman wrote:
> nit: indent.

Done.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/fra...
File content/renderer/gpu/frame_swap_message_queue.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/fra...
content/renderer/gpu/frame_swap_message_queue.cc:20:
FrameSwapMessageQueue::~FrameSwapMessageQueue() {
On 2014/05/20 19:54:05, piman wrote:
> There may be still messages in the map here. Delete them?
> Alternatively, use a linked_ptr in the map.

Done.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/fra...
content/renderer/gpu/frame_swap_message_queue.cc:49: i !=
queued_messages_.upper_bound(source_frame_number);
On 2014/05/20 19:54:05, piman wrote:
> You likely want to extract the upper_bound out of the loop since it's O(log
n).

Done.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/gpu/fra...
content/renderer/gpu/frame_swap_message_queue.cc:49: i !=
queued_messages_.upper_bound(source_frame_number);
On 2014/05/20 19:54:05, piman wrote:
> You likely want to extract the upper_bound out of the loop since it's O(log
n).

Done.

https://codereview.chromium.org/240163005/diff/80001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:1262: if (!compositor_ ||
!compositor_->commitRequested()) {
On 2014/05/20 19:54:05, piman wrote:
> I think I would move the commitRequested() logic to the caller.
> Some other use cases (I'm thinking, ViewHostMsg_UpdateRect) would want to wait
> for the commit, even if one is not requested yet.

Originally I was thinking of having an enum specifying how you want the message
to be sent. I think it would make sense to somehow couple the policy on how to
deliver the message with the type of the message being sent. Do you think that
makes sense?

https://codereview.chromium.org/240163005/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:1269:
compositor_->QueueSwapPromise(promise.Pass());
On 2014/05/20 19:54:05, piman wrote:
> I still have concerns about the arbitrary limit in LTH::QueueSwapPromise.
> Looking back at the CL, I think it was added "just in case" but is not well
> founded.

ok, I removed the limit. I can add miletus@ to the review as FYI

https://codereview.chromium.org/240163005/diff/120001/content/renderer/messag...
File content/renderer/message_delivery_policy_lookup.h (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/messag...
content/renderer/message_delivery_policy_lookup.h:21: struct
MessageDeliveryPolicyLookup {};
It's not ideal (especially the bit where you forward-declare messages) but I
think it's better than requiring folks to specify the policy on each
QueueMessage call.

Maybe this could be some sort of optional message trait?

[mkosiba (inactive), 2014-05-22 17:41:32]

+CC miletus@

miletus,
  In this change I wanted to remove the limit for the max number of
SwapPromises. Was there a particular reason you put that in or was it just in
case?

[Yufeng Shen (Slow to review), 2014-05-22 18:38:21]

On 2014/05/22 17:41:32, mkosiba wrote:
> +CC miletus@
> 
> miletus,
>   In this change I wanted to remove the limit for the max number of
> SwapPromises. Was there a particular reason you put that in or was it just in
> case?

I am fine with that. But please also remove SWAP_PROMISE_LIST_OVERFLOW and where
it is used.

[piman, 2014-05-22 23:32:01]

Trying to simplify things...

https://codereview.chromium.org/240163005/diff/120001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:171:
FrameSwapMessageQueue::SendMessageScope send_message_scope =
Do we really need to expose this lock?

The only case it would make a difference is in RenderWidget::QueueMessage in the
MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP case, where there's no guarantee anyway
about which frame the message will go with. (I'm not even sure in which case
this policy is useful).

https://codereview.chromium.org/240163005/diff/120001/content/renderer/messag...
File content/renderer/message_delivery_policy_lookup.h (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/messag...
content/renderer/message_delivery_policy_lookup.h:21: struct
MessageDeliveryPolicyLookup {};
On 2014/05/22 17:40:24, mkosiba wrote:
> It's not ideal (especially the bit where you forward-declare messages) but I
> think it's better than requiring folks to specify the policy on each
> QueueMessage call.
> 
> Maybe this could be some sort of optional message trait?

It seems overkill to me.
There's going to be maybe a handful of messages that need to piggy-back on the
SwapCompositorFrame. Specifying the policy at the callsite doesn't seem like
much of a burden.

https://codereview.chromium.org/240163005/diff/120001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/render...
content/renderer/render_widget.cc:173: delete msg_;
Maybe we should have LayerTreeHost / LayerTreeImpl break their promises on
destruction, that would be more consistent. Then we can DCHECK(!msg_) here.

https://codereview.chromium.org/240163005/diff/120001/content/renderer/render...
content/renderer/render_widget.cc:178: if
(!message_queue_->TryQueueMessage(source_frame_number, msg_)) {
So, I don't think this can ever fail.

SwapPromise::DidSwap is called right before OutputSurface::SwapBuffers, which is
where we call DrainMessages (with the same frame number).

[boliu, 2014-05-23 06:26:59]

https://codereview.chromium.org/240163005/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/240163005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:151:
// We don't care about the SendMessageScope since we're already on the UI
This might not be UI in webview until we turn on ubercomp on latest android.

https://codereview.chromium.org/240163005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:154:
frame_swap_message_queue_->DrainMessages(client_->GetSourceFrameNumber(),
Should the SourceFrameNumber be part of CompositorFrame, ie
CompositorFrameMetadata?

[mkosiba (inactive), 2014-05-23 15:50:37]

Thanks! Patch is ~100 lines less now.

https://codereview.chromium.org/240163005/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/240163005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:151:
// We don't care about the SendMessageScope since we're already on the UI
On 2014/05/23 06:27:00, boliu wrote:
> This might not be UI in webview until we turn on ubercomp on latest android.

What?! Then the DCHECK(CalledOnValidThread()); on Line 141 is super-misleading.

I'll refactor this to be a post to the UI thread (like UpdateFrameMetaData is).

https://codereview.chromium.org/240163005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:154:
frame_swap_message_queue_->DrainMessages(client_->GetSourceFrameNumber(),
On 2014/05/23 06:27:00, boliu wrote:
> Should the SourceFrameNumber be part of CompositorFrame, ie
> CompositorFrameMetadata?

Don't need it at all now.

https://codereview.chromium.org/240163005/diff/120001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:171:
FrameSwapMessageQueue::SendMessageScope send_message_scope =
On 2014/05/22 23:32:01, piman wrote:
> Do we really need to expose this lock?
> 
> The only case it would make a difference is in RenderWidget::QueueMessage in
the
> MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP case, where there's no guarantee anyway
> about which frame the message will go with. (I'm not even sure in which case
> this policy is useful).

The reason I added it was for MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE to
preserve FIFO ordering but as I was writing this I realized it actually doesn't
work since I'm not synchronizing anything on QueueMessage.

Below is an example of what I wanted to prevent. Let's assume I want to use that
policy to send IPC about frame load progress. Sample scenario:

[main thread]
- source_frame_number = 1
- setNeedsCommit
- didCommitProvisionalLoad -> FrameHostMsg_DidCommitProvisionalLoad
(WITH_VISUAL_STATE)
  we create a SwapPromise
- commit (source_frame_number => 2, needsCommit==false)
[impl thread]
- OutputSurface::SwapFrame()
   FrameMessageQueue::Drain(source_frame_number, messages)
[main thread]
- DidFinishLoad -> FrameHostMsg_DidFinishLoad (WITH_VISUAL_STATE)
  We go into QueueMessage, needsCommit == false, so we send the message using
Send()
[impl thread]
   Send(ViewHostMsg_SwapCompositorFrame(..., messages));
..

The effect of this is that we send FrameHostMsg_DidFinishLoad before
ViewHostMsg_SwapCompositorFrame which causes us to deliver
FrameHostMsg_DidFinishLoad before FrameHostMsg_DidCommitProvisionalLoad.

For my purposes it's not good enough to always schedule
FrameHostMsg_DidFinishLoad with the next swap: if we assume an application is
using this to decide when to animate the webview into view then waiting for the
next swap will add unnecessary latency.

https://codereview.chromium.org/240163005/diff/120001/content/renderer/messag...
File content/renderer/message_delivery_policy_lookup.h (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/messag...
content/renderer/message_delivery_policy_lookup.h:21: struct
MessageDeliveryPolicyLookup {};
On 2014/05/22 23:32:01, piman wrote:
> On 2014/05/22 17:40:24, mkosiba wrote:
> > It's not ideal (especially the bit where you forward-declare messages) but I
> > think it's better than requiring folks to specify the policy on each
> > QueueMessage call.
> > 
> > Maybe this could be some sort of optional message trait?
> 
> It seems overkill to me.
> There's going to be maybe a handful of messages that need to piggy-back on the
> SwapCompositorFrame. Specifying the policy at the callsite doesn't seem like
> much of a burden.

Done.

https://codereview.chromium.org/240163005/diff/120001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/render...
content/renderer/render_widget.cc:173: delete msg_;
On 2014/05/22 23:32:01, piman wrote:
> Maybe we should have LayerTreeHost / LayerTreeImpl break their promises on
> destruction, that would be more consistent. Then we can DCHECK(!msg_) here.

Done.

https://codereview.chromium.org/240163005/diff/120001/content/renderer/render...
content/renderer/render_widget.cc:178: if
(!message_queue_->TryQueueMessage(source_frame_number, msg_)) {
On 2014/05/22 23:32:01, piman wrote:
> So, I don't think this can ever fail.
> 
> SwapPromise::DidSwap is called right before OutputSurface::SwapBuffers, which
is
> where we call DrainMessages (with the same frame number).

yes, you're right. This simplifies things a bit.

[mkosiba (inactive), 2014-05-23 15:50:43]

Thanks! Patch is ~100 lines less now.

[piman, 2014-05-23 20:26:35]

https://codereview.chromium.org/240163005/diff/120001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/240163005/diff/120001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:171:
FrameSwapMessageQueue::SendMessageScope send_message_scope =
On 2014/05/23 15:50:38, mkosiba wrote:
> On 2014/05/22 23:32:01, piman wrote:
> > Do we really need to expose this lock?
> > 
> > The only case it would make a difference is in RenderWidget::QueueMessage in
> the
> > MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP case, where there's no guarantee
anyway
> > about which frame the message will go with. (I'm not even sure in which case
> > this policy is useful).
> 
> The reason I added it was for MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE to
> preserve FIFO ordering but as I was writing this I realized it actually
doesn't
> work since I'm not synchronizing anything on QueueMessage.
> 
> Below is an example of what I wanted to prevent. Let's assume I want to use
that
> policy to send IPC about frame load progress. Sample scenario:
> 
> [main thread]
> - source_frame_number = 1
> - setNeedsCommit
> - didCommitProvisionalLoad -> FrameHostMsg_DidCommitProvisionalLoad
> (WITH_VISUAL_STATE)
>   we create a SwapPromise
> - commit (source_frame_number => 2, needsCommit==false)
> [impl thread]
> - OutputSurface::SwapFrame()
>    FrameMessageQueue::Drain(source_frame_number, messages)
> [main thread]
> - DidFinishLoad -> FrameHostMsg_DidFinishLoad (WITH_VISUAL_STATE)
>   We go into QueueMessage, needsCommit == false, so we send the message using
> Send()
> [impl thread]
>    Send(ViewHostMsg_SwapCompositorFrame(..., messages));
> ..
> 
> The effect of this is that we send FrameHostMsg_DidFinishLoad before
> ViewHostMsg_SwapCompositorFrame which causes us to deliver
> FrameHostMsg_DidFinishLoad before FrameHostMsg_DidCommitProvisionalLoad.
> 
> For my purposes it's not good enough to always schedule
> FrameHostMsg_DidFinishLoad with the next swap: if we assume an application is
> using this to decide when to animate the webview into view then waiting for
the
> next swap will add unnecessary latency.


If I modify your scenario a bit:

[main thread]
- source_frame_number = 1
- setNeedsCommit
- didCommitProvisionalLoad -> FrameHostMsg_DidCommitProvisionalLoad
(WITH_VISUAL_STATE)
  we create a SwapPromise
- commit (source_frame_number => 2, needsCommit==false)
- DidFinishLoad -> FrameHostMsg_DidFinishLoad (WITH_VISUAL_STATE)
  We go into QueueMessage, needsCommit == false, so we send the message using
Send()
[impl thread]
- OutputSurface::SwapFrame()
   FrameMessageQueue::Drain(source_frame_number, messages)
   Send(ViewHostMsg_SwapCompositorFrame(..., messages));


The lock doesn't save you in this case, you still have out-of-order messages.

It sounds like what you want, if you want to conserve order, you need to take
into account whether or not a SwapPromise is in flight, and whether or not the
queue is empty

https://codereview.chromium.org/240163005/diff/130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/240163005/diff/130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:149:
BreakSwapPromises(SwapPromise::DID_NOT_SWAP_UNKNOWN);
COMMIT_FAILED? Basically if we still have a SwapPromise, it means we never
committed it.
Or maybe we should add a specific state like "DESTROYED"

https://codereview.chromium.org/240163005/diff/130001/content/renderer/gpu/fr...
File content/renderer/gpu/frame_swap_message_queue.cc (right):

https://codereview.chromium.org/240163005/diff/130001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.cc:47:
DCHECK(send_message_scope_aquired_);
How about lock_.AssertAcquired()? That way you don't need
send_message_scope_aquired_, which is not thread safe if the lock is not,
actually, taken (i.e. you could have false positives).

[mkosiba (inactive), 2014-06-18 14:58:03]

hey! looks like everyone's back from vacation so I'm going to take the patch out
of the icebox.

PTAL

https://codereview.chromium.org/240163005/diff/130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/240163005/diff/130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:149:
BreakSwapPromises(SwapPromise::DID_NOT_SWAP_UNKNOWN);
On 2014/05/23 20:26:36, piman wrote:
> COMMIT_FAILED? Basically if we still have a SwapPromise, it means we never
> committed it.
> Or maybe we should add a specific state like "DESTROYED"

COMMIT_FAILS makes more sense. If we wanted a specific DESTROYED state we'd need
2, which seems like overkill (one for LTH and the other for LTHImpl, right?).
I'm going to do the same for LTHImpl.

Also, looks like unknown is only used for tests (no production code breaks a
promise for reason=unknown), maybe we should get rid of it.

https://codereview.chromium.org/240163005/diff/130001/content/renderer/gpu/fr...
File content/renderer/gpu/frame_swap_message_queue.cc (right):

https://codereview.chromium.org/240163005/diff/130001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.cc:47:
DCHECK(send_message_scope_aquired_);
On 2014/05/23 20:26:36, piman wrote:
> How about lock_.AssertAcquired()? That way you don't need
> send_message_scope_aquired_, which is not thread safe if the lock is not,
> actually, taken (i.e. you could have false positives).

Done.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:188: // end up being sent prematurely if
reason == COMMIT_FAILS.
Locally I have a CL which adds policy-specific sub-queues to
FrameSwapMessageQueue which should solve this particular issue, but it's pretty
big so I'd prefer to do that as a follow-up.

Do you have any specific requirements/preferences for the behavior here? Should
messages enqueued with the NEXT_SWAP policy be sent when DidNotSwap(SWAP_FAILS)
is called or should they remain in the queue till we get a successful swap?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:1295: !frame_swap_message_queue_->Empty()) {
so this should handle FIFO ordering correctly now. We only short-circuit if
there are no messages 'in transit' in the queue.

This runs a small chance of unnecessarily adding a message to the queue if the
queue is completely drained between now and line 1304 but I don't think that
matters (should happen infrequently).

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:1310: source_frame_number));
This way uses less memory since we'll use at most one SwapPromise per
source_frame_number rather than one SwapPromise per message.

[piman, 2014-06-19 23:02:30]

Is there a way to test the RenderWidget logic?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
File content/renderer/gpu/frame_swap_message_queue.h (right):

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.h:42: // ownership of |msg|.
nit: pass scoped_ptr<IPC::Message> to be explicit about it?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.h:48: void QueueMessage(int
source_frame_number, IPC::Message* msg, bool* is_first);
nit: no overloading... QueueMessageForFrame ?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.h:64: void
DrainMessages(std::vector<IPC::Message*>* messages);
nit: no overloading.

For the second version, it would be useful to specify that the caller takes
ownership of the messages. Maybe use a ScopedVector?


Maybe you can just remove the first version, it's called at only one place, and
you could move the TransferMessages logic there (or leave it as a public helper
method).

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:188: // end up being sent prematurely if
reason == COMMIT_FAILS.
On 2014/06/18 14:58:02, mkosiba wrote:
> Locally I have a CL which adds policy-specific sub-queues to
> FrameSwapMessageQueue which should solve this particular issue, but it's
pretty
> big so I'd prefer to do that as a follow-up.
> 
> Do you have any specific requirements/preferences for the behavior here?
Should
> messages enqueued with the NEXT_SWAP policy be sent when
DidNotSwap(SWAP_FAILS)
> is called or should they remain in the queue till we get a successful swap?

SWAP_FAILS happens when we went all the way to drawing, but there was no damage,
so we skipped. I think this is a valid reason to send immediately once we know
that (we would have sent a frame normally, not sending one is an optimization).

COMMIT_FAILS gathers several cases, that we may want to discriminate for this
purpose:
1- the renderer is hidden
2- we lost the output surface / context
3- we went through the commit but found that nothing had changed

case 3 is like SWAP_FAILS - it's an optimization but for practical purposes, we
would have committed and sent a frame normally, so it's ok to send immediately.
case 2 is definitely valid to delay.
I think case 1 too, but I'd have to double-check the specifics for e.g. resize
or plugin moves.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:197: message_sender_->Send(&*i);
Send is supposed to take ownership of the IPC::Message, it's not ok to pass it
the address of the message in the vector.

I think you want to use the other version of DrainMessages here.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:206: bool completed_;
Is this used only for DCHECKs? Should it be in #ifndef NDEBUG ?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:1295: !frame_swap_message_queue_->Empty()) {
Did you mean && frame_swap_message_queue_->Empty() (no !) ?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:1295: !frame_swap_message_queue_->Empty()) {
On 2014/06/18 14:58:03, mkosiba wrote:
> so this should handle FIFO ordering correctly now. We only short-circuit if
> there are no messages 'in transit' in the queue.
> 
> This runs a small chance of unnecessarily adding a message to the queue if the
> queue is completely drained between now and line 1304 but I don't think that
> matters (should happen infrequently).

For that case, you may want to force a commit if
!compositor_->commitRequested(). That way you know the SwapPromise will make
progress (worst case with a COMMIT_FAILS) and you'll eventually send the
message.

[mkosiba (inactive), 2014-06-24 18:29:54]

added some unittests (will add more test cases in the next iteration). Was this
what you had in mind or would you like me to try and do a full integration test?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
File content/renderer/gpu/frame_swap_message_queue.h (right):

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.h:42: // ownership of |msg|.
On 2014/06/19 23:02:29, piman wrote:
> nit: pass scoped_ptr<IPC::Message> to be explicit about it?

Done.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.h:48: void QueueMessage(int
source_frame_number, IPC::Message* msg, bool* is_first);
On 2014/06/19 23:02:29, piman wrote:
> nit: no overloading... QueueMessageForFrame ?

Done.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.h:64: void
DrainMessages(std::vector<IPC::Message*>* messages);
On 2014/06/19 23:02:29, piman wrote:
> nit: no overloading.
> 
> For the second version, it would be useful to specify that the caller takes
> ownership of the messages. Maybe use a ScopedVector?
> 
> 
> Maybe you can just remove the first version, it's called at only one place,
and
> you could move the TransferMessages logic there (or leave it as a public
helper
> method).

Done.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:188: // end up being sent prematurely if
reason == COMMIT_FAILS.
On 2014/06/19 23:02:29, piman wrote:
> On 2014/06/18 14:58:02, mkosiba wrote:
> COMMIT_FAILS gathers several cases, that we may want to discriminate for this
> purpose:
> 1- the renderer is hidden
> 2- we lost the output surface / context
> 3- we went through the commit but found that nothing had changed
> 
> case 3 is like SWAP_FAILS - it's an optimization but for practical purposes,
we
> would have committed and sent a frame normally, so it's ok to send
immediately.
> case 2 is definitely valid to delay.
> I think case 1 too, but I'd have to double-check the specifics for e.g. resize
> or plugin moves.

If I'm reading that right it means there are cases where we might not get a 
DidSwap for a long time. I think might be better to send on failures then,
otherwise we're creating an API that works most of the time but can also lead to
deadlocks on rare occasions (at least theoretically, say the Host won't 'unhide'
the view till some IPC arrives but the IPC is waiting for a successful DidSwap).

Does it actually matter if we end up delivering messages 'too early' for cases 1
and 2 ?

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:197: message_sender_->Send(&*i);
On 2014/06/19 23:02:29, piman wrote:
> Send is supposed to take ownership of the IPC::Message, it's not ok to pass it
> the address of the message in the vector.
> 
> I think you want to use the other version of DrainMessages here.

argh, right right

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:206: bool completed_;
On 2014/06/19 23:02:29, piman wrote:
> Is this used only for DCHECKs? Should it be in #ifndef NDEBUG ?

Done.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:1295: !frame_swap_message_queue_->Empty()) {
On 2014/06/19 23:02:29, piman wrote:
> For that case, you may want to force a commit if
> !compositor_->commitRequested(). That way you know the SwapPromise will make
> progress (worst case with a COMMIT_FAILS) and you'll eventually send the
> message.

Done.

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:1295: !frame_swap_message_queue_->Empty()) {
On 2014/06/19 23:02:29, piman wrote:
> Did you mean && frame_swap_message_queue_->Empty() (no !) ?

yes, right, I had this method called HasMessages before and forgot to remove the
negation

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
File content/renderer/queue_message_swap_promise.h (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
content/renderer/queue_message_swap_promise.h:6: #define
CONTENT_RENDERER_QUEUE_MESSAGE_SWAP_PROMISE_H_
should this go under renderer/gpu ?

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
File content/renderer/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
content/renderer/queue_message_swap_promise_unittest.cc:119: }
I'll add more tests in a subsequent iteration.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
content/renderer/render_widget.cc:1226: // TODO: do we want to SetNeedsRedraw
here?
do we?

[piman, 2014-06-24 19:35:54]

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/210001/content/renderer/render...
content/renderer/render_widget.cc:188: // end up being sent prematurely if
reason == COMMIT_FAILS.
On 2014/06/24 18:29:54, mkosiba wrote:
> On 2014/06/19 23:02:29, piman wrote:
> > On 2014/06/18 14:58:02, mkosiba wrote:
> > COMMIT_FAILS gathers several cases, that we may want to discriminate for
this
> > purpose:
> > 1- the renderer is hidden
> > 2- we lost the output surface / context
> > 3- we went through the commit but found that nothing had changed
> > 
> > case 3 is like SWAP_FAILS - it's an optimization but for practical purposes,
> we
> > would have committed and sent a frame normally, so it's ok to send
> immediately.
> > case 2 is definitely valid to delay.
> > I think case 1 too, but I'd have to double-check the specifics for e.g.
resize
> > or plugin moves.
> 
> If I'm reading that right it means there are cases where we might not get a 
> DidSwap for a long time. I think might be better to send on failures then,
> otherwise we're creating an API that works most of the time but can also lead
to
> deadlocks on rare occasions (at least theoretically, say the Host won't
'unhide'
> the view till some IPC arrives but the IPC is waiting for a successful
DidSwap).

I don't think that would happen. The visibility is managed by the UI side, and
would not wait on the renderer.

> 
> Does it actually matter if we end up delivering messages 'too early' for cases
1
> and 2 ?

Yes, I think so. I would like to use this mechanism to send the
ViewHostMsg_UpdateRect together with the frame. Today it's only sent after we
receive the swap ack from the browser, which means after we've actually drawn a
frame and sent it to the browser (i.e. only when we're visible). Some code on
the browser side relies on that ordering. If we were to send the
ViewHostMsg_UpdateRect too early, this would violate the assumptions.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:108: virtual void
setNeedsCommit();
nit: that's not part of WebLayerTreeView, so you can keep as a non-virtual
method (with Chrome style).

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
File content/renderer/queue_message_swap_promise.h (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
content/renderer/queue_message_swap_promise.h:6: #define
CONTENT_RENDERER_QUEUE_MESSAGE_SWAP_PROMISE_H_
On 2014/06/24 18:29:54, mkosiba wrote:
> should this go under renderer/gpu ?

Yeah, good idea.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
File content/renderer/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
content/renderer/queue_message_swap_promise_unittest.cc:33: }
nit: blank lines between methods.
(also 'git cl format' can help).

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
content/renderer/render_widget.cc:1226: // TODO: do we want to SetNeedsRedraw
here?
On 2014/06/24 18:29:54, mkosiba wrote:
> do we?

It's hard to tell with no caller yet.
Maybe it's best for now to leave it in the hands of the caller.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
content/renderer/render_widget.cc:1267: !compositor_->commitRequested(),
no ! I think?

[mkosiba (inactive), 2014-06-25 18:40:38]

PTAL

I ended up creating separate queues for the two types of messages to get the
next_swap messages to not be sent when the ThreadProxy EarlyOuts.

I also added a 'unique' policy for the UpdateRect stuff - it should make sense
to use that for anything that represents state updates.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:108: virtual void
setNeedsCommit();
On 2014/06/24 19:35:53, piman wrote:
> nit: that's not part of WebLayerTreeView, so you can keep as a non-virtual
> method (with Chrome style).

Done.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
File content/renderer/queue_message_swap_promise.h (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
content/renderer/queue_message_swap_promise.h:6: #define
CONTENT_RENDERER_QUEUE_MESSAGE_SWAP_PROMISE_H_
On 2014/06/24 19:35:53, piman wrote:
> On 2014/06/24 18:29:54, mkosiba wrote:
> > should this go under renderer/gpu ?
> 
> Yeah, good idea.

Done.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
File content/renderer/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/queue_...
content/renderer/queue_message_swap_promise_unittest.cc:33: }
On 2014/06/24 19:35:53, piman wrote:
> nit: blank lines between methods.
> (also 'git cl format' can help).

Done. clang-format seems to not care about this.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
content/renderer/render_widget.cc:1226: // TODO: do we want to SetNeedsRedraw
here?
On 2014/06/24 19:35:53, piman wrote:
> On 2014/06/24 18:29:54, mkosiba wrote:
> > do we?
> 
> It's hard to tell with no caller yet.
> Maybe it's best for now to leave it in the hands of the caller.

ack.

https://codereview.chromium.org/240163005/diff/250001/content/renderer/render...
content/renderer/render_widget.cc:1267: !compositor_->commitRequested(),
On 2014/06/24 19:35:53, piman wrote:
> no ! I think?

right, copy-paste is the most evil thing ever.

[piman, 2014-06-25 21:20:32]

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4539: // Initiate an abortable commit.
What causes this commit to be aborted?

https://codereview.chromium.org/240163005/diff/270001/content/renderer/gpu/fr...
File content/renderer/gpu/frame_swap_message_queue.cc (right):

https://codereview.chromium.org/240163005/diff/270001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.cc:96: if (insert_unique)
I don't think we need the complexity for the UNIQUE policy, at least at this
point.
For UpdateRect, each message can contain different type of updates (e.g. input
ack or plugin moves), and it wouldn't make sense to simply drop one. One could
think of a way to combine several messages into one, but this smells like
premature optimization - it can always be done on the host side, it's unclear
the cost of sending an extra message is higher than the extra logic needed here.
Let's just leave it off until we realize we need it.

[mkosiba (inactive), 2014-06-25 22:08:33]

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4539: // Initiate an abortable commit.
On 2014/06/25 21:20:32, piman wrote:
> What causes this commit to be aborted?

I'd be thrilled to know too, to be honest. I spent some time trying to figure
out a way to force a commit to EarlyOut due to visibility=false, but couldn't
get that to happen so I nicked bits from
LayerTreeHostTestAbortedCommitDoesntStall.

The magic sauce seems to be setting throttle_frame_production to false and
calling host_impl->SetNeedsCommit from here.

[mkosiba (inactive), 2014-06-26 12:36:47]

ok, new iteration up

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4539: // Initiate an abortable commit.
On 2014/06/25 22:08:33, mkosiba wrote:
> On 2014/06/25 21:20:32, piman wrote:
> > What causes this commit to be aborted?

Ok, so I think this has something to do with the scheduler not going through
with the commit on the impl thread. From ThreadProxy perspective the
BeginMainFrame method runs to completion but the trees aren't actually swapped,
so we get a COMMIT_FAILS callback.

I tried finding a test that actually hits the EarlyOut_NotVisible path in
ThreadProxy::BeginMainFrame, the only one that does is
LayerTreeHostAnimationTestAnimationTickTimeIsMonotonic but the code there looks
equally unintuitive to me as this.

https://codereview.chromium.org/240163005/diff/270001/content/renderer/gpu/fr...
File content/renderer/gpu/frame_swap_message_queue.cc (right):

https://codereview.chromium.org/240163005/diff/270001/content/renderer/gpu/fr...
content/renderer/gpu/frame_swap_message_queue.cc:96: if (insert_unique)
On 2014/06/25 21:20:32, piman (slow to review) wrote:
> I don't think we need the complexity for the UNIQUE policy, at least at this
> point.
> For UpdateRect, each message can contain different type of updates (e.g. input
> ack or plugin moves), and it wouldn't make sense to simply drop one. One could
> think of a way to combine several messages into one, but this smells like
> premature optimization - it can always be done on the host side, it's unclear
> the cost of sending an extra message is higher than the extra logic needed
here.
> Let's just leave it off until we realize we need it.

Done.

[piman, 2014-06-26 20:40:12]

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4539: // Initiate an abortable commit.
On 2014/06/26 12:36:47, mkosiba wrote:
> On 2014/06/25 22:08:33, mkosiba wrote:
> > On 2014/06/25 21:20:32, piman wrote:
> > > What causes this commit to be aborted?
> 
> Ok, so I think this has something to do with the scheduler not going through
> with the commit on the impl thread. From ThreadProxy perspective the
> BeginMainFrame method runs to completion but the trees aren't actually
swapped,
> so we get a COMMIT_FAILS callback.

Mmh, I think the case where that happens is the EarlyOut_NoLayerTree in
ThreadProxy::StartCommitOnImplThread
Not sure why we get into it though, is it because we destroy the compositor
before the commit takes place? Not sure why that would be.
I'm worried this may be due to a race and would cause a flaky test.

> I tried finding a test that actually hits the EarlyOut_NotVisible path in
> ThreadProxy::BeginMainFrame, the only one that does is
> LayerTreeHostAnimationTestAnimationTickTimeIsMonotonic but the code there
looks
> equally unintuitive to me as this.

EarlyOut_NotVisible is essentially to handle a race:
1- main thread asked for a commit while visible
2- impl thread posted the task to the main thread to start the commit
3- main thread calls SetVisible(false)
4- main thread executes the task.

It's hard to trigger though, because (3) will also post a task to the impl
thread to basically tell it not to do commits.

Unless you add a test hook, I think the way you can test is by:
- LTH::SetDeferCommit(true)
- LTH::SetNeedsCommit()

in the DidDeferCommit hook:
- LTH::SetVisible(false)
- LTH::SetDeferCommit(false)

The last call will re-post the task to do the commit at which point we'll get
into the EarlyOut_NotVisible case.

https://codereview.chromium.org/240163005/diff/290001/content/renderer/messag...
File content/renderer/message_delivery_policy.h (right):

https://codereview.chromium.org/240163005/diff/290001/content/renderer/messag...
content/renderer/message_delivery_policy.h:13: // the message will be sent
immediately usint regular IPC.
nit: usint->using

https://codereview.chromium.org/240163005/diff/330001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/240163005/diff/330001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:752: DCHECK(false);
?

[mkosiba (inactive), 2014-06-27 14:18:59]

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4539: // Initiate an abortable commit.
On 2014/06/26 20:40:12, piman (slow to review) wrote:
> On 2014/06/26 12:36:47, mkosiba wrote:
> > On 2014/06/25 22:08:33, mkosiba wrote:
> > > On 2014/06/25 21:20:32, piman wrote:
> > > > What causes this commit to be aborted?
> > 
> > Ok, so I think this has something to do with the scheduler not going through
> > with the commit on the impl thread. From ThreadProxy perspective the
> > BeginMainFrame method runs to completion but the trees aren't actually
> swapped,
> > so we get a COMMIT_FAILS callback.
> 
> Mmh, I think the case where that happens is the EarlyOut_NoLayerTree in
> ThreadProxy::StartCommitOnImplThread
> Not sure why we get into it though, is it because we destroy the compositor
> before the commit takes place? Not sure why that would be.
> I'm worried this may be due to a race and would cause a flaky test.
> 
> > I tried finding a test that actually hits the EarlyOut_NotVisible path in
> > ThreadProxy::BeginMainFrame, the only one that does is
> > LayerTreeHostAnimationTestAnimationTickTimeIsMonotonic but the code there
> looks
> > equally unintuitive to me as this.
> 
> EarlyOut_NotVisible is essentially to handle a race:
> 1- main thread asked for a commit while visible
> 2- impl thread posted the task to the main thread to start the commit
> 3- main thread calls SetVisible(false)
> 4- main thread executes the task.
> 
> It's hard to trigger though, because (3) will also post a task to the impl
> thread to basically tell it not to do commits.
> 
> Unless you add a test hook, I think the way you can test is by:
> - LTH::SetDeferCommit(true)
> - LTH::SetNeedsCommit()
> 
> in the DidDeferCommit hook:
> - LTH::SetVisible(false)
> - LTH::SetDeferCommit(false)
> 
> The last call will re-post the task to do the commit at which point we'll get
> into the EarlyOut_NotVisible case.

That works, thanks!

https://codereview.chromium.org/240163005/diff/290001/content/renderer/messag...
File content/renderer/message_delivery_policy.h (right):

https://codereview.chromium.org/240163005/diff/290001/content/renderer/messag...
content/renderer/message_delivery_policy.h:13: // the message will be sent
immediately usint regular IPC.
On 2014/06/26 20:40:12, piman (slow to review) wrote:
> nit: usint->using

Done.

https://codereview.chromium.org/240163005/diff/330001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/240163005/diff/330001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:752: DCHECK(false);
On 2014/06/26 20:40:12, piman (slow to review) wrote:
> ?

high-tech method of figuring out which tests hit this code. removed

[piman, 2014-06-27 19:17:30]

lgtm

https://codereview.chromium.org/240163005/diff/370001/content/renderer/gpu/qu...
File content/renderer/gpu/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/370001/content/renderer/gpu/qu...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:136:
{MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP, false, 1},
nit: indent looked correct before (also everywhere below).

[mkosiba (inactive), 2014-06-30 16:23:10]

dcheng@chromium.org: Please review IPC changes

jbauman@chromium.org: Please review changes in ui/events/latency_info*

[mkosiba (inactive), 2014-06-30 16:28:09]

https://codereview.chromium.org/240163005/diff/370001/content/renderer/gpu/qu...
File content/renderer/gpu/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/240163005/diff/370001/content/renderer/gpu/qu...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:136:
{MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP, false, 1},
On 2014/06/27 19:17:30, piman (slow to review) wrote:
> nit: indent looked correct before (also everywhere below).

thanks! I didn't notice format-patch changed this.

[jbauman, 2014-06-30 21:07:32]

lgtm

[mkosiba (inactive), 2014-07-01 22:14:41]

+inferno, could you review the IPC changes since it looks like dcheng is OOO?

[Tom Sepez, 2014-07-02 17:56:04]

I can give you a "Message headers LGTM" in place of inferno, but I am adding
@jam to ensure there aren't any funny interactions between your message queue
and the rest of the system that I haven't considered.

https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1498:
rph->OnMessageReceived(*i);
You should check if the message was handled, calling BadMessageReceived().

[Tom Sepez, 2014-07-02 17:58:37]

I mean:
> You should check if the message was handled, calling BadMessageReceived()
otherwise.

[jam, 2014-07-02 20:10:54]

https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1498:
rph->OnMessageReceived(*i);
On 2014/07/02 17:56:04, Tom Sepez wrote:
> You should check if the message was handled, calling BadMessageReceived().

In general, not handling IPCs is not considered an error. In which case this
would be:

if (i->dispatch_error())
  rph->OnBadMessageReceived(*i);


However, RenderWidget::QueueMessage is somewhat error prone because it only
delivers the queued IPCs to the RPH. That means any IPC currently dispatched by
a BrowserMessageFilter wouldn't work if someone used QueueMessage to send them.

I'm wondering why we need this generic infrastructure. Do we expect more IPCs to
be queued like this? I only see FrameHostMsg_DidFirstVisuallyNonEmptyPaint using
this now, is that correct? If it's just that one IPC, can we do a one-off
solution?

[mkosiba (inactive), 2014-07-03 15:47:33]

https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1498:
rph->OnMessageReceived(*i);
On 2014/07/02 20:10:54, jam wrote:
> On 2014/07/02 17:56:04, Tom Sepez wrote:
> > You should check if the message was handled, calling BadMessageReceived().
> 
> In general, not handling IPCs is not considered an error. In which case this
> would be:
> 
> if (i->dispatch_error())
>   rph->OnBadMessageReceived(*i);

Done.

> 
> However, RenderWidget::QueueMessage is somewhat error prone because it only
> delivers the queued IPCs to the RPH. That means any IPC currently dispatched
by
> a BrowserMessageFilter wouldn't work if someone used QueueMessage to send
them.

The value of RenderWidget::QueueMessage is that we deliver the messages together
with the frame swap notification, so posting back to the IO thread to run the
filters from here would make it less useful.

Getting message filters to work transparently would essentially require cracking
this message open on the IO thread, filtering the inner messages vector and
forwarding the 'filtered' message on, right?

For now the intent is not to provide a drop-in replacement for IPC.

> I'm wondering why we need this generic infrastructure. Do we expect more IPCs
to
> be queued like this? I only see FrameHostMsg_DidFirstVisuallyNonEmptyPaint
using
> this now, is that correct? If it's just that one IPC, can we do a one-off
> solution?

So I only used DidFirstVisuallyNonEmptyPaint as a guinea pig. I'm working a CL
that uses this mechanism to implement some new webview functionality and piman@
was interested in using this for LatencyInfo and ViewHostMsg_UpdateRect (see
comment #4).

[jam, 2014-07-14 17:13:36]

On 2014/07/03 15:47:33, mkosiba wrote:
>
https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/240163005/diff/450001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_impl.cc:1498:
> rph->OnMessageReceived(*i);
> On 2014/07/02 20:10:54, jam wrote:
> > On 2014/07/02 17:56:04, Tom Sepez wrote:
> > > You should check if the message was handled, calling BadMessageReceived().
> > 
> > In general, not handling IPCs is not considered an error. In which case this
> > would be:
> > 
> > if (i->dispatch_error())
> >   rph->OnBadMessageReceived(*i);
> 
> Done.
> 
> > 
> > However, RenderWidget::QueueMessage is somewhat error prone because it only
> > delivers the queued IPCs to the RPH. That means any IPC currently dispatched
> by
> > a BrowserMessageFilter wouldn't work if someone used QueueMessage to send
> them.
> 
> The value of RenderWidget::QueueMessage is that we deliver the messages
together
> with the frame swap notification, so posting back to the IO thread to run the
> filters from here would make it less useful.
> 
> Getting message filters to work transparently would essentially require
cracking
> this message open on the IO thread, filtering the inner messages vector and
> forwarding the 'filtered' message on, right?
> 
> For now the intent is not to provide a drop-in replacement for IPC.
> 
> > I'm wondering why we need this generic infrastructure. Do we expect more
IPCs
> to
> > be queued like this? I only see FrameHostMsg_DidFirstVisuallyNonEmptyPaint
> using
> > this now, is that correct? If it's just that one IPC, can we do a one-off
> > solution?
> 
> So I only used DidFirstVisuallyNonEmptyPaint as a guinea pig. I'm working a CL
> that uses this mechanism to implement some new webview functionality and
piman@
> was interested in using this for LatencyInfo and ViewHostMsg_UpdateRect (see
> comment #4).

lgtm

[mkosiba (inactive), 2014-07-21 17:22:17]

+yaron for
content/public/android/javatests/src/org/chromium/content/browser/WebContentsObserverAndroidTest.java

[mkosiba (inactive), 2014-07-22 09:25:28]

The CQ bit was checked by mkosiba@chromium.org

[mkosiba (inactive), 2014-07-22 09:26:22]

On 2014/07/21 17:22:17, mkosiba wrote:
> +yaron for
>
content/public/android/javatests/src/org/chromium/content/browser/WebContentsObserverAndroidTest.java

Yaron - I'm going to CQ this and address any feedback in a subsequent patch.

[commit-bot: I haz the power, 2014-07-22 09:27:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/240163005/530001

[commit-bot: I haz the power, 2014-07-22 11:23:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-22 12:03:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 12:03:53]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[mkosiba (inactive), 2014-07-22 14:03:54]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-07-22 14:04:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/240163005/530001

[Yaron, 2014-07-22 16:13:47]

lgtm

(sorry was ooo)

https://codereview.chromium.org/240163005/diff/530001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/WebContentsObserverAndroidTest.java
(right):

https://codereview.chromium.org/240163005/diff/530001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/WebContentsObserverAndroidTest.java:52:
@Feature({"AndroidWebView", "Android-PostMessage"})
This doesn't really need to list AW, does it? It's generic to webcontents.

https://codereview.chromium.org/240163005/diff/530001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/WebContentsObserverAndroidTest.java:67:
getActivity().getActiveShell().getContentViewCore().loadUrl(
nit: you could drop getActivity().getActiveShell()

[commit-bot: I haz the power, 2014-07-22 17:05:18]

Change committed as 284712