[webkit-reviews] review denied: [Bug 22541] This program is capable to store all the cookies in a hidden folder in ur home directory and it is also capable to render a SSL certificate on the terminal.. : [Attachment 25569] program is capable to store all the cookies in a hidden folder in ur home directory and it is also capable to render a SSL certificate on the terminal.. I have done some modifications as said by Mr. Oliver Kindly have a look at it.

[bugzilla-daemon at webkit.org]

Holger Freyther <zecke at selfish.org> has denied Nimish Nayak
<nimishnayk at gmail.com>'s request for review:
Bug 22541: This program is capable to store all the cookies in a hidden folder
in ur home directory and it is also capable to render a SSL certificate on the
terminal..
https://bugs.webkit.org/show_bug.cgi?id=22541

Attachment 25569: program is capable to store all the cookies in a hidden
folder in ur home directory and it is also capable to render a SSL certificate
on the terminal.. I have done some modifications as said by Mr. Oliver Kindly
have a look at it.
https://bugs.webkit.org/attachment.cgi?id=25569&action=review

------- Additional Comments from Holger Freyther <zecke at selfish.org>
Please see http://webkit.org/coding/contributing.html for how to contribute.
e.g. use the scripts to generate a ChangeLog, then create the patch.

Some general rules:
  - Try to not revert things.... (e.g. you manually ran diff?, use svn diff...,
git...)
  - Try to not keep dead code around
  - Try to not do whitespace changes, as they are hard to review (increasing
the size of the diff...)


> +//curl_share_setop - set obj for a shared obj
> +

> +   /* if (m_cookieJarFileName)	
> +	   free(m_cookieJarFileName);*/

No, we never keep dead code around. We have a SCM for that.


> +
> +static size_t wrfu(void *ptr,  size_t  size,  size_t  nmemb,  void *stream)

??? what is this supposed to mean?


> -
> +

you have many many whitespace changes...



>	       curl_easy_getinfo(d->m_handle, CURLINFO_EFFECTIVE_URL, &url);
> -	       fprintf(stderr, "Curl ERROR for url='%s', error: '%s'\n", url,
curl_easy_strerror(msg->data.result));
> +	       printf("Curl ERROR for url='%s', error: '%s'\n", url,
curl_easy_strerror(msg->data.result));


this is reverting a patch/fix.


> -	   if (outData && outLength > 0)
> +	   if (outData)

> -	   if (base64Decode(data.latin1().data(), data.latin1().length(), out)
&& out.size() > 0)
> +	   if (base64Decode(data.latin1().data(), data.latin1().length(), out))

>	       client->didReceiveData(handle, out.data(), out.size(), 0);


> -	   if (data.length() > 0)
> -	       client->didReceiveData(handle, reinterpret_cast<const
char*>(data.characters()), data.length() * sizeof(UChar), 0);
> +	   client->didReceiveData(handle, reinterpret_cast<const
char*>(data.characters()), data.length() * sizeof(UChar), 0);

these revert another set of patches... not good.


>  #ifndef NDEBUG
> -	   fprintf(stderr, "Error %d starting job %s\n", ret,
encodeWithURLEscapeSequences(job->request().url().string()).latin1().data());
> +	   printf("Error %d starting job %s\n", ret,
encodeWithURLEscapeSequences(job->request().url().string()).latin1().data

more patches reverted


> -    curl_easy_setopt(d->m_handle, CURLOPT_FOLLOWLOCATION, 1);
> +    curl_easy_setopt(d->m_handle, CURLOPT_FOLLOWLOCATION,1);

whitespace change...

> +    strcpy(path , loc);
> +strcat(path , "/.midori");
> +	
> +	mkdir (path, S_IRWXU); 
> +	
> +	strcat(path , "/cookies.txt");		


No, I would so much prefer to get the the other cookie patch in (ironically I
have to review that too...). Within WebCore we can never do the right thing
with regard to storing the cookies to disk. This part is even wrong for
"midori", not honoring XDG specs for the location of the config dir, locking
when multiple apps using webkit get cookies, poisoning the "shared" cookies...

One wants to expose API to do the cookie management outside of WebKit/WebCore.
Please see Bug #14730 for another approach to the problem.