Bug 112902

Summary: [Soup] Use ResourceLoadPriority
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: WebKitGTKAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, cgarcia, danw, ddkilzer, gustavo, mrobinson, philn, rakuco, rego+ews, svillar, tmpsantos, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Soup
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
This won't build because it depends on a non-committed libsoup feature. I am also not setting any libsoup guard until we decide whether or not this change gets into the next libsoup release.
none
Patch
none
Patch
none
Patch none

Description Sergio Villar Senin 2013-03-21 05:30:35 PDT 
WebCore already provides a priority for each resource. Since libsoup uses a message queue it should be quite convenient to use that priority to sort messages in the queue and prioritize urgent requests.

See https://bugzilla.gnome.org/show_bug.cgi?id=696277 for the libsoup part of this bug.
Comment 1 Sergio Villar Senin 2013-03-21 05:36:41 PDT 
Created attachment 194237 [details]
This won't build because it depends on a non-committed libsoup feature. I am also not setting any libsoup guard until we decide whether or not this change gets into the next libsoup release.
Comment 2 Sergio Villar Senin 2013-03-21 05:37:34 PDT 
Created attachment 194239 [details]
Patch

This won't build because it depends on a non-committed libsoup feature. I am also not setting any libsoup guard until we decide whether or not this change gets into the next libsoup release.
Comment 3 EFL EWS Bot 2013-03-21 05:50:57 PDT 
Comment on attachment 194239 [details]
Patch

Attachment 194239 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17291025
Comment 4 Martin Robinson 2013-03-21 07:23:06 PDT 
Comment on attachment 194239 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceRequest.h:102
> +    switch (priority) {
> +    case ResourceLoadPriorityUnresolved:

What is does unresolved mean? I'm curious, because perhaps it should have SOUP_MESSAGE_PRIORITY_NORMAL instead.
Comment 5 Sergio Villar Senin 2013-03-21 08:24:25 PDT 
(In reply to comment #4)
> (From update of attachment 194239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194239&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceRequest.h:102
> > +    switch (priority) {
> > +    case ResourceLoadPriorityUnresolved:
> 
> What is does unresolved mean? I'm curious, because perhaps it should have SOUP_MESSAGE_PRIORITY_NORMAL instead.

It's basically the value used by the loader to initialize the priority, and if I understand the loader code correctly, we should never get such a priority because the resource load scheduler has ASSERTs to control that, so we might even break and ASSERT_NOT_REACHED in debug builds and return the default value (which might be NORMAL again).
Comment 6 Sergio Villar Senin 2013-04-17 01:52:09 PDT 
Created attachment 198484 [details]
Patch
Comment 7 Sergio Villar Senin 2013-04-17 01:53:41 PDT 
Adding a couple of guys who might have an opinion on the ResourceLoadPriorityUnresolved thing.
Comment 8 Sergio Villar Senin 2013-04-29 02:54:22 PDT 
ping
Comment 9 Carlos Garcia Campos 2013-06-11 10:32:22 PDT 
(In reply to comment #4)
> (From update of attachment 194239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194239&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceRequest.h:102
> > +    switch (priority) {
> > +    case ResourceLoadPriorityUnresolved:
> 
> What is does unresolved mean? I'm curious, because perhaps it should have SOUP_MESSAGE_PRIORITY_NORMAL instead.

Yes, I agree, if the priority is unresolved I would use the default one in libsoup that is NORMAL.
Comment 10 Sergio Villar Senin 2013-06-12 03:04:06 PDT 
Created attachment 204421 [details]
Patch
Comment 11 Carlos Garcia Campos 2013-06-12 03:56:22 PDT 
Comment on attachment 204421 [details]
Patch

LGTM
Comment 12 Sergio Villar Senin 2013-06-12 04:34:27 PDT 
Comment on attachment 204421 [details]
Patch

Clearing flags on attachment: 204421

Committed r151493: <http://trac.webkit.org/changeset/151493>
Comment 13 Sergio Villar Senin 2013-06-12 04:34:50 PDT 
All reviewed patches have been landed.  Closing bug.