Use standardized and extendable accelerometer update type.

Use standardized and extendable accelerometer update type.

Updates the type used when delivering accelerometer updates in ash to:
- support a variable number of accelerometers
- be measured in m/s^2
- use axes consistent with the web device motion API.

BUG=380831
TEST=MaximizeModeController unit tests still pass.
TEST=Run on a touchview device and verify that entering / exiting touchview as well as screen rotation still works.

Committed: https://crrev.com/45f31ae7bb08076b4f696d0d5567411e82e49782
Cr-Commit-Position: refs/heads/master@{#292968}

[flackr, 2014-08-27 19:04:33]

Patchset #1 (id:1) has been deleted

[flackr, 2014-08-27 19:05:25]

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

[flackr, 2014-08-27 19:09:13]

Dan, this is part of my effort to be able to support various device form factors
from chromeos/accelerometer/. Please take a look and let me know what you think,
thanks! I've scaled up most of the "fuzzy" constants by a factor of 10 since
they weren't exact in the first place so that they're still simple numbers and
only slightly more than 9.86 times the original.

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

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:447: // data to
accelerometer updates consistent with accelerometer_reader.cc.
Rather than completely update the file, I did the conversion here. Let me know
if you think it'd be better to update the literals file.

[Daniel Erat, 2014-08-28 00:27:07]

https://codereview.chromium.org/500613003/diff/20001/ash/accelerometer/accele...
File ash/accelerometer/accelerometer_observer.h (right):

https://codereview.chromium.org/500613003/diff/20001/ash/accelerometer/accele...
ash/accelerometer/accelerometer_observer.h:16: // Invoked when an accelerometer
reading has been taken. The |base| and |lid|
nit: update this comment

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

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:59: // The mean acceleration
due to gravity on Earth.
nit: describe units

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:68: const float
kHingeAngleDetectionThreshold = 2.5f;
please mention units in the comments for all of these constants too

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:341: bool is_angle_stable =
lid_angle > kMinStableAngle &&
the constant names suggest that these should be >= and <=

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:355: lid_angle <
kExitMaximizeModeAngle) {
<= ?

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:358: lid_angle >
kEnterMaximizeModeAngle &&
>= ?

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:390: static gfx::Vector3dF
rotation_reference(-1.0f, -1.0f, 0.0f);
const?

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

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_reader.cc:56: const float kMeanGravity =
9.80665f;
heh, nice constant :-P

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
File chromeos/accelerometer/accelerometer_types.h (right):

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_types.h:33: AccelerometerReading() :
present(false) {}
i suspect that this struct needs an explicit c'tor and d'tor in a .cc file:
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_types.h:46: AccelerometerUpdate() {}
same here

[flackr, 2014-08-28 01:21:37]

https://codereview.chromium.org/500613003/diff/20001/ash/accelerometer/accele...
File ash/accelerometer/accelerometer_observer.h (right):

https://codereview.chromium.org/500613003/diff/20001/ash/accelerometer/accele...
ash/accelerometer/accelerometer_observer.h:16: // Invoked when an accelerometer
reading has been taken. The |base| and |lid|
On 2014/08/28 00:27:06, Daniel Erat wrote:
> nit: update this comment

Done.

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

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:59: // The mean acceleration
due to gravity on Earth.
On 2014/08/28 00:27:06, Daniel Erat wrote:
> nit: describe units

Done.

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:68: const float
kHingeAngleDetectionThreshold = 2.5f;
On 2014/08/28 00:27:06, Daniel Erat wrote:
> please mention units in the comments for all of these constants too

Done.

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:341: bool is_angle_stable =
lid_angle > kMinStableAngle &&
On 2014/08/28 00:27:06, Daniel Erat wrote:
> the constant names suggest that these should be >= and <=

Done.

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:355: lid_angle <
kExitMaximizeModeAngle) {
On 2014/08/28 00:27:06, Daniel Erat wrote:
> <= ?

Done.

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:358: lid_angle >
kEnterMaximizeModeAngle &&
On 2014/08/28 00:27:06, Daniel Erat wrote:
> >= ?

Done.

https://codereview.chromium.org/500613003/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:390: static gfx::Vector3dF
rotation_reference(-1.0f, -1.0f, 0.0f);
On 2014/08/28 00:27:06, Daniel Erat wrote:
> const?

Done.

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

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_reader.cc:56: const float kMeanGravity =
9.80665f;
On 2014/08/28 00:27:06, Daniel Erat wrote:
> heh, nice constant :-P

=) added units.

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
File chromeos/accelerometer/accelerometer_types.h (right):

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_types.h:33: AccelerometerReading() :
present(false) {}
On 2014/08/28 00:27:07, Daniel Erat wrote:
> i suspect that this struct needs an explicit c'tor and d'tor in a .cc file:
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

Done.

https://codereview.chromium.org/500613003/diff/20001/chromeos/accelerometer/a...
chromeos/accelerometer/accelerometer_types.h:46: AccelerometerUpdate() {}
On 2014/08/28 00:27:07, Daniel Erat wrote:
> same here

Done.

[Daniel Erat, 2014-08-28 14:42:32]

lgtm

[flackr, 2014-08-28 15:09:36]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-08-28 15:10:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/500613003/60001

[commit-bot: I haz the power, 2014-08-28 16:05:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-28 16:18:47]

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

[commit-bot: I haz the power, 2014-08-28 16:18:48]

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

[flackr, 2014-08-28 18:39:39]

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

[flackr, 2014-08-28 18:39:39]

Hi Scott,

In this patch I'm creating a type to use for accelerometer updates. Since we
compile MaximizeModeController on windows as well though the type used for these
updates needs to be available on windows.

In patchset #4 I moved the AccelerometerUpdate type to ui/base/ but found out
that ui/base/ime (compiled as part of ui/base/ui_base.gyp:ui_base) depends on
chromeos/ so this would introduce a circular dependency. Do you think I should
move ui/base/ime to its own location (ui/ime maybe?), or put accelerometer_types
into ui/accelerometer or somewhere else?

Thanks for having a look!

[sky, 2014-08-28 23:03:53]

On 2014/08/28 18:39:39, flackr wrote:
> Hi Scott,
> 
> In this patch I'm creating a type to use for accelerometer updates. Since we
> compile MaximizeModeController on windows as well though the type used for
these
> updates needs to be available on windows.
> 
> In patchset #4 I moved the AccelerometerUpdate type to ui/base/ but found out
> that ui/base/ime (compiled as part of ui/base/ui_base.gyp:ui_base) depends on
> chromeos/ so this would introduce a circular dependency. Do you think I should
> move ui/base/ime to its own location (ui/ime maybe?), or put
accelerometer_types
> into ui/accelerometer or somewhere else?
> 
> Thanks for having a look!

I'm not opposed to that. You'll have to straighten out a couple of includes in
ime that include ui/base/foo, but it should be possible. Alternatively you could
create ui/accelerometer.

[flackr, 2014-08-29 01:55:08]

I looked into moving ui/base/ime but I'd rather not block this CL on that after
seeing how many places must be updated. I've created ui/accelerometer instead as
suggested. PTAL, thanks.

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/ui_acc...
File ui/accelerometer/ui_accelerometer.gyp (right):

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/ui_acc...
ui/accelerometer/ui_accelerometer.gyp:11: # GN version: //ui/accelerometer
Do I need to write a BUILD.gn?

[sky, 2014-08-29 02:15:27]

https://codereview.chromium.org/500613003/diff/100001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (left):

https://codereview.chromium.org/500613003/diff/100001/chromeos/chromeos.gyp#o...
chromeos/chromeos.gyp:31: '../ui/gfx/gfx.gyp:gfx_geometry',
Is no one using this dependency anymore?

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

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/accele...
ui/accelerometer/accelerometer_types.h:17: // position. Positive Y points up the
screen. Positive Z is perpendicular to
Don't we normally have positive y going down?

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/ui_acc...
File ui/accelerometer/ui_accelerometer.gyp (right):

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/ui_acc...
ui/accelerometer/ui_accelerometer.gyp:11: # GN version: //ui/accelerometer
On 2014/08/29 01:55:07, flackr wrote:
> Do I need to write a BUILD.gn?

Yes. You should add a BUILD.gn file.

[flackr, 2014-08-29 03:09:53]

Verified that I could use gn to build linux chrome, as far as I can tell gn
doesn't do linux chromeos yet?

https://codereview.chromium.org/500613003/diff/100001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (left):

https://codereview.chromium.org/500613003/diff/100001/chromeos/chromeos.gyp#o...
chromeos/chromeos.gyp:31: '../ui/gfx/gfx.gyp:gfx_geometry',
On 2014/08/29 02:15:27, sky wrote:
> Is no one using this dependency anymore?

Nope, it was added to use gfx::Vector3dF. I've checked that there's no remaining
includes of this.

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

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/accele...
ui/accelerometer/accelerometer_types.h:17: // position. Positive Y points up the
screen. Positive Z is perpendicular to
On 2014/08/29 02:15:27, sky wrote:
> Don't we normally have positive y going down?

Normally we do at least in pixel space, but not in the device orientation spec
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...
or Android SensorEvents
http://developer.android.com/reference/android/hardware/SensorEvent.html.

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/ui_acc...
File ui/accelerometer/ui_accelerometer.gyp (right):

https://codereview.chromium.org/500613003/diff/100001/ui/accelerometer/ui_acc...
ui/accelerometer/ui_accelerometer.gyp:11: # GN version: //ui/accelerometer
On 2014/08/29 02:15:27, sky wrote:
> On 2014/08/29 01:55:07, flackr wrote:
> > Do I need to write a BUILD.gn?
> 
> Yes. You should add a BUILD.gn file.

Done.

[sky, 2014-09-02 15:21:07]

LGTM

[flackr, 2014-09-02 16:29:59]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-09-02 16:30:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/500613003/120001

[commit-bot: I haz the power, 2014-09-02 17:29:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-02 17:32:49]

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

[commit-bot: I haz the power, 2014-09-02 17:32:51]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[flackr, 2014-09-02 17:53:32]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-09-02 17:54:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/500613003/140001

[commit-bot: I haz the power, 2014-09-02 18:51:23]

Committed patchset #7 (id:140001) as 6c194f2bdff9ce41849f60ef88c4d6c48d1f83c3

[commit-bot: I haz the power, 2014-09-10 03:19:46]

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

[stevenjb, 2014-11-06 16:38:36]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2014-11-06 16:38:37]

https://codereview.chromium.org/500613003/diff/140001/chromeos/DEPS
File chromeos/DEPS (right):

https://codereview.chromium.org/500613003/diff/140001/chromeos/DEPS#newcode8
chromeos/DEPS:8: "+ui/accelerometer",
I just ran across this and have to say I am not a fan of having ui/ dependencies
in chromeos/. The intention of chromeos/ was to be a low level system library,
any dependencies should be the other way around. Is there an easy way to fix
this?

[flackr, 2014-11-06 16:54:01]

https://codereview.chromium.org/500613003/diff/140001/chromeos/DEPS
File chromeos/DEPS (right):

https://codereview.chromium.org/500613003/diff/140001/chromeos/DEPS#newcode8
chromeos/DEPS:8: "+ui/accelerometer",
On 2014/11/06 16:38:37, stevenjb wrote:
> I just ran across this and have to say I am not a fan of having ui/
dependencies
> in chromeos/. The intention of chromeos/ was to be a low level system library,
> any dependencies should be the other way around. Is there an easy way to fix
> this?

We could have a special type for the updates returned from chromeos/ and then
translate to some other type in ash/accelerometer/accelerometer_reader, but
duplicating all of the information in the type seems suboptimal. Are there any
better suited locations for defining a common type we want to share between
chromeos/ and ui code?

[stevenjb, 2014-11-06 17:14:51]

We could put the common types in a component, or do the extra translation
as you suggest, I would be fine either way.

The reason I dislike the src/ui dependency in src/chromeos is that if we
want to use the chromeos library in a non UI module (e.g. a hypothetical
headless network manager for Chromecast), we end up pulling in a bunch of
UI code that we do not need.


On Thu, Nov 6, 2014 at 9:54 AM, <flackr@chromium.org> wrote:

> Reviewers: Daniel Erat, sky, stevenjb,
>
>
> https://codereview.chromium.org/500613003/diff/140001/chromeos/DEPS
> File chromeos/DEPS (right):
>
> https://codereview.chromium.org/500613003/diff/140001/
> chromeos/DEPS#newcode8
> chromeos/DEPS:8: "+ui/accelerometer",
> On 2014/11/06 16:38:37, stevenjb wrote:
>
>> I just ran across this and have to say I am not a fan of having ui/
>>
> dependencies
>
>> in chromeos/. The intention of chromeos/ was to be a low level system
>>
> library,
>
>> any dependencies should be the other way around. Is there an easy way
>>
> to fix
>
>> this?
>>
>
> We could have a special type for the updates returned from chromeos/ and
> then translate to some other type in
> ash/accelerometer/accelerometer_reader, but duplicating all of the
> information in the type seems suboptimal. Are there any better suited
> locations for defining a common type we want to share between chromeos/
> and ui code?
>
> Description:
> Use standardized and extendable accelerometer update type.
>
> Updates the type used when delivering accelerometer updates in ash to:
> - support a variable number of accelerometers
> - be measured in m/s^2
> - use axes consistent with the web device motion API.
>
> BUG=380831
> TEST=MaximizeModeController unit tests still pass.
> TEST=Run on a touchview device and verify that entering / exiting
> touchview as
> well as screen rotation still works.
>
> Committed: https://crrev.com/45f31ae7bb08076b4f696d0d5567411e82e49782
> Cr-Commit-Position: refs/heads/master@{#292968}
>
> Please review this at https://codereview.chromium.org/500613003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+382, -195 lines):
>   M ash/BUILD.gn
>   M ash/accelerometer/accelerometer_controller.h
>   M ash/accelerometer/accelerometer_controller.cc
>   M ash/accelerometer/accelerometer_observer.h
>   M ash/ash.gyp
>   M ash/wm/maximize_mode/maximize_mode_controller.h
>   M ash/wm/maximize_mode/maximize_mode_controller.cc
>   M ash/wm/maximize_mode/maximize_mode_controller_unittest.cc
>   M chrome/browser/chromeos/display/display_preferences_unittest.cc
>   M chrome/chrome_tests_unit.gypi
>   M chromeos/DEPS
>   M chromeos/accelerometer/accelerometer_reader.h
>   M chromeos/accelerometer/accelerometer_reader.cc
>   M chromeos/chromeos.gyp
>   A + ui/accelerometer/BUILD.gn
>   A ui/accelerometer/DEPS
>   A ui/accelerometer/accelerometer_types.h
>   A ui/accelerometer/accelerometer_types.cc
>   A + ui/accelerometer/ui_accelerometer.gyp
>   A ui/accelerometer/ui_accelerometer_export.h
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.