Bug 24306 - Add a way to figure-out if a ResourceRequest requires upload progress notifications
Summary: Add a way to figure-out if a ResourceRequest requires upload progress notific...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 420+
Hardware: All OS X 10.5
: P2 Normal
Assignee: Jay Campan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-02 16:46 PST by Jay Campan
Modified: 2009-04-27 23:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch that adds a flag to ResourceRequest to indicate whether upload progress notifications are needed. (4.04 KB, patch)
2009-03-02 16:49 PST, Jay Campan
sam: review-
Details | Formatted Diff | Diff
Moving the repotUploadProgress to ResourceRequestBase (3.26 KB, patch)
2009-03-03 12:28 PST, Jay Campan
fishd: review-
Details | Formatted Diff | Diff
New patch with ChangeLog fixed. (3.21 KB, patch)
2009-03-06 13:06 PST, Jay Campan
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Campan 2009-03-02 16:46:52 PST
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.
Comment 1 Jay Campan 2009-03-02 16:49:14 PST
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).
Comment 2 Sam Weinig 2009-03-02 17:15:45 PST
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?
Comment 3 Darin Fisher (:fishd, Google) 2009-03-02 20:01:56 PST
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?
Comment 4 Jay Campan 2009-03-03 12:28:10 PST
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.
Comment 5 Darin Fisher (:fishd, Google) 2009-03-06 11:54:02 PST
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.
Comment 6 Jay Campan 2009-03-06 13:06:13 PST
Created attachment 28370 [details]
New patch with ChangeLog fixed.
Comment 7 Darin Fisher (:fishd, Google) 2009-03-06 13:12:49 PST
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
Comment 8 Darin Fisher (:fishd, Google) 2009-03-06 13:33:01 PST
Landed as http://trac.webkit.org/changeset/41500

Please remember to put a bug link in the ChangeLog next time :)
Comment 9 Alexey Proskuryakov 2009-04-27 23:14:09 PDT
> 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.
Comment 10 Adam Barth 2009-04-27 23:18:04 PDT
(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.