Add a ReadProductEvents() method to RlzValueStore.

Add a ReadProductEvents() method to RlzValueStore.

Change GetProductEventsAsCgiHelper() to use it.

Also address review comments on earlier CLs.
Committed: https://code.google.com/p/rlz/source/detail?r=88

[thakis, 2012-03-15 20:02:27]

As discussed via email (question 1).

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_lib.cc
File win/lib/rlz_lib.cc (left):

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_lib.cc#oldcode108
win/lib/rlz_lib.cc:108: num_values--;
This is no longer needed because the loop no longer goes 1 too far to check if
it's at the end.

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_lib.cc
File win/lib/rlz_lib.cc (right):

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_lib.cc#newcode67
win/lib/rlz_lib.cc:67: LONG GetProductEventsAsCgiHelper(rlz_lib::Product
product, char* cgi,
I kept the return type unchanged for now. This CL is already fairly complicated
as is.

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
File win/lib/rlz_value_store_registry.cc (right):

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
win/lib/rlz_value_store_registry.cc:135: // Max 32767 bytes according to MSDN,
but we never use that much.
A single very long key can render RLZ unfunctional, but that was true with the
old code as well as far as I can tell.

[Roger Tawa (Google), 2012-03-15 20:38:51]

See a couple of comments below.

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_lib.cc
File win/lib/rlz_lib.cc (right):

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_lib.cc#newcode67
win/lib/rlz_lib.cc:67: LONG GetProductEventsAsCgiHelper(rlz_lib::Product
product, char* cgi,
On 2012/03/15 20:02:27, thakis wrote:
> I kept the return type unchanged for now. This CL is already fairly
complicated
> as is.
OK.  When you cleanup the return value, you can probably also cleanup the DWORD
below.  I think it can be a size_t.

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
File win/lib/rlz_value_store_registry.cc (right):

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
win/lib/rlz_value_store_registry.cc:144: }
replace this loop with base::win::RegistryValueIterator instead of
RegEnumValueA() ?

[thakis, 2012-03-15 20:41:40]

Thanks!

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
File win/lib/rlz_value_store_registry.cc (right):

https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
win/lib/rlz_value_store_registry.cc:144: }
On 2012/03/15 20:38:51, rogerta wrote:
> replace this loop with base::win::RegistryValueIterator instead of
> RegEnumValueA() ?

Is it ok to do this as a follow-up (I'll write it immediately after landing
this)? This CL is about more about moving the code around, not about changing
it.

[Roger Tawa (Google), 2012-03-15 20:44:37]

lgtm

OK to do as follow up.

Thanks,
Roger

-



On Thu, Mar 15, 2012 at 16:41,  <thakis@chromium.org> wrote:
> Thanks!
>
>
>
>
https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
> File win/lib/rlz_value_store_registry.cc (right):
>
>
https://codereview.appspot.com/5837049/diff/4003/win/lib/rlz_value_store_regi...
> win/lib/rlz_value_store_registry.cc:144: }
> On 2012/03/15 20:38:51, rogerta wrote:
>>
>> replace this loop with base::win::RegistryValueIterator instead of
>> RegEnumValueA() ?
>
>
> Is it ok to do this as a follow-up (I'll write it immediately after
> landing this)? This CL is about more about moving the code around, not
> about changing it.
>
> https://codereview.appspot.com/5837049/