You need to
before you can comment on or make changes to this bug.
Currently it is done in ResourceHandleManager.cpp.
To match the other ports, we should move it in its own class.
Created an attachment (id=19904) [details]
(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.
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
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.
(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. :)
Committed in r32956 (with m_handle -> m_resourceHandle change)