Issue 2304953002: NoState Prefetch: nostate prefetch browser tests.

mattcary

David--- Here's a reasonably clean set of prefetch browser tests. It covers what's appropriate from ...

4 years, 3 months ago (2016-09-19 14:01:42 UTC) #1

mattcary

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

4 years, 3 months ago (2016-09-20 08:01:07 UTC) #2

mattcary

(trying again, this time with email addresses...) David--- Here's a reasonably clean set of prefetch ...

4 years, 3 months ago (2016-09-20 08:01:07 UTC) #3

droger

Looks good, only nits. https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerender/prerender_contents.h File chrome/browser/prerender/prerender_contents.h (right): https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerender/prerender_contents.h#newcode207 chrome/browser/prerender/prerender_contents.h:207: virtual void Destroy(FinalStatus reason); Add ...

4 years, 3 months ago (2016-09-20 10:57:20 UTC) #4

mattcary

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

4 years, 3 months ago (2016-09-21 08:43:37 UTC) #5

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-21 08:43:55 UTC) #6

mattcary

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

4 years, 3 months ago (2016-09-21 08:45:09 UTC) #7

mattcary

Egor--- your turn! https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerender/prerender_contents.h File chrome/browser/prerender/prerender_contents.h (right): https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerender/prerender_contents.h#newcode207 chrome/browser/prerender/prerender_contents.h:207: virtual void Destroy(FinalStatus reason); On 2016/09/20 ...

4 years, 3 months ago (2016-09-21 08:45:11 UTC) #8

Egor--- your turn!

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.h:207: virtual void
Destroy(FinalStatus reason);
On 2016/09/20 10:57:19, droger wrote:
> Add a comment:
> // Virtual for testing.
> 
> I think it would be fine to have the observer in the base class instead, but I
> don't have a strong preference.

That's not a bad idea. Why multiply observers if we don't need to?

And looking closer, it seems that OnPrerenderStop already fits the bill, so I
don't need a new OnDestroy call. Yay!

I did take this opportunity to clean up the observer class which was a little
messy.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:110:
ScopedVector<TestPrerender> PrerenderTestURLImpl(
On 2016/09/20 10:57:19, droger wrote:
> Optional: I think ScopedVector is deprecated, use
std::vector<std::unique_ptr<>>
> instead.
> Optional, because maybe it does not belong to this CL.

I thought of that, but to tear it out would require bigger changes to
prerender_browsertest.cc than I wanted to make. I'll do it in a subsequent CL
since I'm mucking around in here anyway.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:122:
ScopedVector<TestPrerender> prerenders = NavigateWithPrerenders(
On 2016/09/20 10:57:19, droger wrote:
> Same comment about ScopedVector (optional).

Acknowledged.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:162: // Checks that
a page is correctly prerendered in the case of a
On 2016/09/20 10:57:20, droger wrote:
> s/prerendered/prefetched/
> ?

Done.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:164: // navigation,
when NoState Prefetch is enabled.
On 2016/09/20 10:57:19, droger wrote:
> Maybe add something like:
> // Check that Javascript is not executed on the prefetched page.

Done.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:185: //
TODO(mattcary): add prefetch-specific histograms when droger's cl lands.
On 2016/09/20 10:57:20, droger wrote:
> That CL is landed now, but it's fine to leave this a TODO.
> Feel free to put TODO(droger) instead of TODO(mattcary).

Thanks. I can go ahead and add them, which histograms do you think I should add?

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:419: //
histogram_tester()->GetTotalCountsForPrefix
On 2016/09/20 10:57:20, droger wrote:
> Remove this line?

Opps, done.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.h:152:
std::vector<std::unique_ptr<DestroyObserver>> destroy_observers_;
On 2016/09/20 10:57:20, droger wrote:
> Use ObserverList instead (base/observer_list.h)

thanks, but moot now.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.h:218: // Does not on the waiter
which must outlive the TestPrerenderContents.
On 2016/09/20 10:57:20, droger wrote:
> I don't understand the comment, maybe there is a word missing.

Done. Only a letter missing, but an important one ;)

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.h:360: ScopedVector<TestPrerender>
NavigateWithPrerenders(
On 2016/09/20 10:57:20, droger wrote:
> same: std::vector<std::unique_ptr<>>

This will be fixed when I tear out the scoped vector from the old test.

https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.h:361: GURL loader_url,
On 2016/09/20 10:57:20, droger wrote:
> const GURL&

Done.

droger

lgtm https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc#newcode185 chrome/browser/prerender/prerender_nostate_prefetch_test.cc:185: // TODO(mattcary): add prefetch-specific histograms when droger's cl ...

4 years, 3 months ago (2016-09-21 10:01:52 UTC) #9

mattcary

On 2016/09/21 10:01:52, droger wrote: > lgtm > > https://codereview.chromium.org/2304953002/diff/40001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc > File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): > ...

4 years, 3 months ago (2016-09-21 14:58:52 UTC) #10

mattcary

On 2016/09/21 14:58:52, mattcary wrote: > On 2016/09/21 10:01:52, droger wrote: > > lgtm > ...

4 years, 3 months ago (2016-09-22 12:33:16 UTC) #11

droger

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h File chrome/browser/prerender/prerender_manager_factory.h (right): https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h#newcode37 chrome/browser/prerender/prerender_manager_factory.h:37: // profile. I don't think this is accurate. BuildServiceInstanceFor() ...

4 years, 3 months ago (2016-09-22 14:54:58 UTC) #12

pasko

I only looked at new files and header modifications, looks good, below are mostly nits ...

4 years, 3 months ago (2016-09-22 18:14:05 UTC) #13

mattcary

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h File chrome/browser/prerender/prerender_manager_factory.h (right): https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h#newcode37 chrome/browser/prerender/prerender_manager_factory.h:37: // profile. On 2016/09/22 14:54:58, droger wrote: > I ...

4 years, 3 months ago (2016-09-23 07:33:18 UTC) #14

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager_factory.h (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager_factory.h:37: // profile.
On 2016/09/22 14:54:58, droger wrote:
> I don't think this is accurate.
> 
> BuildServiceInstanceFor() will be called only once per profile.
> 
> If the prerender manager was created before the test starts, then it won't be
> created after and the testing_create_function_ will not be called.
> 
> I don't understand why exactly you need this, and how the test could fail
> without this but pass with it.
> 
> I discussed a bit with sdefresne (which is our local expert on service
> factories), and it seems that the natural approach would be to add the
necessary
> customizations in the vanilla PrerenderManager instead of trying to
instantiate
> a test one.
> 
> I guess this would be something like adding a
> SetPrefetchCacheOverrideTimeoutForTesting() method and setting it to a very
low
> value for the test, to get a "cold" prefetch.

I think this issue is specific to the in-process browser tests. It is indeed
true that this is called once per profile, but we don't get into the testing
infrastructure code until after the browser and profile have been created, and
by that time the prerender manager has already been created due (I think) to the
prerender tab helper.

So it seemed the easiest way to do it was to make a change that could be used
without having a profile, which I could hook before the browser is created.

In my experience, trying to use "low" values for timing issues ends up being
very fragile, because of different test setups, machines etc. On my desktop
there's about 1/2 second between the two navigations, so I don't think we could
consistently set a non-trivial low value for the cold prefetch.

Plus there's my aversion to having testing harnesses mixed up with production
code.

droger

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h File chrome/browser/prerender/prerender_manager_factory.h (right): https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h#newcode37 chrome/browser/prerender/prerender_manager_factory.h:37: // profile. On 2016/09/23 07:33:18, mattcary wrote: > On ...

4 years, 3 months ago (2016-09-23 08:15:36 UTC) #15

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager_factory.h (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager_factory.h:37: // profile.
On 2016/09/23 07:33:18, mattcary wrote:
> On 2016/09/22 14:54:58, droger wrote:
> > I don't think this is accurate.
> > 
> > BuildServiceInstanceFor() will be called only once per profile.
> > 
> > If the prerender manager was created before the test starts, then it won't
be
> > created after and the testing_create_function_ will not be called.
> > 
> > I don't understand why exactly you need this, and how the test could fail
> > without this but pass with it.
> > 
> > I discussed a bit with sdefresne (which is our local expert on service
> > factories), and it seems that the natural approach would be to add the
> necessary
> > customizations in the vanilla PrerenderManager instead of trying to
> instantiate
> > a test one.
> > 
> > I guess this would be something like adding a
> > SetPrefetchCacheOverrideTimeoutForTesting() method and setting it to a very
> low
> > value for the test, to get a "cold" prefetch.
> 
> I think this issue is specific to the in-process browser tests. It is indeed
> true that this is called once per profile, but we don't get into the testing
> infrastructure code until after the browser and profile have been created, and
> by that time the prerender manager has already been created due (I think) to
the
> prerender tab helper.
> 
> So it seemed the easiest way to do it was to make a change that could be used
> without having a profile, which I could hook before the browser is created.

My point is:

- if the prerender manager is created earlier than the test, then your new code
is not called at all.

Another way to say the same thing:

- if your code is called, then it means that the prerender manager is not
created earlier.


I think at minimum the comment is wrong. Let me try to patch the CL to
investigate a bit.

mattcary

On 2016/09/23 08:15:36, droger wrote: > https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h > File chrome/browser/prerender/prerender_manager_factory.h (right): > > https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h#newcode37 > ...

4 years, 3 months ago (2016-09-23 08:42:10 UTC) #16

On 2016/09/23 08:15:36, droger wrote:
>
https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
> File chrome/browser/prerender/prerender_manager_factory.h (right):
> 
>
https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
> chrome/browser/prerender/prerender_manager_factory.h:37: // profile.
> On 2016/09/23 07:33:18, mattcary wrote:
> > On 2016/09/22 14:54:58, droger wrote:
> > > I don't think this is accurate.
> > > 
> > > BuildServiceInstanceFor() will be called only once per profile.
> > > 
> > > If the prerender manager was created before the test starts, then it won't
> be
> > > created after and the testing_create_function_ will not be called.
> > > 
> > > I don't understand why exactly you need this, and how the test could fail
> > > without this but pass with it.
> > > 
> > > I discussed a bit with sdefresne (which is our local expert on service
> > > factories), and it seems that the natural approach would be to add the
> > necessary
> > > customizations in the vanilla PrerenderManager instead of trying to
> > instantiate
> > > a test one.
> > > 
> > > I guess this would be something like adding a
> > > SetPrefetchCacheOverrideTimeoutForTesting() method and setting it to a
very
> > low
> > > value for the test, to get a "cold" prefetch.
> > 
> > I think this issue is specific to the in-process browser tests. It is indeed
> > true that this is called once per profile, but we don't get into the testing
> > infrastructure code until after the browser and profile have been created,
and
> > by that time the prerender manager has already been created due (I think) to
> the
> > prerender tab helper.
> > 
> > So it seemed the easiest way to do it was to make a change that could be
used
> > without having a profile, which I could hook before the browser is created.
> 
> My point is:
> 
> - if the prerender manager is created earlier than the test, then your new
code
> is not called at all.
> 
> Another way to say the same thing:
> 
> - if your code is called, then it means that the prerender manager is not
> created earlier.
> 
> 
> I think at minimum the comment is wrong. Let me try to patch the CL to
> investigate a bit.

Discussed offline where I explained myself a little better. I clarified the
comment, LMK.

mattcary

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_browsertest.cc#newcode1611 chrome/browser/prerender/prerender_browsertest.cc:1611: PrerenderNoSSLReferrer) { On 2016/09/22 18:14:04, pasko wrote: > nit: ...

4 years, 3 months ago (2016-09-23 09:00:16 UTC) #17

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1611: PrerenderNoSSLReferrer)
{
On 2016/09/22 18:14:04, pasko wrote:
> nit: fits one line

Done. Strange that git cl format didn't catch that. Oh, I bet this wasn't
actually in my CL, so git didn't touch it. Anyway, fixed now.

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1613: // Use non-https url for
first page.
On 2016/09/22 18:14:04, pasko wrote:
> I am confused. Is this comment applying to the line below it? Does it say
> something extra on top of the test description? The latter is just 3-4 lines
> above...

The test description says that the source page is HTTPS. This is clarifying that
the first prerendered page is *not* https, which is why we have to create the
URL directly rather than just
 PrerenderTestUrl("/prerender/prerender_no_referrer.html", ...)

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2101: // We only send the
loaded page, not the loader, through SSL.
On 2016/09/22 18:14:04, pasko wrote:
> Isn't this a subset of what the comment two lines above says? When we say
about
> something 'cancelled' in context of prerender, it is assumed to be about the
> prerendered page.

It's further detail that explains further why this test differs from other
tests, in particular why we are instantiating a separate EmbeddedTestServer here
rather than just calling UseHttpsSrcServer(). That's a key detail that probably
won't be obvious to someone changing this test in the future.

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:180: PrerenderContents* contents)
{}
On 2016/09/22 18:14:04, pasko wrote:
> nit: consistency in how we put braces would be nice

This is again git cl format, which does not format untouched code. I've changed
the code outside of my cl.

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
File chrome/test/data/prerender/prerender_prefetch2.js (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
chrome/test/data/prerender/prerender_prefetch2.js:6: //
prerender_prefetch_page2.html.
On 2016/09/22 18:14:04, pasko wrote:
> Should this be referred to as 'alternative' instead of 'duplicate'? It is not
> duplicating the code, nor the functionality.

Done.

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
File chrome/test/data/prerender/prerender_prefetch_image.html (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
chrome/test/data/prerender/prerender_prefetch_image.html:3: This page checks for
prefetching of a an image.
On 2016/09/22 18:14:05, pasko wrote:
> let's name the files like:
> 
> chrome/test/data/prerender/prefetch_image.html
> 
> i.e. without the leading "prerender_" in the last component of the path. Most
of
> other files read as if "prerender" is a verb, like "prerender_and_do_foo", so
a
> consistent extension to this would be "prefetch_and_do_foo", and luckily we
> already have the word "prerender" in the path.

Yeah, I was thinking about that. I started naming them consistently with
existing files, but all the prerender_prefetch_stuff is pretty silly.

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
chrome/test/data/prerender/prerender_prefetch_image.html:6: <title>Prefetch
Page</title>
On 2016/09/22 18:14:05, pasko wrote:
> please add consistent 2 space indentation in the new html files. I know that
not
> all files in this directory are doing it, but it really helps to clearly see
> that something is in head and something other is in body, for example.
> 
> Not asking to do this for .cc files because .cc file names should be unique
due
> to [...] reasons in various build systems.

Done.

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
File chrome/test/data/prerender/prerender_prefetch_subresource_redirect.html
(right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
chrome/test/data/prerender/prerender_prefetch_subresource_redirect.html:4: that
we aren't executing.
On 2016/09/22 18:14:05, pasko wrote:
> It is generally preferable to avoid 'we' in comments. Makes it clearer what is
> actually happening.
> 
> Alternative formulation: The test checks that the javascript code included in
> this file is not executed by the prefetch mechanism.

Done.

https://codereview.chromium.org/2304953002/diff/100001/chrome/test/data/prere...
chrome/test/data/prerender/prerender_prefetch_subresource_redirect.html:4: that
we aren't executing.
On 2016/09/22 18:14:05, pasko wrote:
> Actually, in the single use of this file in prerender_nostate_prefetch_test.cc
I
> cannot see where script execution is checked, am I missing something?

Nope, copy-paste remnents.

droger

On 2016/09/23 08:42:10, mattcary wrote: > Discussed offline where I explained myself a little better. ...

4 years, 3 months ago (2016-09-23 09:04:35 UTC) #18

mattcary

On 2016/09/23 09:04:35, droger wrote: > On 2016/09/23 08:42:10, mattcary wrote: > > Discussed offline ...

4 years, 3 months ago (2016-09-23 09:07:45 UTC) #19

droger

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h File chrome/browser/prerender/prerender_manager_factory.h (right): https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h#newcode38 chrome/browser/prerender/prerender_manager_factory.h:38: using PrerenderCreateFunction = PrerenderManager*(Profile*); We don't need a profile ...

4 years, 3 months ago (2016-09-23 09:09:33 UTC) #20

droger

On 2016/09/23 09:07:45, mattcary wrote: > On 2016/09/23 09:04:35, droger wrote: > > On 2016/09/23 ...

4 years, 3 months ago (2016-09-23 09:11:16 UTC) #21

mattcary

On 2016/09/23 09:09:33, droger wrote: > https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h > File chrome/browser/prerender/prerender_manager_factory.h (right): > > https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_manager_factory.h#newcode38 > ...

4 years, 3 months ago (2016-09-23 09:26:14 UTC) #22

mattcary

On 2016/09/23 09:11:16, droger wrote: > On 2016/09/23 09:07:45, mattcary wrote: > > On 2016/09/23 ...

4 years, 3 months ago (2016-09-23 09:26:32 UTC) #23

droger

LGTM https://codereview.chromium.org/2304953002/diff/120001/chrome/browser/prerender/prerender_test_utils.cc File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2304953002/diff/120001/chrome/browser/prerender/prerender_test_utils.cc#newcode483 chrome/browser/prerender/prerender_test_utils.cc:483: return static_cast<std::unique_ptr<KeyedService>>( Is the static cast really needed? ...

4 years, 3 months ago (2016-09-23 09:54:40 UTC) #24

mattcary

On 2016/09/23 09:54:40, droger wrote: > LGTM > > https://codereview.chromium.org/2304953002/diff/120001/chrome/browser/prerender/prerender_test_utils.cc > File chrome/browser/prerender/prerender_test_utils.cc (right): > ...

4 years, 3 months ago (2016-09-23 10:04:47 UTC) #25

pasko

one big concern about TestPrerenderManager, the rest is nitpicking about the style of comments I ...

4 years, 2 months ago (2016-09-30 18:29:21 UTC) #26

one big concern about TestPrerenderManager, the rest is nitpicking about the
style of comments

I only started looking at the actual tests, they start looking quite reasonable,
thanks!

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1613: // Use non-https url for
first page.
On 2016/09/23 09:00:15, mattcary wrote:
> On 2016/09/22 18:14:04, pasko wrote:
> > I am confused. Is this comment applying to the line below it? Does it say
> > something extra on top of the test description? The latter is just 3-4 lines
> > above...
> 
> The test description says that the source page is HTTPS. This is clarifying
that
> the first prerendered page is *not* https, which is why we have to create the
> URL directly rather than just
>  PrerenderTestUrl("/prerender/prerender_no_referrer.html", ...)

OK, I think it would be more readable like that:
===
  // Use http:// url for the prerendered page.
  GURL url(
      embedded_test_server()->GetURL("/prerender/prerender_no_referrer.html"));

  UseHttpsSrcServer();
  PrerenderTestURL(url, FINAL_STATUS_USED, 1);
  NavigateToDestURL();
===

This would:
* by using extra spacing suggest which range of lines the comment corresponds to
* avoid ambiguity of 'first' that could refer to the page issuing the prerender
* more precise with 'http://' compared to 'non-https' (I know, my nits are
always great)

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2101: // We only send the
loaded page, not the loader, through SSL.
On 2016/09/23 09:00:15, mattcary wrote:
> On 2016/09/22 18:14:04, pasko wrote:
> > Isn't this a subset of what the comment two lines above says? When we say
> about
> > something 'cancelled' in context of prerender, it is assumed to be about the
> > prerendered page.
> 
> It's further detail that explains further why this test differs from other
> tests, in particular why we are instantiating a separate EmbeddedTestServer
here
> rather than just calling UseHttpsSrcServer(). That's a key detail that
probably
> won't be obvious to someone changing this test in the future.

I see. I agree that manipulations in this test are somewhat surprising compared
to other tests, so extra commenting looks good.

For me confusing parts were:
1. use of 'we'
2. big overlap between the two comments begged for more efficiency in comments
:)
3. ambiguity in the term 'loader'

To fix the above my suggestion would be:

// Sets up initial page on HTTP and checks that an SSL error on the prerendered
page cancels the prerender.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager_factory.h (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager_factory.h:41: // PrerenderManager
instance has already by created (eg, by the prerender tab
nit: s/by/been/

(but actually it is better to remove this hack entirely, see other comment)

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:33: namespace
prerender {
nit: extra empty line above namespace

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:36: // etc) are
fussy about relative versus absolute paths. With the exception of
fussy? that's very concrete and easy to understand. Am I being fussy at
explaining what I want or just ironic?

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:82: bool
GetJavascriptBoolean(const std::string& javascript_variable,
static? why does this need to be in the NoStatePrefetchBrowserTest? Putting this
(and below) functions to anonymous namespace would make the dependencies
clearer.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:85: // In order to
detect unknown variables we need a three-valued return.
nit: ... a three-valued return is needed.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87:
EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
should this rather be a fatal failure? It does not really make much sense to
continue the test if this functionality is not working. In the current form we
risk crashing after EXPECT_TRUE fails. This may be a problem if the tests are
all run in the same process (even though this consideration might be
hypothetical for these particular tests).

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:138: // Checks that
when the prefetch page is loaded in full, javascript values are
s/prefetch/prefetcher/ or 'the initial page requesting the prefetch' for extra
clarity?

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:198: // Check the
histograms on reuse of a prefetch.
More concrete: Checks that the TTFCP histograms are updated on use of a
prefetch.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:211: // Check the
histograms on reuse of a prefetch.
Checks

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:225: // Test that we
prefetch an img tag in body using our particular
Tests
avoid we, our

why using polluting words like 'particular'? Wouldn't it be better to load this
space occupied by the comment with something more concrete? Another option:
remove the words after the word 'body'.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:242: // Check
cross-domain prefetching by looking at histograms.
Checks that .. cross-domain prefetching does X when Y.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:307: return
static_cast<TestPrerenderManager*>(GetPrerenderManager());
Problems with having a new TestPrerenderManager as far as I can see:
* This downcast
* Hard to follow differences between TestPrerenderManager::AdvanceTime() and
UnitTestPrerenderManager::AdvanceTime(), having AdvanceTimeTicks() in one and
not having it in another
* crazy static testing_create_function_ - not for limited brains like mine

A simpler solution is to have a utility class that can be mixed into the
whatever prerendermanager we have.

class SyntheticTimeOverride {
 public:
  SyntheticTimeOverride(base::Time, base::TimeTicks);
  GetCurrentTime();
  GetCurrentTimeTicks();
 private:
  ...
};

In PrerenderManager we would have:

void SetSyntheticTimeForTesting(std::unique_ptr<SyntheticTimeOverride>);

void GetCurrentTime() {
  if (synthetic_time_override_)
    return synthetic_time_override_->GetCurrentTime();
}

As usual, would be glad to know why this might not work :)

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:329: void
UseHttpsSrcServer(std::function<void(net::EmbeddedTestServer*)> startup);
This form with std::function is not called anywhere except with the trivial
lambda below. Hard to tell why you need it when reviewing this. Please remove it
in this change and re-introduce in the change that actually needs it. We will be
able to discuss alternatives there.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:334: // Returns the currently
active server (see UseHttpsSrcServer).
nit: 
// Returns the currently active server. See UseHttpsSrcServer().

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:335: net::EmbeddedTestServer*
src_server() {
non-trivial implementation -> .cc

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:346:
GetFakeSafeBrowsingDatabaseManager() {
non-trivial implementation -> .cc

mattcary

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerender/prerender_browsertest.cc#newcode1613 chrome/browser/prerender/prerender_browsertest.cc:1613: // Use non-https url for first page. On 2016/09/30 ...

4 years, 2 months ago (2016-10-03 10:58:35 UTC) #27

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1613: // Use non-https url for
first page.
On 2016/09/30 18:29:20, pasko wrote:
> On 2016/09/23 09:00:15, mattcary wrote:
> > On 2016/09/22 18:14:04, pasko wrote:
> > > I am confused. Is this comment applying to the line below it? Does it say
> > > something extra on top of the test description? The latter is just 3-4
lines
> > > above...
> > 
> > The test description says that the source page is HTTPS. This is clarifying
> that
> > the first prerendered page is *not* https, which is why we have to create
the
> > URL directly rather than just
> >  PrerenderTestUrl("/prerender/prerender_no_referrer.html", ...)
> 
> OK, I think it would be more readable like that:
> ===
>   // Use http:// url for the prerendered page.
>   GURL url(
>      
embedded_test_server()->GetURL("/prerender/prerender_no_referrer.html"));
> 
>   UseHttpsSrcServer();
>   PrerenderTestURL(url, FINAL_STATUS_USED, 1);
>   NavigateToDestURL();
> ===
> 
> This would:
> * by using extra spacing suggest which range of lines the comment corresponds
to
> * avoid ambiguity of 'first' that could refer to the page issuing the
prerender
> * more precise with 'http://' compared to 'non-https' (I know, my nits are
> always great)

Done, with slight modification.

https://codereview.chromium.org/2304953002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2101: // We only send the
loaded page, not the loader, through SSL.
On 2016/09/30 18:29:20, pasko wrote:
> On 2016/09/23 09:00:15, mattcary wrote:
> > On 2016/09/22 18:14:04, pasko wrote:
> > > Isn't this a subset of what the comment two lines above says? When we say
> > about
> > > something 'cancelled' in context of prerender, it is assumed to be about
the
> > > prerendered page.
> > 
> > It's further detail that explains further why this test differs from other
> > tests, in particular why we are instantiating a separate EmbeddedTestServer
> here
> > rather than just calling UseHttpsSrcServer(). That's a key detail that
> probably
> > won't be obvious to someone changing this test in the future.
> 
> I see. I agree that manipulations in this test are somewhat surprising
compared
> to other tests, so extra commenting looks good.
> 
> For me confusing parts were:
> 1. use of 'we'
> 2. big overlap between the two comments begged for more efficiency in comments
> :)
> 3. ambiguity in the term 'loader'
> 
> To fix the above my suggestion would be:
> 
> // Sets up initial page on HTTP and checks that an SSL error on the
prerendered
> page cancels the prerender.

Done, thanks, I am still quite sloppy on use of "we".

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:33: namespace
prerender {
On 2016/09/30 18:29:21, pasko wrote:
> nit: extra empty line above namespace

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:36: // etc) are
fussy about relative versus absolute paths. With the exception of
On 2016/09/30 18:29:21, pasko wrote:
> fussy? that's very concrete and easy to understand. Am I being fussy at
> explaining what I want or just ironic?

Acknowledged.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:82: bool
GetJavascriptBoolean(const std::string& javascript_variable,
On 2016/09/30 18:29:20, pasko wrote:
> static? why does this need to be in the NoStatePrefetchBrowserTest? Putting
this
> (and below) functions to anonymous namespace would make the dependencies
> clearer.

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:85: // In order to
detect unknown variables we need a three-valued return.
On 2016/09/30 18:29:20, pasko wrote:
> nit: ... a three-valued return is needed.

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87:
EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
On 2016/09/30 18:29:21, pasko wrote:
> should this rather be a fatal failure? It does not really make much sense to
> continue the test if this functionality is not working. In the current form we
> risk crashing after EXPECT_TRUE fails. This may be a problem if the tests are
> all run in the same process (even though this consideration might be
> hypothetical for these particular tests).

Yes, but it won't compile! ASSERT_TRUE can only be used directly from a test
function as the macro expands to include a void return statement. At least
that's as much as I could tell from the error message.

I'd be happy to use another suggestion for assert macro as I had the same
feeling as you, but it wasn't clear to me what the right macro to use was.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:138: // Checks that
when the prefetch page is loaded in full, javascript values are
On 2016/09/30 18:29:20, pasko wrote:
> s/prefetch/prefetcher/ or 'the initial page requesting the prefetch' for extra
> clarity?

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:198: // Check the
histograms on reuse of a prefetch.
On 2016/09/30 18:29:20, pasko wrote:
> More concrete: Checks that the TTFCP histograms are updated on use of a
> prefetch.

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:211: // Check the
histograms on reuse of a prefetch.
On 2016/09/30 18:29:20, pasko wrote:
> Checks

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:225: // Test that we
prefetch an img tag in body using our particular
On 2016/09/30 18:29:21, pasko wrote:
> Tests
> avoid we, our
> 
> why using polluting words like 'particular'? Wouldn't it be better to load
this
> space occupied by the comment with something more concrete? Another option:
> remove the words after the word 'body'.

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:242: // Check
cross-domain prefetching by looking at histograms.
On 2016/09/30 18:29:20, pasko wrote:
> Checks that .. cross-domain prefetching does X when Y.

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:307: return
static_cast<TestPrerenderManager*>(GetPrerenderManager());
On 2016/09/30 18:29:21, pasko wrote:
> Problems with having a new TestPrerenderManager as far as I can see:
> * This downcast
> * Hard to follow differences between TestPrerenderManager::AdvanceTime() and
> UnitTestPrerenderManager::AdvanceTime(), having AdvanceTimeTicks() in one and
> not having it in another
> * crazy static testing_create_function_ - not for limited brains like mine
> 
> A simpler solution is to have a utility class that can be mixed into the
> whatever prerendermanager we have.
> 
> class SyntheticTimeOverride {
>  public:
>   SyntheticTimeOverride(base::Time, base::TimeTicks);
>   GetCurrentTime();
>   GetCurrentTimeTicks();
>  private:
>   ...
> };
> 
> In PrerenderManager we would have:
> 
> void SetSyntheticTimeForTesting(std::unique_ptr<SyntheticTimeOverride>);
> 
> void GetCurrentTime() {
>   if (synthetic_time_override_)
>     return synthetic_time_override_->GetCurrentTime();
> }
> 
> As usual, would be glad to know why this might not work :)

I really dislike mixing testing overrides into production code, especially for
things like time management which is hard enough to do right just one time. So
the dependency injection style of testing works well from this perspective. 

I agree there is extra cognitive load to having different test time adjustments
in the unittests vs the browser tests. But, OTOH, one would expect the test
setup to be different.

But, you know, whatever. If you feel that the advantages to something like this
outweigh this disadvantage of having testing hooks live in production code I can
change it.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:329: void
UseHttpsSrcServer(std::function<void(net::EmbeddedTestServer*)> startup);
On 2016/09/30 18:29:21, pasko wrote:
> This form with std::function is not called anywhere except with the trivial
> lambda below. Hard to tell why you need it when reviewing this. Please remove
it
> in this change and re-introduce in the change that actually needs it. We will
be
> able to discuss alternatives there.

Oops, I guess this was a git screwup to have this appear here.

However, AFAIU std:function is the right way to declare lambda types, as there
is no direct lambda declaration.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:334: // Returns the currently
active server (see UseHttpsSrcServer).
On 2016/09/30 18:29:21, pasko wrote:
> nit: 
> // Returns the currently active server. See UseHttpsSrcServer().

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:335: net::EmbeddedTestServer*
src_server() {
On 2016/09/30 18:29:21, pasko wrote:
> non-trivial implementation -> .cc

Done.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:346:
GetFakeSafeBrowsingDatabaseManager() {
On 2016/09/30 18:29:21, pasko wrote:
> non-trivial implementation -> .cc

Done.

droger

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc#newcode87 chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87: EXPECT_TRUE(content::ExecuteScriptAndExtractInt( On 2016/10/03 10:58:35, mattcary wrote: > On 2016/09/30 ...

4 years, 2 months ago (2016-10-03 11:07:13 UTC) #28

droger

On 2016/10/03 11:07:13, droger wrote: > https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc > File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): > > https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc#newcode87 > ...

4 years, 2 months ago (2016-10-03 11:11:04 UTC) #29

mattcary

On 2016/10/03 11:11:04, droger wrote: > On 2016/10/03 11:07:13, droger wrote: > > > https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc ...

4 years, 2 months ago (2016-10-03 11:17:02 UTC) #30

On 2016/10/03 11:11:04, droger wrote:
> On 2016/10/03 11:07:13, droger wrote:
> >
>
https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
> > File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
> > chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87:
> > EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
> > On 2016/10/03 10:58:35, mattcary wrote:
> > > On 2016/09/30 18:29:21, pasko wrote:
> > > > should this rather be a fatal failure? It does not really make much
sense
> to
> > > > continue the test if this functionality is not working. In the current
> form
> > we
> > > > risk crashing after EXPECT_TRUE fails. This may be a problem if the
tests
> > are
> > > > all run in the same process (even though this consideration might be
> > > > hypothetical for these particular tests).
> > > 
> > > Yes, but it won't compile! ASSERT_TRUE can only be used directly from a
test
> > > function as the macro expands to include a void return statement. At least
> > > that's as much as I could tell from the error message.
> > > 
> > > I'd be happy to use another suggestion for assert macro as I had the same
> > > feeling as you, but it wasn't clear to me what the right macro to use was.
> > 
> > The relevant gtest doc is here:
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > 
> > I think the options are:
> > - use a macro to add a failure here, such as FAIL(), and call
> HasFatalFailure()
> > at the call site.
> > - change the return value to convey fatal failures (such as return an error
> code
> > instead of a bool), and then use ASSERT macros at the call site.
> > 
> > Both are a bit clunky unfortunately.
> 
> Or also you can change your function to return void.

Thanks for the links, David!

I feel like the EXPECT_EQ is the least worst option. But I will bow to the
majority opinion (note that GetJavascriptBoolean already has a value-return
parameter, so return a second one is also quite clunky).

droger

On 2016/10/03 11:17:02, mattcary wrote: > I feel like the EXPECT_EQ is the least worst ...

4 years, 2 months ago (2016-10-03 11:56:31 UTC) #31

pasko

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc#newcode87 chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87: EXPECT_TRUE(content::ExecuteScriptAndExtractInt( On 2016/10/03 11:07:13, droger wrote: > On 2016/10/03 ...

4 years, 2 months ago (2016-10-03 18:54:17 UTC) #32

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87:
EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
On 2016/10/03 11:07:13, droger wrote:
> On 2016/10/03 10:58:35, mattcary wrote:
> > On 2016/09/30 18:29:21, pasko wrote:
> > > should this rather be a fatal failure? It does not really make much sense
to
> > > continue the test if this functionality is not working. In the current
form
> we
> > > risk crashing after EXPECT_TRUE fails. This may be a problem if the tests
> are
> > > all run in the same process (even though this consideration might be
> > > hypothetical for these particular tests).
> > 
> > Yes, but it won't compile! ASSERT_TRUE can only be used directly from a test
> > function as the macro expands to include a void return statement. At least
> > that's as much as I could tell from the error message.
> > 
> > I'd be happy to use another suggestion for assert macro as I had the same
> > feeling as you, but it wasn't clear to me what the right macro to use was.
> 
> The relevant gtest doc is here:
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> 
> I think the options are:
> - use a macro to add a failure here, such as FAIL(), and call
HasFatalFailure()
> at the call site.
> - change the return value to convey fatal failures (such as return an error
code
> instead of a bool), and then use ASSERT macros at the call site.
> 
> Both are a bit clunky unfortunately.

Thanks for investigation, folks. Agreed that ASSERT_NO_FATAL_FAILURE /
HasFatalFailure() is burdensome and does not buy us much.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:307: return
static_cast<TestPrerenderManager*>(GetPrerenderManager());
On 2016/10/03 10:58:35, mattcary wrote:
> On 2016/09/30 18:29:21, pasko wrote:
> > Problems with having a new TestPrerenderManager as far as I can see:
> > * This downcast
> > * Hard to follow differences between TestPrerenderManager::AdvanceTime() and
> > UnitTestPrerenderManager::AdvanceTime(), having AdvanceTimeTicks() in one
and
> > not having it in another
> > * crazy static testing_create_function_ - not for limited brains like mine
> > 
> > A simpler solution is to have a utility class that can be mixed into the
> > whatever prerendermanager we have.
> > 
> > class SyntheticTimeOverride {
> >  public:
> >   SyntheticTimeOverride(base::Time, base::TimeTicks);
> >   GetCurrentTime();
> >   GetCurrentTimeTicks();
> >  private:
> >   ...
> > };
> > 
> > In PrerenderManager we would have:
> > 
> > void SetSyntheticTimeForTesting(std::unique_ptr<SyntheticTimeOverride>);
> > 
> > void GetCurrentTime() {
> >   if (synthetic_time_override_)
> >     return synthetic_time_override_->GetCurrentTime();
> > }
> > 
> > As usual, would be glad to know why this might not work :)
> 
> I really dislike mixing testing overrides into production code, especially for
> things like time management which is hard enough to do right just one time. So
> the dependency injection style of testing works well from this perspective. 
> 
> I agree there is extra cognitive load to having different test time
adjustments
> in the unittests vs the browser tests. But, OTOH, one would expect the test
> setup to be different.
> 
> But, you know, whatever. If you feel that the advantages to something like
this
> outweigh this disadvantage of having testing hooks live in production code I
can
> change it.

Yes please. I agree that mocking out methods for testing via overrides is a good
tool, but when it requires messing up with complicated initialization order
versus three injection checks in production code, I am voting for injection.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:329: void
UseHttpsSrcServer(std::function<void(net::EmbeddedTestServer*)> startup);
On 2016/10/03 10:58:35, mattcary wrote:
> On 2016/09/30 18:29:21, pasko wrote:
> > This form with std::function is not called anywhere except with the trivial
> > lambda below. Hard to tell why you need it when reviewing this. Please
remove
> it
> > in this change and re-introduce in the change that actually needs it. We
will
> be
> > able to discuss alternatives there.
> 
> Oops, I guess this was a git screwup to have this appear here.
> 
> However, AFAIU std:function is the right way to declare lambda types, as there
> is no direct lambda declaration.

Yes, the type of lambda is hidden from the language. I never understood it,
since the debuggers will need to print this type anyway.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2123: // Set up HTTPS server
for prerendered page, and check that an SSL error will
Please replace the toplevel comment for this test with this comment and
s/Set/Sets/ s/check/checks/.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2124: // cancel the prerender.
The prerenderer loader will serve through HTTP.
perhaps "will be served"? I do not understand how a prerender loader can become
an HTTP server.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
nit: prerender_nostate_prefetch_browsertest.cc

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:72: // etc) are
fussy about relative versus absolute paths. With the exception of
I cannot see why the first sentence in this comment is useful. It may get out of
date because it is not easy to confirm that it is still true. Maybe remove it
and slightly rephrase the second sentence?

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:142: // Perform a
full load of the target page and check that javascript values are
nit: s/Perform/Performs/

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:204: // Checks the
TTFCP histograms on reuse of a prefetch.
// Checks the TTFCP histograms on fast reuse of a prefetch.
IN_PROC_BROWSER_TEST_F(NoStatePrefetchBrowserTest, PrefetchReusedWarm) {

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:256:
histogram_tester().ExpectTotalCount("Prerender.PerceivedPLT", 1);
would it be possible to check the Prefetch histograms here instead of the pure
Prerender ones? Is it A TODO?

PLT is being phased out, so let's try not to add dependencies on PerceivedPLT as
well.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:262: // Checks that
we support simultaneous prefetch.
I did not realize that we allow it. Sounds like a bunch of link rel=prerenders
can evict whole lots of tabs from memory and bring the browser to its knees.
Should he have an item to fix it?

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:286: // Checks that
we correctly handle a prefetch to a nonexisting page.
'correctly handle' is too general. Please avoid 'we' here and in all comments
below, thanks ;)

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:292: // what we
want?
Will this leave the renderer up until it expires?

I think counting it is fine, but there is an implicit TODO to kill the renderer
in this case, which is not killed automatically, I assume.

Meta-question: will these tests accumulate many renderers when run? If so, may
make some other tests flaky by sitting there, occupying memory and making OOM
killers happy.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:364: // We should
not have a problem if a subresource fails SSL.
'a problem' is a bit too general, how can we clarify?

// Checks that a subresource with an SSL error does not stop other subresources
from being prefetched.

?

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:443: // Checks that
the prefetch of png correctly loads only the png.
is it essential to check different img types? Are we in fact testing that only
the png is prefetched?

If prefetch also had pulled the favicon? Would this test have noticed this?

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:465: // Prefetch
resource are blocked, but the prerender is not killed in any
s/resource/resources/

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:467: //
TODO(mattcary): do we care about detecting if the main resource is fetched
We should not prefetch the main resource to cache if it is blocked by SB. How
does regular Prerender solve this problem?

mattcary

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerender/prerender_nostate_prefetch_test.cc#newcode87 chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87: EXPECT_TRUE(content::ExecuteScriptAndExtractInt( On 2016/10/03 18:54:16, pasko wrote: > On 2016/10/03 ...

4 years, 2 months ago (2016-10-04 08:25:12 UTC) #33

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:87:
EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
On 2016/10/03 18:54:16, pasko wrote:
> On 2016/10/03 11:07:13, droger wrote:
> > On 2016/10/03 10:58:35, mattcary wrote:
> > > On 2016/09/30 18:29:21, pasko wrote:
> > > > should this rather be a fatal failure? It does not really make much
sense
> to
> > > > continue the test if this functionality is not working. In the current
> form
> > we
> > > > risk crashing after EXPECT_TRUE fails. This may be a problem if the
tests
> > are
> > > > all run in the same process (even though this consideration might be
> > > > hypothetical for these particular tests).
> > > 
> > > Yes, but it won't compile! ASSERT_TRUE can only be used directly from a
test
> > > function as the macro expands to include a void return statement. At least
> > > that's as much as I could tell from the error message.
> > > 
> > > I'd be happy to use another suggestion for assert macro as I had the same
> > > feeling as you, but it wasn't clear to me what the right macro to use was.
> > 
> > The relevant gtest doc is here:
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > 
> > I think the options are:
> > - use a macro to add a failure here, such as FAIL(), and call
> HasFatalFailure()
> > at the call site.
> > - change the return value to convey fatal failures (such as return an error
> code
> > instead of a bool), and then use ASSERT macros at the call site.
> > 
> > Both are a bit clunky unfortunately.
> 
> Thanks for investigation, folks. Agreed that ASSERT_NO_FATAL_FAILURE /
> HasFatalFailure() is burdensome and does not buy us much.

Acknowledged.

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2304953002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:329: void
UseHttpsSrcServer(std::function<void(net::EmbeddedTestServer*)> startup);
On 2016/10/03 18:54:16, pasko wrote:
> On 2016/10/03 10:58:35, mattcary wrote:
> > On 2016/09/30 18:29:21, pasko wrote:
> > > This form with std::function is not called anywhere except with the
trivial
> > > lambda below. Hard to tell why you need it when reviewing this. Please
> remove
> > it
> > > in this change and re-introduce in the change that actually needs it. We
> will
> > be
> > > able to discuss alternatives there.
> > 
> > Oops, I guess this was a git screwup to have this appear here.
> > 
> > However, AFAIU std:function is the right way to declare lambda types, as
there
> > is no direct lambda declaration.
> 
> Yes, the type of lambda is hidden from the language. I never understood it,
> since the debuggers will need to print this type anyway.

Acknowledged.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2123: // Set up HTTPS server
for prerendered page, and check that an SSL error will
On 2016/10/03 18:54:17, pasko wrote:
> Please replace the toplevel comment for this test with this comment and
> s/Set/Sets/ s/check/checks/.

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2124: // cancel the prerender.
The prerenderer loader will serve through HTTP.
On 2016/10/03 18:54:17, pasko wrote:
> perhaps "will be served"? I do not understand how a prerender loader can
become
> an HTTP server.

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2016/10/03 18:54:17, pasko wrote:
> nit: prerender_nostate_prefetch_browsertest.cc

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:72: // etc) are
fussy about relative versus absolute paths. With the exception of
On 2016/10/03 18:54:17, pasko wrote:
> I cannot see why the first sentence in this comment is useful. It may get out
of
> date because it is not easy to confirm that it is still true. Maybe remove it
> and slightly rephrase the second sentence?

Why don't you give me the exact wording you want--that will be faster.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:142: // Perform a
full load of the target page and check that javascript values are
On 2016/10/03 18:54:17, pasko wrote:
> nit: s/Perform/Performs/

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:204: // Checks the
TTFCP histograms on reuse of a prefetch.
On 2016/10/03 18:54:17, pasko wrote:
> // Checks the TTFCP histograms on fast reuse of a prefetch.
> IN_PROC_BROWSER_TEST_F(NoStatePrefetchBrowserTest, PrefetchReusedWarm) {

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:256:
histogram_tester().ExpectTotalCount("Prerender.PerceivedPLT", 1);
On 2016/10/03 18:54:17, pasko wrote:
> would it be possible to check the Prefetch histograms here instead of the pure
> Prerender ones? Is it A TODO?
> 
> PLT is being phased out, so let's try not to add dependencies on PerceivedPLT
as
> well.

There are PLT histograms everywhere, let's do that in a different CL. When this
CL was started we didn't have any of the new histograms in.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:262: // Checks that
we support simultaneous prefetch.
On 2016/10/03 18:54:17, pasko wrote:
> I did not realize that we allow it. Sounds like a bunch of link rel=prerenders
> can evict whole lots of tabs from memory and bring the browser to its knees.
> Should he have an item to fix it?

Yup, probably.... I feel like there are several tests here that document current
behavior that we will want to change.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:286: // Checks that
we correctly handle a prefetch to a nonexisting page.
On 2016/10/03 18:54:17, pasko wrote:
> 'correctly handle' is too general. Please avoid 'we' here and in all comments
> below, thanks ;)

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:292: // what we
want?
On 2016/10/03 18:54:17, pasko wrote:
> Will this leave the renderer up until it expires?
> 
> I think counting it is fine, but there is an implicit TODO to kill the
renderer
> in this case, which is not killed automatically, I assume.
> 
Yeah, see comment above.

> Meta-question: will these tests accumulate many renderers when run? If so, may
> make some other tests flaky by sitting there, occupying memory and making OOM
> killers happy.

As far as I understand these browser tests, we spin up a new chrome instance for
each test. So all processes are killed and none of the tests should interfere
with each other.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:364: // We should
not have a problem if a subresource fails SSL.
On 2016/10/03 18:54:17, pasko wrote:
> 'a problem' is a bit too general, how can we clarify?
> 
> // Checks that a subresource with an SSL error does not stop other
subresources
> from being prefetched.
> 
> ?

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:443: // Checks that
the prefetch of png correctly loads only the png.
On 2016/10/03 18:54:17, pasko wrote:
> is it essential to check different img types? Are we in fact testing that only
> the png is prefetched?
> 
> If prefetch also had pulled the favicon? Would this test have noticed this?

We do pull the favicon on prefetch, and we could check for that. Should I? None
of the prerender tests were concerned.

These are just copied from the prerender browsertests. It's probably worth
having one test were we try a prefetch on a non-html main resource, but I agree
it may be a little excessive to try many different formats, unless we know
there's some different code path in the preload scanner somehow.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:465: // Prefetch
resource are blocked, but the prerender is not killed in any
On 2016/10/03 18:54:17, pasko wrote:
> s/resource/resources/

Done.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:467: //
TODO(mattcary): do we care about detecting if the main resource is fetched
On 2016/10/03 18:54:17, pasko wrote:
> We should not prefetch the main resource to cache if it is blocked by SB. How
> does regular Prerender solve this problem?

The prerender terminates with FINAL_STATUS_SAFE_BROWSING. For us, however,
because the main resource fetch doesn't ever come back we just terminate. We
should probably investigate dying in a more controlled way, this should probably
be grouped under the general category of prefetch dying gracefully that has come
up a couple more times above (nonexisting pages, multiple prefetches, etc).

The TODO about testing for the main resource not being fetched is a test-harness
question: it's hard to confirm that a fetch does not happen. I can do it in some
instances, where some resources are being fetched and others are not, because,
for example, we can arrange for the second of two subresources to be fetched
while the first is not, and then we're confident that if we only see the second
fetch, the first one didn't happen (although even that is making assumptions
about serialization in the network stack that may not be valid).

In this case, when nothing at all is fetched, it's tricky to see what to do. In
particular, the implementation of the RequestCounter can't pump the message loop
(because there might not be any messages), but that means that the test could
terminate before we saw an erroneous fetch.

So, in summary, I think it would be a good idea to improve the test framework to
handle this sort of thing, but the amount of work makes me think we should start
performance testing first.

mattcary

FYI, I'm in process changing the timing stuff from injection to test methods.

4 years, 2 months ago (2016-10-04 08:42:42 UTC) #34

pasko

On 2016/10/04 08:42:42, mattcary wrote: > FYI, I'm in process changing the timing stuff from ...

4 years, 2 months ago (2016-10-04 11:50:35 UTC) #35

pasko

just a few more responses to comment-related questions, otherwise looks beautiful! injection thing is the ...

4 years, 2 months ago (2016-10-04 11:52:22 UTC) #36

just a few more responses to comment-related questions, otherwise looks
beautiful!

injection thing is the only remaining part to be done here, as far as I can
tell.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:72: // etc) are
fussy about relative versus absolute paths. With the exception of
On 2016/10/04 08:25:12, mattcary wrote:
> On 2016/10/03 18:54:17, pasko wrote:
> > I cannot see why the first sentence in this comment is useful. It may get
out
> of
> > date because it is not easy to confirm that it is still true. Maybe remove
it
> > and slightly rephrase the second sentence?
> 
> Why don't you give me the exact wording you want--that will be faster.

.. because it is more to write? The explanation of why I want it this way is
still needed, I think. Plus I did not have much time before leaving in the
evening, sorry about that.

Here you are:
\begin{replacement comment}
// These URLs are relative with the exception of |PrefetchLoaderPath|, which is
// only used in |PrerenderTestURLImpl()|.
\end{replacement comment}

This is to replace the whole comment from line 71 to line 74, both boundaries
included.

I also formatted the text to fit 80 char columns o save you from pressing a
complicated Emacs keystroke. Hopefully now it is clearer and hence super fast.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:256:
histogram_tester().ExpectTotalCount("Prerender.PerceivedPLT", 1);
On 2016/10/04 08:25:12, mattcary wrote:
> On 2016/10/03 18:54:17, pasko wrote:
> > would it be possible to check the Prefetch histograms here instead of the
pure
> > Prerender ones? Is it A TODO?
> > 
> > PLT is being phased out, so let's try not to add dependencies on
PerceivedPLT
> as
> > well.
> 
> There are PLT histograms everywhere, let's do that in a different CL. When
this
> CL was started we didn't have any of the new histograms in.

I was not aware that this requires new histograms. If there are indeed no
prefetch histograms to check this, then let's have PerceivedPLT for now. I
already added a task for removing this dependency.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:262: // Checks that
we support simultaneous prefetch.
On 2016/10/04 08:25:12, mattcary wrote:
> On 2016/10/03 18:54:17, pasko wrote:
> > I did not realize that we allow it. Sounds like a bunch of link
rel=prerenders
> > can evict whole lots of tabs from memory and bring the browser to its knees.
> > Should he have an item to fix it?
> 
> Yup, probably.... I feel like there are several tests here that document
current
> behavior that we will want to change.

OK, added this as our future task. Could be a TODO, but those are harder to
track.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:292: // what we
want?
On 2016/10/04 08:25:12, mattcary wrote:
> On 2016/10/03 18:54:17, pasko wrote:
> > Will this leave the renderer up until it expires?
> > 
> > I think counting it is fine, but there is an implicit TODO to kill the
> renderer
> > in this case, which is not killed automatically, I assume.
> > 
> Yeah, see comment above.
> 
> > Meta-question: will these tests accumulate many renderers when run? If so,
may
> > make some other tests flaky by sitting there, occupying memory and making
OOM
> > killers happy.
> 
> As far as I understand these browser tests, we spin up a new chrome instance
for
> each test. So all processes are killed and none of the tests should interfere
> with each other.

Acknowledged.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:443: // Checks that
the prefetch of png correctly loads only the png.
On 2016/10/04 08:25:12, mattcary wrote:
> On 2016/10/03 18:54:17, pasko wrote:
> > is it essential to check different img types? Are we in fact testing that
only
> > the png is prefetched?
> > 
> > If prefetch also had pulled the favicon? Would this test have noticed this?
> 
> We do pull the favicon on prefetch, and we could check for that. Should I?
None
> of the prerender tests were concerned.
> 
> These are just copied from the prerender browsertests. It's probably worth
> having one test were we try a prefetch on a non-html main resource, but I
agree
> it may be a little excessive to try many different formats, unless we know
> there's some different code path in the preload scanner somehow.

OK, got it. This slightly excessive testing is OK with me, just wanted to know
where it is coming from because it seemed that I am missing something important.

So if we are pulling the favicon, and the test does not check all extra stuff
being pulled (I have no idea how to make a test that would check this), then we
should remove the word 'only' from the comment.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:467: //
TODO(mattcary): do we care about detecting if the main resource is fetched
On 2016/10/04 08:25:12, mattcary wrote:
> On 2016/10/03 18:54:17, pasko wrote:
> > We should not prefetch the main resource to cache if it is blocked by SB.
How
> > does regular Prerender solve this problem?
> 
> The prerender terminates with FINAL_STATUS_SAFE_BROWSING. For us, however,
> because the main resource fetch doesn't ever come back we just terminate. We
> should probably investigate dying in a more controlled way, this should
probably
> be grouped under the general category of prefetch dying gracefully that has
come
> up a couple more times above (nonexisting pages, multiple prefetches, etc).
> 
> The TODO about testing for the main resource not being fetched is a
test-harness
> question: it's hard to confirm that a fetch does not happen. I can do it in
some
> instances, where some resources are being fetched and others are not, because,
> for example, we can arrange for the second of two subresources to be fetched
> while the first is not, and then we're confident that if we only see the
second
> fetch, the first one didn't happen (although even that is making assumptions
> about serialization in the network stack that may not be valid).
> 
> In this case, when nothing at all is fetched, it's tricky to see what to do.
In
> particular, the implementation of the RequestCounter can't pump the message
loop
> (because there might not be any messages), but that means that the test could
> terminate before we saw an erroneous fetch.
> 
> So, in summary, I think it would be a good idea to improve the test framework
to
> handle this sort of thing, but the amount of work makes me think we should
start
> performance testing first.

OK. Agreed.

Later we will need some sort of way to distinguish the reasons of the failed
prefetches, but polishing performance should not be blocked on it, at least for
now.

mattcary

I just discovered that some of the tests (eg, SafeBrowsingTopLevel) may be flakey. I suspect ...

4 years, 2 months ago (2016-10-04 16:24:17 UTC) #37

I just discovered that some of the tests (eg, SafeBrowsingTopLevel) may be
flakey. I suspect there may be a race with the prerenderer getting killed after
it detaches.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right):

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:72: // etc) are
fussy about relative versus absolute paths. With the exception of
On 2016/10/04 11:52:22, pasko wrote:
> On 2016/10/04 08:25:12, mattcary wrote:
> > On 2016/10/03 18:54:17, pasko wrote:
> > > I cannot see why the first sentence in this comment is useful. It may get
> out
> > of
> > > date because it is not easy to confirm that it is still true. Maybe remove
> it
> > > and slightly rephrase the second sentence?
> > 
> > Why don't you give me the exact wording you want--that will be faster.
> 
> .. because it is more to write? The explanation of why I want it this way is
> still needed, I think. Plus I did not have much time before leaving in the
> evening, sorry about that.
> 
> Here you are:
> \begin{replacement comment}
> // These URLs are relative with the exception of |PrefetchLoaderPath|, which
is
> // only used in |PrerenderTestURLImpl()|.
> \end{replacement comment}
> 
> This is to replace the whole comment from line 71 to line 74, both boundaries
> included.
> 
> I also formatted the text to fit 80 char columns o save you from pressing a
> complicated Emacs keystroke. Hopefully now it is clearer and hence super fast.

Done, with a slight clarification to make it more clear for authors of future
tests.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:256:
histogram_tester().ExpectTotalCount("Prerender.PerceivedPLT", 1);
On 2016/10/04 11:52:22, pasko wrote:
> On 2016/10/04 08:25:12, mattcary wrote:
> > On 2016/10/03 18:54:17, pasko wrote:
> > > would it be possible to check the Prefetch histograms here instead of the
> pure
> > > Prerender ones? Is it A TODO?
> > > 
> > > PLT is being phased out, so let's try not to add dependencies on
> PerceivedPLT
> > as
> > > well.
> > 
> > There are PLT histograms everywhere, let's do that in a different CL. When
> this
> > CL was started we didn't have any of the new histograms in.
> 
> I was not aware that this requires new histograms. If there are indeed no
> prefetch histograms to check this, then let's have PerceivedPLT for now. I
> already added a task for removing this dependency.

Acknowledged.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:262: // Checks that
we support simultaneous prefetch.
On 2016/10/04 11:52:22, pasko wrote:
> On 2016/10/04 08:25:12, mattcary wrote:
> > On 2016/10/03 18:54:17, pasko wrote:
> > > I did not realize that we allow it. Sounds like a bunch of link
> rel=prerenders
> > > can evict whole lots of tabs from memory and bring the browser to its
knees.
> > > Should he have an item to fix it?
> > 
> > Yup, probably.... I feel like there are several tests here that document
> current
> > behavior that we will want to change.
> 
> OK, added this as our future task. Could be a TODO, but those are harder to
> track.

Acknowledged.

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:443: // Checks that
the prefetch of png correctly loads only the png.
On 2016/10/04 11:52:22, pasko wrote:
> On 2016/10/04 08:25:12, mattcary wrote:
> > On 2016/10/03 18:54:17, pasko wrote:
> > > is it essential to check different img types? Are we in fact testing that
> only
> > > the png is prefetched?
> > > 
> > > If prefetch also had pulled the favicon? Would this test have noticed
this?
> > 
> > We do pull the favicon on prefetch, and we could check for that. Should I?
> None
> > of the prerender tests were concerned.
> > 
> > These are just copied from the prerender browsertests. It's probably worth
> > having one test were we try a prefetch on a non-html main resource, but I
> agree
> > it may be a little excessive to try many different formats, unless we know
> > there's some different code path in the preload scanner somehow.
> 
> OK, got it. This slightly excessive testing is OK with me, just wanted to know
> where it is coming from because it seemed that I am missing something
important.
> 
> So if we are pulling the favicon, and the test does not check all extra stuff
> being pulled (I have no idea how to make a test that would check this), then
we
> should remove the word 'only' from the comment.

Oh, I see what you mean. Done

https://codereview.chromium.org/2304953002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_test.cc:467: //
TODO(mattcary): do we care about detecting if the main resource is fetched
On 2016/10/04 11:52:22, pasko wrote:
> On 2016/10/04 08:25:12, mattcary wrote:
> > On 2016/10/03 18:54:17, pasko wrote:
> > > We should not prefetch the main resource to cache if it is blocked by SB.
> How
> > > does regular Prerender solve this problem?
> > 
> > The prerender terminates with FINAL_STATUS_SAFE_BROWSING. For us, however,
> > because the main resource fetch doesn't ever come back we just terminate. We
> > should probably investigate dying in a more controlled way, this should
> probably
> > be grouped under the general category of prefetch dying gracefully that has
> come
> > up a couple more times above (nonexisting pages, multiple prefetches, etc).
> > 
> > The TODO about testing for the main resource not being fetched is a
> test-harness
> > question: it's hard to confirm that a fetch does not happen. I can do it in
> some
> > instances, where some resources are being fetched and others are not,
because,
> > for example, we can arrange for the second of two subresources to be fetched
> > while the first is not, and then we're confident that if we only see the
> second
> > fetch, the first one didn't happen (although even that is making assumptions
> > about serialization in the network stack that may not be valid).
> > 
> > In this case, when nothing at all is fetched, it's tricky to see what to do.
> In
> > particular, the implementation of the RequestCounter can't pump the message
> loop
> > (because there might not be any messages), but that means that the test
could
> > terminate before we saw an erroneous fetch.
> > 
> > So, in summary, I think it would be a good idea to improve the test
framework
> to
> > handle this sort of thing, but the amount of work makes me think we should
> start
> > performance testing first.
> 
> OK. Agreed.
> 
> Later we will need some sort of way to distinguish the reasons of the failed
> prefetches, but polishing performance should not be blocked on it, at least
for
> now.

Done.

mattcary

So I unflaked the test, but at the cost of it not actually testing anything ...

4 years, 2 months ago (2016-10-04 16:37:15 UTC) #38

mattcary

On 2016/10/04 17:33:47, pasko wrote: > is it ready for taking another look? Yes. If ...

4 years, 2 months ago (2016-10-05 06:37:50 UTC) #40

pasko

On 2016/10/05 06:37:50, mattcary wrote: > On 2016/10/04 17:33:47, pasko wrote: > > is it ...

4 years, 2 months ago (2016-10-05 12:34:06 UTC) #41

pasko

asking to share more code for the time override, plus a few nits, otherwise looks ...

4 years, 2 months ago (2016-10-05 12:35:25 UTC) #42

mattcary

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_manager.cc File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_manager.cc#newcode1089 chrome/browser/prerender/prerender_manager.cc:1089: time_override_.reset(override.release()); On 2016/10/05 12:35:24, pasko wrote: > why not ...

4 years, 2 months ago (2016-10-10 11:54:43 UTC) #43

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:1089:
time_override_.reset(override.release());
On 2016/10/05 12:35:24, pasko wrote:
> why not std::move?

Sure, why not? Nothing like an R-value reference to make things more clear.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:110: if
(delta_.is_zero()) {
On 2016/10/05 12:35:25, pasko wrote:
> could this avoid storing delta_? In the c-tor it could initialize to
> base::Time::Now() and then just adds delta on AdvanceTime().
> 
> This code seems to cover the case of returning time naturally when
AdvanceTime()
> was not called. Is it required? In all prior tests initializing a time
override
> declared that time is managed in the test and it worked well.

This seemed the easier way to manipulate both time_ticks_ and time_, especially
if more complex time management is added in the future (as TimeTicks and Time
are mutually opaque).

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:133: void
SetUpOnMainThread() override {
On 2016/10/05 12:35:24, pasko wrote:
> I think we would need to call the parent's SetUpOnMainThread() here.
> 
> Sounds like it would be simpler to do this work in the constructor and just
not
> override this. The text fixture is re-created for every test anyway.

? PrerenderInProcessBrowserTest is the parent of this class?

GetPrerenderManager() is null in the constructor, it's not initialized until
after the browser is created.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154:
BrowserTestTime* GetTimeOverride() const { return browser_test_time_; }
On 2016/10/05 12:35:25, pasko wrote:
> please make it private or explain why this method should be public

It's used by the test subclasses, like all the other public methods (eg,
CountRequestFor). Those don't have comments. I suppose there's a bunch of
methods in general that could be made protected rather than public, but that
breaks with existing convention for tests. I'm trying to maintain consistency.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:102: class UnittestTime : public
TimeOverride {
On 2016/10/05 12:35:25, pasko wrote:
> Compared to the other TimeOverride this one only adds SetTime() which I
believe
> will be shorter and more concise if implemented in the same class and the rest
> of the code would hence avoid duplication.
> 
> Alternative to the design in this change would be to add the time override
only
> in tests that need it, and there the initial value of tests could be taken
from
> base::Time::Now() for most of the tests and for the special case of
> PrerenderSilence* it would take the time from string.
> 
> I think that would be simpler overall.

I think the use of time in prerender_browsertest.cc is confusing, because Time
and TimeTicks are treated differently: TimeTicks are always based on Now(),
whereas Time can be set to an arbitrary date. In that latter case, TimeTicks are
out of sync with the Time, which seems dangerous to me.

So I didn't want to use that same behavior in the new tests. I also don't want
to do a big change with these old tests.

Finally, since these time override classes are so short, I think it's much more
readable and maintainable to have two different classes, rather than some
inheritance which makes it difficult to read what exactly the class does by
spreading it out into two different locations.

pasko

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode110 chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:110: if (delta_.is_zero()) { On 2016/10/10 11:54:43, mattcary wrote: > ...

4 years, 2 months ago (2016-10-10 12:40:20 UTC) #44

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:110: if
(delta_.is_zero()) {
On 2016/10/10 11:54:43, mattcary wrote:
> On 2016/10/05 12:35:25, pasko wrote:
> > could this avoid storing delta_? In the c-tor it could initialize to
> > base::Time::Now() and then just adds delta on AdvanceTime().
> > 
> > This code seems to cover the case of returning time naturally when
> AdvanceTime()
> > was not called. Is it required? In all prior tests initializing a time
> override
> > declared that time is managed in the test and it worked well.
> 
> This seemed the easier way to manipulate both time_ticks_ and time_,
especially
> if more complex time management is added in the future (as TimeTicks and Time
> are mutually opaque).

I don't think TimeTicks and Time will ever need to be updated in sync. If some
code assumes that Time goes at the same rate as TimeTicks, then it is probably
buggy, no? And if there is a need, a test could manage both Time and TimeTicks
on the test level, no need to have subtleties in utility classes.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:133: void
SetUpOnMainThread() override {
On 2016/10/10 11:54:43, mattcary wrote:
> On 2016/10/05 12:35:24, pasko wrote:
> > I think we would need to call the parent's SetUpOnMainThread() here.
> > 
> > Sounds like it would be simpler to do this work in the constructor and just
> not
> > override this. The text fixture is re-created for every test anyway.
> 
> ? PrerenderInProcessBrowserTest is the parent of this class?

Yes.

> GetPrerenderManager() is null in the constructor, it's not initialized until
> after the browser is created.

Yes.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154:
BrowserTestTime* GetTimeOverride() const { return browser_test_time_; }
On 2016/10/10 11:54:43, mattcary wrote:
> On 2016/10/05 12:35:25, pasko wrote:
> > please make it private or explain why this method should be public
> 
> It's used by the test subclasses, like all the other public methods (eg,
> CountRequestFor). Those don't have comments. I suppose there's a bunch of
> methods in general that could be made protected rather than public, but that
> breaks with existing convention for tests. I'm trying to maintain consistency.

I see. I was thinking about my proposal where time overrides are created in each
test that needs them, in this case there would be no need in GetTimeOverride().

I would still ask for least possible visibility for all the new code. Makes
easier to reason about reachability of various members in various places, even
if the old code is less careful. Not sure this kind of consistency buys us
anything material.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:102: class UnittestTime : public
TimeOverride {
On 2016/10/10 11:54:43, mattcary wrote:
> On 2016/10/05 12:35:25, pasko wrote:
> > Compared to the other TimeOverride this one only adds SetTime() which I
> believe
> > will be shorter and more concise if implemented in the same class and the
rest
> > of the code would hence avoid duplication.
> > 
> > Alternative to the design in this change would be to add the time override
> only
> > in tests that need it, and there the initial value of tests could be taken
> from
> > base::Time::Now() for most of the tests and for the special case of
> > PrerenderSilence* it would take the time from string.
> > 
> > I think that would be simpler overall.
> 
> I think the use of time in prerender_browsertest.cc is confusing, because Time
> and TimeTicks are treated differently: TimeTicks are always based on Now(),
> whereas Time can be set to an arbitrary date. In that latter case, TimeTicks
are
> out of sync with the Time, which seems dangerous to me.

That does not look dangerous to me. TimeTicks and Time should have discrepancies
by design. Compare on POSIX:
1. TimeTicks::Now(): https://linux.die.net/man/3/clock_gettime
2. Time::Now(): http://man7.org/linux/man-pages/man2/gettimeofday.2.html

Note that nothing stops the wall clock (i.e. (2)) to jump back in time, and it
does. When getting out of hibernation, Time jumps much more than TimeTicks.
TimeTicks is a special animal, and had interesting bugs on some old AMD CPUs for
not synchronizing clock ticks between cores. Different API, different
assumptions.

> So I didn't want to use that same behavior in the new tests. I also don't want
> to do a big change with these old tests.
> 
> Finally, since these time override classes are so short, I think it's much
more
> readable and maintainable to have two different classes, rather than some
> inheritance which makes it difficult to read what exactly the class does by
> spreading it out into two different locations.

I disagree. Remembering which type of time override is which requires extra
mental load when reading the code and jumping to definitions, the names do not
logically explain the core of the difference.

mattcary

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode110 chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:110: if (delta_.is_zero()) { On 2016/10/10 12:40:20, pasko wrote: > ...

4 years, 2 months ago (2016-10-10 13:56:08 UTC) #45

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:110: if
(delta_.is_zero()) {
On 2016/10/10 12:40:20, pasko wrote:
> On 2016/10/10 11:54:43, mattcary wrote:
> > On 2016/10/05 12:35:25, pasko wrote:
> > > could this avoid storing delta_? In the c-tor it could initialize to
> > > base::Time::Now() and then just adds delta on AdvanceTime().
> > > 
> > > This code seems to cover the case of returning time naturally when
> > AdvanceTime()
> > > was not called. Is it required? In all prior tests initializing a time
> > override
> > > declared that time is managed in the test and it worked well.
> > 
> > This seemed the easier way to manipulate both time_ticks_ and time_,
> especially
> > if more complex time management is added in the future (as TimeTicks and
Time
> > are mutually opaque).
> 
> I don't think TimeTicks and Time will ever need to be updated in sync. If some
> code assumes that Time goes at the same rate as TimeTicks, then it is probably
> buggy, no? And if there is a need, a test could manage both Time and TimeTicks
> on the test level, no need to have subtleties in utility classes.

Also discussed offline w/rt SimpleTestClock changes.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:133: void
SetUpOnMainThread() override {
On 2016/10/10 12:40:20, pasko wrote:
> On 2016/10/10 11:54:43, mattcary wrote:
> > On 2016/10/05 12:35:24, pasko wrote:
> > > I think we would need to call the parent's SetUpOnMainThread() here.
> > > 
> > > Sounds like it would be simpler to do this work in the constructor and
just
> > not
> > > override this. The text fixture is re-created for every test anyway.
> > 
> > ? PrerenderInProcessBrowserTest is the parent of this class?
> 
> Yes.
> 
> > GetPrerenderManager() is null in the constructor, it's not initialized until
> > after the browser is created.
> 
> Yes.

Acknowledged.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154:
BrowserTestTime* GetTimeOverride() const { return browser_test_time_; }
On 2016/10/10 12:40:20, pasko wrote:
> On 2016/10/10 11:54:43, mattcary wrote:
> > On 2016/10/05 12:35:25, pasko wrote:
> > > please make it private or explain why this method should be public
> > 
> > It's used by the test subclasses, like all the other public methods (eg,
> > CountRequestFor). Those don't have comments. I suppose there's a bunch of
> > methods in general that could be made protected rather than public, but that
> > breaks with existing convention for tests. I'm trying to maintain
consistency.
> 
> I see. I was thinking about my proposal where time overrides are created in
each
> test that needs them, in this case there would be no need in
GetTimeOverride().
> 
> I would still ask for least possible visibility for all the new code. Makes
> easier to reason about reachability of various members in various places, even
> if the old code is less careful. Not sure this kind of consistency buys us
> anything material.

Looking through overrides of InProcessBrowserTest, methods that are used in
tests are consistently public (throughout the codebase as well as in the
prerender tests in particular). Why should we have a different convention here?
It just makes the review process longer to nit unnecessarily.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:102: class UnittestTime : public
TimeOverride {
On 2016/10/10 12:40:20, pasko wrote:
> On 2016/10/10 11:54:43, mattcary wrote:
> > On 2016/10/05 12:35:25, pasko wrote:
> > > Compared to the other TimeOverride this one only adds SetTime() which I
> > believe
> > > will be shorter and more concise if implemented in the same class and the
> rest
> > > of the code would hence avoid duplication.
> > > 
> > > Alternative to the design in this change would be to add the time override
> > only
> > > in tests that need it, and there the initial value of tests could be taken
> > from
> > > base::Time::Now() for most of the tests and for the special case of
> > > PrerenderSilence* it would take the time from string.
> > > 
> > > I think that would be simpler overall.
> > 
> > I think the use of time in prerender_browsertest.cc is confusing, because
Time
> > and TimeTicks are treated differently: TimeTicks are always based on Now(),
> > whereas Time can be set to an arbitrary date. In that latter case, TimeTicks
> are
> > out of sync with the Time, which seems dangerous to me.
> 
> That does not look dangerous to me. TimeTicks and Time should have
discrepancies
> by design. Compare on POSIX:
> 1. TimeTicks::Now(): https://linux.die.net/man/3/clock_gettime
> 2. Time::Now(): http://man7.org/linux/man-pages/man2/gettimeofday.2.html
> 
> Note that nothing stops the wall clock (i.e. (2)) to jump back in time, and it
> does. When getting out of hibernation, Time jumps much more than TimeTicks.
> TimeTicks is a special animal, and had interesting bugs on some old AMD CPUs
for
> not synchronizing clock ticks between cores. Different API, different
> assumptions.
> 
> > So I didn't want to use that same behavior in the new tests. I also don't
want
> > to do a big change with these old tests.
> > 
> > Finally, since these time override classes are so short, I think it's much
> more
> > readable and maintainable to have two different classes, rather than some
> > inheritance which makes it difficult to read what exactly the class does by
> > spreading it out into two different locations.
> 
> I disagree. Remembering which type of time override is which requires extra
> mental load when reading the code and jumping to definitions, the names do not
> logically explain the core of the difference.

Discussed offline, added todo & task to use SimpleTestClock etc.

pasko

lgtm with a few nits https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode133 chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:133: void SetUpOnMainThread() override { ...

4 years, 2 months ago (2016-10-11 13:56:23 UTC) #46

lgtm with a few nits

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:133: void
SetUpOnMainThread() override {
On 2016/10/10 13:56:07, mattcary wrote:
> On 2016/10/10 12:40:20, pasko wrote:
> > On 2016/10/10 11:54:43, mattcary wrote:
> > > On 2016/10/05 12:35:24, pasko wrote:
> > > > I think we would need to call the parent's SetUpOnMainThread() here.
> > > > 
> > > > Sounds like it would be simpler to do this work in the constructor and
> just
> > > not
> > > > override this. The text fixture is re-created for every test anyway.
> > > 
> > > ? PrerenderInProcessBrowserTest is the parent of this class?

Pardon, I don't know how, but I missed the fact that this method is called here.
The remainder of the comment related to desireability of manage the time
override here vs. the actual tests.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154:
BrowserTestTime* GetTimeOverride() const { return browser_test_time_; }
On 2016/10/10 13:56:07, mattcary wrote:
> On 2016/10/10 12:40:20, pasko wrote:
> > On 2016/10/10 11:54:43, mattcary wrote:
> > > On 2016/10/05 12:35:25, pasko wrote:
> > > > please make it private or explain why this method should be public
> > > 
> > > It's used by the test subclasses, like all the other public methods (eg,
> > > CountRequestFor). Those don't have comments. I suppose there's a bunch of
> > > methods in general that could be made protected rather than public, but
that
> > > breaks with existing convention for tests. I'm trying to maintain
> consistency.
> > 
> > I see. I was thinking about my proposal where time overrides are created in
> each
> > test that needs them, in this case there would be no need in
> GetTimeOverride().
> > 
> > I would still ask for least possible visibility for all the new code. Makes
> > easier to reason about reachability of various members in various places,
even
> > if the old code is less careful. Not sure this kind of consistency buys us
> > anything material.
> 
> Looking through overrides of InProcessBrowserTest, methods that are used in
> tests are consistently public (throughout the codebase as well as in the
> prerender tests in particular). Why should we have a different convention
here?
> It just makes the review process longer to nit unnecessarily.

I am not sure we are talking about the same thing. I looked at a few
not-so-random in process browser test classes, found one that makes things
public and three that keep everything under protected:
CloudPolicyManagerTest, PhishingClassifierTest, ImageFetcherImplBrowserTest

what are you referring to as consistently public throughout the codebase?

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:190: PrerenderContents* contents)
{}
nit: empty bodies would be more readable if they stay in the
prerender_contents.h

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:306: // Returns the currently
active server. See UseHttpsSrcServer.
nit: |UseHttpsSrcServer()|

mattcary

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode133 chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:133: void SetUpOnMainThread() override { On 2016/10/11 13:56:23, pasko wrote: ...

4 years, 2 months ago (2016-10-11 15:08:58 UTC) #47

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:133: void
SetUpOnMainThread() override {
On 2016/10/11 13:56:23, pasko wrote:
> On 2016/10/10 13:56:07, mattcary wrote:
> > On 2016/10/10 12:40:20, pasko wrote:
> > > On 2016/10/10 11:54:43, mattcary wrote:
> > > > On 2016/10/05 12:35:24, pasko wrote:
> > > > > I think we would need to call the parent's SetUpOnMainThread() here.
> > > > > 
> > > > > Sounds like it would be simpler to do this work in the constructor and
> > just
> > > > not
> > > > > override this. The text fixture is re-created for every test anyway.
> > > > 
> > > > ? PrerenderInProcessBrowserTest is the parent of this class?
> 
> Pardon, I don't know how, but I missed the fact that this method is called
here.
> The remainder of the comment related to desireability of manage the time
> override here vs. the actual tests.

Right, it can't be done in the constructor as the PrerenderManager doesn't exist
yet. We could just override it for the tests that adjust time, but that would
lead to a lot of repeated boilerplate and possible inconsistency on how time is
dealt with in tests.

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154:
BrowserTestTime* GetTimeOverride() const { return browser_test_time_; }
On 2016/10/11 13:56:23, pasko wrote:
> On 2016/10/10 13:56:07, mattcary wrote:
> > On 2016/10/10 12:40:20, pasko wrote:
> > > On 2016/10/10 11:54:43, mattcary wrote:
> > > > On 2016/10/05 12:35:25, pasko wrote:
> > > > > please make it private or explain why this method should be public
> > > > 
> > > > It's used by the test subclasses, like all the other public methods (eg,
> > > > CountRequestFor). Those don't have comments. I suppose there's a bunch
of
> > > > methods in general that could be made protected rather than public, but
> that
> > > > breaks with existing convention for tests. I'm trying to maintain
> > consistency.
> > > 
> > > I see. I was thinking about my proposal where time overrides are created
in
> > each
> > > test that needs them, in this case there would be no need in
> > GetTimeOverride().
> > > 
> > > I would still ask for least possible visibility for all the new code.
Makes
> > > easier to reason about reachability of various members in various places,
> even
> > > if the old code is less careful. Not sure this kind of consistency buys us
> > > anything material.
> > 
> > Looking through overrides of InProcessBrowserTest, methods that are used in
> > tests are consistently public (throughout the codebase as well as in the
> > prerender tests in particular). Why should we have a different convention
> here?
> > It just makes the review process longer to nit unnecessarily.
> 
> I am not sure we are talking about the same thing. I looked at a few
> not-so-random in process browser test classes, found one that makes things
> public and three that keep everything under protected:
> CloudPolicyManagerTest, PhishingClassifierTest, ImageFetcherImplBrowserTest
> 
> what are you referring to as consistently public throughout the codebase?

ErrorPageBrowserTest, UnloadTest, SdchBrowserTest... all the nontrivial
overrides when search for "InProcessBrowserTest"

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:190: PrerenderContents* contents)
{}
On 2016/10/11 13:56:23, pasko wrote:
> nit: empty bodies would be more readable if they stay in the
> prerender_contents.h

I thought we avoided inlining? At any rate, I did this to be consistent with the
existing OnPrerenderStopLoading def.

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_test_utils.h:306: // Returns the currently
active server. See UseHttpsSrcServer.
On 2016/10/11 13:56:23, pasko wrote:
> nit: |UseHttpsSrcServer()|

Done.

pasko

still lgtm, please land ASAP, all the discussion is converted to unnecessary nits :) https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc ...

4 years, 2 months ago (2016-10-11 15:40:19 UTC) #48

still lgtm, please land ASAP, all the discussion is converted to unnecessary
nits :)

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154:
BrowserTestTime* GetTimeOverride() const { return browser_test_time_; }
On 2016/10/11 15:08:58, mattcary wrote:
> On 2016/10/11 13:56:23, pasko wrote:
> > On 2016/10/10 13:56:07, mattcary wrote:
> > > On 2016/10/10 12:40:20, pasko wrote:
> > > > On 2016/10/10 11:54:43, mattcary wrote:
> > > > > On 2016/10/05 12:35:25, pasko wrote:
> > > > > > please make it private or explain why this method should be public
> > > > > 
> > > > > It's used by the test subclasses, like all the other public methods
(eg,
> > > > > CountRequestFor). Those don't have comments. I suppose there's a bunch
> of
> > > > > methods in general that could be made protected rather than public,
but
> > that
> > > > > breaks with existing convention for tests. I'm trying to maintain
> > > consistency.
> > > > 
> > > > I see. I was thinking about my proposal where time overrides are created
> in
> > > each
> > > > test that needs them, in this case there would be no need in
> > > GetTimeOverride().
> > > > 
> > > > I would still ask for least possible visibility for all the new code.
> Makes
> > > > easier to reason about reachability of various members in various
places,
> > even
> > > > if the old code is less careful. Not sure this kind of consistency buys
us
> > > > anything material.
> > > 
> > > Looking through overrides of InProcessBrowserTest, methods that are used
in
> > > tests are consistently public (throughout the codebase as well as in the
> > > prerender tests in particular). Why should we have a different convention
> > here?
> > > It just makes the review process longer to nit unnecessarily.
> > 
> > I am not sure we are talking about the same thing. I looked at a few
> > not-so-random in process browser test classes, found one that makes things
> > public and three that keep everything under protected:
> > CloudPolicyManagerTest, PhishingClassifierTest, ImageFetcherImplBrowserTest
> > 
> > what are you referring to as consistently public throughout the codebase?
> 
> ErrorPageBrowserTest, UnloadTest, SdchBrowserTest... all the nontrivial
> overrides when search for "InProcessBrowserTest"

SdchBrowserTest::SetUpCommandLine(...) is non-trivial and private.

Others: hm, indeed

So, given there are examples of both, there is really no consistency to
maintain. Knowing that the classes use the principle of least privilege makes it
easier to maintain. One possible disadvantage is that it is an extra mental load
to think what visibility each thing should be in, which does not seem applicable
here because some stuff is thrown to private nontrivially anyway.

Sounds like a nit though, feel free to leave it as is, taking too much time to
argue.

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:190: PrerenderContents* contents)
{}
On 2016/10/11 15:08:58, mattcary wrote:
> On 2016/10/11 13:56:23, pasko wrote:
> > nit: empty bodies would be more readable if they stay in the
> > prerender_contents.h
> 
> I thought we avoided inlining? At any rate, I did this to be consistent with
the
> existing OnPrerenderStopLoading def.
 
We certainly inline trivial getters, so empty bodies seem natural to inline.

We do both, actually, but your pattern is less common (not sure how much):
shell> git grep 'virtual void.*On.*\(.*\) {}' -- '*.h' | wc -l
553
shell> git grep 'virtual void.*On.*\(.*\);' -- '*.h' | grep -v '= 0' | wc -l
364

I agree that it is good to be consistent here, but the other consistent way to
do it looks more readable/maintainable because:
1. the reader knows that all non-overridden methods are no-ops by just looking
at the interface of this base class
2. less boilerplate-placed-in-correct-namespace to scan through in the .cc file

mattcary

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode154 chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154: BrowserTestTime* GetTimeOverride() const { return browser_test_time_; } On 2016/10/11 ...

4 years, 2 months ago (2016-10-12 09:02:52 UTC) #49

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2304953002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:154:
BrowserTestTime* GetTimeOverride() const { return browser_test_time_; }
On 2016/10/11 15:40:19, pasko wrote:
> On 2016/10/11 15:08:58, mattcary wrote:
> > On 2016/10/11 13:56:23, pasko wrote:
> > > On 2016/10/10 13:56:07, mattcary wrote:
> > > > On 2016/10/10 12:40:20, pasko wrote:
> > > > > On 2016/10/10 11:54:43, mattcary wrote:
> > > > > > On 2016/10/05 12:35:25, pasko wrote:
> > > > > > > please make it private or explain why this method should be public
> > > > > > 
> > > > > > It's used by the test subclasses, like all the other public methods
> (eg,
> > > > > > CountRequestFor). Those don't have comments. I suppose there's a
bunch
> > of
> > > > > > methods in general that could be made protected rather than public,
> but
> > > that
> > > > > > breaks with existing convention for tests. I'm trying to maintain
> > > > consistency.
> > > > > 
> > > > > I see. I was thinking about my proposal where time overrides are
created
> > in
> > > > each
> > > > > test that needs them, in this case there would be no need in
> > > > GetTimeOverride().
> > > > > 
> > > > > I would still ask for least possible visibility for all the new code.
> > Makes
> > > > > easier to reason about reachability of various members in various
> places,
> > > even
> > > > > if the old code is less careful. Not sure this kind of consistency
buys
> us
> > > > > anything material.
> > > > 
> > > > Looking through overrides of InProcessBrowserTest, methods that are used
> in
> > > > tests are consistently public (throughout the codebase as well as in the
> > > > prerender tests in particular). Why should we have a different
convention
> > > here?
> > > > It just makes the review process longer to nit unnecessarily.
> > > 
> > > I am not sure we are talking about the same thing. I looked at a few
> > > not-so-random in process browser test classes, found one that makes things
> > > public and three that keep everything under protected:
> > > CloudPolicyManagerTest, PhishingClassifierTest,
ImageFetcherImplBrowserTest
> > > 
> > > what are you referring to as consistently public throughout the codebase?
> > 
> > ErrorPageBrowserTest, UnloadTest, SdchBrowserTest... all the nontrivial
> > overrides when search for "InProcessBrowserTest"
> 
> SdchBrowserTest::SetUpCommandLine(...) is non-trivial and private.
> 
> Others: hm, indeed
> 
> So, given there are examples of both, there is really no consistency to
> maintain. Knowing that the classes use the principle of least privilege makes
it
> easier to maintain. One possible disadvantage is that it is an extra mental
load
> to think what visibility each thing should be in, which does not seem
applicable
> here because some stuff is thrown to private nontrivially anyway.
> 
> Sounds like a nit though, feel free to leave it as is, taking too much time to
> argue.

Acknowledged.

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2304953002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:190: PrerenderContents* contents)
{}
On 2016/10/11 15:40:19, pasko wrote:
> On 2016/10/11 15:08:58, mattcary wrote:
> > On 2016/10/11 13:56:23, pasko wrote:
> > > nit: empty bodies would be more readable if they stay in the
> > > prerender_contents.h
> > 
> > I thought we avoided inlining? At any rate, I did this to be consistent with
> the
> > existing OnPrerenderStopLoading def.
>  
> We certainly inline trivial getters, so empty bodies seem natural to inline.
> 
> We do both, actually, but your pattern is less common (not sure how much):
> shell> git grep 'virtual void.*On.*\(.*\) {}' -- '*.h' | wc -l
> 553
> shell> git grep 'virtual void.*On.*\(.*\);' -- '*.h' | grep -v '= 0' | wc -l
> 364
> 
> I agree that it is good to be consistent here, but the other consistent way to
> do it looks more readable/maintainable because:
> 1. the reader knows that all non-overridden methods are no-ops by just looking
> at the interface of this base class
> 2. less boilerplate-placed-in-correct-namespace to scan through in the .cc
file

I like the inlining for empty ftns better myself, done.

mattcary

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to ...

4 years, 2 months ago (2016-10-12 09:16:00 UTC) #51

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 09:16:13 UTC) #52

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 10:02:12 UTC) #53

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/294095)

4 years, 2 months ago (2016-10-12 10:02:13 UTC) #54

mattcary

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

4 years, 2 months ago (2016-10-12 13:22:47 UTC) #55

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 13:23:02 UTC) #56

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 13:56:35 UTC) #57

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/101544)

4 years, 2 months ago (2016-10-12 13:56:37 UTC) #58

mattcary

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to ...

4 years, 2 months ago (2016-10-12 14:42:27 UTC) #60

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 14:42:41 UTC) #61

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 15:18:27 UTC) #62

commit-bot: I haz the power

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_rel_ng/builds/314581)

4 years, 2 months ago (2016-10-12 15:18:30 UTC) #63

mattcary

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to ...

4 years, 2 months ago (2016-10-12 15:40:26 UTC) #65

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 15:40:40 UTC) #66

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-12 16:10:47 UTC) #67

commit-bot: I haz the power

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_ng/builds/313155)

4 years, 2 months ago (2016-10-12 16:10:49 UTC) #68

mattcary

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to ...

4 years, 2 months ago (2016-10-13 07:50:27 UTC) #70

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-13 07:50:45 UTC) #71

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-13 08:23:51 UTC) #72

commit-bot: I haz the power

Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/297160)

4 years, 2 months ago (2016-10-13 08:23:53 UTC) #73

mattcary

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to ...

4 years, 2 months ago (2016-10-13 08:56:50 UTC) #75

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-13 08:57:06 UTC) #76

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-13 09:25:53 UTC) #77

commit-bot: I haz the power

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_ng/builds/313864)

4 years, 2 months ago (2016-10-13 09:25:56 UTC) #78

mattcary

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to ...

4 years, 2 months ago (2016-10-13 09:31:04 UTC) #80

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-13 09:31:26 UTC) #81

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-13 10:01:43 UTC) #82

commit-bot: I haz the power

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_rel_ng/builds/315319)

4 years, 2 months ago (2016-10-13 10:01:45 UTC) #83

mattcary

https://codereview.chromium.org/2304953002/diff/500001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/500001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode178 chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:178: base::RunLoop loop; +pasko Is this the right way to ...

4 years, 2 months ago (2016-10-13 13:40:39 UTC) #84

pasko

https://codereview.chromium.org/2304953002/diff/500001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2304953002/diff/500001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode178 chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:178: base::RunLoop loop; On 2016/10/13 13:40:38, mattcary wrote: > +pasko ...

4 years, 2 months ago (2016-10-13 14:00:45 UTC) #85

mattcary

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

4 years, 2 months ago (2016-10-14 08:36:26 UTC) #86

mattcary

On 2016/10/13 14:00:45, pasko wrote: > https://codereview.chromium.org/2304953002/diff/500001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc > File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): > > https://codereview.chromium.org/2304953002/diff/500001/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc#newcode178 > ...

4 years, 2 months ago (2016-10-14 08:36:31 UTC) #87

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 08:36:36 UTC) #88

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 09:23:23 UTC) #89

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 09:23:24 UTC) #90

mattcary

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

4 years, 2 months ago (2016-10-14 09:55:45 UTC) #91

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 09:56:06 UTC) #92

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 11:18:29 UTC) #93

commit-bot: I haz the power

Dry run: Exceeded global retry quota

4 years, 2 months ago (2016-10-14 11:18:30 UTC) #94

mattcary

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

4 years, 2 months ago (2016-10-14 11:50:23 UTC) #95

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 11:50:40 UTC) #96

pasko

https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h File chrome/browser/prerender/prerender_final_status.h (right): https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h#newcode70 chrome/browser/prerender/prerender_final_status.h:70: FINAL_STATUS_TESTING_IGNORE = 55, It is unfortunate that we need ...

4 years, 2 months ago (2016-10-14 12:49:14 UTC) #97

mattcary

https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h File chrome/browser/prerender/prerender_final_status.h (right): https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h#newcode70 chrome/browser/prerender/prerender_final_status.h:70: FINAL_STATUS_TESTING_IGNORE = 55, On 2016/10/14 12:49:13, pasko wrote: > ...

4 years, 2 months ago (2016-10-14 12:54:45 UTC) #98

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 13:25:53 UTC) #99

commit-bot: I haz the power

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_rel_ng/builds/316153)

4 years, 2 months ago (2016-10-14 13:25:55 UTC) #100

pasko

https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h File chrome/browser/prerender/prerender_final_status.h (right): https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h#newcode70 chrome/browser/prerender/prerender_final_status.h:70: FINAL_STATUS_TESTING_IGNORE = 55, On 2016/10/14 12:54:45, mattcary wrote: > ...

4 years, 2 months ago (2016-10-14 13:28:06 UTC) #101

https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_final_status.h (right):

https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerend...
chrome/browser/prerender/prerender_final_status.h:70:
FINAL_STATUS_TESTING_IGNORE = 55,
On 2016/10/14 12:54:45, mattcary wrote:
> On 2016/10/14 12:49:13, pasko wrote:
> > It is unfortunate that we need this (not too serious, but makes test code
> harder
> > to understand). Is this because the FinalStatus can be different depending
on
> > racy conditions? Wondering whether it can be avoided, after all,
> > prereneder_browsertests managed to avoid mispredicting FinalStatus in tests.
> 
> Actually, see TestPrerenderContents::RenderProcessGone, where we see that app
> terminating or renderer crashed can both occur for prerender_browsertests.
> 
> For nostate prefetch, it's even harder, because the prerender process may or
may
> not have been killed by the time all the requests are done (or the requests
> didn't even occur, as is the case in the tests needing this).

Can you explain a little more on why is it killed? Is it flaky because we did
not allow for enough RAM in the renderer? I thought the current behavior should
keep the PrerenderContents alive until the test wraps up (and a long timeout
that we disable?).

> Hopefully your lifetime changes will plug a lot of these cases and we can
remove
> it.
> 
> I thought about setting the constant to some high value, so that we can remove
> it from the enum if it turns out to be necessary without polluting the range
(we
> know this can never occur in a histogram. Thoughts?

I am OK with a high number, yes, since reusing values in histogram-related enums
is not allowed. Also potentially augmenting, if there is still a reason to
survive-or-kill undeterministically, tiny nit: the value could be named as
PREFETCH_ALIVE_OR_GONE_FOR_TESTING to keep it targeted at a specific problem and
suggest not reusing it for anything else in the meantime.

mattcary

https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h File chrome/browser/prerender/prerender_final_status.h (right): https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h#newcode70 chrome/browser/prerender/prerender_final_status.h:70: FINAL_STATUS_TESTING_IGNORE = 55, > Can you explain a little ...

4 years, 2 months ago (2016-10-14 13:41:52 UTC) #102

mattcary

On 2016/10/14 13:41:52, mattcary wrote: > https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h > File chrome/browser/prerender/prerender_final_status.h (right): > > https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerender/prerender_final_status.h#newcode70 > ...

4 years, 2 months ago (2016-10-14 14:14:41 UTC) #103

On 2016/10/14 13:41:52, mattcary wrote:
>
https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerend...
> File chrome/browser/prerender/prerender_final_status.h (right):
> 
>
https://codereview.chromium.org/2304953002/diff/560001/chrome/browser/prerend...
> chrome/browser/prerender/prerender_final_status.h:70:
> FINAL_STATUS_TESTING_IGNORE = 55,
> > Can you explain a little more on why is it killed? Is it flaky because we
did
> > not allow for enough RAM in the renderer? I thought the current behavior
> should
> > keep the PrerenderContents alive until the test wraps up (and a long timeout
> > that we disable?).
> > 
> The race in question for the original prerender_browsertests is between
> app_terminating and renderer_crashed because, I presume, of the race between
the
> renderprocesses getting killed and the prerender manager getting destroyed.
> 
> The race that inspired my odious final status is between things like the
> renderer getting killed and safe browsing returning, or the renderer getting
> destroyed and the unsupport-scheme final status that I believe comes from the
> error page one gets redirected to when trying to load a nonexisting page.
> 
> > > Hopefully your lifetime changes will plug a lot of these cases and we can
> > remove
> > > it.
> > > 
> > > I thought about setting the constant to some high value, so that we can
> remove
> > > it from the enum if it turns out to be necessary without polluting the
range
> > (we
> > > know this can never occur in a histogram. Thoughts?
> > 
> > I am OK with a high number, yes, since reusing values in histogram-related
> enums
> > is not allowed. Also potentially augmenting, if there is still a reason to
> > survive-or-kill undeterministically, tiny nit: the value could be named as
> > PREFETCH_ALIVE_OR_GONE_FOR_TESTING to keep it targeted at a specific problem
> and
> > suggest not reusing it for anything else in the meantime.
> 
> As explained above, I don't think that value is actually an accurate
description
> of what's going on.

Discussed offline, disabling the tests instead of making a new enum value, will
be able to fix when I merge with prerenderer killing CL.

mattcary

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

4 years, 2 months ago (2016-10-14 15:34:48 UTC) #104

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 15:35:07 UTC) #105

mattcary

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to ...

4 years, 2 months ago (2016-10-17 07:25:53 UTC) #107

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-17 07:26:16 UTC) #108

commit-bot: I haz the power

Description was changed from ========== NoState Prefetch: nostate prefetch browser tests. This adds browser tests ...

4 years, 2 months ago (2016-10-17 08:36:24 UTC) #110

commit-bot: I haz the power

4 years, 2 months ago (2016-10-17 08:36:29 UTC) #111

Message was sent while issue was closed.

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

Issue 2304953002: NoState Prefetch: nostate prefetch browser tests. (Closed)

Description

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : All browser tests added and cleaned up for prefetch-specific issues. #

Patch Set 4 : comments #

Patch Set 5 : format #

Patch Set 6 : histogram tests #

Patch Set 7 : comments #

Patch Set 8 : histogram name fixes #

Patch Set 9 : comments #

Patch Set 10 : comments #

Patch Set 11 : comments #

Patch Set 12 : Unflake test #

Patch Set 13 : Remove injection at factory #

Patch Set 14 : comments/rebase #

Patch Set 15 : comment #

Patch Set 16 : comments #

Patch Set 17 : comments #

Patch Set 18 : fix windows compile error #

Patch Set 19 : another try for windows build failure #

Patch Set 20 : more tries for windows compiling #

Patch Set 21 : Getting closer to a windows build: FilePath is wchar for windows but char elsewhere. #

Patch Set 22 : Yet another windows build iteration #

Patch Set 23 : I will slay the wchar dragon #

Patch Set 24 : String literal character types #

Patch Set 25 : Try unsafe utf8 conversion rather than just typedef magic #

Patch Set 26 : reliable waiting for paints #

Patch Set 27 : Histograms are flakey to test, remove them #

Patch Set 28 : use-after-free fix #

Patch Set 29 : Unflake prerender content creation #

Patch Set 30 : Clean up tests #

Patch Set 31 : Remove final_status.cc as it had only formatting changes. #

Messages