Split ThreadProxy methods to ProxyMain and ProxyImpl.

cc: Move inter-thread communication in ThreadProxy through ChannelMain
and ChannelImpl.

ThreadProxy currently uses PostTasks to communicate between the parts
which run on the main and impl thread.
Add methods to control this communication to the ChannelMain and
ChannelImpl interfaces to used by ProxyMain and ProxyImpl for splitting
ThreadProxy. Add the implementation for the case of the Threaded Compositor
to ThreadedChannel.

This is a follow up patch for: https://codereview.chromium.org/1357373002

BUG=527200
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e3c9fa95884885ec885c934597b09a1bc3d8979e
Cr-Commit-Position: refs/heads/master@{#356491}

[Khushal, 2015-09-30 13:46:39]

khushalsagar@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-09-30 14:44:57]

https://chromiumcodereview.appspot.com/1377063003/diff/1/cc/trees/threaded_ch...
File cc/trees/threaded_channel.h (right):

https://chromiumcodereview.appspot.com/1377063003/diff/1/cc/trees/threaded_ch...
cc/trees/threaded_channel.h:106: void PostFrameTimingEventsToMain(
ToMain -> OnMain?

https://chromiumcodereview.appspot.com/1377063003/diff/1/cc/trees/threaded_ch...
cc/trees/threaded_channel.h:145: base::WeakPtrFactory<ThreadedChannel>
impl_weak_factory_;
Will we still store the weak factory here in the future?

[Khushal, 2015-09-30 15:26:34]

khushalsagar@chromium.org changed reviewers:
+ enne@chromium.org

[Khushal, 2015-09-30 15:26:34]

https://chromiumcodereview.appspot.com/1377063003/diff/1/cc/trees/threaded_ch...
File cc/trees/threaded_channel.h (right):

https://chromiumcodereview.appspot.com/1377063003/diff/1/cc/trees/threaded_ch...
cc/trees/threaded_channel.h:106: void PostFrameTimingEventsToMain(
On 2015/09/30 14:44:56, David Trainor wrote:
> ToMain -> OnMain?

Done.

https://chromiumcodereview.appspot.com/1377063003/diff/1/cc/trees/threaded_ch...
cc/trees/threaded_channel.h:145: base::WeakPtrFactory<ThreadedChannel>
impl_weak_factory_;
On 2015/09/30 14:44:57, David Trainor wrote:
> Will we still store the weak factory here in the future?

Yes. We can probably have an accessor for this to DCHECK that it is used to
initialize and invalidate the impl_thread_weak_ptr_ on the impl thread.

[khushalsagar, 2015-10-01 21:19:37]

khushalsagar@google.com changed reviewers:
+ khushalsagar@google.com

[khushalsagar, 2015-10-01 21:19:37]

https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
File cc/trees/thread_proxy.h (right):

https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
cc/trees/thread_proxy.h:260: void DidInitializeOutputSurface(
The output surface creation and release will be handled on the client for Blimp.
So we don't need to send these messages to the server.

But there are flags in the LayerTreeHost which are set with this call,
output_surface_lost_. It DCHECKs new_output_surface_ since the LayerTreeHost
holds a reference to the output surface in the current implementation, which it
won't for Blimp. The call to BeginMainFrame is aborted based on the value of
output_surface_lost_.

Should we add another method to LayerTreeHost to set a flag that turns off the
output surface lost check for aborting begin main frame calls?

https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
cc/trees/thread_proxy.h:278: void SetVisibleOnImpl(bool visible) override;
This is caused by SetVisible sent to LTH which sets a flag in LTH that can cause
the BeginMainFrame call to abort.
Since we'll be sending the SetVisible calls to the RWC running on the server,
this should be okay.

[David Trainor- moved to gerrit, 2015-10-08 18:01:57]

lgtm

[danakj, 2015-10-08 18:03:32]

https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
File cc/trees/thread_proxy.h (right):

https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
cc/trees/thread_proxy.h:260: void DidInitializeOutputSurface(
On 2015/10/01 21:19:37, khushalsagar wrote:
> The output surface creation and release will be handled on the client for
Blimp.
> So we don't need to send these messages to the server.
> 
> But there are flags in the LayerTreeHost which are set with this call,
> output_surface_lost_. It DCHECKs new_output_surface_ since the LayerTreeHost
> holds a reference to the output surface in the current implementation, which
it
> won't for Blimp. The call to BeginMainFrame is aborted based on the value of
> output_surface_lost_.
> 
> Should we add another method to LayerTreeHost to set a flag that turns off the
> output surface lost check for aborting begin main frame calls?

I think we can get rid of the output surface pointer in LTH, by making the LTHI
pointer to the OS be null after losing the output surface. +sievers.

[danakj, 2015-10-08 18:16:49]

On 2015/10/08 18:03:32, danakj_OOO_til_10-12 wrote:
>
https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
> File cc/trees/thread_proxy.h (right):
> 
>
https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
> cc/trees/thread_proxy.h:260: void DidInitializeOutputSurface(
> On 2015/10/01 21:19:37, khushalsagar wrote:
> > The output surface creation and release will be handled on the client for
> Blimp.
> > So we don't need to send these messages to the server.
> > 
> > But there are flags in the LayerTreeHost which are set with this call,
> > output_surface_lost_. It DCHECKs new_output_surface_ since the LayerTreeHost
> > holds a reference to the output surface in the current implementation, which
> it
> > won't for Blimp. The call to BeginMainFrame is aborted based on the value of
> > output_surface_lost_.
> > 
> > Should we add another method to LayerTreeHost to set a flag that turns off
the
> > output surface lost check for aborting begin main frame calls?
> 
> I think we can get rid of the output surface pointer in LTH, by making the
LTHI
> pointer to the OS be null after losing the output surface. +sievers.

The output_surface_lost_ abort is because the main frame depends on the renderer
capabilities in a few places, and they aren't valid without knowing what the
output surface is like.

1. LayerTreeHost::UsingSharedMemoryResources

This is used by TextureLayer to tell if it should submit a GL resource or a
software one. What is the story with pepper plugins and webgl in blimp?

2. The HeadsUpLayer uses the max_texture_size to keep its size below that, as it
is displayed with a single texture.

This is pretty much a debugging tool for us, and it's not too critical to be
exact here. However if the chrome window size was ever > the max texture size it
would break (disappear) not knowing a correct value for this. Does this ever
happen in practice? A fullscreen texture seems like it's always possible. I'm
probably fine with even not using the max texture size for this ever.. enne?

3. PaintedScrollbarLayer uses the max_texture_size as well, to not become too
large.

When you pinch zoom in a lot, scrollbar layer can become very large, but it is a
single texture. So it uses this to clamp its scale and not disappear.

PaintedScrollbarLayer is used for mac/windows/chromeos/linux, but not android,
right?  In order to break this dependency the scrollbar layer could simply tile
its thumb into like <= 512px pieces or something which is always going to be a
valid texture size.

[David Trainor- moved to gerrit, 2015-10-08 18:29:17]

On 2015/10/08 18:16:49, danakj_OOO_til_10-12 wrote:
> On 2015/10/08 18:03:32, danakj_OOO_til_10-12 wrote:
> >
>
https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
> > File cc/trees/thread_proxy.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1377063003/diff/20001/cc/trees/thread_...
> > cc/trees/thread_proxy.h:260: void DidInitializeOutputSurface(
> > On 2015/10/01 21:19:37, khushalsagar wrote:
> > > The output surface creation and release will be handled on the client for
> > Blimp.
> > > So we don't need to send these messages to the server.
> > > 
> > > But there are flags in the LayerTreeHost which are set with this call,
> > > output_surface_lost_. It DCHECKs new_output_surface_ since the
LayerTreeHost
> > > holds a reference to the output surface in the current implementation,
which
> > it
> > > won't for Blimp. The call to BeginMainFrame is aborted based on the value
of
> > > output_surface_lost_.
> > > 
> > > Should we add another method to LayerTreeHost to set a flag that turns off
> the
> > > output surface lost check for aborting begin main frame calls?
> > 
> > I think we can get rid of the output surface pointer in LTH, by making the
> LTHI
> > pointer to the OS be null after losing the output surface. +sievers.
> 
> The output_surface_lost_ abort is because the main frame depends on the
renderer
> capabilities in a few places, and they aren't valid without knowing what the
> output surface is like.
I think we could still pass the renderer capabilities back to the main component
from the client.  Maybe it'd be worth decoupling that from the output surface? 
That way a lot of the below issues will still work out.
> 
> 1. LayerTreeHost::UsingSharedMemoryResources
> 
> This is used by TextureLayer to tell if it should submit a GL resource or a
> software one. What is the story with pepper plugins and webgl in blimp?

Sadly there isn't one yet :(.  We're still trying to figure out if we want to
support these.

> 
> 2. The HeadsUpLayer uses the max_texture_size to keep its size below that, as
it
> is displayed with a single texture.
> 
> This is pretty much a debugging tool for us, and it's not too critical to be
> exact here. However if the chrome window size was ever > the max texture size
it
> would break (disappear) not knowing a correct value for this. Does this ever
> happen in practice? A fullscreen texture seems like it's always possible. I'm
> probably fine with even not using the max texture size for this ever.. enne?
> 
> 3. PaintedScrollbarLayer uses the max_texture_size as well, to not become too
> large.
> 
> When you pinch zoom in a lot, scrollbar layer can become very large, but it is
a
> single texture. So it uses this to clamp its scale and not disappear.
> 
> PaintedScrollbarLayer is used for mac/windows/chromeos/linux, but not android,
> right?  In order to break this dependency the scrollbar layer could simply
tile
> its thumb into like <= 512px pieces or something which is always going to be a
> valid texture size.

Yeah Android still uses SolidColorScrollbarLayer IIRC.  That makes life a little
easier because I don't think we have a story for UIResourceIds yet, as they were
primarily used for UI.  But we need to solve that somewhat soon.

[danakj, 2015-10-08 19:14:04]

>
> I think we could still pass the renderer capabilities back to the main
> component
> from the client.  Maybe it'd be worth decoupling that from the output
> surface?
> That way a lot of the below issues will still work out.


Well, they are a product of the output surface which is why they are tied
this way. For instance, max texture size depends on the GL context, and it
is different if the output surface is for software compositing instead of
gpu. So, renderer capabilities change iff a new output surface is used and
while the surface is lost we don't know and shouldn't be making frames.

I was trying to understand why we have the early out based on
output_surface_lost in thread proxy to abort commit. I'm pretty sure that
shouldn't happen anymore. That was added in
https://codereview.chromium.org/287193003 to fix the bug
https://code.google.com/p/chromium/issues/detail?id=376649. The bug was
that a deferred commit would be started independently by the main thread,
and so it would end up trying to make a commit while the surface was lost.
We have since moved control of deferred commits into the scheduler, so it
all comes from the impl thread.

If the surface is lost, we won't send begin main frames
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...

Except for in one case where FORCED_REDRAW_STATE_WAITING_FOR_COMMIT... but
we'd just abort it anyways. So I propose that that should change.. and we
should delete the early out in ThreadProxy. (Not in this CL just trying to
understand the world.) +brianderson


>
> 1. LayerTreeHost::UsingSharedMemoryResources
>>
>
> This is used by TextureLayer to tell if it should submit a GL resource or a
>> software one. What is the story with pepper plugins and webgl in blimp?
>>
>
> Sadly there isn't one yet :(.  We're still trying to figure out if we want
> to
> support these.
>
>
> 2. The HeadsUpLayer uses the max_texture_size to keep its size below that,
>> as
>>
> it
>
>> is displayed with a single texture.
>>
>
> This is pretty much a debugging tool for us, and it's not too critical to
>> be
>> exact here. However if the chrome window size was ever > the max texture
>> size
>>
> it
>
>> would break (disappear) not knowing a correct value for this. Does this
>> ever
>> happen in practice? A fullscreen texture seems like it's always possible.
>> I'm
>> probably fine with even not using the max texture size for this ever..
>> enne?
>>
>
> 3. PaintedScrollbarLayer uses the max_texture_size as well, to not become
>> too
>> large.
>>
>
> When you pinch zoom in a lot, scrollbar layer can become very large, but
>> it is
>>
> a
>
>> single texture. So it uses this to clamp its scale and not disappear.
>>
>
> PaintedScrollbarLayer is used for mac/windows/chromeos/linux, but not
>> android,
>> right?  In order to break this dependency the scrollbar layer could simply
>>
> tile
>
>> its thumb into like <= 512px pieces or something which is always going to
>> be a
>> valid texture size.
>>
>
> Yeah Android still uses SolidColorScrollbarLayer IIRC.  That makes life a
> little
> easier because I don't think we have a story for UIResourceIds yet, as
> they were
> primarily used for UI.  But we need to solve that somewhat soon.
>

Is android going to use a non-solid color scrollbar, or what are you
referring to that is soon?

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

[danakj, 2015-10-08 19:21:08]

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
cc/trees/thread_proxy.cc:159: main().channel_main->SetVisibleOnImpl(visible);
i'd like it if it was obvious which calls are going to block or not.

can you maybe prefix or suffix all such waiting calls with Synchronous?

[Khushal, 2015-10-08 20:32:07]

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
cc/trees/thread_proxy.cc:159: main().channel_main->SetVisibleOnImpl(visible);
On 2015/10/08 19:21:08, danakj_OOO_til_10-12 wrote:
> i'd like it if it was obvious which calls are going to block or not.
> 
> can you maybe prefix or suffix all such waiting calls with Synchronous?

We will try to make some calls asynchronous for Blimp. For instance, when
starting the commit on impl, we send the serialized commit state of the Layer
Tree to the client to commit from, after this the main thread will not be
blocked. This is why I was trying to limit this within the channel
implementation.

Should I still consider marking it as synchronous in the channel interface?

[danakj, 2015-10-09 17:54:43]

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
cc/trees/thread_proxy.cc:159: main().channel_main->SetVisibleOnImpl(visible);
On 2015/10/08 20:32:07, Khushal wrote:
> On 2015/10/08 19:21:08, danakj_OOO_til_10-12 wrote:
> > i'd like it if it was obvious which calls are going to block or not.
> > 
> > can you maybe prefix or suffix all such waiting calls with Synchronous?
> 
> We will try to make some calls asynchronous for Blimp. For instance, when
> starting the commit on impl, we send the serialized commit state of the Layer
> Tree to the client to commit from, after this the main thread will not be
> blocked. This is why I was trying to limit this within the channel
> implementation.
> 
> Should I still consider marking it as synchronous in the channel interface?

I think we should mark everything as Sync that is sync right now. For the
commit, perhaps it would split into two separate steps in the proxy.

1) SendCommitStuff
2) SendCommitStuffSync

or whatever. Non-blimp would use the sync version. Then it's obvious at
callsites what the behaviour will be.

[Khushal, 2015-10-09 19:46:56]

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
cc/trees/thread_proxy.cc:159: main().channel_main->SetVisibleOnImpl(visible);
On 2015/10/09 17:54:43, danakj_OOO_til_10-12 wrote:
> On 2015/10/08 20:32:07, Khushal wrote:
> > On 2015/10/08 19:21:08, danakj_OOO_til_10-12 wrote:
> > > i'd like it if it was obvious which calls are going to block or not.
> > > 
> > > can you maybe prefix or suffix all such waiting calls with Synchronous?
> > 
> > We will try to make some calls asynchronous for Blimp. For instance, when
> > starting the commit on impl, we send the serialized commit state of the
Layer
> > Tree to the client to commit from, after this the main thread will not be
> > blocked. This is why I was trying to limit this within the channel
> > implementation.
> > 
> > Should I still consider marking it as synchronous in the channel interface?
> 
> I think we should mark everything as Sync that is sync right now. For the
> commit, perhaps it would split into two separate steps in the proxy.
> 
> 1) SendCommitStuff
> 2) SendCommitStuffSync
> 
> or whatever. Non-blimp would use the sync version. Then it's obvious at
> callsites what the behaviour will be.

That would definitely be better. Just had one doubt, since the details of
whether the Proxy is using an implementation of the channel for chrome or for
blimp is completely abstracted by the channel interface, we'd need something in
Proxy to decide whether it uses a sync or non-sync method.

Would be want to add this as a setting to ProxyMain?

[Khushal, 2015-10-14 01:50:44]

On 2015/10/09 19:46:56, Khushal wrote:
>
https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
> File cc/trees/thread_proxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1377063003/diff/40001/cc/trees/thread_...
> cc/trees/thread_proxy.cc:159: main().channel_main->SetVisibleOnImpl(visible);
> On 2015/10/09 17:54:43, danakj_OOO_til_10-12 wrote:
> > On 2015/10/08 20:32:07, Khushal wrote:
> > > On 2015/10/08 19:21:08, danakj_OOO_til_10-12 wrote:
> > > > i'd like it if it was obvious which calls are going to block or not.
> > > > 
> > > > can you maybe prefix or suffix all such waiting calls with Synchronous?
> > > 
> > > We will try to make some calls asynchronous for Blimp. For instance, when
> > > starting the commit on impl, we send the serialized commit state of the
> Layer
> > > Tree to the client to commit from, after this the main thread will not be
> > > blocked. This is why I was trying to limit this within the channel
> > > implementation.
> > > 
> > > Should I still consider marking it as synchronous in the channel
interface?
> > 
> > I think we should mark everything as Sync that is sync right now. For the
> > commit, perhaps it would split into two separate steps in the proxy.
> > 
> > 1) SendCommitStuff
> > 2) SendCommitStuffSync
> > 
> > or whatever. Non-blimp would use the sync version. Then it's obvious at
> > callsites what the behaviour will be.
> 
> That would definitely be better. Just had one doubt, since the details of
> whether the Proxy is using an implementation of the channel for chrome or for
> blimp is completely abstracted by the channel interface, we'd need something
in
> Proxy to decide whether it uses a sync or non-sync method.
> 
> Would be want to add this as a setting to ProxyMain?

I have added the completion event to the method signatures itself. For Blimp, we
can signal the event before the call to ChannelMain returns. Since the event is
already signaled, wait will not actually block.

[danakj, 2015-10-14 17:56:25]

danakj@chromium.org changed reviewers:
+ vmpstr@chromium.org
- danakj@chromium.org

[danakj, 2015-10-14 17:56:26]

I think vlad should be primary on this. He did a lot on the prototype for blimp.

+vmpstr

[danakj, 2015-10-14 17:57:32]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-10-14 17:57:33]

Please consider formatting your CL description according to guidelines like
these: http://chris.beams.io/posts/git-commit/

Specifically, wrapping at 72 chars is nice:
http://chris.beams.io/posts/git-commit/#wrap-72

[danakj, 2015-10-14 17:57:41]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[Khushal, 2015-10-14 22:06:06]

On 2015/10/14 17:57:33, danakj wrote:
> Please consider formatting your CL description according to guidelines like
> these: http://chris.beams.io/posts/git-commit/
> 
> Specifically, wrapping at 72 chars is nice:
> http://chris.beams.io/posts/git-commit/#wrap-72

Thanks Dana. I'll try to keep note of it from now on. :)

[vmpstr, 2015-10-15 20:25:04]

Sorry for high level questions, but is this meant to split thread proxy into two
(ie explicitly two different classes)? Also, should there be two separate
concrete channel objects or is threaded_channel the final product?

If this is also meant to work for blimp, is there a story for us changing this
interface in the future? Ie, what happens when the client is issuing calls that
the server doesn't have any more, etc.

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:142:
main().channel_main->FinishAllRenderingOnImpl(&completion);
It's a bit awkward for me that channel_main refers to the channel that
communicates with the impl side. Is "main" referring to where the channel is
located, but not where it's communicating to?

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:143: completion.Wait();
The typical thing to do is create a completion / post task / wait.

What's the story for when this isn't a post task? Should the parameter to the
function be something like a bool wait_until_done; and it can create the
completion in the channel? Or do you think it's better to keep this outside?

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:394: Proxy::ImplThreadTaskRunner()->PostTask(
Why did you only do this for a particular set of PostTasks but not for others?

[Khushal, 2015-10-15 23:44:57]

On 2015/10/15 20:25:04, vmpstr wrote:
> Sorry for high level questions, but is this meant to split thread proxy into
two
> (ie explicitly two different classes)? Also, should there be two separate
> concrete channel objects or is threaded_channel the final product?
> 
> If this is also meant to work for blimp, is there a story for us changing this
> interface in the future? Ie, what happens when the client is issuing calls
that
> the server doesn't have any more, etc.
> 
> https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
> cc/trees/thread_proxy.cc:142:
> main().channel_main->FinishAllRenderingOnImpl(&completion);
> It's a bit awkward for me that channel_main refers to the channel that
> communicates with the impl side. Is "main" referring to where the channel is
> located, but not where it's communicating to?
> 
>
https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
> cc/trees/thread_proxy.cc:143: completion.Wait();
> The typical thing to do is create a completion / post task / wait.
> 
> What's the story for when this isn't a post task? Should the parameter to the
> function be something like a bool wait_until_done; and it can create the
> completion in the channel? Or do you think it's better to keep this outside?
> 
>
https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
> cc/trees/thread_proxy.cc:394: Proxy::ImplThreadTaskRunner()->PostTask(
> Why did you only do this for a particular set of PostTasks but not for others?

The aim is to split ThreadProxy into 2 concrete classes, ProxyMain and ProxyImpl
connected by the Channel Interfaces so Blimp uses the same ProxyMain and
ProxyImpl just connected by a different channel.
For Blimp there will be an implementation of ChannelMain, RemoteChannelMain on
the server which ProxyMain uses to send messages to and get messages from the
client. An implementation of ChannelImpl, RemoteChannelImpl on the client does
the same thing for ProxyImpl.
For the in-process case threaded compositor, ThreadedChannel implements both
since it can invoke the methods on ProxyMain and ProxyImpl directly.

We have to deal with the client-server skewing for Blimp since this is a problem
for all parts of the chromium code the client depends on. I think we plan to
have a versioning scheme to make sure when the client connects, it is connected
to the same version on the server to handle such changes. +dtrainor

[Khushal, 2015-10-15 23:45:51]

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:142:
main().channel_main->FinishAllRenderingOnImpl(&completion);
On 2015/10/15 20:25:04, vmpstr wrote:
> It's a bit awkward for me that channel_main refers to the channel that
> communicates with the impl side. Is "main" referring to where the channel is
> located, but not where it's communicating to?

Since the class is split into the main and impl components, each side needs an
interface to communicate with the other side. The current naming scheme is such
that ChannelMain is the interface and its implementation is located on the main
side. For Blimp, a RemoteChannelMain will implement this that lives on the
server.

We can rename it channel_main_to_impl if that makes it clearer?

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:143: completion.Wait();
On 2015/10/15 20:25:04, vmpstr wrote:
> The typical thing to do is create a completion / post task / wait.
> 
> What's the story for when this isn't a post task? Should the parameter to the
> function be something like a bool wait_until_done; and it can create the
> completion in the channel? Or do you think it's better to keep this outside?

Right now completion events make it explicit to methods on both side that the
main thread is blocked for this operation. And in case of the commit, since it
is done asynchronously on the impl side, it holds on to the event to decide when
to release the main thread. To hide the completion events completely inside the
channel, we'll have to move some of that logic inside the channel.

Would it be better if we had CompletionEvent as an interface instead.

CompletionEvent {
  Wait();
  Signal();
}
And the current completion event can be the implementation of this as
ThreadedCompletionEvent.

We pass the wait_until_done param to a function on ChannelMain to make a
synchronous call, which for ThreadedChannel will create this event and pass it
to ProxyImpl so the logic there remains same as now.

For Blimp we just make the network call. It shouldn't need to block the main
thread on the server since any state needed in the call at that point is
serialized and sent to the client.
The completion event implementation sent to ProxyImpl on the client will
probably just have a callback that clears any local state after an operation.
For instance, dropping the commit proto/recycling the local LTH object after the
impl side has committed the state. Is there anything I could be missing here?

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:394: Proxy::ImplThreadTaskRunner()->PostTask(
On 2015/10/15 20:25:04, vmpstr wrote:
> Why did you only do this for a particular set of PostTasks but not for others?

I missed this one. Will change it in this patch.

I was trying to do it separately for these methods and then for the methods in
the commit sequence starting from setneedscommit to break it into smaller
patches.

The last patch which splits the class will include the methods that initializes
and closes ProxyImpl.

[vmpstr, 2015-10-17 00:05:51]

I also wonder if we should include some smoke tests here, just to make sure that
the channel functions actually post corresponding calls. Wdyt?

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:142:
main().channel_main->FinishAllRenderingOnImpl(&completion);
On 2015/10/15 23:45:51, Khushal wrote:
> On 2015/10/15 20:25:04, vmpstr wrote:
> > It's a bit awkward for me that channel_main refers to the channel that
> > communicates with the impl side. Is "main" referring to where the channel is
> > located, but not where it's communicating to?
> 
> Since the class is split into the main and impl components, each side needs an
> interface to communicate with the other side. The current naming scheme is
such
> that ChannelMain is the interface and its implementation is located on the
main
> side. For Blimp, a RemoteChannelMain will implement this that lives on the
> server.
> 
> We can rename it channel_main_to_impl if that makes it clearer?

I'm not sure, that's also very verbose. I guess we can keep it as is for now,
but if you think of something better that would be good :P

I'm almost tempted to say that main().channel->Foo(); is better than
main().channel_main->Foo(). But I think that's not verbose enough.

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:143: completion.Wait();
On 2015/10/15 23:45:50, Khushal wrote:
> On 2015/10/15 20:25:04, vmpstr wrote:
> > The typical thing to do is create a completion / post task / wait.
> > 
> > What's the story for when this isn't a post task? Should the parameter to
the
> > function be something like a bool wait_until_done; and it can create the
> > completion in the channel? Or do you think it's better to keep this outside?
> 
> Right now completion events make it explicit to methods on both side that the
> main thread is blocked for this operation. And in case of the commit, since it
> is done asynchronously on the impl side, it holds on to the event to decide
when
> to release the main thread. To hide the completion events completely inside
the
> channel, we'll have to move some of that logic inside the channel.

Specifically what logic are you referring to? I mostly see

CompletionEvent event;
channel->Foo(&event);
event.Wait();

On the other end this maps to
Foo(CompletionEvent* event) {
 ...
 event->Signal();
}

If the logic is more complicated than that, we can keep it outside or whatever.
But if it's exactly this pattern over and over again, I would say that the
channel itself can do this, no?

> 
> Would it be better if we had CompletionEvent as an interface instead.
> 
> CompletionEvent {
>   Wait();
>   Signal();
> }
> And the current completion event can be the implementation of this as
> ThreadedCompletionEvent.
> 
> We pass the wait_until_done param to a function on ChannelMain to make a
> synchronous call, which for ThreadedChannel will create this event and pass it
> to ProxyImpl so the logic there remains same as now.
> 
> For Blimp we just make the network call. It shouldn't need to block the main
> thread on the server since any state needed in the call at that point is
> serialized and sent to the client.

All the more reason here to move the details of completion events into the
channel, right? If they remain outside, then who would signal the completion?
The channel, right?

> The completion event implementation sent to ProxyImpl on the client will
> probably just have a callback that clears any local state after an operation.
> For instance, dropping the commit proto/recycling the local LTH object after
the
> impl side has committed the state. Is there anything I could be missing here?

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:394: Proxy::ImplThreadTaskRunner()->PostTask(
On 2015/10/15 23:45:50, Khushal wrote:
> On 2015/10/15 20:25:04, vmpstr wrote:
> > Why did you only do this for a particular set of PostTasks but not for
others?
> 
> I missed this one. Will change it in this patch.
> 
> I was trying to do it separately for these methods and then for the methods in
> the commit sequence starting from setneedscommit to break it into smaller
> patches.
> 
> The last patch which splits the class will include the methods that
initializes
> and closes ProxyImpl.

You can probably do all of the call sites that are simply changing from
PostTask(Foo) to channel->Foo(). I agree if there's some logic changes, then
that should be a separate patch.

[Khushal, 2015-10-17 02:02:48]

It would be a great idea to add tests for the Channel. I'll add them to this
patch, once we are good with the channel interfaces. :)

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:142:
main().channel_main->FinishAllRenderingOnImpl(&completion);
On 2015/10/17 00:05:51, vmpstr wrote:
> On 2015/10/15 23:45:51, Khushal wrote:
> > On 2015/10/15 20:25:04, vmpstr wrote:
> > > It's a bit awkward for me that channel_main refers to the channel that
> > > communicates with the impl side. Is "main" referring to where the channel
is
> > > located, but not where it's communicating to?
> > 
> > Since the class is split into the main and impl components, each side needs
an
> > interface to communicate with the other side. The current naming scheme is
> such
> > that ChannelMain is the interface and its implementation is located on the
> main
> > side. For Blimp, a RemoteChannelMain will implement this that lives on the
> > server.
> > 
> > We can rename it channel_main_to_impl if that makes it clearer?
> 
> I'm not sure, that's also very verbose. I guess we can keep it as is for now,
> but if you think of something better that would be good :P
> 
> I'm almost tempted to say that main().channel->Foo(); is better than
> main().channel_main->Foo(). But I think that's not verbose enough.

main() wouldn't be there when ProxyMain comes. Then it would just be
channel_main_->Foo(). We can may be name the variable channel_ then?
Can't think of anything better right now. :(

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:143: completion.Wait();
On 2015/10/17 00:05:51, vmpstr wrote:
> On 2015/10/15 23:45:50, Khushal wrote:
> > On 2015/10/15 20:25:04, vmpstr wrote:
> > > The typical thing to do is create a completion / post task / wait.
> > > 
> > > What's the story for when this isn't a post task? Should the parameter to
> the
> > > function be something like a bool wait_until_done; and it can create the
> > > completion in the channel? Or do you think it's better to keep this
outside?
> > 
> > Right now completion events make it explicit to methods on both side that
the
> > main thread is blocked for this operation. And in case of the commit, since
it
> > is done asynchronously on the impl side, it holds on to the event to decide
> when
> > to release the main thread. To hide the completion events completely inside
> the
> > channel, we'll have to move some of that logic inside the channel.
> 
> Specifically what logic are you referring to? I mostly see
> 
> CompletionEvent event;
> channel->Foo(&event);
> event.Wait();
> 
> On the other end this maps to
> Foo(CompletionEvent* event) {
>  ...
>  event->Signal();
> }
> 
> If the logic is more complicated than that, we can keep it outside or
whatever.
> But if it's exactly this pattern over and over again, I would say that the
> channel itself can do this, no?
> 
> > 
> > Would it be better if we had CompletionEvent as an interface instead.
> > 
> > CompletionEvent {
> >   Wait();
> >   Signal();
> > }
> > And the current completion event can be the implementation of this as
> > ThreadedCompletionEvent.
> > 
> > We pass the wait_until_done param to a function on ChannelMain to make a
> > synchronous call, which for ThreadedChannel will create this event and pass
it
> > to ProxyImpl so the logic there remains same as now.
> > 
> > For Blimp we just make the network call. It shouldn't need to block the main
> > thread on the server since any state needed in the call at that point is
> > serialized and sent to the client.
> 
> All the more reason here to move the details of completion events into the
> channel, right? If they remain outside, then who would signal the completion?
> The channel, right?
> 
> > The completion event implementation sent to ProxyImpl on the client will
> > probably just have a callback that clears any local state after an
operation.
> > For instance, dropping the commit proto/recycling the local LTH object after
> the
> > impl side has committed the state. Is there anything I could be missing
here?
> 

I like the idea of keeping it completely in the channel. Something like:
channel->Foo(bool wait_until_done);

And in the channel,
Foo(bool wait_until_done) {
  CompletionEvent event;
  TaskRunner->PostTask(FROM_HERE,
      base::Bind(&ThreadedChannel::DoBlockingFoo,
                 channel_weak_ptr_,
                 &completion);
  event.Wait();
}

DoBlockingFoo(CompletionEvent* event) {
  proxy_impl_->Foo();
  event->Signal();
}

With the commit, thread proxy holds on to the event to unblock it after the
commit is finished/after the sync tree is activated.
We could add callbacks for to the channel to move this logic into the channel?

channel_impl->DidActivateSyncTree() which would unblock the main thread?

https://codereview.chromium.org/1377063003/diff/60001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:394: Proxy::ImplThreadTaskRunner()->PostTask(
On 2015/10/17 00:05:51, vmpstr wrote:
> On 2015/10/15 23:45:50, Khushal wrote:
> > On 2015/10/15 20:25:04, vmpstr wrote:
> > > Why did you only do this for a particular set of PostTasks but not for
> others?
> > 
> > I missed this one. Will change it in this patch.
> > 
> > I was trying to do it separately for these methods and then for the methods
in
> > the commit sequence starting from setneedscommit to break it into smaller
> > patches.
> > 
> > The last patch which splits the class will include the methods that
> initializes
> > and closes ProxyImpl.
> 
> You can probably do all of the call sites that are simply changing from
> PostTask(Foo) to channel->Foo(). I agree if there's some logic changes, then
> that should be a separate patch.

Done.
The only calls left are for initializing and closing the impl side, which would
make sense to do when the class is actually split.

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/proxy_common.h
File cc/trees/proxy_common.h (right):

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/proxy_common.h...
cc/trees/proxy_common.h:26: struct CC_EXPORT BlockedMainCommitState {
This is basically to simulate some form of message passing in the threaded case
of the channel. It would be great if ProxyImpl didn't hold on to a pointer to
the LTH. Since it sees the actual state of the LayerTree only during the commit.

I would prefer it if we could something like,

channel_main->StartCommitOnImpl(LayerTreeHost* layer_tree_host, bool*
hold_commit_for_activation);

Channel holds on to the pointer or proto.

channel_impl->FinishCommitToLayerTreeHostImpl(LayerTreeHostImpl* host_impl )
In this method, we finish the commit with
LTH->FinishCommitOnImplThread(host_impl) and clear the
hold_commit_for_activation flag.
I would just pass the flag value and clear it when starting the commit, but the
startcommitonimpl in ProxyImpl can abort the call if there is no LTHI and the
flag remains uncleared then.

[vmpstr, 2015-10-19 18:08:08]

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (left):

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:799: completion->Signal();
We can keep the completion logic as is for now and follow-up with one of the
approaches we discussed offline.

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:514: const BlockedMainCommitState*
ThreadProxy::blocked_commit_state() const {
Can this be moved into a separate patch? Let's just do the post task to channel
transition here. What do you think?

[Khushal, 2015-10-20 11:58:27]

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (left):

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:799: completion->Signal();
On 2015/10/19 18:08:07, vmpstr wrote:
> We can keep the completion logic as is for now and follow-up with one of the
> approaches we discussed offline. 

Sounds good. I've added the methods we discussed to CompletionEvent. I think
regularly checking the event for IsSignaled might be equivalent to blocking the
thread, which we'd want to avoid on the client. We can change it if it turns out
to be a problem.

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1377063003/diff/80001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:514: const BlockedMainCommitState*
ThreadProxy::blocked_commit_state() const {
On 2015/10/19 18:08:07, vmpstr wrote:
> Can this be moved into a separate patch? Let's just do the post task to
channel
> transition here. What do you think?

Sounds good, will handle this in the next patch.

[vmpstr, 2015-10-26 17:57:36]

Looks good, just one comment

https://codereview.chromium.org/1377063003/diff/100001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/1377063003/diff/100001/cc/trees/thread_proxy....
cc/trees/thread_proxy.h:244: void SetRendererCapabilitiesMainCopy(
I'm a bit hesitant about these becoming public. Can we instead make the channel
classes friends of this one? That would limit the exposure of these functions
that should not be called directly.

[Khushal, 2015-10-26 20:56:02]

https://codereview.chromium.org/1377063003/diff/100001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/1377063003/diff/100001/cc/trees/thread_proxy....
cc/trees/thread_proxy.h:244: void SetRendererCapabilitiesMainCopy(
On 2015/10/26 17:57:36, vmpstr wrote:
> I'm a bit hesitant about these becoming public. Can we instead make the
channel
> classes friends of this one? That would limit the exposure of these functions
> that should not be called directly.

Done. Added ThreadProxyForTest as a friend class as well.

[vmpstr, 2015-10-27 21:38:08]

lgtm thanks.

[Khushal, 2015-10-27 21:39:31]

On 2015/10/27 21:38:08, vmpstr wrote:
> lgtm thanks.

Thanks Vlad!

[Khushal, 2015-10-27 21:39:44]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2015-10-27 21:39:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1377063003/#ps140001
(title: "Keep ProxyMain and ProxyImpl methods private.")

[commit-bot: I haz the power, 2015-10-27 21:42:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377063003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377063003/140001

[commit-bot: I haz the power, 2015-10-28 00:01:06]

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

[commit-bot: I haz the power, 2015-10-28 00:01:06]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Khushal, 2015-10-28 01:33:53]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2015-10-28 01:34:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377063003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377063003/140001

[commit-bot: I haz the power, 2015-10-28 02:08:38]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-28 02:09:08]

Patchset 8 (id:??) landed as
https://crrev.com/e3c9fa95884885ec885c934597b09a1bc3d8979e
Cr-Commit-Position: refs/heads/master@{#356491}