Add WaitForInterstitialReady to browser_test_utils.

Add WaitForInterstitialReady to browser_test_utils.

CaptivePortal and SafeBrowsingBlockingPage tests wait for the interstitial to get ready in different ways. This CL unifies them.

BUG=448486
Committed: https://crrev.com/8543a23f0bc2072149902d80bf36ae143a8dee48
Cr-Commit-Position: refs/heads/master@{#312908}

[meacer, 2015-01-22 02:04:28]

meacer@chromium.org changed reviewers:
+ mattm@chromium.org, mmenke@chromium.org, phajdan.jr@chromium.org

[meacer, 2015-01-22 02:04:28]

Can you PTAL? Thanks.

[mattm, 2015-01-22 02:28:32]

https://codereview.chromium.org/829263004/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/829263004/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:516:
browser()->tab_strip_model()->GetActiveWebContents());
I think there should be a "if (!interstitial) return false;" here.

https://codereview.chromium.org/829263004/diff/1/content/public/test/browser_...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/829263004/diff/1/content/public/test/browser_...
content/public/test/browser_test_utils.cc:728: content::RenderFrameHost* rfh =
interstitial->GetMainFrame();
Since the rest of this function isn't interstitial specific, would it be useful
to just take a RenderFrameHost and call it WaitForRenderFrameReady or something
like that?

[meacer, 2015-01-22 03:17:50]

https://codereview.chromium.org/829263004/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/829263004/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:516:
browser()->tab_strip_model()->GetActiveWebContents());
On 2015/01/22 02:28:32, mattm wrote:
> I think there should be a "if (!interstitial) return false;" here.

Done.

[meacer, 2015-01-22 03:18:07]

https://codereview.chromium.org/829263004/diff/1/content/public/test/browser_...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/829263004/diff/1/content/public/test/browser_...
content/public/test/browser_test_utils.cc:728: content::RenderFrameHost* rfh =
interstitial->GetMainFrame();
On 2015/01/22 02:28:32, mattm wrote:
> Since the rest of this function isn't interstitial specific, would it be
useful
> to just take a RenderFrameHost and call it WaitForRenderFrameReady or
something
> like that?

Done.

[mattm, 2015-01-22 04:51:27]

lgtm

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.cc:24: #include
"content/public/browser/interstitial_page.h"
no longer needed

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:55: class InterstitialPage;
no longer needed

[Paweł Hajdan Jr., 2015-01-22 15:43:10]

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.cc:743: " 
document.addEventListener('readystatechange', checkState);"
I might be wrong (not a JS/DOM expert), but shouldn't we add the event listener
first? I wonder if there might be a race if the condition becomes true right
between checkState() and addEventListener() calls.

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:248: bool
WaitForRenderFrameReady(RenderFrameHost* rfh);
nit: Please add WARN_UNUSED_RESULT.

[mmenke, 2015-01-22 15:53:25]

https://codereview.chromium.org/829263004/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/829263004/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:224: if (command ==
"pageLoadComplete") {
Should have something in the WaitForRenderFrameReady docs about this.  Would be
better if we could prevent anything else from seeing it, but guess there may not
be a clean way to do that.

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.cc:743: " 
document.addEventListener('readystatechange', checkState);"
On 2015/01/22 15:43:10, Paweł Hajdan Jr. wrote:
> I might be wrong (not a JS/DOM expert), but shouldn't we add the event
listener
> first? I wonder if there might be a race if the condition becomes true right
> between checkState() and addEventListener() calls.

Javascript and the DOM are essentially single-threaded.  The DOM can't change
and an event can't happen in the middle of the execution of Javascript running.

That having been said, I'm certainly fine with changing order, it just doesn't
matter, either way, in this case.

[mmenke, 2015-01-22 15:53:59]

And LGTM

[mmenke, 2015-01-22 15:55:00]

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.cc:743: " 
document.addEventListener('readystatechange', checkState);"
On 2015/01/22 15:53:25, mmenke wrote:
> On 2015/01/22 15:43:10, Paweł Hajdan Jr. wrote:
> > I might be wrong (not a JS/DOM expert), but shouldn't we add the event
> listener
> > first? I wonder if there might be a race if the condition becomes true right
> > between checkState() and addEventListener() calls.
> 
> Javascript and the DOM are essentially single-threaded.  The DOM can't change
> and an event can't happen in the middle of the execution of Javascript
running.
> 
> That having been said, I'm certainly fine with changing order, it just doesn't
> matter, either way, in this case.

That is, they can't change except as a result of a Javascript function call, of
course.

[meacer, 2015-01-22 18:31:37]

https://codereview.chromium.org/829263004/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/829263004/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:224: if (command ==
"pageLoadComplete") {
On 2015/01/22 15:53:25, mmenke wrote:
> Should have something in the WaitForRenderFrameReady docs about this.  Would
be
> better if we could prevent anything else from seeing it, but guess there may
not
> be a clean way to do that.

Added a note to WaitForRenderFrameReady doc.

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.cc:24: #include
"content/public/browser/interstitial_page.h"
On 2015/01/22 04:51:27, mattm wrote:
> no longer needed

Done.

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:55: class InterstitialPage;
On 2015/01/22 04:51:27, mattm wrote:
> no longer needed

Done.

https://codereview.chromium.org/829263004/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:248: bool
WaitForRenderFrameReady(RenderFrameHost* rfh);
On 2015/01/22 15:43:10, Paweł Hajdan Jr. wrote:
> nit: Please add WARN_UNUSED_RESULT.

Done.

[Paweł Hajdan Jr., 2015-01-23 10:49:14]

LGTM

[meacer, 2015-01-23 18:06:48]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2015-01-23 18:07:37]

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

[commit-bot: I haz the power, 2015-01-23 19:59:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-23 20:01:31]

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