Issue 330733002: Move IdentityProvider usage from GCMDriverDesktop to GCMProfileService

Issue 330733002: Move IdentityProvider usage from GCMDriverDesktop to GCMProfileService (Closed)

Created:
6 years, 6 months ago by jianli

Modified:
6 years, 6 months ago

Reviewers:
bartfab (slow), Nicolas Zea, fgorski

CC:
chromium-reviews

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Move IdentityProvider usage from GCMDriverDesktop to GCMProfileService This is the 1st step towards removing sign-in enforcement for GCM. We also remove the logic to pass account IDs to check-in request since it seems to work fine without them and TokenService::GetAccounts returns the empty list. BUG=384041 TEST=existing and new tests R=bartfab@chromium.org, fgorski@chromium.org, zea@chromium.org TBR=jochen@chromium.org, rogerta@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277789

Patch Set 1 : Patch #

Total comments: 35

Patch Set 2 : Address feedback #

Total comments: 6

Patch Set 3 : Address more feedback #

Patch Set 4 : Sync #

Total comments: 4

Patch Set 5 : Sync plus address feedback #

Patch Set 6 : Fix after sync #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+203 lines, -334 lines)			Patch
M	chrome/browser/browser_process_impl.cc	View	1 2 3 4	3 chunks	+5 lines, -14 lines	0 comments	Download
M	chrome/browser/services/gcm/gcm_desktop_utils.h	View		2 chunks	+0 lines, -3 lines	0 comments	Download
M	chrome/browser/services/gcm/gcm_desktop_utils.cc	View		2 chunks	+0 lines, -2 lines	0 comments	Download
M	chrome/browser/services/gcm/gcm_profile_service.h	View	1 2	2 chunks	+11 lines, -0 lines	0 comments	Download
M	chrome/browser/services/gcm/gcm_profile_service.cc	View	1 2 3 4	3 chunks	+73 lines, -4 lines	0 comments	Download
M	chrome/browser/services/gcm/gcm_profile_service_unittest.cc	View	1	4 chunks	+34 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/webui/gcm_internals_ui.cc	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	components/gcm_driver/fake_gcm_client.h	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	components/gcm_driver/fake_gcm_client.cc	View	1 2 3 4	1 chunk	+0 lines, -1 line	0 comments	Download
M	components/gcm_driver/fake_gcm_driver.h	View	1 2 3 4	2 chunks	+2 lines, -1 line	0 comments	Download
M	components/gcm_driver/fake_gcm_driver.cc	View	1 2 3 4	2 chunks	+6 lines, -4 lines	0 comments	Download
M	components/gcm_driver/gcm_client.h	View	1 2 3 4	2 chunks	+0 lines, -2 lines	0 comments	Download
M	components/gcm_driver/gcm_client_impl.h	View	1	2 chunks	+0 lines, -2 lines	0 comments	Download
M	components/gcm_driver/gcm_client_impl.cc	View	1 2 3 4	3 chunks	+0 lines, -3 lines	0 comments	Download
M	components/gcm_driver/gcm_client_impl_unittest.cc	View	1 2 3 4	1 chunk	+0 lines, -1 line	0 comments	Download
M	components/gcm_driver/gcm_driver.h	View	1 2 3 4	2 chunks	+8 lines, -3 lines	0 comments	Download
M	components/gcm_driver/gcm_driver_android.h	View	1 2 3 4	2 chunks	+2 lines, -1 line	0 comments	Download
M	components/gcm_driver/gcm_driver_android.cc	View	1 2 3 4	2 chunks	+6 lines, -4 lines	0 comments	Download
M	components/gcm_driver/gcm_driver_desktop.h	View	1 2 3 4	7 chunks	+10 lines, -16 lines	0 comments	Download
M	components/gcm_driver/gcm_driver_desktop.cc	View	1 2 3 4 5	17 chunks	+38 lines, -64 lines	0 comments	Download
M	components/gcm_driver/gcm_driver_desktop_unittest.cc	View	1 2 3 4	7 chunks	+4 lines, -32 lines	0 comments	Download
M	google_apis/BUILD.gn	View		2 chunks	+0 lines, -9 lines	0 comments	Download
D	google_apis/gaia/dummy_identity_provider.h	View		1 chunk	+0 lines, -39 lines	0 comments	Download
D	google_apis/gaia/dummy_identity_provider.cc	View		1 chunk	+0 lines, -93 lines	0 comments	Download
M	google_apis/gcm/engine/checkin_request.h	View	1	3 chunks	+0 lines, -4 lines	0 comments	Download
M	google_apis/gcm/engine/checkin_request.cc	View		2 chunks	+0 lines, -9 lines	0 comments	Download
M	google_apis/gcm/engine/checkin_request_unittest.cc	View	1	5 chunks	+0 lines, -6 lines	0 comments	Download
M	google_apis/gcm/tools/mcs_probe.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	google_apis/google_apis.gyp	View		2 chunks	+0 lines, -8 lines	0 comments	Download

Messages

Total messages: 23 (0 generated)

Expand Messages | Collapse Messages

bartfab (slow)

lgtm https://codereview.chromium.org/330733002/diff/20001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/330733002/diff/20001/chrome/browser/browser_process_impl.cc#newcode95 chrome/browser/browser_process_impl.cc:95: #include "google_apis/gaia/identity_provider.h" Nit: No longer used. https://codereview.chromium.org/330733002/diff/20001/chrome/browser/browser_process_impl.cc#newcode1029 chrome/browser/browser_process_impl.cc:1029: ...

6 years, 6 months ago (2014-06-13 11:27:22 UTC) #2

jianli

https://codereview.chromium.org/330733002/diff/20001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/330733002/diff/20001/chrome/browser/browser_process_impl.cc#newcode95 chrome/browser/browser_process_impl.cc:95: #include "google_apis/gaia/identity_provider.h" On 2014/06/13 11:27:21, bartfab wrote: > Nit: ...

6 years, 6 months ago (2014-06-13 18:04:49 UTC) #3

fgorski

lgtm, but perhaps we could not modify the checkin request. https://codereview.chromium.org/330733002/diff/20001/google_apis/gcm/engine/checkin_request.cc File google_apis/gcm/engine/checkin_request.cc (left): https://codereview.chromium.org/330733002/diff/20001/google_apis/gcm/engine/checkin_request.cc#oldcode84 ...

6 years, 6 months ago (2014-06-13 18:17:10 UTC) #4

Nicolas Zea

https://codereview.chromium.org/330733002/diff/40001/chrome/browser/services/gcm/gcm_profile_service.h File chrome/browser/services/gcm/gcm_profile_service.h (right): https://codereview.chromium.org/330733002/diff/40001/chrome/browser/services/gcm/gcm_profile_service.h#newcode38 chrome/browser/services/gcm/gcm_profile_service.h:38: class GCMProfileService : public KeyedService, Agree with Filip, I ...

6 years, 6 months ago (2014-06-13 19:49:31 UTC) #5

Nicolas Zea

https://codereview.chromium.org/330733002/diff/40001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/330733002/diff/40001/chrome/browser/browser_process_impl.cc#newcode1029 chrome/browser/browser_process_impl.cc:1029: gcm_driver_->SignIn(); Why is this here? Isn't signin getting called ...

6 years, 6 months ago (2014-06-13 19:50:25 UTC) #6

jianli

On 2014/06/13 18:17:10, fgorski wrote: > lgtm, but perhaps we could not modify the checkin ...

6 years, 6 months ago (2014-06-13 21:57:54 UTC) #7

jianli

6 years, 6 months ago (2014-06-13 21:58:04 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jianli@chromium.org/330733002/80001

6 years, 6 months ago (2014-06-13 22:59:57 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jianli@chromium.org/330733002/80001

6 years, 6 months ago (2014-06-13 23:10:40 UTC) #14

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 6 months ago (2014-06-14 19:40:01 UTC) #15

commit-bot: I haz the power

Failed to apply patch for components/gcm_driver/gcm_driver_desktop.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

6 years, 6 months ago (2014-06-14 19:40:02 UTC) #16

Failed to apply patch for components/gcm_driver/gcm_driver_desktop.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/gcm_driver/gcm_driver_desktop.cc
  Hunk #1 FAILED at 16.
  Hunk #2 succeeded at 119 (offset 10 lines).
  Hunk #3 succeeded at 163 (offset 10 lines).
  Hunk #4 succeeded at 171 (offset 10 lines).
  Hunk #5 FAILED at 325.
  Hunk #6 succeeded at 376 (offset 24 lines).
  Hunk #7 succeeded at 450 (offset 24 lines).
  Hunk #8 succeeded at 554 (offset 24 lines).
  Hunk #9 succeeded at 595 (offset 28 lines).
  Hunk #10 succeeded at 605 (offset 28 lines).
  Hunk #11 succeeded at 620 (offset 28 lines).
  Hunk #12 succeeded at 630 (offset 28 lines).
  Hunk #13 succeeded at 651 (offset 28 lines).
  Hunk #14 succeeded at 663 (offset 28 lines).
  Hunk #15 succeeded at 727 with fuzz 2 (offset 64 lines).
  2 out of 15 hunks FAILED -- saving rejects to file
components/gcm_driver/gcm_driver_desktop.cc.rej

Patch:       components/gcm_driver/gcm_driver_desktop.cc
Index: components/gcm_driver/gcm_driver_desktop.cc
diff --git a/components/gcm_driver/gcm_driver_desktop.cc
b/components/gcm_driver/gcm_driver_desktop.cc
index
9f6a7c2c24b7996428458929cd64b6f2869ad4ba..d0758e18c16038e3b605ad1106e7458dd8a98ff4
100644
--- a/components/gcm_driver/gcm_driver_desktop.cc
+++ b/components/gcm_driver/gcm_driver_desktop.cc
@@ -16,7 +16,6 @@
 #include "components/gcm_driver/gcm_app_handler.h"
 #include "components/gcm_driver/gcm_client_factory.h"
 #include "components/gcm_driver/system_encryptor.h"
-#include "google_apis/gaia/oauth2_token_service.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace gcm {
@@ -109,7 +108,6 @@ class GCMDriverDesktop::IOWorker : public
GCMClient::Delegate {
       scoped_ptr<GCMClientFactory> gcm_client_factory,
       const GCMClient::ChromeBuildInfo& chrome_build_info,
       const base::FilePath& store_path,
-      const std::vector<std::string>& account_ids,
       const scoped_refptr<net::URLRequestContextGetter>& request_context,
       const scoped_refptr<base::SequencedTaskRunner> blocking_task_runner);
   void Start(const base::WeakPtr<GCMDriverDesktop>& service);
@@ -154,7 +152,6 @@ void GCMDriverDesktop::IOWorker::Initialize(
     scoped_ptr<GCMClientFactory> gcm_client_factory,
     const GCMClient::ChromeBuildInfo& chrome_build_info,
     const base::FilePath& store_path,
-    const std::vector<std::string>& account_ids,
     const scoped_refptr<net::URLRequestContextGetter>& request_context,
     const scoped_refptr<base::SequencedTaskRunner> blocking_task_runner) {
   DCHECK(io_thread_->RunsTasksOnCurrentThread());
@@ -163,7 +160,6 @@ void GCMDriverDesktop::IOWorker::Initialize(
 
   gcm_client_->Initialize(chrome_build_info,
                           store_path,
-                          account_ids,
                           blocking_task_runner,
                           request_context,
                           make_scoped_ptr<Encryptor>(new SystemEncryptor),
@@ -328,23 +324,19 @@ void GCMDriverDesktop::IOWorker::SetGCMRecording(bool
recording) {
 
 GCMDriverDesktop::GCMDriverDesktop(
     scoped_ptr<GCMClientFactory> gcm_client_factory,
-    scoped_ptr<IdentityProvider> identity_provider,
     const GCMClient::ChromeBuildInfo& chrome_build_info,
     const base::FilePath& store_path,
     const scoped_refptr<net::URLRequestContextGetter>& request_context,
     const scoped_refptr<base::SequencedTaskRunner>& ui_thread,
     const scoped_refptr<base::SequencedTaskRunner>& io_thread,
     const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner)
-    : gcm_enabled_(true),
+    : signed_in_(false),
+      gcm_started_(false),
+      gcm_enabled_(true),
       gcm_client_ready_(false),
-      identity_provider_(identity_provider.Pass()),
       ui_thread_(ui_thread),
       io_thread_(io_thread),
       weak_ptr_factory_(this) {
-  // Get the list of available accounts.
-  std::vector<std::string> account_ids;
-  account_ids = identity_provider_->GetTokenService()->GetAccounts();
-
   // Create and initialize the GCMClient. Note that this does not initiate the
   // GCM check-in.
   io_worker_.reset(new IOWorker(ui_thread, io_thread));
@@ -355,29 +347,36 @@ GCMDriverDesktop::GCMDriverDesktop(
                  base::Passed(&gcm_client_factory),
                  chrome_build_info,
                  store_path,
-                 account_ids,
                  request_context,
                  blocking_task_runner));
-
-  identity_provider_->AddObserver(this);
 }
 
 GCMDriverDesktop::~GCMDriverDesktop() {
 }
 
-void GCMDriverDesktop::OnActiveAccountLogin() {
-  EnsureStarted();
+void GCMDriverDesktop::Shutdown() {
+  DCHECK(ui_thread_->RunsTasksOnCurrentThread());
+  GCMDriver::Shutdown();
+  io_thread_->DeleteSoon(FROM_HERE, io_worker_.release());
 }
 
-void GCMDriverDesktop::OnActiveAccountLogout() {
-  CheckOut();
+void GCMDriverDesktop::OnSignedIn() {
+  signed_in_ = true;
+  EnsureStarted();
 }
 
-void GCMDriverDesktop::Shutdown() {
+void GCMDriverDesktop::Purge() {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
-  identity_provider_->RemoveObserver(this);
-  GCMDriver::Shutdown();
-  io_thread_->DeleteSoon(FROM_HERE, io_worker_.release());
+
+  // We still proceed with the check-out logic even if the check-in is not
+  // initiated in the current session. This will make sure that all the
+  // persisted data written previously will get purged.
+  signed_in_ = false;
+  RemoveCachedData();
+
+  io_thread_->PostTask(FROM_HERE,
+                       base::Bind(&GCMDriverDesktop::IOWorker::CheckOut,
+                       base::Unretained(io_worker_.get())));
 }
 
 void GCMDriverDesktop::AddAppHandler(const std::string& app_id,
@@ -422,7 +421,7 @@ void GCMDriverDesktop::Stop() {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
 
   // No need to stop GCM service if not started yet.
-  if (account_id_.empty())
+  if (!gcm_started_)
     return;
 
   RemoveCachedData();
@@ -526,7 +525,7 @@ GCMClient* GCMDriverDesktop::GetGCMClientForTesting() const
{
 
 bool GCMDriverDesktop::IsStarted() const {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
-  return !account_id_.empty();
+  return gcm_started_;
 }
 
 bool GCMDriverDesktop::IsGCMClientReady() const {
@@ -563,6 +562,9 @@ void GCMDriverDesktop::SetGCMRecording(const
GetGCMStatisticsCallback& callback,
 GCMClient::Result GCMDriverDesktop::EnsureStarted() {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
 
+  if (gcm_started_)
+    return GCMClient::SUCCESS;
+
   if (!gcm_enabled_)
     return GCMClient::GCM_DISABLED;
 
@@ -570,18 +572,10 @@ GCMClient::Result GCMDriverDesktop::EnsureStarted() {
   if (app_handlers().empty())
     return GCMClient::UNKNOWN_ERROR;
 
-  // Is the user signed in?
-  const std::string account_id = identity_provider_->GetActiveAccountId();
-  if (account_id.empty())
+  // TODO(jianli): To be removed when sign-in enforcement is dropped.
+  if (!signed_in_)
     return GCMClient::NOT_SIGNED_IN;
 
-  // CheckIn could be called more than once when:
-  // 1) The password changes.
-  // 2) Register/send function calls it to ensure CheckIn is done.
-  if (account_id_ == account_id)
-    return GCMClient::SUCCESS;
-  account_id_ = account_id;
-
   DCHECK(!delayed_task_controller_);
   delayed_task_controller_.reset(new DelayedTaskController);
 
@@ -593,6 +587,7 @@ GCMClient::Result GCMDriverDesktop::EnsureStarted() {
                  base::Unretained(io_worker_.get()),
                  weak_ptr_factory_.GetWeakPtr()));
 
+  gcm_started_ = true;
   return GCMClient::SUCCESS;
 }
 
@@ -602,34 +597,19 @@ void GCMDriverDesktop::RemoveCachedData() {
   // GCM service is stopped.
   weak_ptr_factory_.InvalidateWeakPtrs();
 
-  account_id_.clear();
+  gcm_started_ = false;
   gcm_client_ready_ = false;
   delayed_task_controller_.reset();
   ClearCallbacks();
 }
 
-void GCMDriverDesktop::CheckOut() {
-  DCHECK(ui_thread_->RunsTasksOnCurrentThread());
-
-  // We still proceed with the check-out logic even if the check-in is not
-  // initiated in the current session. This will make sure that all the
-  // persisted data written previously will get purged.
-
-  RemoveCachedData();
-
-  io_thread_->PostTask(
-      FROM_HERE,
-      base::Bind(&GCMDriverDesktop::IOWorker::CheckOut,
-                 base::Unretained(io_worker_.get())));
-}
-
 void GCMDriverDesktop::MessageReceived(
     const std::string& app_id,
     const GCMClient::IncomingMessage& message) {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
 
-  // Drop the event if signed out.
-  if (account_id_.empty())
+  // Drop the event if the service has been stopped.
+  if (!gcm_started_)
     return;
 
   GetAppHandler(app_id)->OnMessage(app_id, message);
@@ -638,8 +618,8 @@ void GCMDriverDesktop::MessageReceived(
 void GCMDriverDesktop::MessagesDeleted(const std::string& app_id) {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
 
-  // Drop the event if signed out.
-  if (account_id_.empty())
+  // Drop the event if the service has been stopped.
+  if (!gcm_started_)
     return;
 
   GetAppHandler(app_id)->OnMessagesDeleted(app_id);
@@ -650,8 +630,8 @@ void GCMDriverDesktop::MessageSendError(
     const GCMClient::SendErrorDetails& send_error_details) {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
 
-  // Drop the event if signed out.
-  if (account_id_.empty())
+  // Drop the event if the service has been stopped.
+  if (!gcm_started_)
     return;
 
   GetAppHandler(app_id)->OnSendError(app_id, send_error_details);
@@ -678,10 +658,4 @@ void GCMDriverDesktop::GetGCMStatisticsFinished(
     LOG(WARNING) << "request_gcm_statistics_callback_ is NULL.";
 }
 
-std::string GCMDriverDesktop::SignedInUserName() const {
-  if (IsStarted())
-    return identity_provider_->GetActiveUsername();
-  return std::string();
-}
-
 }  // namespace gcm

bartfab (slow)

lgtm https://codereview.chromium.org/330733002/diff/80001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/330733002/diff/80001/chrome/browser/browser_process_impl.cc#newcode1029 chrome/browser/browser_process_impl.cc:1029: // be started when none is asking for ...

6 years, 6 months ago (2014-06-16 09:17:06 UTC) #17

jianli

https://codereview.chromium.org/330733002/diff/80001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/330733002/diff/80001/chrome/browser/browser_process_impl.cc#newcode1029 chrome/browser/browser_process_impl.cc:1029: // be started when none is asking for it. ...

6 years, 6 months ago (2014-06-16 16:52:06 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jianli@chromium.org/330733002/120001

6 years, 6 months ago (2014-06-16 17:41:12 UTC) #20

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 6 months ago (2014-06-17 02:38:03 UTC) #21

commit-bot: I haz the power

Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/builds/27966)

6 years, 6 months ago (2014-06-17 02:38:04 UTC) #22

jianli

6 years, 6 months ago (2014-06-17 17:04:30 UTC) #23

Message was sent while issue was closed.

Committed patchset #6 manually as r277789 (presubmit successful).

Expand Messages | Collapse Messages