Start moving FinancialPing over to the cross-platform part.

Start moving FinancialPing over to the cross-platform part.

Since FinancialPing is fairly intertwined with rlz_lib.cc,
add a few stub methods on mac to keep this patch small for now.

Move all the enums in rlz_lib.h (which is still windows-only) into
a new file lib/rlz_enums.h.

Introduce lib/rlz_value_store, which will slowly grow more methods
to abstract away all registry reads. rlz_lib.cc will talk only to
RlzValueStore, and the windows implementation of that file will
write to the registry. Give this class 4 methods as proof-of-concept
for now, just enough to move FinancialPing over.

Don't move financial_ping_test.cc yet, as it more or less relies on
everything working to pass (it calls several functions from rlz_lib.cc).

landed in r72

[thakis, 2012-03-11 22:25:15]

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (left):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#oldcode71
lib/financial_ping.cc:71: LibMutex lock;
As far as I could tell, this lock was only used to protect SupplementaryBranding
in this function. I made it local to that piece of code. Please check if this
looks right.

[Roger Tawa (Google), 2012-03-12 16:31:04]

Thanks for all the work Nico.  A few comments below.

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (left):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#oldcode71
lib/financial_ping.cc:71: LibMutex lock;
On 2012/03/11 22:25:15, thakis wrote:
> As far as I could tell, this lock was only used to protect
SupplementaryBranding
> in this function. I made it local to that piece of code. Please check if this
> looks right.

It protects all registry accesses.  Some calls below, like GetAccessPointRlz()
and GetPingParams() access the registry.  While each of those methods does its
own locking, the lock here ensures that the entire operation of this function is
atomic.

So the lock should stay here.

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#newcode115
lib/financial_ping.cc:115: // http://crbug.com/117744
It does to support chrome reactivation on mac.

https://codereview.appspot.com/5754080/diff/1/lib/rlz_value_store.h
File lib/rlz_value_store.h (right):

https://codereview.appspot.com/5754080/diff/1/lib/rlz_value_store.h#newcode20
lib/rlz_value_store.h:20: // All methods in this class must be implemented in a
thread-safe way.
The requirement is actually stronger: must be safe across processes too.  Please
add this to the comment.

https://codereview.appspot.com/5754080/diff/1/mac/lib/rlz_value_store_mac.h
File mac/lib/rlz_value_store_mac.h (right):

https://codereview.appspot.com/5754080/diff/1/mac/lib/rlz_value_store_mac.h#n...
mac/lib/rlz_value_store_mac.h:23: #endif  // RLZ_MAC_LIB_RLZ_VALUE_STORE_MAC_H_
generally, more commentary would be good in this header.  also, get rid of extra
lines below.

[thakis, 2012-03-12 16:46:48]

Thanks for the quick review!

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (left):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#oldcode71
lib/financial_ping.cc:71: LibMutex lock;
On 2012/03/12 16:31:04, rogerta wrote:
> On 2012/03/11 22:25:15, thakis wrote:
> > As far as I could tell, this lock was only used to protect
> SupplementaryBranding
> > in this function. I made it local to that piece of code. Please check if
this
> > looks right.
> 
> It protects all registry accesses.  Some calls below, like GetAccessPointRlz()
> and GetPingParams() access the registry.  While each of those methods does its
> own locking, the lock here ensures that the entire operation of this function
is
> atomic.
> 
> So the lock should stay here.

Both GetAccessPointRlz() and GetPingParams() have their own locks. My plan is to
move their registry accesses to RlzValueStore and make the locks an
implementation detail of the windows implementation of RlzValueStore. That way,
platforms that don't need external locking don't need to use it (not sure which
mac apis i'll use on mac yet).

In general, it's good to have locks as narrowly scoped as possible.

(GetProductEventsAsCgi() also has its own lock, so that should be fine.)

GetMachineId() doesn't have an internal lock and uses statics, so I added a lock
to that block.

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#newcode115
lib/financial_ping.cc:115: // http://crbug.com/117744
On 2012/03/12 16:31:04, rogerta wrote:
> It does to support chrome reactivation on mac.

What's chrome reactivation? User toasts?

https://codereview.appspot.com/5754080/diff/1/lib/rlz_value_store.h
File lib/rlz_value_store.h (right):

https://codereview.appspot.com/5754080/diff/1/lib/rlz_value_store.h#newcode20
lib/rlz_value_store.h:20: // All methods in this class must be implemented in a
thread-safe way.
On 2012/03/12 16:31:04, rogerta wrote:
> The requirement is actually stronger: must be safe across processes too. 
Please
> add this to the comment.

Done.

https://codereview.appspot.com/5754080/diff/1/mac/lib/rlz_value_store_mac.h
File mac/lib/rlz_value_store_mac.h (right):

https://codereview.appspot.com/5754080/diff/1/mac/lib/rlz_value_store_mac.h#n...
mac/lib/rlz_value_store_mac.h:23: #endif  // RLZ_MAC_LIB_RLZ_VALUE_STORE_MAC_H_
On 2012/03/12 16:31:04, rogerta wrote:
> generally, more commentary would be good in this header.  also, get rid of
extra
> lines below.

Removed the empty lines. I'll add more commentary when I actually implement
these methods on mac :-) Added a TODO with bug link for now.

[Roger Tawa (Google), 2012-03-12 17:03:26]

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (left):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#oldcode71
lib/financial_ping.cc:71: LibMutex lock;
On 2012/03/12 16:46:48, thakis wrote:
> On 2012/03/12 16:31:04, rogerta wrote:
> > On 2012/03/11 22:25:15, thakis wrote:
> > > As far as I could tell, this lock was only used to protect
> > SupplementaryBranding
> > > in this function. I made it local to that piece of code. Please check if
> this
> > > looks right.
> > 
> > It protects all registry accesses.  Some calls below, like
GetAccessPointRlz()
> > and GetPingParams() access the registry.  While each of those methods does
its
> > own locking, the lock here ensures that the entire operation of this
function
> is
> > atomic.
> > 
> > So the lock should stay here.
> 
> Both GetAccessPointRlz() and GetPingParams() have their own locks. My plan is
to
> move their registry accesses to RlzValueStore and make the locks an
> implementation detail of the windows implementation of RlzValueStore. That
way,
> platforms that don't need external locking don't need to use it (not sure
which
> mac apis i'll use on mac yet).
> 
> In general, it's good to have locks as narrowly scoped as possible.
> 
> (GetProductEventsAsCgi() also has its own lock, so that should be fine.)
> 
> GetMachineId() doesn't have an internal lock and uses statics, so I added a
lock
> to that block.

Agree that locks should be as narrowly scoped as possible.  However, this code
should make all accesses to the new value store atomic, which it does not at the
moment.  So something needs to be done about that.  Whatever api you use on the
mac, it will need the same atomicity requirements as for windows.  The way rlz
works, one product can access the rlz store entries of another (read-only) and
must make sure that it does not read corrupted data.  So this same issue
exists/will exist for the mac implementation.


GetMachineId() does not need locking, correct.

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#newcode115
lib/financial_ping.cc:115: // http://crbug.com/117744
On 2012/03/12 16:46:48, thakis wrote:
> On 2012/03/12 16:31:04, rogerta wrote:
> > It does to support chrome reactivation on mac.
> 
> What's chrome reactivation? User toasts?

If chrome has been dormant or some time (that is, installed on the machine but
not used for some time) a partner installer which would normally install chrome
may ask the user instead if they want to start using chrome again, and will
launch it if the user agrees.  No need to reinstall chrome.  For proper
attribution of this reactivation, the installer will write a supplementary brand
code which is read here.

https://codereview.appspot.com/5754080/diff/1/mac/lib/rlz_value_store_mac.h
File mac/lib/rlz_value_store_mac.h (right):

https://codereview.appspot.com/5754080/diff/1/mac/lib/rlz_value_store_mac.h#n...
mac/lib/rlz_value_store_mac.h:23: #endif  // RLZ_MAC_LIB_RLZ_VALUE_STORE_MAC_H_
On 2012/03/12 16:46:48, thakis wrote:
> On 2012/03/12 16:31:04, rogerta wrote:
> > generally, more commentary would be good in this header.  also, get rid of
> extra
> > lines below.
> 
> Removed the empty lines. I'll add more commentary when I actually implement
> these methods on mac :-) Added a TODO with bug link for now.

sgtm

[thakis, 2012-03-12 18:52:13]

Doh, forgot to publish draft comments :-/

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (left):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#oldcode71
lib/financial_ping.cc:71: LibMutex lock;
> Agree that locks should be as narrowly scoped as possible.  However, this code
> should make all accesses to the new value store atomic, which it does not at
the
> moment.

My thinking was that the store implementations need to make sure that every call
to it is thread-safe. (Every method in the window implementation is using a lock
at the moment.)

Are you concerned about the data changing between two atomic read calls?

If so, one possible solution to that would be to make
RlzValueStore::GetInstance() private, and to have a scoped RlzValueStoreLock
class which takes the store lock while in scope, and which can hand out a
RlzValueStore object pointer. That way, the type system would make sure that the
store is always locked while it's being accessed. Does that sound ok?

> So something needs to be done about that.  Whatever api you use on the
> mac, it will need the same atomicity requirements as for windows.  The way rlz
> works, one product can access the rlz store entries of another (read-only) and
> must make sure that it does not read corrupted data.  So this same issue
> exists/will exist for the mac implementation.

Technically, the only rlz client on mac is chrome ;-)

> GetMachineId() does not need locking, correct.

It uses static variables, so it does need some kind of lock. Not necessarily the
same lock that the rlz store uses, though.

[Roger Tawa (Google), 2012-03-12 19:31:35]

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (left):

https://codereview.appspot.com/5754080/diff/1/lib/financial_ping.cc#oldcode71
lib/financial_ping.cc:71: LibMutex lock;
On 2012/03/12 18:52:13, thakis wrote:
> > Agree that locks should be as narrowly scoped as possible.  However, this
code
> > should make all accesses to the new value store atomic, which it does not at
> the
> > moment.
> 
> My thinking was that the store implementations need to make sure that every
call
> to it is thread-safe. (Every method in the window implementation is using a
lock
> at the moment.)
> 
> Are you concerned about the data changing between two atomic read calls?
> 
> If so, one possible solution to that would be to make
> RlzValueStore::GetInstance() private, and to have a scoped RlzValueStoreLock
> class which takes the store lock while in scope, and which can hand out a
> RlzValueStore object pointer. That way, the type system would make sure that
the
> store is always locked while it's being accessed. Does that sound ok?
> 
> > So something needs to be done about that.  Whatever api you use on the
> > mac, it will need the same atomicity requirements as for windows.  The way
rlz
> > works, one product can access the rlz store entries of another (read-only)
and
> > must make sure that it does not read corrupted data.  So this same issue
> > exists/will exist for the mac implementation.
> 
> Technically, the only rlz client on mac is chrome ;-)
> 
> > GetMachineId() does not need locking, correct.
> 
> It uses static variables, so it does need some kind of lock. Not necessarily
the
> same lock that the rlz store uses, though.

RlzValueStoreLock sounds like a great idea.

Yup, chrome is the only rlz client on the mac (for now ;-) ).

For GetMachiineId(), you are correct.  It does need a lock, just not the same as
the store lock.

[thakis, 2012-03-12 20:17:36]

Added the ScopedRlzValueStore concept as discussed. Please take another look.

[Roger Tawa (Google), 2012-03-13 17:24:39]

lgtm

Thanks Nico.  One comment below.  Can you run the windows unit tests before
committing?

http://codereview.appspot.com/5754080/diff/3032/win/lib/rlz_value_store_regis...
File win/lib/rlz_value_store_registry.cc (right):

http://codereview.appspot.com/5754080/diff/3032/win/lib/rlz_value_store_regis...
win/lib/rlz_value_store_registry.cc:52: return false;
Is it necessary to have the local locks in the method implementations?

[thakis, 2012-03-13 17:56:32]

Thanks! Tests pass, I'll land this once codesite's svn is back up.

http://codereview.appspot.com/5754080/diff/3032/win/lib/rlz_value_store_regis...
File win/lib/rlz_value_store_registry.cc (right):

http://codereview.appspot.com/5754080/diff/3032/win/lib/rlz_value_store_regis...
win/lib/rlz_value_store_registry.cc:52: return false;
On 2012/03/13 17:24:39, rogerta wrote:
> Is it necessary to have the local locks in the method implementations?

No, because RlzValueStoreRegistry methods are always guarded by the
ScopedRlzValueLock. I forgot to remove this, done now.