Fix flaky test: RenderWidgetHostViewBrowserTest.CopyFromBackingStore.

Fix flaky test: Enable RenderWidgetHostViewBrowserTest.CopyFromBackingStore.

Added a spin-wait to cause the test to wait for the browser to create the BackingStore via its normal mechanism.

...and then I went ahead and refactored and cleaned up all the code.  I did not add any significant additional testing.  We will want to do this in a follow-up change.  However, I did get rid of the #ifdef OS_XXX pragmas, so the tests will attempt to run on all platforms, not just Mac and Win, and use the CanXXX() methods to determine which features are supported for testing.

BUG=224351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200226

[miu, 2013-04-29 23:01:26]

nick: Please review.

kbr: OWNERs approval, please.

Thanks,
Yuri

[Ken Russell (switch to Gerrit), 2013-04-29 23:08:54]

I can't vouch for the correctness of this test, but LGTM if it improves the
situation. jamesr and ccameron may have suggestions on improving the test.

https://codereview.chromium.org/14495016/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/14495016/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:281: <<
"Accelerated compositing is still running for a static HTML page.";
It looks to me like this test's assumptions are broken on platforms that use
threaded compositing and/or force-compositing-mode.

[ncarter (slow), 2013-04-29 23:34:21]

lgtm in the sense of "enabled tests are better than disabled ones", but I agree
with kbr's observation that the way that static content is used is shaky at
best.

https://codereview.chromium.org/14495016/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/14495016/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:108: // to
turn on compositing.
The text here describes the opposite of what the name suggests.

https://codereview.chromium.org/14495016/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:281: <<
"Accelerated compositing is still running for a static HTML page.";
On 2013/04/29 23:08:54, kbr wrote:
> It looks to me like this test's assumptions are broken on platforms that use
> threaded compositing and/or force-compositing-mode.

Agree, and I'm not such a big fan of a LOG_WARNING from  tests which nobody
might notice -- my attitude might be summarized as "fail or do not fail, there
is no warn".

Kbr, can you think of some way (from the RVH) that software mode can be forced
on or off for a page?

If not, we might refactor the intent of these tests so that they're more like
"CopyFromBackingStore_StaticContent" and "CopyFromBackingStore_3DTransform", so
that while the tests are designed to provide a certain kind of coverage, the
test logic doesn't depend on the actual path taken. Such a test might be easier
to maintain as the underlying heuristics change.

[jamesr, 2013-04-29 23:41:58]

If the intent of this test is to verify the behavior in different modes
(compositing vs non-compositing), you should configure that explicitly rather
than relying on certain types of pages going down certain paths since we change
that all the time.  I think the hooks you want are (as command line flags)
--force-compositing-mode and --disable-accelerated-compositing.  I'm not sure
what the right way to pipe that in from a content browser test is.

[miu, 2013-04-30 00:38:48]

Discussed this with Nick a bit.  After looking at these tests, I think it's
clear that they have become rather messy due to the several massive API changes
over the past few months.  I'm going to clean things up, using a combination of
everyone's suggestions.  I'd also like to make sure we have more coverage than
just "#if defined(OS_MACOSX) || defined(OS_WINDOWS)" since the functionality
exists for other platforms.

I have a bad feeling I'm going to be getting really intimate with the try bots
soon...

I will post a "PTAL" message when things are ready for another look.

Thanks,
Yuri

[miu, 2013-04-30 23:05:02]

nick/kbr: PTAL.  I went the route of forcing compositing on vs. disabling it. 
In each case, I replaced the "#ifdef PLATFORM" pragmas, and replaced them with
usage of the built-in CanCopyXXX() and CanSubscribe() methods.

Thanks,
Yuri

[Ken Russell (switch to Gerrit), 2013-04-30 23:23:02]

LGTM, with a couple of questions.

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:158: class
RenderWidgetHostViewBrowserTest_Compositing
The "_" here and in the subclass below doesn't really follow the naming
convention.

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:204: //
compositing.
Wasn't the point of the subclass to avoid these assumptions? Can you use real
content now instead of about:blank?

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:213: return
GetRenderWidgetHost()->GetBackingStore(false) != NULL;
Do you want to make assertions that the composited code path isn't active at
this point?

[ncarter (slow), 2013-05-01 00:04:31]

lgtm, & agree with kbr's observations

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:325:
return;
I expect this will happen on XP.

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:349: "on
this platform.");
I expect this will happen on XP.

[miu, 2013-05-01 00:19:36]

Thanks for the review, guys.  I'll commit this later tonight, since there may be
weirdness around certain build bot configurations (I don't want to close the
tree during business hours).

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:158: class
RenderWidgetHostViewBrowserTest_Compositing
On 2013/04/30 23:23:02, kbr wrote:
> The "_" here and in the subclass below doesn't really follow the naming
> convention.

Done.  Was concerned about what people might try in --gtest_filter=... command
line args.  Move "Compositing" to front of class name.  Similar change for the
"NonCompositing" one, too.

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:204: //
compositing.
On 2013/04/30 23:23:02, kbr wrote:
> Wasn't the point of the subclass to avoid these assumptions? Can you use real
> content now instead of about:blank?

Ah!  A should have removed the comment.  The command line flag will ensure
compositing is turned off.  Done.

BTW--This code path used to NavigateToURL(rwhv_compositing_static.html).  I
changed it to NavigateToURL(about:blank) as an optimization (since there's no
testing that depends on the content of the page).

https://codereview.chromium.org/14495016/diff/20001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:213: return
GetRenderWidgetHost()->GetBackingStore(false) != NULL;
On 2013/04/30 23:23:02, kbr wrote:
> Do you want to make assertions that the composited code path isn't active at
> this point?

Good point.  Done.

[miu, 2013-05-14 19:21:37]

nick/kbr: PTAL (delta from PS 4).  Based on try bot results, I needed to add
some logic to detect whether each platform supports compositing mode, rather
than just forcing it on for all platforms.  In addition, Aura can only do
compositing, so I needed to add logic to detect the case where explicitly
disabling the compositor has no effect.

Thanks,
Yuri

[Ken Russell (switch to Gerrit), 2013-05-14 20:04:08]

LGTM if the test passes.

https://codereview.chromium.org/14495016/diff/68001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/14495016/diff/68001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:198: return
false;  // Not supported due to hardware or blacklisting.
It's confusing that SetUpCommandLine says that it doesn't append
kForceCompositingMode to the command line, but this method requires it.

[ncarter (slow), 2013-05-14 20:44:11]

lgtm with obligatory "eww macros" admonishment

[commit-bot: I haz the power, 2013-05-15 00:21:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/14495016/80001

[miu, 2013-05-15 00:21:39]

Thanks guys.  Trybots were green for content_browsertests across the board.

https://codereview.chromium.org/14495016/diff/68001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/14495016/diff/68001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:198: return
false;  // Not supported due to hardware or blacklisting.
On 2013/05/14 20:04:08, kbr wrote:
> It's confusing that SetUpCommandLine says that it doesn't append
> kForceCompositingMode to the command line, but this method requires it.

Improved code comments to explain why this is necessary and how the "lack of
support for compositing" issue is being dealt with.

[commit-bot: I haz the power, 2013-05-15 06:23:55]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-15 07:13:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/14495016/80001

[commit-bot: I haz the power, 2013-05-15 10:00:15]

Change committed as 200226