Bug 112902 - [Soup] Use ResourceLoadPriority
Summary: [Soup] Use ResourceLoadPriority
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: Soup
Depends on:
Blocks:
 
Reported: 2013-03-21 05:30 PDT by Sergio Villar Senin
Modified: 2013-06-12 04:34 PDT (History)
14 users (show)

See Also:


Attachments
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. (3.03 KB, patch)
2013-03-21 05:36 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2013-03-21 05:37 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (3.11 KB, patch)
2013-04-17 01:52 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (3.11 KB, patch)
2013-06-12 03:04 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.