Refactor AccelerometerReader to provide an Observer

Refactor AccelerometerReader to provide an Observer

Change chromeos::AccelerometerReader from using a single delegate, to using observers. We need to begin listening to it from both ash/ and content/

Removed the abstraction ash::AccelerometerController, and updated all locations to use the reader directly.

TEST=MaximizeModeControllerTes
BUG=431865
Committed: https://crrev.com/8834fc4b4fe4997dd9b991ce5984e0d7e157a4d4
Cr-Commit-Position: refs/heads/master@{#309219}

[jonross, 2014-12-11 23:09:27]

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

[jonross, 2014-12-11 23:09:27]

https://codereview.chromium.org/795333002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/795333002/diff/1/ash/shell.cc#newcode647
ash/shell.cc:647:
scoped_ptr<::wm::NativeCursorManager>(native_cursor_manager_)),
cpplint

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:204: void
MaximizeModeController::OnDisplayConfigurationChanged() {
I actually moved OnAccelerometerUpdated down into the CHROME_OS block. No code
changes.

[flackr, 2014-12-12 16:47:11]

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:204: void
MaximizeModeController::OnDisplayConfigurationChanged() {
On 2014/12/11 23:09:27, jonross wrote:
> I actually moved OnAccelerometerUpdated down into the CHROME_OS block. No code
> changes.

Acknowledged.

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.h:125: //
AccelerometerReader::Observer:
chromeos::AccelerometerReader::Observer:

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:74: #if
defined(OS_CHROMEOS)
Hmm, I think it's more readable to match this to the #if defined(OS_CHROMEOS)
around just the accelerometer_reader access in the SetUp rather than ifdef the
entire TearDown with only part of the SetUp ifdef'd

https://codereview.chromium.org/795333002/diff/1/chrome/browser/ui/ash/ash_in...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/795333002/diff/1/chrome/browser/ui/ash/ash_in...
chrome/browser/ui/ash/ash_init.cc:67: shell->accelerometer_reader()->Initialize(
move to #if defined(OS_CHROMEOS)?

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
File chromeos/accelerometer/accelerometer_reader.cc (right):

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
chromeos/accelerometer/accelerometer_reader.cc:197: task_runner_ =
blocking_task_runner.get();
I think this can just be task_runner_ = blocking_task_runner

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co...

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
File chromeos/accelerometer/accelerometer_reader.h (right):

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
chromeos/accelerometer/accelerometer_reader.h:89:
base::WeakPtrFactory<AccelerometerReader> weak_factory_;
Keep this as the last member of the class. Members are destructed in reverse
declaration order and this helps ensure that this class will not be used as soon
as it starts to be destructed.

[jonross, 2014-12-12 19:03:25]

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.h:125: //
AccelerometerReader::Observer:
On 2014/12/12 16:47:10, flackr wrote:
> chromeos::AccelerometerReader::Observer:

Done.

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/795333002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:74: #if
defined(OS_CHROMEOS)
On 2014/12/12 16:47:10, flackr wrote:
> Hmm, I think it's more readable to match this to the #if defined(OS_CHROMEOS)
> around just the accelerometer_reader access in the SetUp rather than ifdef the
> entire TearDown with only part of the SetUp ifdef'd

Done.

https://codereview.chromium.org/795333002/diff/1/chrome/browser/ui/ash/ash_in...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/795333002/diff/1/chrome/browser/ui/ash/ash_in...
chrome/browser/ui/ash/ash_init.cc:67: shell->accelerometer_reader()->Initialize(
On 2014/12/12 16:47:10, flackr wrote:
> move to #if defined(OS_CHROMEOS)?

oops.

Done.

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
File chromeos/accelerometer/accelerometer_reader.cc (right):

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
chromeos/accelerometer/accelerometer_reader.cc:197: task_runner_ =
blocking_task_runner.get();
On 2014/12/12 16:47:11, flackr wrote:
> I think this can just be task_runner_ = blocking_task_runner
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co...

Done.

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
File chromeos/accelerometer/accelerometer_reader.h (right):

https://codereview.chromium.org/795333002/diff/1/chromeos/accelerometer/accel...
chromeos/accelerometer/accelerometer_reader.h:89:
base::WeakPtrFactory<AccelerometerReader> weak_factory_;
On 2014/12/12 16:47:11, flackr wrote:
> Keep this as the last member of the class. Members are destructed in reverse
> declaration order and this helps ensure that this class will not be used as
soon
> as it starts to be destructed.

Done.

[flackr, 2014-12-15 13:52:42]

lgtm

[jonross, 2014-12-16 14:38:37]

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

[jonross, 2014-12-16 14:38:37]

Hi Oshima,

I am working towards moving accelerometer api into content, while the hardware
reading would remain in chromeos.

In this change I move the observer into chromeos from ash.

Could you provide a review?

Thanks,
Jon

[oshima, 2014-12-16 16:50:54]

lgtm

https://codereview.chromium.org/795333002/diff/20001/athena/system/orientatio...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/795333002/diff/20001/athena/system/orientatio...
athena/system/orientation_controller.cc:32:
OrientationController::~OrientationController() {
how about

DCHECK_FALSE(accelerometer_reader_.get())

?

[jonross, 2014-12-19 02:03:42]

https://codereview.chromium.org/795333002/diff/20001/athena/system/orientatio...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/795333002/diff/20001/athena/system/orientatio...
athena/system/orientation_controller.cc:32:
OrientationController::~OrientationController() {
On 2014/12/16 16:50:54, oshima wrote:
> how about
> 
> DCHECK_FALSE(accelerometer_reader_.get())
> 
> ?

Done.

[jonross, 2014-12-19 02:03:47]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-12-19 02:07:38]

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

[commit-bot: I haz the power, 2014-12-19 02:38:04]

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

[commit-bot: I haz the power, 2014-12-19 02:38:05]

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

[jonross, 2014-12-19 03:53:45]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-12-19 03:54:53]

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

[commit-bot: I haz the power, 2014-12-19 04:02:44]

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

[commit-bot: I haz the power, 2014-12-19 04:02:45]

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

[jonross, 2014-12-19 14:48:39]

Rob,

With the accelerometer being ChromeOS only, the MaximizeModeControllerTest can
no longer be ran on non ChromeOS.

I've updated the builds to reflect this. I've also removed the #ifdef is the
test file.

Mind taking a look at them?

[flackr, 2014-12-19 15:43:53]

slgtm

https://codereview.chromium.org/795333002/diff/80001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/795333002/diff/80001/ash/BUILD.gn#newcode282
ash/BUILD.gn:282: # Acceleromter is only available on Chrome OS.
nit: Accelerometer

[jonross, 2014-12-19 15:45:34]

The CQ bit was checked by jonross@chromium.org

[jonross, 2014-12-19 15:45:40]

https://codereview.chromium.org/795333002/diff/80001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/795333002/diff/80001/ash/BUILD.gn#newcode282
ash/BUILD.gn:282: # Acceleromter is only available on Chrome OS.
On 2014/12/19 15:43:53, flackr wrote:
> nit: Accelerometer

Done.

[commit-bot: I haz the power, 2014-12-19 15:45:48]

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

[commit-bot: I haz the power, 2014-12-19 16:18:50]

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

[commit-bot: I haz the power, 2014-12-19 16:18:50]

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

[jonross, 2014-12-19 16:33:00]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-12-19 16:33:19]

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

[commit-bot: I haz the power, 2014-12-19 17:29:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-19 17:30:38]

Patchset 7 (id:??) landed as
https://crrev.com/8834fc4b4fe4997dd9b991ce5984e0d7e157a4d4
Cr-Commit-Position: refs/heads/master@{#309219}