chromeos: Add operations to monitor the screen orientation.

chromeos: Add operations to monitor the screen orientation.

This is intended to help test <http://codereview.chromium.org/7273073/>;.
These orientation-only functions will later be replaced with a
generalized sensors API (after appropriate design review). Getting
this pipeline working helps root out bugs and issues in the interim.

BUG=None
TEST=Inject orientation changes via dbus API.
  With <http://codereview.chromium.org/7845029/>;, inject changes using dbus-send:
  dbus-send --system / org.chromium.Sensors.ScreenOrientationChanged int32:1


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

[cwolfe, 2011-09-10 19:37:13]

Has known bugs and debug code. Not for submission yet. Just putting this up now
so I can feel better ignoring it until Monday (feel free to do likewise).

Took a pass at moving the orientation receiver into the dbus::* API. Am clearly
missing something. It initializes, but neither the connected nor received
callbacks ever fire. bus->Start-like method I'm missing? BrowserThread::IO not
running that early?

Will bug my local folks to help fix --enable-orientation, as it's only getting
passed to the login session.

[cwolfe, 2011-09-12 14:40:41]

Getting there. The dbus problem is that BrowserThread::IO is not yet valid, so
the message posted by ConnectToSignal is silently dropped. Digging for a fix.

sadrul pointed me to the fix for the enable-orientation flag.

[cwolfe, 2011-09-12 19:23:34]

The orientation handler now works in my local tests, so it's PTAL time.

+sky: Sorry the full design for the sensors is taking so long. Coarse
orientation has become a blocker for other people so I need to quickly add a
simple dbus signal to trigger changes.

[satorux1, 2011-09-12 19:46:38]

Looks good in general. Minor comments only:

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:15: static const char
kSensorsServiceName[] = "org.chromium.Sensors";
D-Bus constants should be defined in system_api.git:

http://git.chromium.org/gitweb/?p=chromiumos/platform/system_api.git;a=blob;f...

If you are in hurry, you can go ahead with TODO(cwolfe): Move the constants to
system_api.git.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:16: static const char
kSensorsPath[] = "/";
Using "/" as  object path is not correct: crosbug.com/20135

I guess the sender object name should be something like /org/chromium/Sensors

Again, if you are in hurry, you can go ahead with a TODO(cwolfe): Fix the object
path.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:43: void
SensorsSourceChromeos::Stop() {
You might want to add

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:45: bus_->ShutdownAndBlock();
Shutdown is a bit tricky. Since you are using the IO thread as the D-Bus thread,
it has to be

bus_->ShutdownOnDBusThreadAndBlock(),

unless Stop() is called on D-Bus thread.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:58: int32 upward;
initialize with 0? Uninitialized variable can be lead a non-deterministic bug if
PopInt32() is buggy.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:60: LOG(WARNING) <<
"Orientation changed signal had incorrect parameters.";
Let's add << signal->ToString() in the warning. That'll make debugging easier.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.h (right):

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:27: bool Init();
Function comment is missing. This may be obvious but would be nice add a brief
one-line comment, like "Initializes the sensor source. Returns true on success".

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Every function declaration should have comments immediately preceding it that
describe what the function does and how to use it. These comments should be
descriptive ("Opens the file") rather than imperative ("Open the file");

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:35: void
OrientationChangedConnected(const std::string& iface_name,
iface ->interface? We generally eschew abbreviation.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:37: bool success);
Ditto. Function comments are missing. One-line comments would suffice.

[tfarina, 2011-09-12 20:07:12]

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:15: static const char
kSensorsServiceName[] = "org.chromium.Sensors";
static is not necessary here. const has implicit internal linkage, it needs
neither static nor an unnamed namespace.

[cwolfe, 2011-09-12 21:17:36]

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:15: static const char
kSensorsServiceName[] = "org.chromium.Sensors";
On 2011/09/12 19:46:38, satorux1 wrote:
> D-Bus constants should be defined in system_api.git:
> 
>
http://git.chromium.org/gitweb/?p=chromiumos/platform/system_api.git;a=blob;f...
> 
> If you are in hurry, you can go ahead with TODO(cwolfe): Move the constants to
> system_api.git.

Forgot about system_api.

Unfortunately that would require restricting these files to 'chromeos==1', and
adding define guards in the main. Added here as if they were included from
system_api.

Fortunately this is a rush job as in "within a few days", rather than "within a
few hours". I've had too many CLs that took a month to get approved.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:16: static const char
kSensorsPath[] = "/";
On 2011/09/12 19:46:38, satorux1 wrote:
> Using "/" as  object path is not correct: crosbug.com/20135
> 
> I guess the sender object name should be something like /org/chromium/Sensors
> 
> Again, if you are in hurry, you can go ahead with a TODO(cwolfe): Fix the
object
> path.

Done.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:43: void
SensorsSourceChromeos::Stop() {
On 2011/09/12 19:46:38, satorux1 wrote:
> You might want to add
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

Come to think of it, isn't there a which-thread-to-delete-in trait for
RefCountedThreadSafe? Of course I can't find it now.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:45: bus_->ShutdownAndBlock();
On 2011/09/12 19:46:38, satorux1 wrote:
> Shutdown is a bit tricky. Since you are using the IO thread as the D-Bus
thread,
> it has to be
> 
> bus_->ShutdownOnDBusThreadAndBlock(),
> 
> unless Stop() is called on D-Bus thread.

Done.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:58: int32 upward;
On 2011/09/12 19:46:38, satorux1 wrote:
> initialize with 0? Uninitialized variable can be lead a non-deterministic bug
if
> PopInt32() is buggy.

Done.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:60: LOG(WARNING) <<
"Orientation changed signal had incorrect parameters.";
On 2011/09/12 19:46:38, satorux1 wrote:
> Let's add << signal->ToString() in the warning. That'll make debugging easier.

Done.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.h (right):

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:27: bool Init();
On 2011/09/12 19:46:38, satorux1 wrote:
> Function comment is missing. This may be obvious but would be nice add a brief
> one-line comment, like "Initializes the sensor source. Returns true on
success".
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> Every function declaration should have comments immediately preceding it that
> describe what the function does and how to use it. These comments should be
> descriptive ("Opens the file") rather than imperative ("Open the file"); 

Done.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:35: void
OrientationChangedConnected(const std::string& iface_name,
On 2011/09/12 19:46:38, satorux1 wrote:
> iface ->interface? We generally eschew abbreviation.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

Done.

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:37: bool success);
On 2011/09/12 19:46:38, satorux1 wrote:
> Ditto. Function comments are missing. One-line comments would suffice.

Done.

[sky, 2011-09-12 21:21:18]

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:38: return true;
This always returns true. Is there any point in a return value?

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:42:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
I don't see this getting invoked. When is it invoked?

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:48:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
How are you guaranteeing this is going to be deleted on the UI thread?

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.h (right):

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:18: namespace sensors {
I'm not familiar with sensors, and it doesn't look like this directory exists
yet. Could you get someone that is familiar with sensors to review this?

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:29: // Returns true if the
update comment.

[cwolfe, 2011-09-12 21:22:32]

http://codereview.chromium.org/7862020/diff/11001/content/browser/sensors/sen...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/11001/content/browser/sensors/sen...
content/browser/sensors/sensors_source_chromeos.cc:14: 
Bah, wasn't thinking. The chromeos browser main is already safe, so I would just
need to exclude these files on non-chromeos.

So the only problem is the deps error. (My local build was broken for other
reasons, but the linux try-bot finally confirmed safety)

[satorux1, 2011-09-12 21:55:14]

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/5001/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:16: static const char
kSensorsPath[] = "/";
On 2011/09/12 19:46:38, satorux1 wrote:
> Using "/" as  object path is not correct: crosbug.com/20135
> 
> I guess the sender object name should be something like /org/chromium/Sensors
> 
> Again, if you are in hurry, you can go ahead with a TODO(cwolfe): Fix the
object
> path.

I take it back. You should keep it as "/" for now, if you are in a hurry. Making
it something like /org/chromium/Sensors would make testing with dbus-send
impossible.

Background:

dbus-send sends a signal from "/" and there is no way to change the sender path,
and we have code like this in object_proxy.cc:

  // The signal is not coming from the remote object we are attaching to.       
                                                           
  if (signal->GetPath() != object_path_)
    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;

This code seems to be too restrictive. We should probably change the code to

1) not to check the object path
2) make it customizable

I think 1) is simpler and I can do it right now.

[cwolfe, 2011-09-12 22:15:55]

Sorry if I clobbered anyone's comments. Had a few utterly broken patch-sets that
I rolled back, and ran over at least satorux.

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:38: return true;
On 2011/09/12 21:21:19, sky wrote:
> This always returns true. Is there any point in a return value?

Only for consistency with other Inits. There was previously a PostTask in here,
but ConnectToSignal doesn't currently report failure.

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:42:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/09/12 21:21:19, sky wrote:
> I don't see this getting invoked. When is it invoked?

Just the destructor at the moment. Initially added it in case I needed a
shutdown hook other than the destructor of ChromeBrowserMainPartsChromeos.

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.cc:48:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/09/12 21:21:19, sky wrote:
> How are you guaranteeing this is going to be deleted on the UI thread?

Now that I finally re-located DeleteOnUIThread, using it.

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
File content/browser/sensors/sensors_source_chromeos.h (right):

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:18: namespace sensors {
On 2011/09/12 21:21:19, sky wrote:
> I'm not familiar with sensors, and it doesn't look like this directory exists
> yet. Could you get someone that is familiar with sensors to review this?

Whoops. Fixed the macros.

I added content/browser/sensors a while back as an experimental framework for
the motion sensors system I'm working toward adding. The design doc is
unfortunately still pending.

http://codereview.chromium.org/7862020/diff/2002/content/browser/sensors/sens...
content/browser/sensors/sensors_source_chromeos.h:29: // Returns true if the
On 2011/09/12 21:21:19, sky wrote:
> update comment.

Done.

http://codereview.chromium.org/7862020/diff/12003/content/browser/sensors/sen...
File content/browser/sensors/sensors_source_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/12003/content/browser/sensors/sen...
content/browser/sensors/sensors_source_chromeos.cc:14: 
My test driver is actually using the dbus APIs, so can set its origin path to
"/org/chromium/Sensors". Might be nice to have dbus-send support later, but it's
not crucial for me now.

[cwolfe, 2011-09-13 17:34:45]

Local testing hit a thread restriction error (doing IO on non-IO thread). Assume
it's my fault, but am still getting the debugging turned on far enough to trace
it.

[satorux1, 2011-09-13 17:40:16]

Where did you hit the error? I think it's a DCHECK failure, and the log contains
the file name and the line number where it failed.

On 2011/09/13 17:34:45, cwolfe wrote:
> Local testing hit a thread restriction error (doing IO on non-IO thread).
Assume
> it's my fault, but am still getting the debugging turned on far enough to
trace
> it.

[cwolfe, 2011-09-13 20:59:35]

jam: Realize you were the content OWNER I drafted for the initial sensors patch.
This CL is an incremental growth to let orientation changes arrive over dbus
(ChromeOS with --enable-sensors).

To keep me honest: My intention is still to condense the lessons learned into a
design doc and circulate that for input. Unfortunately I've been slow enough on
that we need an easy way to inject the orientation changes for testing.

Changes in this update:

Moved over to BrowserThread::FILE to avoid restrictions on the kinds of IO we
can perform. Thanks for the pointer satorux.

Switched over to a more generic '--enable-sensors' for consistency with the
namespaces.

<http://codereview.chromium.org/7845029/>, or something like it, would restore
the ability for dbus-send to inject events. My test driver is using the C API,
so works as-is.

Believe it now works as needed. If I've missed anyone's comments please yell at
me :)

[jam, 2011-09-13 21:37:52]

any reason that the chromeos file needs to be in content? we've avoided putting
chromeos files in content, since content by definition is the core code needed
to render a page in a multi-process sandboxed browser. seems that sensor related
code for chromeos doesn't belong there.

[cwolfe, 2011-09-14 13:42:28]

No problem moving sensors_source_chromeos.* back to chrome/browser/chromeos.
Moved initially to avoid changing the DEPS.

Needs +dbus, which is currently set only in content/). If you prefer I change
the DEPS, how far down the tree should it be?

On 2011/09/13 21:37:52, John Abd-El-Malek wrote:
> any reason that the chromeos file needs to be in content? we've avoided
putting
> chromeos files in content, since content by definition is the core code needed
> to render a page in a multi-process sandboxed browser. seems that sensor
related
> code for chromeos doesn't belong there.

[satorux1, 2011-09-14 17:27:53]

On 2011/09/14 13:42:28, cwolfe wrote:
> No problem moving sensors_source_chromeos.* back to chrome/browser/chromeos.
> Moved initially to avoid changing the DEPS.
> 
> Needs +dbus, which is currently set only in content/). If you prefer I change
> the DEPS, how far down the tree should it be?

Let's put it under chrome/browser/chromeos. We can move it up as need.

> 
> On 2011/09/13 21:37:52, John Abd-El-Malek wrote:
> > any reason that the chromeos file needs to be in content? we've avoided
> putting
> > chromeos files in content, since content by definition is the core code
needed
> > to render a page in a multi-process sandboxed browser. seems that sensor
> related
> > code for chromeos doesn't belong there.

[cwolfe, 2011-09-14 17:52:07]

On 2011/09/14 17:27:53, satorux1 wrote:
> On 2011/09/14 13:42:28, cwolfe wrote:
> > No problem moving sensors_source_chromeos.* back to chrome/browser/chromeos.
> > Moved initially to avoid changing the DEPS.
> > 
> > Needs +dbus, which is currently set only in content/). If you prefer I
change
> > the DEPS, how far down the tree should it be?
> 
> Let's put it under chrome/browser/chromeos. We can move it up as need.
> 

Moved. Quick compile test works; running more complete pass now.

[cwolfe, 2011-09-14 19:52:12]

On 2011/09/14 17:52:07, cwolfe wrote:
> On 2011/09/14 17:27:53, satorux1 wrote:
> > On 2011/09/14 13:42:28, cwolfe wrote:
> > > No problem moving sensors_source_chromeos.* back to
chrome/browser/chromeos.
> > > Moved initially to avoid changing the DEPS.
> > > 
> > > Needs +dbus, which is currently set only in content/). If you prefer I
> change
> > > the DEPS, how far down the tree should it be?
> > 
> > Let's put it under chrome/browser/chromeos. We can move it up as need.
> > 
> 
> Moved. Quick compile test works; running more complete pass now.

Rolled in a fix from vollick to
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc. That resolves a
freezing problem which was appearing after random combinations of orientation
changes.

Local tests behave, so looks good to me. The trybots blowing up on random and
disjoint sets of tests is not particularly reassuring, but seems about par.

[satorux1, 2011-09-14 20:21:07]

LGTM

http://codereview.chromium.org/7862020/diff/22001/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/22001/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:100: }
I think file-local functions are usually place at the beginning of the file in
an anonymous namespace.

[cwolfe, 2011-09-14 20:41:24]

Moved and compile-tested.

derat wanted to pass (e-mail), so over to jam.

http://codereview.chromium.org/7862020/diff/22001/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/7862020/diff/22001/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:100: }
On 2011/09/14 20:21:07, satorux1 wrote:
> I think file-local functions are usually place at the beginning of the file in
> an anonymous namespace.

Done.

[jam, 2011-09-15 17:58:41]

sorry for the delay, I didn't know you were still waiting for me. Since this
doesn't touch content, you don't need an owners approval for any of it. Looks
like satorux has already given a lgtm

[cwolfe, 2011-09-15 18:01:18]

jam: No problem, and thanks.

[commit-bot: I haz the power, 2011-09-15 18:01:47]

Can't apply patch for file chrome/browser/chromeos/DEPS.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/DEPS
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/DEPS.rej

[cwolfe, 2011-09-15 18:10:46]

Nothing suspicious in the rebase; going ahead to CQ.

On 2011/09/15 18:01:47, I haz the power (commit-bot) wrote:
> Can't apply patch for file chrome/browser/chromeos/DEPS.
> While running patch -p1 --forward --force;
> patching file chrome/browser/chromeos/DEPS
> Hunk #1 FAILED at 1.
> 1 out of 1 hunk FAILED -- saving rejects to file
> chrome/browser/chromeos/DEPS.rej

[commit-bot: I haz the power, 2011-09-15 18:11:14]

Can't apply patch for file chrome/browser/chromeos/DEPS.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/DEPS
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/DEPS.rej

[commit-bot: I haz the power, 2011-09-15 21:54:15]

Change committed as 101387