[webkit-reviews] review denied: [Bug 24306] Add a way to figure-out if a ResourceRequest requires upload progress notifications : [Attachment 28235] Moving the repotUploadProgress to ResourceRequestBase

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <darin at chromium.org> has denied Jay Campan
<jcampan at google.com>'s request for review:
Bug 24306: Add a way to figure-out if a ResourceRequest requires upload
progress notifications
https://bugs.webkit.org/show_bug.cgi?id=24306

Attachment 28235: Moving the repotUploadProgress to ResourceRequestBase
https://bugs.webkit.org/attachment.cgi?id=28235&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<darin at chromium.org>
>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.