[VrShell] Mask system ui flags before checking.

[VrShell] Mask system ui flags before checking.

This fixes an issue where extraneous flags were being set on some
devices and causing the flag check to fail.

BUG=719013
Review-Url: https://codereview.chromium.org/2881403003
Cr-Commit-Position: refs/heads/master@{#472257}
Committed: https://chromium.googlesource.com/chromium/src/+/edc6da3999ae141bef700d80b69323849e2290de

[amp, 2017-05-16 21:37:35]

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

[billorr1, 2017-05-16 21:50:05]

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

[billorr1, 2017-05-16 21:50:05]

LGTM

https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:537:
return (flags & VR_SYSTEM_UI_FLAGS) == VR_SYSTEM_UI_FLAGS
Are there any flags we want to ensure are not set?  There are only two that we
don't have set currently (*LOW_PROFILE and *HIDDEN), not sure if those could
interact with other flags in a negative way.

[mthiesse, 2017-05-16 21:56:14]

https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:537:
return (flags & VR_SYSTEM_UI_FLAGS) == VR_SYSTEM_UI_FLAGS
On 2017/05/16 21:50:05, billorr1 wrote:
> Are there any flags we want to ensure are not set?  There are only two that we
> don't have set currently (*LOW_PROFILE and *HIDDEN), not sure if those could
> interact with other flags in a negative way.

We probably want to make sure that we don't have SYSTEM_UI_FLAG_VISIBLE.

[chromium-reviews, 2017-05-16 21:59:55]

That has a constant value of 0, so we should be good.

On Tue, May 16, 2017 at 2:56 PM, <mthiesse@chromium.org> wrote:

>
> https://codereview.chromium.org/2881403003/diff/1/chrome/
> android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
> File
> chrome/android/java/src/org/chromium/chrome/browser/vr_
> shell/VrShellDelegate.java
> (right):
>
> https://codereview.chromium.org/2881403003/diff/1/chrome/
> android/java/src/org/chromium/chrome/browser/vr_shell/
> VrShellDelegate.java#newcode537
> chrome/android/java/src/org/chromium/chrome/browser/vr_
> shell/VrShellDelegate.java:537:
> return (flags & VR_SYSTEM_UI_FLAGS) == VR_SYSTEM_UI_FLAGS
> On 2017/05/16 21:50:05, billorr1 wrote:
> > Are there any flags we want to ensure are not set? There are only two
> that we
> > don't have set currently (*LOW_PROFILE and *HIDDEN), not sure if those
> could
> > interact with other flags in a negative way.
>
> We probably want to make sure that we don't have SYSTEM_UI_FLAG_VISIBLE.
>
> https://codereview.chromium.org/2881403003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mthiesse, 2017-05-16 22:04:05]

On 2017/05/16 21:59:55, chromium-reviews wrote:
> That has a constant value of 0, so we should be good.
> 

hah weird. Good point.

lgtm

[amp, 2017-05-16 22:14:36]

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

[commit-bot: I haz the power, 2017-05-16 22:16:49]

Dry run: CQ is trying da patch.

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

[amp, 2017-05-16 22:19:54]

https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:537:
return (flags & VR_SYSTEM_UI_FLAGS) == VR_SYSTEM_UI_FLAGS
On 2017/05/16 21:50:05, billorr1 wrote:
> Are there any flags we want to ensure are not set?  There are only two that we
> don't have set currently (*LOW_PROFILE and *HIDDEN), not sure if those could
> interact with other flags in a negative way.

I'm not sure.  It's possible we may have to adjust these in future SDK
revisions, but these seem to work for now (albeit I only tested on s8 and
pixel).

I'll have the testers try out on more varying devices to make sure nothing odd
pops up when this goes in.

Anyway, I understand the concern, as according to the spec
https://developer.android.com/reference/android/view/View.html#getSystemUiVis...
getSystemUiVisibility is only supposed to return the values that you set.  So
really we shouldn't need to mask at all.

I'm open to other suggestions as to better mitigate this, any ideas?

[billorr1, 2017-05-16 22:39:51]

On 2017/05/16 22:19:54, amp wrote:
>
https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
> (right):
> 
>
https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:537:
> return (flags & VR_SYSTEM_UI_FLAGS) == VR_SYSTEM_UI_FLAGS
> On 2017/05/16 21:50:05, billorr1 wrote:
> > Are there any flags we want to ensure are not set?  There are only two that
we
> > don't have set currently (*LOW_PROFILE and *HIDDEN), not sure if those could
> > interact with other flags in a negative way.
> 
> I'm not sure.  It's possible we may have to adjust these in future SDK
> revisions, but these seem to work for now (albeit I only tested on s8 and
> pixel).
> 
> I'll have the testers try out on more varying devices to make sure nothing odd
> pops up when this goes in.
> 
> Anyway, I understand the concern, as according to the spec
>
https://developer.android.com/reference/android/view/View.html#getSystemUiVis...
> getSystemUiVisibility is only supposed to return the values that you set.  So
> really we shouldn't need to mask at all.
> 
> I'm open to other suggestions as to better mitigate this, any ideas?

I think its fine as-is, but you could have another constant for the union of all
known flags, and do "if ((flags & ALL_KNOWN_FLAGS) == VR_SYSTEM_UI_FLAGS)...". 
Potentially we could also add some kind of logging or debug break for when a
flag we don't expect is set so we know to update this code as new flags come
out.

[amp, 2017-05-16 23:02:39]

On 2017/05/16 22:39:51, billorr1 wrote:
> On 2017/05/16 22:19:54, amp wrote:
> >
>
https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2881403003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:537:
> > return (flags & VR_SYSTEM_UI_FLAGS) == VR_SYSTEM_UI_FLAGS
> > On 2017/05/16 21:50:05, billorr1 wrote:
> > > Are there any flags we want to ensure are not set?  There are only two
that
> we
> > > don't have set currently (*LOW_PROFILE and *HIDDEN), not sure if those
could
> > > interact with other flags in a negative way.
> > 
> > I'm not sure.  It's possible we may have to adjust these in future SDK
> > revisions, but these seem to work for now (albeit I only tested on s8 and
> > pixel).
> > 
> > I'll have the testers try out on more varying devices to make sure nothing
odd
> > pops up when this goes in.
> > 
> > Anyway, I understand the concern, as according to the spec
> >
>
https://developer.android.com/reference/android/view/View.html#getSystemUiVis...
> > getSystemUiVisibility is only supposed to return the values that you set. 
So
> > really we shouldn't need to mask at all.
> > 
> > I'm open to other suggestions as to better mitigate this, any ideas?
> 
> I think its fine as-is, but you could have another constant for the union of
all
> known flags, and do "if ((flags & ALL_KNOWN_FLAGS) == VR_SYSTEM_UI_FLAGS)...".

> Potentially we could also add some kind of logging or debug break for when a
> flag we don't expect is set so we know to update this code as new flags come
> out.

In the interest of keeping the change as small as possible for easier merging
I'm going to keep it to this one line for now.  Future refactoring of this code
(ala new architecture etc) should take into account the need to be more robust
in the face of system configurations outside of what we test on.

[amp, 2017-05-16 23:03:32]

The CQ bit was unchecked by amp@chromium.org

[amp, 2017-05-16 23:03:37]

The CQ bit was checked by amp@chromium.org

[commit-bot: I haz the power, 2017-05-16 23:05:42]

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-05-17 00:08:27]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1494975817392410, "parent_rev":
"3948d50a08e75cd127eea4ae47267adeb1d945bf", "commit_rev":
"edc6da3999ae141bef700d80b69323849e2290de"}

[commit-bot: I haz the power, 2017-05-17 00:08:38]

Description was changed from

==========
[VrShell] Mask system ui flags before checking.

This fixes an issue where extraneous flags were being set on some
devices and causing the flag check to fail.

BUG=719013
==========

to

==========
[VrShell] Mask system ui flags before checking.

This fixes an issue where extraneous flags were being set on some
devices and causing the flag check to fail.

BUG=719013

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

[commit-bot: I haz the power, 2017-05-17 00:08:39]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/edc6da3999ae141bef700d80b693...