content/gpu: Set up a mojom connection between browser and gpu.

content/gpu: Set up a mojom connection between browser and gpu.

Set up a mojom connection for content-browser to talk to content-gpu
through the mojom.GpuMain interface. Subsequent CLs will add code to
allow chrome-browser to set up a mojom.GpuService connection through
GpuMain to the gpu process. This GpuService will replace the existing
chrome-ipc messages between chrome-browser and chrome-gpu.

BUG=643746, 630895
Review-Url: https://codereview.chromium.org/2625933002
Cr-Commit-Position: refs/heads/master@{#443044}
Committed: https://chromium.googlesource.com/chromium/src/+/6c5aed8c3c8b5a67bff553badfe229f96f3fff23

[sadrul, 2017-01-11 01:24:51]

sadrul@chromium.org changed reviewers:
+ avi@chromium.org, jbauman@chromium.org, rockot@chromium.org

[sadrul, 2017-01-11 01:24:52]

jbauman@ for content/browser/gpu and content/gpu

rockot@ for manifest.json, and changes in GpuChildThread and GpuProcessHost for
setting up the GpuMain connection.

avi@ for content/browser, content/common, and content/public changes.

[sadrul, 2017-01-11 01:25:12]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 01:25:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-01-11 01:48:30]

sadrul@chromium.org changed reviewers:
+ tsepez@chromium.org

[sadrul, 2017-01-11 01:48:30]

+tsepez@ for manifest.json

[jbauman, 2017-01-11 01:49:22]

lgtm, though I've got one question.

https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
File content/gpu/gpu_child_thread.cc (left):

https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
content/gpu/gpu_child_thread.cc:207: // OnInitialize() is invoked.
Just checking, but is the reason we can remove this that mojo messages are now
processed in order with the GpuMsg_Initialize IPC message?

[sadrul, 2017-01-11 01:55:46]

https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
File content/gpu/gpu_child_thread.cc (left):

https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
content/gpu/gpu_child_thread.cc:207: // OnInitialize() is invoked.
On 2017/01/11 01:49:22, jbauman wrote:
> Just checking, but is the reason we can remove this that mojo messages are now
> processed in order with the GpuMsg_Initialize IPC message?

The Pause/Resume was added in https://crrev.com/402996, to make sure
OnInitialize() is called before any of the mojo requests are processed.
https://crrev.com/406761 then moved the mojo registrations in OnInitialize(),
which means the mojo requests could not be processed before OnInitialize() is
called anyway. So I think that makes it safe to remove Pause/Remove now.

This is also necessary because OnInitialize() itself will be replaced by a mojom
call (see more details in https://codereview.chromium.org/2612623002/), which
means this needs to allow processing mojo requests from the start.

I have added rockot@ to the reviewer list to make sure this reasoning is OK, and
there isn't a better way for doing this.

[commit-bot: I haz the power, 2017-01-11 03:35:16]

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

[commit-bot: I haz the power, 2017-01-11 03:35:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Avi (use Gerrit), 2017-01-11 17:22:38]

lgtm

stamp

[Tom Sepez, 2017-01-11 17:50:40]

lgtm

[Ken Rockot(use gerrit already), 2017-01-11 18:17:00]

https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
File content/gpu/gpu_child_thread.cc (left):

https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
content/gpu/gpu_child_thread.cc:207: // OnInitialize() is invoked.
On 2017/01/11 at 01:55:46, sadrul wrote:
> On 2017/01/11 01:49:22, jbauman wrote:
> > Just checking, but is the reason we can remove this that mojo messages are
now
> > processed in order with the GpuMsg_Initialize IPC message?
> 
> The Pause/Resume was added in https://crrev.com/402996, to make sure
OnInitialize() is called before any of the mojo requests are processed.
https://crrev.com/406761 then moved the mojo registrations in OnInitialize(),
which means the mojo requests could not be processed before OnInitialize() is
called anyway. So I think that makes it safe to remove Pause/Remove now.

This rationale seems kind of circular to me. I don't think it's safe to remove
this behavior. r406761 was able to move interface registration to OnInitialize
precisely *because* we pause the registry binding here. If we don't pause it,
interface requests can be dispatched (and thus those interfaces can be bound and
receive messages) before OnInitialize() runs.

> 
> This is also necessary because OnInitialize() itself will be replaced by a
mojom call (see more details in https://codereview.chromium.org/2612623002/),
which means this needs to allow processing mojo requests from the start.
> 
> I have added rockot@ to the reviewer list to make sure this reasoning is OK,
and there isn't a better way for doing this.

[Ken Rockot(use gerrit already), 2017-01-11 18:18:19]

On 2017/01/11 at 18:17:00, Ken Rockot wrote:
>
https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
> File content/gpu/gpu_child_thread.cc (left):
> 
>
https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
> content/gpu/gpu_child_thread.cc:207: // OnInitialize() is invoked.
> On 2017/01/11 at 01:55:46, sadrul wrote:
> > On 2017/01/11 01:49:22, jbauman wrote:
> > > Just checking, but is the reason we can remove this that mojo messages are
now
> > > processed in order with the GpuMsg_Initialize IPC message?
> > 
> > The Pause/Resume was added in https://crrev.com/402996, to make sure
OnInitialize() is called before any of the mojo requests are processed.
https://crrev.com/406761 then moved the mojo registrations in OnInitialize(),
which means the mojo requests could not be processed before OnInitialize() is
called anyway. So I think that makes it safe to remove Pause/Remove now.
> 
> This rationale seems kind of circular to me. I don't think it's safe to remove
this behavior. r406761 was able to move interface registration to OnInitialize
precisely *because* we pause the registry binding here. If we don't pause it,
interface requests can be dispatched (and thus those interfaces can be bound and
receive messages) before OnInitialize() runs.
> 

Well, correction: if we just stop pausing, interface requests may be dispatched
and *dropped* before OnInitialize() runs. We'd have to move the registrations
back here as well, and then we would have the problem of the interfaces being
reachable before OnInitialize().

> > 
> > This is also necessary because OnInitialize() itself will be replaced by a
mojom call (see more details in https://codereview.chromium.org/2612623002/),
which means this needs to allow processing mojo requests from the start.
> > 
> > I have added rockot@ to the reviewer list to make sure this reasoning is OK,
and there isn't a better way for doing this.

[sadrul, 2017-01-11 21:15:44]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 21:16:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-01-11 21:16:19]

On 2017/01/11 18:18:19, Ken Rockot wrote:
> On 2017/01/11 at 18:17:00, Ken Rockot wrote:
> >
>
https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
> > File content/gpu/gpu_child_thread.cc (left):
> > 
> >
>
https://codereview.chromium.org/2625933002/diff/1/content/gpu/gpu_child_threa...
> > content/gpu/gpu_child_thread.cc:207: // OnInitialize() is invoked.
> > On 2017/01/11 at 01:55:46, sadrul wrote:
> > > On 2017/01/11 01:49:22, jbauman wrote:
> > > > Just checking, but is the reason we can remove this that mojo messages
are
> now
> > > > processed in order with the GpuMsg_Initialize IPC message?
> > > 
> > > The Pause/Resume was added in https://crrev.com/402996, to make sure
> OnInitialize() is called before any of the mojo requests are processed.
> https://crrev.com/406761 then moved the mojo registrations in OnInitialize(),
> which means the mojo requests could not be processed before OnInitialize() is
> called anyway. So I think that makes it safe to remove Pause/Remove now.
> > 
> > This rationale seems kind of circular to me. I don't think it's safe to
remove
> this behavior. r406761 was able to move interface registration to OnInitialize
> precisely *because* we pause the registry binding here. If we don't pause it,
> interface requests can be dispatched (and thus those interfaces can be bound
and
> receive messages) before OnInitialize() runs.
> > 
> 
> Well, correction: if we just stop pausing, interface requests may be
dispatched
> and *dropped* before OnInitialize() runs. We'd have to move the registrations
> back here as well, and then we would have the problem of the interfaces being
> reachable before OnInitialize().
> 
> > > 
> > > This is also necessary because OnInitialize() itself will be replaced by a
> mojom call (see more details in https://codereview.chromium.org/2612623002/),
> which means this needs to allow processing mojo requests from the start.
> > > 
> > > I have added rockot@ to the reviewer list to make sure this reasoning is
OK,
> and there isn't a better way for doing this.

Thanks for the detailed explanation! I have restored Pause/Resume. PTAL

[commit-bot: I haz the power, 2017-01-11 21:19:37]

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

[commit-bot: I haz the power, 2017-01-11 21:19:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Ken Rockot(use gerrit already), 2017-01-11 21:21:22]

lgtm

https://codereview.chromium.org/2625933002/diff/20001/content/public/app/mojo...
File content/public/app/mojo/content_gpu_manifest.json (right):

https://codereview.chromium.org/2625933002/diff/20001/content/public/app/mojo...
content/public/app/mojo/content_gpu_manifest.json:10: "ui::mojom::GpuMain"
Note that Channel-associated interfaces are in no way filtered against manifest
capabilities (or anything else), so this isn't necessary.

[sadrul, 2017-01-11 21:21:31]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 21:22:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 22:44:20]

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

[commit-bot: I haz the power, 2017-01-11 22:44:21]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-01-11 22:52:09]

https://codereview.chromium.org/2625933002/diff/20001/content/public/app/mojo...
File content/public/app/mojo/content_gpu_manifest.json (right):

https://codereview.chromium.org/2625933002/diff/20001/content/public/app/mojo...
content/public/app/mojo/content_gpu_manifest.json:10: "ui::mojom::GpuMain"
On 2017/01/11 21:21:22, Ken Rockot wrote:
> Note that Channel-associated interfaces are in no way filtered against
manifest
> capabilities (or anything else), so this isn't necessary.

Ah, I see. Thanks! I am keeping it for now since I think it's nice to look at
the manifest to see what communication channels exist between apps. But let me
know if you would prefer that I remove this, and I can do that in a follow up
CL.

[sadrul, 2017-01-11 22:56:51]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2017-01-11 22:56:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
avi@chromium.org, jbauman@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2625933002/#ps40001
(title: "tot merge")

[commit-bot: I haz the power, 2017-01-11 22:57:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 23:11:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484175411175620,
"parent_rev": "b4836c494c0ce6a8c5cf285736efc7cf1ae5fc91", "commit_rev":
"6c5aed8c3c8b5a67bff553badfe229f96f3fff23"}

[commit-bot: I haz the power, 2017-01-11 23:12:16]

Description was changed from

==========
content/gpu: Set up a mojom connection between browser and gpu.

Set up a mojom connection for content-browser to talk to content-gpu
through the mojom.GpuMain interface. Subsequent CLs will add code to
allow chrome-browser to set up a mojom.GpuService connection through
GpuMain to the gpu process. This GpuService will replace the existing
chrome-ipc messages between chrome-browser and chrome-gpu.

BUG=643746, 630895
==========

to

==========
content/gpu: Set up a mojom connection between browser and gpu.

Set up a mojom connection for content-browser to talk to content-gpu
through the mojom.GpuMain interface. Subsequent CLs will add code to
allow chrome-browser to set up a mojom.GpuService connection through
GpuMain to the gpu process. This GpuService will replace the existing
chrome-ipc messages between chrome-browser and chrome-gpu.

BUG=643746, 630895

Review-Url: https://codereview.chromium.org/2625933002
Cr-Commit-Position: refs/heads/master@{#443044}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c5aed8c3c8b5a67bff553badfe2...
==========

[commit-bot: I haz the power, 2017-01-11 23:12:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6c5aed8c3c8b5a67bff553badfe2...