Bug 52885

Summary: [Gtk] ResourceHandleSoup: do not wait for streams to close to issue didFinishLoading
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: WebKitGTKAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, mrobinson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch mrobinson: review+

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