Bug 24306 – Add a way to figure-out if a ResourceRequest requires upload progress notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jay Campan]

ResourceLoader::didSendData() must be called for the XMLHttpRequest update callbacks to be invoked.
It would be good for the request to indicate whether such callbacks are registered so the loader does not have to call didSendData if there are no consumers.
This would be especially useful in the Chromium case to limit the number of notifications sent as they are sent over IPC.

[Jay Campan]

Created attachment 28204 [details]
Patch that adds a flag to ResourceRequest to indicate whether upload progress notifications are needed. 

Adding a flag to ResourceRequest to indicate whether or not upload progress notifications are needed. This is useful to avoid sending these notifications when there are no consumers (especially in the Chromium case where IPC are involved).

[Sam Weinig]

Comment on attachment 28204 [details]
Patch that adds a flag to ResourceRequest to indicate whether upload progress notifications are needed. 

I believe this will break all non-chromium platforms.

Also, what if a progress handler gets added during the load?

[Darin Fisher (:fishd, Google)]

Yeah, good point.  We should always send upload events to XMLHttpRequest.

Sam, is a #ifdef PLATFORM(CHROMIUM) desirable for the setReportUploadProgress call, or would it be better to move that property down to ResourceRequestBase?

[Jay Campan]

Created attachment 28235 [details]
Moving the repotUploadProgress to ResourceRequestBase

I moved the reportUploadProgress property to the ResourceRequestBase class.
Let me know if you prefer having an #ifdef CHROMIUM in XMLHttpRequest.cpp instead, as Darin suggested.
I now always set the property to true for XMLHttpRequest to make sure that upload progress notifications will happen even if registered after the load has started.

[Darin Fisher (:fishd, Google)]

Comment on attachment 28235 [details]
Moving the repotUploadProgress to ResourceRequestBase

>Index: WebCore/ChangeLog
...
>+        Adding a flag to ResourceRequest to indicate whether or not upload
>+        progress notifications are needed for a resource. This is useful to
>+        avoid sending these notifications when there are no consumers
>+        (especially in the Chromium case where IPC are involved). 

the comment should say ResourceRequestBase instead. 

nit: "where IPC _is_ involved"


>+        WARNING: NO TEST CASES ADDED OR CHANGED

This line should be removed from the ChangeLog.


>Index: WebCore/platform/network/ResourceRequestBase.h
...
>+        bool reportUploadProgress() const { return m_reportUploadProgress; }
>+        void setReportUploadProgress(bool reportUploadProgress)
>+        {
>+            m_reportUploadProgress = reportUploadProgress;
>+        }    

nit: this file seems to put the implementation for short methods like this
all on one line.  it might be nice to the original author to maintain that
style.


>     private:
>         const ResourceRequest& asResourceRequest() const;
>+        bool m_reportUploadProgress;
>     };

I think this new member should be protected like all of the other data
members.


Otherwise, this all LG to me.  Please post a cleaned up patch and re-request
review.

[Jay Campan]

Created attachment 28370 [details]
New patch with ChangeLog fixed.

[Darin Fisher (:fishd, Google)]

Comment on attachment 28370 [details]
New patch with ChangeLog fixed.

>Index: WebCore/platform/network/ResourceRequestBase.h
...
>+        void setReportUploadProgress(bool reportUploadProgress) { m_reportUploadProgress = reportUploadProgress;  }

nit: extra whitespace before the "}"

otherwise LGTM

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/41500

Please remember to put a bug link in the ChangeLog next time :)

[Alexey Proskuryakov]

> Also, what if a progress handler gets added during the load?

Looks like this was not addressed, after all.

More importantly, it is wrong to have m_reportUploadProgress in ResourceResponseBase. On platforms that let a client override a request via a delegate call (like Mac), it will be simply lost. See e.g. WebFrameLoaderClient::dispatchWillSendRequest() in WebFrameLoaderClient.mm:

    if (implementations->willSendRequestFunc)
        request = (NSURLRequest *)CallResourceLoadDelegate(implementations->willSendRequestFunc, webView, @selector(webView:resource:willSendRequest:redirectResponse:fromDataSource:), [webView _objectForIdentifier:identifier], request.nsURLRequest(), redirectResponse.nsURLResponse(), dataSource(loader));

So, the boolean in ResourceRequestBase is useless for cross-platform code, but it adds confusion and increases the size of request objects.

[Adam Barth]

(In reply to comment #9)
> More importantly, it is wrong to have m_reportUploadProgress in
> ResourceResponseBase. On platforms that let a client override a request via a
> delegate call (like Mac), it will be simply lost.

This design constraint keeps coming up.  It seems like we need some generalized way to handle these use cases.