Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23258005:
Give SxS distribution its own registration GUIDs. (Closed)
|
Created:
7 years, 4 months ago by zturner Modified:
7 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, grt+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionGive SxS distribution its own registration GUIDs.
See the linked bug for more information about this change.
BUG=273248
gab: chrome/installer/*
ananta, cpu: win8/
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222987
Patch Set 1 #Patch Set 2 : Fix misaligned brace #
Total comments: 2
Patch Set 3 : Make DelegateExecute create object with a dynamic guid. #
Total comments: 12
Patch Set 4 : Address review issues. #
Total comments: 7
Patch Set 5 : Remove registry code since we don't need to support /RegServer #
Total comments: 7
Patch Set 6 : Remove the word 'we' from comment. #
Total comments: 20
Patch Set 7 : More installer changes so that default Canary doesn't trash a regular chrome install. #
Total comments: 1
Patch Set 8 : Add linker dependency from metro_driver to installer_util #
Total comments: 28
Patch Set 9 : Refactoring and cleanup, and fixed some bugs related to registration #Patch Set 10 : Fixed a few more issues. #Patch Set 11 : Remove startup SxS registration, and fix various bugs. #Patch Set 12 : Remove magic key combo from browser_options_handler.cc #
Total comments: 26
Patch Set 13 : Removed IsSetAsDefaultSupported() and GetCommandExecuteImplClsid(), and other minor cleanup. #
Total comments: 10
Patch Set 14 : More code cleanup, and remove DebugEnableSetAsDefault #
Total comments: 27
Patch Set 15 : More cleanup #
Total comments: 5
Patch Set 16 : More fixes. #Patch Set 17 : Move typedef #
Total comments: 4
Messages
Total messages: 77 (0 generated)
Drive-by: I love, love, love the idea of this. I think you'll need to do a bit more work. For Windows 8, the DelegateExecute handler will need to be registered with a new GUID for the SxS case. At the moment, the binary's GUID is a compile-time constant (see src/win8/delegate_execute/command_execute_impl.h). To support SxS, the GOOGLE_CHROME_BUILD variation will need to dynamically determine its GUID on the basis of the install directory (see InstallUtil::IsChromeSxSProcess). I imagine there may be other BrowserDistribution methods that will need to be overridden for SxS (e.g., GetCommandExecuteImplClsid) as well. I'll poke around a little tomorrow and see what other sticking points I come across.
As Greg said, you will need to make sure that none of the GUIDs Canary uses collide with Google Chrome GUIDs; this is trivial in most cases except for delegate_execute.exe where the GUID is currently baked into the binary GOOGLE_CHROME_BUILD which both Google Chrome and Google Chrome Canary are (see command_execute_impl.h). I told Carlos I'd take a look, but I don't have any prior knowledge about returning different COM class GUIDs at runtime and I'm sure Carlos has ideas (he wrote delegate_execute.exe) so maybe it's easier if you do this as well? https://codereview.chromium.org/23258005/diff/4001/chrome/browser/ui/webui/op... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/23258005/diff/4001/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/browser_options_handler.cc:769: if (GetAsyncKeyState(VK_SHIFT) && GetAsyncKeyState(VK_F12)) { #include <windows.h> for GetAsyncKeyState
https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:41: } btw please update http://www.chromium.org/developers/installer https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:60: gab, greg do we still need AddCommonRGSReplacements ? given that setup can now do this job for developers? https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:80: virtual HRESULT PreMessageLoop(int nShowCmd) { needs OVERRIDE I suppose? https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:120: DWORD reg_token_; reg_token -> registration_token.
Awesome :)!!! Great work! https://codereview.chromium.org/23258005/diff/4001/chrome/browser/ui/webui/op... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/23258005/diff/4001/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/browser_options_handler.cc:769: if (GetAsyncKeyState(VK_SHIFT) && GetAsyncKeyState(VK_F12)) { On 2013/08/21 15:27:46, gab wrote: > #include <windows.h> for GetAsyncKeyState ping. https://codereview.chromium.org/23258005/diff/10001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/10001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:261: Should this be a NOTREACHED()? It doesn't really make sense to call this on a BrowserDistribution and anyone that does by mistake should probably be told. https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:60: On 2013/08/23 00:45:20, cpu wrote: > gab, greg do we still need AddCommonRGSReplacements ? given that setup can now > do this job for developers? Is this to support delegate_execute.exe /RegServer (or something like that?); if so, no we no longer need that, install_worker.cc does all the registrations manually on install (and developers can trigger that via setup.exe --register-dev-chrome for local builds). https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:92: // _ATL_OBJMAP_ENTRY110::RegisterClassObject(). Can you point to where this code can be found? Googling around returns nothing interesting; is it in the SDK (I'm not in front of a source tree atm...)?
https://codereview.chromium.org/23258005/diff/10001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/10001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:261: On 2013/08/23 14:18:09, gab wrote: > Should this be a NOTREACHED()? It doesn't really make sense to call this on a > BrowserDistribution and anyone that does by mistake should probably be told. Done. https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:41: } On 2013/08/23 00:45:20, cpu wrote: > btw please update > > http://www.chromium.org/developers/installer Will do so after the patch lands. https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:80: virtual HRESULT PreMessageLoop(int nShowCmd) { On 2013/08/23 00:45:20, cpu wrote: > needs OVERRIDE I suppose? I actually needed to remove virtual. CAtlExeModuleT<> uses static polymorphism. https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:92: // _ATL_OBJMAP_ENTRY110::RegisterClassObject(). On 2013/08/23 14:18:09, gab wrote: > Can you point to where this code can be found? Googling around returns nothing > interesting; is it in the SDK (I'm not in front of a source tree atm...)? Done. https://codereview.chromium.org/23258005/diff/10001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:120: DWORD reg_token_; On 2013/08/23 00:45:20, cpu wrote: > reg_token -> registration_token. Done.
I might just be missing a couple levels of Windows guru-ness, but I have questions about this awesome tweak to have dynamic runtime GUIDs :)! https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:92: // _ATL_OBJMAP_ENTRY110::RegisterClassObject() in atlbase.h So I'm looking @ C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\atlmfc\include\atlbase.h I don't have a _ATL_OBJMAP_ENTRY110, but I have a _ATL_OBJMAP_ENTRY30; which is typedef'ed to a generic _ATL_OBJMAP_ENTRY. Could we simply use a _ATL_OBJMAP_ENTRY, load it with the correct values, and call its RegisterClassObject() method? https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:92: // _ATL_OBJMAP_ENTRY110::RegisterClassObject() in atlbase.h atlbase.h's RegisterClassObjects() also calls AtlComModuleRegisterClassObjects() after calling RegisterClassObject() for each object; do we need to call this as well? https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:111: if (impl_ != NULL) { In _ATL_OBJMAP_ENTRY30 impl_ is released immediately after CoRegisterClassObject().
https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:92: // _ATL_OBJMAP_ENTRY110::RegisterClassObject() in atlbase.h On 2013/08/23 17:44:02, gab wrote: > atlbase.h's RegisterClassObjects() also calls AtlComModuleRegisterClassObjects() > after calling RegisterClassObject() for each object; do we need to call this as > well? Actually I think it's the other way around, but there's so many different versions of the function that it's hard to follow. PreMessageLoop calls RegisterClassObjects, which then calls AtlComModuleRegisterClassObjects, and that goes through the loop calling RegisterClassObject on each of the entries in the object map. Depending on which version of the Windows SDK you have the source code for AtlComModuleRegisterClassObjects is in a different place. On 2013, the function is inlined in atlbase.h. In 2010 and 2012, the function is in atlbase.inl. https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:92: // _ATL_OBJMAP_ENTRY110::RegisterClassObject() in atlbase.h On 2013/08/23 17:44:02, gab wrote: > So I'm looking @ C:\Program Files (x86)\Microsoft Visual Studio > 10.0\VC\atlmfc\include\atlbase.h > > I don't have a _ATL_OBJMAP_ENTRY110, but I have a _ATL_OBJMAP_ENTRY30; which is > typedef'ed to a generic _ATL_OBJMAP_ENTRY. > > Could we simply use a _ATL_OBJMAP_ENTRY, load it with the correct values, and > call its RegisterClassObject() method? I can change the comment to use the generic name (I guess the numbered name depends on SDK version?), but actually using an _ATL_OBJMAP_ENTRY to do the registration seems a little odd because the struct has lots of irrelevant members and we would be relying on an implementation detail (i.e. implementation is subject to change). Conceptually, this code is pretty simple though. The ClassFactoryCreatorClass is a class factory, and the CreatorClass is the one that does a new and AddRef. So we're calling the class factory's creation method, and telling it which function to use to do the actual creation. We could even do this ourselves by direct COM api calls I suppose, but this seemed cleaner. https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:111: if (impl_ != NULL) { On 2013/08/23 17:44:02, gab wrote: > In _ATL_OBJMAP_ENTRY30 impl_ is released immediately after > CoRegisterClassObject(). I think both versions are correct. I Release() on the revoke operation, it looks like it's just releasing earlier (probably since it knows that CoRegister also does an AddRef)
lgtm, thanks for the detailed explanations! https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/12001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:92: // _ATL_OBJMAP_ENTRY110::RegisterClassObject() in atlbase.h On 2013/08/23 18:12:33, zturner wrote: > On 2013/08/23 17:44:02, gab wrote: > > So I'm looking @ C:\Program Files (x86)\Microsoft Visual Studio > > 10.0\VC\atlmfc\include\atlbase.h > > > > I don't have a _ATL_OBJMAP_ENTRY110, but I have a _ATL_OBJMAP_ENTRY30; which > is > > typedef'ed to a generic _ATL_OBJMAP_ENTRY. > > > > Could we simply use a _ATL_OBJMAP_ENTRY, load it with the correct values, and > > call its RegisterClassObject() method? > > I can change the comment to use the generic name (I guess the numbered name > depends on SDK version?), but actually using an _ATL_OBJMAP_ENTRY to do the > registration seems a little odd because the struct has lots of irrelevant > members and we would be relying on an implementation detail (i.e. implementation > is subject to change). Conceptually, this code is pretty simple though. The > ClassFactoryCreatorClass is a class factory, and the CreatorClass is the one > that does a new and AddRef. So we're calling the class factory's creation > method, and telling it which function to use to do the actual creation. We > could even do this ourselves by direct COM api calls I suppose, but this seemed > cleaner. Using the generic name in the comment sgtm.
https://codereview.chromium.org/23258005/diff/32001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/23258005/diff/32001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/browser_options_handler.cc:767: // On Windows, we allow a magic key combination to enable SxS distributions Who is 'we'?
On 2013/08/23 22:42:46, James Hawkins wrote: > https://codereview.chromium.org/23258005/diff/32001/chrome/browser/ui/webui/o... > File chrome/browser/ui/webui/options/browser_options_handler.cc (right): > > https://codereview.chromium.org/23258005/diff/32001/chrome/browser/ui/webui/o... > chrome/browser/ui/webui/options/browser_options_handler.cc:767: // On Windows, > we allow a magic key combination to enable SxS distributions > Who is 'we'? The Chromium authors. Are pronouns frowned upon in comments? I can remove that word, or let me know if you prefer something different.
lgtm https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:44: please put a comment here about what you are doing at a high level (i.e dynamically registering blah because of canary and stable .. blah) https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:72: (LPVOID*)&impl_); change the c-sytle cast to reinterpret_cast https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:97: IUnknown* impl_; impl_ strikes me as a bad name, how about factory_, class_object_ or something like that?
Fixed comment in browser_options_handler
https://codereview.chromium.org/23258005/diff/36001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/browser_options_handler.cc:771: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); Why does this need to be in the settings page?
https://codereview.chromium.org/23258005/diff/36001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/browser_options_handler.cc:771: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); On 2013/08/26 21:19:09, James Hawkins wrote: > Why does this need to be in the settings page? The full context of this is in the linked bug. It's marked as Restrict so I think we should continue there if there are some high-level design concerns you have. That being said, the immediate reason for putting this in the settings page is because we decided that the feature should be enabled if the user is holding Shift+F12 while the settings page is loading. I'll cc you on the bug.
ping james...
other things that will likely need to change for this to work without trashing a regular Google Chrome install: - src/win8/delegate_execute/chrome_util.cc's GetAppId - src/win8/metro_driver/winrt_utils.cc's ReadArgumentsFromPinnedTaskbarShortcut - src/chrome/installer/util/shell_util.cc's kChromeHTMLProgId - uninstalling canary that has been made the default browser will not cause its registration to go away unless changes are made to uninstall.cc's DeleteChromeRegistrationKeys there may be others https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:261: NOTREACHED(); why NOTREACHED rather than noop? NOTREACHED will cause debug builds of Google Chrome and Chromium to crash if someone holds down the magic key combo. https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... chrome/installer/util/google_chrome_sxs_distribution.cc:21: L"{1BEAC3E3-B852-44F4-B468-8906C062422E}"; nit: 4-space indent https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... File win8/delegate_execute/command_execute_impl.h (right): https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... win8/delegate_execute/command_execute_impl.h:108: OBJECT_ENTRY_AUTO(__uuidof(CommandExecuteImpl), CommandExecuteImpl) just curious: what's up with the whitespace change here? was the file previously missing an EOL here? https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:39: DEFINE_GUID(default_command_execute_impl_clsid, 0xA2DF06F9, 0xA21A, 0x44A8, suggestion: rename "default" to "chromium" and "chrome" to "google_chrome" so that the intent is more obvious. https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:97: IUnknown* impl_; consider base::win::ScopedHandle impl_; impl_.Receive() instead of &impl_ on line 72 impl_.Close(); instead of lines 88-91 https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:99: GUID clsid_; does this need to be an instance variable, or can it be a local var in PreMessageLoop?
On 2013/08/30 03:28:18, grt wrote: > other things that will likely need to change for this to work without trashing a > regular Google Chrome install: > - src/win8/delegate_execute/chrome_util.cc's GetAppId > - src/win8/metro_driver/winrt_utils.cc's ReadArgumentsFromPinnedTaskbarShortcut > - src/chrome/installer/util/shell_util.cc's kChromeHTMLProgId > - uninstalling canary that has been made the default browser will not cause its > registration to go away unless changes are made to uninstall.cc's > DeleteChromeRegistrationKeys > there may be others Oh wow, yes indeed, my bad, I had only looked at BrowserDistribution discrepancies, forgot that kChromeHTMLProgId wasn't in BrowserDistribution (it really should be). Good catch on chrome_util.cc's GetAppId; can delegate_execute depend on installer_util and avoid duplicating all this code now that we integrated it in src/? Same question for ReadArgumentsFromPinnedTaskbarShortcut() if we can depend on installer_util we can avoid having duplicate hardcoded logic there. Ah good catch on uninstall, I had in mind that we did because we do for DelegateExecute's regserver keys, but just noticed that's because we do uninstall the keys dropped by install_worker, but not does by shell_util for SxS apparently... installer::kMediaPlayerRegPath also has special handling which is now incorrect on uninstall. Also, on Win7- getting default installs some keys in HKLM, most of these are branded+suffixed so it doesn't matter, but some keys are shared amongst all user-level install (i.e. the latest one to register/take default grabs it... and that could now be SxS...) -- also, those shared keys are deleted on uninstall IIRC (unless --do-not-remove-shared-items is used), this can be a problem if SxS is uninstalled without being made default yet still deletes those keys... This is already a problem for multiple user-level installs on the same system; perhaps we should do like we do with shortcuts and only uninstall shared keys that point to the chrome.exe being uninstalled. Looking @ http://www.chromium.org/developers/installer#reg I think the only unsuffixed/shared key in HKLM on Win7- is HKLM\Software\Microsoft\Windows\CurrentVersion\App Paths\chrome.exe (for ShellExecute to be able to find chrome.exe without needing to modify PATH); so this shouldn't be too hard to do. It looks like kMediaPlayerRegPath might also be missing from the documented list of shared paths; although I don't really know what that one does and why it matters... Sorry for missing all of this on my original review... Gab > > https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... > File chrome/installer/util/browser_distribution.cc (right): > > https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... > chrome/installer/util/browser_distribution.cc:261: NOTREACHED(); > why NOTREACHED rather than noop? NOTREACHED will cause debug builds of Google > Chrome and Chromium to crash if someone holds down the magic key combo. > > https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... > chrome/installer/util/google_chrome_sxs_distribution.cc:21: > L"{1BEAC3E3-B852-44F4-B468-8906C062422E}"; > nit: 4-space indent > > https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... > File win8/delegate_execute/command_execute_impl.h (right): > > https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... > win8/delegate_execute/command_execute_impl.h:108: > OBJECT_ENTRY_AUTO(__uuidof(CommandExecuteImpl), CommandExecuteImpl) > just curious: what's up with the whitespace change here? was the file previously > missing an EOL here? > > https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... > File win8/delegate_execute/delegate_execute.cc (right): > > https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... > win8/delegate_execute/delegate_execute.cc:39: > DEFINE_GUID(default_command_execute_impl_clsid, 0xA2DF06F9, 0xA21A, 0x44A8, > suggestion: rename "default" to "chromium" and "chrome" to "google_chrome" so > that the intent is more obvious. > > https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... > win8/delegate_execute/delegate_execute.cc:97: IUnknown* impl_; > consider base::win::ScopedHandle impl_; > impl_.Receive() instead of &impl_ on line 72 > impl_.Close(); instead of lines 88-91 > > https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... > win8/delegate_execute/delegate_execute.cc:99: GUID clsid_; > does this need to be an instance variable, or can it be a local var in > PreMessageLoop?
Some probably dumb questions, but since I don't know anything about the installer, bear with me. On 2013/08/30 16:13:29, gab wrote: > Good catch on chrome_util.cc's GetAppId; can delegate_execute depend on > installer_util and avoid duplicating all this code now that we integrated it in > src/? > Same question for ReadArgumentsFromPinnedTaskbarShortcut() if we can depend on > installer_util we can avoid having duplicate hardcoded logic there. Although delegate_execute::GetAppId() currently has a comment saying that ShellUtil currently implements the same function, this doesn't seem to be true. So as best I can tell, there's actually no duplicated logic. Does your comment still apply? In response to grt@'s other comments, I looked over them and most of them boil down to some code where we do #if defined(GOOGLE_CHROME_BUILD) ... #else ... #endif. If I'm understanding the comments correctly, all I need to do is go to each of these locations and check the BrowserDistribution inside the GOOGLE_CHROME_BUILD block, and use different values for a canary distribution. Is this right?
On 2013/08/30 16:54:23, zturner wrote: > Some probably dumb questions, but since I don't know anything about the > installer, > bear with me. > > On 2013/08/30 16:13:29, gab wrote: > > Good catch on chrome_util.cc's GetAppId; can delegate_execute depend on > > installer_util and avoid duplicating all this code now that we integrated it > in > > src/? > > Same question for ReadArgumentsFromPinnedTaskbarShortcut() if we can depend on > > installer_util we can avoid having duplicate hardcoded logic there. > Although delegate_execute::GetAppId() currently has a comment saying that > ShellUtil > currently implements the same function, this doesn't seem to be true. So as > best I > can tell, there's actually no duplicated logic. Does your comment still apply? I think this is ShellUtil::GetBrowserModelId(). > > > In response to grt@'s other comments, I looked over them and most of them boil > down > to some code where we do #if defined(GOOGLE_CHROME_BUILD) ... #else ... #endif. > > If I'm understanding the comments correctly, all I need to do is go to each of > these > locations and check the BrowserDistribution inside the GOOGLE_CHROME_BUILD > block, and > use different values for a canary distribution. Is this right? Yes, some of these use hardcoded values right now because delegate_execute was outside of src/ when first written; it should really use values from BrowserDistribution now if we're ok with having it depend on installer_util (cpu/grt/robertshield?).
On 2013/08/30 17:22:40, gab wrote: > On 2013/08/30 16:54:23, zturner wrote: > > Some probably dumb questions, but since I don't know anything about the > > installer, > > bear with me. > > > > On 2013/08/30 16:13:29, gab wrote: > > > Good catch on chrome_util.cc's GetAppId; can delegate_execute depend on > > > installer_util and avoid duplicating all this code now that we integrated it > > in > > > src/? > > > Same question for ReadArgumentsFromPinnedTaskbarShortcut() if we can depend > on > > > installer_util we can avoid having duplicate hardcoded logic there. > > Although delegate_execute::GetAppId() currently has a comment saying that > > ShellUtil > > currently implements the same function, this doesn't seem to be true. So as > > best I > > can tell, there's actually no duplicated logic. Does your comment still > apply? > > I think this is ShellUtil::GetBrowserModelId(). Maybe someone more experienced with the installer should look at this function? They appear to have a similar purpose, but the version in ShellUtil is substantially more complicated. It's building something at runtime, whereas the other version is just returning 1 of 2 values depending on a preprocessor define. Why the difference? delegate_execute already depends on installer_util for other stuff, so it's easy to put a function in InstallerUtil that returns the app user model id. But does it need to do the complicated version as is being done in ShellUtil, or can it do the simple version as is done in delegate_execute?
I had to do a slight refactor because metro_driver was directly compiling 1 of the files from delegate_execute, but did not include the same linker dependencies as delegate_execute. Rather than make the hack worse, I went ahead and shuffled some functions around into InstallerUtil / ShellUtil (most of the functions already had comments indicating that they were unnecessarily duplicated). So I'd like to get comments (ananata|cpu) & (grt|gab) again regarding this new set of changes.
Added a linker dependency from metro_driver to installer_util. I tried to accomplish this by doing a minor refactor to remove some duplicated code, but I ultimately had to revert that change and delete the corresponding patchset, because it led to a rabbit hole, so that should probably be dealt with separately. https://codereview.chromium.org/23258005/diff/52001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/23258005/diff/52001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/browser_options_handler.cc:15: #include "base/metrics/field_trial.h" Changes to this file in this patchset are entirely due to a rebase.
Looking good; installer stuff below. Cheers! Gab https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:132: string16 InstallUtil::GetAppUserModelId() { This is GetBaseAppId() from BrowserDistribution. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:322: static string16 GetBrowserProgIdPrefix(); This should be in BrowserDistribution and have subclassing and runtime assignment through GetDistribution() do the logic split between Chrome/Canary/Chromium. See GetBaseAppId() in BrowserDistribution for an example. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:325: static string16 GetBrowserProgIdDesc(); Same for this. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/chr... File win8/delegate_execute/chrome_util.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/chr... win8/delegate_execute/chrome_util.cc:254: string16 GetAppId(const base::FilePath& chrome_exe) { All calls to this can be replaced by ShellUtil::GetBrowserModelId(). The extra complexity in there is: 1) Command-line stuff for the installer; will no matter for delegate_execute and be a no-op. 2) Use the BuildAppModelId to finish the final value; this is not really necessary for this path (hence why I didn't copy that logic here I guess), but it is not wrong to go with the generic code and call it anyways... To respond to your comment; the current method is also building something at runtime (i.e. appending a constant and the user-specific-runtime-generated-suffix); they will have the same results for this code path. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:23: #include "chrome/installer/util/install_util.h" What is this include for? https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:61: clsid_ = sxs_command_execute_impl_clsid; Use BrowserDistribution's GetCommandExecuteImplClsid() instead https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/metro_d... File win8/metro_driver/metro_driver.gyp (right): https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/metro_d... win8/metro_driver/metro_driver.gyp:59: '../../chrome/chrome.gyp:installer_util', Please insert this in alphabetical order; i.e., just below '../../base/base.gyp:base', Please document in the CL description how much bigger this makes delegate_execute.exe (we want to keep it as lightweight as possible; or at least that used to be a goal initially...). I don't think installer_util is big (in particular thanks to a refactoring of it recently driven by huangs@ to be able to depend on it in app_host.exe) so you should be fine, but better to double-check and document for all :). https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/winrt_u... File win8/metro_driver/winrt_utils.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/winrt_u... win8/metro_driver/winrt_utils.cc:213: if (InstallUtil::IsChromeSxSProcess()) Use BrowserDistribution::GetShortcutName() instead of hardcoding the name here (you'll have to append installer::kLnkExt to that name).
https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:261: NOTREACHED(); On 2013/08/30 03:28:18, grt wrote: > why NOTREACHED rather than noop? NOTREACHED will cause debug builds of Google > Chrome and Chromium to crash if someone holds down the magic key combo. ping? https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... chrome/installer/util/google_chrome_sxs_distribution.cc:21: L"{1BEAC3E3-B852-44F4-B468-8906C062422E}"; On 2013/08/30 03:28:18, grt wrote: > nit: 4-space indent ping? https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... File win8/delegate_execute/command_execute_impl.h (right): https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... win8/delegate_execute/command_execute_impl.h:108: OBJECT_ENTRY_AUTO(__uuidof(CommandExecuteImpl), CommandExecuteImpl) On 2013/08/30 03:28:18, grt wrote: > just curious: what's up with the whitespace change here? was the file previously > missing an EOL here? ping? https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:39: DEFINE_GUID(default_command_execute_impl_clsid, 0xA2DF06F9, 0xA21A, 0x44A8, On 2013/08/30 03:28:18, grt wrote: > suggestion: rename "default" to "chromium" and "chrome" to "google_chrome" so > that the intent is more obvious. ping? https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:97: IUnknown* impl_; On 2013/08/30 03:28:18, grt wrote: > consider base::win::ScopedHandle impl_; > impl_.Receive() instead of &impl_ on line 72 > impl_.Close(); instead of lines 88-91 ping? https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:99: GUID clsid_; On 2013/08/30 03:28:18, grt wrote: > does this need to be an instance variable, or can it be a local var in > PreMessageLoop? ping? https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:264: nit: remove extra newline https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... chrome/installer/util/install_util.h:34: static string16 GetAppUserModelId(); de-indent one space and add a comment https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1347: const wchar_t* ShellUtil::kChromeStableHTMLProgId = L"ChromeHTML"; these constants are no longer public, so move them up into the unnamed namespace. looks like there's a private constant on line 74, so that's a good place for them. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:233: static const wchar_t* kChromeStableHTMLProgId; remove these constants from the header since they shouldn't be used directly. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/chr... File win8/delegate_execute/chrome_util.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/chr... win8/delegate_execute/chrome_util.cc:34: // TODO(grt): These constants live in installer_util. Consider moving them how many of these constants can be replaced with corresponding items in chrome/installer/util/util_constants.h? https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:96: private: nit: indent one space
A bit of refactoring. The only big behavioral change was that prior to this patch, SxS registry settings would not get created because dist->CanSetAsDefault() returned false unless DebugEnableSetAsDefault() had been previously called. So I added a new function which returns true if the browser distribution supports the operation of setting it as the default, and use this function during registration instead. https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:261: NOTREACHED(); On 2013/09/04 03:33:36, grt wrote: > On 2013/08/30 03:28:18, grt wrote: > > why NOTREACHED rather than noop? NOTREACHED will cause debug builds of Google > > Chrome and Chromium to crash if someone holds down the magic key combo. > > ping? Done. https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/23258005/diff/36001/chrome/installer/util/goo... chrome/installer/util/google_chrome_sxs_distribution.cc:21: L"{1BEAC3E3-B852-44F4-B468-8906C062422E}"; On 2013/09/04 03:33:36, grt wrote: > On 2013/08/30 03:28:18, grt wrote: > > nit: 4-space indent > > ping? Done. https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... File win8/delegate_execute/command_execute_impl.h (right): https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/com... win8/delegate_execute/command_execute_impl.h:108: OBJECT_ENTRY_AUTO(__uuidof(CommandExecuteImpl), CommandExecuteImpl) On 2013/09/04 03:33:36, grt wrote: > On 2013/08/30 03:28:18, grt wrote: > > just curious: what's up with the whitespace change here? was the file > previously > > missing an EOL here? > > ping? Fixed this, the newline was there, I had unintentionally removed it. It should be back now. https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:39: DEFINE_GUID(default_command_execute_impl_clsid, 0xA2DF06F9, 0xA21A, 0x44A8, On 2013/09/04 03:33:36, grt wrote: > On 2013/08/30 03:28:18, grt wrote: > > suggestion: rename "default" to "chromium" and "chrome" to "google_chrome" so > > that the intent is more obvious. > > ping? All of these are gone now. https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:97: IUnknown* impl_; On 2013/09/04 03:33:36, grt wrote: > On 2013/08/30 03:28:18, grt wrote: > > consider base::win::ScopedHandle impl_; > > impl_.Receive() instead of &impl_ on line 72 > > impl_.Close(); instead of lines 88-91 > > ping? Used base::win::ScopedComPtr https://codereview.chromium.org/23258005/diff/36001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:99: GUID clsid_; On 2013/09/04 03:33:36, grt wrote: > On 2013/08/30 03:28:18, grt wrote: > > does this need to be an instance variable, or can it be a local var in > > PreMessageLoop? > > ping? Done. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:264: On 2013/09/04 03:33:36, grt wrote: > nit: remove extra newline Done. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... chrome/installer/util/install_util.cc:132: string16 InstallUtil::GetAppUserModelId() { On 2013/09/03 21:05:19, gab wrote: > This is GetBaseAppId() from BrowserDistribution. Done. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/ins... chrome/installer/util/install_util.h:34: static string16 GetAppUserModelId(); On 2013/09/04 03:33:36, grt wrote: > de-indent one space and add a comment Function is gone from latest patch. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1347: const wchar_t* ShellUtil::kChromeStableHTMLProgId = L"ChromeHTML"; On 2013/09/04 03:33:36, grt wrote: > these constants are no longer public, so move them up into the unnamed > namespace. looks like there's a private constant on line 74, so that's a good > place for them. Done. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:233: static const wchar_t* kChromeStableHTMLProgId; On 2013/09/04 03:33:36, grt wrote: > remove these constants from the header since they shouldn't be used directly. Done. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:322: static string16 GetBrowserProgIdPrefix(); On 2013/09/03 21:05:19, gab wrote: > This should be in BrowserDistribution and have subclassing and runtime > assignment through GetDistribution() do the logic split between > Chrome/Canary/Chromium. > > See GetBaseAppId() in BrowserDistribution for an example. Done. https://codereview.chromium.org/23258005/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:325: static string16 GetBrowserProgIdDesc(); On 2013/09/03 21:05:19, gab wrote: > Same for this. Done. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/chr... File win8/delegate_execute/chrome_util.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/chr... win8/delegate_execute/chrome_util.cc:34: // TODO(grt): These constants live in installer_util. Consider moving them On 2013/09/04 03:33:36, grt wrote: > how many of these constants can be replaced with corresponding items in > chrome/installer/util/util_constants.h? I was only able to replace one constant, the one for the return code. The rest do not seem to exist anywhere else. Hopefully I didn't miss anything too obvious. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/chr... win8/delegate_execute/chrome_util.cc:254: string16 GetAppId(const base::FilePath& chrome_exe) { On 2013/09/03 21:05:19, gab wrote: > All calls to this can be replaced by ShellUtil::GetBrowserModelId(). > > The extra complexity in there is: > 1) Command-line stuff for the installer; will no matter for delegate_execute and > be a no-op. > 2) Use the BuildAppModelId to finish the final value; this is not really > necessary for this path (hence why I didn't copy that logic here I guess), but > it is not wrong to go with the generic code and call it anyways... > > To respond to your comment; the current method is also building something at > runtime (i.e. appending a constant and the > user-specific-runtime-generated-suffix); they will have the same results for > this code path. Done. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:23: #include "chrome/installer/util/install_util.h" On 2013/09/03 21:05:19, gab wrote: > What is this include for? It was originally for InstallUtil::IsChromeSxSProcess, but now that's been removed. This include is still in patch 9, but I will upload patch 10 to fix this. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:61: clsid_ = sxs_command_execute_impl_clsid; On 2013/09/03 21:05:19, gab wrote: > Use BrowserDistribution's GetCommandExecuteImplClsid() instead Done. https://codereview.chromium.org/23258005/diff/78001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:96: private: On 2013/09/04 03:33:36, grt wrote: > nit: indent one space Done. https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/metro_d... File win8/metro_driver/metro_driver.gyp (right): https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/metro_d... win8/metro_driver/metro_driver.gyp:59: '../../chrome/chrome.gyp:installer_util', On 2013/09/03 21:05:19, gab wrote: > Please insert this in alphabetical order; i.e., just below > '../../base/base.gyp:base', > > Please document in the CL description how much bigger this makes > delegate_execute.exe (we want to keep it as lightweight as possible; or at least > that used to be a goal initially...). > I don't think installer_util is big (in particular thanks to a refactoring of it > recently driven by huangs@ to be able to depend on it in app_host.exe) so you > should be fine, but better to double-check and document for all :). Done. https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/winrt_u... File win8/metro_driver/winrt_utils.cc (right): https://codereview.chromium.org/23258005/diff/78001/win8/metro_driver/winrt_u... win8/metro_driver/winrt_utils.cc:213: if (InstallUtil::IsChromeSxSProcess()) On 2013/09/03 21:05:19, gab wrote: > Use BrowserDistribution::GetShortcutName() instead of hardcoding the name here > (you'll have to append installer::kLnkExt to that name). Done.
https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:44: On 2013/08/25 23:27:11, cpu wrote: > please put a comment here about what you are doing at a high level (i.e > dynamically registering blah because of canary and stable .. blah) Done. https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:72: (LPVOID*)&impl_); On 2013/08/25 23:27:11, cpu wrote: > change the c-sytle cast to reinterpret_cast Changed to a scoped com ptr. https://codereview.chromium.org/23258005/diff/32001/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:97: IUnknown* impl_; On 2013/08/25 23:27:11, cpu wrote: > impl_ strikes me as a bad name, how about factory_, class_object_ or something > like that? > It's not really a factory of anything, it's created *by* the factory. So it's actually just the actual object itself. I changed the name to instance_ to make this more clear.
On 2013/09/05 01:35:29, zturner wrote: > A bit of refactoring. The only big behavioral change was that prior to this > patch, SxS registry settings would not get created because > dist->CanSetAsDefault() returned false unless DebugEnableSetAsDefault() had been > previously called. I thought that was desirable. SxS shouldn't register unless the magic key combination is used, should it?
On 2013/09/05 02:37:39, grt wrote: > On 2013/09/05 01:35:29, zturner wrote: > > A bit of refactoring. The only big behavioral change was that prior to this > > patch, SxS registry settings would not get created because > > dist->CanSetAsDefault() returned false unless DebugEnableSetAsDefault() had > been > > previously called. > > I thought that was desirable. SxS shouldn't register unless the magic key > combination is used, should it? To clarify: if SxS registers, then it can be made the default browser by ordinary Windowsy means (i.e., the Default Programs control panel), no? I wasn't under the impression that this was the intent.
On 2013/09/05 02:48:35, grt wrote: > On 2013/09/05 02:37:39, grt wrote: > > On 2013/09/05 01:35:29, zturner wrote: > > > A bit of refactoring. The only big behavioral change was that prior to this > > > patch, SxS registry settings would not get created because > > > dist->CanSetAsDefault() returned false unless DebugEnableSetAsDefault() had > > been > > > previously called. > > > > I thought that was desirable. SxS shouldn't register unless the magic key > > combination is used, should it? > > To clarify: if SxS registers, then it can be made the default browser by > ordinary Windowsy means (i.e., the Default Programs control panel), no? I wasn't > under the impression that this was the intent. Hmm, you're probably right but I suppose I thought there was more to it than that. I'll verify that in the morning. But this begs the question of whether we need any magic key combination at all. If the ProgID and AppID are the only things required to be able to set the default browser, then isn't it sufficient to just re-run setup with --register-dev-chrome from the install directory of a SxS installation?
On 2013/09/05 03:51:12, zturner1 wrote: > On 2013/09/05 02:48:35, grt wrote: > > On 2013/09/05 02:37:39, grt wrote: > > > On 2013/09/05 01:35:29, zturner wrote: > > > > A bit of refactoring. The only big behavioral change was that prior to > this > > > > patch, SxS registry settings would not get created because > > > > dist->CanSetAsDefault() returned false unless DebugEnableSetAsDefault() > had > > > been > > > > previously called. > > > > > > I thought that was desirable. SxS shouldn't register unless the magic key > > > combination is used, should it? > > > > To clarify: if SxS registers, then it can be made the default browser by > > ordinary Windowsy means (i.e., the Default Programs control panel), no? I > wasn't > > under the impression that this was the intent. > > Hmm, you're probably right but I suppose I thought there was more to it than > that. I'll verify that in the morning. But this begs the question of whether > we need any magic key combination at all. If the ProgID and AppID are the only > things required to be able to set the default browser, then isn't it sufficient > to just re-run setup with --register-dev-chrome from the install directory of a > SxS installation? Actually after thinking about it for a little bit I think I understand where I got confused. I'll fix this up this morning.
* Fixed bug where CheckIsChromeSxSProcess was returning false when run from executables under the application base dir. * Fixed bug where some of the updating code was using the wrong GUIDs. * Re-disabled register-on-install for SxS (but kept unregister-on-uninstall) Tested as follows: 1) Built mini-installer 2) setup.exe --chrome --multi-install 3) setup.exe --chrome-sxs (Except where not applicable, I verified all of these with both aura and non-aura builds) * Verified that Chrome can be set as default * Verified that Chrome SXS can be set as default (through key combo) * Verified that the default browser dialog displays the correct title for each distro (e.g. "Google Chrome Canary", and "Google Chrome") * Verified that automatic update check works and successfully downloads/applies the update (SxS) * Verified that both browsers can be launched from the desktop shortcut regardless of whether they are default browser. * Verified that both browsers can be launched in metro mode if and only if they are set as the default browser. * Verified that when launched from any shortcut (desktop/start) and in any mode (metro/desktop), all relevant executables (chrome.exe/delegate_execute.exe) are always run from the correct installation directory. * Verified that setup.exe --chrome --uninstall uninstalls the chrome distribution and removes registration entries. * Verified that setup.exe --chrome-sxs --uninstall uninstalls the SxS distribution and removes registration entries.
how does this work now that DebugEnableSetAsDefault is never called? https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:128: // Returns true if setting the default browser is a supported operation. it seems that uninstall is the only real use of this function (the metro-specific user data dir is history). what do you think of removing this and changing uninstall.cc's DeleteChromeRegistrationKeys so that it does an early exit if the distribution has an empty BrowserProgIdPrefix (and provide overrides for GCF and the binaries so that GetBrowserProgIdPrefix() returns an empty string for them)? https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:52: if (!dist->GetCommandExecuteImplClsid(&clsid)) did you consider using CLSIDFromString on the clsid-as-string here instead of plumbing new methods through?
On 2013/09/06 02:53:38, grt wrote: > how does this work now that DebugEnableSetAsDefault is never called? It doesn't (as in there's no way to invoke any of this set-as-default code without calling DebugEnableSetAsDefault. However, that particular change was not going to make it through the OWNERS of the settings page, so in the bug (see the link in the initial code review message), we decided that the best course of action was to remove the magic key combination from the settings page and put it somewhere else. Where else isn't totally decided upon, so I thought the best course of action is to submit the infrastructure work to make the change possible now, and then in a subsequent changelist just submit a simple CL that sets the flag somewhere. > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > File chrome/installer/util/browser_distribution.h (right): > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > chrome/installer/util/browser_distribution.h:128: // Returns true if setting the > default browser is a supported operation. > it seems that uninstall is the only real use of this function (the > metro-specific user data dir is history). what do you think of removing this and > changing uninstall.cc's DeleteChromeRegistrationKeys so that it does an early > exit if the distribution has an empty BrowserProgIdPrefix (and provide overrides > for GCF and the binaries so that GetBrowserProgIdPrefix() returns an empty > string for them)? Yea I toyed with that idea as well. It's also fine, I can make that change tomorrow. > > https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/del... > File win8/delegate_execute/delegate_execute.cc (right): > > https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/del... > win8/delegate_execute/delegate_execute.cc:52: if > (!dist->GetCommandExecuteImplClsid(&clsid)) > did you consider using CLSIDFromString on the clsid-as-string here instead of > plumbing new methods through? I didn't find CLSIDFromString incidentally, I thought UuidFromString was the only method. And UuidFromString is really ugly since it requires both a const_cast<> *and* a reinterpret_cast<>. So it was because of UuidFromString that I decided to do it this way. CLSIDFromSTring looks const-correct though, so I suppose that's probably a better solution. Incidentally, we have base::guid, but it doesn't support any useful methods. At some point we should think about building up base::guid to be more useful.
Preliminary review before I get on the plane... will take a deeper look on Monday. Cheers! Gab https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:212: // 39character limit mentioned on MSDN includes the NULL character... The generic part of this comment should go in the header; the parts specific to the fact that this had to change for Chromium can stay here. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:276: return true; How about having this return CanSetAsDefault() unless overriden to do something else? Than all distributions for which this copies CanSetAsDefault don't need to also override this method. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:94: // alpha-numeric identifier. |suffix| is actually a '.' followed by a 26-character alpha-numeric identifier. Also add to this comment that this prefix must be 11 characters or less so that |prefix||.suffix| is 38 characters or less given the restriction that the ProgId be 39 characters or less including the NULL character (something like the previous comment in shell_util.cc, maybe slightly less details would do here). https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:128: // Returns true if setting the default browser is a supported operation. On 2013/09/06 02:53:39, grt wrote: > it seems that uninstall is the only real use of this function (the > metro-specific user data dir is history). what do you think of removing this and > changing uninstall.cc's DeleteChromeRegistrationKeys so that it does an early > exit if the distribution has an empty BrowserProgIdPrefix (and provide overrides > for GCF and the binaries so that GetBrowserProgIdPrefix() returns an empty > string for them)? Hmmm, I'm not sure about that, we usually put NOTREACHED() in methods of distributions that shouldn't be called (i.e. GetBrowserProgIdPrefix for binaries/CF); I find that safer than returning empty strings only as a mean to check what this new method is trying to provide clearly. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/chr... File chrome/installer/util/chrome_frame_distribution.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/chr... chrome/installer/util/chrome_frame_distribution.cc:138: return false; I find it sad to have to duplicate this logic from GetCommandExecuteImplClsid() everywhere... I remember we talked about this and determined this was probably the easiest way; but what was the reason again for not being able to only store the GUID as a constant and have this method return the string conversion of each distribution's GetCommandExecuteImplClsid()? https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/chr... File chrome/installer/util/chromium_binaries_distribution.h (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/chr... chrome/installer/util/chromium_binaries_distribution.h:38: virtual string16 GetStateKey() OVERRIDE; Binaries and CF (and maybe others) should also override BrowserProgIdPrefix() and have it be a NOTREACHED(); This is weird I know, but it's a sad side-effect of the bad state of having the Chromium distribution at the root of the class hierarchy... https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/hel... File chrome/installer/util/helper.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/hel... chrome/installer/util/helper.cc:46: void GetChromeUserDataPaths(BrowserDistribution* dist, I doubt we even need this method anymore... the Metro user-data-dir is a thing of the past; we never even shipped it to stable AFAIK...? cpu? https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/ins... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/ins... chrome/installer/util/install_util.cc:374: while (iter != components.end()) { I would say it's faster (and perhaps slightly more reliable) to do a reverse lookup (i.e. using reverse iterators); and look for kInstallBinaryDir and make sure it's under chrome_sxs_dir. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/ins... chrome/installer/util/install_util.cc:389: // still be treated as a SxS installation. This case is safer with the reverse iteration proposed above. https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/chr... File win8/delegate_execute/chrome_util.cc (left): https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/chr... win8/delegate_execute/chrome_util.cc:263: !GetUserSpecificRegistrySuffix(&suffix)) { You can likely get rid of these local methods copied here likely only to support this method. https://codereview.chromium.org/23258005/diff/63029/win8/metro_driver/winrt_u... File win8/metro_driver/winrt_utils.cc (right): https://codereview.chromium.org/23258005/diff/63029/win8/metro_driver/winrt_u... win8/metro_driver/winrt_utils.cc:214: link_name.append(installer::kLnkExt); I think it's fine to do this inline above (i.e. GetShortcutName() + installer::kLnkExt).
Latest set of fixes based on review comments. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:212: // 39character limit mentioned on MSDN includes the NULL character... On 2013/09/06 18:40:08, gab wrote: > The generic part of this comment should go in the header; the parts specific to > the fact that this had to change for Chromium can stay here. Done. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:212: // 39character limit mentioned on MSDN includes the NULL character... On 2013/09/06 18:40:08, gab wrote: > The generic part of this comment should go in the header; the parts specific to > the fact that this had to change for Chromium can stay here. Done. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:276: return true; On 2013/09/06 18:40:08, gab wrote: > How about having this return CanSetAsDefault() unless overriden to do something > else? Than all distributions for which this copies CanSetAsDefault don't need to > also override this method. If we follow Greg's suggestion of deleting this method entirely and then using the return value of GetBrowserProgIdPrefix() to check, then that would also solve the problem. I kind of agree that it's awkward to have to have another virtual function for this purpose. Another option is to keep this function, but make it a non-virtual base class helper function that is implemented by calling the virtual GetBrowserProgIdPrefix() function and checking if the return value is the empty string. That has the other issue you point out elsewhere though, where we rely on the function returning an empty string instead of calling NOTREACHED(), so I guess we have to decide how to deal with that. One idea I had was to change the signature of CanSetAsDefault() to return a tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would require a lot of minor code tweaks to get various other parts of the codebase to compile, and bring in some unnecessary owners. It's possible we can do something like that as a subsequent changelist if you like the idea. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:94: // alpha-numeric identifier. On 2013/09/06 18:40:08, gab wrote: > |suffix| is actually a '.' followed by a 26-character alpha-numeric identifier. > > Also add to this comment that this prefix must be 11 characters or less so that > |prefix||.suffix| is 38 characters or less given the restriction that the ProgId > be 39 characters or less including the NULL character (something like the > previous comment in shell_util.cc, maybe slightly less details would do here). Done. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/chr... File chrome/installer/util/chrome_frame_distribution.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/chr... chrome/installer/util/chrome_frame_distribution.cc:138: return false; On 2013/09/06 18:40:08, gab wrote: > I find it sad to have to duplicate this logic from GetCommandExecuteImplClsid() > everywhere... I remember we talked about this and determined this was probably > the easiest way; but what was the reason again for not being able to only store > the GUID as a constant and have this method return the string conversion of each > distribution's GetCommandExecuteImplClsid()? StringFromCLSID() and UuidToString() both internally allocate memory for the string, which the caller is required to free. (CoTaskMemFree and RpcStringFree, respectively). We could still provide the helper and to the memory freeing there, but it just seemed ugly. We decided not to do the reverse (store the string and convert to GUID) because the opposite function, UuidFromString, isn't const correct and would require both a const_cast<> and a reinterpret_cast<> in order to call the function. So that was also ugly. However, Greg pointed out CLSIDFromString(), which is const-correct, so that's probably the best solution. At some point, someone (I wouldn't mind doing it) should build out base::guid so that it's more useful and does all this for us. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/ins... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/ins... chrome/installer/util/install_util.cc:374: while (iter != components.end()) { On 2013/09/06 18:40:08, gab wrote: > I would say it's faster (and perhaps slightly more reliable) to do a reverse > lookup (i.e. using reverse iterators); and look for kInstallBinaryDir and make > sure it's under chrome_sxs_dir. Done. https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/ins... chrome/installer/util/install_util.cc:389: // still be treated as a SxS installation. On 2013/09/06 18:40:08, gab wrote: > This case is safer with the reverse iteration proposed above. Done. https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/chr... File win8/delegate_execute/chrome_util.cc (left): https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/chr... win8/delegate_execute/chrome_util.cc:263: !GetUserSpecificRegistrySuffix(&suffix)) { On 2013/09/06 18:40:08, gab wrote: > You can likely get rid of these local methods copied here likely only to support > this method. Done. https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/del... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/63029/win8/delegate_execute/del... win8/delegate_execute/delegate_execute.cc:52: if (!dist->GetCommandExecuteImplClsid(&clsid)) On 2013/09/06 02:53:39, grt wrote: > did you consider using CLSIDFromString on the clsid-as-string here instead of > plumbing new methods through? Done. I made this comment somewhere else as well. But TL;DR is that I didn't know about CLSIDFromString, only UuidFromString, and UuidFromString is kinda ugly so I did this instead. Changed to use CLSIDFromString since it doesn't have the same problem. https://codereview.chromium.org/23258005/diff/63029/win8/metro_driver/winrt_u... File win8/metro_driver/winrt_utils.cc (right): https://codereview.chromium.org/23258005/diff/63029/win8/metro_driver/winrt_u... win8/metro_driver/winrt_utils.cc:214: link_name.append(installer::kLnkExt); On 2013/09/06 18:40:08, gab wrote: > I think it's fine to do this inline above (i.e. GetShortcutName() + > installer::kLnkExt). Done.
https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:128: // Returns true if setting the default browser is a supported operation. On 2013/09/06 18:40:08, gab wrote: > On 2013/09/06 02:53:39, grt wrote: > > it seems that uninstall is the only real use of this function (the > > metro-specific user data dir is history). what do you think of removing this > and > > changing uninstall.cc's DeleteChromeRegistrationKeys so that it does an early > > exit if the distribution has an empty BrowserProgIdPrefix (and provide > overrides > > for GCF and the binaries so that GetBrowserProgIdPrefix() returns an empty > > string for them)? > > Hmmm, I'm not sure about that, we usually put NOTREACHED() in methods of > distributions that shouldn't be called (i.e. GetBrowserProgIdPrefix for > binaries/CF); I find that safer than returning empty strings only as a mean to > check what this new method is trying to provide clearly. I see your point. I find these two methods (IsFooSupported and CanFoo) confusing. I think a simpler API is preferable. I find making CanSetAsDefault a tristate less so. How do you find that proposal? https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/hel... File chrome/installer/util/helper.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/hel... chrome/installer/util/helper.cc:46: void GetChromeUserDataPaths(BrowserDistribution* dist, On 2013/09/06 18:40:08, gab wrote: > I doubt we even need this method anymore... the Metro user-data-dir is a thing > of the past; we never even shipped it to stable AFAIK...? > > cpu? Yes, this is history. I think it can be removed (in a separate CL). https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/br... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/br... chrome/installer/util/browser_distribution.h:146: virtual bool GetCommandExecuteImplClsidString(string16* handler_class_uuid); please revert this to its original name to reduce needless code churn. https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... chrome/installer/util/install_util.cc:373: auto iter = components.rbegin(); i don't believe auto is approved for use yet. please either drop its use, or show me what i missed. https://codereview.chromium.org/23258005/diff/138001/win8/delegate_execute/de... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/138001/win8/delegate_execute/de... win8/delegate_execute/delegate_execute.cc:55: if (FAILED(hr = ::CLSIDFromString(clsid_string.c_str(), &clsid))) please do the assignment in its own statement
On 2013/09/09 18:39:14, grt wrote: > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > File chrome/installer/util/install_util.cc (right): > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > chrome/installer/util/install_util.cc:373: auto iter = components.rbegin(); > i don't believe auto is approved for use yet. please either drop its use, or > show me what i missed. Will address the rest of the comments with a patch upload, but in regards to this one, I actually checked before I used it and it looks ok. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#auto
On 2013/09/09 19:04:14, zturner wrote: > On 2013/09/09 18:39:14, grt wrote: > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > File chrome/installer/util/install_util.cc (right): > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > chrome/installer/util/install_util.cc:373: auto iter = components.rbegin(); > > i don't believe auto is approved for use yet. please either drop its use, or > > show me what i missed. > > Will address the rest of the comments with a patch upload, but in regards to > this one, I actually checked before I used it and it looks ok. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#auto Google style-guide and Chromium style guide are different. I thought auto was C++11 which we don't yet support in Chromium.
On 2013/09/09 19:14:28, gab wrote: > On 2013/09/09 19:04:14, zturner wrote: > > On 2013/09/09 18:39:14, grt wrote: > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > > File chrome/installer/util/install_util.cc (right): > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > > chrome/installer/util/install_util.cc:373: auto iter = components.rbegin(); > > > i don't believe auto is approved for use yet. please either drop its use, or > > > show me what i missed. > > > > Will address the rest of the comments with a patch upload, but in regards to > > this one, I actually checked before I used it and it looks ok. > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#auto > > Google style-guide and Chromium style guide are different. I thought auto was > C++11 which we don't yet support in Chromium. I was also going off of the chromium style guide: http://dev.chromium.org/developers/coding-style which says at the top "If this document doesn't mention a rule, follow the google style guide". I thought we did allow some C++11, but only a very small amount (in particular whatever is supported by the google style guide)
https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... chrome/installer/util/google_chrome_sxs_distribution.cc:80: return enable_set_as_default_; I just talked about this with grt@; can we get the ok to make this return true, always, but explicitly hide all the UI bits for SxS? That way Canary will always register itself, it just won't be possible to make it default through the UI (you'd have to go in Windows' Programs & Features/Default Programs to give Chrome default -- which no one does by accident). This would avoid code-churn to enable this.
On 2013/09/09 19:17:21, zturner wrote: > On 2013/09/09 19:14:28, gab wrote: > > On 2013/09/09 19:04:14, zturner wrote: > > > On 2013/09/09 18:39:14, grt wrote: > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > > > File chrome/installer/util/install_util.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > > > chrome/installer/util/install_util.cc:373: auto iter = > components.rbegin(); > > > > i don't believe auto is approved for use yet. please either drop its use, > or > > > > show me what i missed. > > > > > > Will address the rest of the comments with a patch upload, but in regards to > > > this one, I actually checked before I used it and it looks ok. > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#auto > > > > Google style-guide and Chromium style guide are different. I thought auto was > > C++11 which we don't yet support in Chromium. > > I was also going off of the chromium style guide: > > http://dev.chromium.org/developers/coding-style > > which says at the top "If this document doesn't mention a rule, follow the > google style guide". > > I thought we did allow some C++11, but only a very small amount (in particular > whatever is supported by the google style guide) I don't think it's allowed yet, not all platforms have a working C++11 compiler; see https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/oVTPsijC7d0
On 2013/09/09 19:04:14, zturner wrote: > On 2013/09/09 18:39:14, grt wrote: > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > File chrome/installer/util/install_util.cc (right): > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/in... > > chrome/installer/util/install_util.cc:373: auto iter = components.rbegin(); > > i don't believe auto is approved for use yet. please either drop its use, or > > show me what i missed. > > Will address the rest of the comments with a patch upload, but in regards to > this one, I actually checked before I used it and it looks ok. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#auto Recent threads on chromium-dev suggest that we still can't use C++11 despite the Google style guide approving a subset; see, for example, https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zdIwdtZnSQI/VJT4f....
On 2013/09/09 19:18:20, gab wrote: > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > enable_set_as_default_; > I just talked about this with grt@; can we get the ok to make this return true, > always, but explicitly hide all the UI bits for SxS? > > That way Canary will always register itself, it just won't be possible to make > it default through the UI (you'd have to go in Windows' Programs & > Features/Default Programs to give Chrome default -- which no one does by > accident). > > This would avoid code-churn to enable this. I think we'd still have to touch browser_options_handler.cc for this, right?
https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... chrome/installer/util/google_chrome_sxs_distribution.cc:80: return enable_set_as_default_; On 2013/09/09 19:18:21, gab wrote: > I just talked about this with grt@; can we get the ok to make this return true, > always, but explicitly hide all the UI bits for SxS? > > That way Canary will always register itself, it just won't be possible to make > it default through the UI (you'd have to go in Windows' Programs & > Features/Default Programs to give Chrome default -- which no one does by > accident). > > This would avoid code-churn to enable this. If that's not an option, them I'm okay with gab's suggestion to have BrowserDistribution::IsSetAsDefaultSupported() return CanSetAsDefault();
On 2013/09/09 19:22:08, zturner wrote: > On 2013/09/09 19:18:20, gab wrote: > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > enable_set_as_default_; > > I just talked about this with grt@; can we get the ok to make this return > true, > > always, but explicitly hide all the UI bits for SxS? > > > > That way Canary will always register itself, it just won't be possible to make > > it default through the UI (you'd have to go in Windows' Programs & > > Features/Default Programs to give Chrome default -- which no one does by > > accident). > > > > This would avoid code-churn to enable this. > > I think we'd still have to touch browser_options_handler.cc for this, right? Actually even aside from that, I don't think laforge will allow that either because even though it's not by accident, he doesn't even want users doing it on purpose.
On 2013/09/09 19:22:56, grt wrote: > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > enable_set_as_default_; > On 2013/09/09 19:18:21, gab wrote: > > I just talked about this with grt@; can we get the ok to make this return > true, > > always, but explicitly hide all the UI bits for SxS? > > > > That way Canary will always register itself, it just won't be possible to make > > it default through the UI (you'd have to go in Windows' Programs & > > Features/Default Programs to give Chrome default -- which no one does by > > accident). > > > > This would avoid code-churn to enable this. > > If that's not an option, them I'm okay with gab's suggestion to have > BrowserDistribution::IsSetAsDefaultSupported() return CanSetAsDefault(); Not sure I follow this option. If the two functions are identical, then why have both? Making it happen through register-dev-chrome might fix everything, because the crux of the issue seems to be that the fact that the value of CanSetAsDefault() changes at runtime. If we keep the existing code's assumption that it will never change at runtime, everything's fine. Setting it as default via the command line option keeps this assumption. So perhaps one approach is for me to just delete the DebugEnableSetAsDefault() function for now, and only check in the code to separate out the guids etc, but don't put any registration stuff in. Then we can deal with the other issues in a separate CL.
On 2013/09/09 19:22:58, zturner wrote: > On 2013/09/09 19:22:08, zturner wrote: > > On 2013/09/09 19:18:20, gab wrote: > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > > enable_set_as_default_; > > > I just talked about this with grt@; can we get the ok to make this return > > true, > > > always, but explicitly hide all the UI bits for SxS? > > > > > > That way Canary will always register itself, it just won't be possible to > make > > > it default through the UI (you'd have to go in Windows' Programs & > > > Features/Default Programs to give Chrome default -- which no one does by > > > accident). > > > > > > This would avoid code-churn to enable this. > > > > I think we'd still have to touch browser_options_handler.cc for this, right? > > Actually even aside from that, I don't think laforge will allow that either > because even though it's not by accident, he doesn't even want users doing it on > purpose. Well however we do it, we can't absolutely prevent motivated users from doing it (i.e., we're open-source...).
On 2013/09/09 19:31:05, gab wrote: > On 2013/09/09 19:22:58, zturner wrote: > > On 2013/09/09 19:22:08, zturner wrote: > > > On 2013/09/09 19:18:20, gab wrote: > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > > > enable_set_as_default_; > > > > I just talked about this with grt@; can we get the ok to make this return > > > true, > > > > always, but explicitly hide all the UI bits for SxS? > > > > > > > > That way Canary will always register itself, it just won't be possible to > > make > > > > it default through the UI (you'd have to go in Windows' Programs & > > > > Features/Default Programs to give Chrome default -- which no one does by > > > > accident). > > > > > > > > This would avoid code-churn to enable this. > > > > > > I think we'd still have to touch browser_options_handler.cc for this, right? > > > > Actually even aside from that, I don't think laforge will allow that either > > because even though it's not by accident, he doesn't even want users doing it > on > > purpose. > > Well however we do it, we can't absolutely prevent motivated users from doing it > (i.e., we're open-source...). Sure, but it's all a matter of difficulty. I mean they could even have done it before by making changes to the installer and getting the CL through review :) Anyway I'm not saying I disagree with you, just that I think laforge@ is pretty firm on this, if any decision to the exact method of enabling the default browser is made, it would probably have to get an lgtm from him.
https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... chrome/installer/util/browser_distribution.cc:276: return true; On 2013/09/06 20:53:16, zturner wrote: > On 2013/09/06 18:40:08, gab wrote: > > How about having this return CanSetAsDefault() unless overriden to do > something > > else? Than all distributions for which this copies CanSetAsDefault don't need > to > > also override this method. > > If we follow Greg's suggestion of deleting this method entirely and then using > the return value of GetBrowserProgIdPrefix() to check, then that would also > solve the problem. I kind of agree that it's awkward to have to have another > virtual function for this purpose. > > Another option is to keep this function, but make it a non-virtual base class > helper function that is implemented by calling the virtual > GetBrowserProgIdPrefix() function and checking if the return value is the empty > string. Meh, I prefer having two virtual functions with a default impl for one of them based on the other. > > That has the other issue you point out elsewhere though, where we rely on the > function returning an empty string instead of calling NOTREACHED(), so I guess > we have to decide how to deal with that. > > One idea I had was to change the signature of CanSetAsDefault() to return a > tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would require a lot of > minor code tweaks to get various other parts of the codebase to compile, and > bring in some unnecessary owners. > > It's possible we can do something like that as a subsequent changelist if you > like the idea. https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... chrome/installer/util/google_chrome_sxs_distribution.cc:80: return enable_set_as_default_; On 2013/09/09 19:22:57, grt wrote: > On 2013/09/09 19:18:21, gab wrote: > > I just talked about this with grt@; can we get the ok to make this return > true, > > always, but explicitly hide all the UI bits for SxS? > > > > That way Canary will always register itself, it just won't be possible to make > > it default through the UI (you'd have to go in Windows' Programs & > > Features/Default Programs to give Chrome default -- which no one does by > > accident). > > > > This would avoid code-churn to enable this. > > If that's not an option, them I'm okay with gab's suggestion to have > BrowserDistribution::IsSetAsDefaultSupported() return CanSetAsDefault(); ref: https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro...
On 2013/09/09 19:34:16, gab wrote: > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > File chrome/installer/util/browser_distribution.cc (right): > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > chrome/installer/util/browser_distribution.cc:276: return true; > On 2013/09/06 20:53:16, zturner wrote: > > On 2013/09/06 18:40:08, gab wrote: > > > How about having this return CanSetAsDefault() unless overriden to do > > something > > > else? Than all distributions for which this copies CanSetAsDefault don't > need > > to > > > also override this method. > > > > If we follow Greg's suggestion of deleting this method entirely and then using > > the return value of GetBrowserProgIdPrefix() to check, then that would also > > solve the problem. I kind of agree that it's awkward to have to have another > > virtual function for this purpose. > > > > Another option is to keep this function, but make it a non-virtual base class > > helper function that is implemented by calling the virtual > > GetBrowserProgIdPrefix() function and checking if the return value is the > empty > > string. > > Meh, I prefer having two virtual functions with a default impl for one of them > based on the other. > > > > > That has the other issue you point out elsewhere though, where we rely on the > > function returning an empty string instead of calling NOTREACHED(), so I guess > > we have to decide how to deal with that. > > > > One idea I had was to change the signature of CanSetAsDefault() to return a > > tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would require a lot > of > > minor code tweaks to get various other parts of the codebase to compile, and > > bring in some unnecessary owners. > > > > It's possible we can do something like that as a subsequent changelist if you > > like the idea. > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > enable_set_as_default_; > On 2013/09/09 19:22:57, grt wrote: > > On 2013/09/09 19:18:21, gab wrote: > > > I just talked about this with grt@; can we get the ok to make this return > > true, > > > always, but explicitly hide all the UI bits for SxS? > > > > > > That way Canary will always register itself, it just won't be possible to > make > > > it default through the UI (you'd have to go in Windows' Programs & > > > Features/Default Programs to give Chrome default -- which no one does by > > > accident). > > > > > > This would avoid code-churn to enable this. > > > > If that's not an option, them I'm okay with gab's suggestion to have > > BrowserDistribution::IsSetAsDefaultSupported() return CanSetAsDefault(); > > ref: > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... Ahh, right. Well if that makes everyone happy then I think that's reasonable. If it makes it less confusing, we can change the name of the function from IsSetAsDefaultSupported. For example, something like IsRegistrationSupported() or HasProgId()
On 2013/09/09 19:37:33, zturner wrote: > On 2013/09/09 19:34:16, gab wrote: > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > File chrome/installer/util/browser_distribution.cc (right): > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > chrome/installer/util/browser_distribution.cc:276: return true; > > On 2013/09/06 20:53:16, zturner wrote: > > > On 2013/09/06 18:40:08, gab wrote: > > > > How about having this return CanSetAsDefault() unless overriden to do > > > something > > > > else? Than all distributions for which this copies CanSetAsDefault don't > > need > > > to > > > > also override this method. > > > > > > If we follow Greg's suggestion of deleting this method entirely and then > using > > > the return value of GetBrowserProgIdPrefix() to check, then that would also > > > solve the problem. I kind of agree that it's awkward to have to have > another > > > virtual function for this purpose. > > > > > > Another option is to keep this function, but make it a non-virtual base > class > > > helper function that is implemented by calling the virtual > > > GetBrowserProgIdPrefix() function and checking if the return value is the > > empty > > > string. > > > > Meh, I prefer having two virtual functions with a default impl for one of them > > based on the other. > > > > > > > > That has the other issue you point out elsewhere though, where we rely on > the > > > function returning an empty string instead of calling NOTREACHED(), so I > guess > > > we have to decide how to deal with that. > > > > > > One idea I had was to change the signature of CanSetAsDefault() to return a > > > tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would require a > lot > > of > > > minor code tweaks to get various other parts of the codebase to compile, and > > > bring in some unnecessary owners. > > > > > > It's possible we can do something like that as a subsequent changelist if > you > > > like the idea. > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > enable_set_as_default_; > > On 2013/09/09 19:22:57, grt wrote: > > > On 2013/09/09 19:18:21, gab wrote: > > > > I just talked about this with grt@; can we get the ok to make this return > > > true, > > > > always, but explicitly hide all the UI bits for SxS? > > > > > > > > That way Canary will always register itself, it just won't be possible to > > make > > > > it default through the UI (you'd have to go in Windows' Programs & > > > > Features/Default Programs to give Chrome default -- which no one does by > > > > accident). > > > > > > > > This would avoid code-churn to enable this. > > > > > > If that's not an option, them I'm okay with gab's suggestion to have > > > BrowserDistribution::IsSetAsDefaultSupported() return CanSetAsDefault(); > > > > ref: > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > Ahh, right. Well if that makes everyone happy then I think that's reasonable. > If it makes it less confusing, we can change the name of the function from > IsSetAsDefaultSupported. For example, something like IsRegistrationSupported() > or HasProgId() IsRegistrationSupported makes sense. This would only be used at uninstall to see if the registration needs to be removed, right?
On 2013/09/09 19:40:06, grt wrote: > On 2013/09/09 19:37:33, zturner wrote: > > On 2013/09/09 19:34:16, gab wrote: > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > File chrome/installer/util/browser_distribution.cc (right): > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > chrome/installer/util/browser_distribution.cc:276: return true; > > > On 2013/09/06 20:53:16, zturner wrote: > > > > On 2013/09/06 18:40:08, gab wrote: > > > > > How about having this return CanSetAsDefault() unless overriden to do > > > > something > > > > > else? Than all distributions for which this copies CanSetAsDefault don't > > > need > > > > to > > > > > also override this method. > > > > > > > > If we follow Greg's suggestion of deleting this method entirely and then > > using > > > > the return value of GetBrowserProgIdPrefix() to check, then that would > also > > > > solve the problem. I kind of agree that it's awkward to have to have > > another > > > > virtual function for this purpose. > > > > > > > > Another option is to keep this function, but make it a non-virtual base > > class > > > > helper function that is implemented by calling the virtual > > > > GetBrowserProgIdPrefix() function and checking if the return value is the > > > empty > > > > string. > > > > > > Meh, I prefer having two virtual functions with a default impl for one of > them > > > based on the other. > > > > > > > > > > > That has the other issue you point out elsewhere though, where we rely on > > the > > > > function returning an empty string instead of calling NOTREACHED(), so I > > guess > > > > we have to decide how to deal with that. > > > > > > > > One idea I had was to change the signature of CanSetAsDefault() to return > a > > > > tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would require a > > lot > > > of > > > > minor code tweaks to get various other parts of the codebase to compile, > and > > > > bring in some unnecessary owners. > > > > > > > > It's possible we can do something like that as a subsequent changelist if > > you > > > > like the idea. > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > > enable_set_as_default_; > > > On 2013/09/09 19:22:57, grt wrote: > > > > On 2013/09/09 19:18:21, gab wrote: > > > > > I just talked about this with grt@; can we get the ok to make this > return > > > > true, > > > > > always, but explicitly hide all the UI bits for SxS? > > > > > > > > > > That way Canary will always register itself, it just won't be possible > to > > > make > > > > > it default through the UI (you'd have to go in Windows' Programs & > > > > > Features/Default Programs to give Chrome default -- which no one does by > > > > > accident). > > > > > > > > > > This would avoid code-churn to enable this. > > > > > > > > If that's not an option, them I'm okay with gab's suggestion to have > > > > BrowserDistribution::IsSetAsDefaultSupported() return CanSetAsDefault(); > > > > > > ref: > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > Ahh, right. Well if that makes everyone happy then I think that's reasonable. > > > If it makes it less confusing, we can change the name of the function from > > IsSetAsDefaultSupported. For example, something like > IsRegistrationSupported() > > or HasProgId() > > IsRegistrationSupported makes sense. This would only be used at uninstall to see > if the registration needs to be removed, right? I think so. At least, that's the only place it would be used now. It makes me wonder though, which is less confusing: 1) Have IsRegistrationSupported() return CanSetAsDefault() unless it's overridden. 2) Have CanSetAsDefault() return IsRegistrationSupported() unless it's overridden. The latter seems to make more sense to me. Thoughts?
On 2013/09/09 19:45:52, zturner wrote: > On 2013/09/09 19:40:06, grt wrote: > > On 2013/09/09 19:37:33, zturner wrote: > > > On 2013/09/09 19:34:16, gab wrote: > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > File chrome/installer/util/browser_distribution.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > chrome/installer/util/browser_distribution.cc:276: return true; > > > > On 2013/09/06 20:53:16, zturner wrote: > > > > > On 2013/09/06 18:40:08, gab wrote: > > > > > > How about having this return CanSetAsDefault() unless overriden to do > > > > > something > > > > > > else? Than all distributions for which this copies CanSetAsDefault > don't > > > > need > > > > > to > > > > > > also override this method. > > > > > > > > > > If we follow Greg's suggestion of deleting this method entirely and then > > > using > > > > > the return value of GetBrowserProgIdPrefix() to check, then that would > > also > > > > > solve the problem. I kind of agree that it's awkward to have to have > > > another > > > > > virtual function for this purpose. > > > > > > > > > > Another option is to keep this function, but make it a non-virtual base > > > class > > > > > helper function that is implemented by calling the virtual > > > > > GetBrowserProgIdPrefix() function and checking if the return value is > the > > > > empty > > > > > string. > > > > > > > > Meh, I prefer having two virtual functions with a default impl for one of > > them > > > > based on the other. > > > > > > > > > > > > > > That has the other issue you point out elsewhere though, where we rely > on > > > the > > > > > function returning an empty string instead of calling NOTREACHED(), so I > > > guess > > > > > we have to decide how to deal with that. > > > > > > > > > > One idea I had was to change the signature of CanSetAsDefault() to > return > > a > > > > > tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would require > a > > > lot > > > > of > > > > > minor code tweaks to get various other parts of the codebase to compile, > > and > > > > > bring in some unnecessary owners. > > > > > > > > > > It's possible we can do something like that as a subsequent changelist > if > > > you > > > > > like the idea. > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > > > enable_set_as_default_; > > > > On 2013/09/09 19:22:57, grt wrote: > > > > > On 2013/09/09 19:18:21, gab wrote: > > > > > > I just talked about this with grt@; can we get the ok to make this > > return > > > > > true, > > > > > > always, but explicitly hide all the UI bits for SxS? > > > > > > > > > > > > That way Canary will always register itself, it just won't be possible > > to > > > > make > > > > > > it default through the UI (you'd have to go in Windows' Programs & > > > > > > Features/Default Programs to give Chrome default -- which no one does > by > > > > > > accident). > > > > > > > > > > > > This would avoid code-churn to enable this. > > > > > > > > > > If that's not an option, them I'm okay with gab's suggestion to have > > > > > BrowserDistribution::IsSetAsDefaultSupported() return CanSetAsDefault(); > > > > > > > > ref: > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > > > Ahh, right. Well if that makes everyone happy then I think that's > reasonable. > > > > > If it makes it less confusing, we can change the name of the function from > > > IsSetAsDefaultSupported. For example, something like > > IsRegistrationSupported() > > > or HasProgId() > > > > IsRegistrationSupported makes sense. This would only be used at uninstall to > see > > if the registration needs to be removed, right? > > I think so. At least, that's the only place it would be used now. > > It makes me wonder though, which is less confusing: > > 1) Have IsRegistrationSupported() return CanSetAsDefault() unless it's > overridden. > 2) Have CanSetAsDefault() return IsRegistrationSupported() unless it's > overridden. > > The latter seems to make more sense to me. Thoughts? CanSetAsDefault() is already overridden by multiple distributions so I'd say it would be easier to go with the former to avoid having to change every other CanSetAsDefault(). Either way, sent a mail to laforge about allowing CanSetAsDefault() to true while removing all UI bits... that makes the most sense to me... let's pick this discussion back up only if we need to...
On 2013/09/09 19:50:06, gab wrote: > On 2013/09/09 19:45:52, zturner wrote: > > On 2013/09/09 19:40:06, grt wrote: > > > On 2013/09/09 19:37:33, zturner wrote: > > > > On 2013/09/09 19:34:16, gab wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > > File chrome/installer/util/browser_distribution.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > > chrome/installer/util/browser_distribution.cc:276: return true; > > > > > On 2013/09/06 20:53:16, zturner wrote: > > > > > > On 2013/09/06 18:40:08, gab wrote: > > > > > > > How about having this return CanSetAsDefault() unless overriden to > do > > > > > > something > > > > > > > else? Than all distributions for which this copies CanSetAsDefault > > don't > > > > > need > > > > > > to > > > > > > > also override this method. > > > > > > > > > > > > If we follow Greg's suggestion of deleting this method entirely and > then > > > > using > > > > > > the return value of GetBrowserProgIdPrefix() to check, then that would > > > also > > > > > > solve the problem. I kind of agree that it's awkward to have to have > > > > another > > > > > > virtual function for this purpose. > > > > > > > > > > > > Another option is to keep this function, but make it a non-virtual > base > > > > class > > > > > > helper function that is implemented by calling the virtual > > > > > > GetBrowserProgIdPrefix() function and checking if the return value is > > the > > > > > empty > > > > > > string. > > > > > > > > > > Meh, I prefer having two virtual functions with a default impl for one > of > > > them > > > > > based on the other. > > > > > > > > > > > > > > > > > That has the other issue you point out elsewhere though, where we rely > > on > > > > the > > > > > > function returning an empty string instead of calling NOTREACHED(), so > I > > > > guess > > > > > > we have to decide how to deal with that. > > > > > > > > > > > > One idea I had was to change the signature of CanSetAsDefault() to > > return > > > a > > > > > > tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would > require > > a > > > > lot > > > > > of > > > > > > minor code tweaks to get various other parts of the codebase to > compile, > > > and > > > > > > bring in some unnecessary owners. > > > > > > > > > > > > It's possible we can do something like that as a subsequent changelist > > if > > > > you > > > > > > like the idea. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > > > > enable_set_as_default_; > > > > > On 2013/09/09 19:22:57, grt wrote: > > > > > > On 2013/09/09 19:18:21, gab wrote: > > > > > > > I just talked about this with grt@; can we get the ok to make this > > > return > > > > > > true, > > > > > > > always, but explicitly hide all the UI bits for SxS? > > > > > > > > > > > > > > That way Canary will always register itself, it just won't be > possible > > > to > > > > > make > > > > > > > it default through the UI (you'd have to go in Windows' Programs & > > > > > > > Features/Default Programs to give Chrome default -- which no one > does > > by > > > > > > > accident). > > > > > > > > > > > > > > This would avoid code-churn to enable this. > > > > > > > > > > > > If that's not an option, them I'm okay with gab's suggestion to have > > > > > > BrowserDistribution::IsSetAsDefaultSupported() return > CanSetAsDefault(); > > > > > > > > > > ref: > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > > > > > Ahh, right. Well if that makes everyone happy then I think that's > > reasonable. > > > > > > > If it makes it less confusing, we can change the name of the function from > > > > IsSetAsDefaultSupported. For example, something like > > > IsRegistrationSupported() > > > > or HasProgId() > > > > > > IsRegistrationSupported makes sense. This would only be used at uninstall to > > see > > > if the registration needs to be removed, right? > > > > I think so. At least, that's the only place it would be used now. > > > > It makes me wonder though, which is less confusing: > > > > 1) Have IsRegistrationSupported() return CanSetAsDefault() unless it's > > overridden. > > 2) Have CanSetAsDefault() return IsRegistrationSupported() unless it's > > overridden. > > > > The latter seems to make more sense to me. Thoughts? > > CanSetAsDefault() is already overridden by multiple distributions so I'd say it > would be easier to go with the former to avoid having to change every other > CanSetAsDefault(). > > Either way, sent a mail to laforge about allowing CanSetAsDefault() to true > while removing all UI bits... that makes the most sense to me... let's pick this > discussion back up only if we need to... Was kind of surprised by his response so I double checked with him to make sure. He said it's fine as long as the browser does not show a pop up asking if they want to set Canary as default when they start the browser for the first time. I think it might actually do this though, can you confirm or deny? (I can also make the change and just test it, but it will take a little bit.)
On 2013/09/09 19:54:39, zturner wrote: > On 2013/09/09 19:50:06, gab wrote: > > On 2013/09/09 19:45:52, zturner wrote: > > > On 2013/09/09 19:40:06, grt wrote: > > > > On 2013/09/09 19:37:33, zturner wrote: > > > > > On 2013/09/09 19:34:16, gab wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > > > File chrome/installer/util/browser_distribution.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > > > chrome/installer/util/browser_distribution.cc:276: return true; > > > > > > On 2013/09/06 20:53:16, zturner wrote: > > > > > > > On 2013/09/06 18:40:08, gab wrote: > > > > > > > > How about having this return CanSetAsDefault() unless overriden to > > do > > > > > > > something > > > > > > > > else? Than all distributions for which this copies CanSetAsDefault > > > don't > > > > > > need > > > > > > > to > > > > > > > > also override this method. > > > > > > > > > > > > > > If we follow Greg's suggestion of deleting this method entirely and > > then > > > > > using > > > > > > > the return value of GetBrowserProgIdPrefix() to check, then that > would > > > > also > > > > > > > solve the problem. I kind of agree that it's awkward to have to > have > > > > > another > > > > > > > virtual function for this purpose. > > > > > > > > > > > > > > Another option is to keep this function, but make it a non-virtual > > base > > > > > class > > > > > > > helper function that is implemented by calling the virtual > > > > > > > GetBrowserProgIdPrefix() function and checking if the return value > is > > > the > > > > > > empty > > > > > > > string. > > > > > > > > > > > > Meh, I prefer having two virtual functions with a default impl for one > > of > > > > them > > > > > > based on the other. > > > > > > > > > > > > > > > > > > > > That has the other issue you point out elsewhere though, where we > rely > > > on > > > > > the > > > > > > > function returning an empty string instead of calling NOTREACHED(), > so > > I > > > > > guess > > > > > > > we have to decide how to deal with that. > > > > > > > > > > > > > > One idea I had was to change the signature of CanSetAsDefault() to > > > return > > > > a > > > > > > > tri-state enum {UNSUPPORTED, DISABLED, ENABLED}, but this would > > require > > > a > > > > > lot > > > > > > of > > > > > > > minor code tweaks to get various other parts of the codebase to > > compile, > > > > and > > > > > > > bring in some unnecessary owners. > > > > > > > > > > > > > > It's possible we can do something like that as a subsequent > changelist > > > if > > > > > you > > > > > > > like the idea. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > > > File chrome/installer/util/google_chrome_sxs_distribution.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/go... > > > > > > chrome/installer/util/google_chrome_sxs_distribution.cc:80: return > > > > > > enable_set_as_default_; > > > > > > On 2013/09/09 19:22:57, grt wrote: > > > > > > > On 2013/09/09 19:18:21, gab wrote: > > > > > > > > I just talked about this with grt@; can we get the ok to make this > > > > return > > > > > > > true, > > > > > > > > always, but explicitly hide all the UI bits for SxS? > > > > > > > > > > > > > > > > That way Canary will always register itself, it just won't be > > possible > > > > to > > > > > > make > > > > > > > > it default through the UI (you'd have to go in Windows' Programs & > > > > > > > > Features/Default Programs to give Chrome default -- which no one > > does > > > by > > > > > > > > accident). > > > > > > > > > > > > > > > > This would avoid code-churn to enable this. > > > > > > > > > > > > > > If that's not an option, them I'm okay with gab's suggestion to have > > > > > > > BrowserDistribution::IsSetAsDefaultSupported() return > > CanSetAsDefault(); > > > > > > > > > > > > ref: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23258005/diff/63029/chrome/installer/util/bro... > > > > > > > > > > Ahh, right. Well if that makes everyone happy then I think that's > > > reasonable. > > > > > > > > > If it makes it less confusing, we can change the name of the function > from > > > > > IsSetAsDefaultSupported. For example, something like > > > > IsRegistrationSupported() > > > > > or HasProgId() > > > > > > > > IsRegistrationSupported makes sense. This would only be used at uninstall > to > > > see > > > > if the registration needs to be removed, right? > > > > > > I think so. At least, that's the only place it would be used now. > > > > > > It makes me wonder though, which is less confusing: > > > > > > 1) Have IsRegistrationSupported() return CanSetAsDefault() unless it's > > > overridden. > > > 2) Have CanSetAsDefault() return IsRegistrationSupported() unless it's > > > overridden. > > > > > > The latter seems to make more sense to me. Thoughts? > > > > CanSetAsDefault() is already overridden by multiple distributions so I'd say > it > > would be easier to go with the former to avoid having to change every other > > CanSetAsDefault(). > > > > Either way, sent a mail to laforge about allowing CanSetAsDefault() to true > > while removing all UI bits... that makes the most sense to me... let's pick > this > > discussion back up only if we need to... > > Was kind of surprised by his response so I double checked with him to make sure. > He said > it's fine as long as the browser does not show a pop up asking if they want to > set Canary > as default when they start the browser for the first time. I think it might > actually do > this though, can you confirm or deny? (I can also make the change and just test > it, but it will take > a little bit.) There will be no popups (as long as we specifically disable the relevant UI bits for IsChromeSxSProcess()). The only one we may get is the Windows 8 slide-in: "You have new apps that can open webpages." If we are really worried about that we can probably trick the registration code to have that not happen for SxS... (but I'm not worried about that; we in fact added the first run dialog on Win8 specifically because we thought users wouldn't notice the slide-in... and that it goes away too fast...). Cheers! Gab
Changed the title of the issue to be more descriptive. Cleaned up the API a bit with this patch, and removed some of the functionality that is more appropriate for a follow-up CL.
lg, comments below. https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/ch... File chrome/installer/util/chromium_binaries_distribution.cc (right): https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/ch... chrome/installer/util/chromium_binaries_distribution.cc:33: return string16(); + NOTREACHED() https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/ch... chrome/installer/util/chromium_binaries_distribution.cc:35: Also override GetBrowserProgIdDesc() here and in other places where you overrode GetBrowserProgIdPrefix(). https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:741: browser_entry_suffix); nit: Align 4 spaces in from above '(' https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:815: browser_entry_suffix); nit: Align 4 spaces in from above '(' https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:371: typedef std::vector<base::FilePath::StringType> ComponentsType; I don't feel this typedef is necessary; it adds complexity only to maybe save a single line wrap below (and it requires a line of itself to declare...). https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:376: auto next = iter+1; I still think you can't use "auto" in Chromium (ref C++11 discussion linked to in earlier CR). I would name this variable something like "parent" instead and use a const& instead. i.e. const base::FilePath::StringType& parent = *(iter + 1); https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:381: if (next != components.rend() && Use "&&" to continue previous condition instead of having an if inside an if. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:386: iter = next; Feels like a for loop to me; e.g.,: for (ComponentsType::const_reverse_iterator iter = components.rbegin(); iter != components.rend(); ++iter) { ... } https://codereview.chromium.org/23258005/diff/154001/win8/delegate_execute/ch... File win8/delegate_execute/chrome_util.cc (right): https://codereview.chromium.org/23258005/diff/154001/win8/delegate_execute/ch... win8/delegate_execute/chrome_util.cc:85: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); Did you make sure that GetDistribution() returns the proper distribution from delegate_execute.exe (I'm guessing yes, but just making sure!)? https://codereview.chromium.org/23258005/diff/154001/win8/delegate_execute/ch... win8/delegate_execute/chrome_util.cc:108: string16 ByteArrayToBase32(const uint8* bytes, size_t size) { Can remove this method now. https://codereview.chromium.org/23258005/diff/154001/win8/delegate_execute/ch... win8/delegate_execute/chrome_util.cc:160: bool GetUserSpecificRegistrySuffix(string16* suffix) { Can remove this method now. https://codereview.chromium.org/23258005/diff/154001/win8/delegate_execute/ch... win8/delegate_execute/chrome_util.cc:200: if (IsPerUserInstall(chrome_exe)) { This can now use InstallUtil::IsPerUserInstall(). And you can subsequently remove IsPerUserInstall() in this file. https://codereview.chromium.org/23258005/diff/154001/win8/delegate_execute/de... File win8/delegate_execute/delegate_execute.cc (right): https://codereview.chromium.org/23258005/diff/154001/win8/delegate_execute/de... win8/delegate_execute/delegate_execute.cc:55: hr = ::CLSIDFromString(clsid_string.c_str(), &clsid); Nice, I much prefer that to the two methods in BrowserDistribution :)!
https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/br... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/br... chrome/installer/util/browser_distribution.h:97: // the return value of this function should have a maximum length of should or must? https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... File chrome/installer/util/chrome_app_host_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... chrome/installer/util/chrome_app_host_distribution.h:21: shouldn't GetBrowserProgIdDesc should NOTREACHED, too? https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... File chrome/installer/util/chrome_frame_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... chrome/installer/util/chrome_frame_distribution.h:19: same comment about NOTREACHED for GetBrowserProgIdDesc here https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... File chrome/installer/util/chromium_binaries_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... chrome/installer/util/chromium_binaries_distribution.h:19: GetBrowserProgIdDesc NOTREACHED here, too?
https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/ch... File chrome/installer/util/chromium_binaries_distribution.cc (right): https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/ch... chrome/installer/util/chromium_binaries_distribution.cc:33: return string16(); On 2013/09/10 19:08:56, gab wrote: > + NOTREACHED() Done. https://codereview.chromium.org/23258005/diff/138001/chrome/installer/util/ch... chrome/installer/util/chromium_binaries_distribution.cc:35: On 2013/09/10 19:08:56, gab wrote: > Also override GetBrowserProgIdDesc() here and in other places where you overrode > GetBrowserProgIdPrefix(). Done. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:371: typedef std::vector<base::FilePath::StringType> ComponentsType; On 2013/09/10 19:08:57, gab wrote: > I don't feel this typedef is necessary; it adds complexity only to maybe save a > single line wrap below (and it requires a line of itself to declare...). Well, it's just that std::vector<base::FilePath::StringType>::const_reverse_iterator is almost an entire line just by itself, and it has to be used in at least two places. Seems like using the typedef is in the same spirit of the Google Style Guide's recommendation to use auto when long type names are involved, but in a way that doesn't require C++11. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:376: auto next = iter+1; On 2013/09/10 19:08:57, gab wrote: > I still think you can't use "auto" in Chromium (ref C++11 discussion linked to > in earlier CR). You're right, I just missed this one. > > I would name this variable something like "parent" instead and use a const& > instead. > > i.e. > const base::FilePath::StringType& parent = *(iter + 1); Won't work exactly like that because I need to check iter+1 against rend() before I can de-reference it, so the best way is to make another iterator on the stack and increment it similar to how I was doing with the auto. That was the main reason for using the typedef. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:386: iter = next; On 2013/09/10 19:08:57, gab wrote: > Feels like a for loop to me; > > e.g.,: > > for (ComponentsType::const_reverse_iterator iter = components.rbegin(); > iter != components.rend(); ++iter) { > ... > } Yea, but then if I don't use the typedef it looks pretty unpleasant, at least to me.
https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:371: typedef std::vector<base::FilePath::StringType> ComponentsType; On 2013/09/11 20:19:53, zturner wrote: > On 2013/09/10 19:08:57, gab wrote: > > I don't feel this typedef is necessary; it adds complexity only to maybe save > a > > single line wrap below (and it requires a line of itself to declare...). > > Well, it's just that > > std::vector<base::FilePath::StringType>::const_reverse_iterator > > is almost an entire line just by itself, and it has to be used in at least two > places. Seems like using the typedef is in the same spirit of the Google Style > Guide's recommendation to use auto when long type names are involved, but in a > way that doesn't require C++11. I like the typedef, although I don't like "Type" in the name of types. I'd like it better if it was Strings or StringVector, since the "components" are strings. FWIW, this has been discussed on chromium-dev: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/qSrl_UAqAkQ/dis....
https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/in... chrome/installer/util/install_util.cc:371: typedef std::vector<base::FilePath::StringType> ComponentsType; On 2013/09/11 20:19:53, zturner wrote: > On 2013/09/10 19:08:57, gab wrote: > > I don't feel this typedef is necessary; it adds complexity only to maybe save > a > > single line wrap below (and it requires a line of itself to declare...). > > Well, it's just that > > std::vector<base::FilePath::StringType>::const_reverse_iterator > > is almost an entire line just by itself, and it has to be used in at least two > places. Seems like using the typedef is in the same spirit of the Google Style > Guide's recommendation to use auto when long type names are involved, but in a > way that doesn't require C++11. Well, one of the 2 places is the line just below this one where it will fit (I think). Actually, looking at this some more, I agree that the ifdef could be nice for the iterator and used as such: typedef std::vector<base::FilePath::StringType>::const_reverse_iterator ComponentsIterator; if (components.size() < 2) return false; for (ComponentsIterator current = components.rbegin(), ComponentsIterator parent = current + 1; parent != components.rend(); current = parent++) { ... }
https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:741: browser_entry_suffix); On 2013/09/10 19:08:57, gab wrote: > nit: Align 4 spaces in from above '(' Thought this looked awkward, instead I put the entire expression on the following line. Let me know if there's a problem with this. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:815: browser_entry_suffix); On 2013/09/10 19:08:57, gab wrote: > nit: Align 4 spaces in from above '(' Same https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/br... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/br... chrome/installer/util/browser_distribution.h:97: // the return value of this function should have a maximum length of On 2013/09/11 03:52:32, grt wrote: > should or must? Done. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... File chrome/installer/util/chrome_app_host_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... chrome/installer/util/chrome_app_host_distribution.h:21: On 2013/09/11 03:52:32, grt wrote: > shouldn't GetBrowserProgIdDesc should NOTREACHED, too? Done. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... File chrome/installer/util/chrome_frame_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... chrome/installer/util/chrome_frame_distribution.h:19: On 2013/09/11 03:52:32, grt wrote: > same comment about NOTREACHED for GetBrowserProgIdDesc here Done. https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... File chrome/installer/util/chromium_binaries_distribution.h (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/util/ch... chrome/installer/util/chromium_binaries_distribution.h:19: On 2013/09/11 03:52:32, grt wrote: > GetBrowserProgIdDesc NOTREACHED here, too? Done.
Thanks, few last comments. Cheers! Gab https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:741: browser_entry_suffix); On 2013/09/11 21:16:59, zturner wrote: > On 2013/09/10 19:08:57, gab wrote: > > nit: Align 4 spaces in from above '(' > > Thought this looked awkward, instead I put the entire expression on the > following line. Let me know if there's a problem with this. Yep, prefer this too. https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... chrome/installer/util/install_util.cc:371: typedef std::vector<base::FilePath::StringType>::const_reverse_iterator I would put the typedef right before the for loop since it's only used for it. https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... chrome/installer/util/install_util.cc:375: for (ComponentsIterator current = components.rbegin(), parent = current+1; Need to check: if (components.size() < 2) return false; before doing this or current+1 might not exist if current == rend(). Technically checking !components.empty() is sufficient to ensure that, but if checking size, might as well check that it's not < 2 in which case we can't succeed anyways... https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... chrome/installer/util/install_util.cc:375: for (ComponentsIterator current = components.rbegin(), parent = current+1; Spaces on each side of '+' sign. https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... chrome/installer/util/install_util.cc:379: if (base::FilePath::CompareEqualIgnoreCase(*parent, chrome_sxs_dir)) Use && instead of nesting ifs.
lgtm on my side with nit below. https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... chrome/installer/util/install_util.cc:377: if (base::FilePath::CompareEqualIgnoreCase(*current, move *current to next line as was done in uninstall.cc (indented four spaces from the 'b' in "base::FilePath")?
On 2013/09/11 21:26:28, gab wrote: > Thanks, few last comments. > > Cheers! > Gab > > https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... > File chrome/installer/setup/uninstall.cc (right): > > https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... > chrome/installer/setup/uninstall.cc:741: browser_entry_suffix); > On 2013/09/11 21:16:59, zturner wrote: > > On 2013/09/10 19:08:57, gab wrote: > > > nit: Align 4 spaces in from above '(' > > > > Thought this looked awkward, instead I put the entire expression on the > > following line. Let me know if there's a problem with this. > > Yep, prefer this too. > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > File chrome/installer/util/install_util.cc (right): > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > chrome/installer/util/install_util.cc:371: typedef > std::vector<base::FilePath::StringType>::const_reverse_iterator > I would put the typedef right before the for loop since it's only used for it. > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > chrome/installer/util/install_util.cc:375: for (ComponentsIterator current = > components.rbegin(), parent = current+1; > Need to check: > > if (components.size() < 2) > return false; > > before doing this or current+1 might not exist if current == rend(). > > Technically checking !components.empty() is sufficient to ensure that, but if > checking size, might as well check that it's not < 2 in which case we can't > succeed anyways... I actually had a DCHECK(!components.empty()) but took it out because it's guaranteed to not be empty (we append kInstallerLnkExt to the path earlier).
On 2013/09/11 21:43:00, zturner wrote: > On 2013/09/11 21:26:28, gab wrote: > > Thanks, few last comments. > > > > Cheers! > > Gab > > > > > https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... > > File chrome/installer/setup/uninstall.cc (right): > > > > > https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... > > chrome/installer/setup/uninstall.cc:741: browser_entry_suffix); > > On 2013/09/11 21:16:59, zturner wrote: > > > On 2013/09/10 19:08:57, gab wrote: > > > > nit: Align 4 spaces in from above '(' > > > > > > Thought this looked awkward, instead I put the entire expression on the > > > following line. Let me know if there's a problem with this. > > > > Yep, prefer this too. > > > > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > > File chrome/installer/util/install_util.cc (right): > > > > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > > chrome/installer/util/install_util.cc:371: typedef > > std::vector<base::FilePath::StringType>::const_reverse_iterator > > I would put the typedef right before the for loop since it's only used for it. > > > > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > > chrome/installer/util/install_util.cc:375: for (ComponentsIterator current = > > components.rbegin(), parent = current+1; > > Need to check: > > > > if (components.size() < 2) > > return false; > > > > before doing this or current+1 might not exist if current == rend(). > > > > Technically checking !components.empty() is sufficient to ensure that, but if > > checking size, might as well check that it's not < 2 in which case we can't > > succeed anyways... > I actually had a DCHECK(!components.empty()) but took it out because it's > guaranteed to not be empty (we append kInstallerLnkExt to the path earlier). Actually my bad, we append kInstallerLnkExt to a different string. In any case, is a DCHECK() ok here as opposed to a runtime check? I can't imagine a scenario where something isn't horribly wrong if exe_dir contains 0 components.
On 2013/09/11 21:46:27, zturner wrote: > On 2013/09/11 21:43:00, zturner wrote: > > On 2013/09/11 21:26:28, gab wrote: > > > Thanks, few last comments. > > > > > > Cheers! > > > Gab > > > > > > > > > https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... > > > File chrome/installer/setup/uninstall.cc (right): > > > > > > > > > https://codereview.chromium.org/23258005/diff/154001/chrome/installer/setup/u... > > > chrome/installer/setup/uninstall.cc:741: browser_entry_suffix); > > > On 2013/09/11 21:16:59, zturner wrote: > > > > On 2013/09/10 19:08:57, gab wrote: > > > > > nit: Align 4 spaces in from above '(' > > > > > > > > Thought this looked awkward, instead I put the entire expression on the > > > > following line. Let me know if there's a problem with this. > > > > > > Yep, prefer this too. > > > > > > > > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > > > File chrome/installer/util/install_util.cc (right): > > > > > > > > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > > > chrome/installer/util/install_util.cc:371: typedef > > > std::vector<base::FilePath::StringType>::const_reverse_iterator > > > I would put the typedef right before the for loop since it's only used for > it. > > > > > > > > > https://codereview.chromium.org/23258005/diff/148001/chrome/installer/util/in... > > > chrome/installer/util/install_util.cc:375: for (ComponentsIterator current = > > > components.rbegin(), parent = current+1; > > > Need to check: > > > > > > if (components.size() < 2) > > > return false; > > > > > > before doing this or current+1 might not exist if current == rend(). > > > > > > Technically checking !components.empty() is sufficient to ensure that, but > if > > > checking size, might as well check that it's not < 2 in which case we can't > > > succeed anyways... > > I actually had a DCHECK(!components.empty()) but took it out because it's > > guaranteed to not be empty (we append kInstallerLnkExt to the path earlier). > > Actually my bad, we append kInstallerLnkExt to a different string. In any case, > is a DCHECK() ok here as opposed to a runtime check? I can't imagine a scenario > where something isn't horribly wrong if exe_dir contains 0 components. I'm fine with DCHECK + comment.
I sent an email out with the last patch, but I think the email didn't go through. Sending out again.
https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... chrome/installer/util/install_util.cc:375: // path to our executable. this is splitting the dir containing the executable. would this not be empty if the executable happened to be at the root of a volume? we don't install chrome that way, but it would be a shame for chrome to crash if it were run like that. https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... chrome/installer/util/install_util.cc:383: base::FilePath::CompareEqualIgnoreCase(*parent, chrome_sxs_dir)) nit: use braces for multi-line conditional.
On 2013/09/12 14:04:52, grt wrote: > https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... > File chrome/installer/util/install_util.cc (right): > > https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... > chrome/installer/util/install_util.cc:375: // path to our executable. > this is splitting the dir containing the executable. would this not be empty if > the executable happened to be at the root of a volume? we don't install chrome > that way, but it would be a shame for chrome to crash if it were run like that. It would still have the drive letter as one component. (I can triple check this later, but I'm 99% sure that's the behavior I observed the other day)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/23258005/187001
lgtm w/ comments below. Cheers! Gab https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... chrome/installer/util/install_util.cc:375: // path to our executable. On 2013/09/12 14:04:53, grt wrote: > this is splitting the dir containing the executable. would this not be empty if > the executable happened to be at the root of a volume? we don't install chrome > that way, but it would be a shame for chrome to crash if it were run like that. zturner: > It would still have the drive letter as one component. (I can triple check this later, but I'm 99% sure that's the behavior I observed the other day) I would say then that it's clearer to mention that, the current comment is unclear imo; i.e., I'd replace: "since we're splitting the path to our executable." with "since the path will at least contain a component for the drive letter." https://codereview.chromium.org/23258005/diff/187001/chrome/installer/util/in... chrome/installer/util/install_util.cc:383: base::FilePath::CompareEqualIgnoreCase(*parent, chrome_sxs_dir)) On 2013/09/12 14:04:53, grt wrote: > nit: use braces for multi-line conditional. ping on this.
Message was sent while issue was closed.
Change committed as 222987
Message was sent while issue was closed.
On 2013/09/13 06:54:30, I haz the power (commit-bot) wrote: > Change committed as 222987 Please address remaining nits in a follow-up CL and TBR me. Thanks, Gab |
