[webkit-reviews] review requested: [Bug 3652] add support for link prefetching : [Attachment 60822] remediated to abarth's comments

[bugzilla-daemon at webkit.org]

Gavin Peters <gavinp at chromium.org> has asked  for review:
Bug 3652: add support for link prefetching
https://bugs.webkit.org/show_bug.cgi?id=3652

Attachment 60822: remediated to abarth's comments
https://bugs.webkit.org/attachment.cgi?id=60822&action=review

------- Additional Comments from Gavin Peters <gavinp at chromium.org>
A remediation per review for abarth's comments.

> WebCore/html/HTMLLinkElement.h:56
> +		 { };
> These shoudn't be indented quite as much and { and } should be on separate
lines.	Also, the ; is superfluous.

Done.

> WebCore/loader/DocLoader.cpp:234
>  +	  case CachedResource::LinkPrefetch:
> I don't think a link prefetch can corrupt a frame's pixels.  Maybe we need to
add a new category here?

Done.

> + ENABLE_DEVICE_ORIENTATION = ;
> Device orientation?
> 
> + ENABLE_IMAGE_RESIZER = ;
> ??

My bad: copied from other build scripts.  Now fixed.

> LayoutTests/fast/dom/HTMLLinkElement/prefetch.html:7
>  +	  setTimeout("layoutTestController.notifyDone()",50);
> Boo

Boo hoo!

> LayoutTests/fast/dom/HTMLLinkElement/prefetch.html:6
>  +	  layoutTestController.dumpResourceResponseMIMETypes();
> Why doesn't the show up in the results?  Maybe you need to use something with
a known file extension, like HTML?

My bad.  Modified the test, now everything is in the results very well.

> LayoutTests/platform/mac/Skipped:285
> +  # Link prefetch is disabled by default
> What platform will run the test?  Why are we putting this behind a #define? 
We should just implement the feature, no?

So I've had good luck testing this enabled in mac, and in chromium.  However
for chromium there's an extra patch required (see FromTargetType in
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/weburlloader_impl.c
c?view=markup ).  To safely build this in chromium, we need to land this patch
with define guards in webkit, and then enable the feature in webkit at the same
time that we update FromTargetType (among other things).  Unfortunately,
DumpRenderTree in chromium won't let this feature be tested.

However, my testing to date shows no reason not to enable this in mac: i'd be
excited to do it.