Componentizes shortcuts unittests.

Componentizes shortcuts unittests.

Removes dependencies on chrome/ and content/ and moves the files to
components/omnibox/browser.

BUG=583316
Committed: https://crrev.com/b6175493ffe1552741b7e27b9c97c6ea6409bbf1
Cr-Commit-Position: refs/heads/master@{#373252}

[rohitrao (ping after 24h), 2016-02-02 15:23:18]

rohitrao@chromium.org changed reviewers:
+ blundell@chromium.org

[rohitrao (ping after 24h), 2016-02-02 15:23:19]

+Colin for a first look.

[blundell, 2016-02-02 15:37:34]

lgtm

The code duplication in the extensions unittest is unfortunate, but my
understanding is that it would be non-trivial to refactor in such a way that it
could be eliminated.

https://codereview.chromium.org/1654833005/diff/60001/components/omnibox/brow...
File components/omnibox/browser/shortcuts_provider_unittest.cc (left):

https://codereview.chromium.org/1654833005/diff/60001/components/omnibox/brow...
components/omnibox/browser/shortcuts_provider_unittest.cc:209: 
I believe these blank lines are intended, although clang-format doesn't agree.

[blundell, 2016-02-02 15:38:19]

blundell@chromium.org changed reviewers:
+ pkasting@chromium.org

[blundell, 2016-02-02 15:38:20]

+PK for whole CL as OWNERS

[rohitrao (ping after 24h), 2016-02-02 15:42:26]

On 2016/02/02 15:37:34, blundell wrote:
> lgtm
> 
> The code duplication in the extensions unittest is unfortunate, but my
> understanding is that it would be non-trivial to refactor in such a way that
it
> could be eliminated.

I tried to pull the ShortcutsProviderTest fixture into it's own class, so that
it could be shared between the two tests, and that was becoming a pain.  The
//components test uses a fake AutocompleteProviderClient, while the //chrome
version needs a TestingProfile, so that wasn't even being shared anyways.  The
duplication of RunTest() and its support code sucks, but it seemed messier to
try and share it.

https://codereview.chromium.org/1654833005/diff/60001/components/omnibox/brow...
File components/omnibox/browser/shortcuts_provider_unittest.cc (left):

https://codereview.chromium.org/1654833005/diff/60001/components/omnibox/brow...
components/omnibox/browser/shortcuts_provider_unittest.cc:209: 
On 2016/02/02 15:37:34, blundell wrote:
> I believe these blank lines are intended, although clang-format doesn't agree.

Yeah, I decided that "---------------------" was a visible enough separator that
I could get rid of the extra blank line.

[blundell, 2016-02-02 16:08:45]

https://codereview.chromium.org/1654833005/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_provider_extension_unittest.cc
(right):

https://codereview.chromium.org/1654833005/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_provider_extension_unittest.cc:220:
TEST_F(ShortcutsProviderExtensionTest, Extension) {
PK, what do you think about just getting rid of this test as an alternative to
this code duplication? AFAICT the only thing it's testing above and beyond the
other ShortcutsProvider tests is that ShortcutsExtensionManager listens for the
right notification and calls the right method in response to it. That is
literally all that ShortcutsExtensionManager does (it's 50 lines of code).

[rohitrao (ping after 24h), 2016-02-02 20:17:44]

I attempted to pull shared code out in
https://codereview.chromium.org/1655933004/.  Please give that a quick glance --
I don't feel like it's significantly better.

[Peter Kasting, 2016-02-03 00:35:41]

LGTM

https://codereview.chromium.org/1654833005/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_provider_extension_unittest.cc
(right):

https://codereview.chromium.org/1654833005/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_provider_extension_unittest.cc:220:
TEST_F(ShortcutsProviderExtensionTest, Extension) {
On 2016/02/02 16:08:44, blundell wrote:
> PK, what do you think about just getting rid of this test as an alternative to
> this code duplication? AFAICT the only thing it's testing above and beyond the
> other ShortcutsProvider tests is that ShortcutsExtensionManager listens for
the
> right notification and calls the right method in response to it. That is
> literally all that ShortcutsExtensionManager does (it's 50 lines of code).

I think it's probably worth keeping this test.

[rohitrao (ping after 24h), 2016-02-03 14:56:51]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[rohitrao (ping after 24h), 2016-02-03 14:57:00]

The CQ bit was unchecked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-02-03 14:57:22]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-03 14:58:26]

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

[commit-bot: I haz the power, 2016-02-03 15:09:49]

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

[commit-bot: I haz the power, 2016-02-03 15:09:50]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[rohitrao (ping after 24h), 2016-02-03 16:28:41]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-02-03 16:28:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1654833005/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-02-03 16:29:07]

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

[commit-bot: I haz the power, 2016-02-03 16:38:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-03 16:39:09]

Description was changed from

==========
Componentizes shortcuts unittests.

Removes dependencies on chrome/ and content/ and moves the files to
components/omnibox/browser.

BUG=583316
==========

to

==========
Componentizes shortcuts unittests.

Removes dependencies on chrome/ and content/ and moves the files to
components/omnibox/browser.

BUG=583316

Committed: https://crrev.com/b6175493ffe1552741b7e27b9c97c6ea6409bbf1
Cr-Commit-Position: refs/heads/master@{#373252}
==========

[commit-bot: I haz the power, 2016-02-03 16:39:10]

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