Screensaver implementation for ChromeOS.

Screensaver implementation for ChromeOS.
This is the initial implementation for screensaver extensions. The feature is currently behind a flag and installing an extension with a screensaver permission without the flag enabled will do nothing.
This currently allows one Screensaver extension to be installed at a time, and brings up the screensaver after a fixed 2 minutes. Further work to be done is to add different timeouts for power manager if the screensaver is active, add more tests and add security features for the screensaver permission (if needed, this needs to be discussed still).

TBR'ed jhawkins for chrome_browser_chromeos.gypi

R=mpcomplete@chromium.org,stevenjb@chromium.org
BUG=163681
TBR=jhawkins@chromium.org

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

[Matt Perry, 2013-01-31 00:37:23]

https://codereview.chromium.org/12093058/diff/3001/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.cc (right):

https://codereview.chromium.org/12093058/diff/3001/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:65: if (g_instance)
nit: can simplify to "return g_instance && g_instance->IsShowing...;"

https://codereview.chromium.org/12093058/diff/3001/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:74:
screensaver_webview_->web_contents()->GetURL() == url) {
ditto, just return this expression

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
We have other systems that only support 1 active extension at a time. The proxy
API, for example, lets you install multiple extensions, but has a stack that
controls which one is active. The most recently installed extension is always
active.

Have you considered handling multiple screensavers that way?

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:24: const int
kScreensaverTimeoutMs = 120000;
nit: could specify this in minutes, and use TimeDelta::FromMinutes()

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:27: Profile*
default_profile = ProfileManager::GetDefaultProfile();
This is generally a bad thing to do... But maybe it makes sense on ChromeOS? Is
the default profile something other than a user's profile on ChromeOS?

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:66:
current->id())).empty()) {
indent

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:108:
UninstallPreviousScreensavers(extension);
I think you mean to do this only if |extension| is a screensaver extension.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:115: return;
indent

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:124: if
(extension->HasAPIPermission(extensions::APIPermission::kScreensaver))
might be clearer to just test if the id matches screensaver_extension_id

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:136:
ash::ShowScreensaver(screensaver_extension->GetFullLaunchURL());
interesting.. so a screensaver is just an app with a "screensaver" permission
that gets launched on idle. That's nice and simple.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller.h (right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.h:25: class
ScreensaverController : public ash::UserActivityObserver,
please add a class comment

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc
(right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc:18:
const extensions::Extension* extension = InstallExtension(
This test could probably be made into a lighter-weight test (possibly even a
unit test) by not using a .crx on disk. See, e.g.
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/...
for how to create an in-memory Extension. You can then load it using
ExtensionService::AddExtension.

https://codereview.chromium.org/12093058/diff/3001/chrome/test/data/extension...
File chrome/test/data/extensions/screensaver_test_extension.pem (right):

https://codereview.chromium.org/12093058/diff/3001/chrome/test/data/extension...
chrome/test/data/extensions/screensaver_test_extension.pem:1: -----BEGIN PRIVATE
KEY-----
don't bother checking this file in. we can just repack a new extension.

https://codereview.chromium.org/12093058/diff/3001/chromeos/dbus/power_manage...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/12093058/diff/3001/chromeos/dbus/power_manage...
chromeos/dbus/power_manager_client.cc:804: virtual void
RequestIdleNotification(int64 threshold) OVERRIDE {
When is this called? I know it's a stub, but it'll do the wrong thing, so maybe
it's better to leave it empty.

[rkc, 2013-01-31 02:15:45]

https://codereview.chromium.org/12093058/diff/3001/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.cc (right):

https://codereview.chromium.org/12093058/diff/3001/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:65: if (g_instance)
On 2013/01/31 00:37:23, Matt Perry wrote:
> nit: can simplify to "return g_instance && g_instance->IsShowing...;"

Done.

https://codereview.chromium.org/12093058/diff/3001/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:74:
screensaver_webview_->web_contents()->GetURL() == url) {
On 2013/01/31 00:37:23, Matt Perry wrote:
> ditto, just return this expression

Done.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/31 00:37:23, Matt Perry wrote:
> We have other systems that only support 1 active extension at a time. The
proxy
> API, for example, lets you install multiple extensions, but has a stack that
> controls which one is active. The most recently installed extension is always
> active.
> 
> Have you considered handling multiple screensavers that way?

So I spoke with Josh (the PM for this feature) and he thinks that it is actually
better if we only keep one screensaver extension around at a time. It keeps
things simple for us (in terms of deciding what to do if a user uninstalls the
most recent screensaver) and for the user (who doesn't have to really worry
about which screensaver is active and which one will get active if he uninstalls
the current one).

This way, if the user installs a screensaver extension, he knows _that_ is his
screensaver, if he uninstalls it, he knows he has no screensaver now. If he
wants a previous screensaver, he can just install that extension again.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:24: const int
kScreensaverTimeoutMs = 120000;
On 2013/01/31 00:37:23, Matt Perry wrote:
> nit: could specify this in minutes, and use TimeDelta::FromMinutes()

Done.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:27: Profile*
default_profile = ProfileManager::GetDefaultProfile();
On 2013/01/31 00:37:23, Matt Perry wrote:
> This is generally a bad thing to do... But maybe it makes sense on ChromeOS?
Is
> the default profile something other than a user's profile on ChromeOS?

The default profile on ChromeOS will always be your logged in user's profile.
This is currently the standard way to get the current user's profile on
ChromeOS.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:66:
current->id())).empty()) {
On 2013/01/31 00:37:23, Matt Perry wrote:
> indent

Done.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:108:
UninstallPreviousScreensavers(extension);
On 2013/01/31 00:37:23, Matt Perry wrote:
> I think you mean to do this only if |extension| is a screensaver extension.

Whoops, yes, this will just unload any previous screensavers on any extension
install.

Done.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:115: return;
On 2013/01/31 00:37:23, Matt Perry wrote:
> indent

Done.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:124: if
(extension->HasAPIPermission(extensions::APIPermission::kScreensaver))
On 2013/01/31 00:37:23, Matt Perry wrote:
> might be clearer to just test if the id matches screensaver_extension_id

Done.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:136:
ash::ShowScreensaver(screensaver_extension->GetFullLaunchURL());
On 2013/01/31 00:37:23, Matt Perry wrote:
> interesting.. so a screensaver is just an app with a "screensaver" permission
> that gets launched on idle. That's nice and simple.

Yep, that's how the screensaver is implemented in retail mode, so it seemed
clean to just extend that.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller.h (right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.h:25: class
ScreensaverController : public ash::UserActivityObserver,
On 2013/01/31 00:37:23, Matt Perry wrote:
> please add a class comment

Done.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc
(right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc:18:
const extensions::Extension* extension = InstallExtension(
On 2013/01/31 00:37:23, Matt Perry wrote:
> This test could probably be made into a lighter-weight test (possibly even a
> unit test) by not using a .crx on disk. See, e.g.
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/...
> for how to create an in-memory Extension. You can then load it using
> ExtensionService::AddExtension.

I reworked the code quite a bit to try to make this work. I can create an test
extension but for some reason "AddExtension" crashes in InitializePermissions.
Is there anything else I need to do to ensure that the 'screensaver' permission
is added and added correctly?

This is what the stack looks like,
 [0x000002700ab0] base::debug::StackTrace::StackTrace()
 [0x0000027271a9] logging::LogMessage::~LogMessage()
 [0x000001e4e155] ExtensionService::InitializePermissions()
 [0x000001e4d72c] ExtensionService::AddExtension()
 [0x00000056a082] (anonymous namespace)::InstallExtensionToDefaultProfile()
 [0x00000056a357]
chromeos::ScreensaverControllerTest_OutOfOrder_Test::RunTestOnMainThread()
 [0x0000021e898d] InProcessBrowserTest::RunTestOnMainThreadLoop()
.
.
.

https://codereview.chromium.org/12093058/diff/3001/chrome/test/data/extension...
File chrome/test/data/extensions/screensaver_test_extension.pem (right):

https://codereview.chromium.org/12093058/diff/3001/chrome/test/data/extension...
chrome/test/data/extensions/screensaver_test_extension.pem:1: -----BEGIN PRIVATE
KEY-----
On 2013/01/31 00:37:23, Matt Perry wrote:
> don't bother checking this file in. we can just repack a new extension.
Removed.
Done.

https://codereview.chromium.org/12093058/diff/3001/chromeos/dbus/power_manage...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/12093058/diff/3001/chromeos/dbus/power_manage...
chromeos/dbus/power_manager_client.cc:804: virtual void
RequestIdleNotification(int64 threshold) OVERRIDE {
On 2013/01/31 00:37:23, Matt Perry wrote:
> When is this called? I know it's a stub, but it'll do the wrong thing, so
maybe
> it's better to leave it empty.

This will only get called when running ChromeOS_Chrome on Linux. It's used for
easy testing (like all other cros stub methods).

[rkc, 2013-01-31 02:53:18]

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc
(right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc:18:
const extensions::Extension* extension = InstallExtension(
On 2013/01/31 02:15:46, Rahul Chaturvedi wrote:
> On 2013/01/31 00:37:23, Matt Perry wrote:
> > This test could probably be made into a lighter-weight test (possibly even a
> > unit test) by not using a .crx on disk. See, e.g.
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/...
> > for how to create an in-memory Extension. You can then load it using
> > ExtensionService::AddExtension.
> 
> I reworked the code quite a bit to try to make this work. I can create an test
> extension but for some reason "AddExtension" crashes in InitializePermissions.
> Is there anything else I need to do to ensure that the 'screensaver'
permission
> is added and added correctly?
> 
> This is what the stack looks like,
>  [0x000002700ab0] base::debug::StackTrace::StackTrace()
>  [0x0000027271a9] logging::LogMessage::~LogMessage()
>  [0x000001e4e155] ExtensionService::InitializePermissions()
>  [0x000001e4d72c] ExtensionService::AddExtension()
>  [0x00000056a082] (anonymous namespace)::InstallExtensionToDefaultProfile()
>  [0x00000056a357]
> chromeos::ScreensaverControllerTest_OutOfOrder_Test::RunTestOnMainThread()
>  [0x0000021e898d] InProcessBrowserTest::RunTestOnMainThreadLoop()
> .
> .
> .

Fixed the tests, they pass now.

[Matt Perry, 2013-01-31 22:43:50]

lgtm

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/31 02:15:46, Rahul Chaturvedi wrote:
> On 2013/01/31 00:37:23, Matt Perry wrote:
> > We have other systems that only support 1 active extension at a time. The
> proxy
> > API, for example, lets you install multiple extensions, but has a stack that
> > controls which one is active. The most recently installed extension is
always
> > active.
> > 
> > Have you considered handling multiple screensavers that way?
> 
> So I spoke with Josh (the PM for this feature) and he thinks that it is
actually
> better if we only keep one screensaver extension around at a time. It keeps
> things simple for us (in terms of deciding what to do if a user uninstalls the
> most recent screensaver) and for the user (who doesn't have to really worry
> about which screensaver is active and which one will get active if he
uninstalls
> the current one).
> 
> This way, if the user installs a screensaver extension, he knows _that_ is his
> screensaver, if he uninstalls it, he knows he has no screensaver now. If he
> wants a previous screensaver, he can just install that extension again.

OK, that seems fair.

https://codereview.chromium.org/12093058/diff/3001/chrome/browser/chromeos/sc...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:66:
current->id())).empty()) {
On 2013/01/31 02:15:46, Rahul Chaturvedi wrote:
> On 2013/01/31 00:37:23, Matt Perry wrote:
> > indent
> 
> Done.

(FYI I just meant that curret_id should be 1 space to the right of the open
paren. But this way is fine.)

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:108: if
(!extension->HasAPIPermission(extensions::APIPermission::kScreensaver))
negate this

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc
(right):

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc:55:
InstallExtensionToDefaultProfile(extension);
nit: I'd prefer to use profile() here (or it might be browser()->profile())

[rkc, 2013-01-31 22:53:55]

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:108: if
(!extension->HasAPIPermission(extensions::APIPermission::kScreensaver))
On 2013/01/31 22:43:51, Matt Perry wrote:
> negate this

Done.

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc
(right):

https://codereview.chromium.org/12093058/diff/12002/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller_browsertest.cc:55:
InstallExtensionToDefaultProfile(extension);
On 2013/01/31 22:43:51, Matt Perry wrote:
> nit: I'd prefer to use profile() here (or it might be browser()->profile())

I'm installing this to the default profile to ensure that the extension is
installed to the profile that the screensaver controller is expecting the
extension to be on.

[stevenjb, 2013-01-31 23:42:18]

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:51: return
extension->id();
nit: {}

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:66:
FindScreensaverExtension(service, current->id())).empty()) {
Does this compile? I'm not sure what this logic is supposed to be...

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:71: screensaver_id
<< " \nError: " << error;
{}
nit: << at beginning of line

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:116: return;
nit: Use either if (HasAPIPermission) {} or if (!HasAPIPermission) return;
consistently here and above

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:173:
ash::Shell::GetInstance()->user_activity_detector()->RemoveObserver(this);
{}

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.h (right):

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.h:26: // responsible
for,
nit: s/,/:

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.h:27: // . Enabling
and disabling of the screensaver, long with ensuring that we have
s/long/along

[rkc, 2013-02-01 00:35:45]

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:51: return
extension->id();
On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> nit: {}

Done.

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:66:
FindScreensaverExtension(service, current->id())).empty()) {
On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> Does this compile? I'm not sure what this logic is supposed to be...

This is fairly standard C++ syntax. It is equivalent to,
screensaver_id = FindScreensaverExtension(service, current->id());
while (!screensaver_id.empty()) {
  screensaver_id = FindScreensaverExtension(service, current->id());
  ...
}

It simplifies the above code into one line.


I've compiled and tested this code manually. The browsertest also tests this
code.

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:71: screensaver_id
<< " \nError: " << error;
On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> {}
> nit: << at beginning of line

Done.

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:116: return;
On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> nit: Use either if (HasAPIPermission) {} or if (!HasAPIPermission) return;
> consistently here and above

Done.

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:173:
ash::Shell::GetInstance()->user_activity_detector()->RemoveObserver(this);
On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> {}

Done.

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.h (right):

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.h:26: // responsible
for,
On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> nit: s/,/:

Done.

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.h:27: // . Enabling
and disabling of the screensaver, long with ensuring that we have
On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> s/long/along

Done.

[stevenjb, 2013-02-01 04:16:27]

lgtm

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:66:
FindScreensaverExtension(service, current->id())).empty()) {
On 2013/02/01 00:35:45, Rahul Chaturvedi wrote:
> On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> > Does this compile? I'm not sure what this logic is supposed to be...
> 
> This is fairly standard C++ syntax. It is equivalent to,
> screensaver_id = FindScreensaverExtension(service, current->id());
> while (!screensaver_id.empty()) {
>   screensaver_id = FindScreensaverExtension(service, current->id());
>   ...
> }
> 
> It simplifies the above code into one line.
> 
> 
> I've compiled and tested this code manually. The browsertest also tests this
> code.

Ahhh.... I'm familiar with the pattern, but I completely missed the .empty() at
the end of the line.

On the one hand I should have seen that, on the other, it is a bit hard to
parse. Maybe consider:

while ((screensaver_id = FindScreensaverExtension(service, current->id()))
        != "") {
}
or put the .empty() on the second line instead? It's fine as-is, but it clearly
confused me :)

[rkc, 2013-02-01 08:37:39]

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
File chrome/browser/chromeos/screensaver/screensaver_controller.cc (right):

https://codereview.chromium.org/12093058/diff/11019/chrome/browser/chromeos/s...
chrome/browser/chromeos/screensaver/screensaver_controller.cc:66:
FindScreensaverExtension(service, current->id())).empty()) {
On 2013/02/01 04:16:27, stevenjb (chromium) wrote:
> On 2013/02/01 00:35:45, Rahul Chaturvedi wrote:
> > On 2013/01/31 23:42:18, stevenjb (chromium) wrote:
> > > Does this compile? I'm not sure what this logic is supposed to be...
> > 
> > This is fairly standard C++ syntax. It is equivalent to,
> > screensaver_id = FindScreensaverExtension(service, current->id());
> > while (!screensaver_id.empty()) {
> >   screensaver_id = FindScreensaverExtension(service, current->id());
> >   ...
> > }
> > 
> > It simplifies the above code into one line.
> > 
> > 
> > I've compiled and tested this code manually. The browsertest also tests this
> > code.
> 
> Ahhh.... I'm familiar with the pattern, but I completely missed the .empty()
at
> the end of the line.
> 
> On the one hand I should have seen that, on the other, it is a bit hard to
> parse. Maybe consider:
> 
> while ((screensaver_id = FindScreensaverExtension(service, current->id()))
>         != "") {
> }
> or put the .empty() on the second line instead? It's fine as-is, but it
clearly
> confused me :)
> 

Done.

[commit-bot: I haz the power, 2013-02-01 08:37:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/12093058/3003

[commit-bot: I haz the power, 2013-02-01 08:38:02]

Presubmit check for 12093058-3003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 5.4s to calculate.

[commit-bot: I haz the power, 2013-02-01 08:40:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/12093058/3003