Refactor to share code between shortcuts tests.

Refactor to share code between shortcuts tests.

BUG=

[Peter Kasting, 2016-02-03 00:31:46]

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

[Peter Kasting, 2016-02-03 00:31:47]

LGTM

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/shortcuts_backend.h (right):

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_backend.h:105: size_t db_size);
Does this let us unfriend any of the test classes?

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/shortcuts_provider_test_util.cc (right):

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_provider_test_util.cc:15: using
base::ASCIIToUTF16;
Nit: Just omit this and qualify the names below

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_provider_test_util.cc:128: EXPECT_EQ(0U,
Leftovers.size());
I think we can nuke all the SetShouldContain stuff by doing something like:

  for (const auto& expected_url : expected_urls) {
    EXPECT_TRUE(std::find_if(ac_matches_.begin(), ac_matches_.end(),
                             [&expected_url](const AutocompleteMatch& match) {
        return expected_url.first == match.destination_url.spec() &&
               expected_url.second == match.allowed_to_be_default_match; }));

(Not sure of the proper formatting for this)

We can just drop the EXPECT_EQ(0U, Leftovers.size()); because it tells us
nothing the previous EXPECTs haven't already told us.

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/shortcuts_provider_test_util.h (right):

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_provider_test_util.h:14: typedef
std::pair<std::string, bool> ShortcutExpectedURLAndAllowedToBeDefault;
Why add the word "Shortcut" on the front of these two types?  Seems like it just
adds verbosity for no (or negative) clarity, and there aren't symbol conflicts
I'm aware of that would necessitate this.

Similarly, I think the names below could be briefer and clearer by omitting most
of the instances of "Shortcut" (e.g. struct TestData, void
PopulateShortcutsBackendWithTestData(), etc.)

Nit: Prefer type aliases to typedefs

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

Thanks for the review!  I have addressed most of the comments and will roll
these changes into https://codereview.chromium.org/1654833005/.

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/shortcuts_backend.h (right):

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_backend.h:105: size_t db_size);
On 2016/02/03 00:31:47, Peter Kasting wrote:
> Does this let us unfriend any of the test classes?

Yup, dropped what I could.

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/shortcuts_provider_test_util.cc (right):

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_provider_test_util.cc:15: using
base::ASCIIToUTF16;
On 2016/02/03 00:31:47, Peter Kasting wrote:
> Nit: Just omit this and qualify the names below

Done.

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_provider_test_util.cc:128: EXPECT_EQ(0U,
Leftovers.size());
On 2016/02/03 00:31:47, Peter Kasting wrote:
> I think we can nuke all the SetShouldContain stuff by doing something like:
> 
>   for (const auto& expected_url : expected_urls) {
>     EXPECT_TRUE(std::find_if(ac_matches_.begin(), ac_matches_.end(),
>                              [&expected_url](const AutocompleteMatch& match) {
>         return expected_url.first == match.destination_url.spec() &&
>                expected_url.second == match.allowed_to_be_default_match; }));
> 
> (Not sure of the proper formatting for this)
> 
> We can just drop the EXPECT_EQ(0U, Leftovers.size()); because it tells us
> nothing the previous EXPECTs haven't already told us.

Done.

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/shortcuts_provider_test_util.h (right):

https://codereview.chromium.org/1655933004/diff/1/components/omnibox/browser/...
components/omnibox/browser/shortcuts_provider_test_util.h:14: typedef
std::pair<std::string, bool> ShortcutExpectedURLAndAllowedToBeDefault;
On 2016/02/03 00:31:47, Peter Kasting wrote:
> Why add the word "Shortcut" on the front of these two types?  Seems like it
just
> adds verbosity for no (or negative) clarity, and there aren't symbol conflicts
> I'm aware of that would necessitate this.
> 
> Similarly, I think the names below could be briefer and clearer by omitting
most
> of the instances of "Shortcut" (e.g. struct TestData, void
> PopulateShortcutsBackendWithTestData(), etc.)

There are currently no symbol conflicts, but I am uncomfortable putting classes
named "TestData" and "ExpectedURLs" into the global namespace.  I think that's
asking for conflicts in the future.  I'd like to keep the "Shortcut" in
TestShortcutData, but I will shorten the "Populate" method name as you
suggested.  I've also dropped the "Shortcut" prefix from the typedefs.

> Nit: Prefer type aliases to typedefs

Done.