Add <link rel=next> support to prerender.

Add <link rel=next> support to prerender.

IEs prerender implementation supports rel=next link elements. Let's
also parse them, and based on a field trial launch these as
prerenders.

R=davidben,tburkard,jochen,jschuh@chromium.org
BUG=329963
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250280

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

[gavinp, 2013-12-19 19:49:25]

This change depends on a blink change at
https://codereview.chromium.org/116183005/

[gavinp, 2013-12-19 19:49:57]

davidben, tburkard: Please take a look.

jochen: Can you look at chrome/common? Perhaps an OWNERS is in order, too?

[davidben, 2013-12-19 20:30:02]

LGTM.

https://codereview.chromium.org/113803003/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/113803003/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_link_manager.cc:278: 
Nit: Maybe split this off into a separate function and check each bit
individually? Something like

if (!ShouldLaunchPrerender(*i))
  continue;

bool PrerenderLinkManager::ShouldLaunchPrerender(LinkPrerender* prerender) {
  DCHECK_NE(0, prerender->rel_types);
  if (prerender->rel_types & PrerenderRelTypePrerender)
    return true;
  if (should_start_rel_next_prerenders_ &&
      (prerender->rel_types & PrerenderRelTypeNext))
    return true;
  return false;
}

The switch-case on a bitmask looks a little goofy. :-) Also this way adding a
new field doesn't have to be multisided.

https://codereview.chromium.org/113803003/diff/20001/chrome/common/prerender_...
File chrome/common/prerender_types.h (right):

https://codereview.chromium.org/113803003/diff/20001/chrome/common/prerender_...
chrome/common/prerender_types.h:14: PrerenderRelTypeNext = 0x2,
Probably worth a comment saying this should stay in sync with the set in Blink.
(Do we want some stricter mechanism? I guess the other place which does this is
the resource type, and that's a nightmare to change. *shrug*)

[gavinp, 2013-12-20 14:44:47]

Thanks David.

In changing how we tested links, I ended up checking the field trial each run.
It's not bad, since it's a std::map, and it removes a lot of subtle noise from
the unit test.

https://codereview.chromium.org/113803003/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/113803003/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_link_manager.cc:278: 
On 2013/12/19 20:30:03, David Benjamin wrote:
> Nit: Maybe split this off into a separate function and check each bit
> individually? Something like
> 
> if (!ShouldLaunchPrerender(*i))
>   continue;
> 
> bool PrerenderLinkManager::ShouldLaunchPrerender(LinkPrerender* prerender) {
>   DCHECK_NE(0, prerender->rel_types);
>   if (prerender->rel_types & PrerenderRelTypePrerender)
>     return true;
>   if (should_start_rel_next_prerenders_ &&
>       (prerender->rel_types & PrerenderRelTypeNext))
>     return true;
>   return false;
> }
> 
> The switch-case on a bitmask looks a little goofy. :-) Also this way adding a
> new field doesn't have to be multisided.

Done.

https://codereview.chromium.org/113803003/diff/20001/chrome/common/prerender_...
File chrome/common/prerender_types.h (right):

https://codereview.chromium.org/113803003/diff/20001/chrome/common/prerender_...
chrome/common/prerender_types.h:14: PrerenderRelTypeNext = 0x2,
On 2013/12/19 20:30:03, David Benjamin wrote:
> Probably worth a comment saying this should stay in sync with the set in
Blink.
> (Do we want some stricter mechanism? I guess the other place which does this
is
> the resource type, and that's a nightmare to change. *shrug*)

Done.

[davidben, 2013-12-20 16:59:56]

(Patch set 5 LGTM)

[tburkard, 2013-12-23 07:47:47]

lgtm.

Nice work, Gavin.

We should also create a new Origin type for link rel=next prerenders, so as
to track them separately in histograms. But we can always add that in a
separate CL.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gavinp, 2013-12-26 16:58:16]

On 2013/12/23 07:47:47, tburkard wrote:
> lgtm.
> 
> Nice work, Gavin.
> 
> We should also create a new Origin type for link rel=next prerenders, so as
> to track them separately in histograms. But we can always add that in a
> separate CL.

I definitely think that should be a followup CL. See
https://codereview.chromium.org/98603015/ for an existing CL that adds
histograms directly to the link manager.

The problem with adding an origin type is that a prerender can have multiple rel
types; I'll go ahead and make a call in the CL adding the new origin type, but
it's one of those annoying corner cases best talked about a bit separately.

> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[gavinp, 2013-12-26 16:59:51]

Procedural note: jochen is OOO until next week. I'll ping him then, unless
someone advocates strongly for a TBR landing (which given that it's a bit of
boilerplate, I can be easily talked into provided someone looks over the common
code).

[jochen (gone - plz use gerrit), 2014-01-03 11:08:45]

lgtm

you need a security review for the ipc messages change

https://codereview.chromium.org/113803003/diff/280001/chrome/common/prerender...
File chrome/common/prerender_types.h (right):

https://codereview.chromium.org/113803003/diff/280001/chrome/common/prerender...
chrome/common/prerender_types.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
it's 2014 now :)

[gavinp, 2014-01-06 17:53:36]

jschuh: PTAL for security review.

[gavinp, 2014-02-07 20:30:13]

jschuh: ping! PTAL?

This is now rebased to trunk, as well, there's a new series of webkit patches;
this is downstream of https://codereview.chromium.org/144863004/ , and then
downstream of chrome patch is https://codereview.chromium.org/116183005/ which
will start sending these rel types after we are safely ignoring them by default
in chrome.

[jschuh, 2014-02-08 17:35:59]

On 2014/02/07 20:30:13, gavinp wrote:
> jschuh: ping! PTAL?
> 
> This is now rebased to trunk, as well, there's a new series of webkit patches;
> this is downstream of https://codereview.chromium.org/144863004/ , and then
> downstream of chrome patch is https://codereview.chromium.org/116183005/ which
> will start sending these rel types after we are safely ignoring them by
default
> in chrome.

So, "jschuh" is not a valid account, meaning I was never added as a reviewer and
never saw this CL (until I noticed a link to it in a stale chat from you this
morning). :(

Anyway, I just added myself as a reviewer. I'm headed out the door and can't
take a look right now, but now that it's on my review queue and I'll get to it
this weekend.

[gavinp, 2014-02-10 17:37:36]

On 2014/02/08 17:35:59, Justin Schuh wrote:
> On 2014/02/07 20:30:13, gavinp wrote:
> > jschuh: ping! PTAL?
> > 
> > This is now rebased to trunk, as well, there's a new series of webkit
patches;
> > this is downstream of https://codereview.chromium.org/144863004/ , and then
> > downstream of chrome patch is https://codereview.chromium.org/116183005/
which
> > will start sending these rel types after we are safely ignoring them by
> default
> > in chrome.
> 
> So, "jschuh" is not a valid account, meaning I was never added as a reviewer
and
> never saw this CL (until I noticed a link to it in a stale chat from you this
> morning). :(
> 
> Anyway, I just added myself as a reviewer. I'm headed out the door and can't
> take a look right now, but now that it's on my review queue and I'll get to it
> this weekend.

Thanks Justin, my apolgies for adding it wrong. Weirdly, it looks like I added
jschuh@chromium.org+, which is a bad fat finger. :-(

I'm glad you saw it now.

[jschuh, 2014-02-10 18:30:12]

ipc security lgtm. added a bitmask to scope prerender triggers.

[gavinp, 2014-02-10 22:13:43]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-10 22:16:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/113803003/920001

[commit-bot: I haz the power, 2014-02-10 22:55:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/113803003/920001

[commit-bot: I haz the power, 2014-02-10 23:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/113803003/920001

[commit-bot: I haz the power, 2014-02-11 02:07:01]

Change committed as 250280

[Lei Zhang, 2014-02-11 02:44:32]

A revert of this CL has been created in
https://codereview.chromium.org/145073009/ by thestig@chromium.org.

The reason for reverting is: Linux ASAN Tests (2) run 22831 failed with leaks in
the RelNextByFieldTrial test..

[gavinp, 2014-02-11 16:53:20]

re-opening since this was reverted

[gavinp, 2014-02-11 17:40:59]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-11 17:41:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/113803003/1590001

[commit-bot: I haz the power, 2014-02-11 21:06:00]

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

[commit-bot: I haz the power, 2014-02-11 21:06:01]

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

[gavinp, 2014-02-11 22:43:11]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-11 22:48:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/113803003/1590001

[commit-bot: I haz the power, 2014-02-11 23:58:43]

Change committed as 250558