Correcly Report CanEnterMaximizeMode on devices without accelerometers

Correcly Report CanEnterMaximizeMode on devices withou accelerometers

MaximizeModeController::CanEnterMaximizeMode was based on receiving an accelerometer event. AccelerometerReader now sends an event to every observer as it is added, to reduce lag on receiving first event. However on devices without accelerometers this sends an empty event.

Updated MaximizeModeController to not set that accelerometer events have been seen until a valid event is received. One with actual data.

TEST=MaximizeModeControllerTest.CanEnterMaximizeModeRequiresValidAccelerometerUpdate
BUG=454662
Committed: https://crrev.com/1418c1251a9d1ca13e2e1afb84d31891fe77e80e
Cr-Commit-Position: refs/heads/master@{#315307}

[jonross, 2015-02-03 22:22:43]

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

[jonross, 2015-02-03 22:22:44]

Hi Rob,

Can you review this change?

I'm updating the logic for CanEnterMaximizeMode to account for receiving empty
accelerometer events on devices without accelerometers.

Thanks,
Jon

[flackr, 2015-02-03 22:32:11]

Wouldn't it be better to not send an update immediately if we haven't received
any accelerometer data yet?

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

https://codereview.chromium.org/895993002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:145: EnterMaximizeMode();
Seems like this path (entering maximize mode when only having a screen
accelerometer) should have been failing and caught by a test but wasn't. Can you
add a test to this effect?

[jonross, 2015-02-04 18:31:23]

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

https://codereview.chromium.org/895993002/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:145: EnterMaximizeMode();
On 2015/02/03 22:32:11, flackr wrote:
> Seems like this path (entering maximize mode when only having a screen
> accelerometer) should have been failing and caught by a test but wasn't. Can
you
> add a test to this effect?

Done.

[flackr, 2015-02-04 18:44:52]

https://codereview.chromium.org/895993002/diff/20001/chromeos/accelerometer/a...
File chromeos/accelerometer/accelerometer_reader.cc (right):

https://codereview.chromium.org/895993002/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_reader.cc:207:
update_.has(chromeos::ACCELEROMETER_SOURCE_SCREEN)) {
I'd prefer adding a bool for if it has gotten a reading yet (i.e. if
AccelerometerReader::OnDataRead has been called with success == true).

[jonross, 2015-02-04 19:00:59]

https://codereview.chromium.org/895993002/diff/20001/chromeos/accelerometer/a...
File chromeos/accelerometer/accelerometer_reader.cc (right):

https://codereview.chromium.org/895993002/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_reader.cc:207:
update_.has(chromeos::ACCELEROMETER_SOURCE_SCREEN)) {
On 2015/02/04 18:44:52, flackr wrote:
> I'd prefer adding a bool for if it has gotten a reading yet (i.e. if
> AccelerometerReader::OnDataRead has been called with success == true).

Done.

[flackr, 2015-02-06 20:18:21]

LGTM with nits

https://codereview.chromium.org/895993002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/895993002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:349: // Tests that
when and empty accelerometer update is received for attaching as a
nit s/and/an
Also comment is stale, we don't receive an empty update anymore..

https://codereview.chromium.org/895993002/diff/40001/chromeos/accelerometer/a...
File chromeos/accelerometer/accelerometer_reader.h (right):

https://codereview.chromium.org/895993002/diff/40001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_reader.h:93: // True if OnDataRead has been
called with success == true, and observers can
nit: Observers can be notified is implementation details. This really just means
true if update_ has a valid accelerometer update. Could even call it
has_update_.

[jonross, 2015-02-09 14:54:05]

New patchsets have been uploaded after l-g-t-m from flackr@chromium.org

[jonross, 2015-02-09 14:54:48]

https://codereview.chromium.org/895993002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/895993002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:349: // Tests that
when and empty accelerometer update is received for attaching as a
On 2015/02/06 20:18:20, flackr wrote:
> nit s/and/an
> Also comment is stale, we don't receive an empty update anymore..

Done.

https://codereview.chromium.org/895993002/diff/40001/chromeos/accelerometer/a...
File chromeos/accelerometer/accelerometer_reader.h (right):

https://codereview.chromium.org/895993002/diff/40001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_reader.h:93: // True if OnDataRead has been
called with success == true, and observers can
On 2015/02/06 20:18:20, flackr wrote:
> nit: Observers can be notified is implementation details. This really just
means
> true if update_ has a valid accelerometer update. Could even call it
> has_update_.

Done.

[jonross, 2015-02-09 14:55:31]

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

[jonross, 2015-02-09 14:55:32]

oshima@chromium.org: Please review changes in 

chromeos/accelerometer/

[oshima, 2015-02-09 15:17:29]

lgtm

[jonross, 2015-02-09 15:18:28]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2015-02-09 15:19:45]

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

[commit-bot: I haz the power, 2015-02-09 15:52:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-09 15:53:39]

Patchset 4 (id:??) landed as
https://crrev.com/1418c1251a9d1ca13e2e1afb84d31891fe77e80e
Cr-Commit-Position: refs/heads/master@{#315307}