Blob Channel failed to register service

Blob Channel failed to register service

Mojo team enforced capability spec renderer <--> browser. Only the interfaces in the manifest are exposed to the renderer (https://codereview.chromium.org/2259903002). The BlobChannel wasn't in the list, so blimp engine wasn't able to register the BlobChannel services, causing the crash

This CL adds BlobChannel to the interfaces exposed to the renderer so now the images are sent correctly.

BUG=649858
Committed: https://crrev.com/dd442d48128ff3043c105d83b922806e4c4f9080
Cr-Commit-Position: refs/heads/master@{#421425}

[Menglin, 2016-09-27 18:06:34]

mlliu@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-27 19:05:43]

This seems correct. Some inline questions to verify that the manifest resource
is actually found and loaded as expected.

https://codereview.chromium.org/2371233002/diff/1/blimp/engine/app/blimp_cont...
File blimp/engine/app/blimp_content_browser_client.cc (right):

https://codereview.chromium.org/2371233002/diff/1/blimp/engine/app/blimp_cont...
blimp/engine/app/blimp_content_browser_client.cc:58: VLOG(0) << "mlliu:
GetServiceManifestOverlay" << name;
Does this run?

https://codereview.chromium.org/2371233002/diff/1/blimp/engine/app/blimp_cont...
blimp/engine/app/blimp_content_browser_client.cc:66: base::StringPiece
manifest_contents =
Is this the value you expect?

[Menglin, 2016-09-27 19:51:39]

On 2016/09/27 at 19:05:43, rockot wrote:
> This seems correct. Some inline questions to verify that the manifest resource
is actually found and loaded as expected.
> 
>
https://codereview.chromium.org/2371233002/diff/1/blimp/engine/app/blimp_cont...
> File blimp/engine/app/blimp_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2371233002/diff/1/blimp/engine/app/blimp_cont...
> blimp/engine/app/blimp_content_browser_client.cc:58: VLOG(0) << "mlliu:
GetServiceManifestOverlay" << name;
> Does this run?
> 
>
https://codereview.chromium.org/2371233002/diff/1/blimp/engine/app/blimp_cont...
> blimp/engine/app/blimp_content_browser_client.cc:66: base::StringPiece
manifest_contents =
> Is this the value you expect?

Thanks for your help!
as a note: I didn't output and load the pak file

[Menglin, 2016-09-27 19:52:10]

mlliu@chromium.org changed reviewers:
+ nyquist@chromium.org

[Menglin, 2016-09-27 19:52:10]

Hi Tommy ptal. thanks!

[Ken Rockot(use gerrit already), 2016-09-27 19:58:41]

FWIW the manifest overlay and usage LGTM

[nyquist, 2016-09-27 20:48:48]

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:15: "$root_gen_dir/blimp/blimp_browser_resources.pak",
Does this mean that this will also work within the container?

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:51: grit("resources") {
Should this have visibility = [ ":*" ]?

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:54: output_dir = "$root_gen_dir/blimp"
Should this be $root_gen_dir/blimp/engine ?

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.cc (right):

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.cc:60: if (name ==
content::kBrowserMojoApplicationName)
Nit: I think most of the rest of the engine code (and generally code in //blimp)
uses curlies. So could you change to that here and below?

[Menglin, 2016-09-27 22:06:40]

new patch uploaded.

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:15: "$root_gen_dir/blimp/blimp_browser_resources.pak",
On 2016/09/27 20:48:48, nyquist wrote:
> Does this mean that this will also work within the container?

Yes. I verified the engine starts correctly in the container and it's able to
get the manifest overlay

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:51: grit("resources") {
On 2016/09/27 20:48:48, nyquist wrote:
> Should this have visibility = [ ":*" ]?

Done.

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:54: output_dir = "$root_gen_dir/blimp"
On 2016/09/27 20:48:48, nyquist wrote:
> Should this be $root_gen_dir/blimp/engine ?

Done.

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.cc (right):

https://codereview.chromium.org/2371233002/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.cc:60: if (name ==
content::kBrowserMojoApplicationName)
On 2016/09/27 20:48:48, nyquist wrote:
> Nit: I think most of the rest of the engine code (and generally code in
//blimp)
> uses curlies. So could you change to that here and below?

Done.

[nyquist, 2016-09-27 22:39:46]

lgtm

[nyquist, 2016-09-27 22:40:44]

Could you expand the CL description?

Maybe one paragraph each for:
- what happened before?
- why was that bad?
- how does this CL make it better?

[Menglin, 2016-09-27 23:01:21]

Description was changed from

==========
Blob Channel failed to register service

BUG=649858
==========

to

==========
Blob Channel failed to register service

Mojo team enforced capability spec renderer <--> browser. Only the interfaces in
the manifest are exposed to the renderer
(https://codereview.chromium.org/2259903002). The BlobChannel wasn't in the
list, so blimp engine wasn't able to register the BlobChannel services, causing
the crash

This CL adds BlobChannel to the interfaces exposed to the renderer so now the
images are sent correctly.

BUG=649858
==========

[Menglin, 2016-09-27 23:01:42]

The CQ bit was checked by mlliu@chromium.org

[Menglin, 2016-09-27 23:01:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2371233002/#ps40001
(title: "change output_dir to blimp/engine. verify on docker")

[commit-bot: I haz the power, 2016-09-27 23:02:35]

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, 2016-09-27 23:10:48]

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

[commit-bot: I haz the power, 2016-09-27 23:10:49]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Menglin, 2016-09-27 23:11:03]

mlliu@chromium.org changed reviewers:
+ thestig@chromium.org

[Menglin, 2016-09-27 23:11:04]

Hi thestig@, ptal. thanks!

[Lei Zhang, 2016-09-27 23:14:18]

lgtm

https://codereview.chromium.org/2371233002/diff/40001/tools/gritsettings/reso...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/2371233002/diff/40001/tools/gritsettings/reso...
tools/gritsettings/resource_ids:287: "includes": [30700],
Do 30680. If headless_lib_resources.grd needs more, they can adjust this number
later.

[Menglin, 2016-09-27 23:15:11]

The CQ bit was checked by mlliu@chromium.org

[commit-bot: I haz the power, 2016-09-27 23:15:24]

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

[Menglin, 2016-09-27 23:20:24]

The CQ bit was unchecked by mlliu@chromium.org

[Menglin, 2016-09-27 23:35:02]

On 2016/09/27 23:14:18, Lei Zhang wrote:
> lgtm
> 
>
https://codereview.chromium.org/2371233002/diff/40001/tools/gritsettings/reso...
> File tools/gritsettings/resource_ids (right):
> 
>
https://codereview.chromium.org/2371233002/diff/40001/tools/gritsettings/reso...
> tools/gritsettings/resource_ids:287: "includes": [30700],
> Do 30680. If headless_lib_resources.grd needs more, they can adjust this
number
> later.

I don't know why if i use 30680 it doesn't work.
BlimpContentBrowserClient::GetServiceManifestOverlay won't even get called..

[Lei Zhang, 2016-09-27 23:38:12]

On 2016/09/27 23:35:02, Menglin wrote:
> I don't know why if i use 30680 it doesn't work.
> BlimpContentBrowserClient::GetServiceManifestOverlay won't even get called..

Patch set 3 is red. Let's leave it at 30700 and get the build green first.

Once you have green try bots. Try 30680, and if it does not work, show me what
the error is.

[Menglin, 2016-09-28 00:48:43]

The CQ bit was checked by mlliu@chromium.org

[Menglin, 2016-09-28 00:48:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, rockot@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2371233002/#ps80001
(title: "update the generated blimp_browser_resources.h path in
blimp/engine/app/blimp_content_browser_clien…")

[commit-bot: I haz the power, 2016-09-28 00:49:11]

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, 2016-09-28 01:58:36]

Description was changed from

==========
Blob Channel failed to register service

Mojo team enforced capability spec renderer <--> browser. Only the interfaces in
the manifest are exposed to the renderer
(https://codereview.chromium.org/2259903002). The BlobChannel wasn't in the
list, so blimp engine wasn't able to register the BlobChannel services, causing
the crash

This CL adds BlobChannel to the interfaces exposed to the renderer so now the
images are sent correctly.

BUG=649858
==========

to

==========
Blob Channel failed to register service

Mojo team enforced capability spec renderer <--> browser. Only the interfaces in
the manifest are exposed to the renderer
(https://codereview.chromium.org/2259903002). The BlobChannel wasn't in the
list, so blimp engine wasn't able to register the BlobChannel services, causing
the crash

This CL adds BlobChannel to the interfaces exposed to the renderer so now the
images are sent correctly.

BUG=649858
==========

[commit-bot: I haz the power, 2016-09-28 01:58:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-28 02:00:45]

Description was changed from

==========
Blob Channel failed to register service

Mojo team enforced capability spec renderer <--> browser. Only the interfaces in
the manifest are exposed to the renderer
(https://codereview.chromium.org/2259903002). The BlobChannel wasn't in the
list, so blimp engine wasn't able to register the BlobChannel services, causing
the crash

This CL adds BlobChannel to the interfaces exposed to the renderer so now the
images are sent correctly.

BUG=649858
==========

to

==========
Blob Channel failed to register service

Mojo team enforced capability spec renderer <--> browser. Only the interfaces in
the manifest are exposed to the renderer
(https://codereview.chromium.org/2259903002). The BlobChannel wasn't in the
list, so blimp engine wasn't able to register the BlobChannel services, causing
the crash

This CL adds BlobChannel to the interfaces exposed to the renderer so now the
images are sent correctly.

BUG=649858

Committed: https://crrev.com/dd442d48128ff3043c105d83b922806e4c4f9080
Cr-Commit-Position: refs/heads/master@{#421425}
==========

[commit-bot: I haz the power, 2016-09-28 02:00:47]

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