Plugin Power Saver: Add comprehensive browser tests.

Plugin Power Saver: Add comprehensive browser tests.

These browser tests should end the need for constant manual testing.
These have been a long time coming.

BUG=475688
Committed: https://crrev.com/0dd13013ffb72091053f670e5c883acb65017422
Cr-Commit-Position: refs/heads/master@{#326867}

[tommycli, 2015-04-14 18:30:55]

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

[tommycli, 2015-04-14 18:30:56]

raymes / teravest:

This patch is a followup to https://codereview.chromium.org/1062163004/.

I had a question below:

Thansk!

https://codereview.chromium.org/1088763002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/message_channel.h (right):

https://codereview.chromium.org/1088763002/diff/20001/content/renderer/pepper...
content/renderer/pepper/message_channel.h:121: void
IsPeripheralToNative(gin::Arguments* args);
This is an extremely convenient place to add a JS call, but isn't in the named
purpose of the class.

Should we change this class name to PluginInstanceJavascriptInterface?

Or should we make a new gin::NamedPropertyInterceptor? Can a Pepper plugin have
two gin::NamedPropertyInterceptor members ?

Thanks!

[Lei Zhang, 2015-04-15 05:52:44]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-04-15 05:52:45]

https://codereview.chromium.org/1088763002/diff/80001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:88:
EXPECT_TRUE(IsPluginPeripheral("plugin"));
Can you remind me why this is peripheral? Is is the poster?

[Lei Zhang, 2015-04-15 07:04:08]

Also, the browser tests are flaky. Sometimes IsPluginPeripheral() will return
true when the expected result is false. Perhaps you are checking before the
plugins finished loading or something?

[Lei Zhang, 2015-04-15 07:29:38]

On 2015/04/15 05:52:45, Lei Zhang (slow to respond) wrote:
>
https://codereview.chromium.org/1088763002/diff/80001/chrome/browser/plugins/...
> File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1088763002/diff/80001/chrome/browser/plugins/...
> chrome/browser/plugins/plugin_power_saver_browsertest.cc:88:
> EXPECT_TRUE(IsPluginPeripheral("plugin"));
> Can you remind me why this is peripheral? Is is the poster?

Oh, I read the poster attribute description. It's just a bit confusing because
the plugin here is a large plugin that's not peripheral, but rather, just
throttled.

[tommycli, 2015-04-15 22:30:12]

On 2015/04/15 07:04:08, Lei Zhang (slow to respond) wrote:
> Also, the browser tests are flaky. Sometimes IsPluginPeripheral() will return
> true when the expected result is false. Perhaps you are checking before the
> plugins finished loading or something?

So ridiculously flaky. I made some changes that fix the flakiness (on my
machine).

The mouse click thing is tricky, as the click needs to get to the renderer
process, the renderer process needs to do some stuff, and eventually some time
later, I can call 'isPeripheral' and get a false statement.

Hopefully you think the setTimeout hack is alright, otherwise we have to add
some kind of observer that can send a notification when the plugin is
unthrottled.

[tommycli, 2015-04-15 22:30:28]

https://codereview.chromium.org/1088763002/diff/80001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:88:
EXPECT_TRUE(IsPluginPeripheral("plugin"));
On 2015/04/15 05:52:44, Lei Zhang (slow to respond) wrote:
> Can you remind me why this is peripheral? Is is the poster?

Yes. Large plugins with posters are not loaded immediately.

Yea I guess they're technically not 'peripheral', depending on how we choose to
define that word.

[raymes, 2015-04-16 02:27:57]

I didn't review the browser test itself, just the pepper stuff.

https://codereview.chromium.org/1088763002/diff/120001/content/renderer/peppe...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/1088763002/diff/120001/content/renderer/peppe...
content/renderer/pepper/message_channel.cc:235: ->GetFunction();
We should avoid changing the MessageChannel just for test code. Can you utilise
PostMessage, e.g 

plugin.addEventListener('message', function(event) { 
  var isPeripheral = event.data.isPeripheral;
}
plugin.postMessage('isPeripheral');

And then you implement a message handler in the plugin. You can modify
PPB_Testing_Private to add a function which returns whether the function is
peripheral.

Would that be sufficient?

[raymes, 2015-04-16 02:28:34]

On 2015/04/16 02:27:57, raymes wrote:
> I didn't review the browser test itself, just the pepper stuff.
> 
>
https://codereview.chromium.org/1088763002/diff/120001/content/renderer/peppe...
> File content/renderer/pepper/message_channel.cc (right):
> 
>
https://codereview.chromium.org/1088763002/diff/120001/content/renderer/peppe...
> content/renderer/pepper/message_channel.cc:235: ->GetFunction();
> We should avoid changing the MessageChannel just for test code. Can you
utilise
> PostMessage, e.g 
> 
> plugin.addEventListener('message', function(event) { 
>   var isPeripheral = event.data.isPeripheral;
> }
> plugin.postMessage('isPeripheral');
> 
> And then you implement a message handler in the plugin. You can modify
> PPB_Testing_Private to add a function which returns whether the function is
> peripheral.

*whether the *plugin* is peripheral :)

> 
> Would that be sufficient?

[tommycli, 2015-04-16 23:51:21]

Patchset #9 (id:160001) has been deleted

[tommycli, 2015-04-17 00:00:20]

Patchset #9 (id:180001) has been deleted

[tommycli, 2015-04-17 00:02:17]

raymes: I moved it to PPB_Testing_Private. See if this one appears better to
you. Thanks!

https://codereview.chromium.org/1088763002/diff/120001/content/renderer/peppe...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/1088763002/diff/120001/content/renderer/peppe...
content/renderer/pepper/message_channel.cc:235: ->GetFunction();
On 2015/04/16 02:27:57, raymes wrote:
> We should avoid changing the MessageChannel just for test code. Can you
utilise
> PostMessage, e.g 
> 
> plugin.addEventListener('message', function(event) { 
>   var isPeripheral = event.data.isPeripheral;
> }
> plugin.postMessage('isPeripheral');
> 
> And then you implement a message handler in the plugin. You can modify
> PPB_Testing_Private to add a function which returns whether the function is
> peripheral.
> 
> Would that be sufficient?
> 
> 

Done.

[raymes, 2015-04-17 01:13:45]

https://codereview.chromium.org/1088763002/diff/200001/content/renderer/peppe...
File content/renderer/pepper/plugin_power_saver_helper.cc (right):

https://codereview.chromium.org/1088763002/diff/200001/content/renderer/peppe...
content/renderer/pepper/plugin_power_saver_helper.cc:12: #include
"content/public/test/ppapi_test_utils.h"
nit: is this needed?

https://codereview.chromium.org/1088763002/diff/200001/ppapi/tests/testing_in...
File ppapi/tests/testing_instance.cc (right):

https://codereview.chromium.org/1088763002/diff/200001/ppapi/tests/testing_in...
ppapi/tests/testing_instance.cc:105: }
It's a bit weird to have this here because this is the plugin which is actually
used to run tests rather than to be used by other tests. 

I wonder if we should create a separate, small plugin which just contains this
logic and can be used in your new test. You can look at ppapi/examples for
inspiration.

[raymes, 2015-04-17 01:13:57]

Thanks!

[tommycli, 2015-04-17 23:45:32]

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

[tommycli, 2015-04-17 23:45:33]

raymes: I made this a separate plugin. Seems to work!
tsepez: May I have a security review of *_messages.h please, thanks!

https://codereview.chromium.org/1088763002/diff/200001/content/renderer/peppe...
File content/renderer/pepper/plugin_power_saver_helper.cc (right):

https://codereview.chromium.org/1088763002/diff/200001/content/renderer/peppe...
content/renderer/pepper/plugin_power_saver_helper.cc:12: #include
"content/public/test/ppapi_test_utils.h"
On 2015/04/17 01:13:44, raymes wrote:
> nit: is this needed?

Done.

https://codereview.chromium.org/1088763002/diff/200001/ppapi/tests/testing_in...
File ppapi/tests/testing_instance.cc (right):

https://codereview.chromium.org/1088763002/diff/200001/ppapi/tests/testing_in...
ppapi/tests/testing_instance.cc:105: }
On 2015/04/17 01:13:44, raymes wrote:
> It's a bit weird to have this here because this is the plugin which is
actually
> used to run tests rather than to be used by other tests. 
> 
> I wonder if we should create a separate, small plugin which just contains this
> logic and can be used in your new test. You can look at ppapi/examples for
> inspiration.

Done.

[raymes, 2015-04-20 01:05:32]

lgtm with nits
Thanks!

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
File ppapi/tests/power_saver_test_plugin.cc (right):

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:20: // PostMessage.
nit: please change the comment

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:22: // This object represents one time
the page says <embed>.
nit remove the comment

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:34: // JavaScript indicating whether or
not that string is a palindrome.
nit: please change the comment

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:46: // as it is loaded.
nit remove the comment

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:52: // Override CreateInstance to create
your customized Instance object.
nit remove the comment

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:60: // Factory function for your
specialization of the Module object.
nit: remove the comment

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/testing_in...
File ppapi/tests/testing_instance.cc (right):

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/testing_in...
ppapi/tests/testing_instance.cc:19: #include "ppapi/tests/test_utils.h"
nit: I think these can be removed now?

[Tom Sepez, 2015-04-20 15:43:29]

Tommy - what happens if your new message happens to arrive at a production
plugin rather than your test plugin?  Presumably it's ignored, but I wasn't sure
where that happened.  Otherwise LGTM.

[tommycli, 2015-04-20 16:26:39]

On 2015/04/20 15:43:29, Tom Sepez wrote:
> Tommy - what happens if your new message happens to arrive at a production
> plugin rather than your test plugin?  Presumably it's ignored, but I wasn't
sure
> where that happened.  Otherwise LGTM.

tsepez: The Pepper Testing interface is only created when
--enable-pepper-testing flag is passed in:

https://code.google.com/p/chromium/codesearch/#chromium/src/content/renderer/...

Thanks!

[Tom Sepez, 2015-04-20 16:33:20]

> tsepez: The Pepper Testing interface is only created when
> --enable-pepper-testing flag is passed in:
right. LGTM.

[tommycli, 2015-04-20 18:03:59]

tommycli@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[tommycli, 2015-04-20 18:04:01]

raymes: thanks!

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
File ppapi/tests/power_saver_test_plugin.cc (right):

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:20: // PostMessage.
On 2015/04/20 01:05:32, raymes wrote:
> nit: please change the comment

Done.

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:22: // This object represents one time
the page says <embed>.
On 2015/04/20 01:05:31, raymes wrote:
> nit remove the comment

Done.

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:34: // JavaScript indicating whether or
not that string is a palindrome.
On 2015/04/20 01:05:31, raymes wrote:
> nit: please change the comment

Done.

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:46: // as it is loaded.
On 2015/04/20 01:05:32, raymes wrote:
> nit remove the comment

Done.

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:52: // Override CreateInstance to create
your customized Instance object.
On 2015/04/20 01:05:31, raymes wrote:
> nit remove the comment

Done.

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:60: // Factory function for your
specialization of the Module object.
On 2015/04/20 01:05:31, raymes wrote:
> nit: remove the comment

Done.

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/testing_in...
File ppapi/tests/testing_instance.cc (right):

https://codereview.chromium.org/1088763002/diff/220001/ppapi/tests/testing_in...
ppapi/tests/testing_instance.cc:19: #include "ppapi/tests/test_utils.h"
On 2015/04/20 01:05:32, raymes wrote:
> nit: I think these can be removed now?

Done.

[tommycli, 2015-04-20 18:04:14]

thestig: ptal chrome/

phajdan.jr: ptal content/public/test

thanks all

[Paweł Hajdan Jr., 2015-04-20 22:38:17]

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:77: "   
setTimeout(checkEssential, 50);"
Is there no way to wait for a proper event instead of looping?

https://codereview.chromium.org/1088763002/diff/280001/content/public/test/pp...
File content/public/test/ppapi_test_utils.h (right):

https://codereview.chromium.org/1088763002/diff/280001/content/public/test/pp...
content/public/test/ppapi_test_utils.h:35: bool
RegisterPowerSaverTestPlugin(base::CommandLine* command_line);
Please add WARN_UNUSED_RESULT.

[tommycli, 2015-04-20 23:25:30]

phajdan.jr: Thanks!

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:77: "   
setTimeout(checkEssential, 50);"
On 2015/04/20 22:38:17, Paweł Hajdan Jr. wrote:
> Is there no way to wait for a proper event instead of looping?

Unfortunately, no. The replacement of the placeholder with the actual plugin
happens asynchronously in renderer process in response to the click.

Hypothetically we could hook into some kind of interprocess notification
mechanism, but just doing this seemed simpler.

https://codereview.chromium.org/1088763002/diff/280001/content/public/test/pp...
File content/public/test/ppapi_test_utils.h (right):

https://codereview.chromium.org/1088763002/diff/280001/content/public/test/pp...
content/public/test/ppapi_test_utils.h:35: bool
RegisterPowerSaverTestPlugin(base::CommandLine* command_line);
On 2015/04/20 22:38:17, Paweł Hajdan Jr. wrote:
> Please add WARN_UNUSED_RESULT.

Done.

[Lei Zhang, 2015-04-21 01:28:14]

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:13: #include
"content/public/test/ppapi_test_utils.h"
not needed?

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:14: #include
"content/public/test/test_utils.h"
ditto

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:43: "var plugin =
window.document.getElementById('%s');"
You may want to also check that |plugin| is actually a plugin, and not some
other tag.

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:77: "   
setTimeout(checkEssential, 50);"
Can you add an onclick handler to the placeholder, and have it do something that
the browser can observe, like changing the page's title and observe that with a
TitleWatcher?

[Paweł Hajdan Jr., 2015-04-21 15:54:49]

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:77: "   
setTimeout(checkEssential, 50);"
On 2015/04/20 23:25:30, tommycli wrote:
> On 2015/04/20 22:38:17, Paweł Hajdan Jr. wrote:
> > Is there no way to wait for a proper event instead of looping?
> 
> Unfortunately, no. The replacement of the placeholder with the actual plugin
> happens asynchronously in renderer process in response to the click.
> 
> Hypothetically we could hook into some kind of interprocess notification
> mechanism, but just doing this seemed simpler.

That sounds like "there is currently no easy way to do that", but I'd like to
understand it better.

Waiting for an even is strongly preferred, and it'll make all tests that need it
more solid (and likely easier to understand - you can easily identify which
places in the code trigger the even you're waiting for).

Could you explain more what would it take to implement some event? Maybe it
could be done all renderer-side, just trigger an event JS can wait for. I'm in
MTV this week, we can VC if that would help.

[tommycli, 2015-04-21 16:35:07]

On 2015/04/21 15:54:49, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
> File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
> chrome/browser/plugins/plugin_power_saver_browsertest.cc:77: "   
> setTimeout(checkEssential, 50);"
> On 2015/04/20 23:25:30, tommycli wrote:
> > On 2015/04/20 22:38:17, Paweł Hajdan Jr. wrote:
> > > Is there no way to wait for a proper event instead of looping?
> > 
> > Unfortunately, no. The replacement of the placeholder with the actual plugin
> > happens asynchronously in renderer process in response to the click.
> > 
> > Hypothetically we could hook into some kind of interprocess notification
> > mechanism, but just doing this seemed simpler.
> 
> That sounds like "there is currently no easy way to do that", but I'd like to
> understand it better.
> 
> Waiting for an even is strongly preferred, and it'll make all tests that need
it
> more solid (and likely easier to understand - you can easily identify which
> places in the code trigger the even you're waiting for).
> 
> Could you explain more what would it take to implement some event? Maybe it
> could be done all renderer-side, just trigger an event JS can wait for. I'm in
> MTV this week, we can VC if that would help.

Hey guys, yes, your interpretation of "there is currently no easy way to do
that" is correct.

Since it seems important to both you and thestig, I will try harder to make this
a non-waiting thing.

phajdan.jr, I will IM you or VC with you if I get stuck.

[tommycli, 2015-04-22 18:17:46]

thestig, phajdan.jr: Okay I was able to get rid of the polling and move to an
event-based mechanism in the latest patch.

It took me two solid days and trying about 5 different methods that didn't work.
I hope you guys like this. :)

Tommy

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/280001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:77: "   
setTimeout(checkEssential, 50);"
On 2015/04/21 15:54:49, Paweł Hajdan Jr. wrote:
> On 2015/04/20 23:25:30, tommycli wrote:
> > On 2015/04/20 22:38:17, Paweł Hajdan Jr. wrote:
> > > Is there no way to wait for a proper event instead of looping?
> > 
> > Unfortunately, no. The replacement of the placeholder with the actual plugin
> > happens asynchronously in renderer process in response to the click.
> > 
> > Hypothetically we could hook into some kind of interprocess notification
> > mechanism, but just doing this seemed simpler.
> 
> That sounds like "there is currently no easy way to do that", but I'd like to
> understand it better.
> 
> Waiting for an even is strongly preferred, and it'll make all tests that need
it
> more solid (and likely easier to understand - you can easily identify which
> places in the code trigger the even you're waiting for).
> 
> Could you explain more what would it take to implement some event? Maybe it
> could be done all renderer-side, just trigger an event JS can wait for. I'm in
> MTV this week, we can VC if that would help.

Done.

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:13: #include
"content/public/test/ppapi_test_utils.h"
On 2015/04/21 01:28:14, Lei Zhang wrote:
> not needed?

Needed for Line 25.

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:14: #include
"content/public/test/test_utils.h"
On 2015/04/21 01:28:14, Lei Zhang wrote:
> ditto

Done.

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:43: "var plugin =
window.document.getElementById('%s');"
On 2015/04/21 01:28:14, Lei Zhang wrote:
> You may want to also check that |plugin| is actually a plugin, and not some
> other tag.

Done.

https://codereview.chromium.org/1088763002/diff/300001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:77: "   
setTimeout(checkEssential, 50);"
On 2015/04/21 01:28:14, Lei Zhang wrote:
> Can you add an onclick handler to the placeholder, and have it do something
that
> the browser can observe, like changing the page's title and observe that with
a
> TitleWatcher?

Done. Moved to a different mechanism.

[Lei Zhang, 2015-04-22 23:35:20]

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:45: " 
window.domAutomationController.send(-1);"
Aren't you trying to extract a bool? i.e. the return value should only be
true/false and not -1.

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:45: " 
window.domAutomationController.send(-1);"
and return here, since we already called send() ?

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:68: // This sends a
simulated click at |point| and waits (by polling) for the
(by polling) .... not any more!

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:80: " 
window.domAutomationController.send(-1);"
Same issue as the JS in IsPluginPeripheral().

https://codereview.chromium.org/1088763002/diff/340001/ppapi/tests/power_save...
File ppapi/tests/power_saver_test_plugin.cc (right):

https://codereview.chromium.org/1088763002/diff/340001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:13: // When compiling natively on
Windows, PostMessage can be #define-d to
Is this needed? ppapi/cpp/instance.h already does this.

[tommycli, 2015-04-23 20:11:23]

thestig: thanks!

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:45: " 
window.domAutomationController.send(-1);"
On 2015/04/22 23:35:20, Lei Zhang wrote:
> and return here, since we already called send() ?

Done.

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:45: " 
window.domAutomationController.send(-1);"
On 2015/04/22 23:35:20, Lei Zhang wrote:
> Aren't you trying to extract a bool? i.e. the return value should only be
> true/false and not -1.

Well I was using -1 as a sentinel value. It would make Line 63 fail, as it would
not be able to extract a bool.

I changed it to use all integers, since the old way was confusing.

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:68: // This sends a
simulated click at |point| and waits (by polling) for the
On 2015/04/22 23:35:20, Lei Zhang wrote:
> (by polling) .... not any more!

Done.

https://codereview.chromium.org/1088763002/diff/340001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:80: " 
window.domAutomationController.send(-1);"
On 2015/04/22 23:35:20, Lei Zhang wrote:
> Same issue as the JS in IsPluginPeripheral().

Done.

https://codereview.chromium.org/1088763002/diff/340001/ppapi/tests/power_save...
File ppapi/tests/power_saver_test_plugin.cc (right):

https://codereview.chromium.org/1088763002/diff/340001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:13: // When compiling natively on
Windows, PostMessage can be #define-d to
On 2015/04/22 23:35:20, Lei Zhang wrote:
> Is this needed? ppapi/cpp/instance.h already does this.

I tried removing this, and the Windows trybots failed.

If it makes you feel any better,
https://code.google.com/p/chromium/codesearch/#chromium/src/ppapi/tests/testi...
also defines this again.

[Lei Zhang, 2015-04-23 22:38:03]

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:67: RESULT_PERIPHERAL,
might be easier to write:
var RESULT_PERIPHERAL = 1;

as JS, then you can just write RESULT_PERIPHERAL in place of the %d. As is, it's
hard to read because you have to look at a %d, figure out which %d it is, match
the arguments here, and remembeer there's also a %s ahead of it.

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:84: //  - In response
to the explicit 'getPeriheralStatus' request, in case the
typo

[Paweł Hajdan Jr., 2015-04-23 23:20:27]

LGTM, thanks!

[tommycli, 2015-04-24 18:08:26]

thestig: addressed your comments below. Thanks!

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:67: RESULT_PERIPHERAL,
On 2015/04/23 22:38:02, Lei Zhang wrote:
> might be easier to write:
> var RESULT_PERIPHERAL = 1;
> 
> as JS, then you can just write RESULT_PERIPHERAL in place of the %d. As is,
it's
> hard to read because you have to look at a %d, figure out which %d it is,
match
> the arguments here, and remembeer there's also a %s ahead of it.

Done. I changed it to just use string literals throughout. Let me know what you
think.

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:84: //  - In response
to the explicit 'getPeriheralStatus' request, in case the
On 2015/04/23 22:38:02, Lei Zhang wrote:
> typo

Done.

[tommycli, 2015-04-24 18:08:41]

tommycli@chromium.org changed reviewers:
- teravest@chromium.org

[Lei Zhang, 2015-04-24 18:30:36]

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:67: RESULT_PERIPHERAL,
On 2015/04/24 18:08:26, tommycli wrote:
> On 2015/04/23 22:38:02, Lei Zhang wrote:
> > might be easier to write:
> > var RESULT_PERIPHERAL = 1;
> > 
> > as JS, then you can just write RESULT_PERIPHERAL in place of the %d. As is,
> it's
> > hard to read because you have to look at a %d, figure out which %d it is,
> match
> > the arguments here, and remembeer there's also a %s ahead of it.
> 
> Done. I changed it to just use string literals throughout. Let me know what
you
> think.

Ok.

[tommycli, 2015-04-24 18:35:53]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2015-04-24 18:35:54]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
tsepez@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1088763002/#ps400001
(title: "x")

[commit-bot: I haz the power, 2015-04-24 18:36:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1088763002/400001

[tommycli, 2015-04-24 19:16:53]

On 2015/04/24 18:30:36, Lei Zhang wrote:
>
https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
> File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1088763002/diff/380001/chrome/browser/plugins...
> chrome/browser/plugins/plugin_power_saver_browsertest.cc:67:
RESULT_PERIPHERAL,
> On 2015/04/24 18:08:26, tommycli wrote:
> > On 2015/04/23 22:38:02, Lei Zhang wrote:
> > > might be easier to write:
> > > var RESULT_PERIPHERAL = 1;
> > > 
> > > as JS, then you can just write RESULT_PERIPHERAL in place of the %d. As
is,
> > it's
> > > hard to read because you have to look at a %d, figure out which %d it is,
> > match
> > > the arguments here, and remembeer there's also a %s ahead of it.
> > 
> > Done. I changed it to just use string literals throughout. Let me know what
> you
> > think.
> 
> Ok.

thestig: Thanks! I think I still need your L-G before CQ will take it. Thanks!

[commit-bot: I haz the power, 2015-04-24 19:22:39]

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

[commit-bot: I haz the power, 2015-04-24 19:22:41]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-04-24 20:12:15]

lgtm

[tommycli, 2015-04-24 20:25:28]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-04-24 20:25:49]

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

[commit-bot: I haz the power, 2015-04-24 20:27:36]

Committed patchset #19 (id:400001)

[commit-bot: I haz the power, 2015-04-24 20:28:19]

Patchset 19 (id:??) landed as
https://crrev.com/0dd13013ffb72091053f670e5c883acb65017422
Cr-Commit-Position: refs/heads/master@{#326867}