Componentizes history_url_provider_unittest.cc.

Componentizes history_url_provider_unittest.cc.

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

BUG=579668
TEST=None

Committed: https://crrev.com/39deaa00ffa9ebe186c5a81dc889883fc49e6c97
Cr-Commit-Position: refs/heads/master@{#370690}

[rohitrao (ping after 24h), 2016-01-20 21:25:11]

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

[rohitrao (ping after 24h), 2016-01-20 21:25:11]

See what you think.  HistoryURLProviderTest is already a friend class of
HistoryService, so I just ended up copying pieces of code out of TestingProfile.

I thought about moving things to a new helper file, but I think that would be
more useful to do after I finish the other autocomplete tests, so that I can see
how many of the tests would really share helpers.

[blundell, 2016-01-21 09:00:01]

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

[blundell, 2016-01-21 09:00:01]

LGTM

It looks like several other of the autocomplete unittests will need similar
functionality, so I'm pretty sure we will end up splitting it out. I'm fine
doing that as part of the componentization of the next test that needs it,
though.

+PK for OWNERS (whole CL)

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/DEPS (right):

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/DEPS:30: "+components/history/core/test",
nit: I don't think we need to get this specific now that we have GN. GN will
complain if you include a file from a test-only target in a non-test-only
target.

[Peter Kasting, 2016-01-21 11:08:40]

LGTM

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

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:156: class
QuittingHistoryDBTask : public history::HistoryDBTask {
Nit: QuitTask?  "Quitting" in particular reads weirdly, and I'm not sure we
really need "HistoryDBTask" in the class name.

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:260: bool
delete_file, bool no_db) WARN_UNUSED_RESULT;
Nit: Prefer to wrap like:

  bool InitializeHistoryService(bool delete_file,
                                bool no_db) WARN_UNUSED_RESULT;

...though honestly I don't think WARN_UNUSED_RESULT is worth doing in tests,
particularly not here where we have just one caller.

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:272: };
If you're going to touch this anyway, add DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:301: if
(!history_dir_.CreateUniqueTempDir()) {
Nit: No {} on any of the conditionals in this function

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:313: if (!no_db) {
Nit: Ugh, double-negative.  Makes me think we should reverse |no_db| (to
"create_db" or something), at least for this function.  (Maybe for Init() as
well, I dunno.)

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:323:
scoped_ptr<history::HistoryDBTask>(
Nit: make_scoped_ptr?

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

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/DEPS (right):

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/DEPS:30: "+components/history/core/test",
On 2016/01/21 09:00:01, blundell wrote:
> nit: I don't think we need to get this specific now that we have GN. GN will
> complain if you include a file from a test-only target in a non-test-only
> target.

Done.

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

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:156: class
QuittingHistoryDBTask : public history::HistoryDBTask {
On 2016/01/21 11:08:40, Peter Kasting wrote:
> Nit: QuitTask?  "Quitting" in particular reads weirdly, and I'm not sure we
> really need "HistoryDBTask" in the class name.

Renamed to QuitTask.

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:260: bool
delete_file, bool no_db) WARN_UNUSED_RESULT;
On 2016/01/21 11:08:40, Peter Kasting wrote:
> Nit: Prefer to wrap like:
> 
>   bool InitializeHistoryService(bool delete_file,
>                                 bool no_db) WARN_UNUSED_RESULT;
> 
> ...though honestly I don't think WARN_UNUSED_RESULT is worth doing in tests,
> particularly not here where we have just one caller.

Removed WARN_UNUSED_RESULT.

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:272: };
On 2016/01/21 11:08:40, Peter Kasting wrote:
> If you're going to touch this anyway, add DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:301: if
(!history_dir_.CreateUniqueTempDir()) {
On 2016/01/21 11:08:40, Peter Kasting wrote:
> Nit: No {} on any of the conditionals in this function

Done.

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:313: if (!no_db) {
On 2016/01/21 11:08:40, Peter Kasting wrote:
> Nit: Ugh, double-negative.  Makes me think we should reverse |no_db| (to
> "create_db" or something), at least for this function.  (Maybe for Init() as
> well, I dunno.)

Leaving this for now.  I think that if we make a change here, we should change
Init() to match.

https://codereview.chromium.org/1610283002/diff/20001/components/omnibox/brow...
components/omnibox/browser/history_url_provider_unittest.cc:323:
scoped_ptr<history::HistoryDBTask>(
On 2016/01/21 11:08:40, Peter Kasting wrote:
> Nit: make_scoped_ptr?

This makes scoped_ptr unhappy, due to the private destructor.

[rohitrao (ping after 24h), 2016-01-21 13:11:00]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-01-21 13:11:01]

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

[commit-bot: I haz the power, 2016-01-21 13:11:09]

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

[commit-bot: I haz the power, 2016-01-21 14:03:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-21 14:04:01]

Description was changed from

==========
Componentizes history_url_provider_unittest.cc.

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

BUG=579668
TEST=None
==========

to

==========
Componentizes history_url_provider_unittest.cc.

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

BUG=579668
TEST=None

Committed: https://crrev.com/39deaa00ffa9ebe186c5a81dc889883fc49e6c97
Cr-Commit-Position: refs/heads/master@{#370690}
==========

[commit-bot: I haz the power, 2016-01-21 14:04:02]

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