Refactor SetOmahaExperimentLabel out of gcpai and into install_util.

chrome/installer/util/install_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // See the corresponding header file for description of the functions in this
 // file.
 
 #include "chrome/installer/util/install_util.h"
 
 #include <shellapi.h>
@@ skipping 20 matching lines @@
 #include "chrome/installer/util/installation_state.h"
 #include "chrome/installer/util/l10n_string_util.h"
 #include "chrome/installer/util/util_constants.h"
 #include "chrome/installer/util/work_item_list.h"
 
 using base::win::RegKey;
 using installer::ProductState;
 
 namespace {
 
+const wchar_t* kExperimentAppGuids[] = {
+    L"{4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D}",

[grt (UTC plus 2), 2012/11/20 16:05:46]

this is the binaries' app guid.  what does it have to do with experiments?

[SteveT, 2012/11/20 23:23:50]

To be honest, I do not know why this is here. I can get in contact with the
original author about this.. and whether is needed here or not.

For now, I don't want to muck with the functionality of these utils... so what
do you think? Can we resolve this in a separate patch after I shoot out some
emails?

[grt (UTC plus 2), 2012/11/21 20:14:18]

sgtm

+    L"{8A69D345-D564-463C-AFF1-A69D9E530F96}",

[grt (UTC plus 2), 2012/11/20 16:05:46]

this is Chrome's app guid.

[SteveT, 2012/11/20 23:23:50]

Yes, and when I am done refactoring this code and start using it... I only want
to write to this particular GUID. So perhaps if we can figure out what 4DC8B4CA
is all about, I can save myself another bit of refactoring in the future.

+};
+
+const wchar_t kExperimentLabels[] = L"experiment_labels";
+
 const wchar_t kStageBinaryPatching[] = L"binary_patching";
 const wchar_t kStageBuilding[] = L"building";
 const wchar_t kStageConfiguringAutoLaunch[] = L"configuring_auto_launch";
 const wchar_t kStageCopyingPreferencesFile[] = L"copying_prefs";
 const wchar_t kStageCreatingShortcuts[] = L"creating_shortcuts";
 const wchar_t kStageEnsemblePatching[] = L"ensemble_patching";
 const wchar_t kStageExecuting[] = L"executing";
 const wchar_t kStageFinishing[] = L"finishing";
 const wchar_t kStagePreconditions[] = L"preconditions";
 const wchar_t kStageRefreshingPolicy[] = L"refreshing_policy";
@@ skipping 465 matching lines @@
                            date_str, arraysize(date_str));
   if (len) {
     --len;  // Subtract terminating \0.
   } else {
     PLOG(DFATAL) << "GetDateFormat";
   }
 
   return string16(date_str, len);
 }
 
+// static
+bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label,

[grt (UTC plus 2), 2012/11/20 16:05:46]

this should only exist and/or do anything useful for Google Chrome builds, no?

[SteveT, 2012/11/20 23:23:50]

Google Chrome and gcapi builds, from what I understand. Both of which link this
code.

Are you suggesting that I restrict the usage of this in other targets?

[grt (UTC plus 2), 2012/11/21 20:14:18]

I think the contents of this should likely be ifdef'd out for non-Google Chrome
branded builds so that if it's ever called from the context of the browser (I
guess that's the intention some day?) it will never do something harmful in a
Chromium build.

Lines 538-540 duplicate functionality in BrowserDistribution, which I think is
undesirable. I think we should chat a bit about the end goal, because I'm not
sure that a function for use by the browser should necessarily be the same as
one for use in GCAPI.

[SteveT, 2012/11/21 23:19:26]

Well we want this to run in the GCAPI DLL... is that still considered a Google
Chrome build?
Sure, we can chat offline and you can point me at any other points of
duplication and I can try to resolve them in this patch.

+                                          HKEY registry_hive) {
+  int successful_writes = 0;
+  for (int i = 0; i < arraysize(kExperimentAppGuids); ++i) {

[grt (UTC plus 2), 2012/11/20 16:05:46]

int -> size_t (arraysize returns a size_t) here and on previous line

[SteveT, 2012/11/20 23:23:50]

Done.

+    string16 experiment_path(google_update::kRegPathClientState);

[grt (UTC plus 2), 2012/11/20 16:05:46]

move this instance out of the loop and just assign to it so it isn't
ctor'd/dtor'd each time through.

[SteveT, 2012/11/20 23:23:50]

Done.

+    experiment_path += L"\\";
+    experiment_path += kExperimentAppGuids[i];
+
+    RegKey client_state(registry_hive, experiment_path.c_str(),

[grt (UTC plus 2), 2012/11/20 16:05:46]

this will create the key if it doesn't exist.  is this intended?  if not, please
use the Open() method instead of the ctor.

[SteveT, 2012/11/20 23:23:50]

Yes, I believe that is the case. In fact, this whole operation clobbers the
entire contents of the key, which is fine now, but not desirable when I start
adding new experiment labels. I have a plan to fix that, but it likely will not
be in this refactoring patch. Again, I do not want to break existing gcapi
functionality in this refactoring patch.

[grt (UTC plus 2), 2012/11/21 20:14:18]

Fortunately, this is not the case. If it were, this function would break Chrome
installs. It clobbers the contents of the experiment_labels value, but doesn't
modify the other contents of the app's ClientState key.
How will this function eventually be called?

[SteveT, 2012/11/21 23:19:26]

This is meant to be a dumb "write to (and over) this Omaha registry key" method.
This will be wrapped in more intelligent logic which will read the key and
ensure that we re-write the existing contents (rather than clobbering it all).

[SteveT, 2012/11/21 23:19:26]

Sorry, when I said clobbers the Key, I meant it clobbers the value. I think we
still want to create the Key if it doesn't exist, right? Otherwise this
operation would fail?

Is it possible for that ClientState key to not exist before this is called
during Chrome reactivation?
This is meant to be a dumb "write to (and over) this Omaha registry key" method.
This will be wrapped in more intelligent logic which will read the key and
ensure that we re-write the existing contents (rather than clobbering it all).

+                        KEY_SET_VALUE);
+    if (client_state.Valid() &&

[grt (UTC plus 2), 2012/11/20 16:05:46]

this check isn't needed

[SteveT, 2012/11/20 23:23:50]

Done.

+        client_state.WriteValue(kExperimentLabels,
+                                experiment_label.c_str()) == ERROR_SUCCESS) {
+      successful_writes++;

[grt (UTC plus 2), 2012/11/20 16:05:46]

prefer preincrement

[SteveT, 2012/11/20 23:23:50]

Done.

+    }
+  }
+
+  return (successful_writes == arraysize(kExperimentAppGuids));
+}
+
 // Open |path| with minimal access to obtain information about it, returning
 // true and populating |handle| on success.
 // static
 bool InstallUtil::ProgramCompare::OpenForInfo(const FilePath& path,
                                               base::win::ScopedHandle* handle) {
   DCHECK(handle);
   handle->Set(base::CreatePlatformFile(path, base::PLATFORM_FILE_OPEN, NULL,
                                        NULL));
   return handle->IsValid();
 }
@@ skipping 49 matching lines @@
   // Open the program and see if it references the expected file.
   base::win::ScopedHandle handle;
   BY_HANDLE_FILE_INFORMATION info = {};
 
   return (OpenForInfo(program, &handle) &&
           GetInfo(handle, &info) &&
           info.dwVolumeSerialNumber == file_info_.dwVolumeSerialNumber &&
           info.nFileIndexHigh == file_info_.nFileIndexHigh &&
           info.nFileIndexLow == file_info_.nFileIndexLow);
 }