[webkit-reviews] review denied: [Bug 96474] Add PrerenderStatusEvent on <link rel=prerender> elements. : [Attachment 170230] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Gavin Peters <gavinp at chromium.org>'s
request for review:
Bug 96474: Add PrerenderStatusEvent on <link rel=prerender> elements.
https://bugs.webkit.org/show_bug.cgi?id=96474

Attachment 170230: Patch
https://bugs.webkit.org/attachment.cgi?id=170230&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=170230&action=review


This looks great.  We just need tests and a spec.

> Source/Platform/ChangeLog:8
> +	   The new PrerenderStatusEvent is sent to link elements when
prerenders are started by the embedder, and also sent when they are stopped.
Pages using this feature can now serialize launching prerenders, and track
timing performance.

Would you mind wrapping these lines to something sensible?  They're just a bit
hard to read like this.

> Source/Platform/chromium/public/WebPrerender.h:72
> +    WEBKIT_EXPORT void didSendLoadForPrerender();
> +    WEBKIT_EXPORT void didSendDOMContentLoadedForPrerender();

These are slightly odd concepts to have at the Platform layer because they're
DOM concepts, but it's for the DOM of the prerender, so I guess that makes
sense.

> Source/WebCore/dom/EventNames.h:249
> +    macro(webkitPrerenderStart) \
> +    macro(webkitPrerenderStop) \
> +    macro(webkitPrerenderLoad) \
> +    macro(webkitPrerenderDOMContentLoaded)

These should be in all lower case.  webkitRegionLayoutUpdate is wrong.

> Source/WebCore/html/HTMLLinkElement.cpp:380
> +#if ENABLE(LINK_PRERENDER)
> +void HTMLLinkElement::didStartLinkPrerender()
> +{
> +    dispatchEvent(Event::create(eventNames().webkitPrerenderStartEvent,
false, false));
> +}
> +
> +void HTMLLinkElement::didStopLinkPrerender()
> +{
> +    dispatchEvent(Event::create(eventNames().webkitPrerenderStopEvent,
false, false));
> +}
> +
> +void HTMLLinkElement::didSendLoadForLinkPrerender()
> +{
> +    dispatchEvent(Event::create(eventNames().webkitPrerenderLoadEvent,
false, false));
> +}
> +
> +void HTMLLinkElement::didSendDOMContentLoadedForLinkPrerender()
> +{
> +   
dispatchEvent(Event::create(eventNames().webkitPrerenderDOMContentLoadedEvent,
false, false));
> +}
> +#endif

Do we want to dispatch these asynchronously?  Will the calling code in the
embedder realize that they can execute arbitrary JavaScript?  It's often better
to do that with a clean stack.

> Source/WebCore/loader/Prerenderer.cpp:71
> -PassRefPtr<PrerenderHandle> Prerenderer::render(const KURL& url)
> +PassRefPtr<PrerenderHandle> Prerenderer::render(PrerenderClient*
prerenderClient, const KURL& url)

I would have just called this parameter "client"