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
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Curl
:
:
  Show dependency treegraph
 
Reported: 2008-03-20 13:03 PST by
Modified: 2008-05-07 06:10 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-03-20 13:03:01 PST
Currently it is done in ResourceHandleManager.cpp.
To match the other ports, we should move it in its own class.
------- Comment #1 From 2008-03-20 13:05:34 PST -------
Created an attachment (id=19904) [details]
First patch
------- Comment #2 From 2008-04-30 23:22:18 PST -------
(From update of attachment 19904 [details])
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 From 2008-05-06 17:27:01 PST -------
Created an attachment (id=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 From 2008-05-06 19:09:14 PST -------
(From update of attachment 20992 [details])
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 From 2008-05-07 06:10:58 PST -------
Committed in r32956 (with m_handle -> m_resourceHandle change)