Rotate screen in response to accelerator or device orientation sensors.

Rotate screen in response to accelerator or device orientation sensors.

BUG=385295
TEST=Press Ctrl+Shift+F3, screen rotates.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290007

[flackr, 2014-07-31 22:21:16]

Hey, this is my first cut at doing screen rotation. Can you have a look? Long
term I will standardize and move the common accelerometer handling code to
chromeos/accelerometer http://crbug.com/380831 but this is a quick
implementation to get us up and running.

I'm not sure what effective unit tests would be. Calling rotate screen and
asserting the new size and transform seem like they would simply be testing that
the code does what it does (i.e. asserting the same things in the test as the
implementation). Perhaps it would make more sense when there are other things
that must respond.

[oshima, 2014-08-01 17:15:12]

lgtm with nits and comments

https://codereview.chromium.org/431183003/diff/1/athena/screen/public/screen_...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/431183003/diff/1/athena/screen/public/screen_...
athena/screen/public/screen_manager.h:70: virtual void
SetRotation(gfx::Display::Rotation rotation) = 0;
this is ok for the time being, but we should extract ash's DisplayManager at
some point. 

can you file a bug and add TODO here?

https://codereview.chromium.org/431183003/diff/1/athena/screen/screen_manager...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/431183003/diff/1/athena/screen/screen_manager...
athena/screen/screen_manager_impl.cc:367:
root_window_->layer()->SetTransform(transform);
Note: this won't update gfx::Display objects. A code that relies on may
misbehave.

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
File athena/system/device_socket_listener.cc (right):

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
2014

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:25: typedef
std::vector<DeviceSocketListener*> DeviceSocketListeners;
ObserverList<DeviceSocketListener> may help?

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:27: class DeviceSocketWatcher : public
base::MessagePumpLibevent::Watcher {
document this class, and should this be called Reader rather than Watcher? (as
it reads the data)

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:44: scoped_ptr<char[]> data_;
there is a StringPiece class. since it's small, I'm fine with this though.

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:179: CloseSocket(socket_path);
probably not now, but maybe useful to notify listeners that the socket was
closed unexpectedly.

https://codereview.chromium.org/431183003/diff/1/athena/system/orientation_co...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/431183003/diff/1/athena/system/orientation_co...
athena/system/orientation_controller.cc:30: struct DeviceSensorEvent {
A short discription (meaning and range of values), plus a pointer to
documentation would be helpful.

[flackr, 2014-08-06 14:13:52]

+danakj for +ui/gfx in athena/system/DEPS
+jam for +content/public/browser/browser_thread.h in athena/system/DEPS

https://codereview.chromium.org/431183003/diff/1/athena/screen/public/screen_...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/431183003/diff/1/athena/screen/public/screen_...
athena/screen/public/screen_manager.h:70: virtual void
SetRotation(gfx::Display::Rotation rotation) = 0;
On 2014/08/01 17:15:11, oshima wrote:
> this is ok for the time being, but we should extract ash's DisplayManager at
> some point. 
> 
> can you file a bug and add TODO here?

Done.

https://codereview.chromium.org/431183003/diff/1/athena/screen/screen_manager...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/431183003/diff/1/athena/screen/screen_manager...
athena/screen/screen_manager_impl.cc:367:
root_window_->layer()->SetTransform(transform);
On 2014/08/01 17:15:11, oshima wrote:
> Note: this won't update gfx::Display objects. A code that relies on may
> misbehave.

Noted.

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
File athena/system/device_socket_listener.cc (right):

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/08/01 17:15:11, oshima wrote:
> 2014

Done.

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:25: typedef
std::vector<DeviceSocketListener*> DeviceSocketListeners;
On 2014/08/01 17:15:11, oshima wrote:
> ObserverList<DeviceSocketListener> may help?

Done.

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:27: class DeviceSocketWatcher : public
base::MessagePumpLibevent::Watcher {
On 2014/08/01 17:15:11, oshima wrote:
> document this class, and should this be called Reader rather than Watcher? (as
> it reads the data)

Done.

https://codereview.chromium.org/431183003/diff/1/athena/system/device_socket_...
athena/system/device_socket_listener.cc:179: CloseSocket(socket_path);
On 2014/08/01 17:15:11, oshima wrote:
> probably not now, but maybe useful to notify listeners that the socket was
> closed unexpectedly.

Added TODO.

https://codereview.chromium.org/431183003/diff/1/athena/system/orientation_co...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/431183003/diff/1/athena/system/orientation_co...
athena/system/orientation_controller.cc:30: struct DeviceSensorEvent {
On 2014/08/01 17:15:11, oshima wrote:
> A short discription (meaning and range of values), plus a pointer to
> documentation would be helpful.

Added a description of meaning and range of values as well as a pointer to
orientation information.

[danakj, 2014-08-06 14:14:28]

https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS
File athena/system/DEPS (right):

https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS#newcode6
athena/system/DEPS:6: "+ui/gfx",
do you need all of ui/gfx?

[flackr, 2014-08-06 14:17:17]

https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS
File athena/system/DEPS (right):

https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS#newcode6
athena/system/DEPS:6: "+ui/gfx",
On 2014/08/06 14:14:28, danakj wrote:
> do you need all of ui/gfx?

No, updated to only add display.h.

[danakj, 2014-08-06 14:18:52]

https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS
File athena/system/DEPS (right):

https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS#newcode6
athena/system/DEPS:6: "+ui/gfx",
On 2014/08/06 14:17:17, flackr wrote:
> On 2014/08/06 14:14:28, danakj wrote:
> > do you need all of ui/gfx?
> 
> No, updated to only add display.h.

Thanks. I'm not familiar with the dep graph here, what places depend on
athena/system?

[flackr, 2014-08-06 14:26:51]

On 2014/08/06 14:18:52, danakj wrote:
> https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS
> File athena/system/DEPS (right):
> 
>
https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS#newcode6
> athena/system/DEPS:6: "+ui/gfx",
> On 2014/08/06 14:17:17, flackr wrote:
> > On 2014/08/06 14:14:28, danakj wrote:
> > > do you need all of ui/gfx?
> > 
> > No, updated to only add display.h.
> 
> Thanks. I'm not familiar with the dep graph here, what places depend on
> athena/system?

In athena we only depend on the /public folders. So for system in particular,
athena/main depends on athena/system/public and athena/system/public/DEPS has
-athena/system and athena/DEPS has -athena.

[danakj, 2014-08-06 14:34:21]

On Wed, Aug 6, 2014 at 4:26 PM, <flackr@chromium.org> wrote:

> On 2014/08/06 14:18:52, danakj wrote:
>
>> https://codereview.chromium.org/431183003/diff/20001/athena/system/DEPS
>> File athena/system/DEPS (right):
>>
>
>
> https://codereview.chromium.org/431183003/diff/20001/
> athena/system/DEPS#newcode6
>
>> athena/system/DEPS:6: "+ui/gfx",
>> On 2014/08/06 14:17:17, flackr wrote:
>> > On 2014/08/06 14:14:28, danakj wrote:
>> > > do you need all of ui/gfx?
>> >
>> > No, updated to only add display.h.
>>
>
>  Thanks. I'm not familiar with the dep graph here, what places depend on
>> athena/system?
>>
>
> In athena we only depend on the /public folders. So for system in
> particular,
> athena/main depends on athena/system/public and athena/system/public/DEPS
> has
> -athena/system and athena/DEPS has -athena.
>

Ok I think that reads as "nothing" then. Thanks, LGTM

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

[danakj, 2014-08-06 15:57:08]

Oh, LGTM not-from-email

[oshima, 2014-08-06 21:26:00]

https://codereview.chromium.org/431183003/diff/40001/athena/screen/screen_man...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/431183003/diff/40001/athena/screen/screen_man...
athena/screen/screen_manager_impl.cc:369:
root_window_->layer()->SetTransform(transform);
Actually, can you use

TestScreen::SetDisplayRotation() ? That should update both display and root
rotation

(plus TODO to use display manager)

[flackr, 2014-08-06 23:35:37]

https://codereview.chromium.org/431183003/diff/40001/athena/screen/screen_man...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/431183003/diff/40001/athena/screen/screen_man...
athena/screen/screen_manager_impl.cc:369:
root_window_->layer()->SetTransform(transform);
On 2014/08/06 21:26:00, oshima wrote:
> Actually, can you use
> 
> TestScreen::SetDisplayRotation() ? That should update both display and root
> rotation
> 
> (plus TODO to use display manager)

Done. Had to change the rotation transforms in TestScreen to get the rotations
correct.

[oshima, 2014-08-06 23:47:23]

lgtm

thanks

[jam, 2014-08-07 05:54:16]

https://codereview.chromium.org/431183003/diff/80001/athena/system/DEPS
File athena/system/DEPS (right):

https://codereview.chromium.org/431183003/diff/80001/athena/system/DEPS#newcode5
athena/system/DEPS:5: "+content/public/browser/browser_thread.h",
why not use dependency injection to avoid having this directory know about
content? pass in a SequencedTaskRunner pointer?

[flackr, 2014-08-11 18:23:30]

-jam as reviewer since no longer adding browser_thread dependency.
+sadrul for ui/aura/

oshima, can you take another look. I've converted this to pass in a taskrunner
though I'm not sure about where the DeviceSocketManager instance should be
constructed/destructed. I didn't want to expose it in athena/system/public so I
tied its lifetime to SystemUIImpl. Perhaps I should use a scoped_ptr to make
that lifetime explicit?

https://codereview.chromium.org/431183003/diff/80001/athena/system/DEPS
File athena/system/DEPS (right):

https://codereview.chromium.org/431183003/diff/80001/athena/system/DEPS#newcode5
athena/system/DEPS:5: "+content/public/browser/browser_thread.h",
On 2014/08/07 05:54:16, jam wrote:
> why not use dependency injection to avoid having this directory know about
> content? pass in a SequencedTaskRunner pointer?

Done.

[sadrul, 2014-08-11 18:35:41]

LGTM

Do we have tests that test .. TestScreen?

https://codereview.chromium.org/431183003/diff/100001/ui/aura/test/test_screen.h
File ui/aura/test/test_screen.h (right):

https://codereview.chromium.org/431183003/diff/100001/ui/aura/test/test_scree...
ui/aura/test/test_screen.h:25: public WindowObserver {
this is built in aura_test_support, which is always a static_library. You
probably don't need this change.

[oshima, 2014-08-11 21:30:00]

On 2014/08/11 18:23:30, flackr wrote:
> -jam as reviewer since no longer adding browser_thread dependency.
> +sadrul for ui/aura/
> 
> oshima, can you take another look. I've converted this to pass in a taskrunner
> though I'm not sure about where the DeviceSocketManager instance should be
> constructed/destructed. I didn't want to expose it in athena/system/public so
I
> tied its lifetime to SystemUIImpl. Perhaps I should use a scoped_ptr to make
> that lifetime explicit?

You mean to put it in SystemUIImpl? Given that this class is internal to system/
I'm fine with that.


> https://codereview.chromium.org/431183003/diff/80001/athena/system/DEPS
> File athena/system/DEPS (right):
> 
>
https://codereview.chromium.org/431183003/diff/80001/athena/system/DEPS#newcode5
> athena/system/DEPS:5: "+content/public/browser/browser_thread.h",
> On 2014/08/07 05:54:16, jam wrote:
> > why not use dependency injection to avoid having this directory know about
> > content? pass in a SequencedTaskRunner pointer?
> 
> Done.

[oshima, 2014-08-11 21:30:18]

and here are comments

https://codereview.chromium.org/431183003/diff/100001/athena/system/orientati...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/431183003/diff/100001/athena/system/orientati...
athena/system/orientation_controller.cc:63: CHECK(base::MessageLoop::current());
base::MessageLoopForUI::current() ?

https://codereview.chromium.org/431183003/diff/100001/athena/system/orientati...
athena/system/orientation_controller.cc:73: weak_factory_.GetWeakPtr()));
Looks like FilePathWacher::Watch has to be called on IO 

Thread.https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_path_watcher_linux.cc&rcl=1407695215&l=430

[flackr, 2014-08-13 15:52:36]

https://codereview.chromium.org/431183003/diff/100001/athena/system/orientati...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/431183003/diff/100001/athena/system/orientati...
athena/system/orientation_controller.cc:63: CHECK(base::MessageLoop::current());
On 2014/08/11 21:30:17, oshima wrote:
> base::MessageLoopForUI::current() ?

Done.

https://codereview.chromium.org/431183003/diff/100001/athena/system/orientati...
athena/system/orientation_controller.cc:73: weak_factory_.GetWeakPtr()));
On 2014/08/11 21:30:17, oshima wrote:
> Looks like FilePathWacher::Watch has to be called on IO 
> 
>
Thread.https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_path_watcher_linux.cc&rcl=1407695215&l=430

Done.

https://codereview.chromium.org/431183003/diff/100001/ui/aura/test/test_screen.h
File ui/aura/test/test_screen.h (right):

https://codereview.chromium.org/431183003/diff/100001/ui/aura/test/test_scree...
ui/aura/test/test_screen.h:25: public WindowObserver {
On 2014/08/11 18:35:40, sadrul wrote:
> this is built in aura_test_support, which is always a static_library. You
> probably don't need this change.

Done.

https://codereview.chromium.org/431183003/diff/140001/content/browser/browser...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/431183003/diff/140001/content/browser/browser...
content/browser/browser_thread_impl.cc:217:
base::ThreadRestrictions::SetIOAllowed(true);
FilePathWatcher asserts that IO is allowed on the thread, which was not getting
set on this IO thread.

[flackr, 2014-08-13 17:48:51]

+sky for content/browser

[sky, 2014-08-13 19:49:23]

https://codereview.chromium.org/431183003/diff/160001/content/browser/browser...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/431183003/diff/160001/content/browser/browser...
content/browser/browser_thread_impl.cc:217:
base::ThreadRestrictions::SetIOAllowed(true);
Isn't true the default?

[flackr, 2014-08-13 20:45:54]

https://codereview.chromium.org/431183003/diff/160001/content/browser/browser...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/431183003/diff/160001/content/browser/browser...
content/browser/browser_thread_impl.cc:217:
base::ThreadRestrictions::SetIOAllowed(true);
On 2014/08/13 19:49:23, sky wrote:
> Isn't true the default?

Oops, on investigation it looks like the browser IO thread is not for IO:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br....
It looks like the file thread is what I want:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

[oshima, 2014-08-13 22:46:35]

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:75: virtual gfx::Display::Rotation
GetRotation() = 0;
can you change to use use gfx::Screen::GetPrimaryDisplay().rotation()

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:26: enum {
SensorType ? I know it's not necessary, but it's informative.
Alternatively, you may document them, whichever you like.

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:65: ui_task_runner_ =
base::MessageLoop::current()->task_runner();
nit: base::MessageLoopForUI::current()

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:66: if
(base::PathExists(base::FilePath(kSocketPath))) {
Sorry I missed that in the previous review, but this also need IO access
permission.

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

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:91: }
nuke {}

[flackr, 2014-08-14 17:29:51]

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:75: virtual gfx::Display::Rotation
GetRotation() = 0;
On 2014/08/13 22:46:34, oshima wrote:
> can you change to use use gfx::Screen::GetPrimaryDisplay().rotation()
> 

Do you mean in the implementation of this? Done. Or did you mean to call
directly to gfx::Screen::GetPrimaryDisplay().rotation() rather than call this
method here?

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
File athena/system/orientation_controller.cc (right):

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:26: enum {
On 2014/08/13 22:46:34, oshima wrote:
> SensorType ? I know it's not necessary, but it's informative.
> Alternatively, you may document them, whichever you like.

Done.

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:65: ui_task_runner_ =
base::MessageLoop::current()->task_runner();
On 2014/08/13 22:46:34, oshima wrote:
> nit: base::MessageLoopForUI::current()

Done.

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:66: if
(base::PathExists(base::FilePath(kSocketPath))) {
On 2014/08/13 22:46:34, oshima wrote:
> Sorry I missed that in the previous review, but this also need IO access
> permission.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/file_util_pos...
> 

Done.

https://codereview.chromium.org/431183003/diff/180001/athena/system/orientati...
athena/system/orientation_controller.cc:91: }
On 2014/08/13 22:46:34, oshima wrote:
> nuke {}

Done.

[oshima, 2014-08-14 19:28:53]

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:75: virtual gfx::Display::Rotation
GetRotation() = 0;
On 2014/08/14 17:29:51, flackr wrote:
> On 2014/08/13 22:46:34, oshima wrote:
> > can you change to use use gfx::Screen::GetPrimaryDisplay().rotation()
> > 
> 
> Do you mean in the implementation of this? Done. Or did you mean to call
> directly to gfx::Screen::GetPrimaryDisplay().rotation() rather than call this
> method here?

The latter. We already have an API to get display rotation, so we should use
that instead of adding new API.

https://codereview.chromium.org/431183003/diff/180001/ui/aura/test/test_scree...
File ui/aura/test/test_screen.cc (right):

https://codereview.chromium.org/431183003/diff/180001/ui/aura/test/test_scree...
ui/aura/test/test_screen.cc:51: // TODO(oshima|mukai): Update the display_ as
well.
sorry I missed this. can you update the display_?

[flackr, 2014-08-14 21:04:10]

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/431183003/diff/180001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:75: virtual gfx::Display::Rotation
GetRotation() = 0;
On 2014/08/14 19:28:53, oshima wrote:
> On 2014/08/14 17:29:51, flackr wrote:
> > On 2014/08/13 22:46:34, oshima wrote:
> > > can you change to use use gfx::Screen::GetPrimaryDisplay().rotation()
> > > 
> > 
> > Do you mean in the implementation of this? Done. Or did you mean to call
> > directly to gfx::Screen::GetPrimaryDisplay().rotation() rather than call
this
> > method here?
> 
> The latter. We already have an API to get display rotation, so we should use
> that instead of adding new API.

Done.

https://codereview.chromium.org/431183003/diff/180001/ui/aura/test/test_scree...
File ui/aura/test/test_screen.cc (right):

https://codereview.chromium.org/431183003/diff/180001/ui/aura/test/test_scree...
ui/aura/test/test_screen.cc:51: // TODO(oshima|mukai): Update the display_ as
well.
On 2014/08/14 19:28:53, oshima wrote:
> sorry I missed this. can you update the display_?

Done.

[oshima, 2014-08-14 21:09:12]

lgtm

thanks!

[flackr, 2014-08-14 21:12:19]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-08-14 21:16:22]

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

[commit-bot: I haz the power, 2014-08-15 03:24:35]

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_...)

[commit-bot: I haz the power, 2014-08-15 03:36:09]

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

[commit-bot: I haz the power, 2014-08-15 03:36:10]

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

[flackr, 2014-08-15 13:49:04]

Can you take another look at athena.gyp? I was missing some dependencies for
building athena_unittests.

[oshima, 2014-08-15 13:53:29]

lgtm

https://codereview.chromium.org/431183003/diff/240001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/431183003/diff/240001/athena/athena.gyp#newco...
athena/athena.gyp:149: 'athena_content_lib',
can you add comment that this is due to header dependency?

[flackr, 2014-08-15 13:55:54]

https://codereview.chromium.org/431183003/diff/240001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/431183003/diff/240001/athena/athena.gyp#newco...
athena/athena.gyp:149: 'athena_content_lib',
On 2014/08/15 13:53:29, oshima wrote:
> can you add comment that this is due to header dependency?

This is due to athena_launcher.cc getting a TaskRunner from
content::BrowserThread.

[flackr, 2014-08-15 17:09:45]

The CQ bit was checked by flackr@chromium.org

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

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

[commit-bot: I haz the power, 2014-08-15 21:07:39]

Committed patchset #12 (240001) as 290007

[Nico, 2014-08-16 05:15:40]

A revert of this CL (patchset #12) has been created in
https://codereview.chromium.org/475533008/ by thakis@chromium.org.

The reason for reverting is: Speculative, athena_unittests and ui_unittests
started failing in this
build:http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%283%29/builds/2771

==6568==ERROR: AddressSanitizer: heap-use-after-free on address 0x60700000ba10
at pc 0x00000167d52a bp 0x7fff95f266f0 sp 0x7fff95f266e8
READ of size 8 at 0x60700000ba10 thread T0
    #0 0x167d529 in DeleteInternal base/memory/ref_counted.h:190:44
    #1 0x167d529 in Destruct base/memory/ref_counted.h:153
    #2 0x167d529 in Release base/memory/ref_counted.h:181
    #3 0x167d529 in ~scoped_refptr base/memory/ref_counted.h:289
    #4 0x167d529 in ~SystemUIImpl athena/system/system_ui_impl.cc:27
    #5 0x167d529 in athena::(anonymous namespace)::SystemUIImpl::~SystemUIImpl()
athena/system/system_ui_impl.cc:26
    #6 0x167d257 in athena::SystemUI::Shutdown()
athena/system/system_ui_impl.cc:49:3
    #7 0x577fa6 in athena::ShutdownAthena() athena/main/athena_launcher.cc:92:3
    #8 0x57645f in athena::test::AthenaTestHelper::TearDown()
athena/test/athena_test_helper.cc:98:3
    #9 0x575266 in athena::test::AthenaTestBase::TearDown()
athena/test/athena_test_base.cc:53:3
    #10 0x505bc8 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5
    #11 0x506906 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5
    #12 0x51cb85 in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4591:11
    #13 0x51c176 in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2418:12
    #14 0x51c176 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209
    #15 0xac6fb4 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10
    #16 0xac6fb4 in base::TestSuite::Run() base/test/test_suite.cc:227
    #17 0xabe39d in Run base/callback.h:401:12
    #18 0xabe39d in base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int,
bool, base::Callback\u003Cvoid ()> const&)
base/test/launcher/unit_test_launcher.cc:498
    #19 0xabf0de in base::LaunchUnitTestsSerially(int, char**,
base::Callback\u003Cint ()> const&)
base/test/launcher/unit_test_launcher.cc:564:10
    #20 0x4d7bba in main athena/test/athena_unittests.cc:51:10
    #21 0x7fd394e3476c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226
    #22 0x4d795c in _start
(/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__3_/build/src/out/Release/athena_unittests+0x4d795c)

0x60700000ba10 is located 0 bytes inside of 72-byte region
[0x60700000ba10,0x60700000ba58)
freed by thread T0 here:
    #0 0x452fcb in operator delete(void*)
(/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__3_/build/src/out/Release/athena_unittests+0x452fcb)
    #1 0x168323d in DeleteInternal base/memory/ref_counted.h:190:44
    #2 0x168323d in Destruct base/memory/ref_counted.h:153
    #3 0x168323d in Release base/memory/ref_counted.h:181
    #4 0x168323d in ~scoped_refptr base/memory/ref_counted.h:289
    #5 0x168323d in
athena::OrientationController::OrientationController(scoped_refptr\u003Cbase::TaskRunner>)
athena/system/orientation_controller.cc:66
    #6 0x167cfea in SystemUIImpl athena/system/system_ui_impl.cc:23:61
    #7 0x167cfea in
athena::SystemUI::Create(scoped_refptr\u003Cbase::TaskRunner>)
athena/system/system_ui_impl.cc:42
    #8 0x577db0 in athena::StartAthena(aura::Window*, athena::ActivityFactory*,
athena::AppModelBuilder*) athena/main/athena_launcher.cc:73:3
    #9 0x576159 in athena::test::AthenaTestHelper::SetUp(ui::ContextFactory*)
athena/test/athena_test_helper.cc:90:3
    #10 0x503eb1 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test,
void> testing/gtest/src/gtest.cc:2418:12
    #11 0x503eb1 in testing::Test::Run() testing/gtest/src/gtest.cc:2430
    #12 0x505bc8 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5
    #13 0x506906 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5
    #14 0x51cb85 in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4591:11
    #15 0x51c176 in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2418:12
    #16 0x51c176 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209
    #17 0xac6fb4 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10
    #18 0xac6fb4 in base::TestSuite::Run() base/test/test_suite.cc:227
    #19 0xabe39d in Run base/callback.h:401:12
    #20 0xabe39d in base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int,
bool, base::Callback\u003Cvoid ()> const&)
base/test/launcher/unit_test_launcher.cc:498
    #21 0xabf0de in base::LaunchUnitTestsSerially(int, char**,
base::Callback\u003Cint ()> const&)
base/test/launcher/unit_test_launcher.cc:564:10
    #22 0x4d7bba in main athena/test/athena_unittests.cc:51:10
    #23 0x7fd394e3476c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226

previously allocated by thread T0 here:
    #0 0x452a8b in operator new(unsigned long)
(/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__3_/build/src/out/Release/athena_unittests+0x452a8b)
    #1 0x167cfb5 in SystemUIImpl athena/system/system_ui_impl.cc:23:61
    #2 0x167cfb5 in
athena::SystemUI::Create(scoped_refptr\u003Cbase::TaskRunner>)
athena/system/system_ui_impl.cc:42
    #3 0x577db0 in athena::StartAthena(aura::Window*, athena::ActivityFactory*,
athena::AppModelBuilder*) athena/main/athena_launcher.cc:73:3
    #4 0x576159 in athena::test::AthenaTestHelper::SetUp(ui::ContextFactory*)
athena/test/athena_test_helper.cc:90:3
    #5 0x503eb1 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void>
testing/gtest/src/gtest.cc:2418:12
    #6 0x503eb1 in testing::Test::Run() testing/gtest/src/gtest.cc:2430
    #7 0x505bc8 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5
    #8 0x506906 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5
    #9 0x51cb85 in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4591:11
    #10 0x51c176 in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2418:12
    #11 0x51c176 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209
    #12 0xac6fb4 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10
    #13 0xac6fb4 in base::TestSuite::Run() base/test/test_suite.cc:227
    #14 0xabe39d in Run base/callback.h:401:12
    #15 0xabe39d in base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int,
bool, base::Callback\u003Cvoid ()> const&)
base/test/launcher/unit_test_launcher.cc:498
    #16 0xabf0de in base::LaunchUnitTestsSerially(int, char**,
base::Callback\u003Cint ()> const&)
base/test/launcher/unit_test_launcher.cc:564:10
    #17 0x4d7bba in main athena/test/athena_unittests.cc:51:10
    #18 0x7fd394e3476c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226
.