Plugin Power Saver: Fix poster-click flaky test.

Plugin Power Saver: Fix poster-click flaky test.

BUG=481687
Committed: https://crrev.com/6b28a8e582113e0417dc2fecc54c324a902fe9ec
Cr-Commit-Position: refs/heads/master@{#329293}

[tommycli, 2015-05-08 19:25:04]

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

[commit-bot: I haz the power, 2015-05-08 19:26:44]

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

[tommycli, 2015-05-08 19:26:57]

tommycli@chromium.org changed reviewers:
+ bauerb@chromium.org, thestig@chromium.org

[tommycli, 2015-05-08 19:26:58]

thestig/bauerb:

I think this is the smallest CL that will fix that flaky test.

It might not be worth it though, given that we have to add another Chrome flag.

If you guys think it's not worth it, I have no problem with simply removing the
'click' portion of that browsertest - it's not a big deal.

https://codereview.chromium.org/1132873002/diff/1/content/renderer/pepper/plu...
File content/renderer/pepper/plugin_module.cc (right):

https://codereview.chromium.org/1132873002/diff/1/content/renderer/pepper/plu...
content/renderer/pepper/plugin_module.cc:195: static void
PostPowerSaverStatusToJavaScript(
The changes in this file are necessary because previously, plugins without
throttlers were not responding to getPowerSaverStatus requests.

When we disabled the test originally, these requests were working (a different
way), and this functionality regressed while the test was disabled.

[commit-bot: I haz the power, 2015-05-08 19:34:25]

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

[commit-bot: I haz the power, 2015-05-08 19:34:27]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bernhard Bauer, 2015-05-11 08:03:53]

Thanks! I have to say, I like this. It is indeed a bit sad that we need to add
another command line flag, but maybe we could make it something like
--enable-testing=plugin_placeholder, so other parts of the code that want to
expose testing functionality would not have to add their own flag?

https://codereview.chromium.org/1132873002/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1132873002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:379: // Enables testing features of
the Plugin Placeholder.
Can you add "For internal use only"?

[tommycli, 2015-05-11 17:00:08]

bauerb: thanks! The --enable-testing=plugin_placeholder would be nice if other
enable-x-testing flags used it, but i worry about introducing something
inconsistent with other flags.

https://codereview.chromium.org/1132873002/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1132873002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:379: // Enables testing features of
the Plugin Placeholder.
On 2015/05/11 08:03:53, Bernhard Bauer wrote:
> Can you add "For internal use only"?

Done.

[Bernhard Bauer, 2015-05-11 18:25:25]

On 2015/05/11 17:00:08, tommycli wrote:
> bauerb: thanks! The --enable-testing=plugin_placeholder would be nice if other
> enable-x-testing flags used it, but i worry about introducing something
> inconsistent with other flags.

What do you mean by that?

Anyway, this LGTM.

[tommycli, 2015-05-11 18:34:21]

On 2015/05/11 18:25:25, Bernhard Bauer wrote:
> On 2015/05/11 17:00:08, tommycli wrote:
> > bauerb: thanks! The --enable-testing=plugin_placeholder would be nice if
other
> > enable-x-testing flags used it, but i worry about introducing something
> > inconsistent with other flags.
> 
> What do you mean by that?
> 
> Anyway, this LGTM.

Hey bernhard: I just meant there were some other flags that followed the
--enable-x-testing pattern:

https://code.google.com/p/chromium/codesearch#search/&q=%5C%22enable-.*?-test...

And I was hesitant to diverge from that and introduce something new... unless we
migrated everything to the --enable-testing=pepper,plugin_placeholder,foo,bar
scheme, which might be good actually.

[tommycli, 2015-05-11 20:12:16]

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

[tommycli, 2015-05-11 20:12:16]

piman: content/ ptal, thanks!

[tommycli, 2015-05-11 20:15:30]

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

[commit-bot: I haz the power, 2015-05-11 20:15:44]

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

[commit-bot: I haz the power, 2015-05-11 20:23:14]

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

[commit-bot: I haz the power, 2015-05-11 20:23:15]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piman, 2015-05-11 21:45:00]

LGTM. I'm not hot on adding test code to run-time, but I don't have a better
solution.

[Lei Zhang, 2015-05-11 22:26:20]

lgtm

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

https://codereview.chromium.org/1132873002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:137: "if
(plugin.hasAttribute('placeholderLoaded')) {"
Just do this instead?

if (plugin.hasAttribute('placeholderLoaded')) {
  window.domAutomationController.send('ready');
} else {
  plugin.addEventListener('message', handleEvent);
}

https://codereview.chromium.org/1132873002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:142: EXPECT_EQ("ready",
result);
ASSERT_EQ() ? Otherwise, if this fails, you would end up timing out like before,
right?

[tommycli, 2015-05-11 23:23:48]

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

[tommycli, 2015-05-11 23:23:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, bauerb@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1132873002/#ps40001
(title: "change expect_eq to assert_eq")

[tommycli, 2015-05-11 23:24:10]

thestig: thanks!

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

https://codereview.chromium.org/1132873002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:137: "if
(plugin.hasAttribute('placeholderLoaded')) {"
On 2015/05/11 22:26:20, Lei Zhang wrote:
> Just do this instead?
> 
> if (plugin.hasAttribute('placeholderLoaded')) {
>   window.domAutomationController.send('ready');
> } else {
>   plugin.addEventListener('message', handleEvent);
> }

Hi. That's what I initially did, but it flaked out occasionally. I think it had
this race condition:

Test process: Check attribute, looks like it hasn't loaded yet...
Render process: Set attribute, fire DOM event. Looks like there's no listener,
drop DOM event.
Test process: Now I'm in the else-clause. Add event listener... and hang.

https://codereview.chromium.org/1132873002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:142: EXPECT_EQ("ready",
result);
On 2015/05/11 22:26:20, Lei Zhang wrote:
> ASSERT_EQ() ? Otherwise, if this fails, you would end up timing out like
before,
> right?

Done. good call

[tommycli, 2015-05-11 23:24:56]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2015-05-11 23:24:57]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-05-11 23:25:45]

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

[commit-bot: I haz the power, 2015-05-12 00:26:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-12 00:27:25]

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