Show an infobar prompting the user to enter feedback when they exit VR

Show an infobar prompting the user to enter feedback when they exit VR

Follow-up work
- Trigger the right Feedback activity
- Control frequency of feedback via Finch

BUG=706438
Review-Url: https://codereview.chromium.org/2887383002
Cr-Commit-Position: refs/heads/master@{#473705}
Committed: https://chromium.googlesource.com/chromium/src/+/bce55d7b55d0b8da8b65315b255e17ba36bef5c8

[ymalik, 2017-05-18 17:13:14]

ymalik@chromium.org changed reviewers:
+ bsheedy@chromium.org, mthiesse@chromium.org

[ymalik, 2017-05-18 17:13:14]

PTAL :)

mthiesse for infobar logic
bsheedy for test

[amp, 2017-05-18 17:32:41]

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

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFeedbackStatus.java:21:
ContextUtils.getAppSharedPreferences()
Does this result in a syncable preferance, or only for this profile?

In other words, if the user is signed in with sync enabled and they use a
different device, will the feedback opt out still work, or will they need to opt
out for each device/profile?

[mthiesse, 2017-05-18 17:44:41]

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

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFeedbackStatus.java:31:
return ContextUtils.getAppSharedPreferences().getBoolean(VR_FEEDBACK_OPT_OUT,
false);
I think at least on pre-N devices, this can cause strictmode violations...
Should probably check to make sure this doesn't.

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

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:651:
}
Should add an else here and set mVrBrowserUsed = true

[bsheedy, 2017-05-18 17:49:27]

Tests LGTM, but added a few notes.

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrFeedbackTest.java
(right):

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrFeedbackTest.java:27:
import org.chromium.chrome.test.util.InfoBarUtil;
Didn't know this was a thing. Handy.

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrFeedbackTest.java:57:
public int loadUrlAndWait(String url, long secondsToWait)
Depending on whether https://codereview.chromium.org/2883273006/ lands before
this or not, this will be redundant. You should be able to use
VrTestRule.loadUrlAndAwaitInitialization once that's in.

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
(right):

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:153:
// TODO(ymalik): This will return true if any infobar is present. Is it
This should be currently possible, although a bit messy, by checking for various
child views and their content.

For example, the upgrade/install InfoBar has a child
"TextViewWithClickableSpans" with ID "info_bar_message" that has text unique to
that type of InfoBar (whatever is in either
R.string.vr_services_check_infobar_install_text or
vr_services_check_infobar_update_text depending on whether it's an install or
update InfoBar).

I imagine that the Feedback InfoBar also has an info_bar_message view with the
text set to whatever is in R.string.vr_shell_feedback_infobar_description.

Alternatively, the InfoBarLayout found this way *should* be the same one that's
returned by InfoBar.getView(), so if we get the list of InfoBars for the current
tab and find the one with the matching view, we can check that the identifier
from InfoBar.getInfoBarIdentifier() is the one we expect, e.g.
InfoBarIdentifier.VR_FEEDBACK_INFOBAR_ANDROID.

[ymalik, 2017-05-18 19:15:18]

mthiesse, bsheedy, PTAL :)

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

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFeedbackStatus.java:21:
ContextUtils.getAppSharedPreferences()
On 2017/05/18 17:32:40, amp wrote:
> Does this result in a syncable preferance, or only for this profile?
> 
> In other words, if the user is signed in with sync enabled and they use a
> different device, will the feedback opt out still work, or will they need to
opt
> out for each device/profile?

No, this is all local do the device and the version of Chrome.

I think this is desired because one may want to have feedback enabled on Dev for
example, and not Stable. They may also see device specific issues and would like
to report those?

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFeedbackStatus.java:31:
return ContextUtils.getAppSharedPreferences().getBoolean(VR_FEEDBACK_OPT_OUT,
false);
On 2017/05/18 17:44:41, mthiesse wrote:
> I think at least on pre-N devices, this can cause strictmode violations...
> Should probably check to make sure this doesn't.

The tests seem to be passing and I don't see anything in logcat. I confirmed
with tedchoc@ that we're okay here.

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

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:651:
}
On 2017/05/18 17:44:41, mthiesse wrote:
> Should add an else here and set mVrBrowserUsed = true

Totally. Added a test for it.

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrFeedbackTest.java
(right):

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrFeedbackTest.java:27:
import org.chromium.chrome.test.util.InfoBarUtil;
On 2017/05/18 17:49:26, bsheedy wrote:
> Didn't know this was a thing. Handy.

Acknowledged.

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrFeedbackTest.java:57:
public int loadUrlAndWait(String url, long secondsToWait)
On 2017/05/18 17:49:27, bsheedy wrote:
> Depending on whether https://codereview.chromium.org/2883273006/ lands before
> this or not, this will be redundant. You should be able to use
> VrTestRule.loadUrlAndAwaitInitialization once that's in.

Acknowledged.

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
(right):

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:153:
// TODO(ymalik): This will return true if any infobar is present. Is it
On 2017/05/18 17:49:27, bsheedy wrote:
> This should be currently possible, although a bit messy, by checking for
various
> child views and their content.
> 
> For example, the upgrade/install InfoBar has a child
> "TextViewWithClickableSpans" with ID "info_bar_message" that has text unique
to
> that type of InfoBar (whatever is in either
> R.string.vr_services_check_infobar_install_text or
> vr_services_check_infobar_update_text depending on whether it's an install or
> update InfoBar).
> 
> I imagine that the Feedback InfoBar also has an info_bar_message view with the
> text set to whatever is in R.string.vr_shell_feedback_infobar_description.
> 
> Alternatively, the InfoBarLayout found this way *should* be the same one
that's
> returned by InfoBar.getView(), so if we get the list of InfoBars for the
current
> tab and find the one with the matching view, we can check that the identifier
> from InfoBar.getInfoBarIdentifier() is the one we expect, e.g.
> InfoBarIdentifier.VR_FEEDBACK_INFOBAR_ANDROID.

Ack. I think the string approach won't because they're private fields. I like
your alternate approach. I'll do it in a different CL.

[mthiesse, 2017-05-18 19:47:45]

vr_shell/ lgtm

[bsheedy, 2017-05-18 20:18:39]

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
(right):

https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:153:
// TODO(ymalik): This will return true if any infobar is present. Is it
On 2017/05/18 19:15:18, ymalik wrote:
> On 2017/05/18 17:49:27, bsheedy wrote:
> > This should be currently possible, although a bit messy, by checking for
> various
> > child views and their content.
> > 
> > For example, the upgrade/install InfoBar has a child
> > "TextViewWithClickableSpans" with ID "info_bar_message" that has text unique
> to
> > that type of InfoBar (whatever is in either
> > R.string.vr_services_check_infobar_install_text or
> > vr_services_check_infobar_update_text depending on whether it's an install
or
> > update InfoBar).
> > 
> > I imagine that the Feedback InfoBar also has an info_bar_message view with
the
> > text set to whatever is in R.string.vr_shell_feedback_infobar_description.
> > 
> > Alternatively, the InfoBarLayout found this way *should* be the same one
> that's
> > returned by InfoBar.getView(), so if we get the list of InfoBars for the
> current
> > tab and find the one with the matching view, we can check that the
identifier
> > from InfoBar.getInfoBarIdentifier() is the one we expect, e.g.
> > InfoBarIdentifier.VR_FEEDBACK_INFOBAR_ANDROID.
> 
> Ack. I think the string approach won't because they're private fields. I like
> your alternate approach. I'll do it in a different CL.
> 

We already do the string comparison approach for the install/update InfoBar
tests, see
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu....
Unless I'm missing something, that should work for other types of InfoBars.

[ymalik, 2017-05-18 22:33:55]

On 2017/05/18 20:18:39, bsheedy wrote:
>
https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
> File
> chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
> (right):
> 
>
https://codereview.chromium.org/2887383002/diff/20001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:153:
> // TODO(ymalik): This will return true if any infobar is present. Is it
> On 2017/05/18 19:15:18, ymalik wrote:
> > On 2017/05/18 17:49:27, bsheedy wrote:
> > > This should be currently possible, although a bit messy, by checking for
> > various
> > > child views and their content.
> > > 
> > > For example, the upgrade/install InfoBar has a child
> > > "TextViewWithClickableSpans" with ID "info_bar_message" that has text
unique
> > to
> > > that type of InfoBar (whatever is in either
> > > R.string.vr_services_check_infobar_install_text or
> > > vr_services_check_infobar_update_text depending on whether it's an install
> or
> > > update InfoBar).
> > > 
> > > I imagine that the Feedback InfoBar also has an info_bar_message view with
> the
> > > text set to whatever is in R.string.vr_shell_feedback_infobar_description.
> > > 
> > > Alternatively, the InfoBarLayout found this way *should* be the same one
> > that's
> > > returned by InfoBar.getView(), so if we get the list of InfoBars for the
> > current
> > > tab and find the one with the matching view, we can check that the
> identifier
> > > from InfoBar.getInfoBarIdentifier() is the one we expect, e.g.
> > > InfoBarIdentifier.VR_FEEDBACK_INFOBAR_ANDROID.
> > 
> > Ack. I think the string approach won't because they're private fields. I
like
> > your alternate approach. I'll do it in a different CL.
> > 
> 
> We already do the string comparison approach for the install/update InfoBar
> tests, see
>
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu....
> Unless I'm missing something, that should work for other types of InfoBars.

Ah yeah I misunderstood your suggestion. I'll consider one of the two solutions
for future CLs.

[ymalik, 2017-05-18 22:37:48]

Description was changed from

==========
Show an inforbar prompting the user to enter feedback when they exit VR

Follow-up work
- Trigger the right Feedback activity
- Control frequency of feedback via Finch

BUG=706438
==========

to

==========
Show an inforbar prompting the user to enter feedback when they exit VR

Follow-up work
- Trigger the right Feedback activity
- Control frequency of feedback via Finch

BUG=706438
==========

[ymalik, 2017-05-18 22:37:48]

ymalik@chromium.org changed reviewers:
+ pkasting@chromium.org

[ymalik, 2017-05-18 22:38:37]

+pkasting for components infobar_delegate.h

[ymalik, 2017-05-18 22:41:26]

ymalik@chromium.org changed reviewers:
+ tedchoc@chromium.org

[ymalik, 2017-05-18 22:41:26]

+tedchoc for BUILD.gn

[Peter Kasting, 2017-05-19 02:12:09]

An infobar feels like a questionable UI surface for this.  Not only are we
trying not to add infobars in general, they're normally something specific to
the underlying content of the current tab (and only shown on the current tab). 
Is this that?  It feels a bit more like a browser notification not specific to
the current tab, which I'd think we'd want to badge the app menu button for or
something.  Did you get UX direction to do this?

Also, your CL title has a typo.

[ymalik, 2017-05-19 02:28:40]

On 2017/05/19 02:12:09, Peter Kasting wrote:
> An infobar feels like a questionable UI surface for this.  Not only are we
> trying not to add infobars in general, they're normally something specific to
> the underlying content of the current tab (and only shown on the current tab).

> Is this that?  It feels a bit more like a browser notification not specific to
> the current tab, which I'd think we'd want to badge the app menu button for or
> something.  Did you get UX direction to do this?
> 
> Also, your CL title has a typo.

Yes, joshcarpenter@, our UX designer ran this by the Chrome UX team for initial
validation (this is confirmed here:
https://codereview.chromium.org/2885293002/). I agree that infobars in general
make more sense when they're related to the underlying content. In this case,
they will show up every time the user exits VR browsing (into the same web page,
but now in standard Chrome instead of Chrome VR), so it still seems page
appropriate (and related to the current tab). 

We discussed other ways for the prompt, such as dialogs, but that felt more
intrusive than using an Infobar.

[Peter Kasting, 2017-05-19 05:59:07]

Thanks for sharing that detail.  I had read the bug report looking for some of
this stuff, but it wasn't documented much there, so it wasn't clear how much of
this had occurred.  An update on the bug might be useful in case others look for
the same information.

LGTM

[ymalik, 2017-05-19 14:15:39]

Description was changed from

==========
Show an inforbar prompting the user to enter feedback when they exit VR

Follow-up work
- Trigger the right Feedback activity
- Control frequency of feedback via Finch

BUG=706438
==========

to

==========
Show an infobar prompting the user to enter feedback when they exit VR

Follow-up work
- Trigger the right Feedback activity
- Control frequency of feedback via Finch

BUG=706438
==========

[ymalik, 2017-05-19 14:22:10]

On 2017/05/19 05:59:07, Peter Kasting wrote:
> Thanks for sharing that detail.  I had read the bug report looking for some of
> this stuff, but it wasn't documented much there, so it wasn't clear how much
of
> this had occurred.  An update on the bug might be useful in case others look
for
> the same information.
> 
> LGTM

yes totally, updated the bug.

And thanks for pointing out the typo in the title :)

[ymalik, 2017-05-19 18:08:57]

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

[ymalik, 2017-05-19 18:09:01]

The CQ bit was unchecked by ymalik@chromium.org

[ymalik, 2017-05-19 18:09:12]

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

[commit-bot: I haz the power, 2017-05-19 18:09:56]

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-05-19 18:13:26]

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

[commit-bot: I haz the power, 2017-05-19 18:13:27]

Dry run: 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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ymalik, 2017-05-19 20:47:43]

@mthiesse, PTAL after resolving merge conflicts.

[ymalik, 2017-05-19 20:47:44]

@mthiesse, PTAL after resolving merge conflicts.

[Ted C, 2017-05-19 21:26:58]

lgtm

[mthiesse, 2017-05-19 22:25:18]

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

https://codereview.chromium.org/2887383002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:692:
// shutdownVr is called with stayingInChrome because other exit cases are
handled
This comment doesn't really make sense here, it's not clear what other exit
cases you're referring to. I'd get rid of the comment, and just annotate the
shutdownVr calls so it's more readable as to what each boolean means.

shutdownVr(true /* setVrMode */, false /* canReenter */, true /* stayingInChrome
*/);

You can do this throughout if you agree that it helps readability.

Also, you mentioned setVrMode being confusing - feel free to just rename it
while doing this.

[ymalik, 2017-05-22 17:21:20]

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

https://codereview.chromium.org/2887383002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:692:
// shutdownVr is called with stayingInChrome because other exit cases are
handled
On 2017/05/19 22:25:18, mthiesse wrote:
> This comment doesn't really make sense here, it's not clear what other exit
> cases you're referring to. I'd get rid of the comment, and just annotate the
> shutdownVr calls so it's more readable as to what each boolean means.
> 
> shutdownVr(true /* setVrMode */, false /* canReenter */, true /*
stayingInChrome
> */);
> 
> You can do this throughout if you agree that it helps readability.
> 
> Also, you mentioned setVrMode being confusing - feel free to just rename it
> while doing this.

Done. I didn't do this earlier because adding the /**/ makes the format weird
when you're viewing this file in codereview. I agree that this is better though.

Renamed setVrMode.

[ymalik, 2017-05-22 17:21:40]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-05-22 17:21:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
bsheedy@chromium.org, mthiesse@chromium.org, pkasting@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2887383002/#ps80001
(title: "add comments")

[commit-bot: I haz the power, 2017-05-22 17:22:13]

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-22 20:02:59]

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

[commit-bot: I haz the power, 2017-05-22 20:03:01]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ymalik, 2017-05-22 20:31:39]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2017-05-22 20:32:30]

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-22 21:36:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495485099417300,
"parent_rev": "e2aed4aa8b104427ef7568065d19606d8e0dc16e", "commit_rev":
"bce55d7b55d0b8da8b65315b255e17ba36bef5c8"}

[commit-bot: I haz the power, 2017-05-22 21:36:47]

Description was changed from

==========
Show an infobar prompting the user to enter feedback when they exit VR

Follow-up work
- Trigger the right Feedback activity
- Control frequency of feedback via Finch

BUG=706438
==========

to

==========
Show an infobar prompting the user to enter feedback when they exit VR

Follow-up work
- Trigger the right Feedback activity
- Control frequency of feedback via Finch

BUG=706438

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

[commit-bot: I haz the power, 2017-05-22 21:36:48]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bce55d7b55d0b8da8b65315b255e...

[mikecase (-- gone --), 2017-05-22 22:49:06]

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

[mikecase (-- gone --), 2017-05-22 22:49:06]

Seems to be causing compile failures on Android x64 Builder (dbg)

  symbol:   variable VrFeedbackStatus
  location: class VrShellDelegate
../../chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:976:
error: cannot find symbol
        VrFeedbackStatus.setUserExitedAndEntered2DCount((exitCount + 1) %
mFeedbackFrequency);

https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20x6...

[mikecase (-- gone --), 2017-05-22 23:01:23]

Im getting an error when trying to revert

Failed to load resource: the server responded with a status of 500 ()
/api/2887383002/80001/revert

Would like to revert this to green up the bots :/

[bsheedy, 2017-05-22 23:10:50]

On 2017/05/22 23:01:23, mikecase wrote:
> Im getting an error when trying to revert
> 
> Failed to load resource: the server responded with a status of 500 ()
> /api/2887383002/80001/revert
> 
> Would like to revert this to green up the bots :/

I have no idea about the failure to revert, but the bot failure is being caused
by VrShellDelegate (which is included as part of chrome_java_sources) now
depending on VrFeedbackStatus (which is included as part of
chrome_vr_java_sources). chrome_vr_java_sources is only added if enable_vr is
true, which is always false on x86/x64 builds.

[ymalik, 2017-05-22 23:15:54]

On 2017/05/22 23:10:50, bsheedy wrote:
> On 2017/05/22 23:01:23, mikecase wrote:
> > Im getting an error when trying to revert
> > 
> > Failed to load resource: the server responded with a status of 500 ()
> > /api/2887383002/80001/revert
> > 
> > Would like to revert this to green up the bots :/
> 
> I have no idea about the failure to revert, but the bot failure is being
caused
> by VrShellDelegate (which is included as part of chrome_java_sources) now
> depending on VrFeedbackStatus (which is included as part of
> chrome_vr_java_sources). chrome_vr_java_sources is only added if enable_vr is
> true, which is always false on x86/x64 builds.

That makes sense. I'll write a CL that adds a VrFeedbackStatusImpl which will
build with chrome_vr_java_sources and VrFeedbackStatus which will be added to
chrome_java_sources to keep the bots happy. Does that make sense bsheedy@?

[bsheedy, 2017-05-22 23:19:30]

On 2017/05/22 23:15:54, ymalik wrote:
> On 2017/05/22 23:10:50, bsheedy wrote:
> > On 2017/05/22 23:01:23, mikecase wrote:
> > > Im getting an error when trying to revert
> > > 
> > > Failed to load resource: the server responded with a status of 500 ()
> > > /api/2887383002/80001/revert
> > > 
> > > Would like to revert this to green up the bots :/
> > 
> > I have no idea about the failure to revert, but the bot failure is being
> caused
> > by VrShellDelegate (which is included as part of chrome_java_sources) now
> > depending on VrFeedbackStatus (which is included as part of
> > chrome_vr_java_sources). chrome_vr_java_sources is only added if enable_vr
is
> > true, which is always false on x86/x64 builds.
> 
> That makes sense. I'll write a CL that adds a VrFeedbackStatusImpl which will
> build with chrome_vr_java_sources and VrFeedbackStatus which will be added to
> chrome_java_sources to keep the bots happy. Does that make sense bsheedy@?

Yes, although this should still be reverted if possible since it'll probably
take at least an hour to get the fix in (have to get through the CQ, etc.).

[ymalik, 2017-05-22 23:23:03]

On 2017/05/22 23:19:30, bsheedy wrote:
> On 2017/05/22 23:15:54, ymalik wrote:
> > On 2017/05/22 23:10:50, bsheedy wrote:
> > > On 2017/05/22 23:01:23, mikecase wrote:
> > > > Im getting an error when trying to revert
> > > > 
> > > > Failed to load resource: the server responded with a status of 500 ()
> > > > /api/2887383002/80001/revert
> > > > 
> > > > Would like to revert this to green up the bots :/
> > > 
> > > I have no idea about the failure to revert, but the bot failure is being
> > caused
> > > by VrShellDelegate (which is included as part of chrome_java_sources) now
> > > depending on VrFeedbackStatus (which is included as part of
> > > chrome_vr_java_sources). chrome_vr_java_sources is only added if enable_vr
> is
> > > true, which is always false on x86/x64 builds.
> > 
> > That makes sense. I'll write a CL that adds a VrFeedbackStatusImpl which
will
> > build with chrome_vr_java_sources and VrFeedbackStatus which will be added
to
> > chrome_java_sources to keep the bots happy. Does that make sense bsheedy@?
> 
> Yes, although this should still be reverted if possible since it'll probably
> take at least an hour to get the fix in (have to get through the CQ, etc.).

Agreed. I think the revert is failing because of enums.xml. Trying to manually
revert now.

[ymalik, 2017-05-22 23:27:09]

On 2017/05/22 23:23:03, ymalik wrote:
> On 2017/05/22 23:19:30, bsheedy wrote:
> > On 2017/05/22 23:15:54, ymalik wrote:
> > > On 2017/05/22 23:10:50, bsheedy wrote:
> > > > On 2017/05/22 23:01:23, mikecase wrote:
> > > > > Im getting an error when trying to revert
> > > > > 
> > > > > Failed to load resource: the server responded with a status of 500 ()
> > > > > /api/2887383002/80001/revert
> > > > > 
> > > > > Would like to revert this to green up the bots :/
> > > > 
> > > > I have no idea about the failure to revert, but the bot failure is being
> > > caused
> > > > by VrShellDelegate (which is included as part of chrome_java_sources)
now
> > > > depending on VrFeedbackStatus (which is included as part of
> > > > chrome_vr_java_sources). chrome_vr_java_sources is only added if
enable_vr
> > is
> > > > true, which is always false on x86/x64 builds.
> > > 
> > > That makes sense. I'll write a CL that adds a VrFeedbackStatusImpl which
> will
> > > build with chrome_vr_java_sources and VrFeedbackStatus which will be added
> to
> > > chrome_java_sources to keep the bots happy. Does that make sense bsheedy@?
> > 
> > Yes, although this should still be reverted if possible since it'll probably
> > take at least an hour to get the fix in (have to get through the CQ, etc.).
> 
> Agreed. I think the revert is failing because of enums.xml. Trying to manually
> revert now.

Revert patch going through CQ now: https://codereview.chromium.org/2896933002/

[ymalik, 2017-05-23 00:14:43]

On 2017/05/22 23:27:09, ymalik wrote:
> On 2017/05/22 23:23:03, ymalik wrote:
> > On 2017/05/22 23:19:30, bsheedy wrote:
> > > On 2017/05/22 23:15:54, ymalik wrote:
> > > > On 2017/05/22 23:10:50, bsheedy wrote:
> > > > > On 2017/05/22 23:01:23, mikecase wrote:
> > > > > > Im getting an error when trying to revert
> > > > > > 
> > > > > > Failed to load resource: the server responded with a status of 500
()
> > > > > > /api/2887383002/80001/revert
> > > > > > 
> > > > > > Would like to revert this to green up the bots :/
> > > > > 
> > > > > I have no idea about the failure to revert, but the bot failure is
being
> > > > caused
> > > > > by VrShellDelegate (which is included as part of chrome_java_sources)
> now
> > > > > depending on VrFeedbackStatus (which is included as part of
> > > > > chrome_vr_java_sources). chrome_vr_java_sources is only added if
> enable_vr
> > > is
> > > > > true, which is always false on x86/x64 builds.
> > > > 
> > > > That makes sense. I'll write a CL that adds a VrFeedbackStatusImpl which
> > will
> > > > build with chrome_vr_java_sources and VrFeedbackStatus which will be
added
> > to
> > > > chrome_java_sources to keep the bots happy. Does that make sense
bsheedy@?
> > > 
> > > Yes, although this should still be reverted if possible since it'll
probably
> > > take at least an hour to get the fix in (have to get through the CQ,
etc.).
> > 
> > Agreed. I think the revert is failing because of enums.xml. Trying to
manually
> > revert now.
> 
> Revert patch going through CQ now: https://codereview.chromium.org/2896933002/

+mthiesse, wdyt?
I think this would be a simple fix. Because VrFeedbackStatus only has static
functions, we can just move it to chrome_java_sources from
chrome_vr_java_sources. Verified locally that this fixes the issue. I'll let the
revert go through though since its further in the commit queue.

[mthiesse, 2017-05-23 00:16:57]

On 2017/05/23 00:14:43, ymalik wrote:
> On 2017/05/22 23:27:09, ymalik wrote:
> > On 2017/05/22 23:23:03, ymalik wrote:
> > > On 2017/05/22 23:19:30, bsheedy wrote:
> > > > On 2017/05/22 23:15:54, ymalik wrote:
> > > > > On 2017/05/22 23:10:50, bsheedy wrote:
> > > > > > On 2017/05/22 23:01:23, mikecase wrote:
> > > > > > > Im getting an error when trying to revert
> > > > > > > 
> > > > > > > Failed to load resource: the server responded with a status of 500
> ()
> > > > > > > /api/2887383002/80001/revert
> > > > > > > 
> > > > > > > Would like to revert this to green up the bots :/
> > > > > > 
> > > > > > I have no idea about the failure to revert, but the bot failure is
> being
> > > > > caused
> > > > > > by VrShellDelegate (which is included as part of
chrome_java_sources)
> > now
> > > > > > depending on VrFeedbackStatus (which is included as part of
> > > > > > chrome_vr_java_sources). chrome_vr_java_sources is only added if
> > enable_vr
> > > > is
> > > > > > true, which is always false on x86/x64 builds.
> > > > > 
> > > > > That makes sense. I'll write a CL that adds a VrFeedbackStatusImpl
which
> > > will
> > > > > build with chrome_vr_java_sources and VrFeedbackStatus which will be
> added
> > > to
> > > > > chrome_java_sources to keep the bots happy. Does that make sense
> bsheedy@?
> > > > 
> > > > Yes, although this should still be reverted if possible since it'll
> probably
> > > > take at least an hour to get the fix in (have to get through the CQ,
> etc.).
> > > 
> > > Agreed. I think the revert is failing because of enums.xml. Trying to
> manually
> > > revert now.
> > 
> > Revert patch going through CQ now:
https://codereview.chromium.org/2896933002/
> 
> +mthiesse, wdyt?
> I think this would be a simple fix. Because VrFeedbackStatus only has static
> functions, we can just move it to chrome_java_sources from
> chrome_vr_java_sources. Verified locally that this fixes the issue. I'll let
the
> revert go through though since its further in the commit queue.

sgtm. Anything that doesn't depend on gvr should probably just always be
compiled in for java.