Disable DateTime, Color, and Select popups when in VR

Disable DateTime, Color, and Select popups when in VR

These popups show as mono on top of stereo content, so can lead to
discomfort.  They will be re-enabled by a subsequent change.

BUG=661243

[billorr, 2017-02-07 21:56:47]

Description was changed from

==========
Disable DateTime, Color, and Select popups when in VR

These popups show as mono on top of stereo content, so can lead to
discomfort.  They will be re-enabled by a subsequent change.

BUG=661243
==========

to

==========
Disable DateTime, Color, and Select popups when in VR

These popups show as mono on top of stereo content, so can lead to
discomfort.  They will be re-enabled by a subsequent change.

BUG=661243
==========

[billorr, 2017-02-07 21:56:47]

billorr@chromium.org changed reviewers:
+ asimjour@chromium.org, mthiesse@chromium.org

[billorr, 2017-02-07 23:08:25]

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

[commit-bot: I haz the power, 2017-02-07 23:09:24]

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-02-08 00:51:51]

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

[commit-bot: I haz the power, 2017-02-08 00:51:52]

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

[billorr, 2017-02-08 01:16:12]

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

[commit-bot: I haz the power, 2017-02-08 01:17:15]

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-02-08 02:45:53]

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

[commit-bot: I haz the power, 2017-02-08 02:45:54]

Dry run: This issue passed the CQ dry run.

[billorr, 2017-02-08 17:50:10]

billorr@chromium.org changed reviewers:
+ jochen@chromium.org

[billorr, 2017-02-08 17:50:11]

jochen@chromium.org: Please review all changes.

[mthiesse, 2017-02-08 18:27:57]

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
components/web_contents_delegate_android/web_contents_delegate_android.cc:63:
NOTIMPLEMENTED();
I don't think we actually want NOTIMPLEMENTED() here... won't that possibly
cause crashes depending on the policy at the time?

We definitely expect this code to be run, so it's not so much unimplemented as
it is disabled.

[mthiesse, 2017-02-08 18:29:13]

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
components/web_contents_delegate_android/web_contents_delegate_android.cc:63:
NOTIMPLEMENTED();
On 2017/02/08 18:27:57, mthiesse wrote:
> I don't think we actually want NOTIMPLEMENTED() here... won't that possibly
> cause crashes depending on the policy at the time?
> 
> We definitely expect this code to be run, so it's not so much unimplemented as
> it is disabled.

Nevermind the crashes point, I see that it either logs or fails to compile. I
still think we don't want to be logging here.

[billorr, 2017-02-08 19:02:46]

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
components/web_contents_delegate_android/web_contents_delegate_android.cc:63:
NOTIMPLEMENTED();
On 2017/02/08 18:29:12, mthiesse wrote:
> On 2017/02/08 18:27:57, mthiesse wrote:
> > I don't think we actually want NOTIMPLEMENTED() here... won't that possibly
> > cause crashes depending on the policy at the time?
> > 
> > We definitely expect this code to be run, so it's not so much unimplemented
as
> > it is disabled.
> 
> Nevermind the crashes point, I see that it either logs or fails to compile. I
> still think we don't want to be logging here.

I was on the fence about adding it.  I figured the extra logging is one more
reminder that we have to fix this, it documents in the code and logs that it
isn't expected to work, and since it isn't called in some loop the logs won't be
flooded with notifications that something isn't implemented.

If you feel strongly, I can remove it, but it won't be harmful.

[mthiesse, 2017-02-08 19:24:46]

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
components/web_contents_delegate_android/web_contents_delegate_android.cc:63:
NOTIMPLEMENTED();
On 2017/02/08 19:02:46, billorr wrote:
> On 2017/02/08 18:29:12, mthiesse wrote:
> > On 2017/02/08 18:27:57, mthiesse wrote:
> > > I don't think we actually want NOTIMPLEMENTED() here... won't that
possibly
> > > cause crashes depending on the policy at the time?
> > > 
> > > We definitely expect this code to be run, so it's not so much
unimplemented
> as
> > > it is disabled.
> > 
> > Nevermind the crashes point, I see that it either logs or fails to compile.
I
> > still think we don't want to be logging here.
> 
> I was on the fence about adding it.  I figured the extra logging is one more
> reminder that we have to fix this, it documents in the code and logs that it
> isn't expected to work, and since it isn't called in some loop the logs won't
be
> flooded with notifications that something isn't implemented.
> 
> If you feel strongly, I can remove it, but it won't be harmful.

It's somewhat harmful, strings are a large contributor to binary size, so
pointless logging is heavily discouraged.

[amp, 2017-02-08 19:31:40]

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
components/web_contents_delegate_android/web_contents_delegate_android.cc:63:
NOTIMPLEMENTED();
On 2017/02/08 19:24:46, mthiesse wrote:
> On 2017/02/08 19:02:46, billorr wrote:
> > On 2017/02/08 18:29:12, mthiesse wrote:
> > > On 2017/02/08 18:27:57, mthiesse wrote:
> > > > I don't think we actually want NOTIMPLEMENTED() here... won't that
> possibly
> > > > cause crashes depending on the policy at the time?
> > > > 
> > > > We definitely expect this code to be run, so it's not so much
> unimplemented
> > as
> > > > it is disabled.
> > > 
> > > Nevermind the crashes point, I see that it either logs or fails to
compile.
> I
> > > still think we don't want to be logging here.
> > 
> > I was on the fence about adding it.  I figured the extra logging is one more
> > reminder that we have to fix this, it documents in the code and logs that it
> > isn't expected to work, and since it isn't called in some loop the logs
won't
> be
> > flooded with notifications that something isn't implemented.
> > 
> > If you feel strongly, I can remove it, but it won't be harmful.
> 
> It's somewhat harmful, strings are a large contributor to binary size, so
> pointless logging is heavily discouraged.

Policy on Android official builds seems to be to strip them out, see:
https://cs.chromium.org/chromium/src/base/logging.h?q=NOTIMPLEMENTED_POLICY&s...

My vote is to keep them in, seems like they would be useful on dev builds and
won't impact the release builds size.

[billorr, 2017-02-08 21:02:05]

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

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

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-02-08 22:13:34]

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

[commit-bot: I haz the power, 2017-02-08 22:13:34]

Dry run: This issue passed the CQ dry run.

[ddorwin, 2017-02-27 06:29:24]

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

[ddorwin, 2017-02-27 06:29:25]

BUG should be 688122.

[jochen (gone - plz use gerrit), 2017-03-06 12:45:56]

I'm waiting for one of the android folks to review the CL first

[mthiesse, 2017-03-06 15:33:07]

lgtm from vr's perspective. I don't think we have any android owners other than
you on this CL jochen.

[jochen (gone - plz use gerrit), 2017-03-06 15:37:33]

i'm not an android owner :)

[mthiesse, 2017-03-06 15:42:55]

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

[mthiesse, 2017-03-06 15:42:56]

+boliu, PTAL

[boliu, 2017-03-06 22:38:10]

side note.. IsInVR should be on WebContents, rather than RenderWidgetHostView.
It makes no sense to what it means for one RenderWidget to be in VR mode and
another in the same webcontents to not be in VR mode

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2683823002/diff/20001/components/web_contents...
components/web_contents_delegate_android/web_contents_delegate_android.cc:61: if
(source->GetRenderWidgetHostView()->IsInVR()) {
instead of this check, make sure content doesn't call this in vr mode

then you don't need to expose IsInVR from content