Bug 61079 - Add rel type prerender to distinguish prerender from prefetch
: Add rel type prerender to distinguish prerender from prefetch
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-18 10:57 PDT by Gavin Peters
Modified: 2011-05-23 12:27 PDT (History)
8 users (show)

See Also:


Attachments
Patch (18.49 KB, patch)
2011-05-18 11:48 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (18.47 KB, patch)
2011-05-18 12:06 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (18.56 KB, patch)
2011-05-18 12:33 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2011-05-18 10:57:22 PDT
Add rel type prerender to distinguish prerender from prefetch
Comment 1 Gavin Peters 2011-05-18 11:48:57 PDT
Created attachment 93958 [details]
Patch
Comment 2 Gavin Peters 2011-05-18 11:50:01 PDT
Also, see how I cleaned things up in CachedResourceRequest.cpp?  I like the new cachedResourceTypeToTargetType a lot better.
Comment 3 WebKit Review Bot 2011-05-18 11:52:22 PDT
Attachment 93958 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/loader/cache/CachedResourceLoader.h:73:  The parameter name "priority" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Gavin Peters 2011-05-18 11:54:43 PDT
Unfortunately, this has tickled me before, the style failure doesn't recognise a default parameter as sufficient reason to name an argument.
Comment 5 Gavin Peters 2011-05-18 11:56:41 PDT
Comment on attachment 93958 [details]
Patch

bad patch.  new upload pending.
Comment 6 Gavin Peters 2011-05-18 12:06:01 PDT
Created attachment 93961 [details]
Patch
Comment 7 Gavin Peters 2011-05-18 12:06:41 PDT
Comment on attachment 93961 [details]
Patch

Better patch.  Thank you dominich@chromium.org.
Comment 8 WebKit Review Bot 2011-05-18 12:07:55 PDT
Attachment 93961 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/loader/cache/CachedResourceLoader.h:73:  The parameter name "priority" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Early Warning System Bot 2011-05-18 12:30:07 PDT
Comment on attachment 93961 [details]
Patch

Attachment 93961 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8707547
Comment 10 Gavin Peters 2011-05-18 12:33:45 PDT
Created attachment 93966 [details]
Patch
Comment 11 Gavin Peters 2011-05-18 12:34:03 PDT
Comment on attachment 93966 [details]
Patch

Fix non-prefetching builds!
Comment 12 WebKit Review Bot 2011-05-18 12:36:22 PDT
Attachment 93966 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/loader/cache/CachedResourceLoader.h:73:  The parameter name "priority" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Adam Barth 2011-05-20 15:27:06 PDT
Comment on attachment 93966 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:263
>              priority = ResourceLoadPriorityLow;

We don't want to set priority = ResourceLoadPriorityLow for prerender as well?
Comment 14 Gavin Peters 2011-05-21 07:32:45 PDT
Comment on attachment 93966 [details]
Patch

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

>> Source/WebCore/html/HTMLLinkElement.cpp:263
>>              priority = ResourceLoadPriorityLow;
> 
> We don't want to set priority = ResourceLoadPriorityLow for prerender as well?

I think no; the VeryLow that it gets by default is probably correct.  Priority in WebKit loading only controls when requests are sent to the network layer (the lower the priority, the later they go out), and for prerender being late like a prefetch is probably correct; the prerender fetch then won't interfere as much with main resource loading.  In the subresource case, the contrast is that the resource is associated with the current page, and Low priority mirrors what something like an image would get.
Comment 15 WebKit Commit Bot 2011-05-21 12:23:33 PDT
The commit-queue encountered the following flaky tests while processing attachment 93966 [details]:

http/tests/websocket/tests/handshake-fail-by-cross-origin.html bug 54147 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2011-05-21 12:25:07 PDT
Comment on attachment 93966 [details]
Patch

Clearing flags on attachment: 93966

Committed r87020: <http://trac.webkit.org/changeset/87020>
Comment 17 WebKit Commit Bot 2011-05-21 12:25:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Commit Bot 2011-05-21 13:25:28 PDT
The commit-queue encountered the following flaky tests while processing attachment 93966 [details]:

http/tests/websocket/tests/close-event.html bug 61240 (author: yutak@chromium.org)
The commit-queue is continuing to process your patch.
Comment 19 Gavin Peters 2011-05-23 12:27:40 PDT
I have learned a valuable lesson; always test the chrome side of a webkit patch that has chrome implication before uploading to WebKit: https://bugs.webkit.org/show_bug.cgi?id=61297 catches two mistakes that shouldn't have been in this bug.