chromeos: power: Refactor RendererFreezer and add tests

chromeos: power: Refactor RendererFreezer and add tests

Refactor the RendererFreezer class so that untestable code is moved into
a delegate and add unit tests.  Modify FakePowerManagerClient so that it
provides more information for tests.

Also use a CancelableClosure for the asynchronous suspend readiness
callback in case we get a SuspendDone before we've had a chance to run
OnReadyForSuspend().

BUG=364339, 414396
Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>;

Committed: https://crrev.com/b651946eea95c8af526e4df1e06809385f8cb822
Cr-Commit-Position: refs/heads/master@{#295111}

[Chirantan Ekbote, 2014-09-06 01:23:35]

chirantan@chromium.org changed reviewers:
+ derat@chromium.org

[Chirantan Ekbote, 2014-09-06 01:23:36]

Please take a look.

[Daniel Erat, 2014-09-06 02:01:25]

thanks for adding tests

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/renderer_freezer.cc (right):

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer.cc:30: delegate_.reset();
nit: you shouldn't need to do this explicitly

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/renderer_freezer_delegate.cc (right):

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_delegate.cc:21:
enabled_(base::PathExists(state_path_) &&
you should be able to just call PathIsWritable (which is access(..., W_OK)) here
without additionally calling PathExists (which is access(..., F_OK))

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_delegate.cc:28: bool
RendererFreezerDelegate::FreezeRenderers() {
maybe DCHECK() or LOG(ERROR) and return early or something here if !enabled_?
ditto for ThawRenderers().

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/renderer_freezer_delegate.h (right):

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_delegate.h:14: class
RendererFreezerDelegate : public RendererFreezer::Delegate {
i don't like the similarity of these names, but i also don't like the only
suggestion i can come up with, "RealRendererFreezerDelegate". hmm...
"CgroupRendererFreezerDelegate", maybe?

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/renderer_freezer_unittest.cc (right):

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_unittest.cc:93:
TestDelegate::TestDelegate()
all of these implementations are short enough that i'd probably inline them in
the above class definition to avoid needing to duplicate all of these signatures
and make the code easier to read. (it's fine to inline in classes that are
defined in .cc files rather than headers.)

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_unittest.cc:145:
FakePowerManagerClient* fake_power_manager_client_;
nit: name this |client_| to avoid awkward-looking wrapping later?

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_unittest.cc:148:
scoped_ptr<RendererFreezer> renderer_freezer_;
nit: name this |freezer_|? (feel free to ignore if it doesn't help with
wrapping)

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_unittest.cc:155:
RendererFreezerTest::RendererFreezerTest()
i'd recommend inlining these too

https://codereview.chromium.org/543303002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_unittest.cc:170:
DBusThreadManager::GetSetterForTesting()->SetPowerManagerClient(
nit: probably a bit cleaner to do this in the c'tor and just leave the
RendererFreezer creation in Init()

https://codereview.chromium.org/543303002/diff/20001/chromeos/dbus/fake_power...
File chromeos/dbus/fake_power_manager_client.h (right):

https://codereview.chromium.org/543303002/diff/20001/chromeos/dbus/fake_power...
chromeos/dbus/fake_power_manager_client.h:72: void SuspendReadinessCallback();
nit: HandleSuspendReadiness()? (i'm addicted to verbs)

[Chirantan Ekbote, 2014-09-12 22:35:09]

ptal

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/freezer_cgroup_process_manager.h (right):

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/freezer_cgroup_process_manager.h:14: class
FreezerCgroupProcessManager : public RendererFreezer::Delegate {
I named it this because I'll be adding more code to this class to actually
manage the processes in the freezer cgroup.

[Daniel Erat, 2014-09-12 22:53:22]

lgtm with some comments

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/freezer_cgroup_process_manager.cc (right):

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/freezer_cgroup_process_manager.cc:35: return
base::WriteFile(state_path_, kFreezeCommand, strlen(kFreezeCommand)) ==
nit: i don't think that WriteFile logs any details about errors, so it might be
worthwhile adding a helper method, e.g.:

  bool WriteCommandToStateFile(const std::string& command) {
    int bytes = base::WriteFile(state_path_, command.c_str(), command.size());
    if (bytes == -1) {
      PLOG(ERROR) << "Writing " << command << " to " << state_path_ << "
failed";
      return false;
    } else if (bytes != command.size()) {
      LOG(ERROR) << "Only wrote " << bytes << " byte(s) when writing "
                 << command << " to " << state_path_;
      return false;
    }
    return true;
  }

(there really ought to be a helper function for this in file_util.h :-/)

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/freezer_cgroup_process_manager.h (right):

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/freezer_cgroup_process_manager.h:13: // Manages
all the processes in the freezer cgroup on chrome os.
nit: s/chrome os/Chrome OS/

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/freezer_cgroup_process_manager.h:27: };
include base/macros.h and add DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/renderer_freezer_unittest.cc (right):

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_unittest.cc:122: // it doesn't
hurt to check.
your call to DBusThreadManager::GetSetterForTesting() in the c'tor does
initialization if needed, iirc. it would be better to crash here if the thread
manager somehow wasn't initialized, in my opinion.

[Chirantan Ekbote, 2014-09-16 02:51:41]

Please take another look.  I moved some things around and added another test.

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/renderer_freezer_unittest.cc (right):

https://codereview.chromium.org/543303002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/renderer_freezer_unittest.cc:122: // it doesn't
hurt to check.
On 2014/09/12 at 22:53:22, Daniel Erat wrote:
> your call to DBusThreadManager::GetSetterForTesting() in the c'tor does
initialization if needed, iirc. it would be better to crash here if the thread
manager somehow wasn't initialized, in my opinion.

You're right.  This is no longer necessary.

[Daniel Erat, 2014-09-16 04:42:47]

lgtm

https://codereview.chromium.org/543303002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/freezer_cgroup_process_manager.h (right):

https://codereview.chromium.org/543303002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/freezer_cgroup_process_manager.h:8: #include
"base/files/file_path.h"
nit: add base/macros.h for DISALLOW_COPY_AND_ASSIGN

[Chirantan Ekbote, 2014-09-16 17:37:32]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-09-16 17:38:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543303002/100001

[commit-bot: I haz the power, 2014-09-16 18:41:30]

Committed patchset #6 (id:100001) as 04d9f351abf634522a39e0953a53e53b27f8c56c

[commit-bot: I haz the power, 2014-09-16 18:42:55]

Patchset 6 (id:??) landed as
https://crrev.com/b651946eea95c8af526e4df1e06809385f8cb822
Cr-Commit-Position: refs/heads/master@{#295111}