[webkit-reviews] review requested: [Bug 199632] Remove support for beforeload on link=prefetch : [Attachment 373911] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rbuis at igalia.com> has asked  for review:
Bug 199632: Remove support for beforeload on link=prefetch
https://bugs.webkit.org/show_bug.cgi?id=199632

Attachment 373911: Patch

https://bugs.webkit.org/attachment.cgi?id=373911&action=review




--- Comment #20 from Rob Buis <rbuis at igalia.com> ---
Comment on attachment 373911
  --> https://bugs.webkit.org/attachment.cgi?id=373911
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373911&action=review

>> Source/WebCore/ChangeLog:13
>> +	    (WebCore::LinkLoader::loadLink):
> 
> It is not clear these changes remove support for beforeload for prefetch.
> I understand it that we remove the call to shouldLoadLink for prefetch which
thus removes the call to before load.
> Can you give some more description in the change log?

Sure! Done.

>> Source/WebCore/loader/LinkLoader.cpp:286
>> +	if (!params.href.isValid() || !document.frame())
> 
> It is initially intriguing to not call shouldLoadLink for prefetch.
> It might be clearer to call m_client.shouldLoadLink() in every case in
LinkLoader::loadLink and return true early if the link is prefetch.

Done.

>> Source/WebCore/loader/LinkLoader.cpp:332
>> +	    return true;
> 
> It seems loadLink is always returning true.
> Maybe loadLink should be void.

Well spotted, done.

>> LayoutTests/http/wpt/prefetch/beforeload-dynamic.html:6
>> +</script>
> 
> Can we create it in the script element below?

Done.

>> LayoutTests/http/wpt/prefetch/beforeload.html:5
>> +	var t = async_test('Makes sure that prefetch does not call
beforeload.');
> 
> Ditto.

Done.