[GCM] 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=343360
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263354

[fgorski, 2014-04-09 23:38:51]

Let's take a fresh look

[fgorski, 2014-04-10 01:08:37]

Applied Jian's feedback form the original (closed) patch.

Using a command line and removing changes to internals builder.

[jianli, 2014-04-10 18:38:12]

lgtm

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:20: #include
"components/os_crypt/os_crypt.h"
This can be removed now.

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

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl_unittest.cc:77:
base::CommandLine::ForCurrentProcess()->AppendSwitch("use-mock-keychain");
better call testing::Test::SetUp();

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:156:
base::CommandLine::ForCurrentProcess()->AppendSwitch("use-mock-keychain");
nit: better call testing::Test::SetUp()

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl_unittest.cc:270:
base::CommandLine::ForCurrentProcess()->AppendSwitch("use-mock-keychain");
better call testing::Test::SetUp();.

[Nicolas Zea, 2014-04-10 18:54:48]

LGTM with some comments.

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:20: #include
"components/os_crypt/os_crypt.h"
On 2014/04/10 18:38:12, jianli wrote:
> This can be removed now.

It's needed for the actual encryption/decryption

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

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl_unittest.cc:77:
base::CommandLine::ForCurrentProcess()->AppendSwitch("use-mock-keychain");
On 2014/04/10 18:38:12, jianli wrote:
> better call testing::Test::SetUp();

That's not necessary, it doesn't do anything. Overriding SetUp on its own is
common.

That said, I think you should #include "os_crypt_switches" and use
kUseMockKeychain here instead of the actual switch.

[jianli, 2014-04-10 18:58:03]

On Thu, Apr 10, 2014 at 11:54 AM, <zea@chromium.org> wrote:

> LGTM with some comments.
>
>
>
> https://codereview.chromium.org/232113004/diff/40001/
> google_apis/gcm/engine/gcm_store_impl.cc
> File google_apis/gcm/engine/gcm_store_impl.cc (right):
>
> https://codereview.chromium.org/232113004/diff/40001/
> google_apis/gcm/engine/gcm_store_impl.cc#newcode20
> google_apis/gcm/engine/gcm_store_impl.cc:20: #include
> "components/os_crypt/os_crypt.h"
> On 2014/04/10 18:38:12, jianli wrote:
>
>> This can be removed now.
>>
>
> It's needed for the actual encryption/decryption
>
>
> https://codereview.chromium.org/232113004/diff/40001/
> google_apis/gcm/engine/gcm_store_impl_unittest.cc
> File google_apis/gcm/engine/gcm_store_impl_unittest.cc (right):
>
> https://codereview.chromium.org/232113004/diff/40001/
> google_apis/gcm/engine/gcm_store_impl_unittest.cc#newcode77
> google_apis/gcm/engine/gcm_store_impl_unittest.cc:77:
> base::CommandLine::ForCurrentProcess()->AppendSwitch("use-mock-keychain");
> On 2014/04/10 18:38:12, jianli wrote:
>
>> better call testing::Test::SetUp();
>>
>
> That's not necessary, it doesn't do anything. Overriding SetUp on its
> own is common.
>

Yes, it is not necessary. But I think it is a good habit to do that in case
that the child class is derived from some other test class that might have
default logic in SetUp.


>
> That said, I think you should #include "os_crypt_switches" and use
> kUseMockKeychain here instead of the actual switch.
>
> https://codereview.chromium.org/232113004/
>

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

[fgorski, 2014-04-10 19:01:15]

I'll sent out updated patch in a few minutes.

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:20: #include
"components/os_crypt/os_crypt.h"
On 2014/04/10 18:54:48, Nicolas Zea wrote:
> On 2014/04/10 18:38:12, jianli wrote:
> > This can be removed now.
> 
> It's needed for the actual encryption/decryption

Correct. It has to stay.

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

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl_unittest.cc:77:
base::CommandLine::ForCurrentProcess()->AppendSwitch("use-mock-keychain");
On 2014/04/10 18:54:48, Nicolas Zea wrote:
> On 2014/04/10 18:38:12, jianli wrote:
> > better call testing::Test::SetUp();
> 
> That's not necessary, it doesn't do anything. Overriding SetUp on its own is
> common.
> 
> That said, I think you should #include "os_crypt_switches" and use
> kUseMockKeychain here instead of the actual switch.

Putting both in (in all 3 files)

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/232113004/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:156:
base::CommandLine::ForCurrentProcess()->AppendSwitch("use-mock-keychain");
On 2014/04/10 18:38:12, jianli wrote:
> nit: better call testing::Test::SetUp()

Done.

[fgorski, 2014-04-10 20:52:01]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-10 20:52:18]

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

[commit-bot: I haz the power, 2014-04-10 21:27:45]

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

[commit-bot: I haz the power, 2014-04-10 21:27:46]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[fgorski, 2014-04-10 23:29:41]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-10 23:30:31]

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

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

The CQ bit was unchecked by fgorski@chromium.org

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

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2014-04-11 18:42:16]

The CQ bit was unchecked by fgorski@chromium.org

[fgorski, 2014-04-11 18:43:14]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-11 18:45:35]

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

[commit-bot: I haz the power, 2014-04-11 21:02:18]

Change committed as 263354