Bug 52885 – [Gtk] ResourceHandleSoup: do not wait for streams to close to issue didFinishLoading

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[Gtk] ResourceHandleSoup: do not wait for streams to close to issue didFinishLoading

[Sergio Villar Senin]

Created attachment 79723 [details]
Patch

[Martin Robinson]

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?

[Sergio Villar Senin]

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

[Martin Robinson]

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

[Sergio Villar Senin]

Committed r76498: <http://trac.webkit.org/changeset/76498>

[WebKit Review Bot]

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