Add PrerenderStatusEvent on Prerenders

Add PrerenderStatusEvent on Prerenders

Changes the events a bit as they are passed down, and unifies the PrerenderingSupport and PrerenderingDispatcher interfaces. Read this together with the WebKit issue https://bugs.webkit.org/show_bug.cgi?id=96474 .

Also, a few other code cleanups/assertion cleanups I came across while coding this.

This change will need to be staged carefully (the change to the WebPrerenderingSupport interface is going to require around four gardened checkins).

R=mmenke@chromium.org
BUG=None

[gavinp, 2012-09-12 07:12:45]

Timo: I banged this out on my flight today. What do you think about it?

[dominich, 2012-09-12 15:21:03]

Can you separate the renderer-side cleanup from the new event code?

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:107: // |creator_child_id| is the
id of the child process that caused the prerender
comment needs ref to prerender_id

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:260: process_id, prerender_id,
url, referrer, size,
I thought the PrerenderHandle work would remove the need for any arbitrary ids
like this. This feels like a step backwards in the API.

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:271: return
AddPrerender(ORIGIN_OMNIBOX, -1, -1, url, content::Referrer(), size,
does this mean the renderer doesn't get informed about Omnibox prerenders? Is
that an issue?

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/renderer/preren...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/renderer/preren...
chrome/renderer/prerender/prerender_dispatcher.cc:63: if (it !=
prerenders_.end()) {
this would mean that we started a prerender before WebKit told us about it. That
seems unlikely - should it be a stronger error condition?

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/renderer/preren...
chrome/renderer/prerender/prerender_dispatcher.cc:71: prerender_aliases_.insert(
are there any invariants expected here - such as the prerender hasn't yet
started?

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/test/data/prere...
File chrome/test/data/prerender/prerender_loader.html (right):

https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/test/data/prere...
chrome/test/data/prerender/prerender_loader.html:86:
prerender.addEventListener('webkitprerenderstatus', PrerenderStatusHandler,
false);
remind me - i thought we decided to do something like the XHR status instead of
individual events. Ie, it would be a single event that is status-changed and the
parameter would be integer-based. No?

[mmenke, 2012-09-12 16:31:45]

http://codereview.chromium.org/10918189/diff/1/chrome/browser/prerender/prere...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10918189/diff/1/chrome/browser/prerender/prere...
chrome/browser/prerender/prerender_manager.cc:834: origin, process_id, -1, url,
referrer, size, session_storage_namespace));
Won't this break PrerenderMsg_StoppedPrerender?  We'll glom all pending
prerenders into a group with an id of -1, send multiple start events for it
(Which won't get passed on to WebKit), and then when one of them stops, we'll
delete them all, renderer-side, but keep them browser-side, while WebKit remains
happily oblivious.

http://codereview.chromium.org/10918189/diff/1/chrome/browser/prerender/prere...
chrome/browser/prerender/prerender_manager.cc:942: return new
PrerenderHandle(preexisting_prerender_data);
When we merge prerenders, only one of them will ever get updates.

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.cc:103: extra_data.size(),
extra_data.render_view_route_id()));
So if the same prerender is added twice, then the first one is stopped, we won't
cancel the underlying prerender, and it will still be used if the second
prerender is navigated to, but we won't be sending any prerender events for it.

Is this the behavior we want?

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.cc:112: prerenders_.erase(it);
Is the find / check / erase needed?

Doesn't just "prerenders_.erase(extra_data.prerender_id());" do the trick, here
and below?

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.h:54: // from the browser
process, which prerenders have been launched.
nit:  Comments are generally capitalized (And those ending in periods should
always be capitalized).

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.h:56: std::set<int>
running_prerenders_;
|running_prerenders_| doesn't seem to serve any purpose.  Also, if there's any
need to track running prerenders apart from prerender_aliases_, I'd suggest
combining them in a single map.