Initial sensor event provider for testing views screen rotation

Initial sensor event provider for testing views screen rotation

The sensors API will unify various types of sensor data into a set of channels, each of which provides change events and periodic updates.

This version of the API is intended only to support the experimental screen rotation code and is not for general use. In particular, the final listener will declare generic |OnSensorChanged| and |OnSensorUpdated| methods, rather than the special-purpose |OnScreenOrientationChanged|.

BUG=none
TEST=manual with http://codereview.chromium.org/7273073/


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93094

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93480

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94181

[cwolfe, 2011-07-14 17:28:03]

sadrul: Please take a pass through. If there are no outside objections, can you
please try+submit this for me.

jam: Grabbed you from the content/ OWNERS. Mainly want to check that I'm in the
right part of the tree, as there's a more complete design to come later. 

vollick: You're the one testing everything.

This is a very preliminary API. I expect, at least, the listener methods to
change in future iterations. Will get a wider spread of reviewers, including the
device_orientation folks, once more of the structure is in place.

[Ian Vollick, 2011-07-15 14:21:57]

LGTM

On 2011/07/14 17:28:03, cwolfe wrote:
> sadrul: Please take a pass through. If there are no outside objections, can
you
> please try+submit this for me.
> 
> jam: Grabbed you from the content/ OWNERS. Mainly want to check that I'm in
the
> right part of the tree, as there's a more complete design to come later. 
> 
> vollick: You're the one testing everything.
> 
> This is a very preliminary API. I expect, at least, the listener methods to
> change in future iterations. Will get a wider spread of reviewers, including
the
> device_orientation folks, once more of the structure is in place.

[sadrul, 2011-07-15 15:12:26]

On 2011/07/14 17:28:03, cwolfe wrote:
> sadrul: Please take a pass through. If there are no outside objections, can
you
> please try+submit this for me.

Left some comments. Looks good. I will submit a tryjob for it now. I can land it
once OWNERS approve.

> jam: Grabbed you from the content/ OWNERS. Mainly want to check that I'm in
the
> right part of the tree, as there's a more complete design to come later. 
> 
> vollick: You're the one testing everything.
> 
> This is a very preliminary API. I expect, at least, the listener methods to
> change in future iterations. Will get a wider spread of reviewers, including
the
> device_orientation folks, once more of the structure is in place.

[sadrul, 2011-07-15 15:12:35]

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
File content/browser/sensors/provider.h (right):

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
content/browser/sensors/provider.h:45: virtual void
FireScreenOrientationChange(const ScreenOrientation& change) = 0;
Perhaps 'ScreenOrientationChanged' goes better with the rest of the code.

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
File content/browser/sensors/provider_impl.cc (right):

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
content/browser/sensors/provider_impl.cc:16: : listeners_(new ListenerList()) {
4-space indent

[sadrul, 2011-07-15 15:32:17]

Question: is it possible to query the current orientation? Or does a Listener
have to remember the state on its own?

[cwolfe, 2011-07-15 16:13:54]

Comments applied.

sadrul wrote:
> Question: is it possible to query the current orientation?
> Or does a Listener have to remember the state on its own?

There is not currently a way to query the orientation. Remembering doesn't quite
solve the problem either: when a listener is added it does not have any way to
know the orientation until a change occurs.

For the purposes of vollick's demo, this doesn't look like a problem: the
listener is added before any change can occur and everything defaults to
right-side-up.

I've been pondering whether the Provider should record the most recent change,
and deliver it immediately to an added listener (before AddListener returns).
That would make sure the listeners get initialized properly, without needing a
separate polling API.

Held off on adding that behavior because I am not yet certain it is safe for the
generalized version. There the Provider won't have any channel-specific code,
just seeing a change as a channel number and sized blob. Want to make sure that
stale changes have reasonable semantics before relying on them.

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
File content/browser/sensors/provider.h (right):

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
content/browser/sensors/provider.h:45: virtual void
FireScreenOrientationChange(const ScreenOrientation& change) = 0;
On 2011/07/15 15:12:35, sadrul wrote:
> Perhaps 'ScreenOrientationChanged' goes better with the rest of the code.

I'm neutral on the naming. 'Fire' removed and all event methods switched to
'Changed'.

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
File content/browser/sensors/provider_impl.cc (right):

http://codereview.chromium.org/7366011/diff/3002/content/browser/sensors/prov...
content/browser/sensors/provider_impl.cc:16: : listeners_(new ListenerList()) {
On 2011/07/15 15:12:35, sadrul wrote:
> 4-space indent

Done.

[jam, 2011-07-18 17:43:37]

content common lgtm

[commit-bot: I haz the power, 2011-07-19 14:57:25]

Can't apply patch for file content/content_browser.gypi.
While running patch -p1 --forward --force;
patching file content/content_browser.gypi
Hunk #1 FAILED at 345.
1 out of 1 hunk FAILED -- saving rejects to file
content/content_browser.gypi.rej

[commit-bot: I haz the power, 2011-07-19 16:16:44]

Try job failure for 7366011-15002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-07-19 20:39:29]

Change committed as 93094

[cwolfe, 2011-07-20 13:25:09]

sadrul: can you please re-land this for me.

It looks like the build breakage last night was unrelated. I'm getting a lot of
flake on the try-bots, but nothing even vaguely near my changes.

On 2011/07/19 20:39:29, I haz the power (commit-bot) wrote:
> Change committed as 93094

[commit-bot: I haz the power, 2011-07-21 21:44:02]

Change committed as 93480

[tfarina, 2011-07-22 15:00:23]

http://codereview.chromium.org/7366011/diff/28007/content/common/sensors.h
File content/common/sensors.h (right):

http://codereview.chromium.org/7366011/diff/28007/content/common/sensors.h#ne...
content/common/sensors.h:5: #ifndef CONTENT_COMMON_SENSORS_H_
can you name this file screen_orientation? Or this file is going to include more
structures?

http://codereview.chromium.org/7366011/diff/28007/content/common/sensors_list...
File content/common/sensors_listener.h (right):

http://codereview.chromium.org/7366011/diff/28007/content/common/sensors_list...
content/common/sensors_listener.h:28: Listener() {}
you don't need this.

http://codereview.chromium.org/7366011/diff/28007/content/common/sensors_list...
content/common/sensors_listener.h:31: DISALLOW_COPY_AND_ASSIGN(Listener);
nor this, since you have an abstract interface with the pure virtual method
above.

[commit-bot: I haz the power, 2011-07-22 15:03:48]

List of reviewers changed.

[tfarina, 2011-07-22 15:04:41]

http://codereview.chromium.org/7366011/diff/28007/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider.cc (right):

http://codereview.chromium.org/7366011/diff/28007/content/browser/sensors/sen...
content/browser/sensors/sensors_provider.cc:20: }  // sensors
// namespace sensors

http://codereview.chromium.org/7366011/diff/28007/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider_impl.cc (right):

http://codereview.chromium.org/7366011/diff/28007/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.cc:5: #include "base/logging.h"
not used, please can you remove?

http://codereview.chromium.org/7366011/diff/28007/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider_impl.h (right):

http://codereview.chromium.org/7366011/diff/28007/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.h:11: #include "base/task.h"
do you need this include?

http://codereview.chromium.org/7366011/diff/28007/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.h:15: struct
DefaultSingletonTraits;
put this at the same line of the template.

[cwolfe, 2011-07-25 14:23:02]

Nits fixed. Also added a few comments and OVERRIDEs.

Will drop this back into the commit queue when I have time to watch the bots
plod.

[tfarina, 2011-07-25 14:57:52]

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider.h (right):

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider.h:5: #ifndef
CONTENT_BROWSER_SENSORS_PROVIDER_H_
CONTENT_BROWSER_SENSORS_SENSORS_PROVIDER_H_

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider_impl.cc (right):

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.cc:6: #include
"content/browser/sensors/sensors_provider_impl.h"
this should be the first include.

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.cc:14:
ProviderImpl::ProviderImpl()
This and the dtor should be defined after RemoveListener so the order match with
the declaration in the header file.

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider_impl.h (right):

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.h:5: #ifndef
CONTENT_BROWSER_SENSORS_PROVIDER_IMPL_H_
CONTENT_BROWSER_SENSORS_SENSORS_PROVIDER_IMPL_H_

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.h:22: virtual void
AddListener(Listener* listener) OVERRIDE;
It's more common to have this named:

AddObserver(Observer* observer);

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.h:24: virtual void
ScreenOrientationChanged(const ScreenOrientation& change)
move this to the line below so you don't leave OVERRIDE alone. This is something
that Scott (sky) prefers and it's like what we do with "const". Also there is a
thread about this on chromium-dev mailing list.

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.h:41:
DISABLE_RUNNABLE_METHOD_REFCOUNT(sensors::ProviderImpl);
Add a comment explaining why you need this?

http://codereview.chromium.org/7366011/diff/26003/content/common/sensors.h
File content/common/sensors.h (right):

http://codereview.chromium.org/7366011/diff/26003/content/common/sensors.h#ne...
content/common/sensors.h:25: BACK = 5,    // The screen is laid flat, facing
downward.
No trailing comma at the end. Some compilers don't like it. It's a gap on gcc
that this works and it isn't in the C++ standard.

http://codereview.chromium.org/7366011/diff/26003/content/common/sensors_list...
File content/common/sensors_listener.h (right):

http://codereview.chromium.org/7366011/diff/26003/content/common/sensors_list...
content/common/sensors_listener.h:9: #include "base/basictypes.h"
remove this. not needed too.

http://codereview.chromium.org/7366011/diff/26003/content/common/sensors_list...
content/common/sensors_listener.h:26: };
please add a virtual dtor

[cwolfe, 2011-07-25 15:49:45]

Nits fixed. Now if we can get the trivial prototype landed, I'll go back to
getting the actual subsystem ready to upload and discuss.

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider.h (right):

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider.h:5: #ifndef
CONTENT_BROWSER_SENSORS_PROVIDER_H_
On 2011/07/25 14:57:52, tfarina wrote:
> CONTENT_BROWSER_SENSORS_SENSORS_PROVIDER_H_

sensors_provider.h is a temporary name, no risk of conflict. Will go back to
provider.h once crbug.com/90097 is fixed.

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider_impl.cc (right):

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.cc:14:
ProviderImpl::ProviderImpl()
On 2011/07/25 14:57:52, tfarina wrote:
> This and the dtor should be defined after RemoveListener so the order match
with
> the declaration in the header file.

Consistent with PluginService, among others. Code style says identical ordering
is optional.

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider_impl.h (right):

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.h:22: virtual void
AddListener(Listener* listener) OVERRIDE;
On 2011/07/25 14:57:52, tfarina wrote:
> It's more common to have this named:
> 
> AddObserver(Observer* observer);

Differences coming in later revisions. Most notably, that listeners subscribe to
specific channels of the sensors providers, rather than observing all messages.

http://codereview.chromium.org/7366011/diff/26003/content/common/sensors_list...
File content/common/sensors_listener.h (right):

http://codereview.chromium.org/7366011/diff/26003/content/common/sensors_list...
content/common/sensors_listener.h:26: };
On 2011/07/25 14:57:52, tfarina wrote:
> please add a virtual dtor 

Suggest you fix:
content/browser/geolocation/geolocation_observer.h
content/browser/device_orientation/provider.h

[tfarina, 2011-07-25 16:18:37]

On 2011/07/25 15:49:45, cwolfe wrote:
> Nits fixed. Now if we can get the trivial prototype landed, I'll go back to
> getting the actual subsystem ready to upload and discuss.
> 
OK, ready to go. But please since this is going to be used with/in views/ I
think Scott is a reviewer that should be included in further patches.

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
File content/browser/sensors/sensors_provider_impl.cc (right):

http://codereview.chromium.org/7366011/diff/26003/content/browser/sensors/sen...
content/browser/sensors/sensors_provider_impl.cc:14:
ProviderImpl::ProviderImpl()
On 2011/07/25 15:49:45, cwolfe wrote:
> On 2011/07/25 14:57:52, tfarina wrote:
> > This and the dtor should be defined after RemoveListener so the order match
> with
> > the declaration in the header file.
> 
> Consistent with PluginService, among others. Code style says identical
ordering
> is optional.

This is not optional :) And Scott will agree with me.

[commit-bot: I haz the power, 2011-07-26 22:04:01]

Change committed as 94181

[jam, 2011-11-23 01:34:36]

oops, looking at the dependencies of chrome/browser, now I see that this file
should have went to content/common since it's only used by browser.

is this code all going away anyways? if so, can you remove it soon (so that we
can enforce DEPS rules against content/common from chrome/browser)

Thanks