Disable Android virtual keyboard when in VR shell

Disable Android virtual keyboard when in VR shell

Disabled the ImeKeyboard and text selection on inputs when in VR.

BUG=671304
Review-Url: https://codereview.chromium.org/2632493005
Cr-Commit-Position: refs/heads/master@{#444390}
Committed: https://chromium.googlesource.com/chromium/src/+/cb3a53fcdc7c60392790a156160a5bd39748afb4

[asimjour1, 2017-01-13 20:27:07]

asimjour@chromium.org changed reviewers:
+ bshe@chromium.org, creis@chromium.org, mthiesse@chromium.org

[asimjour1, 2017-01-13 20:27:08]

PTAL

[mthiesse, 2017-01-13 20:32:47]

vr_shell/ lgtm.

https://codereview.chromium.org/2632493005/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2632493005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:67: // Tells the renderer if in
vr mode.
nit: Tells the renderer if it is in vr mode.

[cjgrant, 2017-01-13 20:44:37]

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

[cjgrant, 2017-01-13 20:44:38]

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.h:332: // Used to
control the appearance of overscroll glow and keyboard.
Should we just drop the comment?  Whenever use of the variable changes, the
comment will become stale.

[Charlie Reis, 2017-01-13 20:59:02]

creis@chromium.org changed reviewers:
+ ekaramad@chromium.org

[Charlie Reis, 2017-01-13 20:59:02]

Generally looks ok, but one question about the SetIsVR documentation and how
it's intended to be used.

Please also get ekaramad's review for the IME changes, to be sure those won't
cause other issues VR mode.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.h:332: // Used to
control the appearance of overscroll glow and keyboard.
On 2017/01/13 20:44:38, cjgrant wrote:
> Should we just drop the comment?  Whenever use of the variable changes, the
> comment will become stale.

Alternatively, you can describe these as examples, so that readers know why this
is useful but doesn't assume it's comprehensive.  Something like:

Used to customize behavior for virtual reality mode, such as the appearance of
overscroll glow and the keyboard.

https://codereview.chromium.org/2632493005/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2632493005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:67: // Tells the renderer if in
vr mode.
On 2017/01/13 20:32:46, mthiesse wrote:
> nit: Tells the renderer if it is in vr mode.

Also, please expand vr to virtual reality (since this is content/public
documentation).

I'm a bit confused by the comment, though, because
RenderWidgetHostViewAndroid::SetIsInVR does not notify the renderer process.  It
only sets a bit locally.

[EhsanK, 2017-01-13 21:55:24]

LGTM overall with some questions which I have mostly left to educate myself. The
only potential issue I see is this: if ContentViewCore should not be notified
about updates, then maybe send an IPC to the renderer to notify it about the
|is_in_vr_| state. RenderWidget could use that to stop sending TextInputState
and SelectionChanged IPCs.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:779: if
(!content_view_core_ || is_in_vr_)
I was wondering if the intention of the CL was to avoid notifying
ContentViewCore altogether, which I guess does more than just not showing the
VK?

If yes, shouldn't we be updating the renderer process(es) on the page to to stop
sending updates (say in RenderWidget::UpdateTextInputState() we just early
return is |is_in_vr_| is true)? WDYT?

If the goal here is just to stop VK from popping up, then wouldn't modifying the
state so that TextInputState.show_ime_if_needed == false do the trick? This
could also happen in RenderWidget::UpdateTextInputState(). In that case this
method should be not be changed I believe.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:782:
content_view_core_->UpdateImeAdapter(
Regarding the second point above: Here I think the UpdateImeAdapter is doing
more than ImeAdapter. The users of this seem to be ImeAdapter,
JoystickScrollProvider, Tab.java for full screen. I am not sure if any of these
are being used in VR mode.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:953: const
gfx::Range& range) {
Similar to the OnUpdateTextInputStateCalled() above:
The call to base class's SelectionChange will update |selection_text_offset_|,
|selection_range_|, and |selection_text_|. However, AFAIK we never use those
values on Android and in fact, it seems like we should at some point get rid of
the call to the base class code (that is a separate problem of course). Sicne we
do not update the |content_view_core_| at all, then maybe we should not receive
updates to begin with? WDYT?

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:975:
content_view_core_->OnSelectionChanged(utf8_selection);
I am not familiar with Java side, but, looking at the code it seems like the
call here would notify the SelectionPopup and eventually the
ContextualSearchClient. Is this part of the change in this CL not to show the
popup in VR mode? In that case maybe the CL description could take note of it
(just a suggestion).

[asimjour, 2017-01-16 19:28:06]

Description was changed from

==========
Disable Android virtual keyboard when in VR shell

Disabled the ImeKeyboard when in VR.

BUG=671304
==========

to

==========
Disable Android virtual keyboard when in VR shell

Disabled the ImeKeyboard and text selection on inputs when in VR.

BUG=671304
==========

[asimjour, 2017-01-16 19:28:40]

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

[asimjour, 2017-01-16 19:28:41]

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:779: if
(!content_view_core_ || is_in_vr_)
On 2017/01/13 21:55:24, EhsanK wrote:
> I was wondering if the intention of the CL was to avoid notifying
> ContentViewCore altogether, which I guess does more than just not showing the
> VK?
> 
> If yes, shouldn't we be updating the renderer process(es) on the page to to
stop
> sending updates (say in RenderWidget::UpdateTextInputState() we just early
> return is |is_in_vr_| is true)? WDYT?
> 
> If the goal here is just to stop VK from popping up, then wouldn't modifying
the
> state so that TextInputState.show_ime_if_needed == false do the trick? This
> could also happen in RenderWidget::UpdateTextInputState(). In that case this
> method should be not be changed I believe.

This is a temporary solution. For now I want to stop all the popups (keyboard,
text selection, paste popup, ...), after that I should add VR support for all
these popups one by one, and remove the disabling from here.
we can have three different temporary solutions
1- return when it's needed (the current solution)
2- send an IPC to set VR mode in RendererWidget or RendererView.
3- update the consumer of the events so that they don't show anything.
Based on our offline discussion and since this is a temporary solution, I would
rather to stick to the first solution.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:782:
content_view_core_->UpdateImeAdapter(
On 2017/01/13 21:55:24, EhsanK wrote:
> Regarding the second point above: Here I think the UpdateImeAdapter is doing
> more than ImeAdapter. The users of this seem to be ImeAdapter,
> JoystickScrollProvider, Tab.java for full screen. I am not sure if any of
these
> are being used in VR mode.

The UpdateImeAdapter is doing more, but other changes are consequence of showing
the keyboard. So, it is safe to stop them all from here.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:975:
content_view_core_->OnSelectionChanged(utf8_selection);
On 2017/01/13 21:55:23, EhsanK wrote:
> I am not familiar with Java side, but, looking at the code it seems like the
> call here would notify the SelectionPopup and eventually the
> ContextualSearchClient. Is this part of the change in this CL not to show the
> popup in VR mode? In that case maybe the CL description could take note of it
> (just a suggestion).

the CL description is updated.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.h:332: // Used to
control the appearance of overscroll glow and keyboard.
On 2017/01/13 20:59:02, Charlie Reis wrote:
> On 2017/01/13 20:44:38, cjgrant wrote:
> > Should we just drop the comment?  Whenever use of the variable changes, the
> > comment will become stale.
> 
> Alternatively, you can describe these as examples, so that readers know why
this
> is useful but doesn't assume it's comprehensive.  Something like:
> 
> Used to customize behavior for virtual reality mode, such as the appearance of
> overscroll glow and the keyboard.

Done.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.h:332: // Used to
control the appearance of overscroll glow and keyboard.
On 2017/01/13 20:44:38, cjgrant wrote:
> Should we just drop the comment?  Whenever use of the variable changes, the
> comment will become stale.

Comment is updated.

https://codereview.chromium.org/2632493005/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2632493005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:67: // Tells the renderer if in
vr mode.
On 2017/01/13 20:32:46, mthiesse wrote:
> nit: Tells the renderer if it is in vr mode.

Done.

https://codereview.chromium.org/2632493005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:67: // Tells the renderer if in
vr mode.
On 2017/01/13 20:59:02, Charlie Reis wrote:
> On 2017/01/13 20:32:46, mthiesse wrote:
> > nit: Tells the renderer if it is in vr mode.
> 
> Also, please expand vr to virtual reality (since this is content/public
> documentation).
> 
> I'm a bit confused by the comment, though, because
> RenderWidgetHostViewAndroid::SetIsInVR does not notify the renderer process. 
It
> only sets a bit locally.

Done.

[EhsanK, 2017-01-16 20:23:11]

Thanks. LGTM for either or the proposed solutions. As a trade off between coding
and performance, maybe leave it as is (we should return the unused calls as soon
as possible rather than down the stack in the Java side.)

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:779: if
(!content_view_core_ || is_in_vr_)
On 2017/01/16 19:28:41, asimjour wrote:
> On 2017/01/13 21:55:24, EhsanK wrote:
> > I was wondering if the intention of the CL was to avoid notifying
> > ContentViewCore altogether, which I guess does more than just not showing
the
> > VK?
> > 
> > If yes, shouldn't we be updating the renderer process(es) on the page to to
> stop
> > sending updates (say in RenderWidget::UpdateTextInputState() we just early
> > return is |is_in_vr_| is true)? WDYT?
> > 
> > If the goal here is just to stop VK from popping up, then wouldn't modifying
> the
> > state so that TextInputState.show_ime_if_needed == false do the trick? This
> > could also happen in RenderWidget::UpdateTextInputState(). In that case this
> > method should be not be changed I believe.
> 
> This is a temporary solution. For now I want to stop all the popups (keyboard,
> text selection, paste popup, ...), after that I should add VR support for all
> these popups one by one, and remove the disabling from here.
> we can have three different temporary solutions
> 1- return when it's needed (the current solution)
> 2- send an IPC to set VR mode in RendererWidget or RendererView.
> 3- update the consumer of the events so that they don't show anything.
> Based on our offline discussion and since this is a temporary solution, I
would
> rather to stick to the first solution.
sgtm. The benefit of doing (2) is not sending unused IPC on Android. But since
this situation is temporary, it might not be worth doing because it will be some
code which will be removed soon. Also there seems to be a bit more to sending
the IPC since Selection updates come from RenderFrame and TextInputState from
RenderWidget. Also, you would have to send a "Page" message through
WebContentsImpl and on the renderer side, handle it in RenderViewImpl to notify
all local root RenderFrameImpls and RenderWidgets in that process.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:782:
content_view_core_->UpdateImeAdapter(
On 2017/01/16 19:28:41, asimjour wrote:
> On 2017/01/13 21:55:24, EhsanK wrote:
> > Regarding the second point above: Here I think the UpdateImeAdapter is doing
> > more than ImeAdapter. The users of this seem to be ImeAdapter,
> > JoystickScrollProvider, Tab.java for full screen. I am not sure if any of
> these
> > are being used in VR mode.
> 
> The UpdateImeAdapter is doing more, but other changes are consequence of
showing
> the keyboard. So, it is safe to stop them all from here.

Acknowledged.

https://codereview.chromium.org/2632493005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:975:
content_view_core_->OnSelectionChanged(utf8_selection);
On 2017/01/16 19:28:40, asimjour wrote:
> On 2017/01/13 21:55:23, EhsanK wrote:
> > I am not familiar with Java side, but, looking at the code it seems like the
> > call here would notify the SelectionPopup and eventually the
> > ContextualSearchClient. Is this part of the change in this CL not to show
the
> > popup in VR mode? In that case maybe the CL description could take note of
it
> > (just a suggestion).
> 
> the CL description is updated.

Acknowledged.

[bshe, 2017-01-17 12:26:16]

lgtm

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

https://codereview.chromium.org/2632493005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:220: // exit vr session
nit: not related to your change, but do you mind remove the comment above as a
minor clean up.

[Charlie Reis, 2017-01-17 17:51:51]

Thanks, content/ LGTM.

[asimjour1, 2017-01-18 15:22:33]

The CQ bit was checked by asimjour@chromium.org

[asimjour1, 2017-01-18 15:22:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, ekaramad@chromium.org, creis@chromium.org,
bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2632493005/#ps40001
(title: "minor clean up")

[commit-bot: I haz the power, 2017-01-18 15:22:59]

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-18 16:42:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484752953188340,
"parent_rev": "d7c441b2bb058b69724261ade0dd36cbbe6f431f", "commit_rev":
"cb3a53fcdc7c60392790a156160a5bd39748afb4"}

[commit-bot: I haz the power, 2017-01-18 16:43:22]

Description was changed from

==========
Disable Android virtual keyboard when in VR shell

Disabled the ImeKeyboard and text selection on inputs when in VR.

BUG=671304
==========

to

==========
Disable Android virtual keyboard when in VR shell

Disabled the ImeKeyboard and text selection on inputs when in VR.

BUG=671304

Review-Url: https://codereview.chromium.org/2632493005
Cr-Commit-Position: refs/heads/master@{#444390}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb3a53fcdc7c60392790a156160a...
==========

[commit-bot: I haz the power, 2017-01-18 16:43:24]

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