Opened 8 years ago

Last modified 8 years ago

#14528 new enhancement

Adding Cookies Support (wxGTK )

Reported by: evstevemd Owned by:
Priority: normal Milestone:
Component: WebView Version: stable-latest
Keywords: cookies-support Cc:
Blocked By: Blocking:
Patch: yes

Description

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!

Attachments (5)

wxwebview_cookies_clean.patch download (13.9 KB) - added by evstevemd 8 years ago.
Working patch
wxwebview_cookies_latest.patch download (12.5 KB) - added by evstevemd 8 years ago.
cookiegtk.patch download (9.8 KB) - added by steve_lamerton 8 years ago.
Slightly cleaned up patch
cookiegtk2.patch download (10.3 KB) - added by steve_lamerton 8 years ago.
Fix compilation, more changes
cookiegtk3.patch download (11.7 KB) - added by steve_lamerton 8 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 8 years ago by PB

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. :)

comment:2 Changed 8 years ago by evstevemd

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

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?

comment:3 Changed 8 years ago by vadz

  • Status changed from new to infoneeded_new

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!

comment:4 Changed 8 years ago by evstevemd

Thanks vadim,

I'll do that!

comment:5 Changed 8 years ago by evstevemd

  • Status changed from infoneeded_new to new

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.

comment:6 Changed 8 years ago by lanurmi

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.

comment:7 Changed 8 years ago by evstevemd

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!

comment:8 Changed 8 years ago by vadz

  • Status changed from new to infoneeded_new

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...

comment:9 Changed 8 years ago by evstevemd

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!

comment:10 Changed 8 years ago by vadz

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!

comment:11 Changed 8 years ago by evstevemd

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

comment:12 Changed 8 years ago by evstevemd

  • Status changed from infoneeded_new to new

Changed 8 years ago by evstevemd

Working patch

comment:13 Changed 8 years ago by steve_lamerton

  • Status changed from new to infoneeded_new

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.

Changed 8 years ago by evstevemd

comment:14 Changed 8 years ago by evstevemd

  • Status changed from infoneeded_new to new

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

comment:15 Changed 8 years ago by steve_lamerton

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

comment:16 Changed 8 years ago by evstevemd

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 :)

comment:17 follow-up: Changed 8 years ago by steve_lamerton

  • Status changed from new to confirmed

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.

comment:18 follow-ups: Changed 8 years ago by steve_lamerton

  • Status changed from confirmed to infoneeded_new

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.

Changed 8 years ago by steve_lamerton

Slightly cleaned up patch

Changed 8 years ago by steve_lamerton

Fix compilation, more changes

comment:19 in reply to: ↑ 18 ; follow-up: Changed 8 years ago by vadz

  • Status changed from infoneeded_new to new

Replying to 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.

comment:20 in reply to: ↑ 18 ; follow-up: Changed 8 years ago by evstevemd

Replying to 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!

comment:21 in reply to: ↑ 19 ; follow-up: Changed 8 years ago by steve_lamerton

Replying to vadz:

Replying to 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!

comment:22 in reply to: ↑ 20 Changed 8 years ago by steve_lamerton

Replying to evstevemd:

Replying to 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!

comment:23 in reply to: ↑ 21 ; follow-ups: Changed 8 years ago by vadz

Replying to steve_lamerton:

Replying to vadz:

Replying to 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().

comment:24 in reply to: ↑ 17 Changed 8 years ago by evstevemd

Replying to 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

comment:25 in reply to: ↑ 23 Changed 8 years ago by evstevemd

Replying to vadz:

Replying to steve_lamerton:

Replying to vadz:

Replying to 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 :)

comment:26 in reply to: ↑ 23 ; follow-up: Changed 8 years ago by steve_lamerton

Replying to 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 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().

Changed 8 years ago by steve_lamerton

comment:27 Changed 8 years ago by steve_lamerton

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.

comment:28 in reply to: ↑ 26 Changed 8 years ago by evstevemd

Replying to steve_lamerton:

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!

comment:29 Changed 8 years ago by vadz

  • Milestone 2.9.5 deleted

I don't think this is 2.9.5-critical.

Note: See TracTickets for help on using tickets.