Support custom handler for CefCookieManager

[GoogleCodeExporter]

Introduce a new CefCookieHandler interface that can be implemented by the 
client and passed to a CefCookieManager static factory method. This interface 
will allow the client to handle all logic related to storing and retrieving 
cookies.

Original issue reported on code.google.com by magreenb...@gmail.com on 22 Jan 2013 at 4:09

[GoogleCodeExporter]

Hi,

Here's a proposal patch for a custom cookie handler implementation. The custome 
cookie handler can be passed along the creation of the CookieManager using a 
new factory method : CreateManagerWithHandler. 

I defined two methods in interface of the cookie handler : 
- bool canSetCookie(url, cefCookie)
- bool canGetCookie(url, cefCookie)

That allow the implementation to inspect cookie data and url before a cookie
is being read from or set in the cookie store. A return value of 'false' will 
prevent the action to be done (set or get)

Can you please review the code and integrate in CEF if it fits your 
requirements?
The code has been done on the branch relase branch 1180 of CEF

Regards
Kevin

Original comment by kevin.v...@gmail.com on 27 Jan 2013 at 9:18

[GoogleCodeExporter]

Meanwhile I created a patch, with changed files only (it not includes the 
translator results which has created a bunch a lot useless entries in the 
previous patch 

Original comment by kevin.v...@gmail.com on 12 Feb 2013 at 8:16

Attachments:

• new.cookiehandler.diff

[GoogleCodeExporter]

@comment#2: Thanks for the patch. Not including translator results is the right 
approach.

A few comments:

1. Please follow the Chromium/CEF style requirements (basically follow the 
style of the files you're editing). Use 2 spaces instead of tabs, wrap 
characters at 80 lines, etc. See 
http://dev.chromium.org/developers/coding-style for the full list of 
requirements.

2. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=86500
02000&name=new.cookiehandler.diff&token=DAevhCnGzuYRO_c5YVyO2lbb4Ko%3A1360795766
742#36
+  static CefRefPtr<CefCookieManager> CreateManagerWithHandler(const CefString& 
path, CefRefPtr<CefCookieHandler> handler);

Let's introduce a SetHandler() method instead of adding a special factory 
method. This will allow us to set a handler on the global cookie manager as 
well. For the global cookie manager the handler reference can be kept in 
CefURLRequestContextGetter so that it outlives any single global 
CefCookieManagerImpl instance.

Original comment by magreenb...@gmail.com on 13 Feb 2013 at 11:03

• Added labels: NextRelease

[GoogleCodeExporter]

@comment#2: Also, please submit patches against trunk instead of a release 
branch.

Original comment by magreenb...@gmail.com on 13 Feb 2013 at 11:05

[GoogleCodeExporter]

@comment#2: Also, please write unit tests for the new methods.

Original comment by magreenb...@gmail.com on 13 Feb 2013 at 11:08

[GoogleCodeExporter]

Hi

Following your comments, I reimplemented the previous patch
Waiting for your feedbacks

Regards
Kevin

Original comment by kevin.v...@gmail.com on 14 Mar 2013 at 12:55

Attachments:

• cookie_handler_trunk.diff

[GoogleCodeExporter]

@comment#6: Sorry for the delay in responding. Some comments:

1. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#24

+  virtual void SetCookieHandler(CefRefPtr<CefCookieHandler> cookieHandler) =0;

We can just call this |SetHandler| since from the context it's clear that the 
handler is a cookie handler.

2. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#270

+  // if no manager are in the request check with the global manager
+  return reinterpret_cast<CefCookieManagerImpl*>
+    (CefCookieManager::GetGlobalManager().get())->
+    CanGetCookies(request.url(), cookie_list);

We shouldn't create a CefCookieManager just for the purpose of calling this 
method. Consider making the CanSetCookie and CanGetCookies methods static and 
passing in the CefRefPtr<CefCookieHandler> as the first argument.

3. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#383

+class TestCookieHandler : public CefCookieHandler {
+
+  virtual bool CanSetCookie(const CefString& url, const CefCookie& cookie) {
+    return false;
+  }
+
+  virtual bool CanGetCookie(const CefString& url, const CefCookie& cookie) {
+    return true;
+  }
+
+  IMPLEMENT_REFCOUNTING(TestCookieHandler);
+};

We should test all 4 conditions as separate tests:
A. Try to set cookie, CanSetCookie returns true.
B. Try to set cookie, CanSetCookie returns false.
C. Try to get cookie, CanGetCookie returns true.
D. Try to get cookie, CanGetCookie returns false.

And various style-related issues:

4. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#119

+      getter->SetCookieStoragePath(new_path, persist_session_cookies);      

Adding some unnecessary white space here.

5. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#128

+void CefCookieManagerImpl::SetCookieHandler(CefRefPtr<CefCookieHandler> 
cookieHandler) {
+  
+  if (CEF_CURRENTLY_ON_IOT()) {

Don't introduce unnecessary lines. Same for other methods.

6. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#211

+  virtual void SetCookieHandler(CefRefPtr<CefCookieHandler> cookieHandler) 
+    OVERRIDE;
+  virtual CefRefPtr<CefCookieHandler> GetCookieHandler() 
+    {return cookie_handler_; }

Indent the 2nd line 4 spaces. Same for 2nd+ lines in url_network_delegate.cc 
and cookie_unittest.cc.

7. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#269

+  // if no manager are in the request check with the global manager

Please use proper grammar and punctuation for comments.

8. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#365

+             base::WaitableEvent* event, bool expectSetFailure = false) {

Put additional arguments on a new line.

9. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
006000&name=cookie_handler_trunk.diff&token=RmkJJI7ckKkibFUy2kqNHRFENt0%3A136518
3533982#458

+  //CreateCookieWithUrl(manager, cookie, true, false, 
"http://www.allowCookie.com", event);
+  //GetCookie(manager, cookie, true, event, false);

Don't leave commented-out lines.

Original comment by magreenb...@gmail.com on 5 Apr 2013 at 5:59

[GoogleCodeExporter]

Kevin, are you planning to update your patch?

Original comment by magreenb...@gmail.com on 8 May 2013 at 6:29

• Removed labels: NextRelease

[GoogleCodeExporter]

Hi yes, for sure, we need this one in a release :)
I have to ask some time to my kind manager, but I am sure he already agrees

Regards 

Original comment by kevin.v...@gmail.com on 10 May 2013 at 8:01

[GoogleCodeExporter]

Why limit ourselves to intercepting/authorizing cookie Set/Get when we could 
let the embedder provide a fully custom CookieMonster implementation in place 
of the default one used?

Original comment by polu.sta...@gmail.com on 28 Jun 2013 at 2:26

[GoogleCodeExporter]

Hi 

I updated the patch. 
I did not follow your comment #2 because I think we can let the user choose to 
set its CookieHandler implementation either in the global CookieManager or its 
own CookieManager implementation.

I added more tests as you suggested, I test the four possible cases you 
mentioned.

Coding conventions are respected, in fact I found cef/tools/check_style.bat 
which is very useful ;)

Best Regards
Kevin

Original comment by kevin.v...@gmail.com on 6 Sep 2013 at 9:03

Attachments:

• cookie_handler_trunk-06-09-13.diff

[GoogleCodeExporter]

Hi, 

    I saw that a new release of CEF3 3.1650.1562 has been done on january 13th. 
    Why didn't you include this fix? 
    From what I see on the release notes of the latest version, another fix #issue 1044 concerning cookie has been included, I was expecting to have this patch included too since it works and fit the requirements.
    Can you please give us an update here?

Best Regards
Christian

Original comment by christia...@gmail.com on 3 Feb 2014 at 9:33

[GoogleCodeExporter]

@comment#11: Thanks for the updated patch. Some comments:

1. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#38

+///
+// Interface to implement as callback object where is CEF
+// asks permission to set a cookie in the cookie store or
+// to get a cookie from the cookie store.
+///
+/*--cef(source=client)--*/
+class CefCookieHandler : public virtual CefBase {

Need to document that the methods of this class will be called on the IO thread.

2. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#26

+  ///
+  // Clear the cookie handler.
+  ///
+  /*--cef()--*/
+  virtual void ClearCookieHandler() =0;

No need for a separate clear method. Just pass NULL to the SetHandler() method.

3. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#222

+  virtual CefRefPtr<CefCookieHandler> GetCookieHandler()
+      {return cookie_handler_;}
+  virtual bool CanSetCookie(const GURL& url, const std::string& cookie_line);
+  virtual bool CanGetCookies(const GURL& url,
+                             const net::CookieList& cookie_list);

These methods don't override anything and nothing overrides them. They 
shouldn't be virtual.

4. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#341

+  void SetHandler(CefRefPtr<CefCookieHandler> cookieHandler);

In URLRequestContextGetter it makes sense to call the method SetCookieHandler 
since that class is responsible for a lot of different functionality.

5. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#364

-             base::WaitableEvent* event) {
+             base::WaitableEvent* event, bool expectSetFailure = false) {

Put the new argument on its own line. Same below for SetCookies.

6. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#470

+}
 }  // namespace

Need an empty line between end of function and end of namespace.

7. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#503

+class TestCookieHandler : public CefCookieHandler {
+ public:
+  virtual bool CanSetCookie(const CefString& url, const CefCookie& cookie) {
+    // forbids url1 to set a cookie
+    if (url == kCookieUrl1)
+      return false;
+    return true;
+  }
+
+  virtual bool CanGetCookie(const CefString& url, const CefCookie& cookie) {
+    // forbids kCookieUrl4 to read the cookies
+    if (url == kCookieUrl4)
+      return false;
+    return true;
+  }

Mark overridden methods with OVERRIDE.

8. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#525

+    virtual CefRefPtr<CefCookieManager> GetCookieManager() OVERRIDE {
+      EXPECT_TRUE(CefCurrentlyOn(TID_IO));
+      CefCookieManager::GetGlobalManager()->SetHandler(new 
TestCookieHandler());
+      return CefCookieManager::GetGlobalManager();
+    }

The test puts cookies into the global manager but doesn't remove them. Tests 
need to clean up after themselves.

9. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#644

+    } else if (url == kCookieUrl3) {
+    frame->LoadURL(kCookieUrl4);
+    } else {

Wrong indent.

10. You're only testing CefCookieHandler with the global cookie manager. You 
should also test with a user-created cookie manager.

11. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=865&aid=8650
011000&name=cookie_handler_trunk-06-09-13.diff&token=Ai_vaeLlL6eX0sVTg1xxshLrzQk
%3A1392064715768#504

+    // forbids url1 to set a cookie

Comments should be proper sentences with capitalization and periods. Same in 
other places.

Original comment by magreenb...@gmail.com on 10 Feb 2014 at 9:06

[GoogleCodeExporter]

CEF is transitioning from Google Code to Bitbucket project hosting. If you 
would like to continue receiving notifications on this issue please add 
yourself as a Watcher at the new location: 
https://bitbucket.org/chromiumembedded/cef/issue/865

Original comment by magreenb...@gmail.com on 14 Mar 2015 at 3:24