Plugin Power Saver: Implement pixel tests for plugin placeholders.

Plugin Power Saver: Implement pixel tests for plugin placeholders.

Tests these tough cases:
1) Centering the play icon for both posters and plugins with posters.
2) Displaying a scaled-down play icon for plugins smaller than the icon.
3) Partially obscured plugins and plugin posters.

BUG=544292
Committed: https://crrev.com/f1959fb91340917f97eee2c0aad26456a7a0a3c4
Cr-Commit-Position: refs/heads/master@{#356215}

[tommycli, 2015-10-20 01:15:45]

Description was changed from

==========
Plugin Power Saver: Implement pixel tests for plugin placeholders.

Tests these tough cases:
1) Centering the play icon for both posters and plugins with posters.
2) Displaying a scaled-down play icon for plugins smaller than the icon.

BUG=
==========

to

==========
Plugin Power Saver: Implement pixel tests for plugin placeholders.

Tests these tough cases:
1) Centering the play icon for both posters and plugins with posters.
2) Displaying a scaled-down play icon for plugins smaller than the icon.
3) Partially obscured plugins and plugin posters.

BUG=
==========

[tommycli, 2015-10-20 01:15:45]

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

[tommycli, 2015-10-20 01:18:15]

Description was changed from

==========
Plugin Power Saver: Implement pixel tests for plugin placeholders.

Tests these tough cases:
1) Centering the play icon for both posters and plugins with posters.
2) Displaying a scaled-down play icon for plugins smaller than the icon.
3) Partially obscured plugins and plugin posters.

BUG=
==========

to

==========
Plugin Power Saver: Implement pixel tests for plugin placeholders.

Tests these tough cases:
1) Centering the play icon for both posters and plugins with posters.
2) Displaying a scaled-down play icon for plugins smaller than the icon.
3) Partially obscured plugins and plugin posters.

BUG=544292
==========

[tommycli, 2015-10-20 22:10:10]

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

[commit-bot: I haz the power, 2015-10-20 22:10:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/140001

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

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

[commit-bot: I haz the power, 2015-10-20 23:23:06]

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

[tommycli, 2015-10-20 23:40:43]

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

[commit-bot: I haz the power, 2015-10-20 23:41:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/40002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/40002

[tommycli, 2015-10-20 23:55:56]

thestig: PTAL

This is ready for review. Seems to work pretty reliably on the bots (except cros
has a few issues that I think i fixed in the latest patchset).

Tommy

[tommycli, 2015-10-21 00:02:17]

On 2015/10/20 23:55:56, tommycli wrote:
> thestig: PTAL
> 
> This is ready for review. Seems to work pretty reliably on the bots (except
cros
> has a few issues that I think i fixed in the latest patchset).
> 
> Tommy

BTW, most of the code is based on https://codereview.chromium.org/1128453003.

However, I had to change numerous things. For instance: the trick of "Find Text"
to force renderer message loop flush doesn't seem to work for my test PPS
plugin. So I had to use RunLoopUntil like the
FlashFullscreenInteractiveBrowsertest uses.

[commit-bot: I haz the power, 2015-10-21 00:48:28]

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

[commit-bot: I haz the power, 2015-10-21 00:48:28]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-10-21 07:24:53]

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:134: scoped_ptr<char[]>
data(new char[size]);
Just use a std::vector, sans scoped_ptr. vector_as_array() may be helpful.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:135: if
(base::ReadFile(reference, data.get(), size) != size)
Or just use ReadFileToString().

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:138: int w, h;
1 decl per line

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:145: if (bitmap.width()
< kComparisonWidth || w < kComparisonWidth ||
You could have done this first without even opening the file.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:150: int32* ref_pixels
= reinterpret_cast<int32*>(&decoded[0]);
Use stdint.h types - int32_t

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:200:
base::CommandLine::ForCurrentProcess()->HasSwitch(
This is just for convenience, right?

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:207:
base::WriteFile(reference, reinterpret_cast<char*>(&png_data[0]),
Use vector_as_array.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:227: // Reuse an image
from the printing tests as a poster image.
You are adding a plugin_power_saver dir. Use your own image.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
What if your machine has a bad day? Are you going to write out the wrong
reference image?

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:309: return
ui_test_utils::RunLoopUntil(
So if the test is going to fail, we'll retry a bunch of times anyway?

If the renderer / plugin is running really slow, we'll flake due to the timeout?

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:319:
base::FilePath().AppendASCII(expected_filename));
Pass |expected_filename| as a FilePath all the way down?

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:401: // This test
simulateously verifies the varied supported poster syntaxes,
typo

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:404: extern const char kRebaselinePixelTests[];
Maybe this should go in chrome/test/base/test_switches.h ?

https://codereview.chromium.org/1412963003/diff/40002/chrome/test/base/ui_tes...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/1412963003/diff/40002/chrome/test/base/ui_tes...
chrome/test/base/ui_test_utils.cc:412: if (current_time - start_time >
base::TimeDelta::FromSeconds(5)) {
Didn't some other test have a 10 second timeout? Does it matter?

https://codereview.chromium.org/1412963003/diff/40002/ppapi/tests/power_saver...
File ppapi/tests/power_saver_test_plugin.cc (left):

https://codereview.chromium.org/1412963003/diff/40002/ppapi/tests/power_saver...
ppapi/tests/power_saver_test_plugin.cc:63: uint32_t color = x % 2 ? 0xFF0000FF :
0xFFFFFFFF;
that's a funny shade of black

[tommycli, 2015-10-21 18:13:32]

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

[tommycli, 2015-10-21 18:13:49]

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

[commit-bot: I haz the power, 2015-10-21 18:14:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/190001

[tommycli, 2015-10-21 18:16:04]

thestig: thanks.

Btw: If LargeCrossOriginObscured pixel test seems flaky, should I remove it?

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:134: scoped_ptr<char[]>
data(new char[size]);
On 2015/10/21 07:24:52, Lei Zhang wrote:
> Just use a std::vector, sans scoped_ptr. vector_as_array() may be helpful.

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:135: if
(base::ReadFile(reference, data.get(), size) != size)
On 2015/10/21 07:24:52, Lei Zhang wrote:
> Or just use ReadFileToString().

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:138: int w, h;
On 2015/10/21 07:24:52, Lei Zhang wrote:
> 1 decl per line

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:145: if (bitmap.width()
< kComparisonWidth || w < kComparisonWidth ||
On 2015/10/21 07:24:52, Lei Zhang wrote:
> You could have done this first without even opening the file.

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:150: int32* ref_pixels
= reinterpret_cast<int32*>(&decoded[0]);
On 2015/10/21 07:24:52, Lei Zhang wrote:
> Use stdint.h types - int32_t

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:200:
base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/10/21 07:24:52, Lei Zhang wrote:
> This is just for convenience, right?

Yeah exactly. I'm not married to this. Since kCCRebaselinePixeltests[] already
exists, I figured having a kRebaselinePixelTests also made sense.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:207:
base::WriteFile(reference, reinterpret_cast<char*>(&png_data[0]),
On 2015/10/21 07:24:52, Lei Zhang wrote:
> Use vector_as_array.

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:227: // Reuse an image
from the printing tests as a poster image.
On 2015/10/21 07:24:52, Lei Zhang wrote:
> You are adding a plugin_power_saver dir. Use your own image.

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
On 2015/10/21 07:24:52, Lei Zhang wrote:
> What if your machine has a bad day? Are you going to write out the wrong
> reference image?

Yes... and there's no real way to tell that it's the wrong reference image
without human inspection. Open to suggestions...

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:309: return
ui_test_utils::RunLoopUntil(
On 2015/10/21 07:24:52, Lei Zhang wrote:
> So if the test is going to fail, we'll retry a bunch of times anyway?
> 
> If the renderer / plugin is running really slow, we'll flake due to the
timeout?

Yes, we keep retrying, but there's a timeout of 5 seconds (within RunLoopUntil).
If it doesn't pass within 5 seconds, it's marked as an explicit failure.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:319:
base::FilePath().AppendASCII(expected_filename));
On 2015/10/21 07:24:52, Lei Zhang wrote:
> Pass |expected_filename| as a FilePath all the way down?

Done. It is converted to a FilePath in VerifySnapshot now.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:401: // This test
simulateously verifies the varied supported poster syntaxes,
On 2015/10/21 07:24:52, Lei Zhang wrote:
> typo

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:404: extern const char kRebaselinePixelTests[];
On 2015/10/21 07:24:52, Lei Zhang wrote:
> Maybe this should go in chrome/test/base/test_switches.h ?

Done.

https://codereview.chromium.org/1412963003/diff/40002/chrome/test/base/ui_tes...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/1412963003/diff/40002/chrome/test/base/ui_tes...
chrome/test/base/ui_test_utils.cc:412: if (current_time - start_time >
base::TimeDelta::FromSeconds(5)) {
On 2015/10/21 07:24:52, Lei Zhang wrote:
> Didn't some other test have a 10 second timeout? Does it matter?

Yeah, one of them did. The ConstrainedDialog* test executed JS and then used
this function to wait for the result.

I feel that 5 seconds should be sufficient. If we see an increase in flakes of
that test, we can move all of these back to 10 seconds.

Or parameter-ize it.

https://codereview.chromium.org/1412963003/diff/40002/ppapi/tests/power_saver...
File ppapi/tests/power_saver_test_plugin.cc (left):

https://codereview.chromium.org/1412963003/diff/40002/ppapi/tests/power_saver...
ppapi/tests/power_saver_test_plugin.cc:63: uint32_t color = x % 2 ? 0xFF0000FF :
0xFFFFFFFF;
On 2015/10/21 07:24:53, Lei Zhang wrote:
> that's a funny shade of black

Yeah... haha the comment was wrong.

[tommycli, 2015-10-21 18:16:36]

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

[tommycli, 2015-10-21 18:16:36]

raymes: PTAL ppapi/tests/power_saver_test_plugin.cc

Thanks

[commit-bot: I haz the power, 2015-10-21 19:21:38]

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

[commit-bot: I haz the power, 2015-10-21 19:21:39]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-10-21 23:25:33]

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
On 2015/10/21 18:16:04, tommycli wrote:
> On 2015/10/21 07:24:52, Lei Zhang wrote:
> > What if your machine has a bad day? Are you going to write out the wrong
> > reference image?
> 
> Yes... and there's no real way to tell that it's the wrong reference image
> without human inspection. Open to suggestions...

Are there any JS / DOM events that corrolates with when the plugin finishes
loading? Are there ways to add some?

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:319:
base::FilePath().AppendASCII(expected_filename));
On 2015/10/21 18:16:04, tommycli wrote:
> On 2015/10/21 07:24:52, Lei Zhang wrote:
> > Pass |expected_filename| as a FilePath all the way down?
> 
> Done. It is converted to a FilePath in VerifySnapshot now.

Why not keep going and change VerifySnapshot() too?

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:404: extern const char kRebaselinePixelTests[];
On 2015/10/21 18:16:04, tommycli wrote:
> On 2015/10/21 07:24:52, Lei Zhang wrote:
> > Maybe this should go in chrome/test/base/test_switches.h ?
> 
> Done.

chrome/test/base/test_switches.h != base/test/test_switches ?

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

https://codereview.chromium.org/1412963003/diff/190001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:154: int32_t*
ref_pixels = reinterpret_cast<int32_t*>(&decoded[0]);
vector_as_array

[tommycli, 2015-10-21 23:37:56]

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

[tommycli, 2015-10-21 23:40:07]

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

[commit-bot: I haz the power, 2015-10-21 23:41:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/210001

[tommycli, 2015-10-21 23:57:22]

thestig: thx

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
On 2015/10/21 23:25:32, Lei Zhang wrote:
> On 2015/10/21 18:16:04, tommycli wrote:
> > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > What if your machine has a bad day? Are you going to write out the wrong
> > > reference image?
> > 
> > Yes... and there's no real way to tell that it's the wrong reference image
> > without human inspection. Open to suggestions...
> 
> Are there any JS / DOM events that corrolates with when the plugin finishes
> loading? Are there ways to add some?

Oh - I tried so hard believe me, the existing VerifyPluginIsThrottled and
SimulateClickAndAwaitMarkedEssential already do a huge song and dance between
Pepper and JS. Fires a custom DOM event actually.

The problem is that even when that's done, it may not have arrived in the
backing store.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:319:
base::FilePath().AppendASCII(expected_filename));
On 2015/10/21 23:25:32, Lei Zhang wrote:
> On 2015/10/21 18:16:04, tommycli wrote:
> > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > Pass |expected_filename| as a FilePath all the way down?
> > 
> > Done. It is converted to a FilePath in VerifySnapshot now.
> 
> Why not keep going and change VerifySnapshot() too?

Ah... because it would cause me to write ui_test_utils::GetTestFilePath(
         base::FilePath(FILE_PATH_LITERAL("plugin_power_saver")),
         base::FilePath().AppendASCII(...) in 4 different places.

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/1412963003/diff/40002/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:404: extern const char kRebaselinePixelTests[];
On 2015/10/21 23:25:33, Lei Zhang wrote:
> On 2015/10/21 18:16:04, tommycli wrote:
> > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > Maybe this should go in chrome/test/base/test_switches.h ?
> > 
> > Done.
> 
> chrome/test/base/test_switches.h != base/test/test_switches ?

Done. Doh!

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

https://codereview.chromium.org/1412963003/diff/190001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:154: int32_t*
ref_pixels = reinterpret_cast<int32_t*>(&decoded[0]);
On 2015/10/21 23:25:33, Lei Zhang wrote:
> vector_as_array

Done.

[Lei Zhang, 2015-10-22 00:16:13]

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
On 2015/10/21 23:57:22, tommycli wrote:
> On 2015/10/21 23:25:32, Lei Zhang wrote:
> > On 2015/10/21 18:16:04, tommycli wrote:
> > > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > > What if your machine has a bad day? Are you going to write out the wrong
> > > > reference image?
> > > 
> > > Yes... and there's no real way to tell that it's the wrong reference image
> > > without human inspection. Open to suggestions...
> > 
> > Are there any JS / DOM events that corrolates with when the plugin finishes
> > loading? Are there ways to add some?
> 
> Oh - I tried so hard believe me, the existing VerifyPluginIsThrottled and
> SimulateClickAndAwaitMarkedEssential already do a huge song and dance between
> Pepper and JS. Fires a custom DOM event actually.
> 
> The problem is that even when that's done, it may not have arrived in the
> backing store.

Can you ask the compositor folks if there's a better way, assuming they are the
right folks to talk to?

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:319:
base::FilePath().AppendASCII(expected_filename));
On 2015/10/21 23:57:22, tommycli wrote:
> On 2015/10/21 23:25:32, Lei Zhang wrote:
> > On 2015/10/21 18:16:04, tommycli wrote:
> > > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > > Pass |expected_filename| as a FilePath all the way down?
> > > 
> > > Done. It is converted to a FilePath in VerifySnapshot now.
> > 
> > Why not keep going and change VerifySnapshot() too?
> 
> Ah... because it would cause me to write ui_test_utils::GetTestFilePath(
>          base::FilePath(FILE_PATH_LITERAL("plugin_power_saver")),
>          base::FilePath().AppendASCII(...) in 4 different places.

Can we do this?

bool VerifySnapshot(const base::FilePath::StringType& expected_filename);

...

VerifySnapshot(FILE_PATH_LITERAL("small_cross_origin_expected.png"));

...

bool TryVerifySnapshot(const base::FilePath::StringType& expected_filename) {
  base::FilePath reference = ui_test_utils::GetTestFilePath(
  base::FilePath(FILE_PATH_LITERAL("plugin_power_saver")),
  base::FilePath(expected_filename));
  ...
}

or do you have to do the wonky base::FilePath().AppendASCII(expected_filename));
?

[tommycli, 2015-10-22 00:45:20]

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

[commit-bot: I haz the power, 2015-10-22 00:46:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/230001

[commit-bot: I haz the power, 2015-10-22 01:50:13]

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

[commit-bot: I haz the power, 2015-10-22 01:50:13]

Dry run: This issue passed the CQ dry run.

[tommycli, 2015-10-22 19:17:18]

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

[tommycli, 2015-10-22 19:17:24]

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
On 2015/10/22 00:16:13, Lei Zhang wrote:
> On 2015/10/21 23:57:22, tommycli wrote:
> > On 2015/10/21 23:25:32, Lei Zhang wrote:
> > > On 2015/10/21 18:16:04, tommycli wrote:
> > > > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > > > What if your machine has a bad day? Are you going to write out the
wrong
> > > > > reference image?
> > > > 
> > > > Yes... and there's no real way to tell that it's the wrong reference
image
> > > > without human inspection. Open to suggestions...
> > > 
> > > Are there any JS / DOM events that corrolates with when the plugin
finishes
> > > loading? Are there ways to add some?
> > 
> > Oh - I tried so hard believe me, the existing VerifyPluginIsThrottled and
> > SimulateClickAndAwaitMarkedEssential already do a huge song and dance
between
> > Pepper and JS. Fires a custom DOM event actually.
> > 
> > The problem is that even when that's done, it may not have arrived in the
> > backing store.
> 
> Can you ask the compositor folks if there's a better way, assuming they are
the
> right folks to talk to?

Yes - I'll reach out to piman / some other compositor guy to get some more
advice on how to avoid the hacky RunLoopUntil.

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:319:
base::FilePath().AppendASCII(expected_filename));
On 2015/10/22 00:16:13, Lei Zhang wrote:
> On 2015/10/21 23:57:22, tommycli wrote:
> > On 2015/10/21 23:25:32, Lei Zhang wrote:
> > > On 2015/10/21 18:16:04, tommycli wrote:
> > > > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > > > Pass |expected_filename| as a FilePath all the way down?
> > > > 
> > > > Done. It is converted to a FilePath in VerifySnapshot now.
> > > 
> > > Why not keep going and change VerifySnapshot() too?
> > 
> > Ah... because it would cause me to write ui_test_utils::GetTestFilePath(
> >          base::FilePath(FILE_PATH_LITERAL("plugin_power_saver")),
> >          base::FilePath().AppendASCII(...) in 4 different places.
> 
> Can we do this?
> 
> bool VerifySnapshot(const base::FilePath::StringType& expected_filename);
> 
> ...
> 
> VerifySnapshot(FILE_PATH_LITERAL("small_cross_origin_expected.png"));
> 
> ...
> 
> bool TryVerifySnapshot(const base::FilePath::StringType& expected_filename) {
>   base::FilePath reference = ui_test_utils::GetTestFilePath(
>   base::FilePath(FILE_PATH_LITERAL("plugin_power_saver")),
>   base::FilePath(expected_filename));
>   ...
> }
> 
> or do you have to do the wonky
base::FilePath().AppendASCII(expected_filename));
> ?

Done. Yeah that's fine with me.

[commit-bot: I haz the power, 2015-10-22 19:18:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/250001

[tommycli, 2015-10-22 20:05:50]

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

[commit-bot: I haz the power, 2015-10-22 20:06:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/270001

[commit-bot: I haz the power, 2015-10-22 20:40:46]

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

[commit-bot: I haz the power, 2015-10-22 20:40:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2015-10-22 20:48:32]

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

[commit-bot: I haz the power, 2015-10-22 20:49:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/290001

[commit-bot: I haz the power, 2015-10-22 21:21:53]

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

[commit-bot: I haz the power, 2015-10-22 21:21:54]

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

[tommycli, 2015-10-22 22:32:42]

piman: I have a compositor question listed below - about testing pixels copied
out of RenderWidgetHost::CopyFromBackingStore.

Perhaps you can shed some light.

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
On 2015/10/22 19:17:24, tommycli wrote:
> On 2015/10/22 00:16:13, Lei Zhang wrote:
> > On 2015/10/21 23:57:22, tommycli wrote:
> > > On 2015/10/21 23:25:32, Lei Zhang wrote:
> > > > On 2015/10/21 18:16:04, tommycli wrote:
> > > > > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > > > > What if your machine has a bad day? Are you going to write out the
> wrong
> > > > > > reference image?
> > > > > 
> > > > > Yes... and there's no real way to tell that it's the wrong reference
> image
> > > > > without human inspection. Open to suggestions...
> > > > 
> > > > Are there any JS / DOM events that corrolates with when the plugin
> finishes
> > > > loading? Are there ways to add some?
> > > 
> > > Oh - I tried so hard believe me, the existing VerifyPluginIsThrottled and
> > > SimulateClickAndAwaitMarkedEssential already do a huge song and dance
> between
> > > Pepper and JS. Fires a custom DOM event actually.
> > > 
> > > The problem is that even when that's done, it may not have arrived in the
> > > backing store.
> > 
> > Can you ask the compositor folks if there's a better way, assuming they are
> the
> > right folks to talk to?
> 
> Yes - I'll reach out to piman / some other compositor guy to get some more
> advice on how to avoid the hacky RunLoopUntil.

I have trying to implement pixel tests for Plugin Power Saver, however, I am
having difficulty with timing issues.

The pixels I copy from the backing store are often older than I expect, even
after trying to synchronize the state between Pepper and JavaScript.

I have been forced to use a hacky RunLoopUntil method to wait for the backing
store pixels to match the reference image.

Line 75 of
https://codereview.chromium.org/1128453003/diff/20001/chrome/browser/ui/pdf/p...
seems to make a comment to the same effect.

The FlashFullscreenInteractiveBrowsertest also uses the same method.

thestig@ suggested I contact some compositor folks, as they may have a better
idea - or can shed more light on the topic.

[tommycli, 2015-10-22 22:32:54]

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

[commit-bot: I haz the power, 2015-10-22 22:34:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/310001

[commit-bot: I haz the power, 2015-10-22 23:40:00]

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

[commit-bot: I haz the power, 2015-10-22 23:40:01]

Dry run: This issue passed the CQ dry run.

[piman, 2015-10-23 01:45:30]

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

[piman, 2015-10-23 01:45:31]

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

https://codereview.chromium.org/1412963003/diff/40002/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:296: // If we are
rebaselining, give the placeholders an extra 2 seconds to
On 2015/10/22 22:32:42, tommycli wrote:
> On 2015/10/22 19:17:24, tommycli wrote:
> > On 2015/10/22 00:16:13, Lei Zhang wrote:
> > > On 2015/10/21 23:57:22, tommycli wrote:
> > > > On 2015/10/21 23:25:32, Lei Zhang wrote:
> > > > > On 2015/10/21 18:16:04, tommycli wrote:
> > > > > > On 2015/10/21 07:24:52, Lei Zhang wrote:
> > > > > > > What if your machine has a bad day? Are you going to write out the
> > wrong
> > > > > > > reference image?
> > > > > > 
> > > > > > Yes... and there's no real way to tell that it's the wrong reference
> > image
> > > > > > without human inspection. Open to suggestions...
> > > > > 
> > > > > Are there any JS / DOM events that corrolates with when the plugin
> > finishes
> > > > > loading? Are there ways to add some?
> > > > 
> > > > Oh - I tried so hard believe me, the existing VerifyPluginIsThrottled
and
> > > > SimulateClickAndAwaitMarkedEssential already do a huge song and dance
> > between
> > > > Pepper and JS. Fires a custom DOM event actually.
> > > > 
> > > > The problem is that even when that's done, it may not have arrived in
the
> > > > backing store.
> > > 
> > > Can you ask the compositor folks if there's a better way, assuming they
are
> > the
> > > right folks to talk to?
> > 
> > Yes - I'll reach out to piman / some other compositor guy to get some more
> > advice on how to avoid the hacky RunLoopUntil.
> 
> I have trying to implement pixel tests for Plugin Power Saver, however, I am
> having difficulty with timing issues.
> 
> The pixels I copy from the backing store are often older than I expect, even
> after trying to synchronize the state between Pepper and JavaScript.
> 
> I have been forced to use a hacky RunLoopUntil method to wait for the backing
> store pixels to match the reference image.
> 
> Line 75 of
>
https://codereview.chromium.org/1128453003/diff/20001/chrome/browser/ui/pdf/p...
> seems to make a comment to the same effect.
> 
> The FlashFullscreenInteractiveBrowsertest also uses the same method.
> 
> thestig@ suggested I contact some compositor folks, as they may have a better
> idea - or can shed more light on the topic.

Well, it's not exactly simple to do it accurately... After the renderer received
a frame from the plugin, you need to wait:
1- for the compositor to commit the layer tree (on the renderer main thread,
asynchronously)
2- then for the compositor to send a frame to the browser (on the renderer's
compositor thread)
3- then for the browser to receive the message (ViewHostMsg_SwapCompositorFrame)
and dispatches it to the view.

After that, you can queue the CopyFromBackingStore, and it should give you the
right thing when it gives you the callback.


The problem is that it's not possible to directly observe (1) and (2) from a
browser test.

I think the best thing you can do is to use
RenderFrameHost::InsertVisualStateCallback, on the WebContent's main frame which
will call you back as soon as the renderer state known so far went through (1),
(2), and (3). Actually that may be your best solution methinks. You do need to
know (from the browser test side) that the renderer has received a frame from
the plugin though.

[raymes, 2015-10-23 03:19:42]

ppapi/ rubberstamp lgtm

[tommycli, 2015-10-26 22:25:17]

piman: Thanks! Your secret sauce really worked.

thestig: This is ready for another look. Using piman's sauce, we were able to
get the test to work without the RunUntil loop.

[Lei Zhang, 2015-10-26 23:15:47]

lgtm

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (left):

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:203: // Test extremely
flaky, see https://crbug.com/547224
So the flakiness should be fixed by this too?

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

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:240:
gfx::PNGCodec::EncodeBGRASkBitmap(clipped_bitmap, false, &png_data);
Check return result?

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:241:
base::WriteFile(reference,
Also check return result?

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:324:
base::FilePath().Append(expected_filename));
Can't this be base::FilePath(expected_filename) now?

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

https://codereview.chromium.org/1412963003/diff/370001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:61: int square_size = 8;
const int kSquarePixelSize = 8; ?

[tommycli, 2015-10-26 23:29:51]

thestig: thanks.

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (left):

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:203: // Test extremely
flaky, see https://crbug.com/547224
On 2015/10/26 23:15:47, Lei Zhang wrote:
> So the flakiness should be fixed by this too?

Broken out into separate patch here: https://codereview.chromium.org/1416723007/

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

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:240:
gfx::PNGCodec::EncodeBGRASkBitmap(clipped_bitmap, false, &png_data);
On 2015/10/26 23:15:47, Lei Zhang wrote:
> Check return result?

Done.

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:241:
base::WriteFile(reference,
On 2015/10/26 23:15:47, Lei Zhang wrote:
> Also check return result?

Done.

https://codereview.chromium.org/1412963003/diff/370001/chrome/browser/plugins...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:324:
base::FilePath().Append(expected_filename));
On 2015/10/26 23:15:47, Lei Zhang wrote:
> Can't this be base::FilePath(expected_filename) now?

Done.

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

https://codereview.chromium.org/1412963003/diff/370001/ppapi/tests/power_save...
ppapi/tests/power_saver_test_plugin.cc:61: int square_size = 8;
On 2015/10/26 23:15:47, Lei Zhang wrote:
> const int kSquarePixelSize = 8; ?

Done. Called it kSquareSizePixels.

[tommycli, 2015-10-27 01:24:22]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-10-27 01:24:22]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1412963003/#ps410001
(title: " ")

[commit-bot: I haz the power, 2015-10-27 01:24:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412963003/410001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412963003/410001

[commit-bot: I haz the power, 2015-10-27 02:41:35]

Committed patchset #23 (id:410001)

[commit-bot: I haz the power, 2015-10-27 02:43:10]

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