Move Sync-specific tests from ExtensionServiceTest into new file

Move the Sync-specific tests from ExtensionServiceTest into a new file

BUG=381251
Committed: https://crrev.com/9afc62139df197f6b0f49c5e87915b1fdae0d790
Cr-Commit-Position: refs/heads/master@{#357158}

[Marc Treib, 2015-10-19 09:57:25]

treib@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Marc Treib, 2015-10-19 09:57:26]

extension_service_unittest.cc down from 8k to 6k LoC! :)

(This is based on https://codereview.chromium.org/1200833004/, so please look at
that first!)

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_with_install.h (right):

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.h:20: class
ExtensionServiceTestWithInstall : public ExtensionServiceTestBase,
All this could have just been moved into ExtensionServiceTestBase, but I think
it's cleaner/easier to read to put all the install-related stuff in a separate
class.

[Devlin, 2015-10-20 17:42:16]

Mostly good.  I assume that 99.9% of this is copy-paste, and I don't have to
really review the content/logic of any of it?

Also, bug=381251

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_base.cc:209: std::string msg =
"while checking: ";
base::StringPrintf is a beautiful thing. :) (here and below)

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_with_install.cc (right):

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:46:
extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
If you don't mind the slight extra churn, making this test use
ExtensionRegistryObserver would be great.  (Feel free to postpone, if you like.)

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:65: for
(std::vector<base::string16>::const_iterator iter = errors->begin();
range-based

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:89: }
indentation

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:190: //
Verifies the result of a CRX installation. Used by InstallCRX. Set the
Seems like these comments should go in the .h?

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:204: // Returns
an Extension pointer if the install succeeded, NULL otherwise.
nit: s/NULL/nullptr||null

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_with_install.h (right):

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.h:20: class
ExtensionServiceTestWithInstall : public ExtensionServiceTestBase,
On 2015/10/19 09:57:26, Marc Treib wrote:
> All this could have just been moved into ExtensionServiceTestBase, but I think
> it's cleaner/easier to read to put all the install-related stuff in a separate
> class.

I agree.

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.h:96: //
TODO(treib,rdcronin): Make these private and add accessors as needed.
nit: s/rdcronin/devlin

[Marc Treib, 2015-10-21 11:47:22]

Description was changed from

==========
Move the Sync-specific tests from ExtensionServiceTest into a new file

BUG=None
==========

to

==========
Move the Sync-specific tests from ExtensionServiceTest into a new file

BUG=381251
==========

[Marc Treib, 2015-10-21 12:51:54]

Yup, this is all copy&paste plus trivial/mechanical changes to make it compile
again. (I should've said so, sorry)

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_base.cc:209: std::string msg =
"while checking: ";
On 2015/10/20 17:42:15, Devlin wrote:
> base::StringPrintf is a beautiful thing. :) (here and below)

You really are a fan of that function, aren't you ;)
I was trying to keep this CL as close as possible to pure copy-and-paste, but
this seems safe enough. Done.

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_with_install.cc (right):

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:46:
extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
On 2015/10/20 17:42:15, Devlin wrote:
> If you don't mind the slight extra churn, making this test use
> ExtensionRegistryObserver would be great.  (Feel free to postpone, if you
like.)

Yes, let's try and keep this CL to trivial changes. I've added a TODO.

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:65: for
(std::vector<base::string16>::const_iterator iter = errors->begin();
On 2015/10/20 17:42:15, Devlin wrote:
> range-based

Done.

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:89: }
On 2015/10/20 17:42:15, Devlin wrote:
> indentation

Done.

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:190: //
Verifies the result of a CRX installation. Used by InstallCRX. Set the
On 2015/10/20 17:42:15, Devlin wrote:
> Seems like these comments should go in the .h?

Indeed. Done.

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.cc:204: // Returns
an Extension pointer if the install succeeded, NULL otherwise.
On 2015/10/20 17:42:15, Devlin wrote:
> nit: s/NULL/nullptr||null

Done.

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_with_install.h (right):

https://codereview.chromium.org/1411773002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.h:96: //
TODO(treib,rdcronin): Make these private and add accessors as needed.
On 2015/10/20 17:42:15, Devlin wrote:
> nit: s/rdcronin/devlin

Done.

[Marc Treib, 2015-10-28 12:41:20]

Ping!

[Devlin, 2015-10-28 16:15:22]

lgtm

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_base.cc:228: bool val;
nitty nit: initialize val.

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_with_install.h (right):

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.h:122: size_t
expected_extensions_count_;
nit: Disallow copy and assign.

[Marc Treib, 2015-10-29 13:27:58]

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_base.cc:228: bool val;
On 2015/10/28 16:15:22, Devlin wrote:
> nitty nit: initialize val.

Done.

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_with_install.h (right):

https://codereview.chromium.org/1411773002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_with_install.h:122: size_t
expected_extensions_count_;
On 2015/10/28 16:15:22, Devlin wrote:
> nit: Disallow copy and assign.

Done. (Also in ExtensionServiceTestBase)

[Marc Treib, 2015-10-29 13:28:03]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-10-29 13:28:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1411773002/#ps80001
(title: "review nits + rebase")

[commit-bot: I haz the power, 2015-10-29 13:28:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411773002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411773002/80001

[commit-bot: I haz the power, 2015-10-29 14:51:10]

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

[commit-bot: I haz the power, 2015-10-29 14:51:11]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Marc Treib, 2015-10-30 17:00:35]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-10-30 17:00:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1411773002/#ps100001
(title: "move ExternalInstallPrompt override")

[commit-bot: I haz the power, 2015-10-30 17:00:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411773002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411773002/100001

[commit-bot: I haz the power, 2015-10-30 18:50:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-30 18:51:18]

Patchset 6 (id:??) landed as
https://crrev.com/9afc62139df197f6b0f49c5e87915b1fdae0d790
Cr-Commit-Position: refs/heads/master@{#357158}