Bug 17971 - [Curl] FormData processing should be moved to its own class
: [Curl] FormData processing should be moved to its own class
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
: Curl
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-20 13:03 PDT by Julien Chaffraix
Modified: 2008-05-07 06:10 PDT (History)
0 users

See Also:


Attachments
First patch (11.87 KB, patch)
2008-03-20 13:05 PDT, Julien Chaffraix
eric: review-
Details | Formatted Diff | Diff
Second version (address Eric's comments) (12.19 KB, patch)
2008-05-06 17:27 PDT, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2008-03-20 13:03:01 PDT
Currently it is done in ResourceHandleManager.cpp.
To match the other ports, we should move it in its own class.
Comment 1 Julien Chaffraix 2008-03-20 13:05:34 PDT
Created attachment 19904 [details]
First patch
Comment 2 Eric Seidel 2008-04-30 23:22:18 PDT
Comment on attachment 19904 [details]
First patch

Looks fine to me.  My only question was if the ownership was right for ResourceHandle*, i.e. why is it safe to hold a weak pointer?  Using FILE* directly kinda sucks too.  But I guess we don't have any nice C++ wrapper for it.

+    size_t toSend = size * nmemb;

integer overflow?  We really should have some sort of safe-multiply function.

What the heck does "nmemb" stand for?  A more clear variable name would be helpful here.

You put * next to the argument name in at least two places.  This is in violation of our style guidelines for C++:
+    size_t read(void *ptr, size_t size, size_t nmemb);


In general looks great.  I was going to r+ it,  but I think there are enough little nits to warrent a second round.
Comment 3 Julien Chaffraix 2008-05-06 17:27:01 PDT
Created attachment 20992 [details]
Second version (address Eric's comments)

> My only question was if the ownership was right for
> ResourceHandle*, i.e. why is it safe to hold a weak pointer?

Checked and added a comment about why a weak pointer is alright (basically we can have that since the ResourceHandle has a strong reference on its FormDataStream).

> Using FILE*
> directly kinda sucks too.  But I guess we don't have any nice C++ wrapper for
> it.
I am afraid we will have to use FILE* until it exists.


> integer overflow?  We really should have some sort of safe-multiply function.

Added an overflow check.

> What the heck does "nmemb" stand for?  A more clear variable name would be
> helpful here.

Number of MEMory Block is my guess. It comes from libcURL documentation. Updated the name in FormDataStream but left it in ResourceHandleManager callback as IMHO it is more intuitive there.

Corrected the coding style issues.
Comment 4 Eric Seidel 2008-05-06 19:09:14 PDT
Comment on attachment 20992 [details]
Second version (address Eric's comments)

Looks sane enough.  m_handle should be m_resourceHandle, since I didn't know what m_handle was when I first read through the code.  I'm not sure I really know what a "ResourceHandle" is as well as I should... but that's my problem. :)

r=me
Comment 5 Julien Chaffraix 2008-05-07 06:10:58 PDT
Committed in r32956 (with m_handle -> m_resourceHandle change)