From 248d5720774923c555e8a4af09f1a140d2102664 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Mon, 27 Nov 2017 08:57:14 +0530 Subject: [PATCH] REVIEW: Subscribe to cookie store for changes in place of CookieMonsterDelegate --- atom/browser/api/atom_api_cookies.cc | 22 ++++----- atom/browser/api/atom_api_cookies.h | 15 +++--- atom/browser/atom_browser_context.cc | 7 +-- atom/browser/atom_browser_context.h | 8 ---- atom/browser/net/atom_cookie_delegate.cc | 44 ----------------- atom/browser/net/atom_cookie_delegate.h | 48 ------------------- brightray/browser/net/cookie_details.h | 27 +++++++++++ .../browser/url_request_context_getter.cc | 34 ++++++++++++- .../browser/url_request_context_getter.h | 17 ++++++- brightray/filenames.gypi | 1 + filenames.gypi | 2 - 11 files changed, 94 insertions(+), 131 deletions(-) delete mode 100644 atom/browser/net/atom_cookie_delegate.cc delete mode 100644 atom/browser/net/atom_cookie_delegate.h create mode 100644 brightray/browser/net/cookie_details.h diff --git a/atom/browser/api/atom_api_cookies.cc b/atom/browser/api/atom_api_cookies.cc index 9f58922fd0a..0ba53714733 100644 --- a/atom/browser/api/atom_api_cookies.cc +++ b/atom/browser/api/atom_api_cookies.cc @@ -20,7 +20,6 @@ #include "net/url_request/url_request_context.h" #include "net/url_request/url_request_context_getter.h" -using atom::AtomCookieDelegate; using content::BrowserThread; namespace mate { @@ -238,17 +237,16 @@ void SetCookieOnIO(scoped_refptr getter, } // namespace -Cookies::Cookies(v8::Isolate* isolate, - AtomBrowserContext* browser_context) - : request_context_getter_(browser_context->url_request_context_getter()), - cookie_delegate_(browser_context->cookie_delegate()) { +Cookies::Cookies(v8::Isolate* isolate, AtomBrowserContext* browser_context) + : request_context_getter_(browser_context->url_request_context_getter()) { Init(isolate); - cookie_delegate_->AddObserver(this); + cookie_change_subscription_ = + browser_context->url_request_context_getter() + ->RegisterCookieChangeCallback( + base::Bind(&Cookies::OnCookieChanged, base::Unretained(this))); } -Cookies::~Cookies() { - cookie_delegate_->RemoveObserver(this); -} +Cookies::~Cookies() {} void Cookies::Get(const base::DictionaryValue& filter, const GetCallback& callback) { @@ -283,10 +281,8 @@ void Cookies::FlushStore(const base::Closure& callback) { base::Bind(FlushCookieStoreOnIOThread, getter, callback)); } -void Cookies::OnCookieChanged(const net::CanonicalCookie& cookie, - bool removed, - net::CookieStore::ChangeCause cause) { - Emit("changed", cookie, cause, removed); +void Cookies::OnCookieChanged(const brightray::CookieDetails* details) { + Emit("changed", *(details->cookie), details->cause, details->removed); } diff --git a/atom/browser/api/atom_api_cookies.h b/atom/browser/api/atom_api_cookies.h index d20dab8394c..47922f343e4 100644 --- a/atom/browser/api/atom_api_cookies.h +++ b/atom/browser/api/atom_api_cookies.h @@ -8,8 +8,8 @@ #include #include "atom/browser/api/trackable_object.h" -#include "atom/browser/net/atom_cookie_delegate.h" #include "base/callback.h" +#include "brightray/browser/net/cookie_details.h" #include "native_mate/handle.h" #include "net/cookies/canonical_cookie.h" @@ -27,8 +27,7 @@ class AtomBrowserContext; namespace api { -class Cookies : public mate::TrackableObject, - public AtomCookieDelegate::Observer { +class Cookies : public mate::TrackableObject { public: enum Error { SUCCESS, @@ -55,14 +54,14 @@ class Cookies : public mate::TrackableObject, void Set(const base::DictionaryValue& details, const SetCallback& callback); void FlushStore(const base::Closure& callback); - // AtomCookieDelegate::Observer: - void OnCookieChanged(const net::CanonicalCookie& cookie, - bool removed, - net::CookieStore::ChangeCause cause) override; + // brightray::URLRequestContextGetter subscription: + void OnCookieChanged(const brightray::CookieDetails*); private: net::URLRequestContextGetter* request_context_getter_; - scoped_refptr cookie_delegate_; + std::unique_ptr< + base::CallbackList::Subscription> + cookie_change_subscription_; DISALLOW_COPY_AND_ASSIGN(Cookies); }; diff --git a/atom/browser/atom_browser_context.cc b/atom/browser/atom_browser_context.cc index 07b351b551b..33ff92d2f13 100644 --- a/atom/browser/atom_browser_context.cc +++ b/atom/browser/atom_browser_context.cc @@ -70,8 +70,7 @@ std::string RemoveWhitespace(const std::string& str) { AtomBrowserContext::AtomBrowserContext(const std::string& partition, bool in_memory, const base::DictionaryValue& options) - : brightray::BrowserContext(partition, in_memory), - cookie_delegate_(new AtomCookieDelegate) { + : brightray::BrowserContext(partition, in_memory) { // Construct user agent string. Browser* browser = Browser::Get(); std::string name = RemoveWhitespace(browser->GetName()); @@ -108,10 +107,6 @@ AtomBrowserContext::CreateNetworkDelegate() { return base::MakeUnique(); } -net::CookieMonsterDelegate* AtomBrowserContext::CreateCookieDelegate() { - return cookie_delegate(); -} - std::string AtomBrowserContext::GetUserAgent() { return user_agent_; } diff --git a/atom/browser/atom_browser_context.h b/atom/browser/atom_browser_context.h index 359d3d6b71a..8f99a5e7410 100644 --- a/atom/browser/atom_browser_context.h +++ b/atom/browser/atom_browser_context.h @@ -8,9 +8,7 @@ #include #include -#include "atom/browser/net/atom_cookie_delegate.h" #include "brightray/browser/browser_context.h" -#include "net/cookies/cookie_monster.h" namespace atom { @@ -33,7 +31,6 @@ class AtomBrowserContext : public brightray::BrowserContext { // brightray::URLRequestContextGetter::Delegate: std::unique_ptr CreateNetworkDelegate() override; - net::CookieMonsterDelegate* CreateCookieDelegate() override; std::string GetUserAgent() override; std::unique_ptr CreateURLRequestJobFactory( content::ProtocolHandlerMap* protocol_handlers) override; @@ -52,9 +49,6 @@ class AtomBrowserContext : public brightray::BrowserContext { void RegisterPrefs(PrefRegistrySimple* pref_registry) override; AtomBlobReader* GetBlobReader(); - AtomCookieDelegate* cookie_delegate() const { - return cookie_delegate_.get(); - } protected: AtomBrowserContext(const std::string& partition, bool in_memory, @@ -69,8 +63,6 @@ class AtomBrowserContext : public brightray::BrowserContext { std::string user_agent_; bool use_cache_; - scoped_refptr cookie_delegate_; - DISALLOW_COPY_AND_ASSIGN(AtomBrowserContext); }; diff --git a/atom/browser/net/atom_cookie_delegate.cc b/atom/browser/net/atom_cookie_delegate.cc deleted file mode 100644 index b94f396d981..00000000000 --- a/atom/browser/net/atom_cookie_delegate.cc +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#include "atom/browser/net/atom_cookie_delegate.h" - -#include "content/public/browser/browser_thread.h" - -namespace atom { - -AtomCookieDelegate::AtomCookieDelegate() { -} - -AtomCookieDelegate::~AtomCookieDelegate() { -} - -void AtomCookieDelegate::AddObserver(Observer* observer) { - observers_.AddObserver(observer); -} - -void AtomCookieDelegate::RemoveObserver(Observer* observer) { - observers_.RemoveObserver(observer); -} - -void AtomCookieDelegate::NotifyObservers( - const net::CanonicalCookie& cookie, - bool removed, - net::CookieStore::ChangeCause cause) { - for (Observer& observer : observers_) - observer.OnCookieChanged(cookie, removed, cause); -} - -void AtomCookieDelegate::OnCookieChanged( - const net::CanonicalCookie& cookie, - bool removed, - net::CookieStore::ChangeCause cause) { - content::BrowserThread::PostTask( - content::BrowserThread::UI, - FROM_HERE, - base::Bind(&AtomCookieDelegate::NotifyObservers, - this, cookie, removed, cause)); -} - -} // namespace atom diff --git a/atom/browser/net/atom_cookie_delegate.h b/atom/browser/net/atom_cookie_delegate.h deleted file mode 100644 index 8c58aa6ada0..00000000000 --- a/atom/browser/net/atom_cookie_delegate.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#ifndef ATOM_BROWSER_NET_ATOM_COOKIE_DELEGATE_H_ -#define ATOM_BROWSER_NET_ATOM_COOKIE_DELEGATE_H_ - -#include "base/observer_list.h" -#include "net/cookies/cookie_monster.h" - -namespace atom { - -class AtomCookieDelegate : public net::CookieMonsterDelegate { - public: - AtomCookieDelegate(); - ~AtomCookieDelegate() override; - - class Observer { - public: - virtual void OnCookieChanged(const net::CanonicalCookie& cookie, - bool removed, - net::CookieStore::ChangeCause cause) {} - protected: - virtual ~Observer() {} - }; - - void AddObserver(Observer* observer); - void RemoveObserver(Observer* observer); - - // net::CookieMonsterDelegate: - void OnCookieChanged(const net::CanonicalCookie& cookie, - bool removed, - net::CookieStore::ChangeCause cause) override; - - - private: - base::ObserverList observers_; - - void NotifyObservers(const net::CanonicalCookie& cookie, - bool removed, - net::CookieStore::ChangeCause cause); - - DISALLOW_COPY_AND_ASSIGN(AtomCookieDelegate); -}; - -} // namespace atom - -#endif // ATOM_BROWSER_NET_ATOM_COOKIE_DELEGATE_H_ diff --git a/brightray/browser/net/cookie_details.h b/brightray/browser/net/cookie_details.h new file mode 100644 index 00000000000..42a5b7eb6b6 --- /dev/null +++ b/brightray/browser/net/cookie_details.h @@ -0,0 +1,27 @@ +// Copyright (c) 2017 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_ +#define BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_ + +#include "base/macros.h" +#include "net/cookies/cookie_store.h" + +namespace brightray { + +struct CookieDetails { + public: + CookieDetails(const net::CanonicalCookie* cookie_copy, + bool is_removed, + net::CookieStore::ChangeCause cause) + : cookie(cookie_copy), removed(is_removed), cause(cause) {} + + const net::CanonicalCookie* cookie; + bool removed; + net::CookieStore::ChangeCause cause; +}; + +} // namespace brightray + +#endif // BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_ diff --git a/brightray/browser/url_request_context_getter.cc b/brightray/browser/url_request_context_getter.cc index 1fc5fc1f8d0..70d19afce62 100644 --- a/brightray/browser/url_request_context_getter.cc +++ b/brightray/browser/url_request_context_getter.cc @@ -13,6 +13,7 @@ #include "base/threading/sequenced_worker_pool.h" #include "base/threading/worker_pool.h" #include "brightray/browser/browser_client.h" +#include "brightray/browser/net/cookie_details.h" #include "brightray/browser/net/devtools_network_controller_handle.h" #include "brightray/browser/net/devtools_network_transaction_factory.h" #include "brightray/browser/net/require_ct_delegate.h" @@ -29,7 +30,6 @@ #include "net/cert/ct_log_verifier.h" #include "net/cert/ct_policy_enforcer.h" #include "net/cert/multi_log_ct_verifier.h" -#include "net/cookies/cookie_monster.h" #include "net/dns/mapped_host_resolver.h" #include "net/http/http_auth_filter.h" #include "net/http/http_auth_handler_factory.h" @@ -155,6 +155,32 @@ URLRequestContextGetter::URLRequestContextGetter( URLRequestContextGetter::~URLRequestContextGetter() { } +std::unique_ptr::Subscription> +URLRequestContextGetter::RegisterCookieChangeCallback( + const base::Callback& cb) { + return cookie_change_sub_list_.Add(cb); +} + +void URLRequestContextGetter::NotifyCookieChange( + const net::CanonicalCookie& cookie, + bool removed, + net::CookieStore::ChangeCause cause) { + CookieDetails cookie_details(&cookie, removed, cause); + cookie_change_sub_list_.Notify(&cookie_details); +} + +void URLRequestContextGetter::OnCookieChanged( + const net::CanonicalCookie& cookie, + net::CookieStore::ChangeCause cause) { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); + + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::BindOnce(&URLRequestContextGetter::NotifyCookieChange, this, cookie, + !(cause == net::CookieStore::ChangeCause::INSERTED), + cause)); +} + net::HostResolver* URLRequestContextGetter::host_resolver() { return url_request_context_->host_resolver(); } @@ -188,6 +214,12 @@ net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() { std::unique_ptr cookie_store = content::CreateCookieStore(cookie_config); storage_->set_cookie_store(std::move(cookie_store)); + // Cookie store will outlive notifier by order of declaration + // in the header. + cookie_change_sub_ = + url_request_context_->cookie_store()->AddCallbackForAllChanges( + base::Bind(&URLRequestContextGetter::OnCookieChanged, this)); + storage_->set_channel_id_service(base::MakeUnique( new net::DefaultChannelIDStore(nullptr))); diff --git a/brightray/browser/url_request_context_getter.h b/brightray/browser/url_request_context_getter.h index a15fb7ffe0e..880473ee7e5 100644 --- a/brightray/browser/url_request_context_getter.h +++ b/brightray/browser/url_request_context_getter.h @@ -8,10 +8,10 @@ #include #include +#include "base/callback_list.h" #include "base/files/file_path.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/content_browser_client.h" -#include "net/cookies/cookie_monster.h" #include "net/cookies/cookie_store.h" #include "net/http/http_cache.h" #include "net/http/transport_security_state.h" @@ -38,6 +38,7 @@ namespace brightray { class RequireCTDelegate; class DevToolsNetworkControllerHandle; class NetLog; +struct CookieDetails; class URLRequestContextGetter : public net::URLRequestContextGetter { public: @@ -71,6 +72,17 @@ class URLRequestContextGetter : public net::URLRequestContextGetter { content::URLRequestInterceptorScopedVector protocol_interceptors); virtual ~URLRequestContextGetter(); + // Register callbacks that needs to notified on any cookie store changes. + std::unique_ptr::Subscription> + RegisterCookieChangeCallback( + const base::Callback& cb); + void NotifyCookieChange(const net::CanonicalCookie& cookie, + bool removed, + net::CookieStore::ChangeCause cause); + // net::CookieStore::CookieChangedCallback implementation. + void OnCookieChanged(const net::CanonicalCookie& cookie, + net::CookieStore::ChangeCause cause); + // net::URLRequestContextGetter: net::URLRequestContext* GetURLRequestContext() override; scoped_refptr GetNetworkTaskRunner() @@ -102,9 +114,12 @@ class URLRequestContextGetter : public net::URLRequestContextGetter { std::unique_ptr host_mapping_rules_; std::unique_ptr http_auth_preferences_; std::unique_ptr http_network_session_; + std::unique_ptr + cookie_change_sub_; content::ProtocolHandlerMap protocol_handlers_; content::URLRequestInterceptorScopedVector protocol_interceptors_; + base::CallbackList cookie_change_sub_list_; net::URLRequestJobFactory* job_factory_; // weak ref DISALLOW_COPY_AND_ASSIGN(URLRequestContextGetter); diff --git a/brightray/filenames.gypi b/brightray/filenames.gypi index 6a7d91f135c..cfce6df6502 100644 --- a/brightray/filenames.gypi +++ b/brightray/filenames.gypi @@ -47,6 +47,7 @@ 'browser/media/media_device_id_salt.h', 'browser/media/media_stream_devices_controller.cc', 'browser/media/media_stream_devices_controller.h', + 'browser/net/cookie_details.h', 'browser/net/devtools_network_conditions.cc', 'browser/net/devtools_network_conditions.h', 'browser/net/devtools_network_controller.cc', diff --git a/filenames.gypi b/filenames.gypi index c2f6d44c5e0..da899cb2696 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -261,8 +261,6 @@ 'atom/browser/net/asar/url_request_asar_job.h', 'atom/browser/net/atom_cert_verifier.cc', 'atom/browser/net/atom_cert_verifier.h', - 'atom/browser/net/atom_cookie_delegate.cc', - 'atom/browser/net/atom_cookie_delegate.h', 'atom/browser/net/atom_network_delegate.cc', 'atom/browser/net/atom_network_delegate.h', 'atom/browser/net/atom_url_request.cc',