Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11280067:
Refactor SetOmahaExperimentLabel out of gcpai and into install_util. (Closed)
|
Created:
8 years, 1 month ago by SteveT Modified:
7 years, 11 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRefactor SetOmahaExperimentLabel out of gcpai and into install_util.
Change the method to no longer write to both App GUIDs, just the one that matches the distribution of the caller. This means that GCAPI will only write to the Google Chrome product's client state key, and not to the Chrome Binaries.
Replace SetOmahaExperimentLabel in the GCAPI code with a more specific version, and have SetOmahaExperimentLabel perform the general clobber-write to the registry.
This is in preparation for future work in using the experiment_labels to transmit Chrome Variations.
BUG=160251
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175671
Patch Set 1 : Init #
Total comments: 6
Patch Set 2 : nits addressed #
Total comments: 22
Patch Set 3 : grt nits #
Total comments: 2
Patch Set 4 : Added doc #
Total comments: 1
Patch Set 5 : Use distribution to determine appguid. #Patch Set 6 : doc #Patch Set 7 : Rebase #
Total comments: 28
Patch Set 8 : Addressed some additional grt comments. #Patch Set 9 : Rebase. DLL Entry point added. #
Total comments: 9
Patch Set 10 : rebase; grt comments; unit tests #
Total comments: 4
Patch Set 11 : robertshield nits #
Total comments: 7
Patch Set 12 : grt test comments #
Total comments: 4
Patch Set 13 : grt additional comments #Messages
Total messages: 37 (0 generated)
Hi Rob, Mind taking a look at this first refactor? FYI - I have yet to manually test this. I was hoping you could give me some advice on how to do that (can I run the gcapi DLL's exported methods somehow to ensure it does the right thing to the registry?) Thanks! Steve
looking good, some minor nits.
re. manual testing, the easiest way I've found is to use python and ctypes:
from ctypes import *
gc = windll.LoadLibrary('gcapi_dll.dll')
gc.ExportYouWantToCall(params)
https://codereview.chromium.org/11280067/diff/1001/chrome/installer/gcapi/gca...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):
https://codereview.chromium.org/11280067/diff/1001/chrome/installer/gcapi/gca...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:13: //#include
"chrome/installer/util/google_update_constants.h"
should delete the commented includes
https://codereview.chromium.org/11280067/diff/1001/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):
https://codereview.chromium.org/11280067/diff/1001/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:126: };
move these up next to the other constants
https://codereview.chromium.org/11280067/diff/1001/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:546: experiment_label.c_str()) ==
ERROR_SUCCESS) {
nit: collapse this to a single if to reduce indentation:
if (client_state.Valid() &&
client_state.WriteValue(kExperimentLabels,
experiment_label.c_str()) == ERROR_SUCCESS)
Okay, nits addressed. I'll try out your suggestions for testing before committing this. Didn't realize Python for Windows could do that. Cool! https://codereview.chromium.org/11280067/diff/1001/chrome/installer/gcapi/gca... File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right): https://codereview.chromium.org/11280067/diff/1001/chrome/installer/gcapi/gca... chrome/installer/gcapi/gcapi_omaha_experiment.cc:13: //#include "chrome/installer/util/google_update_constants.h" Oops, sorry. Was playing around with which were still necessary and forgot to remove them. Done! https://codereview.chromium.org/11280067/diff/1001/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/1001/chrome/installer/util/inst... chrome/installer/util/install_util.cc:126: }; Moved them up in alpha order as well. Thanks. Done. https://codereview.chromium.org/11280067/diff/1001/chrome/installer/util/inst... chrome/installer/util/install_util.cc:546: experiment_label.c_str()) == ERROR_SUCCESS) { On 2012/11/20 15:29:49, robertshield wrote: > nit: collapse this to a single if to reduce indentation: > > if (client_state.Valid() && > client_state.WriteValue(kExperimentLabels, > experiment_label.c_str()) == ERROR_SUCCESS) Done.
drive-by. is it possible to get rid of the duplication of constants? https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:42: L"{4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D}", this is the binaries' app guid. what does it have to do with experiments? https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:43: L"{8A69D345-D564-463C-AFF1-A69D9E530F96}", this is Chrome's app guid. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:534: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, this should only exist and/or do anything useful for Google Chrome builds, no? https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:537: for (int i = 0; i < arraysize(kExperimentAppGuids); ++i) { int -> size_t (arraysize returns a size_t) here and on previous line https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:538: string16 experiment_path(google_update::kRegPathClientState); move this instance out of the loop and just assign to it so it isn't ctor'd/dtor'd each time through. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:542: RegKey client_state(registry_hive, experiment_path.c_str(), this will create the key if it doesn't exist. is this intended? if not, please use the Open() method instead of the ctor. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:544: if (client_state.Valid() && this check isn't needed https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:547: successful_writes++; prefer preincrement
Thanks for the comments. PTAL. Re: Duplication of constants: Are you referring to the App GUIDS in kExperimentAppGuids? Where else in the common installer are they defined? https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:42: L"{4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D}", 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? https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:43: L"{8A69D345-D564-463C-AFF1-A69D9E530F96}", 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. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:534: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, 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? https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:537: for (int i = 0; i < arraysize(kExperimentAppGuids); ++i) { On 2012/11/20 16:05:46, grt wrote: > int -> size_t (arraysize returns a size_t) here and on previous line Done. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:538: string16 experiment_path(google_update::kRegPathClientState); On 2012/11/20 16:05:46, grt wrote: > move this instance out of the loop and just assign to it so it isn't > ctor'd/dtor'd each time through. Done. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:542: RegKey client_state(registry_hive, experiment_path.c_str(), 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. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:544: if (client_state.Valid() && On 2012/11/20 16:05:46, grt wrote: > this check isn't needed Done. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:547: successful_writes++; On 2012/11/20 16:05:46, grt wrote: > prefer preincrement Done.
The definitive location for the app guids is in the various implementations of BrowserDistribution. If this utility function is ever called from Chrome or Chrome's installer, then it really should go through the relevant BrowserDistribution instances. I think we should chat about this for a few minutes so I can better understand the end goal. This will help me give better feedback. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:42: L"{4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D}", On 2012/11/20 23:23:50, SteveT wrote: > 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? sgtm https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:534: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, On 2012/11/20 23:23:50, SteveT wrote: > 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? 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. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:542: RegKey client_state(registry_hive, experiment_path.c_str(), On 2012/11/20 23:23:50, SteveT wrote: > Yes, I believe that is the case. In fact, this whole operation clobbers the > entire contents of the key, 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. > 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. How will this function eventually be called? https://codereview.chromium.org/11280067/diff/10001/chrome/installer/gcapi/gc... File chrome/installer/gcapi/gcapi_omaha_experiment.h (right): https://codereview.chromium.org/11280067/diff/10001/chrome/installer/gcapi/gc... chrome/installer/gcapi/gcapi_omaha_experiment.h:8: bool SetGCAPIOmahaExperimentLabel(const wchar_t* brand_code, int shell_mode); please add a doc comment here.
Hey Greg, sounds like we need to discuss this a bit more so you understand what I'm up to with these changes. I will tag you in my design doc so you can have a look, and we can discuss offline what your thoughts are on my changes. You guys are the experts, so I'd rather defer to you before making these changes! https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:534: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, On 2012/11/21 20:14:18, grt wrote: > On 2012/11/20 23:23:50, SteveT wrote: > > 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? > > 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. Well we want this to run in the GCAPI DLL... is that still considered a Google Chrome 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. 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. https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:542: RegKey client_state(registry_hive, experiment_path.c_str(), 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). https://codereview.chromium.org/11280067/diff/5002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:542: RegKey client_state(registry_hive, experiment_path.c_str(), On 2012/11/21 20:14:18, grt wrote: > On 2012/11/20 23:23:50, SteveT wrote: > > Yes, I believe that is the case. In fact, this whole operation clobbers the > > entire contents of the key, > > 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. 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? > > > 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. > > How will this function eventually be called? 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). https://codereview.chromium.org/11280067/diff/10001/chrome/installer/gcapi/gc... File chrome/installer/gcapi/gcapi_omaha_experiment.h (right): https://codereview.chromium.org/11280067/diff/10001/chrome/installer/gcapi/gc... chrome/installer/gcapi/gcapi_omaha_experiment.h:8: bool SetGCAPIOmahaExperimentLabel(const wchar_t* brand_code, int shell_mode); On 2012/11/21 20:14:18, grt wrote: > please add a doc comment here. Done.
Hi Steve. Major apologies for being unavailable to discuss this in person today. I've hammered all of my thoughts into the CR so you can read it and digest it a bit. I'll be free tomorrow afternoon to discuss in person if you'd like. Cheers, Greg https://chromiumcodereview.appspot.com/11280067/diff/1002/chrome/installer/ut... File chrome/installer/util/install_util.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/1002/chrome/installer/ut... chrome/installer/util/install_util.cc:534: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, If this is meant to be called from the context of chrome.exe, then it could potentially be called for Chromium, Google Chrome, Google Chrome SxS (canaray), Google Chrome Frame, or Google Chrome App Host (or possibly even setup.exe). Cases like this are handled by the BrowserDistribution class -- the base class handles the Chromium case, and various derived types handle all of the others. I think the right thing to do here is to add something like: virtual bool HasExperiments(); to BrowserDistribution that returns false. Override this in GoogleChromeDistribution (return true), GoogleChromeSxSDistribution (return false), and GoogleChromeBinariesDistribution (return true). Then in this function, assuming it is to be called from chrome.exe and GCAPI, you do something like this: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); if (dist->HasExperiments()) { string16 client_state_path(dist->GetStateKey()); RegKey client_state(root, client_state_root.c_str(), KEY_SET_VALUE); if (client_state.WriteValue(...) == ERROR_SUCCESS) { ... } } This will only write to the registry for the products that actually have experiments, and you get the correct registry location by going through the GetStateKey method. Since GCAPI is only meant to be used on Google Chrome, I think the call to GetDistribution() will return the proper instance automatically. If you need to preserve the behavior of writing into the key for the binaries (I don't know why you would, but I haven't read any design docs so I'm completely uninformed), then you'll furthermore need to do something like: if (InstallUtil::IsMultiInstall(dist, system_install)) { BrowserDistribution *binaries_dist = BrowserDistribution::GetSpecificDistribution(BrowserDistribution:: CHROME_BINARIES); // do the same HasExperiments check such for binaries_dist } Now come the extra twists. Have a drink while you read on. :-) To handle the case of per-user vs. per-machine installs, the utility functions in here typically take a |system_level| bool in param. Callers either already know if they're user or system, or they use !InstallUtil::IsPerUserInstall(...) to figure it out. This function should follow the same parameter, which means replacing the HKEY registry_hive param with a bool system_level param, and then inside the func have something like: HKEY root = system_level ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; But wait, there's more! System-level Chrome doesn't have permission to write to ClientState in HKLM. Omaha creates a lower-rights key called ClientStateMedium for use in this case. I've just looked at the corresponding Omaha code, and it does not appear to read from ClientStateMedium, so you have a small problem here. You have two options: have Omaha give priority to ClientStateMedium for per-machine installs (this is how things like the EULA-accepted flag are handled), or use Omaha facilities to launch setup.exe at high integrity and add special handling to setup.exe to set this value. I think the former solution is easier, but it means that any user-level process on a machine can potentially tweak the value. Maybe this isn't a problem. If so, it's not a difficult thing to change Omaha's behavior.
Had a question:
If I use your suggested code in my SetOmahaExperimentLabel method:
BrowserDistribution* dist = BrowserDistribution::GetDistribution();
if (dist->HasExperiments()) {
string16 client_state_path(dist->GetStateKey());
RegKey client_state(root, client_state_root.c_str(), KEY_SET_VALUE);
if (client_state.WriteValue(...) == ERROR_SUCCESS) { ... }
}
We run into the problem where the GCAPI call to
BrowserDistribution::GetDistribution()->GetStateKey() will result in just one of
the two App GUIDS it writes to today (presumably the Google Chrome App GUID). So
how do I make sure we also write to the Chrome binaries app guid?
Again, I am trying to avoid changing the GCAPI functionality before verifying
with them if they really need to write to both GUIDs (will contact them
presently)...
On 2012/11/23 16:48:46, SteveT wrote:
> Had a question:
>
> If I use your suggested code in my SetOmahaExperimentLabel method:
>
> BrowserDistribution* dist = BrowserDistribution::GetDistribution();
> if (dist->HasExperiments()) {
> string16 client_state_path(dist->GetStateKey());
> RegKey client_state(root, client_state_root.c_str(), KEY_SET_VALUE);
> if (client_state.WriteValue(...) == ERROR_SUCCESS) { ... }
> }
>
> We run into the problem where the GCAPI call to
> BrowserDistribution::GetDistribution()->GetStateKey() will result in just one
of
> the two App GUIDS it writes to today (presumably the Google Chrome App GUID).
So
> how do I make sure we also write to the Chrome binaries app guid?
>
> Again, I am trying to avoid changing the GCAPI functionality before verifying
> with them if they really need to write to both GUIDs (will contact them
> presently)...
I think the other block of code is safe to use when called from GCAPI as well
(where |dist| is the result of the previous call to GetDistribution()):
if (InstallUtil::IsMultiInstall(dist, system_install)) {
BrowserDistribution *binaries_dist =
BrowserDistribution::GetSpecificDistribution(BrowserDistribution::
CHROME_BINARIES);
// do the same HasExperiments check and such for binaries_dist
}
On 2012/11/23 18:26:37, grt wrote:
> On 2012/11/23 16:48:46, SteveT wrote:
> > Had a question:
> >
> > If I use your suggested code in my SetOmahaExperimentLabel method:
> >
> > BrowserDistribution* dist = BrowserDistribution::GetDistribution();
> > if (dist->HasExperiments()) {
> > string16 client_state_path(dist->GetStateKey());
> > RegKey client_state(root, client_state_root.c_str(), KEY_SET_VALUE);
> > if (client_state.WriteValue(...) == ERROR_SUCCESS) { ... }
> > }
> >
> > We run into the problem where the GCAPI call to
> > BrowserDistribution::GetDistribution()->GetStateKey() will result in just
one
> of
> > the two App GUIDS it writes to today (presumably the Google Chrome App
GUID).
> So
> > how do I make sure we also write to the Chrome binaries app guid?
> >
> > Again, I am trying to avoid changing the GCAPI functionality before
verifying
> > with them if they really need to write to both GUIDs (will contact them
> > presently)...
>
> I think the other block of code is safe to use when called from GCAPI as well
> (where |dist| is the result of the previous call to GetDistribution()):
>
> if (InstallUtil::IsMultiInstall(dist, system_install)) {
> BrowserDistribution *binaries_dist =
> BrowserDistribution::GetSpecificDistribution(BrowserDistribution::
> CHROME_BINARIES);
> // do the same HasExperiments check and such for binaries_dist
> }
So
BrowserDistribution::GetSpecificDistribution(BrowserDistribution::CHROME_BINARIES)->GetStateKey()
will give me the right Key for the Binaries App GUID. (which is the answer I was
going for) However, how I need to figure out how to specify that JUST for GCAPI
(since my new use cases will not write to the Chrome Binaries App GUID)
I guess I can add a "bool write_to_binaries" which, when set, causes the method
to write to both the Google Chrome product and Chrome Binaries' keys. How does
that sound?
On 2012/11/23 18:32:01, SteveT wrote:
> On 2012/11/23 18:26:37, grt wrote:
> > On 2012/11/23 16:48:46, SteveT wrote:
> > > Had a question:
> > >
> > > If I use your suggested code in my SetOmahaExperimentLabel method:
> > >
> > > BrowserDistribution* dist = BrowserDistribution::GetDistribution();
> > > if (dist->HasExperiments()) {
> > > string16 client_state_path(dist->GetStateKey());
> > > RegKey client_state(root, client_state_root.c_str(), KEY_SET_VALUE);
> > > if (client_state.WriteValue(...) == ERROR_SUCCESS) { ... }
> > > }
> > >
> > > We run into the problem where the GCAPI call to
> > > BrowserDistribution::GetDistribution()->GetStateKey() will result in just
> one
> > of
> > > the two App GUIDS it writes to today (presumably the Google Chrome App
> GUID).
> > So
> > > how do I make sure we also write to the Chrome binaries app guid?
> > >
> > > Again, I am trying to avoid changing the GCAPI functionality before
> verifying
> > > with them if they really need to write to both GUIDs (will contact them
> > > presently)...
> >
> > I think the other block of code is safe to use when called from GCAPI as
well
> > (where |dist| is the result of the previous call to GetDistribution()):
> >
> > if (InstallUtil::IsMultiInstall(dist, system_install)) {
> > BrowserDistribution *binaries_dist =
> > BrowserDistribution::GetSpecificDistribution(BrowserDistribution::
> > CHROME_BINARIES);
> > // do the same HasExperiments check and such for binaries_dist
> > }
>
> So
>
BrowserDistribution::GetSpecificDistribution(BrowserDistribution::CHROME_BINARIES)->GetStateKey()
> will give me the right Key for the Binaries App GUID. (which is the answer I
was
> going for).
Yes. It will be the Chromium or the Google Chrome one based on the brand used at
compile-time.
> However, how I need to figure out how to specify that JUST for GCAPI
> (since my new use cases will not write to the Chrome Binaries App GUID)
>
> I guess I can add a "bool write_to_binaries" which, when set, causes the
method
> to write to both the Google Chrome product and Chrome Binaries' keys. How does
> that sound?
Can you just stop writing to the binaries' key altogether?
After chatting with Robert, we've decided to contact mgraboski@ to see if we can just stop with this Chrome Binaries nonsense (you're cc'd). In the meantime, I'll continue on this patch assuming the answer is "yes, let's stop writing to that app guid".
Hey Greg, PTAL and let me know if this resolves some of the major issues you pointed out earlier. * I got rid of the repeated App GUIDs now that we use BrowserDistribution to do everything. * I purposely did not set the chrome_frame_distribution to return true for ShouldWriteExperimentLabels, because currently Chrome Variations is not concerned about CF. This is obviously very easy to change in the future if we change our minds on this. Note that we're still blocked on a response about whether or not we can stop writing to Chrome Binaries, as well as manual testing to ensure everything falls into the right places. Thanks for your help with all this!
Hi Steve. This looks really good. Comments below. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/gcapi/gc... File chrome/installer/gcapi/gcapi_omaha_experiment.h (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/gcapi/gc... chrome/installer/gcapi/gcapi_omaha_experiment.h:13: bool SetGCAPIOmahaExperimentLabel(const wchar_t* brand_code, int shell_mode); "SetReactivationExperimentLabels" seems more accurate. WDYT? https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:169: virtual bool ShouldWriteExperimentLabels() const; I admire (and share) your desire to make this const. In the interest of staying close to the style of the existing methods, I think it's best to make it non-const. Otherwise readers may spend time trying to figure out why this one is special. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:169: virtual bool ShouldWriteExperimentLabels() const; While writing the experiment labels is the fine-grained operation that this bit controls, I wonder if there's a broader name that is even more applicable. Is it really only the writing of the value that's special to Google Chrome, or is there anything else like the way field trials are set up, the way they're named, etc? Is it possible that other code in Chrome might query this bit to decide how to set up, participate in, or report on field trials? If you stick with this name, I suggest "ShouldSetExperimentLabels" for consistency with the label-setting function. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_distribution.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... chrome/installer/util/google_chrome_distribution.cc:880: bool GoogleChromeDistribution::ShouldWriteExperimentLabels() const { I think this should be moved down below the #endif. You'll need an implementation in the _dummy.cc, too. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... chrome/installer/util/google_chrome_sxs_distribution.cc:75: return true; Note: GCAPI doesn't have facilities for changing the labels for canary. I'd expected to see "false" here as a result. If canary really should be writing the labels, then you can remove this override since the SxS class extends the GoogleChrome class. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:42: const wchar_t kExperimentLabels[] = L"experiment_labels"; Since this is a value defined by Omaha, please move it into google_update_constants.{cc,h}. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:525: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, Upon further consideration, I think this belongs in google_update_settings.{cc,h} since that's where most of our Omaha-interop stuff lives. In that case, the name can be simplified to just SetExperimentLabels (plural for consistency with the name of the registry value). https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:527: HKEY registry_hive = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; nit: other code in the installer uses either the name reg_root or just root for this. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.h:179: // ClientState key for this Chrome product. If |system_install| is true, this The HKLM/HKCU part of the comment is a bit superfluous, since that's how everything in here works. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.h:181: // if the label was successfully written, false otherwise (even if the label I think it's less ambiguous from the standpoint of callers for false to represent only an error condition rather than either an error or noop. Unless, of course, the thing a caller would care about is whether or not the registry was modified. Do you have an idea of what's more important (e.g., being able to log that a value was written vs. being able to log that there was an error trying to write a value)? https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.h:183: static bool SetOmahaExperimentLabel(const string16& experiment_label, I think it's more consistent with the surrounding code for system_install to precede experiment_labels.
Two final thoughts for now. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:525: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, On 2012/11/24 02:58:37, grt wrote: > Upon further consideration, I think this belongs in > google_update_settings.{cc,h} since that's where most of our Omaha-interop stuff > lives. In that case, the name can be simplified to just SetExperimentLabels > (plural for consistency with the name of the registry value). Also, please add a test case to google_update_settings_unittest.cc. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:538: success = client_state.WriteValue(kExperimentLabels, It looks like Omaha deletes the value in the registry when its setter is called with an empty string. For consistency, I imagine you should do the same here.
Hey Greg - had a couple questions inline... Addressed most of everything else. PTAL. I still have to write that unittest - currently trying to figure out how this google_update_settings_unittest stuff works. I'll come by if I have more questions about it. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/gcapi/gc... File chrome/installer/gcapi/gcapi_omaha_experiment.h (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/gcapi/gc... chrome/installer/gcapi/gcapi_omaha_experiment.h:13: bool SetGCAPIOmahaExperimentLabel(const wchar_t* brand_code, int shell_mode); Sure, let's be more specific now that we've refactored out some more general functionality. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:169: virtual bool ShouldWriteExperimentLabels() const; I _was_ a bit confused that similar methods in this class (and subclasses) were not const. But better readability/understandability sounds good to me. However, I also think that future readers might come across the same initial confusion that I did :\ https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:169: virtual bool ShouldWriteExperimentLabels() const; I thought about your original suggestion "HasExperiments", and realized that it's a bit inaccurate, in that almost all Chrome binaries _have_ experiments/field trials, but not all of them should be setting these values in the registry. For example, I chose, in this latest patch set, to exclude Chrome Frame as distributions that writes experiment labels, because we currently do not care to do that for CF. I've renamed this to ShouldSetExperimentLabels. Thanks for pointing that out. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_distribution.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... chrome/installer/util/google_chrome_distribution.cc:880: bool GoogleChromeDistribution::ShouldWriteExperimentLabels() const { On 2012/11/24 02:58:37, grt wrote: > I think this should be moved down below the #endif. You'll need an > implementation in the _dummy.cc, too. Done. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... chrome/installer/util/google_chrome_sxs_distribution.cc:75: return true; Hmm, well I guess this "ShouldWriteExperimentLabels" semantically refers to whether or not we should write them for FieldTrials for Chrome... not necessarily whether or not we should write them for GCAPI. Should GCAPI be doing this for Chrome Frame installs (writing their brand_whatever entry in experiment_labels)? If so, we have to change this logic around a bit.. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:42: const wchar_t kExperimentLabels[] = L"experiment_labels"; On 2012/11/24 02:58:37, grt wrote: > Since this is a value defined by Omaha, please move it into > google_update_constants.{cc,h}. Done. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:525: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, Moved it. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:525: bool InstallUtil::SetOmahaExperimentLabel(const string16& experiment_label, Currently trying to understand the tests in google_update_settings_unittest.cc (I'm guessing you mean the one under chrome/installer/util, as there is also one under chrome/browser/google as well. I'll let you have a look at the questions in my response (which my change a few things) before responding with a test. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:527: HKEY registry_hive = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; On 2012/11/24 02:58:37, grt wrote: > nit: other code in the installer uses either the name reg_root or just root for > this. Done. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:538: success = client_state.WriteValue(kExperimentLabels, Okay, done. The return value will also represent the success or failure of the erase (I'll assume the caller knows whether or not they are passing an empty string into this). https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.h:179: // ClientState key for this Chrome product. If |system_install| is true, this K, removed. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.h:181: // if the label was successfully written, false otherwise (even if the label GCAPI doesn't seem to care at all what the result is - they don't use it at all (so I don't know the answer for that use case). I suspect that my code is going to be a best-effort attempt, too. What's the best practice for cases where you fail to write a registry key? Are there cases where this retries later on a timer? Or are such errors ever logged? https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.h:183: static bool SetOmahaExperimentLabel(const string16& experiment_label, On 2012/11/24 02:58:37, grt wrote: > I think it's more consistent with the surrounding code for system_install to > precede experiment_labels. Done.
Looking good. I forgot to mention: voluminous review feedback from me is a sign of affection. :-) https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... chrome/installer/util/google_chrome_sxs_distribution.cc:75: return true; On 2012/11/26 20:45:06, SteveT wrote: > Hmm, well I guess this "ShouldWriteExperimentLabels" semantically refers to > whether or not we should write them for FieldTrials for Chrome... not > necessarily whether or not we should write them for GCAPI. What I meant was that the original GCAPI code didn't write the reactivation label for canary (because GCAPI only does a "reactivation" for Google Chrome). > Should GCAPI be doing this for Chrome Frame installs (writing their > brand_whatever entry in experiment_labels)? I don't think so. GCAPI is for Google Chrome, not canary or Chrome Frame. > If so, we have to change this logic > around a bit.. https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... chrome/installer/util/install_util.h:181: // if the label was successfully written, false otherwise (even if the label On 2012/11/26 20:45:06, SteveT wrote: > GCAPI doesn't seem to care at all what the result is - they don't use it at all > (so I don't know the answer for that use case). I suspect that my code is going > to be a best-effort attempt, too. > > What's the best practice for cases where you fail to write a registry key? Are > there cases where this retries later on a timer? Or are such errors ever logged? In the installer, we make heavy use of LOG(foo) and VLOG(1). For regular Chrome, DLOG(ERROR) is appropriate. Since this is regular Chrome code, you could DLOG in the implementation if you fail to set/delete the value. In that case, it'd nice to stream out the return value from the RegKey operation so that it appears in the log.
On 2012/11/26 21:19:05, grt wrote: > Looking good. > > I forgot to mention: voluminous review feedback from me is a sign of affection. > :-) I view it as a friendly learning experience :) > > https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/goo... > chrome/installer/util/google_chrome_sxs_distribution.cc:75: return true; > On 2012/11/26 20:45:06, SteveT wrote: > > Hmm, well I guess this "ShouldWriteExperimentLabels" semantically refers to > > whether or not we should write them for FieldTrials for Chrome... not > > necessarily whether or not we should write them for GCAPI. > > What I meant was that the original GCAPI code didn't write the reactivation > label for canary (because GCAPI only does a "reactivation" for Google Chrome). > > > Should GCAPI be doing this for Chrome Frame installs (writing their > > brand_whatever entry in experiment_labels)? > > I don't think so. GCAPI is for Google Chrome, not canary or Chrome Frame. Okay, so the issue here is that I lost sight of what we're trying to achieve with the ShouldSetExperimentLabels method. The semantics of that method is misused in my current patchset. I think what you _meant_ for that method to do is to tell the caller if this browser distribution has _Field Trials_ that need to be written to experiment_labels (which is my future use case). GCAPI should not care about this. More explicitly: GCAPI only wants to write its "brand_X" entry in experiment_labels for Google Chrome. Field Trials wants to write this for Google Chrome and Chrome Canary and maybe Chrome Frame in the future. So ShouldSetExperimentLabels shouldn't be used in the GCAPI case. What this means is that google_update_settings::SetExperimentLabels method should not call ShouldSetExperimentLabels. It should just dumbly write whatever experiment_labels the caller gives is. The caller (GCAPI or Field Trials) should determine whether or not it should call SetExperimentLabels in the first place. Does this make sense? Feel free to fire back if you feel otherwise and we'll work this out. Again, my goal with this patch is to not mess with the semantics of GCAPI, and just factor out common code that I can reuse. I'm happy to move the new BrowserDistribution::ShouldSetExperimentLabels stuff to a separate patch since technically it should be orthogonal to all this. > > > If so, we have to change this logic > > around a bit.. > > https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... > File chrome/installer/util/install_util.h (right): > > https://codereview.chromium.org/11280067/diff/15001/chrome/installer/util/ins... > chrome/installer/util/install_util.h:181: // if the label was successfully > written, false otherwise (even if the label > On 2012/11/26 20:45:06, SteveT wrote: > > GCAPI doesn't seem to care at all what the result is - they don't use it at > all > > (so I don't know the answer for that use case). I suspect that my code is > going > > to be a best-effort attempt, too. > > > > What's the best practice for cases where you fail to write a registry key? Are > > there cases where this retries later on a timer? Or are such errors ever > logged? > > In the installer, we make heavy use of LOG(foo) and VLOG(1). For regular Chrome, > DLOG(ERROR) is appropriate. Since this is regular Chrome code, you could DLOG > in the implementation if you fail to set/delete the value. In that case, it'd > nice to stream out the return value from the RegKey operation so that it appears > in the log.
Hey Greg,
Earlier we had our fingers crossed that calling
BrowserDistribution::GetDistribution() would "do the right thing" in gcapi (that
is, we were hoping it would return the Google Chrome distribution), but it turns
out this isn't the case. In fact, it crashes, as it has troubles accessing some
global state like MasterPrefs, CommandLine and other things that are available
in Chrome but not in this standalone DLL.
Not sure if this crash is something we can hack through, or if it's a
fundamental issue with the use of BrowserDistribution. Thoughts?
=====
Some details: I get this stacktrace when I call into BrowserDistribution for the
first time:
[1127/155052:FATAL:command_line.cc(198)] Check failed: current_process_commandli
ne_.
Backtrace:
base::debug::StackTrace::StackTrace [0x02669731+33] (s:\gitdepot\src\bas
e\debug\stack_trace_win.cc:171)
logging::LogMessage::~LogMessage [0x026BF62E+94] (s:\gitdepot\src\base\l
ogging.cc:564)
CommandLine::ForCurrentProcess [0x026551F3+227] (s:\gitdepot\src\base\co
mmand_line.cc:199)
installer::MasterPreferences::MasterPreferences [0x100218F4+116] (s:\git
depot\src\chrome\installer\util\master_preferences.cc:82)
base::DefaultLazyInstanceTraits<installer::MasterPreferences>::New [0x10
0242E0+288] (s:\gitdepot\src\base\lazy_instance.h:68)
base::LazyInstance<installer::MasterPreferences,base::DefaultLazyInstanc
eTraits<installer::MasterPreferences> >::Pointer [0x100232FC+124] (s:\gitdepot\s
rc\base\lazy_instance.h:159)
base::LazyInstance<installer::MasterPreferences,base::DefaultLazyInstanc
eTraits<installer::MasterPreferences> >::Get [0x10022DB6+22] (s:\gitdepot\src\ba
se\lazy_instance.h:134)
installer::MasterPreferences::ForCurrentProcess [0x10022B9D+13] (s:\gitd
epot\src\chrome\installer\util\master_preferences.cc:314)
`anonymous namespace'::GetCurrentDistributionType [0x100151EF+79] (s:\gi
tdepot\src\chrome\installer\util\browser_distribution.cc:60)
BrowserDistribution::GetDistribution [0x10015338+8] (s:\gitdepot\src\chr
ome\installer\util\browser_distribution.cc:89)
GoogleUpdateSettings::SetExperimentLabels [0x10014A54+100] (s:\gitdepot\
src\chrome\installer\util\google_update_settings.cc:656)
SetReactivationExperimentLabels [0x1000D888+216] (s:\gitdepot\src\chrome
\installer\gcapi\gcapi_omaha_experiment.cc:87)
TestLabels [0x1000442B+59] (s:\gitdepot\src\chrome\installer\gcapi\gcapi
.cc:650)
The problem (which is neither your doing nor insurmountable) is that gcapi is calling into parts of base/ that haven't been initialized. gcapi will need a DllMain entrypoint that minimally does: // Initialize the commandline singleton from the environment. CommandLine::Init(0, NULL); and possibly also: // The exit manager is in charge of calling the dtors of singletons. base::AtExitManager exit_manager; during DLL_PROCESS_ATTACH and the inverse during DLL_PROCESS_DETACH. I can give you a hand with this if it's new territory for you.
Hey there! Finally got around to some basic manual testing and clean up that ensures this actually runs and does the right thing. I've added a DLLMain as suggested, to handle some init tasks. I still need to write some unit tests - suggestions here are welcome. I noticed that there are some gcapi_test files, but I'm not sure how to proceed with them yet. I also need to do a complete E2E test to ensure I haven't broken the ReactivateChrome stuff refactored in this patch. Will talk to Robert about that. In the meantime, PTAL at the new changes in gcapi_dll.cc.
lookin' good! google_update_settings_unittest.cc is a good place to add a unit test. the GoogleUpdateSettingsTest fixture does some registry redirection so that tests can safely muck with the registry. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... File chrome/installer/gcapi/gcapi_dll.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:4: #include <windows.h> for things like WINAPI, HINSTANCE, etc. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:11: base::AtExitManager* g_exit_manager = NULL; put this in the unnamed namespace. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:27: } else { } else if (reason == DLL_PROCESS_DETACH) { (since you don't want to do this for thread creation/destruction) https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:28: delete g_exit_manager; it seems prudent (although not required) to call CommandLine::Reset() before this. what do you think?
Okay, unit tests are in. Let me know if there are some additional edge cases worth covering. Thanks for pointing me at the installer util unittests... made it pretty easy. Also addressed your last round of comments. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... File chrome/installer/gcapi/gcapi_dll.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:4: On 2012/12/20 19:45:33, grt wrote: > #include <windows.h> for things like WINAPI, HINSTANCE, etc. Done. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:11: base::AtExitManager* g_exit_manager = NULL; On 2012/12/20 19:45:33, grt wrote: > put this in the unnamed namespace. Done. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:27: } else { On 2012/12/20 19:45:33, grt wrote: > } else if (reason == DLL_PROCESS_DETACH) { > (since you don't want to do this for thread creation/destruction) Done. Thanks for catching this. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:28: delete g_exit_manager; Prudent, yes. I'll add it in. (I am guessing that the exit_manager handles destroying the CommandLine stuff when it's all done, but may not call CommandLine::Reset. Correct me if this is not the case...)
change looks good, one note is that since the BrowserDistribuion classes are now being used, gcapi_dll.dll now has to be compiled with GOOGLE_CHROME_BUILD. This may not always be the case as frequently the distribution folk build it and hand it out to partners. When this lands, we should send out a note to the distribution people indicating the new build requirement. (I can take care of doing this if need be). https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/g... File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/g... chrome/installer/gcapi/gcapi_omaha_experiment.cc:78: string16 experiment_label; micro nit: experiment_label -> experiment_labels https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/u... File chrome/installer/util/google_chrome_distribution.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/u... chrome/installer/util/google_chrome_distribution.cc:871: #endif this preceded your change, but please could you add a comment here indicating which #if this closes.
Thanks for bringing that up. I'll bug you about doing that after this lands. https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/g... File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/g... chrome/installer/gcapi/gcapi_omaha_experiment.cc:78: string16 experiment_label; On 2013/01/07 22:14:04, robertshield wrote: > micro nit: experiment_label -> experiment_labels Done. https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/u... File chrome/installer/util/google_chrome_distribution.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/27002/chrome/installer/u... chrome/installer/util/google_chrome_distribution.cc:871: #endif On 2013/01/07 22:14:04, robertshield wrote: > this preceded your change, but please could you add a comment here indicating > which #if this closes. Done.
lookin' sweet. https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... File chrome/installer/gcapi/gcapi_dll.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/18001/chrome/installer/g... chrome/installer/gcapi/gcapi_dll.cc:28: delete g_exit_manager; On 2013/01/07 21:16:52, SteveT wrote: > Prudent, yes. I'll add it in. > > (I am guessing that the exit_manager handles destroying the CommandLine stuff > when it's all done, but may not call CommandLine::Reset. Correct me if this is > not the case...) It doesn't appear that CommandLine uses the AtExitManager, so I don't think deleting the manager would clean up the global CommandLine. https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... File chrome/installer/util/google_update_settings_unittest.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:119: EXPECT_TRUE(GoogleUpdateSettings::SetExperimentLabels( i think you need to make this test brand-aware. do all of this work for a GOOGLE_CHROME_BUILD (as well as test that the BrowserDistribution's ShouldSetExperimentLabels method returns true). for a non-GOOGLE_CHROME_BUILD, only test that ShouldSet... returns false. https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:143: RegKey key2; you could re-use |key| on lines 147 and 149 rather than introducing a new variable.
Cool beans. Back to you! https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... File chrome/installer/util/google_update_settings_unittest.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:119: EXPECT_TRUE(GoogleUpdateSettings::SetExperimentLabels( Done. It's a bit odd using ifdefs in a unit test - is this test actually run by our bots on both build types? Also, is it cool if I test the BrowserDistribution::ShouldSetExperimentLabels behaviour here. Is there another test I should be doing this in? (like maybe google_chrome_distribution_unittests.cc, though I don't see similar tests in there...) https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:143: RegKey key2; Done. Okay great. I thought you might have to close the original key to ensure the value was delete-able.
lgtm w/ the two changes below. https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... File chrome/installer/util/google_update_settings_unittest.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:119: EXPECT_TRUE(GoogleUpdateSettings::SetExperimentLabels( On 2013/01/08 16:33:04, SteveT wrote: > Done. > > It's a bit odd using ifdefs in a unit test - is this test actually run by our > bots on both build types? The normal case on the trybots, CQ, etc is Chromium. > Also, is it cool if I test the BrowserDistribution::ShouldSetExperimentLabels > behaviour here. Yup, looks good to me. https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:143: RegKey key2; 2013/01/08 16:33:04, SteveT wrote: > Done. > > Okay great. I thought you might have to close the original key to ensure the > value was delete-able. I liked the explicit call to key.Close() on line 139 for the reason you state. Please add that back in. The one line line 150 is superfluous, though, and could be removed (or not if you like the symmetry). https://chromiumcodereview.appspot.com/11280067/diff/29002/chrome/installer/u... File chrome/installer/util/google_update_settings_unittest.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/29002/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:119: #ifdef GOOGLE_CHROME_BUILD #if defined(GOOGLE_CHROME_BUILD)
https://chromiumcodereview.appspot.com/11280067/diff/29002/chrome/installer/u... File chrome/installer/util/google_update_settings_unittest.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/29002/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:152: EXPECT_FALSE(chrome->ShouldSetExperimentLabels()); chrome -> BrowserDistribution::GetSpecificDistribution(BrowserDistribution::CHROME_BROWSER)
Thanks! Waiting for happy bots, then I'll commit. Is there anything on the waterfall that checks additional GCAPI stuff that I should be looking out for? https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... File chrome/installer/util/google_update_settings_unittest.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/33001/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:143: RegKey key2; On 2013/01/08 17:06:36, grt wrote: > 2013/01/08 16:33:04, SteveT wrote: > > Done. > > > > Okay great. I thought you might have to close the original key to ensure the > > value was delete-able. > > I liked the explicit call to key.Close() on line 139 for the reason you state. > Please add that back in. The one line line 150 is superfluous, though, and could > be removed (or not if you like the symmetry). Done. Kept the second Close. https://chromiumcodereview.appspot.com/11280067/diff/29002/chrome/installer/u... File chrome/installer/util/google_update_settings_unittest.cc (right): https://chromiumcodereview.appspot.com/11280067/diff/29002/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:119: #ifdef GOOGLE_CHROME_BUILD On 2013/01/08 17:06:36, grt wrote: > #if defined(GOOGLE_CHROME_BUILD) Done. https://chromiumcodereview.appspot.com/11280067/diff/29002/chrome/installer/u... chrome/installer/util/google_update_settings_unittest.cc:152: EXPECT_FALSE(chrome->ShouldSetExperimentLabels()); On 2013/01/08 17:07:39, grt wrote: > chrome -> > BrowserDistribution::GetSpecificDistribution(BrowserDistribution::CHROME_BROWSER) Yeah... the trybots caught that error. I refactored it in a different way (defined |chrome| above the ifdef).
On 2013/01/08 17:53:15, SteveT wrote: > Is there anything on the waterfall that checks additional GCAPI stuff that I > should be looking out for? Not to my knowledge.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/11280067/24002
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/11280067/24002
Message was sent while issue was closed.
Change committed as 175671
Message was sent while issue was closed.
Robert - this patch has landed. What do we need to do with re: informing people of build changes? |
