Removing the mock-keychain related bool from GCMStore constructor

Removing the mock-keychain related bool from GCMStore constructor

Removing the first parameter of GCMStore constructor.
Adding BuildGCMStore and SetUpForTesting to GCMInternalsBuilder.
Moving GCMInternalsBuilder to its own files in gcm/engine/

BUG=342360
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261842

Closing and moving to https://codereview.chromium.org/232113004/ for a fresh code review.

[fgorski, 2014-04-01 22:17:31]

PTAL. Change removes the mock keychain calls.

[Nicolas Zea, 2014-04-01 22:38:56]

LGTM if you're confident this won't affect the trybots/buildbots.

[jianli, 2014-04-01 22:45:03]

lgtm

[fgorski, 2014-04-02 17:24:22]

Nicolas, I am adding the mock keychain back in gcm_unit_tests for the time
being. Once I figure out how to force the use-mock-keychain command line switch,
I'll flip it for all gcm_unit_tests and remove the code.

[jianli, 2014-04-02 17:28:05]

As long as we make sure that InProcessBrowserTest::SetUp is called, it
should be taken care of:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...


On Wed, Apr 2, 2014 at 10:24 AM, <fgorski@chromium.org> wrote:

> Nicolas, I am adding the mock keychain back in gcm_unit_tests for the time
> being. Once I figure out how to force the use-mock-keychain command line
> switch,
> I'll flip it for all gcm_unit_tests and remove the code.
>
> https://codereview.chromium.org/221453003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fgorski, 2014-04-02 17:32:02]

Yeah, that is exactly what Nicolas asked for. It does not work for unit tests.
-->
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/chrome...
That line makes it happen for unit_tests, but because it is under chrome/ it has
no effect on gcm_unit_tests...
I'll investigate that and prepare a change that enforces the switch for
gcm_unit_tests.


On 2014/04/02 17:28:05, jianli wrote:
> As long as we make sure that InProcessBrowserTest::SetUp is called, it
> should be taken care of:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...
> 
> 
> On Wed, Apr 2, 2014 at 10:24 AM, <mailto:fgorski@chromium.org> wrote:
> 
> > Nicolas, I am adding the mock keychain back in gcm_unit_tests for the time
> > being. Once I figure out how to force the use-mock-keychain command line
> > switch,
> > I'll flip it for all gcm_unit_tests and remove the code.
> >
> > https://codereview.chromium.org/221453003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nicolas Zea, 2014-04-02 17:42:21]

lgtm

[jianli, 2014-04-02 17:58:17]

On Wed, Apr 2, 2014 at 10:32 AM, <fgorski@chromium.org> wrote:

> Yeah, that is exactly what Nicolas asked for. It does not work for unit
> tests.
> -->
> https://code.google.com/p/chromium/codesearch#chromium/
> src/chrome/test/chromedriver/chrome_launcher.cc&l=117
> That line makes it happen for unit_tests, but because it is under chrome/
> it has
> no effect on gcm_unit_tests...
> I'll investigate that and prepare a change that enforces the switch for
> gcm_unit_tests.
>
>
We can add a base class like GCMClientTestBase that is derived
from testing::Test and make all gcm unit test classes derive from this one.

In GCMClientTestBase::SetUp, we can duplicate the relevant logic from
InProcessBrowserTest::SetUp.


>
>
> On 2014/04/02 17:28:05, jianli wrote:
>
>> As long as we make sure that InProcessBrowserTest::SetUp is called, it
>> should be taken care of:
>>
>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/chrome/test/base/in_process_browser_test.cc&q=
> UseMockKeychain&sq=package:chromium&l=177&type=cs
>
>
>  On Wed, Apr 2, 2014 at 10:24 AM, <mailto:fgorski@chromium.org> wrote:
>>
>
>  > Nicolas, I am adding the mock keychain back in gcm_unit_tests for the
>> time
>> > being. Once I figure out how to force the use-mock-keychain command line
>> > switch,
>> > I'll flip it for all gcm_unit_tests and remove the code.
>> >
>> > https://codereview.chromium.org/221453003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/221453003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fgorski, 2014-04-03 23:30:37]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-03 23:31:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/221453003/40001

[commit-bot: I haz the power, 2014-04-03 23:31:17]

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

[commit-bot: I haz the power, 2014-04-03 23:31:18]

Failed to apply patch for
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/services/gcm/gcm_profile_service_unittest.cc
  Hunk #1 FAILED at 531.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/services/gcm/gcm_profile_service_unittest.cc.rej

Patch:       chrome/browser/services/gcm/gcm_profile_service_unittest.cc
Index: chrome/browser/services/gcm/gcm_profile_service_unittest.cc
diff --git a/chrome/browser/services/gcm/gcm_profile_service_unittest.cc
b/chrome/browser/services/gcm/gcm_profile_service_unittest.cc
index
4bf894e4aff5631daad21ea8da12c729450be126..e26a2426dc44f9e18e1ae1ce464bb3bc77448d79
100644
--- a/chrome/browser/services/gcm/gcm_profile_service_unittest.cc
+++ b/chrome/browser/services/gcm/gcm_profile_service_unittest.cc
@@ -531,12 +531,6 @@ class GCMProfileServiceTest : public testing::Test {
     test_user_manager_.reset(new chromeos::ScopedTestUserManager());
 #endif
 
-    // OSCrypt ends up needing access to the keychain on OS X. So use the mock
-    // keychain to prevent prompts.
-#if defined(OS_MACOSX)
-    OSCrypt::UseMockKeychain(true);
-#endif
-
     // Create a main profile consumer.
     consumer_.reset(new GCMProfileServiceTestConsumer(&waiter_));
   }

[fgorski, 2014-04-04 17:10:11]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-04 17:10:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/221453003/60001

[commit-bot: I haz the power, 2014-04-04 19:34:56]

Change committed as 261842

[fgorski, 2014-04-08 04:48:14]

A revert of this CL has been created in
https://codereview.chromium.org/226223005/ by fgorski@chromium.org.

The reason for reverting is: gcm_unit_tests were disabled on a Mac because they
were hanging on build bots. This is due to a combination of factors such as
gcm_unit_tests not running in the CQ and possible keychain related configuration
updates. In order to fix the problem step by step, this change will have to be
attempted again..

[fgorski, 2014-04-09 23:23:27]

PTAL, I moved the mock keychain code to chrome binary.

[jianli, 2014-04-09 23:36:58]

Why can't we append a command line switch in SetUp?

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_internals_builder.cc (right):

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.cc:20:
GCMInternalsBuilder::~GCMInternalsBuilder() {}
nit: empty line above

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_internals_builder.h (right):

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.h:12: #include
"google_apis/gcm/engine/gcm_store.h"
Do we need to include this?

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.h:13: #include
"google_apis/gcm/engine/mcs_client.h"
ditto

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.h:15: #include "net/base/net_log.h"
ditto

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.h:16: #include
"net/url_request/url_request_context_getter.h"
ditto

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.h:43: const
scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner);
Do we need forward declaration for SequencedTaskRunner?

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_internals_builder.h:59: bool testing_;
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl_unittest.cc (right):

https://codereview.chromium.org/221453003/diff/80001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl_unittest.cc:78:
message_loop_.message_loop_proxy()).Pass();
No need to call Pass.