Add a method to read the Omaha experiment_labels value.

Add a method to read the Omaha experiment_labels value.

This includes updated unit tests.

BUG=160251
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176154

[SteveT, 2013-01-09 17:44:08]

Follow up to my other patch. PTAL.

BTW I am having some issues manually testing this with python. I used Robert's
suggested code:

from ctypes import *
gc = windll.LoadLibrary('gcapi_dll')  // error
gc.TestFunc()

but the second line gives me this output:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\python_26_amd64\files\lib\ctypes\__init__.py", line 431, in LoadLibra
ry
    return self._dlltype(name)
  File "C:\python_26_amd64\files\lib\ctypes\__init__.py", line 353, in __init__
    self._handle = _dlopen(self._name, mode)
WindowsError: [Error 193] %1 is not a valid Win32 application

Any idea what the issue is off the top of your head? I haven't looked at it too
closely yet.

Thanks!


Steve

[grt (UTC plus 2), 2013-01-09 18:49:51]

what's python? ;-)

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings.cc:684: RegKey
client_state(reg_root, client_state_path.c_str(), KEY_READ);
KEY_READ -> KEY_QUERY_VALUE

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings_unittest.cc:130:
EXPECT_TRUE(GoogleUpdateSettings::ReadExperimentLabels(
this tests that Set/Read round-trips properly, but no longer tests that they
write to the expected registry location.  how about leaving the previous
validation check in place?

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings_unittest.cc:141:
EXPECT_FALSE(chrome->ShouldSetExperimentLabels());
add an expectation that ReadExperimentLables returns false here?

[SteveT, 2013-01-09 19:36:10]

Back to you.

Maybe I'll bug Robert about the python testing.  :)

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings.cc:684: RegKey
client_state(reg_root, client_state_path.c_str(), KEY_READ);
On 2013/01/09 18:49:51, grt wrote:
> KEY_READ -> KEY_QUERY_VALUE

Done. Care to explain the difference, though?

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings_unittest.cc:130:
EXPECT_TRUE(GoogleUpdateSettings::ReadExperimentLabels(
On 2013/01/09 18:49:51, grt wrote:
> this tests that Set/Read round-trips properly, but no longer tests that they
> write to the expected registry location.  how about leaving the previous
> validation check in place?

Makes sense, assuming we trust the validation code, which is sort of a repeat of
ReadExperimentsLabels, to some extent. :)

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings_unittest.cc:141:
EXPECT_FALSE(chrome->ShouldSetExperimentLabels());
On 2013/01/09 18:49:51, grt wrote:
> add an expectation that ReadExperimentLables returns false here?

Done.

[grt (UTC plus 2), 2013-01-10 02:58:51]

lgtm

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings.cc:684: RegKey
client_state(reg_root, client_state_path.c_str(), KEY_READ);
On 2013/01/09 19:36:10, SteveT wrote:
> On 2013/01/09 18:49:51, grt wrote:
> > KEY_READ -> KEY_QUERY_VALUE
> 
> Done. Care to explain the difference, though?

I think it's good practice to request least privileges. QUERY_VALUE is all
that's needed for the next line, so it's all that should be requested.

[SteveT, 2013-01-10 15:05:50]

Thanks!

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings.cc:684: RegKey
client_state(reg_root, client_state_path.c_str(), KEY_READ);
What's the difference between the two?

Feel free to point me to a link if you don't feel like writing a big blurb here
:)  I'm just curious for the future.

[commit-bot: I haz the power, 2013-01-10 15:06:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/11826028/3004

[grt (UTC plus 2), 2013-01-10 15:10:18]

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/11826028/diff/1/chrome/installer/util/google_...
chrome/installer/util/google_update_settings.cc:684: RegKey
client_state(reg_root, client_state_path.c_str(), KEY_READ);
On 2013/01/10 15:05:51, SteveT wrote:
> What's the difference between the two?
> 
> Feel free to point me to a link if you don't feel like writing a big blurb
here
> :)  I'm just curious for the future.

http://msdn.microsoft.com/library/windows/desktop/ms724878.aspx

[commit-bot: I haz the power, 2013-01-10 15:28:05]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-10 15:45:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/11826028/3008

[commit-bot: I haz the power, 2013-01-10 15:59:15]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-10 18:26:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/11826028/3008

[commit-bot: I haz the power, 2013-01-10 21:19:29]

Change committed as 176154