Remove unnecessary UI element math.

Remove unnecessary UI element math.

This change makes UI element computations more efficient by eliminating
unncessary reverse-transform support. Remove other unused code along the way.

BUG=None

Review-Url: https://codereview.chromium.org/2730883003
Cr-Commit-Position: refs/heads/master@{#454912}
Committed: https://chromium.googlesource.com/chromium/src/+/532c96fad9f5e4bcf277dfc5cffee840ca769001

[cjgrant, 2017-03-03 21:24:56]

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

[cjgrant, 2017-03-03 21:30:34]

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:344: element->transform.Rotate(1.0f,
0.0f, 0.0f, screen_tilt);
Whoa, this looks broken, and the unit tests don't catch it.

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (left):

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:534: if (target_element_ !=
nullptr) {
For the record - I haven't fully tested this on device yet.

[mthiesse, 2017-03-06 17:26:07]

lgtm

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:344: element->transform.Rotate(1.0f,
0.0f, 0.0f, screen_tilt);
On 2017/03/03 21:30:34, cjgrant wrote:
> Whoa, this looks broken, and the unit tests don't catch it.

THAT explains a lot. I thought recentering was doing weird things and not
actually centering.

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (left):

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:534: if (target_element_ !=
nullptr) {
On 2017/03/03 21:30:34, cjgrant wrote:
> For the record - I haven't fully tested this on device yet.

I trust you'll do so before submitting :P

[cjgrant, 2017-03-06 17:43:35]

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:344: element->transform.Rotate(1.0f,
0.0f, 0.0f, screen_tilt);
On 2017/03/03 21:30:34, cjgrant wrote:
> Whoa, this looks broken, and the unit tests don't catch it.

Oops, disregard, this block is fine.

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:344: element->transform.Rotate(1.0f,
0.0f, 0.0f, screen_tilt);
On 2017/03/06 17:26:07, mthiesse wrote:
> On 2017/03/03 21:30:34, cjgrant wrote:
> > Whoa, this looks broken, and the unit tests don't catch it.
> 
> THAT explains a lot. I thought recentering was doing weird things and not
> actually centering.

Since my original comment was not valid, I need to ask you what you meant by
this.

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (left):

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:534: if (target_element_ !=
nullptr) {
On 2017/03/06 17:26:07, mthiesse wrote:
> On 2017/03/03 21:30:34, cjgrant wrote:
> > For the record - I haven't fully tested this on device yet.
> 
> I trust you'll do so before submitting :P

I should never have tried to hastily post this before the weekend.  It had been
tested on-device, but not after the latest edits, etc.  It all works fine.  :)

[mthiesse, 2017-03-06 17:46:00]

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2730883003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:344: element->transform.Rotate(1.0f,
0.0f, 0.0f, screen_tilt);
On 2017/03/06 17:43:34, cjgrant wrote:
> On 2017/03/06 17:26:07, mthiesse wrote:
> > On 2017/03/03 21:30:34, cjgrant wrote:
> > > Whoa, this looks broken, and the unit tests don't catch it.
> > 
> > THAT explains a lot. I thought recentering was doing weird things and not
> > actually centering.
> 
> Since my original comment was not valid, I need to ask you what you meant by
> this.

We can follow up offline and make sure I'm not crazy. Feel free to submit as-is.

[tiborg, 2017-03-06 17:51:44]

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:21: float* distance) {
Check for null pointer?

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:66: bool GetRayDistance(const
gvr::Vec3f& ray_origin,
Add comment to describe the return type and parameters?

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:69: gvr::Vec2f
GetUnitRectangleCoordinates(const gvr::Vec3f& world_point);
Also here I would say comments help a lot (return type, parameters, what the
function does).

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_math.cc (left):

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_math.cc:50: void TranslateMRight(gvr::Mat4f&
tmat,
Should we delete all these matrix operations? Maybe they are useful in the
future.

[cjgrant, 2017-03-06 18:41:02]

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:21: float* distance) {
On 2017/03/06 17:51:43, tiborg wrote:
> Check for null pointer?

IMO, null checks are good in APIs to protect against bad outside callers.  In
this local function, it would only protect against the other code in this
module, so it's not worth the 1 extra CPU cycle, I think.

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_math.cc (left):

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_math.cc:50: void TranslateMRight(gvr::Mat4f&
tmat,
On 2017/03/06 17:51:43, tiborg wrote:
> Should we delete all these matrix operations? Maybe they are useful in the
> future.

We should add the methods only when we need them. I'm pretty sure that thinking
applies across Chromium/Google3.

[cjgrant, 2017-03-06 18:41:47]

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:66: bool GetRayDistance(const
gvr::Vec3f& ray_origin,
On 2017/03/06 17:51:43, tiborg wrote:
> Add comment to describe the return type and parameters?

Done.

https://codereview.chromium.org/2730883003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:69: gvr::Vec2f
GetUnitRectangleCoordinates(const gvr::Vec3f& world_point);
On 2017/03/06 17:51:43, tiborg wrote:
> Also here I would say comments help a lot (return type, parameters, what the
> function does).

Done.

[cjgrant, 2017-03-06 18:41:51]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-03-06 18:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2730883003/#ps40001
(title: "Add comments to non-obvious methods.")

[commit-bot: I haz the power, 2017-03-06 18:42:25]

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-03-06 19:20:18]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488825711251620,
"parent_rev": "317a9b943cc6e28bb3f7d0cda4ae9382d84f9288", "commit_rev":
"532c96fad9f5e4bcf277dfc5cffee840ca769001"}

[commit-bot: I haz the power, 2017-03-06 19:20:56]

Description was changed from

==========
Remove unnecessary UI element math.

This change makes UI element computations more efficient by eliminating
unncessary reverse-transform support. Remove other unused code along the way.

BUG=None
==========

to

==========
Remove unnecessary UI element math.

This change makes UI element computations more efficient by eliminating
unncessary reverse-transform support. Remove other unused code along the way.

BUG=None

Review-Url: https://codereview.chromium.org/2730883003
Cr-Commit-Position: refs/heads/master@{#454912}
Committed:
https://chromium.googlesource.com/chromium/src/+/532c96fad9f5e4bcf277dfc5cffe...
==========

[commit-bot: I haz the power, 2017-03-06 19:20:57]

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