Issue 12775019: chromeos: Remove PowerStateOverride.

Issue 12775019: chromeos: Remove PowerStateOverride. (Closed)

Created:
7 years, 9 months ago by Daniel Erat

Modified:
7 years, 9 months ago

Reviewers:
bartfab (slow)

CC:
chromium-reviews, nkostylev+watch_chromium.org, derat+watch_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

chromeos: Remove PowerStateOverride. This removes the PowerStateOverride class, which made D-Bus requests to powerd to override various aspects of power management. It updates the Chrome OS implementation of PowerSaveBlocker to instead register requests with PowerPolicyController, which integrates said requests with the pref-derived power management policy. BUG=176405 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189699

Patch Set 1 #

Patch Set 2 : clear reason field in tests #

Patch Set 3 : add simple tests #

Patch Set 4 : remove unneeded dependency #

Total comments: 45

Patch Set 5 : apply review feedback #

Total comments: 2

Patch Set 6 : more feedback #

Created: 7 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+251 lines, -465 lines)			Patch
M	chrome/browser/chromeos/chrome_browser_main_chromeos.h	View	1 2 3 4 5	3 chunks	+5 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/chrome_browser_main_chromeos.cc	View	1 2 3 4 5	3 chunks	+5 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/policy/power_policy_browsertest.cc	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/chromeos.gyp	View	1 2 3 4	6 chunks	+3 lines, -19 lines	0 comments	Download
M	chromeos/dbus/mock_power_manager_client.h	View	1 2 3 4	1 chunk	+0 lines, -6 lines	0 comments	Download
M	chromeos/dbus/power_manager_client.h	View	1 2 3 4	3 chunks	+0 lines, -25 lines	0 comments	Download
M	chromeos/dbus/power_manager_client.cc	View	1 2 3 4 5	7 chunks	+2 lines, -76 lines	0 comments	Download
M	chromeos/dbus/power_policy_controller.h	View	1 2	4 chunks	+28 lines, -0 lines	0 comments	Download
M	chromeos/dbus/power_policy_controller.cc	View	1 2 3 4	4 chunks	+98 lines, -28 lines	0 comments	Download
A	chromeos/dbus/power_policy_controller_unittest.cc	View	1 2 3 4	1 chunk	+82 lines, -0 lines	0 comments	Download
D	chromeos/power/power_state_override.h	View		1 chunk	+0 lines, -79 lines	0 comments	Download
D	chromeos/power/power_state_override.cc	View		1 chunk	+0 lines, -100 lines	0 comments	Download
D	chromeos/power/power_state_override_unittest.cc	View		1 chunk	+0 lines, -108 lines	0 comments	Download
M	content/browser/power_save_blocker_chromeos.cc	View	1 2 3 4	3 chunks	+27 lines, -18 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

Daniel Erat

I'm investigating how to write tests for this (ideally without using gMock :-/).

7 years, 9 months ago (2013-03-20 16:34:28 UTC) #1

Daniel Erat

Okay, I've added some simple tests (and fixed an embarrassing bug that they caught... today ...

7 years, 9 months ago (2013-03-21 05:12:42 UTC) #2

bartfab (slow)

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode622 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:622: "Kiosk mode"); "Retail mode" https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/chrome_browser_main_chromeos.h File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/chrome_browser_main_chromeos.h#newcode93 ...

7 years, 9 months ago (2013-03-21 14:22:26 UTC) #3

Daniel Erat

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/chrome_browser_main_chromeos.h File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/chrome_browser_main_chromeos.h#newcode93 chrome/browser/chromeos/chrome_browser_main_chromeos.h:93: scoped_ptr<content::PowerSaveBlocker> kiosk_mode_power_save_blocker_; On 2013/03/21 14:22:26, bartfab wrote: > Since ...

7 years, 9 months ago (2013-03-21 14:48:45 UTC) #4

bartfab (slow)

7 years, 9 months ago (2013-03-21 15:34:38 UTC) #5

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:93:
scoped_ptr<content::PowerSaveBlocker> kiosk_mode_power_save_blocker_;
On 2013/03/21 14:48:46, Daniel Erat wrote:
> On 2013/03/21 14:22:26, bartfab wrote:
> > Since we renamed "kiosk mode" to "retail mode" a long time ago, I'd prefer
> > retail_mode over kiosk_mode in new variables.
> 
> Any plans to rename the rest of the code (e.g. classes) to match soon? 
> Referring to the feature as two different things within the code seems like
the
> worst of both worlds to me.

The situation already is bad: We have at least three distinct, entirely
unrelated features that are called "kiosk mode". Any new code we add that refers
to "kiosk mode" only adds to the confusion.

We do not plan to rename any of the existing components though, at least not for
retail mode - retail mode is expected to go away as soon as public sessions are
ready to take its place.

https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller.cc (right):

https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.cc:19: #define SET_DELAY_FROM_PREF(pref,
proto_field, proto, got_prefs)               \
On 2013/03/21 14:48:46, Daniel Erat wrote:
> On 2013/03/21 14:22:26, bartfab wrote:
> > Optional nit: When you see the calls to these macros, you cannot tell which
> are
> > input and which are output parameters. How about making |proto| and
> |got_prefs|
> > pointers so that they variables passed to these are prepended with an & when
> > calling the macro, in line with the way we pass output parameters to
> functions?
> 
> Makes sense; done.  (I had considered this but incorrectly thought that it
would
> lead to more line-wrapping below.)

The |proto| should also be made a pointer then, just as it is for
SET_ACTION_FROM_PREF.

https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller_unittest.cc (right):

https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller_unittest.cc:7: #include <string>
On 2013/03/21 14:48:46, Daniel Erat wrote:
> On 2013/03/21 14:22:26, bartfab wrote:
> > Already included by power_policy_controller.h
> 
> I prefer explicitly including stuff wherever it's used (outside of e.g.
headers
> that are using forward declarations of objects in an interface that they're
> implementing) but don't feel strongly about this.  Done.

People have different styles I guess. I always follow IWYU except for things
already included by the header file corresponding to an implementation file. So
if foo.h includes something, neither foo.cc nor foo_*test.cc need to include it
again. But yeah, styles differ. I don't feel strongly about this.

https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller_unittest.cc:36: // TODO(derat): Write what
looks like it will be a ridiculously large
On 2013/03/21 14:48:46, Daniel Erat wrote:
> On 2013/03/21 14:22:26, bartfab wrote:
> > You can always invoke UpdatePolicyFromPrefs() directly, without registering
> any
> > prefs.
> 
> Preference's c'tor says that PrefService::Register*Pref (I think this is
> outdated) should be used.  Looking at the implementation of
> PrefService::Preference::GetValue(), I think that I'd need to register the
> preference with the PrefService.

We should discuss this offline. I am not sure what kind of test you are thinking
exactly but I am quite sure the mocking won't be that bad.

https://codereview.chromium.org/12775019/diff/8001/content/browser/power_save...
File content/browser/power_save_blocker_chromeos.cc (right):

https://codereview.chromium.org/12775019/diff/8001/content/browser/power_save...
content/browser/power_save_blocker_chromeos.cc:20: Delegate(PowerSaveBlockerType
type, const std::string& reason)
On 2013/03/21 14:48:46, Daniel Erat wrote:
> On 2013/03/21 14:22:26, bartfab wrote:
> > #include <string>
> 
> Done (but it's already included by
content/public/browser/power_save_blocker.h,
> which power_save_blocker_impl.h includes).

Borderline case for IWYU :).

https://codereview.chromium.org/12775019/diff/17001/chromeos/dbus/power_manag...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/12775019/diff/17001/chromeos/dbus/power_manag...
chromeos/dbus/power_manager_client.cc:852: uint32 next_request_id_;
No longer needed.

Daniel Erat

7 years, 9 months ago (2013-03-21 15:42:30 UTC) #6

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:622: "Kiosk mode");
On 2013/03/21 14:22:26, bartfab wrote:
> "Retail mode"

Done.

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

https://codereview.chromium.org/12775019/diff/8001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:93:
scoped_ptr<content::PowerSaveBlocker> kiosk_mode_power_save_blocker_;
On 2013/03/21 15:34:38, bartfab wrote:
> On 2013/03/21 14:48:46, Daniel Erat wrote:
> > On 2013/03/21 14:22:26, bartfab wrote:
> > > Since we renamed "kiosk mode" to "retail mode" a long time ago, I'd prefer
> > > retail_mode over kiosk_mode in new variables.
> > 
> > Any plans to rename the rest of the code (e.g. classes) to match soon? 
> > Referring to the feature as two different things within the code seems like
> the
> > worst of both worlds to me.
> 
> The situation already is bad: We have at least three distinct, entirely
> unrelated features that are called "kiosk mode". Any new code we add that
refers
> to "kiosk mode" only adds to the confusion.
> 
> We do not plan to rename any of the existing components though, at least not
for
> retail mode - retail mode is expected to go away as soon as public sessions
are
> ready to take its place.

Ugh, I see.  Thanks for the explanation.

https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller.cc (right):

https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.cc:19: #define SET_DELAY_FROM_PREF(pref,
proto_field, proto, got_prefs)               \
On 2013/03/21 15:34:38, bartfab wrote:
> On 2013/03/21 14:48:46, Daniel Erat wrote:
> > On 2013/03/21 14:22:26, bartfab wrote:
> > > Optional nit: When you see the calls to these macros, you cannot tell
which
> > are
> > > input and which are output parameters. How about making |proto| and
> > |got_prefs|
> > > pointers so that they variables passed to these are prepended with an &
when
> > > calling the macro, in line with the way we pass output parameters to
> > functions?
> > 
> > Makes sense; done.  (I had considered this but incorrectly thought that it
> would
> > lead to more line-wrapping below.)
> 
> The |proto| should also be made a pointer then, just as it is for
> SET_ACTION_FROM_PREF.

I'm confused; maybe I'm misunderstanding the request.  |proto| is already a
pointer, both in the above comment and in its usage here
("(proto)->set_##proto_field(value);").

https://codereview.chromium.org/12775019/diff/17001/chromeos/dbus/power_manag...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/12775019/diff/17001/chromeos/dbus/power_manag...
chromeos/dbus/power_manager_client.cc:852: uint32 next_request_id_;
On 2013/03/21 15:34:38, bartfab wrote:
> No longer needed.

Thanks!

bartfab (slow)

lgtm https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy_controller.cc File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/12775019/diff/8001/chromeos/dbus/power_policy_controller.cc#newcode19 chromeos/dbus/power_policy_controller.cc:19: #define SET_DELAY_FROM_PREF(pref, proto_field, proto, got_prefs) \ On 2013/03/21 ...

7 years, 9 months ago (2013-03-21 16:27:34 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/12775019/32001

7 years, 9 months ago (2013-03-21 16:33:54 UTC) #8

Daniel Erat

7 years, 9 months ago (2013-03-21 21:49:25 UTC) #9

Message was sent while issue was closed.

Committed patchset #6 manually as r189699 (presubmit successful).

Expand Messages | Collapse Messages