Add way to get native VR UI information from Java

Add way to get native VR UI information from Java

Adds a way to request information about native UI elements in Java
for instrumentation testing. Based on the earlier crrev.com/2766503002,
but goes directly from Java > Native instead of Java > JS > Native.

BUG=702700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[bsheedy, 2017-03-24 17:51:49]

Description was changed from

==========
Add way to get native VR UI information from Java

Adds a way to request information about native UI elements in Java
for instrumentation testing. Based on the earlier crrev.com/2766503002,
but goes directly from Java > Native instead of Java > JS > Native.

BUG=702700
==========

to

==========
Add way to get native VR UI information from Java

Adds a way to request information about native UI elements in Java
for instrumentation testing. Based on the earlier crrev.com/2766503002,
but goes directly from Java > Native instead of Java > JS > Native.

BUG=702700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[bsheedy, 2017-03-24 18:47:54]

bsheedy@chromium.org changed reviewers:
+ cjgrant@chromium.org, tiborg@chromium.org

[bsheedy, 2017-03-24 18:47:54]

Now with 75% less Javascript.

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

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:561: base::JSONWriter::Write(*info,
&json_reply);
Converting to a JSON string in native then back to an object in Java doesn't
seem like the ideal approach, but I'm not aware of a better way to get a
base::Value type back to Java.

[tiborg, 2017-03-27 15:14:33]

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

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:647:
// Lazily instantiate since we don't need it in most cases
Nit: end sentence with full stop.

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:648:
private SparseArray<Runnable> getUiElementInfoReplyCallbacks() {
Should we append ForTesting to make sure it is only used by tests?

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:655:
// Lazily instantiate since we don't need it in most cases
Nit: end sentence with full stop.

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:657:
public SparseArray<JSONObject> getUiElementInfoJson() {
Here, too, append ForTesting?

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:668:
public void requestUiElementInfoFromNative(int[] elementIds, Runnable onReply) {
Here, too, append ForTesting?

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:675:
public void replyToUiElementInfoRequest(String jsonString, int callbackId)
Here, too, append ForTesting?

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
(right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:340:
// TODO(bsheedy): Request directly using native names once that is supported
Nit: end with full stop.

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

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:561: base::JSONWriter::Write(*info,
&json_reply);
On 2017/03/24 18:47:54, bsheedy wrote:
> Converting to a JSON string in native then back to an object in Java doesn't
> seem like the ideal approach, but I'm not aware of a better way to get a
> base::Value type back to Java.

I'm not aware of a better solution either. I think it is suitable for testing.

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:346: onCreate) {
Nit: I think we generally follow the convention to use underbars as spaces for
parameter names. So, onCreate -> on_created.

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:215:
domIdToUiElementIdMap[domId] = this.id;
cjgrant@: is this a good workaround until the names exist?

[cjgrant, 2017-03-27 15:41:32]

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:215:
domIdToUiElementIdMap[domId] = this.id;
On 2017/03/27 15:14:33, tiborg wrote:
> cjgrant@: is this a good workaround until the names exist?

Brian said he'd wait on using element names, as that patch will land soon. 
Personally I'd prefer not to have this particular map, but either way, we will
need a "name to element" map.

[bsheedy, 2017-03-27 17:49:10]

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

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:647:
// Lazily instantiate since we don't need it in most cases
On 2017/03/27 15:14:32, tiborg wrote:
> Nit: end sentence with full stop.

Done.

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:648:
private SparseArray<Runnable> getUiElementInfoReplyCallbacks() {
On 2017/03/27 15:14:33, tiborg wrote:
> Should we append ForTesting to make sure it is only used by tests?

If this is only ever going to be used for testing purposes, sure - are we ever
going to want to get the native state during normal use?

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:655:
// Lazily instantiate since we don't need it in most cases
On 2017/03/27 15:14:33, tiborg wrote:
> Nit: end sentence with full stop.

Done.

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:657:
public SparseArray<JSONObject> getUiElementInfoJson() {
On 2017/03/27 15:14:33, tiborg wrote:
> Here, too, append ForTesting?

Same as above.

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:668:
public void requestUiElementInfoFromNative(int[] elementIds, Runnable onReply) {
On 2017/03/27 15:14:33, tiborg wrote:
> Here, too, append ForTesting?

Same as above.

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:675:
public void replyToUiElementInfoRequest(String jsonString, int callbackId)
On 2017/03/27 15:14:33, tiborg wrote:
> Here, too, append ForTesting?

Same as above.

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
(right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:340:
// TODO(bsheedy): Request directly using native names once that is supported
On 2017/03/27 15:14:33, tiborg wrote:
> Nit: end with full stop.

Done, although this TODO will be resolved before submitting anyways.

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:346: onCreate) {
On 2017/03/27 15:14:33, tiborg wrote:
> Nit: I think we generally follow the convention to use underbars as spaces for
> parameter names. So, onCreate -> on_created.

Done.

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:215:
domIdToUiElementIdMap[domId] = this.id;
On 2017/03/27 15:41:32, cjgrant wrote:
> On 2017/03/27 15:14:33, tiborg wrote:
> > cjgrant@: is this a good workaround until the names exist?
> 
> Brian said he'd wait on using element names, as that patch will land soon. 
> Personally I'd prefer not to have this particular map, but either way, we will
> need a "name to element" map.

Do we really need a map since we're removing all the JS from this CL once your
patch lands? On the native side, GetUiElementById just uses a linear search, so
I imagine GetUiElementByName will do the same.

[cjgrant, 2017-03-27 19:49:43]

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:215:
domIdToUiElementIdMap[domId] = this.id;
On 2017/03/27 17:49:09, bsheedy wrote:
> On 2017/03/27 15:41:32, cjgrant wrote:
> > On 2017/03/27 15:14:33, tiborg wrote:
> > > cjgrant@: is this a good workaround until the names exist?
> > 
> > Brian said he'd wait on using element names, as that patch will land soon. 
> > Personally I'd prefer not to have this particular map, but either way, we
will
> > need a "name to element" map.
> 
> Do we really need a map since we're removing all the JS from this CL once your
> patch lands? On the native side, GetUiElementById just uses a linear search,
so
> I imagine GetUiElementByName will do the same.

That works if test will only ever check native state.  I can't imagine that
being the case though.  I picture tests checking a mix of native and Javascript
state, depending on what exactly you're testing.  But as always, I guess we add
what we need, when we need it.

[tiborg, 2017-03-27 19:59:59]

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

https://codereview.chromium.org/2773903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:648:
private SparseArray<Runnable> getUiElementInfoReplyCallbacks() {
On 2017/03/27 17:49:09, bsheedy wrote:
> On 2017/03/27 15:14:33, tiborg wrote:
> > Should we append ForTesting to make sure it is only used by tests?
> 
> If this is only ever going to be used for testing purposes, sure - are we ever
> going to want to get the native state during normal use?

I'm not aware of any plans to use this functionality during normal use. So, I
would append for ForTesting and if we ever want to use it in normal use we just
remove the suffix.

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2773903003/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:215:
domIdToUiElementIdMap[domId] = this.id;
On 2017/03/27 19:49:43, cjgrant wrote:
> On 2017/03/27 17:49:09, bsheedy wrote:
> > On 2017/03/27 15:41:32, cjgrant wrote:
> > > On 2017/03/27 15:14:33, tiborg wrote:
> > > > cjgrant@: is this a good workaround until the names exist?
> > > 
> > > Brian said he'd wait on using element names, as that patch will land soon.

> > > Personally I'd prefer not to have this particular map, but either way, we
> will
> > > need a "name to element" map.
> > 
> > Do we really need a map since we're removing all the JS from this CL once
your
> > patch lands? On the native side, GetUiElementById just uses a linear search,
> so
> > I imagine GetUiElementByName will do the same.
> 
> That works if test will only ever check native state.  I can't imagine that
> being the case though.  I picture tests checking a mix of native and
Javascript
> state, depending on what exactly you're testing.  But as always, I guess we
add
> what we need, when we need it.

+1 to add the map once we need it.

[bsheedy, 2017-03-27 23:56:14]

Now with 100% less Javascript.

[cjgrant, 2017-03-29 15:43:48]

On 2017/03/27 23:56:14, bsheedy wrote:
> Now with 100% less Javascript.

Brian, can you clarify this CL vs 2766503002?  Why are they both up?  I assume
we're now focused on this one only.

[bsheedy, 2017-03-29 16:25:38]

On 2017/03/29 15:43:48, cjgrant wrote:
> On 2017/03/27 23:56:14, bsheedy wrote:
> > Now with 100% less Javascript.
> 
> Brian, can you clarify this CL vs 2766503002?  Why are they both up?  I assume
> we're now focused on this one only.

Yes, the intention is to only focus on/commit this one now. I'm keeping the
previous one around for now so I can remember exactly what I did in order to get
the Javascript side of things working if/when we decide to add that
functionality in, but I don't plan to commit that specific CL.

[cjgrant, 2017-03-29 17:12:47]

On 2017/03/29 16:25:38, bsheedy wrote:
> On 2017/03/29 15:43:48, cjgrant wrote:
> > On 2017/03/27 23:56:14, bsheedy wrote:
> > > Now with 100% less Javascript.
> > 
> > Brian, can you clarify this CL vs 2766503002?  Why are they both up?  I
assume
> > we're now focused on this one only.
> 
> Yes, the intention is to only focus on/commit this one now. I'm keeping the
> previous one around for now so I can remember exactly what I did in order to
get
> the Javascript side of things working if/when we decide to add that
> functionality in, but I don't plan to commit that specific CL.

You can close the review, and still refer to it, FWIW.

[bsheedy, 2017-03-29 17:14:47]

On 2017/03/29 17:12:47, cjgrant wrote:
> On 2017/03/29 16:25:38, bsheedy wrote:
> > On 2017/03/29 15:43:48, cjgrant wrote:
> > > On 2017/03/27 23:56:14, bsheedy wrote:
> > > > Now with 100% less Javascript.
> > > 
> > > Brian, can you clarify this CL vs 2766503002?  Why are they both up?  I
> assume
> > > we're now focused on this one only.
> > 
> > Yes, the intention is to only focus on/commit this one now. I'm keeping the
> > previous one around for now so I can remember exactly what I did in order to
> get
> > the Javascript side of things working if/when we decide to add that
> > functionality in, but I don't plan to commit that specific CL.
> 
> You can close the review, and still refer to it, FWIW.

Done

[cjgrant, 2017-03-29 21:16:06]

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.h:71: ContentRectangle*
GetUiElementByName(std::string element_name);
The parameter should be (const std::string& element_name), to avoid copying the
string.

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.h:81:
std::unique_ptr<base::DictionaryValue> CreateUiElementInfo(
Please comment that this is for testing (or name the method as such).

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:349:
std::unique_ptr<base::DictionaryValue> reply =
s/std::unique_ptr<base::DictionaryValue>/auto/ is good here.  We want to avoid
auto when the type isn't clear, but in a MakeUnique case, it's crystal clear.

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:351: for (unsigned int i = 0; i <
element_names.size(); i++) {
for (const auto& name : element_names) {
  reply->Set(name, ...)

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.h:114: std::vector<std::string>
element_names,
const std::vector<std::string>&...  Again, avoid the copy.

[tiborg, 2017-03-29 21:20:03]

On 2017/03/29 21:16:06, cjgrant wrote:
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_scene.h (right):
> 
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene.h:71: ContentRectangle*
> GetUiElementByName(std::string element_name);
> The parameter should be (const std::string& element_name), to avoid copying
the
> string.
> 
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene.h:81:
> std::unique_ptr<base::DictionaryValue> CreateUiElementInfo(
> Please comment that this is for testing (or name the method as such).
> 
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> 
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:349:
> std::unique_ptr<base::DictionaryValue> reply =
> s/std::unique_ptr<base::DictionaryValue>/auto/ is good here.  We want to avoid
> auto when the type isn't clear, but in a MakeUnique case, it's crystal clear.
> 
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:351: for (unsigned int i = 0; i
<
> element_names.size(); i++) {
> for (const auto& name : element_names) {
>   reply->Set(name, ...)
> 
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell_gl.h (right):
> 
>
https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.h:114: std::vector<std::string>
> element_names,
> const std::vector<std::string>&...  Again, avoid the copy.

lgtm with cjgrant@'s comments addressed.

[bsheedy, 2017-03-29 22:33:44]

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

[bsheedy, 2017-03-29 22:33:45]

mthiesse@chromium.org: chrome/android/java

dtrainor@chromium.org: chrome/android/javatests

FYI, I also snuck in a one-line stability improvement (make resetting the
synchronization boolean blocking) since creating a separate CL for that seems
unnecessary.

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.h:71: ContentRectangle*
GetUiElementByName(std::string element_name);
On 2017/03/29 21:16:05, cjgrant wrote:
> The parameter should be (const std::string& element_name), to avoid copying
the
> string.

Done.

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.h:81:
std::unique_ptr<base::DictionaryValue> CreateUiElementInfo(
On 2017/03/29 21:16:06, cjgrant wrote:
> Please comment that this is for testing (or name the method as such).

Done.

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:349:
std::unique_ptr<base::DictionaryValue> reply =
On 2017/03/29 21:16:06, cjgrant wrote:
> s/std::unique_ptr<base::DictionaryValue>/auto/ is good here.  We want to avoid
> auto when the type isn't clear, but in a MakeUnique case, it's crystal clear.

Done.

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:351: for (unsigned int i = 0; i <
element_names.size(); i++) {
On 2017/03/29 21:16:06, cjgrant wrote:
> for (const auto& name : element_names) {
>   reply->Set(name, ...)

Done.

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2773903003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.h:114: std::vector<std::string>
element_names,
On 2017/03/29 21:16:06, cjgrant wrote:
> const std::vector<std::string>&...  Again, avoid the copy.

Done.

[David Trainor- moved to gerrit, 2017-03-30 04:45:38]

lgtm % nits

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
(right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:133:
CriteriaHelper.pollInstrumentationThread(Criteria.equals(true, new
Callable<Boolean>() {
Should this be instrumentation thread?  Some of the other calls to
VrShellDelegate are made on the UI thread (waitForVrSupported above for
example).

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:136:
if (VrShellDelegate.getVrShellForTesting() == null) return false;
VrShellImpl vrShell = (VrShellImpl) VrShellDelegate.getVrShellForTesting();
return vrShell != null && vrShell.getUiContentsForTesting() != null;

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:142:
return ((VrShellImpl)
VrShellDelegate.getVrShellForTesting()).getUiContentsForTesting();
Maybe try AtomicReference and just set it as we poll or something?  Or create a
[1] array that you set the return value on?

[mthiesse, 2017-03-30 14:25:39]

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:108:
private SparseArray<Runnable> mUiElementInfoReplyCallbacks;
Why do we need multiple callbacks at once? Seems unnecessarily complex. Can we
get away with having only one?

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:658:
public HashMap<String, JSONObject> getUiElementInfoJsonForTesting() {
Why expose this? Why not just create this object in
replyToUiElementInfoRequestForTesting and pass it through the callback?

[cjgrant, 2017-03-30 14:48:36]

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.cc (right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.cc:399: CHECK_NE(element, nullptr);
Given that this is for testing, are you sure you want to blow up if an element
name doesn't exist?  This should likely be a test failure rather than a crash. 
You could return a nullptr, or an empty dictionary, depending on how you want
the tests to handle this case.

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.h:81: // For testing whether the native
state matches expectations
- Needs a period (all comments need to be full sentences as per style guide).
- Sorry to be picky, but could you further elaborate?  Something like "Builds a
dictionary of UI element properties for testing purposes. The dictionary can be
serialized and passed to Java in end-to-end tests."  Or something.

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.h:83: std::string element_name);
const std::string& here too.  Could you scrub the rest of the CL and make sure
none are missed?  In general, any time you're passing an object for read-only
purposes, you want to do it as a const reference.  The style guide points out
that if you'll modify the object it should be passed through a pointer instead,
but that doesn't matter here.

[bsheedy, 2017-03-30 21:05:14]

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:108:
private SparseArray<Runnable> mUiElementInfoReplyCallbacks;
On 2017/03/30 14:25:38, mthiesse wrote:
> Why do we need multiple callbacks at once? Seems unnecessarily complex. Can we
> get away with having only one?

For testing, a single callback should be sufficient. Changed to support only one
callback now. I originally wrote this assuming we might need to get the native
UI info for non-testing purposes at some point, in which case multiple callbacks
might be necessary.

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:658:
public HashMap<String, JSONObject> getUiElementInfoJsonForTesting() {
On 2017/03/30 14:25:39, mthiesse wrote:
> Why expose this? Why not just create this object in
> replyToUiElementInfoRequestForTesting and pass it through the callback?

Done.

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
(right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:133:
CriteriaHelper.pollInstrumentationThread(Criteria.equals(true, new
Callable<Boolean>() {
On 2017/03/30 04:45:38, David Trainor-ping if over 24h wrote:
> Should this be instrumentation thread?  Some of the other calls to
> VrShellDelegate are made on the UI thread (waitForVrSupported above for
> example).

I don't think it really matters in this case, but I've changed it to the UI
thread for consistency.

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:136:
if (VrShellDelegate.getVrShellForTesting() == null) return false;
On 2017/03/30 04:45:38, David Trainor-ping if over 24h wrote:
> VrShellImpl vrShell = (VrShellImpl) VrShellDelegate.getVrShellForTesting();
> return vrShell != null && vrShell.getUiContentsForTesting() != null;

Done.

https://codereview.chromium.org/2773903003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:142:
return ((VrShellImpl)
VrShellDelegate.getVrShellForTesting()).getUiContentsForTesting();
On 2017/03/30 04:45:38, David Trainor-ping if over 24h wrote:
> Maybe try AtomicReference and just set it as we poll or something?  Or create
a
> [1] array that you set the return value on?

Done. Didn't know about AtomicReference, that could be handy in the future.

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.cc (right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.cc:399: CHECK_NE(element, nullptr);
On 2017/03/30 14:48:35, cjgrant wrote:
> Given that this is for testing, are you sure you want to blow up if an element
> name doesn't exist?  This should likely be a test failure rather than a crash.

> You could return a nullptr, or an empty dictionary, depending on how you want
> the tests to handle this case.

Done.

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.h:81: // For testing whether the native
state matches expectations
On 2017/03/30 14:48:35, cjgrant wrote:
> - Needs a period (all comments need to be full sentences as per style guide).
> - Sorry to be picky, but could you further elaborate?  Something like "Builds
a
> dictionary of UI element properties for testing purposes. The dictionary can
be
> serialized and passed to Java in end-to-end tests."  Or something.

Done.

https://codereview.chromium.org/2773903003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.h:83: std::string element_name);
On 2017/03/30 14:48:36, cjgrant wrote:
> const std::string& here too.  Could you scrub the rest of the CL and make sure
> none are missed?  In general, any time you're passing an object for read-only
> purposes, you want to do it as a const reference.  The style guide points out
> that if you'll modify the object it should be passed through a pointer
instead,
> but that doesn't matter here.  

Done, found one additional place where const ref could be used (callback to the
main thread).

[cjgrant, 2017-03-30 21:20:17]

lgtm

https://codereview.chromium.org/2773903003/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.cc (right):

https://codereview.chromium.org/2773903003/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.cc:399: // Return an empty dict if not
found and let Java handle the failure
The comment is unnecessary, but if you feel it's needed, it needs it's period.

[bsheedy, 2017-03-30 21:23:04]

https://codereview.chromium.org/2773903003/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.cc (right):

https://codereview.chromium.org/2773903003/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.cc:399: // Return an empty dict if not
found and let Java handle the failure
On 2017/03/30 21:20:16, cjgrant wrote:
> The comment is unnecessary, but if you feel it's needed, it needs it's period.

Removed. Not quite sure why I'm having so much trouble with periods.

[mthiesse, 2017-03-31 16:20:26]

We should hold off on landing this until we know what direction the UI is going
in. Word on the street is HTML UI is dead, and depending on where we go this
infrastructure won't be useful.

[cjgrant, 2017-04-13 14:20:57]

On 2017/03/31 16:20:26, mthiesse wrote:
> We should hold off on landing this until we know what direction the UI is
going
> in. Word on the street is HTML UI is dead, and depending on where we go this
> infrastructure won't be useful.

Bsheedy, we might as well pull this down.  The HTML UI removal CL is up and
pending commit (before or after branch cut, depending on leads' preference).

[bsheedy, 2017-04-13 16:26:34]

Description was changed from

==========
Add way to get native VR UI information from Java

Adds a way to request information about native UI elements in Java
for instrumentation testing. Based on the earlier crrev.com/2766503002,
but goes directly from Java > Native instead of Java > JS > Native.

BUG=702700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add way to get native VR UI information from Java

Adds a way to request information about native UI elements in Java
for instrumentation testing. Based on the earlier crrev.com/2766503002,
but goes directly from Java > Native instead of Java > JS > Native.

BUG=702700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========