Pull in vr_shell code for webvr

Pull in vr_shell code for webvr

BUG=389343
Committed: https://crrev.com/f4fa4460242c7a38d416fe7f83caa88c18f8ae03
Cr-Commit-Position: refs/heads/master@{#424616}

[bshe, 2016-10-05 15:16:59]

bshe@chromium.org changed reviewers:
+ agrieve@chromium.org, dtrainor@chromium.org, xiyuan@chromium.org

[bshe, 2016-10-05 15:17:00]

dtrainor@chromium.org: Please review changes in 
chrome/android/BUILD.gn
chrome/browser/android/chrome_jni_registrar.cc
chrome/browser/android/vr_shell/BUILD.gn

xiyuan@chromium.org: Please review changes in 
chrome/browser/browser_resources.grd
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
chrome/common/url_constants.h
chrome/common/url_constants.h

agrieve@chromium.org: Please review changes in 
gn changes in general.

Thanks!
asimjour@chromium.org, cjgrant@chromium.org, mthiesse@chromium.org,  please feel
free to chime in

[mthiesse, 2016-10-05 15:30:03]

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

[mthiesse, 2016-10-05 15:30:04]

Please un-enable the vr shell menu item incidentally enabled by this patch.

https://codereview.chromium.org/2399533002/diff/1/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (left):

https://codereview.chromium.org/2399533002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:201: <!-- TODO: Enable only for VrShell,
independent of Android. -->
Delete this stale comment.

[bshe, 2016-10-05 18:20:58]

And hided the menu entry behind a new flag.

https://codereview.chromium.org/2399533002/diff/1/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (left):

https://codereview.chromium.org/2399533002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:201: <!-- TODO: Enable only for VrShell,
independent of Android. -->
On 2016/10/05 15:30:03, mthiesse wrote:
> Delete this stale comment.

Done.

[mthiesse, 2016-10-05 18:47:25]

lgtm

[agrieve, 2016-10-05 19:04:58]

https://codereview.chromium.org/2399533002/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/20001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:249: if ((enable_vr_shell || enable_webvr) &&
android_java_ui) {
nit: remove android_java_ui (here and in the other files you're touching)

https://codereview.chromium.org/2399533002/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3357: deps += [ "android/vr_shell:vr_shell" ]
It seems this is actually the point of the change to pull in "shell" things for
just "enable_webvr", but it's quite confusing to read. 

Should we make enable_webvr force enable_vr_shell to be true?
Can we rename all of these "vr_shell" things that are required by webvr to
"vr_common"?

[bshe, 2016-10-05 19:53:06]

Thanks! PTAL

https://codereview.chromium.org/2399533002/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/20001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:249: if ((enable_vr_shell || enable_webvr) &&
android_java_ui) {
On 2016/10/05 19:04:58, agrieve wrote:
> nit: remove android_java_ui (here and in the other files you're touching)

Done.

https://codereview.chromium.org/2399533002/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3357: deps += [ "android/vr_shell:vr_shell" ]
On 2016/10/05 19:04:58, agrieve wrote:
> It seems this is actually the point of the change to pull in "shell" things
for
> just "enable_webvr", but it's quite confusing to read. 
> 
> Should we make enable_webvr force enable_vr_shell to be true?
> Can we rename all of these "vr_shell" things that are required by webvr to
> "vr_common"?

make sense. renamed to vr_common. We can't force enable_vr_shell to be true
when enable_webvr is true. Overtime, we will have code and resources that are
only for vr shell. And it is possible that we enable webvr by default but not vr
shell.

[xiyuan, 2016-10-05 19:56:53]

lgtm

[David Trainor- moved to gerrit, 2016-10-07 06:38:38]

lgtm

[bshe, 2016-10-07 17:29:03]

On 2016/10/07 06:38:38, David Trainor wrote:
> lgtm

friendly ping? Andrew, PTAL

[agrieve, 2016-10-07 17:36:44]

Sorry, thought I sent these but seems they were just drafts still.

If this all makes sense to you as-is, feel free to push back. Just as an
unknowing reviewer, the shell vs webvr is hard to grok.

https://codereview.chromium.org/2399533002/diff/40001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:250: java_files += chrome_vr_java_sources
note: if you ever have java files that will be different for shell vs webvr,
it'd be better to just inline them here rather than have this
chrome_vr_java_sources variable.

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:179: #include
"chrome/browser/android/vr_shell/vr_shell.h"
can these be vr_common/vr_common.h?

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/BUILD.gn:57:
generate_jni("vr_shell_jni_headers") {
nit: put this and vr_shell_unittests behind if (enable_vr_shell)?

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:202: <include name="IDR_VR_SHELL_UI_HTML"
file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true"
flattenhtml="true" type="BINDATA" />
same here. vr_common.html?

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:2211:
"webui/vr_shell/vr_shell_ui_message_handler.cc",
and here

[bshe, 2016-10-07 20:36:22]

Thanks for review, Andrew! PTAL

https://codereview.chromium.org/2399533002/diff/40001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:250: java_files += chrome_vr_java_sources
On 2016/10/07 17:36:43, agrieve wrote:
> note: if you ever have java files that will be different for shell vs webvr,
> it'd be better to just inline them here rather than have this
> chrome_vr_java_sources variable.

We are expecting to ship webvr and vr shell at M56. And once shipped,
does it mean we will need to move these files back to  java_sources.gni
like other java files. If this is the case, I would prefer to keep it this way
for now if you don't mind.

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:179: #include
"chrome/browser/android/vr_shell/vr_shell.h"
On 2016/10/07 17:36:43, agrieve wrote:
> can these be vr_common/vr_common.h?

I think it probably makes sense to keep vr_shell ATM. WebVr only delegates its
presentation path to vr shell. A lot of the code in it still relevant to
vr_shell instead of WebVr. Ideally, we want to have a better separation of vr
shell and webvr's presentation code. But it is probably out the scope of this
CL. And once we have a better isolation of code, a file called vr_common then
makes more sense.

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/BUILD.gn:57:
generate_jni("vr_shell_jni_headers") {
On 2016/10/07 17:36:43, agrieve wrote:
> nit: put this and vr_shell_unittests behind if (enable_vr_shell)?

webvr also needs jni headers so I probably can't do it.
Added enable_vr_shell for vr_shell_unittests.

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:202: <include name="IDR_VR_SHELL_UI_HTML"
file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true"
flattenhtml="true" type="BINDATA" />
On 2016/10/07 17:36:43, agrieve wrote:
> same here. vr_common.html?

The reason I need to set enable webvr here is that webvr needs vr_shell's ui
system to draw
a shell ui on top of insecure content. So vr_shell.html probably makes more
senses since we
are still drawing shell ui in webvr?

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:2211:
"webui/vr_shell/vr_shell_ui_message_handler.cc",
On 2016/10/07 17:36:43, agrieve wrote:
> and here

ditto

[agrieve, 2016-10-09 02:32:50]

On 2016/10/07 20:36:22, bshe wrote:
> Thanks for review, Andrew! PTAL
> 
> https://codereview.chromium.org/2399533002/diff/40001/chrome/android/BUILD.gn
> File chrome/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/android/BUILD.gn...
> chrome/android/BUILD.gn:250: java_files += chrome_vr_java_sources
> On 2016/10/07 17:36:43, agrieve wrote:
> > note: if you ever have java files that will be different for shell vs webvr,
> > it'd be better to just inline them here rather than have this
> > chrome_vr_java_sources variable.
> 
> We are expecting to ship webvr and vr shell at M56. And once shipped,
> does it mean we will need to move these files back to  java_sources.gni
> like other java files. If this is the case, I would prefer to keep it this way
> for now if you don't mind.
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/chrome_jni_registrar.cc (right):
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
> chrome/browser/android/chrome_jni_registrar.cc:179: #include
> "chrome/browser/android/vr_shell/vr_shell.h"
> On 2016/10/07 17:36:43, agrieve wrote:
> > can these be vr_common/vr_common.h?
> 
> I think it probably makes sense to keep vr_shell ATM. WebVr only delegates its
> presentation path to vr shell. A lot of the code in it still relevant to
> vr_shell instead of WebVr. Ideally, we want to have a better separation of vr
> shell and webvr's presentation code. But it is probably out the scope of this
> CL. And once we have a better isolation of code, a file called vr_common then
> makes more sense.
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/android/...
> chrome/browser/android/vr_shell/BUILD.gn:57:
> generate_jni("vr_shell_jni_headers") {
> On 2016/10/07 17:36:43, agrieve wrote:
> > nit: put this and vr_shell_unittests behind if (enable_vr_shell)?
> 
> webvr also needs jni headers so I probably can't do it.
> Added enable_vr_shell for vr_shell_unittests.
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/browser_...
> File chrome/browser/browser_resources.grd (right):
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/browser_...
> chrome/browser/browser_resources.grd:202: <include name="IDR_VR_SHELL_UI_HTML"
> file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true"
> flattenhtml="true" type="BINDATA" />
> On 2016/10/07 17:36:43, agrieve wrote:
> > same here. vr_common.html?
> 
> The reason I need to set enable webvr here is that webvr needs vr_shell's ui
> system to draw
> a shell ui on top of insecure content. So vr_shell.html probably makes more
> senses since we
> are still drawing shell ui in webvr?
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/ui/BUILD.gn
> File chrome/browser/ui/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2399533002/diff/40001/chrome/browser/ui/BUILD...
> chrome/browser/ui/BUILD.gn:2211:
> "webui/vr_shell/vr_shell_ui_message_handler.cc",
> On 2016/10/07 17:36:43, agrieve wrote:
> > and here
> 
> ditto

Thanks for explaining the layout a bit more. Sounds like maybe what's needed is
a better name for "enable_vr_shell" so that it's clear that it means something
different from the many "vr_shell" targets that webvr relies on. 

Anyways, not a huge deal if the goal is to delete the flags once it's enabled
anyways. lgtm

[bshe, 2016-10-11 18:00:59]

The CQ bit was checked by bshe@chromium.org

[bshe, 2016-10-11 18:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, xiyuan@chromium.org, dtrainor@chromium.org,
agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2399533002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-11 18:01:51]

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-10-11 18:10:05]

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

[commit-bot: I haz the power, 2016-10-11 18:10:06]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[bshe, 2016-10-11 21:58:38]

The CQ bit was checked by bshe@chromium.org

[bshe, 2016-10-11 21:58:39]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
dtrainor@chromium.org, mthiesse@chromium.org, agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2399533002/#ps100001
(title: "fix android_clang_dbg_receipt error")

[commit-bot: I haz the power, 2016-10-11 21:58:52]

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-10-12 00:42:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-12 00:45:40]

Description was changed from

==========
Pull in vr_shell code for webvr


BUG=389343
==========

to

==========
Pull in vr_shell code for webvr

BUG=389343

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

[commit-bot: I haz the power, 2016-10-12 00:45:41]

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