Adding unit tests to RLZ code. Refactoring RLZ code to make it more testable.

Adding unit tests to RLZ code.  Refactoring RLZ code to make it more testable.
There is one new functionality, which is to support the CHROME_HOME_PAGE access
point, but in this CL that new function is disabled.  However, it is unit
tested.

BUG=None
TEST=See new unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98775

[cpu_(ooo_6.6-7.5), 2011-08-25 17:06:13]

On 2011/08/24 21:28:53, Roger Tawa wrote:

I am OOO today. I'll look at it tomorrow.

[Glenn Wilson, 2011-08-26 01:04:27]

http://codereview.chromium.org/7736001/diff/6006/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/7736001/diff/6006/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:186: registrar_.Add(this,
chrome::NOTIFICATION_OMNIBOX_OPENED_URL,
Do we know that NotificationService acts the same as the OmniboxUsageObserver,
even in RLZ's special edge cases?  Are there other consumers of the
OmniboxUsageObserver outside of this class?

[Roger Tawa OOO till Jul 10th, 2011-08-26 01:21:02]

Thanks Glenn.  Have a good vacation!

http://codereview.chromium.org/7736001/diff/6006/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/7736001/diff/6006/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:186: registrar_.Add(this,
chrome::NOTIFICATION_OMNIBOX_OPENED_URL,
On 2011/08/26 01:04:27, Glenn Wilson wrote:
> Do we know that NotificationService acts the same as the OmniboxUsageObserver,
> even in RLZ's special edge cases?  Are there other consumers of the
> OmniboxUsageObserver outside of this class?

I have not changed the behaviour wrt notifications.  OmniboxUsageObserver
registered for notifications and then deleted itself upon reception of the
event.  The new implementation registers for the same notifications and removes
itself from those events when received.  So really no behaviour change.

[cpu_(ooo_6.6-7.5), 2011-08-26 21:37:51]

Good job on the testing. See my comments so far:

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:70: // committed, which will happen before 2011-09-01. 
This constant will be
In the existing code here there is only one space after a period, lets keep it
that way.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:78: // Performs initialization of RLZ tracker that is
purposfully delayed so
purposfully: typo

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:79: // that it does not interfere with chrome startup
time.
That

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:95: friend class
base::RefCountedThreadSafe<RLZTracker>;
are we refcounted?

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
File chrome/browser/rlz/rlz_unittest.cc (right):

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:73: EXPECT_PRED_FORMAT2(CmpHelperSTRNC, str,
substr)
remove lines 33 to 73, this is overkill and only used once.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:93: bool pingnow_called() {
const function

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:98: return assume_io_thread_;
const function

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:160: }
remove lines 156 - 160

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:173:
::SetEnvironmentVariable(ASCIIToWide(env_vars::kHeadless).c_str(), NULL);
why?

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:217: 
so we don't delete the registry keys?

[Roger Tawa OOO till Jul 10th, 2011-08-27 02:00:05]

Thanks Carlos, addressed most comments.  See below for more details.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:70: // committed, which will happen before 2011-09-01. 
This constant will be
On 2011/08/26 21:37:51, cpu wrote:
> In the existing code here there is only one space after a period, lets keep it
> that way.

Did global search and replace in all files.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:78: // Performs initialization of RLZ tracker that is
purposfully delayed so
On 2011/08/26 21:37:51, cpu wrote:
> purposfully: typo

Done.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:79: // that it does not interfere with chrome startup
time.
On 2011/08/26 21:37:51, cpu wrote:
> That

Not sure what you mean.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz.h#ne...
chrome/browser/rlz/rlz.h:95: friend class
base::RefCountedThreadSafe<RLZTracker>;
On 2011/08/26 21:37:51, cpu wrote:
> are we refcounted?

In order to use NewRunnableMethod(), the class that declares that method must be
refcounted.  However, since this is a singleton, it will outlive the the tasks,
so not strictly needed.  For this reason I used the macro at line 160 below.

If there is a better way to do this please let me know.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
File chrome/browser/rlz/rlz_unittest.cc (right):

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:73: EXPECT_PRED_FORMAT2(CmpHelperSTRNC, str,
substr)
On 2011/08/26 21:37:51, cpu wrote:
> remove lines 33 to 73, this is overkill and only used once.

I think you meant 41-73.  Actually, I disagree.  If you look at how
EXPECT_PRED_FORMAT2() is implemented, it would be more difficult to integrate
with the existing gtest infrastructure and have the same output for consistency,
etc.  If I'm missing something please let me know.

Also, my plan is to eventually move this to a more common place so its available
to other tests.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:93: bool pingnow_called() {
On 2011/08/26 21:37:51, cpu wrote:
> const function

Done.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:98: return assume_io_thread_;
On 2011/08/26 21:37:51, cpu wrote:
> const function

Done.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:160: }
On 2011/08/26 21:37:51, cpu wrote:
> remove lines 156 - 160

Done.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:173:
::SetEnvironmentVariable(ASCIIToWide(env_vars::kHeadless).c_str(), NULL);
On 2011/08/26 21:37:51, cpu wrote:
> why?

See rlz.cc, line 213.  It seems like headless may defined for some tests, so
just to be safe I am clearing this env var to make sure the code will actually
run.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:217: 
On 2011/08/26 21:37:51, cpu wrote:
> so we don't delete the registry keys?

No need, done by the dtor of RegistryOverrideManager.

[cpu_(ooo_6.6-7.5), 2011-08-29 19:43:03]

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
File chrome/browser/rlz/rlz_unittest.cc (right):

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:73: EXPECT_PRED_FORMAT2(CmpHelperSTRNC, str,
substr)
On 2011/08/27 02:00:06, Roger Tawa wrote:
> On 2011/08/26 21:37:51, cpu wrote:
> > remove lines 33 to 73, this is overkill and only used once.
> 

basically on the only place where is called

if (NULL == strstr(str, substr))
  FAIL() << str; 

Or maybe use ASSERT_PRED1



> I think you meant 41-73.  Actually, I disagree.  If you look at how
> EXPECT_PRED_FORMAT2() is implemented, it would be more difficult to integrate
> with the existing gtest infrastructure and have the same output for
consistency,
> etc.  If I'm missing something please let me know.
> 
> Also, my plan is to eventually move this to a more common place so its
available
> to other tests.

http://codereview.chromium.org/7736001/diff/13005/chrome/browser/rlz/rlz_unit...
chrome/browser/rlz/rlz_unittest.cc:173:
::SetEnvironmentVariable(ASCIIToWide(env_vars::kHeadless).c_str(), NULL);
On 2011/08/27 02:00:06, Roger Tawa wrote:
> On 2011/08/26 21:37:51, cpu wrote:
> > why?
> 
> See rlz.cc, line 213.  It seems like headless may defined

Hmmm... better ask Pawel. Sounds fishy to me.

[cpu_(ooo_6.6-7.5), 2011-08-29 19:51:30]

lgtm

http://codereview.chromium.org/7736001/diff/19001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/7736001/diff/19001/chrome/browser/rlz/rlz.cc#n...
chrome/browser/rlz/rlz.cc:212: // because sometimes the very act of loading the
dll causes QEMU to crash.
I don't think this is true anymore. I believe you can remove this env var test.

[Roger Tawa OOO till Jul 10th, 2011-08-29 19:53:35]

Tbanks Carlos.

I will check with Pawel if the env var is still needed, and if not I'll remove
it from both rlz.cc and the unittest code.

I'll also see if there is a nicer want to do the EXPECT macros.

[Roger Tawa OOO till Jul 10th, 2011-08-30 03:26:42]

http://codereview.chromium.org/7736001/diff/19001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/7736001/diff/19001/chrome/browser/rlz/rlz.cc#n...
chrome/browser/rlz/rlz.cc:212: // because sometimes the very act of loading the
dll causes QEMU to crash.
On 2011/08/29 19:51:30, cpu wrote:
> I don't think this is true anymore. I believe you can remove this env var
test.

Done.