Support rendering Android Native Pages in VR Shell.

Support rendering Android Native Pages in VR Shell.

Input to these native pages is not yet implemented.

BUG=661609
Review-Url: https://codereview.chromium.org/2683953007
Cr-Commit-Position: refs/heads/master@{#450102}
Committed: https://chromium.googlesource.com/chromium/src/+/7f989aaee547259373cf231cfcf32c23729c154d

[mthiesse, 2017-02-09 22:20:07]

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

[mthiesse, 2017-02-09 22:20:08]

PTAL

[cjgrant, 2017-02-10 15:21:47]

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

[cjgrant, 2017-02-10 15:21:47]

https://codereview.chromium.org/2683953007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(left):

https://codereview.chromium.org/2683953007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:163:
// For now we don't handle native pages. crbug.com/661609.
This comment no longer looks accurate.

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:374: content::NavigationController&
controller = main_contents_->GetController();
For readability, it's odd to have several different "don't have a webcontents"
checks in the first 5 cases (main_contents_, controller, !controller), which are
all derived from main_contents_.  Maybe it'd be better to drop the controller
pointer, put "if (!main_contents_) break" at the start of the first 5 cases, and
just reference the controller as main_contents_->GetController() on-site when
needed.  A thought, anyway.

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:119: ContentFrameWasResized(false /*
unused */);
Is "unused" necessary?

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:134:
html_interface_->SetFullscreen(main_contents_->IsFullscreen());
Should probably create a SetUiState() method that sets UI properties, rather
than have two sets of set calls (here and in OnDomContentsLoaded()).

[mthiesse, 2017-02-10 16:13:40]

https://codereview.chromium.org/2683953007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(left):

https://codereview.chromium.org/2683953007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:163:
// For now we don't handle native pages. crbug.com/661609.
On 2017/02/10 15:21:47, cjgrant wrote:
> This comment no longer looks accurate.

Done.

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:374: content::NavigationController&
controller = main_contents_->GetController();
On 2017/02/10 15:21:47, cjgrant wrote:
> For readability, it's odd to have several different "don't have a webcontents"
> checks in the first 5 cases (main_contents_, controller, !controller), which
are
> all derived from main_contents_.  Maybe it'd be better to drop the controller
> pointer, put "if (!main_contents_) break" at the start of the first 5 cases,
and
> just reference the controller as main_contents_->GetController() on-site when
> needed.  A thought, anyway.

Done.

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:119: ContentFrameWasResized(false /*
unused */);
On 2017/02/10 15:21:47, cjgrant wrote:
> Is "unused" necessary?

Technically no, but I'm pointing out that this value is arbitrary. It's only
there because we're overriding an interface.

https://codereview.chromium.org/2683953007/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:134:
html_interface_->SetFullscreen(main_contents_->IsFullscreen());
On 2017/02/10 15:21:47, cjgrant wrote:
> Should probably create a SetUiState() method that sets UI properties, rather
> than have two sets of set calls (here and in OnDomContentsLoaded()).

Done.

[mthiesse, 2017-02-10 16:17:18]

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

[mthiesse, 2017-02-10 16:17:18]

+enne for the DEPS change to add cc/layers. This should be equivalent to
chrome/browser/android/compositor/compositor_view.h

[enne (OOO), 2017-02-10 17:03:08]

On 2017/02/10 at 16:17:18, mthiesse wrote:
> +enne for the DEPS change to add cc/layers. This should be equivalent to
chrome/browser/android/compositor/compositor_view.h

So vr shell is similar to another browser compositor? Does it do raster at all
or is it like Android's browser compositor?

lgtm, regardless

[bshe, 2017-02-10 20:15:44]

Awesome

https://codereview.chromium.org/2683953007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2683953007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:167:
// For now we don't the sad tab page. crbug.com/661609.
nit: we don't support the sad tab page?

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:118: SetIsInVR(main_contents_,
true);
if (main_contents_) before this line?

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:140: // TODO(mthiesse): Properly
handle native page URLs.
nit: file a bug or reference to existing bug?

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:218: if (metrics_helper_)
does this mean we may miss recording the metrics?

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:365:
Java_VrShellImpl_contentSurfaceCreated(env, j_vr_shell_.obj());
I am not sure I understand correctly here. So contentSurfaceCreated will
indirectly trigger TakeContentSurface. Why not just passing the surface directly
as a parameter of contentSufaceCreated so the TakeContentSurface is no longer
necessary.
Also, is there a reason to not name the java function to contentSurfaceChanged
to match the function name at 361?

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:411: case OMNIBOX_CONTENT:
braces here too? Or remove brances at 414

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:425: return;
Does this mean we can't use the navigate controls if it is a native page? If so,
perhaps create a bug?

[cjgrant, 2017-02-10 20:20:22]

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:411: case OMNIBOX_CONTENT:
On 2017/02/10 20:15:44, bshe wrote:
> braces here too? Or remove brances at 414

The braces on 414 keep "bool paused" in the scope of that case.  What's wrong
with braces on some bigger cases, but not trivially short ones?  If this was a
tiny switch block, consistency would seem more important...  My 2 cents.

[mthiesse, 2017-02-13 15:27:36]

https://codereview.chromium.org/2683953007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2683953007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:167:
// For now we don't the sad tab page. crbug.com/661609.
On 2017/02/10 20:15:44, bshe wrote:
> nit: we don't support the sad tab page?

Done.

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:118: SetIsInVR(main_contents_,
true);
On 2017/02/10 20:15:44, bshe wrote:
> if (main_contents_) before this line? 

No, we just do the check inside SetIsInVR

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:140: // TODO(mthiesse): Properly
handle native page URLs.
On 2017/02/10 20:15:44, bshe wrote:
> nit: file a bug or reference to existing bug?

Done.

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:218: if (metrics_helper_)
On 2017/02/10 20:15:44, bshe wrote:
> does this mean we may miss recording the metrics?

Yes, when on a native page.

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:365:
Java_VrShellImpl_contentSurfaceCreated(env, j_vr_shell_.obj());
On 2017/02/10 20:15:44, bshe wrote:
> I am not sure I understand correctly here. So contentSurfaceCreated will
> indirectly trigger TakeContentSurface. Why not just passing the surface
directly
> as a parameter of contentSufaceCreated so the TakeContentSurface is no longer
> necessary.
> Also, is there a reason to not name the java function to contentSurfaceChanged
> to match the function name at 361?

We can't just pass the surface up as a parameter because we need to know if java
took ownership of the surface. I could return a boolean, but I think it's
cleaner to just have a separate function call for java taking ownership of the
surface.

Note that once java takes ownership of the surface, native can't reclaim it - we
have to create a new one.

I'll rename the function.

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:411: case OMNIBOX_CONTENT:
On 2017/02/10 20:20:22, cjgrant wrote:
> On 2017/02/10 20:15:44, bshe wrote:
> > braces here too? Or remove brances at 414
> 
> The braces on 414 keep "bool paused" in the scope of that case.  What's wrong
> with braces on some bigger cases, but not trivially short ones?  If this was a
> tiny switch block, consistency would seem more important...  My 2 cents.

Leaving braces as-is.

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:425: return;
On 2017/02/10 20:15:44, bshe wrote:
> Does this mean we can't use the navigate controls if it is a native page? If
so,
> perhaps create a bug?

No need for a separate bug I think, this is part of the implementation that
isn't finished.

[mthiesse, 2017-02-13 15:30:46]

On 2017/02/10 17:03:08, enne wrote:
> On 2017/02/10 at 16:17:18, mthiesse wrote:
> > +enne for the DEPS change to add cc/layers. This should be equivalent to
> chrome/browser/android/compositor/compositor_view.h
> 
> So vr shell is similar to another browser compositor? Does it do raster at all
> or is it like Android's browser compositor?
> 
> lgtm, regardless

It's like Android's browser compositor. I don't think it does raster, but I'm
not entirely sure what you mean in this context (I'm not a graphics person :P)

[bshe, 2017-02-13 15:56:11]

lgtm

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2683953007/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:411: case OMNIBOX_CONTENT:
On 2017/02/13 15:27:35, mthiesse wrote:
> On 2017/02/10 20:20:22, cjgrant wrote:
> > On 2017/02/10 20:15:44, bshe wrote:
> > > braces here too? Or remove brances at 414
> > 
> > The braces on 414 keep "bool paused" in the scope of that case.  What's
wrong
> > with braces on some bigger cases, but not trivially short ones?  If this was
a
> > tiny switch block, consistency would seem more important...  My 2 cents.
> 
> Leaving braces as-is.

It is not a blocker. It is a suggestion to keep style consistent for each
"case". Either have braces for all cases or no braces at all. I am not saying
there is anything wrong with braces. But I am not sure I understand the life
scope argument. The code wont be any different after you remove the braces. It
is still out of scope after return.

[mthiesse, 2017-02-13 16:27:12]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-02-13 16:27:13]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2683953007/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2017-02-13 16:28:02]

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-02-13 16:34:23]

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

[commit-bot: I haz the power, 2017-02-13 16:34:24]

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...)

[mthiesse, 2017-02-13 20:40:45]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-02-13 20:40:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2683953007/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-13 20:41:36]

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-02-13 21:45:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487018445312810,
"parent_rev": "e730d23a432b03640f27ce12b739874369990c02", "commit_rev":
"7f989aaee547259373cf231cfcf32c23729c154d"}

[commit-bot: I haz the power, 2017-02-13 21:46:31]

Description was changed from

==========
Support rendering Android Native Pages in VR Shell.

Input to these native pages is not yet implemented.

BUG=661609
==========

to

==========
Support rendering Android Native Pages in VR Shell.

Input to these native pages is not yet implemented.

BUG=661609

Review-Url: https://codereview.chromium.org/2683953007
Cr-Commit-Position: refs/heads/master@{#450102}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f989aaee547259373cf231cfcf3...
==========

[commit-bot: I haz the power, 2017-02-13 21:46:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7f989aaee547259373cf231cfcf3...