Adding Cookies Support (wxGTK )

[wxtrac]

Issue migrated from trac ticket # 14528

component: WebView | priority: normal | keywords: cookies-support

2012-07-27 09:29:54: evstevemd (Stefano) created the issue

---

Here is a patch to add cookies support for cookies support. This patch adds only for wxGTK and I plan to add same for IE (MSW). Features includes

• Cookie Jar holding all cookies (Hence class wxWebviewCookieJar)
• Add new Cookie
• Remove cookie by name
• Remove all cookies in a domain

Basically this adds persistence to Webview for GTK.

Since this is my first contribution I would accept constructive criticism in coding style and any other mis-done/unstandard thing

Long live wxWidgets!

[wxtrac]

2012-07-27 11:03:38: @PBfordev commented

---

I know nothing about webview or GTK, but out of curiosity I peeked at the patch, surprised at its size of 92 kB. Are you sure you've submitted the correct patch file? It seems there's included a lot of changes in various unrelated files, perhaps these should be removed. Maybe you should also get rid of the differences which are there because only of whitespace changes, these make reviewing new code unnecessarily more difficult - I remember Vadim "scolding me" for that when I submitted my first patch. I believe SVN has an option for ignoring whitespace changes, but I've never used it so I am not sure how reliable it is.

I hope you take no offense in my comments, I'm just trying to help. :)

[wxtrac]

2012-07-27 11:24:53: evstevemd (Stefano) commented

---

Thanks for your comment PB. I take it positively!

Here is what I did to make the patch

cd wxWidgets

svn diff >wxwebview_cookies.patch

 ./misc/scripts/clean_patch wxwebview_cookies.patch >wxwebview_cookies_clean.patch [[BR]][[BR]]This is supposed to make patch clean by removing unnecessary files according to wxBlog.

I suspect code beautification I use with my IDE cause addition of new lines/whitespaces

Can you suggest what I can do?

[wxtrac]

2012-07-27 11:52:13: @vadz changed status from new to infoneeded_new

2012-07-27 11:52:13: @vadz commented

The patch is definitely impossible to review or apply in the current state :-( I don't know what happened with the "$Id$" lines, but this, at least, could be filtered out easily with filterdiff. As for white space changes, you could try using -b or -w option to ignore it. But it's a really bad idea to apply any automatically generated changes to wx sources, so in the future please don't use your IDE to reformat the code.

For now I'd recommend that you:

1. Diff only the files really affected by your changes.
2. Use diff -w or svn diff -x -w to avoid white space-only changes.
3. Review your patch to verify that it only includes the necessary changes.

Thanks!

[wxtrac]

2012-07-27 11:58:21: evstevemd (Stefano) commented

---

Thanks vadim,

I'll do that!

[wxtrac]

2012-07-27 16:17:03: evstevemd (Stefano) commented

---

So I did clean svn checkout and applied changes and here is clean patch

If anything missing please point it out and I will work it out.

2012-07-27 16:17:03: evstevemd (Stefano) changed status from infoneeded_new to new

[wxtrac]

2012-07-27 17:28:53: @lanurmi commented

---

There are still a number of whitespace-only changes and wrong indentation in the patch. Use a tool such as meld in your SVN working copy to easily and interactively eliminate such changed lines.

[wxtrac]

2012-07-29 15:47:02: evstevemd (Stefano) commented

---

Can you explain to me what is missing? I have used meld to make the patch.

I'm not maverick at meld and so can you point me to tutorial or something like directions?

Thanks!

[wxtrac]

2012-07-29 16:31:54: @vadz changed status from new to infoneeded_new

2012-07-29 16:31:54: @vadz commented

I don't know how to use meld for this but typically you'd open a 2 side diff view and undo the changes consisting of white space only. As for the wrong indent, there is no magic solution, you need to reindent your code to use 4 space indentation manually. Finally, I don't understand if wxEVT_COMMAND_WEB_VIEW_LOADING_PROGRESS changes are supposed to make part of the same patch?

Also, a couple of small remarks about the changes themselves:

1. There is probably no need to include "wx/gtk/webview_cookie_jar.h" from the main header, a forward declaration should do.
2. HasCookieJar() should really be const. GetCookieJar() should probably be const too.
3. Where is the documentation for the new methods?

Other than that, I can't say much because the latest patch doesn't include the new files...

[wxtrac]

2012-07-29 21:28:37: evstevemd (Stefano) commented

---

Hi Vadim,

thanks for comments. I think I have to learn more how can meld undo whitespace changes. Yes, I added wxEVT_COMMAND_WEB_VIEW_LOADING_PROGRESS as new event monitoring loading progress. It is fired each time progress changes. As for remarks in numbering:

1. I have SoupCookieJar which is a typedef of a struct. I tried to find how to forward declare a typedef and found now way. That is the reason I decided to include file. I would like to hear your comments on this

2. Noted I will change them to const

3.I'll add the methods documentation. Sorry for not including them.

I have to wrest with woes of making a patch!

[wxtrac]

2012-07-29 21:34:45: @vadz commented

---

Adding wxEVT_COMMAND_WEB_VIEW_LOADING_PROGRESS is perfectly fine, it's just that I'd prefer to have a separate patch for it. But I can separate the patch in two parts without too many problems so don't worry about this for now, just please keep the atomicity advice from HowToSubmitPatches in mind for your next patches.

The rest of the points:

1. You don't need typedefs for structs in C++, this is a C idiom. Just use struct SoupCookieJar { ... } and forward declare it as struct SoupCookieJar;.
2. TIA!
3. Please also document the new LOADING_PROGRESS event too and please use @since 2.9.5 in their descriptions.

As for making the patch: your patch is perfectly correct from patch submission point of view, the problem is just with the (unnecessary) whitespace changes you had made in the first place :-( Anyhow, it's not a huge deal, if you just look at the patch in Trac UI you can see which changes are unnecessary and simply revert them in your local copy and then redo the patch. Please review the patch file before submitting it to ensure it doesn't have any unnecessary changes. Thanks in advance and good luck!

[wxtrac]

2012-08-15 17:49:14: evstevemd (Stefano) commented

---

Hi Vadim and team,

some commitments interrupted me and I had no way to get into a patch. I will work on patch in these few days and re-submit it. After that I will start the download part before I try to add same support in windows port

regards,

Stefano

[wxtrac]

2012-09-12 12:56:35: evstevemd (Stefano) changed status from infoneeded_new to new

[wxtrac]

2012-09-12 13:50:12: evstevemd (Stefano) uploaded file wxwebview_cookies_clean.patch (13.9 KiB)

Working patch

[wxtrac]

2012-09-16 17:09:58: @sjlamerton changed status from new to infoneeded_new

2012-09-16 17:09:58: @sjlamerton commented

Thanks for the patch, but I'm afraid it doesn't apply for me, I get an error at line 101. I will extract the progress parts and implement them for other platforms as well. If I can I'll try and extract the cookie parts as well but if you could submit a working patch with only the cookie parts that would be great.

[wxtrac]

2012-09-18 11:49:16: evstevemd (Stefano) uploaded file wxwebview_cookies_latest.patch (12.5 KiB)

[wxtrac]

2012-09-18 11:50:53: evstevemd (Stefano) changed status from infoneeded_new to new

2012-09-18 11:50:53: evstevemd (Stefano) commented

I hope this one will be working but with both progress and cookies.

[wxtrac]

2012-09-26 17:25:38: @sjlamerton commented

---

I have extracted the progress event stuff into a separate patch with some changes in #14699

[wxtrac]

2012-10-01 11:15:36: evstevemd (Stefano) commented

---

So what about the cookies? I wanted to finish them so I can do something with download. If that be hard, can I send you zipped files (only those with changes) that compiles with latest SVN trunk?
This patch thing is a real block :)

[wxtrac]

2012-10-01 17:52:38: @sjlamerton changed status from new to confirmed

2012-10-01 17:52:38: @sjlamerton commented

Although the patch still doesn't apply cleanly for me (the trac preview not working is a good indication of this) I'll try and clean it up tonight and post my changes back here so you can check if they seem reasonable.

I was also looking at adding OSX support, it looks like we can implement most of the API using NSHTTPCookieStorage and NSHTTPCookit.

However as with Windows (as far as I know) we can only access the global cookie jar and not create our own local one so the API might need some small adjustments. I'll try and incorperate them into my changes.

[wxtrac]

2012-10-01 23:22:23: @sjlamerton changed status from confirmed to infoneeded_new

2012-10-01 23:22:23: @sjlamerton commented

I have attached an updated version of the patch, note I haven't tried compiling it yet as I have some most questions about the actual patch itself. I have made the following changes:

• Added the .h file to files.bkl
• Used utf8_str rather than mb_str(wxConvUTF8)
• Moved [Get/Set/Has]CookieJar out of webview.cpp and into webview_webkit.cpp
• Hopefully fixed compilation for other ports
• Added the missing file header block to webview_cookie_jar.h
• Moved the cookie functions to their own section in the wxWebView docs
• Renamed RemoveCookieBy[Name/Domaian] to RemoveCookiesBy[Name/Domain]

I still have a few questions though:

• Do we really need the SetCwd code in the cookie jar creator?
• Also does the file really need to exist before we create the cookie jar, if so this needs to be documented?
• Does wxWebViewCookieJar really need wxWebView as a friend class? I removed it for now.
• Can we have the internal m_cookieJar as a SoupCookieJar* rather than void*?

I will try and figure these out when I do further testing tomorrow but if you already know any of the answers that would be great!

We will also need to change the API slightly for backends that don't support creating new cookie jars but do allow accessing the existing one as I mentioned above.

[wxtrac]

2012-10-01 23:22:53: @sjlamerton uploaded file cookiegtk.patch (9.8 KiB)

Slightly cleaned up patch

[wxtrac]

2012-10-02 22:08:45: @sjlamerton uploaded file cookiegtk2.patch (10.3 KiB)

Fix compilation, more changes

[wxtrac]

2012-10-03 10:15:44: @vadz changed status from infoneeded_new to new

2012-10-03 10:15:44: @vadz commented

Replying to [comment:18 steve_lamerton]:

• Used utf8_str rather than mb_str(wxConvUTF8)

Notice that there are still a few occurrences of the latter remaining in webview_cookie_jar.cpp.

Some other minor stylistic comments:

• Please use wxString() instead of wxEmptyString.
• Don't use const int age, just int age (for consistency and also because this const is useless anyhow).
• {Get,Set,Has}CookieJar() methods don't have "@SInCE 2.9.5" in their documentation.
• It would be great if the lines could be made if not shorter than 80 characters (although it would be nice), but at least somewhat shorter. Doing a 3 column merge with the lines as long as that is really not nice, even on a 1920 pixel wide screen.
• Most minor comment ever, but why not write just return m_cookirJar != NULL in HasCookieJar() implementation?

I still have a few questions though:

• Do we really need the SetCwd code in the cookie jar creator?

You seem to have removed it in the latest version of the patch, so I guess this is not relevant any more, but FWIW I definitely think CWD should never be changed implicitly.

We will also need to change the API slightly for backends that don't support creating new cookie jars but do allow accessing the existing one as I mentioned above.

We probably need some way of determining whether this functionality is available. Traditionally, we just define wxHAS_FOO to indicate that you have Foo but it wouldn't work here as you could use different backends, so I guess we'd have to add HasCookieJarSupport() or something like this to wxWebView itself.

[wxtrac]

2012-10-03 10:47:33: evstevemd (Stefano) commented

---

Replying to [comment:18 steve_lamerton]:
Hi Steve,
Sorry for late reply, I have been out of internet and I will be getting lost for a little while online but all will soon be fine.

I have attached an updated version of the patch, note I haven't tried compiling it yet as I have some most questions about the actual patch itself. I have made the following changes:

• Added the .h file to files.bkl
• Used utf8_str rather than mb_str(wxConvUTF8)
• Moved [Get/Set/Has]CookieJar out of webview.cpp and into webview_webkit.cpp
• Hopefully fixed compilation for other ports
• Added the missing file header block to webview_cookie_jar.h
• Moved the cookie functions to their own section in the wxWebView docs
• Renamed RemoveCookieBy[Name/Domaian] to RemoveCookiesBy[Name/Domain]

I'm checking out fresh SVN so that I can Apply the patch and move on :)

I still have a few questions though:

• Do we really need the SetCwd code in the cookie jar creator?
  Nope, we don't. I added for some reasons I have forgotten so just remove it!

• Also does the file really need to exist before we create the cookie jar, if so this needs to be documented?
  In GTK it creates the file if does not exist. I don't know about OSX though. So file checking can be removed.

• Does wxWebViewCookieJar really need wxWebView as a friend class? I removed it for now.
  If it does not hurt remove it. There was a member in wxWebView that needed to be accessed from wxWebViewCookieJar. Since I changed a lot of stuffs, you can remove it (the compiler will tell if that member is still there :) )

• Can we have the internal m_cookieJar as a SoupCookieJar* rather than void*?
  Do you think we are going to need anything than SoupCookieJar*? If yes then I believe your suggestion is valid, else we can leave it that way. I mean void* is versatile but my question is is that versatility necessary?

I will try and figure these out when I do further testing tomorrow but if you already know any of the answers that would be great!

That is what I can say AFAIK

We will also need to change the API slightly for backends that don't support creating new cookie jars but do allow accessing the existing one as I mentioned above.
That will be great!

[wxtrac]

2012-10-03 11:12:04: @sjlamerton commented

---

Replying to [comment:19 vadz]:

Replying to [comment:18 steve_lamerton]:

• Used utf8_str rather than mb_str(wxConvUTF8)

Notice that there are still a few occurrences of the latter remaining in webview_cookie_jar.cpp.

Yes, I had to put them back, it didn't work.

Some other minor stylistic comments:

• Please use wxString() instead of wxEmptyString.
• Don't use const int age, just int age (for consistency and also because this const is useless anyhow).
• {Get,Set,Has}CookieJar() methods don't have "@SInCE 2.9.5" in their documentation.
• It would be great if the lines could be made if not shorter than 80 characters (although it would be nice), but at least somewhat shorter. Doing a 3 column merge with the lines as long as that is really not nice, even on a 1920 pixel wide screen.
• Most minor comment ever, but why not write just return m_cookirJar != NULL in HasCookieJar() implementation?

I'll sort all these out, the patch I attached is very much still a work in progress!

We will also need to change the API slightly for backends that don't support creating new cookie jars but do allow accessing the existing one as I mentioned above.

We probably need some way of determining whether this functionality is available. Traditionally, we just define wxHAS_FOO to indicate that you have Foo but it wouldn't work here as you could use different backends, so I guess we'd have to add HasCookieJarSupport() or something like this to wxWebView itself.

Probably HasSetableCookieCar or CanSetCookieJar or something similar, better name suggestions welcome!

[wxtrac]

2012-10-03 11:15:31: @sjlamerton commented

---

Replying to [comment:20 evstevemd]:

Replying to [comment:18 steve_lamerton]:

I will try and figure these out when I do further testing tomorrow but if you already know any of the answers that would be great!

That is what I can say AFAIK

Thanks for your answers, I made most of the changes already in my more up to date patch but was slightly worried I might have broken something and not realised it!

[wxtrac]

2012-10-03 11:17:49: @vadz commented

---

Replying to [comment:21 steve_lamerton]:

Replying to [comment:19 vadz]:

Replying to [comment:18 steve_lamerton]:

• Used utf8_str rather than mb_str(wxConvUTF8)

Notice that there are still a few occurrences of the latter remaining in webview_cookie_jar.cpp.

Yes, I had to put them back, it didn't work.

Err, how so? utf8_str() really should be used, not only it's more clear but potentially (in wxUSE_UNICODE_UTF8 build) more efficient too.

Probably HasSetableCookieCar or CanSetCookieJar or something similar, better name suggestions welcome!

I'm not sure here... HasXXX() is more consistent with wxHAS_XXX mentioned before but CanSetCookie() definitely reads better. Also, we already have HasSelection() which does not mean that we support selection. So finally I'd vote for CanXXX().

[wxtrac]

2012-10-03 11:29:24: evstevemd (Stefano) commented

---

Replying to [comment:17 steve_lamerton]:

Although the patch still doesn't apply cleanly for me (the trac preview not working is a good indication of this) I'll try and clean it up tonight and post my changes back here so you can check if they seem reasonable. I was also looking at adding OSX support, it looks like we can implement most of the API using NSHTTPCookieStorage and NSHTTPCookit.

I have no Mac computer so a bit hard to say authoritatively But a good finding!

However as with Windows (as far as I know) we can only access the global cookie jar and not create our own local one so the API might need some small adjustments. I'll try and incorperate them into my changes.

Since there is no creation of Jar here here are my suggestions

• SetCookieJar(wxWebViewCookieJar* jar) should load this global cookie jar which should initially be unloaded (hence NULL). If the global jar is already loaded (checked with HasCookieJar() ) then it will do nothing. This will mimick creation of the Jar in other backends
• GetCookieJar() will return the Instance of wxWebViewCookieJar with the global Jar
• HasCookieJar() const; will check if the global Jar is loaded as you have suggested above i.e return m_cookirJar != NULL

[wxtrac]

2012-10-03 11:35:54: evstevemd (Stefano) commented

---

Replying to [comment:23 vadz]:

Replying to [comment:21 steve_lamerton]:

Replying to [comment:19 vadz]:

Replying to [comment:18 steve_lamerton]:

• Used utf8_str rather than mb_str(wxConvUTF8)
  Notice that there are still a few occurrences of the latter remaining in webview_cookie_jar.cpp.
  Yes, I had to put them back, it didn't work.
  Err, how so? utf8_str() really should be used, not only it's more clear but potentially (in wxUSE_UNICODE_UTF8 build) more efficient too.
  Probably HasSetableCookieCar or CanSetCookieJar or something similar, better name suggestions welcome!
  I'm not sure here... HasXXX() is more consistent with wxHAS_XXX mentioned before but CanSetCookie() definitely reads better. Also, we already have HasSelection() which does not mean that we support selection. So finally I'd vote for CanXXX().

I don't think this is consistent either. My reasons being this: CanSetCookie sounds like there is a place where user cannot set the jar. That is not really the case. The case is that user in one of backends can only set jar which is available and global. I believe the method I suggested above is better. But then I'm up for correction :)

[wxtrac]

2012-10-03 17:50:24: @sjlamerton commented

---

Replying to [comment:23 vadz]:

Err, how so? utf8_str() really should be used, not only it's more clear but potentially (in wxUSE_UNICODE_UTF8 build) more efficient too.

Please ignore my comment here, I have no idea why this didn't work last night, presumably a typo.

Replying to [comment:25 evstevemd]:

I don't think this is consistent either. My reasons being this: CanSetCookie sounds like there is a place where user cannot set the jar. That is not really the case. The case is that user in one of backends can only set jar which is available and global. I believe the method I suggested above is better. But then I'm up for correction :)

I think the problem is that under OSX and MSW the backend always has its own global cookie jar, we cannot choose not to have it. As such we cannot set the cookie jar on those platforms. So I was thinking of an API similar to:

if(!browser->HasCookieJar() && browser->CanSetCookieJar())
{
    browser->SetCookieJar(new wxWebViewCookieJar("cookies.txt"));
}

At that point we then know that we have a valid cookie jar and can access it as normal with GetCookieJar().

[wxtrac]

2012-10-04 11:27:34: @sjlamerton uploaded file cookiegtk3.patch (11.7 KiB)

[wxtrac]

2012-10-04 11:29:15: @sjlamerton commented

---

I have made the various mentioned improvements and uploaded a new revision of the patch. The names of the cookie jar files should probably be changes but otherwise it is nearly there.

[wxtrac]

2012-10-04 12:19:28: evstevemd (Stefano) commented

---

Replying to [comment:26 steve_lamerton]:

[comment:25 evstevemd]:

I don't think this is consistent either. My reasons being this: CanSetCookie sounds like there is a place where user cannot set the jar. That is not really the case. The case is that user in one of backends can only set jar which is available and global. I believe the method I suggested above is better. But then I'm up for correction :)
I think the problem is that under OSX and MSW the backend always has its own global cookie jar, we cannot choose not to have it. As such we cannot set the cookie jar on those platforms. So I was thinking of an API similar to: if(!browser->HasCookieJar() && browser->CanSetCookieJar()) { browser->SetCookieJar(new wxWebViewCookieJar("cookies.txt")); } At that point we then know that we have a valid cookie jar and can access it as normal with GetCookieJar().

I see. That will be nice but needs a "good" documentation to avoid confusion which I believe we can do!

[wxtrac]

2012-11-25 23:56:50: @vadz commented

---

I don't think this is 2.9.5-critical.