Plugin Power Saver: Refactor Plugin Instance Throttling code

Plugin Power Saver: Refactor Plugin Instance Throttling code

Currently we are polluting the already-crowded PepperPluginInstanceImpl
class. This patch moves a lot of the Power Saver code to its own class.

BUG=403800
Committed: https://crrev.com/958f5e401524d8a31c5cf9228b3c9adace3d30ff
Cr-Commit-Position: refs/heads/master@{#304066}

[tommycli, 2014-11-07 01:34:47]

tommycli@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2014-11-10 02:34:20]

Thanks for doing this! I haven't really reviewed properly but had one comment
while skimming it.

https://codereview.chromium.org/707113002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/707113002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_instance_impl.cc:844: this,
base::Bind(&PepperPluginInstanceImpl::SendDidChangeView,
Since we're already passing the PepperPluginInstanceImpl* can we avoid passing
the callback?

[tommycli, 2014-11-11 19:54:30]

raymes: let me know what you think. :)

https://codereview.chromium.org/707113002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/707113002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_instance_impl.cc:844: this,
base::Bind(&PepperPluginInstanceImpl::SendDidChangeView,
On 2014/11/10 02:34:20, raymes wrote:
> Since we're already passing the PepperPluginInstanceImpl* can we avoid passing
> the callback? 

Yes we can, but then we have to make SendDidChangeView a public method. Is that
what you prefer?

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc
(right):

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:28: void
SetPluginThrottledForTest(bool throttled) {
This seems a bit janky. Let me know if you have a better suggestion.

[raymes, 2014-11-12 06:38:20]

Some initial comments, mainly on the test.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1653: : view_data_;
nit: indentation looks odd. 4 spaces from the margin is normal.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_throttler.cc (right):

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:111:
PepperPluginInstanceImpl* instance,
It looks like if we pass in:
-The PluginPowerSaverHelper*
-The blink::WebRect (bounds)
-The module name
-Plugin GURL

We can decouple this from the PepperPluginInstanceImpl* which is nice for
testing too.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:163:
PepperPluginInstanceThrottler::PepperPluginInstanceThrottler(
It's better to avoid having a separate constructor for testing. If we make the
change suggested above, can we use the same constructor for testing? This would
allow you to test the code in the constructor too if you needed to.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc
(right):

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:15: class
TestPepperPluginInstanceThrottler : public PepperPluginInstanceThrottler {
I think we can avoid inheriting from the throttler. It looks like you mainly
just want to call some private functions. The usual pattern is to have a test
class (PepperPluginInstanceThrottlerTest) which inherits from testing::Test.
Then you make the test class a friend of the PepperPluginInstanceThrottler. 

Note that you won't be able to call directly into the private methods from
inside the TEST macros below - you may need to have helper functions which do
that.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:28: void
SetPluginThrottledForTest(bool throttled) {
On 2014/11/11 19:54:30, tommycli wrote:
> This seems a bit janky. Let me know if you have a better suggestion.

Rather than calling SetPluginThrottled directly, couldn't you call
ConsumeInputEvent with a crafted input event? That would test the logic there as
well.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:33:
DisablePowerSaverByRetroactiveWhitelist();
Similarly here, you could instead pass in a mocked version of the
PluginPowerSaverHelper which gave you control over when this callback was
called.

[tommycli, 2014-11-12 21:18:14]

raymes: I was able to take most of your suggestions. Hopefully this looks less
janky now :)

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1653: : view_data_;
On 2014/11/12 06:38:20, raymes wrote:
> nit: indentation looks odd. 4 spaces from the margin is normal.

I'm just running git cl format on everything.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_throttler.cc (right):

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:111:
PepperPluginInstanceImpl* instance,
On 2014/11/12 06:38:20, raymes wrote:
> It looks like if we pass in:
> -The PluginPowerSaverHelper*
> -The blink::WebRect (bounds)
> -The module name
> -Plugin GURL
> 
> We can decouple this from the PepperPluginInstanceImpl* which is nice for
> testing too.

Done.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:163:
PepperPluginInstanceThrottler::PepperPluginInstanceThrottler(
On 2014/11/12 06:38:20, raymes wrote:
> It's better to avoid having a separate constructor for testing. If we make the
> change suggested above, can we use the same constructor for testing? This
would
> allow you to test the code in the constructor too if you needed to. 

Done.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc
(right):

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:15: class
TestPepperPluginInstanceThrottler : public PepperPluginInstanceThrottler {
On 2014/11/12 06:38:20, raymes wrote:
> I think we can avoid inheriting from the throttler. It looks like you mainly
> just want to call some private functions. The usual pattern is to have a test
> class (PepperPluginInstanceThrottlerTest) which inherits from testing::Test.
> Then you make the test class a friend of the PepperPluginInstanceThrottler. 
> 
> Note that you won't be able to call directly into the private methods from
> inside the TEST macros below - you may need to have helper functions which do
> that.

Done.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:28: void
SetPluginThrottledForTest(bool throttled) {
On 2014/11/12 06:38:20, raymes wrote:
> On 2014/11/11 19:54:30, tommycli wrote:
> > This seems a bit janky. Let me know if you have a better suggestion.
> 
> Rather than calling SetPluginThrottled directly, couldn't you call
> ConsumeInputEvent with a crafted input event? That would test the logic there
as
> well.

ConsumeInputEvent can only unthrottle, not throttle.

https://codereview.chromium.org/707113002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:33:
DisablePowerSaverByRetroactiveWhitelist();
On 2014/11/12 06:38:20, raymes wrote:
> Similarly here, you could instead pass in a mocked version of the
> PluginPowerSaverHelper which gave you control over when this callback was
> called.

The 'real' PluginPowerSaverHelper underlying the mock relies on IPC, and didn't
quite work right when I tried to use it to trigger the retroactive whitelist.

[raymes, 2014-11-13 00:05:16]

Looks close. Just a few more suggestions.

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc
(right):

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:70:
throttler_->DisablePowerSaverByRetroactiveWhitelist();
You can avoid needing to make this public without too much trouble. The idea is
that in MockPluginPowerSaverHelper you provide an implementation of
RegisterPeripheralPlugin which just stores the callback as a local variable.
Then you can access the callback through power_saver_helper_.

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:88:
SetPluginThrottledForTest(true);
Rather than calling SetPluginThrottledForTest(true) another option is to use a
RunLoop to pump the message loop and simulate the real behavior. But it's a bit
of work to do and I don't want to hold this up on it.

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:92:
SetPluginThrottledForTest(false);
Can we use ConsumeInputEvent here instead?

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:131:
EXPECT_TRUE(throttler()->ConsumeInputEvent(event));
A few more tests for ConumeInputEvent could be good:
1) Trying to unthrottle with any event other than a click doesn't unthrottle and
consumes the event
2) Trying to unthrottle with a click if the plugin is a) already unthrottled or
b) peripheral doesn't do anything and doesn't consume the event.

[tommycli, 2014-11-13 02:33:36]

raymes: Let me know what you think. Thanks!

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc
(right):

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:70:
throttler_->DisablePowerSaverByRetroactiveWhitelist();
On 2014/11/13 00:05:16, raymes wrote:
> You can avoid needing to make this public without too much trouble. The idea
is
> that in MockPluginPowerSaverHelper you provide an implementation of
> RegisterPeripheralPlugin which just stores the callback as a local variable.
> Then you can access the callback through power_saver_helper_.

Done.

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:88:
SetPluginThrottledForTest(true);
On 2014/11/13 00:05:16, raymes wrote:
> Rather than calling SetPluginThrottledForTest(true) another option is to use a
> RunLoop to pump the message loop and simulate the real behavior. But it's a
bit
> of work to do and I don't want to hold this up on it.

The current 'real' behavior is to simply wait 5 seconds, then engage the
throttle. That's so lame as to not worth be testing imo.

The next CL (coming soon) will add a luma histogram based frame inspecting
heuristic. If it's okay with you, we can add a more legit test then?

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:92:
SetPluginThrottledForTest(false);
On 2014/11/13 00:05:16, raymes wrote:
> Can we use ConsumeInputEvent here instead?

Done. I consolidated this with UnthrottleByClick now, since it now becomes the
same test.

https://codereview.chromium.org/707113002/diff/100001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler_unittest.cc:131:
EXPECT_TRUE(throttler()->ConsumeInputEvent(event));
On 2014/11/13 00:05:16, raymes wrote:
> A few more tests for ConumeInputEvent could be good:
> 1) Trying to unthrottle with any event other than a click doesn't unthrottle
and
> consumes the event
> 2) Trying to unthrottle with a click if the plugin is a) already unthrottled
or
> b) peripheral doesn't do anything and doesn't consume the event.

Done. Added a few more tests. Actually this caught a bug :)

[raymes, 2014-11-13 02:42:43]

lgtm

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_throttler.cc (right):

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:14: #include
"content/renderer/pepper/plugin_module.h"
nit: I think we can get rid of this

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:16: #include
"content/renderer/render_frame_impl.h"
nit: I think we can get rid of this

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:18: #include
"third_party/WebKit/public/web/WebElement.h"
nit: I think we can get rid of this

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:20: #include
"third_party/WebKit/public/web/WebPluginContainer.h"
nit: I think we can get rid of this

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:52: const int
kThrottleTimeout = 5000;
nit: perhaps move this below all the histogram stuff

[tommycli, 2014-11-13 19:03:41]

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_throttler.cc (right):

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:14: #include
"content/renderer/pepper/plugin_module.h"
On 2014/11/13 02:42:43, raymes wrote:
> nit: I think we can get rid of this

Done.

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:16: #include
"content/renderer/render_frame_impl.h"
On 2014/11/13 02:42:43, raymes wrote:
> nit: I think we can get rid of this

Done.

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:18: #include
"third_party/WebKit/public/web/WebElement.h"
On 2014/11/13 02:42:43, raymes wrote:
> nit: I think we can get rid of this

Done.

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:20: #include
"third_party/WebKit/public/web/WebPluginContainer.h"
On 2014/11/13 02:42:43, raymes wrote:
> nit: I think we can get rid of this

Done.

https://codereview.chromium.org/707113002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_throttler.cc:52: const int
kThrottleTimeout = 5000;
On 2014/11/13 02:42:43, raymes wrote:
> nit: perhaps move this below all the histogram stuff

Done.

[tommycli, 2014-11-13 19:04:14]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-11-13 19:05:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/707113002/140001

[commit-bot: I haz the power, 2014-11-13 20:07:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-13 20:08:09]

Patchset 8 (id:??) landed as
https://crrev.com/958f5e401524d8a31c5cf9228b3c9adace3d30ff
Cr-Commit-Position: refs/heads/master@{#304066}