Move Screen Rotation from MaximizeModeController to ScreenOrientationController

This change renames ScreenOrientationDelegate to ScreenOrientationController to reflect on the new responsibilities that it is assuming.

The tracking of the accelerometer screen rotation, as well as the rotation lock, has been moved from MaximizeModeController to ScreenOrientationController.

ScreenOrientationController is now responsible for all rotation lock requests, from both TouchView user interface controls, as well as the Screen Orientation JavaScript API.

ScreenOrientationController is Chrome OS only. Classes using the rotation lock API have been updated to only do so on Chrome OS.

Screen rotation tests have been moved from MaximizeModeControllerTest to ScreenOrientationControllerTest. Other unittests have been updated to reflect the Chrome OS change. All pre-existing tests for rotation behaviour have been ran. Manual testing of both TouchView and Screen Orientation API have been done.

TEST=ScreenOrientationControllerTest, TrayRotationLockTest, MaximizeModeControllerTest, DisplayPreferencesTest
BUG=396760
Committed: https://crrev.com/0af4521339c7ca15535ea92dd4c27f94610b9a67
Cr-Commit-Position: refs/heads/master@{#311299}

[jonross, 2014-11-25 22:37:44]

Patchset #1 (id:1) has been deleted

[jonross, 2014-11-25 22:40:22]

jonross@chromium.org changed reviewers:
+ flackr@chromium.org

[jonross, 2014-11-25 22:40:22]

Hi Rob,

Could you review this change? I refactor rotation lock out of
MaximizeModeController and into ScreenOrientationDelegate.

Thanks,
Jon

[flackr, 2014-11-27 15:39:16]

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
File ash/content/display/screen_orientation_delegate_chromeos.cc (right):

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.cc:11: #include
"ash/wm/maximize_mode/maximize_mode_controller.h"
Obsolete?

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.cc:74:
base::AutoReset<bool> auto_ignore_display_configuration_updates(
Can you scope this to just the call which triggers a display configuration
update?

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
File ash/content/display/screen_orientation_delegate_chromeos.h (right):

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.h:33: // Invoked
whenever |rotation_locked_| is changed.
Since |rotation_locked_| isn't a member of the observer, describe this by the
meaning.

Invoked when rotation is locked or unlocked.

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.h:71: //
OnAccelerometerUpdated should not be used to change the rotation.
Can you update this comment? OnAccelerometerUpdated doesn't refer to something
local.

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
File ash/content/display/screen_orientation_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos_unittest.cc:13:
#include "ash/wm/maximize_mode/maximize_mode_controller.h"
Obsolete?

https://codereview.chromium.org/759063002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/759063002/diff/20001/ash/shell.cc#newcode488
ash/shell.cc:488: return
!((maximize_mode_controller_->IsMaximizeModeWindowManagerEnabled() &&
Do we need to check if maximize mode window manager is enabled? isn't just the
ignore_display_configuration_updates sufficient to ignore them?

https://codereview.chromium.org/759063002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/759063002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:210: #if defined(OS_CHROMEOS)
There's a lot of "#if defined(OS_CHROMEOS)" going on here now. Can we make
screen rotation the responsibility of the increasingly inaccurately named
ScreenOrientationDelegate (I'd suggest renaming it ScreenOrientationController
with the responsibilities it would have with this suggestion).

We could have that class listen to the accelerometer updates and implement all
of the screen rotation. It could listen to when maximize mode starts/stops to
know when to start/stop freely rotating in response to accelerometer updates. I
think this would provide a cleaner separation of responsibilities.

[jonross, 2014-12-10 17:57:30]

ScreenOrientationDelegate has been renamed ScreenOrientationController to
reflect how it will be responsible for screen orientation requests from multiple
sources.

Rotation work has been refactored out of MaximizeModeController and into
ScreenOrientationController,

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
File ash/content/display/screen_orientation_delegate_chromeos.cc (right):

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.cc:11: #include
"ash/wm/maximize_mode/maximize_mode_controller.h"
On 2014/11/27 15:39:15, flackr wrote:
> Obsolete?

Nope, used in ScreenOrientationDelegate::ScreenOrientationProviderSupported()

Currently we only accept the Javascript calls while in TouchView mode.

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.cc:74:
base::AutoReset<bool> auto_ignore_display_configuration_updates(
On 2014/11/27 15:39:15, flackr wrote:
> Can you scope this to just the call which triggers a display configuration
> update?

Done.

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
File ash/content/display/screen_orientation_delegate_chromeos.h (right):

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.h:33: // Invoked
whenever |rotation_locked_| is changed.
On 2014/11/27 15:39:15, flackr wrote:
> Since |rotation_locked_| isn't a member of the observer, describe this by the
> meaning.
> 
> Invoked when rotation is locked or unlocked.

Done.

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos.h:71: //
OnAccelerometerUpdated should not be used to change the rotation.
On 2014/11/27 15:39:15, flackr wrote:
> Can you update this comment? OnAccelerometerUpdated doesn't refer to something
> local.

Done.

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
File ash/content/display/screen_orientation_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/759063002/diff/20001/ash/content/display/scre...
ash/content/display/screen_orientation_delegate_chromeos_unittest.cc:13:
#include "ash/wm/maximize_mode/maximize_mode_controller.h"
On 2014/11/27 15:39:15, flackr wrote:
> Obsolete?

Yup, removed.

https://codereview.chromium.org/759063002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/759063002/diff/20001/ash/shell.cc#newcode488
ash/shell.cc:488: return
!((maximize_mode_controller_->IsMaximizeModeWindowManagerEnabled() &&
On 2014/11/27 15:39:15, flackr wrote:
> Do we need to check if maximize mode window manager is enabled? isn't just the
> ignore_display_configuration_updates sufficient to ignore them?

Yeah the maximize mode check is no longer needed

https://codereview.chromium.org/759063002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/759063002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:210: #if defined(OS_CHROMEOS)
On 2014/11/27 15:39:15, flackr wrote:
> There's a lot of "#if defined(OS_CHROMEOS)" going on here now. Can we make
> screen rotation the responsibility of the increasingly inaccurately named
> ScreenOrientationDelegate (I'd suggest renaming it ScreenOrientationController
> with the responsibilities it would have with this suggestion).
> 
> We could have that class listen to the accelerometer updates and implement all
> of the screen rotation. It could listen to when maximize mode starts/stops to
> know when to start/stop freely rotating in response to accelerometer updates.
I
> think this would provide a cleaner separation of responsibilities.

Done.

[jonross, 2014-12-11 16:49:26]

jonross@chromium.org changed reviewers:
+ oshima@chromium.org

[jonross, 2014-12-11 16:49:27]

Hi Oshima,

I have refactored MaximizeModeController, removing the parts that track
accelerometer based screen rotation.

These have been moved into ScreenOrientationController. This class was
responsible for handling javascript requests to set the screen rotation. I'm
having it now handle both sources.

Could you review this?

Thanks,
Jon

[oshima, 2014-12-11 23:51:28]

https://codereview.chromium.org/759063002/diff/60001/ash/content/display/scre...
File ash/content/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/759063002/diff/60001/ash/content/display/scre...
ash/content/display/screen_orientation_controller_chromeos.h:59: bool
rotation_locked() { return rotation_locked_; }
nit: const

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (left):

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:202: LeaveMaximizeMode();
don't we have to keep this for shutdown?

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:181:
kDeviationFromGravityThreshold;
can you define a utility function in ash/accelerometer and remove dup ?

https://codereview.chromium.org/759063002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/759063002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/display/display_preferences_unittest.cc:45: bool
rotation_locked() {
IsRotationLocked()

[flackr, 2014-12-12 16:19:17]

Can you try git cl upload with a lower similarity so that the moves show as
updates from the original file?

[jonross, 2014-12-12 18:30:05]

I have also uploaded the latest patch with a lower similarity to show the moved
files better.

https://codereview.chromium.org/759063002/diff/60001/ash/content/display/scre...
File ash/content/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/759063002/diff/60001/ash/content/display/scre...
ash/content/display/screen_orientation_controller_chromeos.h:59: bool
rotation_locked() { return rotation_locked_; }
On 2014/12/11 23:51:28, oshima wrote:
> nit: const

Done.

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (left):

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:202: LeaveMaximizeMode();
On 2014/12/11 23:51:28, oshima wrote:
> don't we have to keep this for shutdown?

Rob and I discussed this, and I looked into it.

All of the listeners to OnMaximizeModeEnded perform work which is unnecessary
during shutdown. Mostly layout/visual changes.

We felt that firing this during shutdown was unnecessary.

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:181:
kDeviationFromGravityThreshold;
On 2014/12/11 23:51:28, oshima wrote:
> can you define a utility function in ash/accelerometer and remove dup ?

Done.

https://codereview.chromium.org/759063002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/759063002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/display/display_preferences_unittest.cc:45: bool
rotation_locked() {
On 2014/12/11 23:51:28, oshima wrote:
> IsRotationLocked()

Done.

[oshima, 2014-12-12 19:59:54]

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (left):

https://codereview.chromium.org/759063002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:202: LeaveMaximizeMode();
On 2014/12/12 18:30:04, jonross wrote:
> On 2014/12/11 23:51:28, oshima wrote:
> > don't we have to keep this for shutdown?
> 
> Rob and I discussed this, and I looked into it.
> 
> All of the listeners to OnMaximizeModeEnded perform work which is unnecessary
> during shutdown. Mostly layout/visual changes.
> 
> We felt that firing this during shutdown was unnecessary.

In that case, can you update the document for OnMaximizedModeEnded and say it
won't be called during shutdown ?

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:158: kMeanGravity)
<= kDeviationFromGravityThreshold)) {
this one too?

[flackr, 2014-12-12 20:46:06]

https://codereview.chromium.org/759063002/diff/100001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/759063002/diff/100001/ash/ash.gyp#newcode969
ash/ash.gyp:969: 'content/display/screen_orientation_controller_chromeos.h',
We have to explicitly exclude these files? Shouldn't the filename_rules take
care of this?
https://code.google.com/p/chromium/codesearch#chromium/src/build/filename_rul...

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:77: float
ClockwiseAngleBetweenVectorsInDegrees(const gfx::Vector3dF& base,
These are still used in maximize_mode_controller as well right? Perhaps we
should add these utility function to Vector3dF?

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:257:
Shell::GetInstance()->accelerometer_controller()->AddObserver(this);
Hmm, this means the rotation update will be delayed by another
kDelayBetweenReadsMs (100ms) right? Perhaps we should change the
accelerometer_controller to deliver a cached update on the call to AddObserver
immediately.

https://codereview.chromium.org/759063002/diff/100001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/759063002/diff/100001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:164:
update.has(ui::ACCELEROMETER_SOURCE_SCREEN)) {
nit: already checked that it has source screen on line 157, this can be removed
from this condition.

https://codereview.chromium.org/759063002/diff/100001/ui/accelerometer/accele...
File ui/accelerometer/accelerometer_types.h (right):

https://codereview.chromium.org/759063002/diff/100001/ui/accelerometer/accele...
ui/accelerometer/accelerometer_types.h:71: bool
IsReadingStable(AccelerometerSource source) const;
I don't think this should be a part of the AccelerometerUpdate type. The
threshold is fairly specific to when/how we want to handle updates for maximize
mode and rotation checks.

[jonross, 2015-01-06 19:56:10]

https://codereview.chromium.org/759063002/diff/100001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/759063002/diff/100001/ash/ash.gyp#newcode969
ash/ash.gyp:969: 'content/display/screen_orientation_controller_chromeos.h',
On 2014/12/12 20:46:06, flackr wrote:
> We have to explicitly exclude these files? Shouldn't the filename_rules take
> care of this?
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/filename_rul...

Did not know of those. Removed

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:77: float
ClockwiseAngleBetweenVectorsInDegrees(const gfx::Vector3dF& base,
On 2014/12/12 20:46:06, flackr (OOO) wrote:
> These are still used in maximize_mode_controller as well right? Perhaps we
> should add these utility function to Vector3dF?

Done.

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:158: kMeanGravity)
<= kDeviationFromGravityThreshold)) {
On 2014/12/12 19:59:53, oshima (OOO Dec 18 - Jan 4) wrote:
> this one too?

Done.

https://codereview.chromium.org/759063002/diff/100001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:257:
Shell::GetInstance()->accelerometer_controller()->AddObserver(this);
On 2014/12/12 20:46:06, flackr (OOO) wrote:
> Hmm, this means the rotation update will be delayed by another
> kDelayBetweenReadsMs (100ms) right? Perhaps we should change the
> accelerometer_controller to deliver a cached update on the call to AddObserver
> immediately.

Done.

https://codereview.chromium.org/759063002/diff/100001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/759063002/diff/100001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:164:
update.has(ui::ACCELEROMETER_SOURCE_SCREEN)) {
On 2014/12/12 20:46:06, flackr (OOO) wrote:
> nit: already checked that it has source screen on line 157, this can be
removed
> from this condition.

Done.

https://codereview.chromium.org/759063002/diff/100001/ui/accelerometer/accele...
File ui/accelerometer/accelerometer_types.h (right):

https://codereview.chromium.org/759063002/diff/100001/ui/accelerometer/accele...
ui/accelerometer/accelerometer_types.h:71: bool
IsReadingStable(AccelerometerSource source) const;
On 2014/12/12 20:46:06, flackr wrote:
> I don't think this should be a part of the AccelerometerUpdate type. The
> threshold is fairly specific to when/how we want to handle updates for
maximize
> mode and rotation checks.

Moved to chromeos/accelerometer/accelerometer_reader to be a helper method for
the ChromeOS specific use case.

[oshima, 2015-01-09 21:23:56]

lgtm with nits.

By the way, what'd happen if I entered to docked mode while the maximized mode
is active?

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:128:
HandleScreenRotation(update.get(ui::ACCELEROMETER_SOURCE_SCREEN));
optional: or you can do

if (IsStable) {
  HandleScreenRotation()
}

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.h:47: virtual
~ScreenOrientationController();
nit: override

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:83:
virtual ~ScreenOrientationControllerTest();
nit: override

https://codereview.chromium.org/759063002/diff/180001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/759063002/diff/180001/ash/shell.cc#newcode702
ash/shell.cc:702: maximize_mode_controller_.reset();
I believe this is cros specific too right? not in this cl, but we should exclude
this too.

https://codereview.chromium.org/759063002/diff/180001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/759063002/diff/180001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.h:14: #include
"ui/gfx/geometry/vector3d_f.h"
forward decl?

[oshima, 2015-01-09 21:28:40]

On 2015/01/09 21:23:56, oshima wrote:
> lgtm with nits.
> 
> By the way, what'd happen if I entered to docked mode while the maximized mode
> is active?

Never mind. You can't close the lid while the lid is flipped. :)

[jonross, 2015-01-12 14:46:49]

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.cc:128:
HandleScreenRotation(update.get(ui::ACCELEROMETER_SOURCE_SCREEN));
On 2015/01/09 21:23:55, oshima wrote:
> optional: or you can do
> 
> if (IsStable) {
>   HandleScreenRotation()
> }

Done.

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos.h:47: virtual
~ScreenOrientationController();
On 2015/01/09 21:23:55, oshima wrote:
> nit: override

Done.

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/759063002/diff/180001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:83:
virtual ~ScreenOrientationControllerTest();
On 2015/01/09 21:23:55, oshima wrote:
> nit: override

Done.

https://codereview.chromium.org/759063002/diff/180001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/759063002/diff/180001/ash/shell.cc#newcode702
ash/shell.cc:702: maximize_mode_controller_.reset();
On 2015/01/09 21:23:55, oshima wrote:
> I believe this is cros specific too right? not in this cl, but we should
exclude
> this too.

It only performs any actions on cros.

I can update it in a separate review.

https://codereview.chromium.org/759063002/diff/180001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/759063002/diff/180001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.h:14: #include
"ui/gfx/geometry/vector3d_f.h"
On 2015/01/09 21:23:55, oshima wrote:
> forward decl?

Removed

[jonross, 2015-01-12 14:48:41]

jonross@chromium.org changed reviewers:
+ vollick@chromium.org

[jonross, 2015-01-12 14:48:42]

vollick@chromium.org: Please review changes in 

ui/fx/geometry/vector3d_f.h/cc

I've added a helper method for determining the angular differences between
vectors.

Thanks,
Jon

[jonross, 2015-01-12 14:48:43]

vollick@chromium.org: Please review changes in 

ui/fx/geometry/vector3d_f.h/cc

I've added a helper method for determining the angular differences between
vectors.

Thanks,
Jon

[jonross, 2015-01-12 14:48:45]

vollick@chromium.org: Please review changes in 

ui/fx/geometry/vector3d_f.h/cc

I've added a helper method for determining the angular differences between
vectors.

Thanks,
Jon

[Ian Vollick, 2015-01-12 15:34:57]

On 2015/01/12 14:48:45, jonross wrote:
> mailto:vollick@chromium.org: Please review changes in 
> 
> ui/fx/geometry/vector3d_f.h/cc
> 
> I've added a helper method for determining the angular differences between
> vectors.
> 
> Thanks,
> Jon

I like the changes to vector3d_f, but you should totally add some unit tests to
vector3d_unittests.

[jonross, 2015-01-12 17:59:06]

On 2015/01/12 15:34:57, vollick wrote:
> On 2015/01/12 14:48:45, jonross wrote:
> > mailto:vollick@chromium.org: Please review changes in 
> > 
> > ui/fx/geometry/vector3d_f.h/cc
> > 
> > I've added a helper method for determining the angular differences between
> > vectors.
> > 
> > Thanks,
> > Jon
> 
> I like the changes to vector3d_f, but you should totally add some unit tests
to
> vector3d_unittests.

Unit tests added.

[jonross, 2015-01-12 19:21:23]

On 2015/01/12 17:59:06, jonross wrote:
> On 2015/01/12 15:34:57, vollick wrote:
> > On 2015/01/12 14:48:45, jonross wrote:
> > > mailto:vollick@chromium.org: Please review changes in 
> > > 
> > > ui/fx/geometry/vector3d_f.h/cc
> > > 
> > > I've added a helper method for determining the angular differences between
> > > vectors.
> > > 
> > > Thanks,
> > > Jon
> > 
> > I like the changes to vector3d_f, but you should totally add some unit tests
> to
> > vector3d_unittests.
> 
> Unit tests added.

As discussed offline, tests have been added for the inverse of each function.

[Ian Vollick, 2015-01-12 19:28:39]

On 2015/01/12 19:21:23, jonross wrote:
> On 2015/01/12 17:59:06, jonross wrote:
> > On 2015/01/12 15:34:57, vollick wrote:
> > > On 2015/01/12 14:48:45, jonross wrote:
> > > > mailto:vollick@chromium.org: Please review changes in 
> > > > 
> > > > ui/fx/geometry/vector3d_f.h/cc
> > > > 
> > > > I've added a helper method for determining the angular differences
between
> > > > vectors.
> > > > 
> > > > Thanks,
> > > > Jon
> > > 
> > > I like the changes to vector3d_f, but you should totally add some unit
tests
> > to
> > > vector3d_unittests.
> > 
> > Unit tests added.
> 
> As discussed offline, tests have been added for the inverse of each function.

thanks, lgtm!

[jonross, 2015-01-12 19:29:03]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2015-01-12 19:30:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759063002/240001

[commit-bot: I haz the power, 2015-01-12 19:35:20]

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

[commit-bot: I haz the power, 2015-01-12 19:35:22]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jonross, 2015-01-13 14:49:38]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2015-01-13 14:50:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759063002/260001

[commit-bot: I haz the power, 2015-01-13 15:04:30]

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

[commit-bot: I haz the power, 2015-01-13 15:04:37]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jonross, 2015-01-13 17:54:42]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2015-01-13 17:56:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759063002/280001

[commit-bot: I haz the power, 2015-01-13 18:56:18]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-01-13 18:57:14]

Patchset 14 (id:??) landed as
https://crrev.com/0af4521339c7ca15535ea92dd4c27f94610b9a67
Cr-Commit-Position: refs/heads/master@{#311299}