Componentizes history_quick_provider_unittest.cc.

Componentizes history_quick_provider_unittest.cc.

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

BUG=581108
TEST=None

Committed: https://crrev.com/83e906aa8a4997f2ca9541fb93493065129c13d4
Cr-Commit-Position: refs/heads/master@{#371784}

[rohitrao (ping after 24h), 2016-01-25 19:40:15]

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

[blundell, 2016-01-26 08:40:25]

This looks like it was not particularly easy.

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

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:25: #include
"components/bookmarks/test/bookmark_test_helpers.h"
Did you double-check that all of the dependencies in this file are already in
the relevant test targets?

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:182: class
QuitTask : public history::HistoryDBTask {
How much duplication is there between this stuff and what you added in the
HistoryURLProvider unittest? Is now the right time to split this stuff out?

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:340:
client_->GetInMemoryURLIndex()->Shutdown();
Comment on why these two lines are necessary?

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:524: void
HistoryQuickProviderTest::BlockUntilHistoryProcessesPendingRequests() {
Same question as above.

[rohitrao (ping after 24h), 2016-01-26 14:15:48]

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

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:25: #include
"components/bookmarks/test/bookmark_test_helpers.h"
On 2016/01/26 08:40:25, blundell wrote:
> Did you double-check that all of the dependencies in this file are already in
> the relevant test targets?

Added "//base/test:test_support".  Everything else looked like it was already
there.

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:182: class
QuitTask : public history::HistoryDBTask {
On 2016/01/26 08:40:25, blundell wrote:
> How much duplication is there between this stuff and what you added in the
> HistoryURLProvider unittest? Is now the right time to split this stuff out?

QuitTask is identical, FakeAutocompleteProviderClient looks very similar.  I was
going to componentize one more test (InMemoryURLProviderTest) and then pull out
the common test support code.  I'm not yet sure if CreateInMemoryURLIndex() will
be shared amongst tests or not.

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:340:
client_->GetInMemoryURLIndex()->Shutdown();
On 2016/01/26 08:40:25, blundell wrote:
> Comment on why these two lines are necessary?

Done.

https://codereview.chromium.org/1635653002/diff/1/components/omnibox/browser/...
components/omnibox/browser/history_quick_provider_unittest.cc:524: void
HistoryQuickProviderTest::BlockUntilHistoryProcessesPendingRequests() {
On 2016/01/26 08:40:25, blundell wrote:
> Same question as above.

This will go into the shared test code, but I haven't yet decided if it should
go into a HistoryTestUtil file or just directly into
FakeAutocompleteProviderClient.

[blundell, 2016-01-26 14:17:17]

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

[blundell, 2016-01-26 14:17:17]

lgtm

+pkasting@ for OWNERS

[Peter Kasting, 2016-01-26 16:21:45]

LGTM

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:238: 
Nit: Why this blank line?

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:315: 
Nit: Another curious blank line

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:329: //
|FillData()| must be called before the InMemoryURLIndex is created.
Nit: It would be nice to motivate this with a "why".

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:466:
ASSERT_GT(ac_matches_.size(), 0U);
Nit: ASSERT_FALSE(ac_matches.empty()), since the only way the assertion can fail
is if size() == 0, and thus there's no useful added detail in the failure
printout from using size() over empty()

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:503: if
(!history_dir_.CreateUniqueTempDir())
Nit: Might as well combine these two conditionals

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:511: if (!no_db)
Nit: I still think it would likely be nice to reverse the sense of this
parameter everywhere.  Seems like every time I see a reference to it it's in
code that reads like a double-negative.

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:542:
index->set_restore_cache_observer(NULL);
Nit: nullptr

[rohitrao (ping after 24h), 2016-01-27 13:55:43]

Thanks!

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:238: 
On 2016/01/26 16:21:45, Peter Kasting wrote:
> Nit: Why this blank line?

Done.

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:315: 
On 2016/01/26 16:21:45, Peter Kasting wrote:
> Nit: Another curious blank line

Done.

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:329: //
|FillData()| must be called before the InMemoryURLIndex is created.
On 2016/01/26 16:21:45, Peter Kasting wrote:
> Nit: It would be nice to motivate this with a "why".

Done.

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:466:
ASSERT_GT(ac_matches_.size(), 0U);
On 2016/01/26 16:21:45, Peter Kasting wrote:
> Nit: ASSERT_FALSE(ac_matches.empty()), since the only way the assertion can
fail
> is if size() == 0, and thus there's no useful added detail in the failure
> printout from using size() over empty()

Done.

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:503: if
(!history_dir_.CreateUniqueTempDir())
On 2016/01/26 16:21:45, Peter Kasting wrote:
> Nit: Might as well combine these two conditionals

Done.

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:511: if (!no_db)
On 2016/01/26 16:21:45, Peter Kasting wrote:
> Nit: I still think it would likely be nice to reverse the sense of this
> parameter everywhere.  Seems like every time I see a reference to it it's in
> code that reads like a double-negative.

Agreed.  In this particular case, the parameter is unused, so I'm just going to
remove it, but I'll try to flip the meaning when I move this code into a helper
class.

https://codereview.chromium.org/1635653002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_quick_provider_unittest.cc:542:
index->set_restore_cache_observer(NULL);
On 2016/01/26 16:21:45, Peter Kasting wrote:
> Nit: nullptr

Done.

[rohitrao (ping after 24h), 2016-01-27 13:58:04]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-01-27 13:58:05]

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/1635653002/#ps40001
(title: "Review.")

[commit-bot: I haz the power, 2016-01-27 13:58:32]

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

[commit-bot: I haz the power, 2016-01-27 14:44:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-27 14:45:24]

Description was changed from

==========
Componentizes history_quick_provider_unittest.cc.

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

BUG=581108
TEST=None
==========

to

==========
Componentizes history_quick_provider_unittest.cc.

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

BUG=581108
TEST=None

Committed: https://crrev.com/83e906aa8a4997f2ca9541fb93493065129c13d4
Cr-Commit-Position: refs/heads/master@{#371784}
==========

[commit-bot: I haz the power, 2016-01-27 14:45:27]

Patchset 3 (id:??) landed as
https://crrev.com/83e906aa8a4997f2ca9541fb93493065129c13d4
Cr-Commit-Position: refs/heads/master@{#371784}