Bug 50080

Looking forward to this! www.nyt.com seems to stall a lot during loading, have you tested that site?

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.

(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 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 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?

(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.

(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 ?

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

Created attachment 76071 [details] QtTestBrowser Patch

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.

Created attachment 76078 [details] QtTestBrowser Patch v2

Comment on attachment 76070 [details] Patch v2 Clearing flags on attachment: 76070 Committed r73710: <http://trac.webkit.org/changeset/73710>

Comment on attachment 76078 [details] QtTestBrowser Patch v2 Clearing flags on attachment: 76078 Committed r73712: <http://trac.webkit.org/changeset/73712>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/73710 might have broken GTK Linux 32-bit Debug

(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...

(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.