Adding unit tests for PrerenderDispatcher.

Adding unit tests for PrerenderDispatcher.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138877

[dominich, 2012-05-23 18:00:59]

Separated adding of unit_tests from renaming CL

[sky, 2012-05-23 20:54:56]

LGTM

https://chromiumcodereview.appspot.com/10424002/diff/1/chrome/renderer/preren...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10424002/diff/1/chrome/renderer/preren...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:7: #include
"chrome/renderer/prerender/prerender_dispatcher.h"
this should be your first include, newline, then rest of includes.

https://chromiumcodereview.appspot.com/10424002/diff/1/chrome/renderer/preren...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:16: :
prerender_dispatcher_(new PrerenderDispatcher()) {
why both with a scoped_ptr here?

https://chromiumcodereview.appspot.com/10424002/diff/1/chrome/renderer/preren...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:25: };
DISALLOW_...

[dominich, 2012-05-24 15:18:46]

http://codereview.chromium.org/10424002/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

http://codereview.chromium.org/10424002/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:7: #include
"chrome/renderer/prerender/prerender_dispatcher.h"
On 2012/05/23 20:54:57, sky wrote:
> this should be your first include, newline, then rest of includes.

Done.

http://codereview.chromium.org/10424002/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:16: :
prerender_dispatcher_(new PrerenderDispatcher()) {
On 2012/05/23 20:54:57, sky wrote:
> why both with a scoped_ptr here?

Bad habits are creeping in to my code. Removed heap alloc.

http://codereview.chromium.org/10424002/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:25: };
On 2012/05/23 20:54:57, sky wrote:
> DISALLOW_...

Done.

http://codereview.chromium.org/10424002/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:25: };
On 2012/05/23 20:54:57, sky wrote:
> DISALLOW_...

Done.

[gavinp, 2012-05-24 15:58:54]

I'm really glad to see some testing coverage here!

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
chrome/renderer/prerender/prerender_dispatcher.h:29: private:
Rant you can ignore, but I must write down:

I prefer writing "friend class PrerenderDispatcherTest;" here, and getting rid
of these macros, but that would require writing convenience functions in
PrerenderDispatcherTest that basically pass-through the private methods.  I like
that technique because it makes me sad to see tests changing the objects header
file.

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:16:
PrerenderDispatcherTest() {}
const PrerenderDispatcher::PrerenderMap& prerender_urls() { return
prerender_dispatcher_.prerender_urls_; }

and use it to shorten all of the prerender_dispatcher_.prerender_urls_ below, as
well as getting const testing on access?

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:55:
prerender_dispatcher_.prerender_urls_.find(foo_url);
I don't like these automatics, and the scope they require us to make.  But I see
that we want to deref the element.  I propose:

if (prerender_urls().count(foo_url) == 1)
  EXPECT_EQ(2, prerender_urls().find(foo_url)->second);
else
  ADD_FAILURE() << "prerender_urls().count(foo_url) != 1";

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:56:
EXPECT_FALSE(prerender_dispatcher_.prerender_urls_.end() == foo_entry);
While we're here, how about we:

EXPECT_EQ(0, prerender_dispatcher_.prerender_urls_.count(bar_url));

and again on most of our other calls to map<>::find()?

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:111: }  // end
namespace prerender
Nit: // namespace prerender

[dominich, 2012-05-24 17:00:15]

Much happier with this interface. Thanks for the kick, Gavin.

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10424002/diff/6001/chrome/renderer/pre...
chrome/renderer/prerender/prerender_dispatcher.h:29: private:
On 2012/05/24 15:58:56, gavinp wrote:
> Rant you can ignore, but I must write down:
> 
> I prefer writing "friend class PrerenderDispatcherTest;" here, and getting rid
> of these macros, but that would require writing convenience functions in
> PrerenderDispatcherTest that basically pass-through the private methods.  I
like
> that technique because it makes me sad to see tests changing the objects
header
> file.

Noted.

[gavinp, 2012-05-24 17:39:40]

Should we also try removing more than we add, and removing urls that have never
been added?

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:36:
FRIEND_TEST_ALL_PREFIXES(PrerenderDispatcherTest, PrerenderDispatcherEmpty);
Do you need these now?  I think you don't, and probably not the #include either.

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:32:
PrerenderDispatcher::PrerenderMap::const_iterator entry = urls().find(url);
if (entry == urls().end())
  return 0;
EXPECT_GT(entry->second, 0);
return entry->second;

Is that reasonable?

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:50: 
Nit: remove this blank line.

[dominich, 2012-05-24 18:11:55]

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:36:
FRIEND_TEST_ALL_PREFIXES(PrerenderDispatcherTest, PrerenderDispatcherEmpty);
On 2012/05/24 17:39:40, gavinp wrote:
> Do you need these now?  I think you don't, and probably not the #include
either.

Done.

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:32:
PrerenderDispatcher::PrerenderMap::const_iterator entry = urls().find(url);
On 2012/05/24 17:39:40, gavinp wrote:
> if (entry == urls().end())
>   return 0;
> EXPECT_GT(entry->second, 0);
> return entry->second;
> 
> Is that reasonable?

Done.

http://codereview.chromium.org/10424002/diff/7005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:50: 
On 2012/05/24 17:39:40, gavinp wrote:
> Nit: remove this blank line.

Done.

[gavinp, 2012-05-24 18:17:29]

LGTM.

http://codereview.chromium.org/10424002/diff/19001/chrome/renderer/prerender/...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

http://codereview.chromium.org/10424002/diff/19001/chrome/renderer/prerender/...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:31: int
GetCountForURL(const GURL& url) const {
Good catch.  That was a style violation, this is not.

[commit-bot: I haz the power, 2012-05-24 18:26:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/10424002/19001

[commit-bot: I haz the power, 2012-05-24 20:39:43]

Change committed as 138877