Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7824010:
Make Chrome Frame registration and unregistration more robust. (Closed)
|
Created:
9 years, 3 months ago by robertshield Modified:
9 years, 3 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, erikwright (departed) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMake Chrome Frame registration and unregistration more robust and make it return extra information in the failure code.
Specifically, there are a series of registration changes made by Chrome Frame at registration and unregistration time. Previously, if any of those changes failed, the previous changes were left and subsequent changes were not made.
During registration this could result in partial registration data being left if one of the items failed which is bad but wasn't a problem in practice, because if ever registration failed, unregistration would then be performed by the installer. Also the most likely step to fail happens first.
During unregistration, if any of the steps fail, the remaining unregistration steps will not be taken. This is bad, as uninstallation is a best effort, "clean up everything you can" type thing.
Changed this to for registration roll back the previous N-1 steps of step N fails and for unregistration to attempt to perform all N steps disregarding failures.
Updated installer logging.
BUG=94848
TEST=Unregistration of e.g. the user-agent modification should always be performed even if an earlier step fails.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100224
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 74
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 42
Patch Set 7 : '' #
Total comments: 10
Patch Set 8 : '' #Patch Set 9 : '' #
Messages
Total messages: 10 (0 generated)
Apologies for the novella. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:494: class CustomRegistrationHelper { Looks like it wouldn't be hard to make this a pure interface rather than an abstract base; remove reg_flags (see comments below) and change Do(reg) to Do(is_system, reg). http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:494: class CustomRegistrationHelper { On second thought, I'd be sorely tempted to simplify this. I think you could whittle down each step down to just the (one- or two-line) Do() functions, then make a constant table kinda like this: typedef HRESULT (*RegistrationFn)(bool is_system, bool reg); struct RegistrationStep { int step_id; uint32 step_condition; RegistrationFn registration_fn; }; static const RegistrationStep registration_steps[] { { kStepSecuredMimeHandler, ACTIVEDOC, &RegisterSecuredMimeHandler }, ... }; And walk it with something like: for (scan = ...; scan != end; ++scan) { if ((reg_flags & scan->step_condition) != 0) { HRESULT hr = scan->registration_fn(...); ...; } } It amounts to the same thing I suppose, but it might be less code overall. I won't be offended if you tell me to stuff it. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:496: CustomRegistrationHelper(UINT reg_flags, bool is_system) i think uint32 is more stylistically appropriate. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:521: HRESULT Do(bool reg) { ) OVERRIDE { here and in all other classes. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:523: // Don't fail to unregister if we can't undo the secure mime This comment and code are stale; the decision of whether or not to continue/fail unregistration is now handled by the calling code, no? In other words, just return S_OK or E_FAIL based on RegisterSecuredMimeHandler. Better yet, follow my advice on RSMH below and just return RSMH(...); http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:526: if (!RegisterSecuredMimeHandler(reg ? true : false, is_system_) && reg) "reg ? true : false" -> "reg" http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:532: This seems to have been lost: class CRHActiveDoc : public CustomRegistrationHelper { ... private: HRESULT Do(bool reg) OVERRIDE { if (reg_flags_ & ACTIVEDOC) return ChromeActiveDocument::UpdateRegistry(reg); } }; plus the corresponding addition to the vector down below. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:562: _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_ELEVATION, reg); why no return this HRESULT? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:563: RefreshElevationPolicy(); why this not its own step with its own return HRESULT? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:606: _AtlModule.UpdateRegistryFromResourceS(IDR_REGISTER_BHO, reg); why u no propagate error code?!?!!? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:610: SetupUserLevelHelper(); or here http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:616: base::win::RemoveCommandFromAutoRun(HKEY_CURRENT_USER, kRunKeyName); eh? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:663: _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_NPAPI, reg); stylistically, i think this should return the error code just like all the others; it's the job of the caller to ignore it if !reg. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:666: // Ignore failures. bad comment http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:702: // |0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1| Interesting bit number choice. I think it'd be more clear to make the least significant bit #0. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:711: DCHECK((error_code & 15) == error_code); nit: i find 0x0F more obvious than 15. I think error_code should be an int, though, so: DCHECK_GE(error_code, 0); DCHECK LE(error_code, 15); would be even better. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:715: DCHECK(static_cast<HRESULT>(hr & 0xF87FFFFF) == hr) << hr; DCHECK_EQ(static_cast<HRESULT>(hr & 0xF87FFFFF), hr); ? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:717: HRESULT fiddled_hr = ((error_code & 15) << 23) | hr; 15 -> 0x0F and you may need to stick a cast in there if error_code is an int. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:723: STDAPI CustomRegistration(UINT reg_flags, BOOL reg, bool is_system) { Why is this STDAPI? Change to just returning an HRESULT unless I'm missing something. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:735: ScopedVector<CustomRegistrationHelper> registration_helpers; If you stick with the classes and vector approach, I think it's easier to follow what's going on if the helpers are added to the vector only if dictated by |flags|. See below for how I think the failure step should be handled. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:737: new CRHSecuredMimeHandler(reg_flags, is_system)); // 0 |flags| rather than |reg_flags| should be used here and everywhere below. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:760: bool do_register = (reg == TRUE); reg != FALSE, unless you take my request to change BOOL to bool. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:764: // correspond to the index of the failing step. Using the index of the step introduces a requirement that the current set of steps never change. I think this is fragile. How about adding an int GetStepIdentifier() = 0; to the base interface so that their numbering is defined explicitly (an enum, integer constants, whatever)? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:765: uint16 registration_failure_step = 0; The arbiters of style say to use "int" for this. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:768: if (!SUCCEEDED(hr = (*reg_iter)->Run(do_register))) { I don't know that this is banned by the style guide, but assignment within macro invocation within conditional seems a bit dense. I think hr = ...; if (!SUCCEEDED(hr) && do_register) { ... } is more readable http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:780: // this does not rollback the failing action. Consider rolling back the failing action. Otherwise an action that does three things has to manage its own internal rollback mechanism. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:781: ScopedVector<CustomRegistrationHelper>::reverse_iterator rollback_iter( I don't see an advantage to making a new object for this rather than just walking reg_iter backwards to the beginning. I think it's more obvious to just --reg_iter. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:788: return MuxErrorIntoHRESULT(hr, registration_failure_step); Consider changing SelfRegWorkItem::RegisterDll as follows: - change PLOG(ERROR) to LOG(ERROR) since the former logs the last error code, which we don't care about - check for the customer bit in the HRESULT and unmux the step number when logging. I think it'd be more useful if the log contains the real HRESULT and the step number since N weeks down the road (where N is some small number) I, at least, will have forgotten about this muxing and will have a heck of a time trying to figure out what the undefined HRESULT means. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:796: HRESULT hr = CustomRegistration(flags, TRUE, true); TRUE, true? Knowing that you didn't commit this sin in this CL, please help me by changing the BOOL to bool or 'splain to me why this is correct. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:810: // DllRegisterServer - Adds entries to the HKCU hive in the registry Please fix comment while you're here. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:823: // DllRegisterServer - Removes entries from the HKCU hive in the registry. Please fix comment while you're here. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:943: static bool SetOrDeleteMimeHandlerKey(bool set, HKEY root_key) { change this to return an HRESULT. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:960: return (result2 == ERROR_SUCCESS) && (result2 == ERROR_SUCCESS); Boy, this is funny. I'm sure I reviewed this and gave an LGTM. sigh. Change this to: return result1 != ERROR_SUCCESS ? HRESULT_FROM_WIN32(result1) : HRESULT_FROM_WIN32(result2); http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:963: bool RegisterSecuredMimeHandler(bool enable, bool is_system) { change this to return an HRESULT http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:979: return false; return E_FAIL or HRESULT_FROM_WIN32(GetLastError()) or somefin'.
Thanks for the thorough review. Removed a bunch of code following your suggestion to use an array of function pointers instead. Cleaned up a lot of the old stuff as well to be more consistent about returning useful error codes. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:494: class CustomRegistrationHelper { On 2011/09/02 19:01:35, grt wrote: > Looks like it wouldn't be hard to make this a pure interface rather than an > abstract base; remove reg_flags (see comments below) and change Do(reg) to > Do(is_system, reg). Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:494: class CustomRegistrationHelper { On 2011/09/02 19:01:35, grt wrote: > On second thought, I'd be sorely tempted to simplify this. I think you could > whittle down each step down to just the (one- or two-line) Do() functions, then > make a constant table kinda like this: > typedef HRESULT (*RegistrationFn)(bool is_system, bool reg); > struct RegistrationStep { > int step_id; > uint32 step_condition; > RegistrationFn registration_fn; > }; > static const RegistrationStep registration_steps[] { > { kStepSecuredMimeHandler, ACTIVEDOC, &RegisterSecuredMimeHandler }, > ... > }; > And walk it with something like: > for (scan = ...; scan != end; ++scan) { > if ((reg_flags & scan->step_condition) != 0) { > HRESULT hr = scan->registration_fn(...); > ...; > } > } > It amounts to the same thing I suppose, but it might be less code overall. I > won't be offended if you tell me to stuff it. This is a much nicer approach, changed as you suggest. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:496: CustomRegistrationHelper(UINT reg_flags, bool is_system) On 2011/09/02 19:01:35, grt wrote: > i think uint32 is more stylistically appropriate. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:521: HRESULT Do(bool reg) { On 2011/09/02 19:01:35, grt wrote: > ) OVERRIDE { here and in all other classes. No longer needed. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:523: // Don't fail to unregister if we can't undo the secure mime On 2011/09/02 19:01:35, grt wrote: > This comment and code are stale; the decision of whether or not to continue/fail > unregistration is now handled by the calling code, no? In other words, just > return S_OK or E_FAIL based on RegisterSecuredMimeHandler. Better yet, follow > my advice on RSMH below and just return RSMH(...); Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:526: if (!RegisterSecuredMimeHandler(reg ? true : false, is_system_) && reg) On 2011/09/02 19:01:35, grt wrote: > "reg ? true : false" -> "reg" Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:532: On 2011/09/02 19:01:35, grt wrote: > This seems to have been lost: > class CRHActiveDoc : public CustomRegistrationHelper { > ... > private: > HRESULT Do(bool reg) OVERRIDE { > if (reg_flags_ & ACTIVEDOC) > return ChromeActiveDocument::UpdateRegistry(reg); > } > }; > plus the corresponding addition to the vector down below. good catch http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:562: _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_ELEVATION, reg); On 2011/09/02 19:01:35, grt wrote: > why no return this HRESULT? This is strictly a developer convenience thing. We don't gate registration on it completing and indeed even the installer does this as a best-effort thing. As such, I'm leery of changing the behaviour here. I did make it only happen on Vista+ though, since it appears to always fail on XP. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:563: RefreshElevationPolicy(); On 2011/09/02 19:01:35, grt wrote: > why this not its own step with its own return HRESULT? Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:606: _AtlModule.UpdateRegistryFromResourceS(IDR_REGISTER_BHO, reg); On 2011/09/02 19:01:35, grt wrote: > why u no propagate error code?!?!!? Fixed up the bho registration to proagate error codes. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:610: SetupUserLevelHelper(); On 2011/09/02 19:01:35, grt wrote: > or here Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:616: base::win::RemoveCommandFromAutoRun(HKEY_CURRENT_USER, kRunKeyName); On 2011/09/02 19:01:35, grt wrote: > eh? Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:663: _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_NPAPI, reg); On 2011/09/02 19:01:35, grt wrote: > stylistically, i think this should return the error code just like all the > others; it's the job of the caller to ignore it if !reg. I tend to agree, but that means that I would need to add an extra field to the RegistrationStep struct to indicate that an item is allowed to fail. Since this is a one off, I'd rather handle it here with a loud comment. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:666: // Ignore failures. On 2011/09/02 19:01:35, grt wrote: > bad comment the comment was correct, just unwanted ;) http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:702: // |0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1| On 2011/09/02 19:01:35, grt wrote: > Interesting bit number choice. I think it'd be more clear to make the least > significant bit #0. I had copied this from the msdn link below, but sure, I can invert it. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:711: DCHECK((error_code & 15) == error_code); On 2011/09/02 19:01:35, grt wrote: > nit: i find 0x0F more obvious than 15. I think error_code should be an int, > though, so: > DCHECK_GE(error_code, 0); > DCHECK LE(error_code, 15); > would be even better. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:715: DCHECK(static_cast<HRESULT>(hr & 0xF87FFFFF) == hr) << hr; On 2011/09/02 19:01:35, grt wrote: > DCHECK_EQ(static_cast<HRESULT>(hr & 0xF87FFFFF), hr); ? Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:717: HRESULT fiddled_hr = ((error_code & 15) << 23) | hr; On 2011/09/02 19:01:35, grt wrote: > 15 -> 0x0F and you may need to stick a cast in there if error_code is an int. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:723: STDAPI CustomRegistration(UINT reg_flags, BOOL reg, bool is_system) { On 2011/09/02 19:01:35, grt wrote: > Why is this STDAPI? Change to just returning an HRESULT unless I'm missing > something. I think once upon a time this was intended to be an export. No more though, so changed. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:735: ScopedVector<CustomRegistrationHelper> registration_helpers; On 2011/09/02 19:01:35, grt wrote: > If you stick with the classes and vector approach, I think it's easier to follow > what's going on if the helpers are added to the vector only if dictated by > |flags|. See below for how I think the failure step should be handled. Approach changed. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:737: new CRHSecuredMimeHandler(reg_flags, is_system)); // 0 On 2011/09/02 19:01:35, grt wrote: > |flags| rather than |reg_flags| should be used here and everywhere below. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:760: bool do_register = (reg == TRUE); On 2011/09/02 19:01:35, grt wrote: > reg != FALSE, unless you take my request to change BOOL to bool. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:764: // correspond to the index of the failing step. On 2011/09/02 19:01:35, grt wrote: > Using the index of the step introduces a requirement that the current set of > steps never change. I think this is fragile. How about adding an int > GetStepIdentifier() = 0; to the base interface so that their numbering is > defined explicitly (an enum, integer constants, whatever)? Thought about that but figured it would add even more code bloat. Since I switched to using an array of RegistrationSteps, I feel an int index is appropriate. What say you? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:765: uint16 registration_failure_step = 0; On 2011/09/02 19:01:35, grt wrote: > The arbiters of style say to use "int" for this. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:768: if (!SUCCEEDED(hr = (*reg_iter)->Run(do_register))) { On 2011/09/02 19:01:35, grt wrote: > I don't know that this is banned by the style guide, but assignment within macro > invocation within conditional seems a bit dense. I think > hr = ...; > if (!SUCCEEDED(hr) && do_register) { ... } > is more readable Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:780: // this does not rollback the failing action. On 2011/09/02 19:01:35, grt wrote: > Consider rolling back the failing action. Otherwise an action that does three > things has to manage its own internal rollback mechanism. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:781: ScopedVector<CustomRegistrationHelper>::reverse_iterator rollback_iter( On 2011/09/02 19:01:35, grt wrote: > I don't see an advantage to making a new object for this rather than just > walking reg_iter backwards to the beginning. I think it's more obvious to just > --reg_iter. Changed to use an index. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:788: return MuxErrorIntoHRESULT(hr, registration_failure_step); On 2011/09/02 19:01:35, grt wrote: > Consider changing SelfRegWorkItem::RegisterDll as follows: > - change PLOG(ERROR) to LOG(ERROR) since the former logs the last error code, > which we don't care about > - check for the customer bit in the HRESULT and unmux the step number when > logging. > I think it'd be more useful if the log contains the real HRESULT and the step > number since N weeks down the road (where N is some small number) I, at least, > will have forgotten about this muxing and will have a heck of a time trying to > figure out what the undefined HRESULT means. Good idea, so changed. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:796: HRESULT hr = CustomRegistration(flags, TRUE, true); On 2011/09/02 19:01:35, grt wrote: > TRUE, true? Knowing that you didn't commit this sin in this CL, please help me > by changing the BOOL to bool or 'splain to me why this is correct. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:810: // DllRegisterServer - Adds entries to the HKCU hive in the registry On 2011/09/02 19:01:35, grt wrote: > Please fix comment while you're here. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:823: // DllRegisterServer - Removes entries from the HKCU hive in the registry. On 2011/09/02 19:01:35, grt wrote: > Please fix comment while you're here. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:943: static bool SetOrDeleteMimeHandlerKey(bool set, HKEY root_key) { On 2011/09/02 19:01:35, grt wrote: > change this to return an HRESULT. Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:960: return (result2 == ERROR_SUCCESS) && (result2 == ERROR_SUCCESS); On 2011/09/02 19:01:35, grt wrote: > Boy, this is funny. I'm sure I reviewed this and gave an LGTM. sigh. Change > this to: > return result1 != ERROR_SUCCESS ? HRESULT_FROM_WIN32(result1) : > HRESULT_FROM_WIN32(result2); Yes, this is awesome. Fixed. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:963: bool RegisterSecuredMimeHandler(bool enable, bool is_system) { On 2011/09/02 19:01:35, grt wrote: > change this to return an HRESULT Done. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:979: return false; On 2011/09/02 19:01:35, grt wrote: > return E_FAIL or HRESULT_FROM_WIN32(GetLastError()) or somefin'. Done.
I think this new direction is pretty easy to follow. Again, apologies for the Vogon poetry. http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:663: _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_NPAPI, reg); On 2011/09/03 05:36:21, robertshield wrote: > On 2011/09/02 19:01:35, grt wrote: > > stylistically, i think this should return the error code just like all the > > others; it's the job of the caller to ignore it if !reg. > > I tend to agree, but that means that I would need to add an extra field to the > RegistrationStep struct to indicate that an item is allowed to fail. Since this > is a one off, I'd rather handle it here with a loud comment. What makes this unregistration code special in that regard (this is in a !reg block)? All other handlers return their error code even if !reg, correct? Is the special thing that this is a best-case cleanup which should be allowed to fail even if a policy change were to be made in the future to fail if any other unregistration code were to fail? http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:666: // Ignore failures. On 2011/09/03 05:36:21, robertshield wrote: > On 2011/09/02 19:01:35, grt wrote: > > bad comment > > the comment was correct, just unwanted ;) "Bad" in that it is in the wrong location. Methinks it's more clear if the comment saying "Ignore failures" immediately precedes the operation that might fail. In this way, the reader doesn't have to look down to know why the return value is ignored. On the other hand, I do see the logic in putting it down here ("Ignore failures from anywhere up there.") http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:702: // |0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1| On 2011/09/03 05:36:21, robertshield wrote: > On 2011/09/02 19:01:35, grt wrote: > > Interesting bit number choice. I think it'd be more clear to make the least > > significant bit #0. > > I had copied this from the msdn link below, but sure, I can invert it. Funny: winerror.h has it the other way. :-) http://codereview.chromium.org/7824010/diff/2003/chrome_frame/chrome_tab.cc#n... chrome_frame/chrome_tab.cc:764: // correspond to the index of the failing step. On 2011/09/03 05:36:21, robertshield wrote: > On 2011/09/02 19:01:35, grt wrote: > > Using the index of the step introduces a requirement that the current set of > > steps never change. I think this is fragile. How about adding an int > > GetStepIdentifier() = 0; to the base interface so that their numbering is > > defined explicitly (an enum, integer constants, whatever)? > > Thought about that but figured it would add even more code bloat. Since I > switched to using an array of RegistrationSteps, I feel an int index is > appropriate. What say you? It just means that you have to refer to the exact same version of the source to figure out on which step a failure occurred. I find this fragile. A non-verbose installer log will have the stage + HRESULT but I don't think it will have the version #. It seems to me that this introduces an opportunity for someone to unknowingly dig up the wrong version of the .cc file to map a stage # back to its meaning. Head scratching may ensue. Looks like the latest edition of the change uses an enum, so I'm satisfied. http://codereview.chromium.org/7824010/diff/10003/chrome/installer/util/self_... File chrome/installer/util/self_reg_work_item.cc (right): http://codereview.chromium.org/7824010/diff/10003/chrome/installer/util/self_... chrome/installer/util/self_reg_work_item.cc:37: *error_code = (hr & 0x07800000) >> 23; why not do this only if the customer bit is set? conceivably, this work item could be used to register a component that doesn't do the muxing magic. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:384: base::win::AddCommandToAutoRun(HKEY_CURRENT_USER, kRunKeyName, if this fails, installation will continue but GCF won't work after the user logs out and back on. is this desirable? http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:392: hr = E_FAIL; this will now cause installation to fail and rollback (it didn't previously). this is desired? http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:394: LOG(ERROR) << "Could not launch helper process."; Do you think the last-error code might be useful here? Change to PLOG if so. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:401: LOG(ERROR) << "Failed to post close message to old helper process: " PLOG and remove GetLastError http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:406: hr = ERROR_FILE_NOT_FOUND; hr = HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND); http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:452: if (ua_key.Create(parent_hive, kPostPlatformUAKey, capture the return value here http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:485: DLOG(ERROR) << __FUNCTION__ << ": " << kPostPlatformUAKey; log the captured return value here http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:486: hr = E_UNEXPECTED; hr = HRESULT_FROM_WIN32(captured value) http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:490: consider adding a comment indicating that what follows are the ordered registration step functions. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:508: // convenience as the installer is really responsible for doing this. This comment says that the installer is responsible for doing this. Does the installer, in fact, do it; or has responsibility been shifted to this function? http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:539: return base::win::RemoveCommandFromAutoRun(HKEY_CURRENT_USER, return base::win::Remove...(...) ? S_OK : E_FAIL; http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:574: remove extra newline http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:579: remove extra newline http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:589: enum RegistrationSteps { nit: make this singular since an instance of the type represents one step (much like an instance of an "int" represents one integer). since RegistrationStep is already taken, maybe RegistrationStepId. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:653: int id; how about RegistrationStepId rather than int? this make looking at the steps in a debugger friendlier. and it's more typesafe. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:654: uint32 condition; since reg_flags is a uint16, should this be, too? http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:688: for (int rollback_step = failure_step; how about reusing "step" so this doesn't need so much wrapping? http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:850: static HRESULT SetOrDeleteMimeHandlerKey(bool set, HKEY root_key) { nit: move this (and the above classes) into the unnamed namespace and remove "static". http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:871: HRESULT RegisterSecuredMimeHandler(bool enable, bool is_system) { nit: move this so that it lives with the other registration funcs, and is in the proper order. mega-nit: move all of them into the unnamed namespace.
Comments addressed, I need to redo my manual testing and see about writing some individual tests for this. PTAL http://codereview.chromium.org/7824010/diff/10003/chrome/installer/util/self_... File chrome/installer/util/self_reg_work_item.cc (right): http://codereview.chromium.org/7824010/diff/10003/chrome/installer/util/self_... chrome/installer/util/self_reg_work_item.cc:37: *error_code = (hr & 0x07800000) >> 23; On 2011/09/06 14:56:55, grt wrote: > why not do this only if the customer bit is set? conceivably, this work item > could be used to register a component that doesn't do the muxing magic. Good idea, done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:384: base::win::AddCommandToAutoRun(HKEY_CURRENT_USER, kRunKeyName, On 2011/09/06 14:56:55, grt wrote: > if this fails, installation will continue but GCF won't work after the user logs > out and back on. is this desirable? Nope, we now fail on this as well. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:392: hr = E_FAIL; On 2011/09/06 14:56:55, grt wrote: > this will now cause installation to fail and rollback (it didn't previously). > this is desired? Yes, this is desired. New IE instances will not render in Chrome Frame if this does not succeed. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:394: LOG(ERROR) << "Could not launch helper process."; On 2011/09/06 14:56:55, grt wrote: > Do you think the last-error code might be useful here? Change to PLOG if so. So changed, although sadly this won't show up in the install log. Me wonders if it is worth it to have the installer listen for logging messages (would be a separate CL). http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:401: LOG(ERROR) << "Failed to post close message to old helper process: " On 2011/09/06 14:56:55, grt wrote: > PLOG and remove GetLastError Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:406: hr = ERROR_FILE_NOT_FOUND; On 2011/09/06 14:56:55, grt wrote: > hr = HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND); Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:452: if (ua_key.Create(parent_hive, kPostPlatformUAKey, On 2011/09/06 14:56:55, grt wrote: > capture the return value here Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:485: DLOG(ERROR) << __FUNCTION__ << ": " << kPostPlatformUAKey; On 2011/09/06 14:56:55, grt wrote: > log the captured return value here Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:486: hr = E_UNEXPECTED; On 2011/09/06 14:56:55, grt wrote: > hr = HRESULT_FROM_WIN32(captured value) Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:490: On 2011/09/06 14:56:55, grt wrote: > consider adding a comment indicating that what follows are the ordered > registration step functions. Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:508: // convenience as the installer is really responsible for doing this. On 2011/09/06 14:56:55, grt wrote: > This comment says that the installer is responsible for doing this. Does the > installer, in fact, do it; or has responsibility been shifted to this function? It does, in fact, do it. See install_worker.cc, AddElevationPolicyWorkItems and RefreshElevationPolicy. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:539: return base::win::RemoveCommandFromAutoRun(HKEY_CURRENT_USER, On 2011/09/06 14:56:55, grt wrote: > return base::win::Remove...(...) ? S_OK : E_FAIL; Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:574: On 2011/09/06 14:56:55, grt wrote: > remove extra newline Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:579: On 2011/09/06 14:56:55, grt wrote: > remove extra newline Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:589: enum RegistrationSteps { On 2011/09/06 14:56:55, grt wrote: > nit: make this singular since an instance of the type represents one step (much > like an instance of an "int" represents one integer). since RegistrationStep is > already taken, maybe RegistrationStepId. Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:653: int id; On 2011/09/06 14:56:55, grt wrote: > how about RegistrationStepId rather than int? this make looking at the steps in > a debugger friendlier. and it's more typesafe. Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:654: uint32 condition; On 2011/09/06 14:56:55, grt wrote: > since reg_flags is a uint16, should this be, too? Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:688: for (int rollback_step = failure_step; On 2011/09/06 14:56:55, grt wrote: > how about reusing "step" so this doesn't need so much wrapping? Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:850: static HRESULT SetOrDeleteMimeHandlerKey(bool set, HKEY root_key) { On 2011/09/06 14:56:55, grt wrote: > nit: move this (and the above classes) into the unnamed namespace and remove > "static". Done. http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:871: HRESULT RegisterSecuredMimeHandler(bool enable, bool is_system) { On 2011/09/06 14:56:55, grt wrote: > nit: move this so that it lives with the other registration funcs, and is in the > proper order. mega-nit: move all of them into the unnamed namespace. Done.
slowly i turned... http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:394: LOG(ERROR) << "Could not launch helper process."; On 2011/09/06 20:23:42, robertshield wrote: > On 2011/09/06 14:56:55, grt wrote: > > Do you think the last-error code might be useful here? Change to PLOG if so. > > So changed, although sadly this won't show up in the install log. Me wonders if > it is worth it to have the installer listen for logging messages (would be a > separate CL). frown. so the logging stuff in DllMain is a noop during installation? does ETW logging (as seen in sawbuck) not work either? http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:18: #include "base/memory/scoped_vector.h" remove this http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:364: if (reg_result = ua_key.Create(parent_hive, kPostPlatformUAKey, assignment has lower precedence than equality, so you need parens around the assignment expression here. probably more readable to move the assignment out of the conditional. http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:614: // Chrome Frame registration functions. why not move all of these into the unnamed namespace? will there be unit tests calling them directly? http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:848: hr = registration_steps[step].func(reg, is_system); I think it'd be useful to return the HRESULT for the first failure encountered during unregistration, since otherwise we may return S_OK if all but the last step fails. Also, I think we should only mux in the step number on failure. With that in mind, I propose: HRESULT step_hr = registration_steps[step].func(reg, is_system); if (FAILED(step_hr)) { if (hr == S_OK) { // Prepare the return value based on the first failure. hr = MuxErrorIntoHRESULT(step_hr, step); } if (reg) { // Stop at the first registration failure and begin rollback. rollback = true; failure_step = step; break; } // Else continue with best-effort unregistration. } You could optionally get rid of a stack var by renaming failure_step to step and removing the first term of each for loop. http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:865: return MuxErrorIntoHRESULT(hr, failure_step); return hr;
http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/10003/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:394: LOG(ERROR) << "Could not launch helper process."; On 2011/09/07 15:00:48, grt wrote: > On 2011/09/06 20:23:42, robertshield wrote: > > On 2011/09/06 14:56:55, grt wrote: > > > Do you think the last-error code might be useful here? Change to PLOG if > so. > > > > So changed, although sadly this won't show up in the install log. Me wonders > if > > it is worth it to have the installer listen for logging messages (would be a > > separate CL). > > frown. so the logging stuff in DllMain is a noop during installation? does ETW > logging (as seen in sawbuck) not work either? As far as I can tell, this is the case. It may be possible to get selfreg dlls to hand off logging information to the installer but that is outside of the scope of the current cl. http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc File chrome_frame/chrome_tab.cc (right): http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:18: #include "base/memory/scoped_vector.h" On 2011/09/07 15:00:48, grt wrote: > remove this Done. http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:364: if (reg_result = ua_key.Create(parent_hive, kPostPlatformUAKey, On 2011/09/07 15:00:48, grt wrote: > assignment has lower precedence than equality, so you need parens around the > assignment expression here. probably more readable to move the assignment out > of the conditional. Done. http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:614: // Chrome Frame registration functions. On 2011/09/07 15:00:48, grt wrote: > why not move all of these into the unnamed namespace? will there be unit tests > calling them directly? So moved. Haven't fully decided on the tests yet, but my initial thought is to have them call in at the level of the DLL registration exports and inspect machine state before and after. http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:848: hr = registration_steps[step].func(reg, is_system); On 2011/09/07 15:00:48, grt wrote: > I think it'd be useful to return the HRESULT for the first failure encountered > during unregistration, since otherwise we may return S_OK if all but the last > step fails. Also, I think we should only mux in the step number on failure. > With that in mind, I propose: > > HRESULT step_hr = registration_steps[step].func(reg, is_system); > if (FAILED(step_hr)) { > if (hr == S_OK) { > // Prepare the return value based on the first failure. > hr = MuxErrorIntoHRESULT(step_hr, step); > } > if (reg) { > // Stop at the first registration failure and begin rollback. > rollback = true; > failure_step = step; > break; > } // Else continue with best-effort unregistration. > } > > You could optionally get rid of a stack var by renaming failure_step to step and > removing the first term of each for loop. Done, but I kept the extra stack var since I think it makes this marginally easier to read. http://codereview.chromium.org/7824010/diff/14001/chrome_frame/chrome_tab.cc#... chrome_frame/chrome_tab.cc:865: return MuxErrorIntoHRESULT(hr, failure_step); On 2011/09/07 15:00:48, grt wrote: > return hr; Done.
LGTM!
On 2011/09/07 19:52:20, grt wrote: > LGTM! I updated the elevation policy registration to fail on error as per recent discovery of it being needed in some cases. PTAL.
LGTM once again |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
