Bug 52885 - [Gtk] ResourceHandleSoup: do not wait for streams to close to issue didFinishLoading
Summary: [Gtk] ResourceHandleSoup: do not wait for streams to close to issue didFinish...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-21 04:13 PST by Sergio Villar Senin
Modified: 2011-01-24 03:23 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.34 KB, patch)
2011-01-21 04:17 PST, Sergio Villar Senin
mrobinson: review+
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 2011-01-21 04:13:47 PST
[Gtk] ResourceHandleSoup: do not wait for streams to close to issue didFinishLoading
Comment 1 Sergio Villar Senin 2011-01-21 04:17:10 PST
Created attachment 79723 [details]
Patch
Comment 2 Martin Robinson 2011-01-21 08:20:29 PST
Comment on attachment 79723 [details]
Patch

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

This makes a lot of sense!

> Source/WebCore/ChangeLog:9
> +        No new tests as it does not change functionality. We will not wait for
> +        the the input stream to close to issue didFinishLoading to WebCore

You probably want to mention here what the motiviation is then. Do you see a performance improvement, if so, is it significant?
Comment 3 Sergio Villar Senin 2011-01-21 10:33:19 PST
(In reply to comment #2)
> (From update of attachment 79723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79723&action=review
> 
> This makes a lot of sense!
> 
> > Source/WebCore/ChangeLog:9
> > +        No new tests as it does not change functionality. We will not wait for
> > +        the the input stream to close to issue didFinishLoading to WebCore
> 
> You probably want to mention here what the motiviation is then. Do you see a performance improvement, if so, is it significant?

The performance improvement exists but it's very subtle as it won't be noticed unless we have a lot of asynchronous operations taking place at the same time, something that, on the other hand, is very likely to happen with complex web pages.

I think even an small improvement worths the change as it's very simple but no strong feelings. At the same time it also seems more correct to issue the notification there than in a stream closing callback.
Comment 4 Martin Robinson 2011-01-21 14:26:52 PST
(In reply to comment #3)
> I think even an small improvement worths the change as it's very simple but no strong feelings. At the same time it also seems more correct to issue the notification there than in a stream closing callback.

I definitely agree that this patch is worthwhile. It should be fine to just mention this performance improvment in the ChangeLog, so that those reading later can understand the motivation for the change. :)
Comment 5 Sergio Villar Senin 2011-01-24 02:57:31 PST
Committed r76498: <http://trac.webkit.org/changeset/76498>
Comment 6 WebKit Review Bot 2011-01-24 03:23:14 PST
http://trac.webkit.org/changeset/76498 might have broken Qt Linux Release
The following tests are not passing:
inspector/debugger-autocontinue-on-syntax-error.html
inspector/debugger-scripts.html
inspector/storage-panel-dom-storage.html
inspector/styles-add-blank-property.html