Moves shared history test helpers into util classes.

Moves shared history test helpers into util classes.

BUG=None

Committed: https://crrev.com/4a13d7bae576141f3dcc334098a190cc092cb568
Cr-Commit-Position: refs/heads/master@{#372681}

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

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

[rohitrao (ping after 24h), 2016-01-29 19:16:55]

rohitrao@chromium.org changed reviewers:
+ pkasting@chromium.org, sky@chromium.org

[rohitrao (ping after 24h), 2016-01-29 19:16:55]

+Scott for components/history
+Peter for components/omnibox

This CL is cleaning up unittests by moving shared code into a set of helper
functions.  (I was hoping I could remove more friend declarations than I ended
up being able to.)

Peter, this CL also flips that |no_db| variable to be more positive.

[sky, 2016-01-29 20:58:22]

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
File components/history/core/test/history_service_test_util.cc (right):

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
components/history/core/test/history_service_test_util.cc:30:
base::MessageLoop::current()->QuitWhenIdle();
QuitWhenIdle is deprecated. Use a RunLoop instead.

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
components/history/core/test/history_service_test_util.cc:53: return
history_service;
nit: I would nuke 52 and return null. It's easier to see 'return nullptr' and
immediately know what's happening.

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
File components/history/core/test/history_service_test_util.h (right):

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
components/history/core/test/history_service_test_util.h:28: };
if you're going to use a class then add private: DISALLOW_IMPLICIT_CONSTRUCTORS.
Although in my opinion I wouldn't bother with the class.

[rohitrao (ping after 24h), 2016-01-29 21:32:35]

Patchset #5 (id:80001) has been deleted

[rohitrao (ping after 24h), 2016-01-29 21:35:12]

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
File components/history/core/test/history_service_test_util.cc (right):

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
components/history/core/test/history_service_test_util.cc:30:
base::MessageLoop::current()->QuitWhenIdle();
On 2016/01/29 20:58:21, sky wrote:
> QuitWhenIdle is deprecated. Use a RunLoop instead.

Done.

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
components/history/core/test/history_service_test_util.cc:53: return
history_service;
On 2016/01/29 20:58:21, sky wrote:
> nit: I would nuke 52 and return null. It's easier to see 'return nullptr' and
> immediately know what's happening.

Done.

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
File components/history/core/test/history_service_test_util.h (right):

https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
components/history/core/test/history_service_test_util.h:28: };
On 2016/01/29 20:58:21, sky wrote:
> if you're going to use a class then add private:
DISALLOW_IMPLICIT_CONSTRUCTORS.
> Although in my opinion I wouldn't bother with the class.

The class is here so I have something to "friend class" in HistoryService.  Is
there a way to get friend access without a class?

[sky, 2016-01-29 21:44:49]

On Fri, Jan 29, 2016 at 1:35 PM,  <rohitrao@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
> File components/history/core/test/history_service_test_util.cc (right):
>
>
https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
> components/history/core/test/history_service_test_util.cc:30:
> base::MessageLoop::current()->QuitWhenIdle();
> On 2016/01/29 20:58:21, sky wrote:
>> QuitWhenIdle is deprecated. Use a RunLoop instead.
>
> Done.
>
>
https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
> components/history/core/test/history_service_test_util.cc:53: return
> history_service;
> On 2016/01/29 20:58:21, sky wrote:
>> nit: I would nuke 52 and return null. It's easier to see 'return
> nullptr' and
>> immediately know what's happening.
>
> Done.
>
>
https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
> File components/history/core/test/history_service_test_util.h (right):
>
>
https://codereview.chromium.org/1646893003/diff/60001/components/history/core...
> components/history/core/test/history_service_test_util.h:28: };
> On 2016/01/29 20:58:21, sky wrote:
>> if you're going to use a class then add private:
> DISALLOW_IMPLICIT_CONSTRUCTORS.
>> Although in my opinion I wouldn't bother with the class.
>
> The class is here so I have something to "friend class" in
> HistoryService.  Is there a way to get friend access without a class?

Instead of the class name give the declaration of the function. At
least I think that works.

  -Scott

>
> https://codereview.chromium.org/1646893003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rohitrao (ping after 24h), 2016-01-29 22:17:50]

Updated to use friend functions for the HistoryService test utils.

I tried to do the same for the InMemoryURLIndex helpers, but things got messy
because those functions are in the top-level namespace.  I think I would have
had to forward-declare the functions in addition to the friend declarations, and
that was getting verbose enough that it didn't seem worth it.

[rohitrao (ping after 24h), 2016-01-29 22:24:12]

Patchset #7 has the forward-declares, so you can judge for yourself.  Patchset
#6 is the other option.

[sky, 2016-01-29 23:43:22]

LGTM

[Peter Kasting, 2016-01-30 01:09:22]

Why does BlockUntilInMemoryURLIndexIsRefreshed() need to be friended by either
class?  I can't find a place where it accesses private data or functions.

And RebuildInMemoryURLIndex() is only called by HistoryQuickProviderTest, so if
it were still a member of that class, you wouldn't need any friend declarations
for it either, since that's already a friend of both other classes whose private
information it needs to access and this change doesn't remove those
declarations.

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
File components/omnibox/browser/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
components/omnibox/browser/history_quick_provider_unittest.cc:189:
history_dir_.path(), GetAcceptLanguages(), /*create_db=*/true);
Nit: Don't add param name comments like this

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
File components/omnibox/browser/in_memory_url_index.h (right):

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
components/omnibox/browser/in_memory_url_index.h:156: history::HistoryService*
history);
Nit: Indenting

[rohitrao (ping after 24h), 2016-01-31 15:45:56]

On 2016/01/30 01:09:22, Peter Kasting wrote:
> Why does BlockUntilInMemoryURLIndexIsRefreshed() need to be friended by either
> class?  I can't find a place where it accesses private data or functions.

You are correct, it doesn't need to be friended by either class.  Removed.

> And RebuildInMemoryURLIndex() is only called by HistoryQuickProviderTest, so
if
> it were still a member of that class, you wouldn't need any friend
declarations
> for it either, since that's already a friend of both other classes whose
private
> information it needs to access and this change doesn't remove those
> declarations.

I was planning to call RebuildInMemoryURLIndex() from some other
yet-to-be-componentized files, but it's also only a two-line helper method. 
I've removed it for now, and I'll consider bringing it back if I need it in more
than a handful of places.

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
File components/omnibox/browser/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
components/omnibox/browser/history_quick_provider_unittest.cc:189:
history_dir_.path(), GetAcceptLanguages(), /*create_db=*/true);
On 2016/01/30 01:09:22, Peter Kasting wrote:
> Nit: Don't add param name comments like this

Removed.

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
File components/omnibox/browser/in_memory_url_index.h (right):

https://codereview.chromium.org/1646893003/diff/140001/components/omnibox/bro...
components/omnibox/browser/in_memory_url_index.h:156: history::HistoryService*
history);
On 2016/01/30 01:09:22, Peter Kasting wrote:
> Nit: Indenting

Done.

[Peter Kasting, 2016-01-31 20:30:43]

LGTM

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
File components/history/core/browser/history_service.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
components/history/core/browser/history_service.h:587: friend void
BlockUntilHistoryProcessesPendingRequests(
I don't think this needs to be a friend?  Looks like it just calls
ScheduleDBTask(), which is public.

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
File components/omnibox/browser/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
components/omnibox/browser/history_quick_provider_unittest.cc:310: //
|FillData()| must be called before |RebuildInMemoryURLIndex()|. This will
This comment refers to a nonexistent function (now).

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
File components/omnibox/browser/in_memory_url_index.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
components/omnibox/browser/in_memory_url_index.h:154: friend void
RebuildInMemoryURLIndex(InMemoryURLIndex* index,
This should be removed

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
File components/omnibox/browser/in_memory_url_index_test_util.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
components/omnibox/browser/in_memory_url_index_test_util.h:10: void
BlockUntilInMemoryURLIndexIsRefreshed(InMemoryURLIndex* index);
Are you planning on calling this from other test files in the immediate future? 
Otherwise we create this whole .h and .cc just for this one function which is
currently called in one place; seems overkill.

[blundell, 2016-02-01 08:28:30]

I don't think you need my LG at this point given your other reviewers, so I just
did a high-level run-through.

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
File components/history/core/test/history_service_test_util.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
components/history/core/test/history_service_test_util.h:20:
scoped_ptr<HistoryService> CreateHistoryService(
These functions could use comments.

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
File components/omnibox/browser/in_memory_url_index_test_util.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
components/omnibox/browser/in_memory_url_index_test_util.h:10: void
BlockUntilInMemoryURLIndexIsRefreshed(InMemoryURLIndex* index);
On 2016/01/31 20:30:43, Peter Kasting wrote:
> Are you planning on calling this from other test files in the immediate
future? 
> Otherwise we create this whole .h and .cc just for this one function which is
> currently called in one place; seems overkill.

(and if it is staying, it could use a comment)

[rohitrao (ping after 24h), 2016-02-01 12:46:48]

Thanks for the reviews!

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
File components/history/core/browser/history_service.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
components/history/core/browser/history_service.h:587: friend void
BlockUntilHistoryProcessesPendingRequests(
On 2016/01/31 20:30:43, Peter Kasting wrote:
> I don't think this needs to be a friend?  Looks like it just calls
> ScheduleDBTask(), which is public.

For some reason I looked at that method and decided it was private.  Fixed.

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
File components/history/core/test/history_service_test_util.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/history/cor...
components/history/core/test/history_service_test_util.h:20:
scoped_ptr<HistoryService> CreateHistoryService(
On 2016/02/01 08:28:30, blundell wrote:
> These functions could use comments.

Done.

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
File components/omnibox/browser/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
components/omnibox/browser/history_quick_provider_unittest.cc:310: //
|FillData()| must be called before |RebuildInMemoryURLIndex()|. This will
On 2016/01/31 20:30:43, Peter Kasting wrote:
> This comment refers to a nonexistent function (now).

Updated with the new method name.

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
File components/omnibox/browser/in_memory_url_index.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
components/omnibox/browser/in_memory_url_index.h:154: friend void
RebuildInMemoryURLIndex(InMemoryURLIndex* index,
On 2016/01/31 20:30:43, Peter Kasting wrote:
> This should be removed

Good catch, thanks.

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
File components/omnibox/browser/in_memory_url_index_test_util.h (right):

https://codereview.chromium.org/1646893003/diff/180001/components/omnibox/bro...
components/omnibox/browser/in_memory_url_index_test_util.h:10: void
BlockUntilInMemoryURLIndexIsRefreshed(InMemoryURLIndex* index);
On 2016/02/01 08:28:30, blundell wrote:
> On 2016/01/31 20:30:43, Peter Kasting wrote:
> > Are you planning on calling this from other test files in the immediate
> future? 
> > Otherwise we create this whole .h and .cc just for this one function which
is
> > currently called in one place; seems overkill.
> 
> (and if it is staying, it could use a comment)

Added a comment.  It's called from InMemoryURLIndexTest, which is the next CL in
the pipe.

[rohitrao (ping after 24h), 2016-02-01 12:47:16]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-02-01 12:47:17]

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

[commit-bot: I haz the power, 2016-02-01 12:47:59]

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

[commit-bot: I haz the power, 2016-02-01 14:34:53]

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

[commit-bot: I haz the power, 2016-02-01 14:34:54]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rohitrao (ping after 24h), 2016-02-01 16:00:39]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-02-01 16:00:39]

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

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

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

[commit-bot: I haz the power, 2016-02-01 17:11:13]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-02-01 17:12:33]

Description was changed from

==========
Moves shared history test helpers into util classes.

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

to

==========
Moves shared history test helpers into util classes.

BUG=None

Committed: https://crrev.com/4a13d7bae576141f3dcc334098a190cc092cb568
Cr-Commit-Position: refs/heads/master@{#372681}
==========

[commit-bot: I haz the power, 2016-02-01 17:12:34]

Patchset 12 (id:??) landed as
https://crrev.com/4a13d7bae576141f3dcc334098a190cc092cb568
Cr-Commit-Position: refs/heads/master@{#372681}