Bug 50080

Summary: [Qt] Support a QNetworkAccessManager affined to a different thread.
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, christian.webkit, eric, kenneth, kling, noam, webkit.review.bot, zecke
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 52081    
Attachments:
Description Flags
Patch (probably has style issues, please disregard them for now)
none
Patch v2
none
QtTestBrowser Patch
none
QtTestBrowser Patch v2 none

Description Jocelyn Turcotte 2010-11-25 07:21:05 PST
On pages with heavy layouting or image decoding, the network stack can wait a long time until the event loop execute its pending actions.

Running the socket interactions in a more responsive thread can improve loading times.
Comment 1 Kenneth Rohde Christiansen 2010-11-25 07:26:14 PST
Looking forward to this! www.nyt.com seems to stall a lot during loading, have you tested that site?
Comment 2 Jocelyn Turcotte 2010-11-25 07:26:27 PST
Created attachment 74877 [details]
Patch (probably has style issues, please disregard them for now)

This patch depends on other smaller patches to apply, I'll post them soon.
Comment 3 Jocelyn Turcotte 2010-11-25 08:17:12 PST
(In reply to comment #1)
> Looking forward to this! www.nyt.com seems to stall a lot during loading, have you tested that site?

On desktop I don't see any notice improvements with the live website, sadly.
The patch mostly tries to improve the time spent with a pending network notification that we need to take action (i.e. writing on the socket).

For download usually the TCP window and the network buffer are large enough and we don't have any trouble keep the download going for most web sites resources.
Comment 4 Kenneth Rohde Christiansen 2010-11-26 02:55:46 PST
Comment on attachment 74877 [details]
Patch (probably has style issues, please disregard them for now)

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

> WebCore/ChangeLog:37
> +        * platform/network/qt/NAMManipulator.cpp: Added.
> +        (WebCore::NAMManipulator::NAMManipulator):
> +        (WebCore::NAMManipulator::localSetCookies):
> +        (WebCore::NAMManipulator::localCookiesForUrl):
> +        (WebCore::NAMManipulator::localWillLoadFromCache):
> +        (WebCore::ReplyManipulator::ReplyManipulator):
> +        (WebCore::ReplyManipulator::~ReplyManipulator):
> +        (WebCore::ReplyManipulator::localGet):
> +        (WebCore::ReplyManipulator::localPost):
> +        (WebCore::ReplyManipulator::localHead):
> +        (WebCore::ReplyManipulator::localPut):
> +        (WebCore::ReplyManipulator::localDeleteResource):
> +        (WebCore::ReplyManipulator::localCustomRequest):
> +        (WebCore::ReplyManipulator::localAbort):
> +        (WebCore::ReplyManipulator::localForwardData):
> +        (WebCore::ReplyManipulator::localSetForwardingDefered):
> +        (WebCore::ReplyManipulator::localMirrorMembers):
> +        (WebCore::ReplyManipulator::localSetReply):
> +        * platform/network/qt/NAMManipulator.h: Added.

For new files you do not need to show the methods. You can just cut this out from the ChangeLog

> WebCore/platform/network/qt/NAMManipulator.cpp:30
> +static int dummyStaticVar1 = qRegisterMetaType<QFutureInterface<bool> >("QFutureInterface<bool>");
> +static int dummyStaticVar2 = qRegisterMetaType<QFutureInterface<QList<QNetworkCookie> > >("QFutureInterface<QList<QNetworkCookie> >");
> +

Could we find better names for this or at least add a comment?

> WebCore/platform/network/qt/NAMManipulator.cpp:97
> +    if (m_reply)
> +        delete m_reply;
> +}

Just do delete m_reply. delete handles null-pointers :-)

> WebCore/platform/network/qt/NAMManipulator.h:60
> +signals:

We probably want to use Q_SIGNALS etc

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:512
> +    connect(m_reply, SIGNAL(finished()),
> +            this, SLOT(finish()), SIGNAL_CONN);

Those should really be one liners :-)
Comment 5 Kenneth Rohde Christiansen 2010-12-09 01:36:48 PST
Comment on attachment 74877 [details]
Patch (probably has style issues, please disregard them for now)

The classes are very Qt specific so we should name them appropiate like QtNAMManipulator. Could we find a better name than manipulator?
Comment 6 Jocelyn Turcotte 2010-12-09 02:14:17 PST
(In reply to comment #5)
> (From update of attachment 74877 [details])
> The classes are very Qt specific so we should name them appropiate like QtNAMManipulator. Could we find a better name than manipulator?

Maybe QtNAMAccessor (can be weird since it translates to QtNetworkAccessManagerAccessor)
or QtThreadSafeNAM (it's not quite a NAM by itself though)
or QtThreadSafeNAMAccess
or QtNAMThreadBridge

Manipulator sounded the best, though it's normally used in a pejorative context. Do you have a favorite or other ideas?
Same thing for ReplyManipulator.
Comment 7 Kenneth Rohde Christiansen 2010-12-09 02:24:30 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 74877 [details] [details])
> > The classes are very Qt specific so we should name them appropiate like QtNAMManipulator. Could we find a better name than manipulator?
> 
> Maybe QtNAMAccessor (can be weird since it translates to QtNetworkAccessManagerAccessor)
> or QtThreadSafeNAM (it's not quite a NAM by itself though)
> or QtThreadSafeNAMAccess
> or QtNAMThreadBridge
> 
> Manipulator sounded the best, though it's normally used in a pejorative context. Do you have a favorite or other ideas?
> Same thing for ReplyManipulator.

QtNAMThreadSafeProxy ?
Comment 8 Jocelyn Turcotte 2010-12-09 08:38:11 PST
Created attachment 76070 [details]
Patch v2

This version:
- Renames NAMManipulator to QtNAMThreadSafeProxy and ReplyManipulator to QtNetworkReplyThreadSafeProxy
- Adds a comment for the dummy variables, though if you have a better idea I would prefer since it generates a warning for unused variable on GCC.
- Fixes style issues
Comment 9 Jocelyn Turcotte 2010-12-09 08:45:50 PST
Created attachment 76071 [details]
QtTestBrowser Patch
Comment 10 WebKit Review Bot 2010-12-09 08:47:10 PST
Attachment 76071 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKitTools/ChangeLog', u'WebKitTools/QtTestBrowser/launcherwindow.cpp', u'WebKitTools/QtTestBrowser/launcherwindow.h', u'WebKitTools/QtTestBrowser/webpage.cpp', u'WebKitTools/QtTestBrowser/webpage.h']" exit_code: 1
WebKitTools/QtTestBrowser/webpage.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Jocelyn Turcotte 2010-12-09 09:56:32 PST
Created attachment 76078 [details]
QtTestBrowser Patch v2
Comment 12 Jocelyn Turcotte 2010-12-10 02:50:24 PST
Comment on attachment 76070 [details]
Patch v2

Clearing flags on attachment: 76070

Committed r73710: <http://trac.webkit.org/changeset/73710>
Comment 13 Jocelyn Turcotte 2010-12-10 02:51:33 PST
Comment on attachment 76078 [details]
QtTestBrowser Patch v2

Clearing flags on attachment: 76078

Committed r73712: <http://trac.webkit.org/changeset/73712>
Comment 14 Jocelyn Turcotte 2010-12-10 02:51:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Review Bot 2010-12-10 11:46:25 PST
http://trac.webkit.org/changeset/73710 might have broken GTK Linux 32-bit Debug
Comment 16 Holger Freyther 2010-12-17 02:49:18 PST
(In reply to comment #3)
> (In reply to comment #1)
> > Looking forward to this! www.nyt.com seems to stall a lot during loading, have you tested that site?
> 
> On desktop I don't see any notice improvements with the live website, sadly.
> The patch mostly tries to improve the time spent with a pending network notification that we need to take action (i.e. writing on the socket).


I was looking at an easy fix like this at the last QtWebKit workshop in Wiesbaden and this had about zero effect. QtWebKit was blocking on font/text handling and was not consuming the data from the network stack, also not sending new requests...
Comment 17 Kenneth Rohde Christiansen 2010-12-17 02:51:58 PST
(In reply to comment #16)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > Looking forward to this! www.nyt.com seems to stall a lot during loading, have you tested that site?
> > 
> > On desktop I don't see any notice improvements with the live website, sadly.
> > The patch mostly tries to improve the time spent with a pending network notification that we need to take action (i.e. writing on the socket).
> 
> 
> I was looking at an easy fix like this at the last QtWebKit workshop in Wiesbaden and this had about zero effect. QtWebKit was blocking on font/text handling and was not consuming the data from the network stack, also not sending new requests...

Luckily we have some optimizations coming in that area soon :-) cc'ing kling.