[NoStatePrefetch] Remove JS injection in tests

[NoStatePrefetch] Remove JS injection in tests

With upcoming changes the renderer that is used for prefetch will exit early
before tests have a chance to execute javascript there. Fortunately, there is
another way to check whether javascript is executed: requesting a resource from
an inline script in a blocking manner. This is simpler than changing the
lifetime of the renderer specifically for tests.

NoStatePrefetchBrowserTest.CheckJavascript would still be able to run with the
new renderer lifetime, but I decided to still remove it because it is testing
mechanisms that are not used any more.

BUG=643652
Committed: https://crrev.com/0b1e4e811cca307ef42d014937ee604161ce637e
Cr-Commit-Position: refs/heads/master@{#426460}

[pasko, 2016-10-19 17:43:46]

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

[commit-bot: I haz the power, 2016-10-19 17:44:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2016-10-19 17:46:10]

Description was changed from

==========
[NoStatePrefetch] Remove JS injection in tests

With upcoming changes the renderer that is used for prefetch will exit early
before tests have a chance to execute javascript there. Fortunately, there is
another way to check whether javascript is executed: requesting a resource from
an inline script in a blocking manner. This is simpler than changing the
lifetime of the renderer specifically for tests.

NoStatePrefetchBrowserTest.CheckJavascript would still be able to run with the
new renderer lifetime, but I decided to still remove it because it is testing
mechanisms that are not used any more.

BUG=643652
==========

to

==========
[NoStatePrefetch] Remove JS injection in tests

With upcoming changes the renderer that is used for prefetch will exit early
before tests have a chance to execute javascript there. Fortunately, there is
another way to check whether javascript is executed: requesting a resource from
an inline script in a blocking manner. This is simpler than changing the
lifetime of the renderer specifically for tests.

NoStatePrefetchBrowserTest.CheckJavascript would still be able to run with the
new renderer lifetime, but I decided to still remove it because it is testing
mechanisms that are not used any more.

BUG=643652
==========

[pasko, 2016-10-19 17:46:10]

pasko@chromium.org changed reviewers:
+ mattcary@chromium.org

[commit-bot: I haz the power, 2016-10-19 21:14:32]

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

[commit-bot: I haz the power, 2016-10-19 21:14:32]

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

[mattcary, 2016-10-20 07:40:53]

lgtm

with nits

https://chromiumcodereview.appspot.com/2438593002/diff/1/chrome/browser/prere...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://chromiumcodereview.appspot.com/2438593002/diff/1/chrome/browser/prere...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:175:
EXPECT_EQ(0, script_counter.count());
Better to do script_counter.WaitForCount(0). Even though right now it's
equivalent to what you're doing, in the future it might not be if we figure out
a way to reliably detect that scripts are not loaded.

https://chromiumcodereview.appspot.com/2438593002/diff/1/chrome/browser/prere...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:261: // The
first prerender is marked as canceled when the second starts, it sees
Agrammatical, you need something connecting the clauses or use a full stop and
then probably disambiguate the pronoun.

[pasko, 2016-10-20 10:05:07]

PTAL before I start landing this

https://codereview.chromium.org/2438593002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2438593002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:175:
EXPECT_EQ(0, script_counter.count());
On 2016/10/20 07:40:53, mattcary wrote:
> Better to do script_counter.WaitForCount(0). Even though right now it's
> equivalent to what you're doing, in the future it might not be if we figure
out
> a way to reliably detect that scripts are not loaded.

Done.

https://codereview.chromium.org/2438593002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:261: // The
first prerender is marked as canceled when the second starts, it sees
On 2016/10/20 07:40:53, mattcary wrote:
> Agrammatical, you need something connecting the clauses or use a full stop and
> then probably disambiguate the pronoun.

Thanks. I was confused by the initial form and indeed made things worse. PTAL at
the new wording with multiple sentences (I am still not sure I preserved the
original meaning, sry). I also removed 'presumably' because comments humans make
should be assumed to be about presumable matters by default.

[mattcary, 2016-10-20 10:09:40]

lgtm

https://codereview.chromium.org/2438593002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2438593002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:261: // The
first prerender is marked as canceled when the second starts, it sees
On 2016/10/20 10:05:07, pasko wrote:
> On 2016/10/20 07:40:53, mattcary wrote:
> > Agrammatical, you need something connecting the clauses or use a full stop
and
> > then probably disambiguate the pronoun.
> 
> Thanks. I was confused by the initial form and indeed made things worse. PTAL
at
> the new wording with multiple sentences (I am still not sure I preserved the
> original meaning, sry). I also removed 'presumably' because comments humans
make
> should be assumed to be about presumable matters by default.

The current version SGTM.

Possibly my version could have been made more clear by adding a comma before the
second "as". But this is just fine as you've done it.

[pasko, 2016-10-20 10:21:29]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2016-10-20 10:21:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 10:37:19]

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

[commit-bot: I haz the power, 2016-10-20 10:37:20]

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

[mattcary, 2016-10-20 11:49:39]

https://codereview.chromium.org/2438593002/diff/20001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2438593002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:174:
script_count.WaitForCount(0);
I missed this the first time around: don't you also want to count
kPrefetchScript to make sure that the preload scanner actually run? As-is,
you're counting only the main resource which will get loaded even if the preload
scanner isn't running correctly.

[pasko, 2016-10-20 12:08:08]

my previous version did not compile :/ now I was careful and even ran the
affected tests locally, which is what I usually do ..

on the good side: allowed to address a nice comment

https://codereview.chromium.org/2438593002/diff/20001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2438593002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:174:
script_count.WaitForCount(0);
On 2016/10/20 11:49:38, mattcary wrote:
> I missed this the first time around: don't you also want to count
> kPrefetchScript to make sure that the preload scanner actually run? As-is,
> you're counting only the main resource which will get loaded even if the
preload
> scanner isn't running correctly.

True. Done.

[pasko, 2016-10-20 12:08:15]

The CQ bit was checked by pasko@chromium.org

[pasko, 2016-10-20 12:08:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org
Link to the patchset: https://codereview.chromium.org/2438593002/#ps40001
(title: "PrefetchSimple: check the should-be-fetched resource is indeed
fetched")

[commit-bot: I haz the power, 2016-10-20 12:08:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 12:56:20]

Description was changed from

==========
[NoStatePrefetch] Remove JS injection in tests

With upcoming changes the renderer that is used for prefetch will exit early
before tests have a chance to execute javascript there. Fortunately, there is
another way to check whether javascript is executed: requesting a resource from
an inline script in a blocking manner. This is simpler than changing the
lifetime of the renderer specifically for tests.

NoStatePrefetchBrowserTest.CheckJavascript would still be able to run with the
new renderer lifetime, but I decided to still remove it because it is testing
mechanisms that are not used any more.

BUG=643652
==========

to

==========
[NoStatePrefetch] Remove JS injection in tests

With upcoming changes the renderer that is used for prefetch will exit early
before tests have a chance to execute javascript there. Fortunately, there is
another way to check whether javascript is executed: requesting a resource from
an inline script in a blocking manner. This is simpler than changing the
lifetime of the renderer specifically for tests.

NoStatePrefetchBrowserTest.CheckJavascript would still be able to run with the
new renderer lifetime, but I decided to still remove it because it is testing
mechanisms that are not used any more.

BUG=643652
==========

[commit-bot: I haz the power, 2016-10-20 12:56:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:17:45]

Description was changed from

==========
[NoStatePrefetch] Remove JS injection in tests

With upcoming changes the renderer that is used for prefetch will exit early
before tests have a chance to execute javascript there. Fortunately, there is
another way to check whether javascript is executed: requesting a resource from
an inline script in a blocking manner. This is simpler than changing the
lifetime of the renderer specifically for tests.

NoStatePrefetchBrowserTest.CheckJavascript would still be able to run with the
new renderer lifetime, but I decided to still remove it because it is testing
mechanisms that are not used any more.

BUG=643652
==========

to

==========
[NoStatePrefetch] Remove JS injection in tests

With upcoming changes the renderer that is used for prefetch will exit early
before tests have a chance to execute javascript there. Fortunately, there is
another way to check whether javascript is executed: requesting a resource from
an inline script in a blocking manner. This is simpler than changing the
lifetime of the renderer specifically for tests.

NoStatePrefetchBrowserTest.CheckJavascript would still be able to run with the
new renderer lifetime, but I decided to still remove it because it is testing
mechanisms that are not used any more.

BUG=643652

Committed: https://crrev.com/0b1e4e811cca307ef42d014937ee604161ce637e
Cr-Commit-Position: refs/heads/master@{#426460}
==========

[commit-bot: I haz the power, 2016-10-21 13:17:46]

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