VR: Implement Omnibox input targeting.

VR: Implement Omnibox input targeting.

Also updates some constants to match spec.

BUG=715554
Review-Url: https://codereview.chromium.org/2885433003
Cr-Commit-Position: refs/heads/master@{#472214}
Committed: https://chromium.googlesource.com/chromium/src/+/febe5cb69cb8c121f2f95f3ec5fb81474958e992

[mthiesse, 2017-05-15 22:50:25]

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

[mthiesse, 2017-05-15 22:50:25]

PTAL

[cjgrant, 2017-05-16 14:07:24]

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/loading_indicator_texture.cc
(right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/loading_indicator_texture.cc:16: static
constexpr SkColor kBackground = 0xCCAAAAAA;
Lets set these too?
background: B9B9B9
foreground: 595959

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:25: static constexpr
SkColor kBackground = 0xCCD6D6D6;
Where is this one from?

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:26: static constexpr
SkColor kBackgroundHover = 0xCCE1E1E1;
Should we drop the 0xCC?

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:31: 
We also have to add "inactive back button" coloring (either here or I'll do it
later)

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:98: float
rounded_radius = kHeight / kWidth / 2;
This is fine if it works, but alternatively, would you find this more clear?

position = PercentageToMeters(position);
circle_center = {width - height/2, height/2}
if (position < circle_center.x())
  return false;
diff = position - circle_center;
return diff.LengthSquared() < height/2;

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.h (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.h:38: bool
HitsURLBar(const gfx::PointF& position) const;
HitsUrlBar (matches style guide and class name)

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements/url_bar.cc (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements/url_bar.cc:42:
OnStateUpdated(gfx::PointF(-1000, -1000));
Were you okay with the numeric limits value? lowest() maybe?

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:39: static constexpr float
kUrlBarDistance = 2.4;
Not in this CL, but what do you think of Josh's approach to this?  For each
element, there'd be a parent element, and we'd be doing something like:

urlBar.set_translation(0, -0.516, -1);
urlBarParent.set_scale(kUrlBarDistance, kUrlBarDistance, kUrlBarDistance);

And at the same time, we refactor the actual element class to set its own size
based on texture dimensions.  I'm not sure I like that approach, but it does
keep dimensions in one spot, and matches the way the spec describes things.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:46: static constexpr float
kLoadingIndicatorOffset = -0.020 * kUrlBarDistance;
Worth it to do (0.016 + kLoadingIndicatorHeight / 2)) * kUrlBarDistance instead?

[mthiesse, 2017-05-16 18:11:02]

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/loading_indicator_texture.cc
(right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/loading_indicator_texture.cc:16: static
constexpr SkColor kBackground = 0xCCAAAAAA;
On 2017/05/16 14:07:23, cjgrant wrote:
> Lets set these too?
> background: B9B9B9
> foreground: 595959

I want to leave as-is for now. Will revisit once we get the background
color/opacity story resolved.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:25: static constexpr
SkColor kBackground = 0xCCD6D6D6;
On 2017/05/16 14:07:23, cjgrant wrote:
> Where is this one from?

I pulled it out of my a-

There wasn't any in the spec (well, there was but it was a weird blue color that
must have been erroneous), so I took the delta between the hover and down states
and set the background to have the same delta.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:26: static constexpr
SkColor kBackgroundHover = 0xCCE1E1E1;
On 2017/05/16 14:07:23, cjgrant wrote:
> Should we drop the 0xCC?

Spec maaaaybe suggests using 0x66. so I'll switch to that.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:31: 
On 2017/05/16 14:07:23, cjgrant wrote:
> We also have to add "inactive back button" coloring (either here or I'll do it
> later)

l8r

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:98: float
rounded_radius = kHeight / kWidth / 2;
On 2017/05/16 14:07:23, cjgrant wrote:
> This is fine if it works, but alternatively, would you find this more clear?
> 
> position = PercentageToMeters(position);
> circle_center = {width - height/2, height/2}
> if (position < circle_center.x())
>   return false;
> diff = position - circle_center;
> return diff.LengthSquared() < height/2;

Yeah, looks clearer when you convert everything to meters, rather than
converting everything to percentages.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.h (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.h:38: bool
HitsURLBar(const gfx::PointF& position) const;
On 2017/05/16 14:07:23, cjgrant wrote:
> HitsUrlBar (matches style guide and class name)

Done.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements/url_bar.cc (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements/url_bar.cc:42:
OnStateUpdated(gfx::PointF(-1000, -1000));
On 2017/05/16 14:07:23, cjgrant wrote:
> Were you okay with the numeric limits value? lowest() maybe?

Whoops missed this for the URL bar.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:39: static constexpr float
kUrlBarDistance = 2.4;
On 2017/05/16 14:07:23, cjgrant wrote:
> Not in this CL, but what do you think of Josh's approach to this?  For each
> element, there'd be a parent element, and we'd be doing something like:
> 
> urlBar.set_translation(0, -0.516, -1);
> urlBarParent.set_scale(kUrlBarDistance, kUrlBarDistance, kUrlBarDistance);
> 
> And at the same time, we refactor the actual element class to set its own size
> based on texture dimensions.  I'm not sure I like that approach, but it does
> keep dimensions in one spot, and matches the way the spec describes things.

For now I'd like to land as-is, we have some thinking and discussing to do here.
For one, the scene manager needs to know size in order to set position
correctly, or we need to somehow allow elements to update their position based
on their size which could get complex quickly if we're not careful.

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:46: static constexpr float
kLoadingIndicatorOffset = -0.020 * kUrlBarDistance;
On 2017/05/16 14:07:23, cjgrant wrote:
> Worth it to do (0.016 + kLoadingIndicatorHeight / 2)) * kUrlBarDistance
instead?

Done.

[cjgrant, 2017-05-16 18:22:22]

lgtm

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2885433003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:98: float
rounded_radius = kHeight / kWidth / 2;
On 2017/05/16 18:11:01, mthiesse wrote:
> On 2017/05/16 14:07:23, cjgrant wrote:
> > This is fine if it works, but alternatively, would you find this more clear?
> > 
> > position = PercentageToMeters(position);
> > circle_center = {width - height/2, height/2}
> > if (position < circle_center.x())
> >   return false;
> > diff = position - circle_center;
> > return diff.LengthSquared() < height/2;
> 
> Yeah, looks clearer when you convert everything to meters, rather than
> converting everything to percentages.

Acknowledged.

[mthiesse, 2017-05-16 18:23:02]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-05-16 18:24:21]

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-16 18:31:44]

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

[commit-bot: I haz the power, 2017-05-16 18:31:45]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mthiesse, 2017-05-16 19:13:05]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-05-16 19:13:06]

The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2885433003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-16 19:14:34]

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-16 19:20:02]

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

[commit-bot: I haz the power, 2017-05-16 19:20:03]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mthiesse, 2017-05-16 19:36:41]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-05-16 19:38:27]

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-16 20:55:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494963401441430,
"parent_rev": "7aac149008e5b6ad016225a6cc594c052e1968fa", "commit_rev":
"febe5cb69cb8c121f2f95f3ec5fb81474958e992"}

[commit-bot: I haz the power, 2017-05-16 20:55:58]

Description was changed from

==========
VR: Implement Omnibox input targeting.

Also updates some constants to match spec.

BUG=715554
==========

to

==========
VR: Implement Omnibox input targeting.

Also updates some constants to match spec.

BUG=715554

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

[commit-bot: I haz the power, 2017-05-16 20:55:58]

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