Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11359013:
Adding App Launcher shortcuts on install. (Closed)
|
Created:
8 years, 1 month ago by huangs Modified:
8 years, 1 month ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding App Launcher shortcuts on install.
This is a direct continuation of https://codereview.chromium.org/11267023/
BUG=151626
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166608
Patch Set 1 : #
Total comments: 62
Patch Set 2 : Cleanups; adding DCHECK to ensure App Launcher-releated stuff are for user-level. #
Total comments: 22
Patch Set 3 : Reshuffling code and comments; setting app id for App Launcher. #Patch Set 4 : Merge with refactor_chrome_shortcut_op. #
Total comments: 32
Patch Set 5 : Moving App Launcher shortcut properties initialization to ChromeAppHostOperations. #Patch Set 6 : Fixing comments; minor rearrangements. #
Total comments: 6
Patch Set 7 : Nits; appending App Host app_id with suffix from ShellUtil::GetUserSpecificRegistrySuffix(). #
Total comments: 2
Patch Set 8 : Nit. #
Messages
Total messages: 31 (0 generated)
The basic functionality has been implemented. Please talk a look.
https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:430: app_host_args.AppendSwitch(::switches::kShowAppList); Note the insertion of 2 spaces!
Looks pretty good. Some minor questions/requests. http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:415: // Shortcuts are always installed per-user unless specified. any reason for this moving from above? http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:426: // Should consult AppListController::GetAppListCommandLine(). Can you provide some context to this comment? http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:428: // This is cleaner than appending strings, but as a result we'll get An extra space in the command line does not merit a comment. http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:307: BrowserDistribution* dist = product.distribution(); For a cleaner diff, move the declaration/assignment of 'dist' to its original location. http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:308: DeleteShortcutsCommon(install_level, dist, chrome_exe); Leave a log statement in DeleteChromeShortcuts like "Deleting Chrome shortcuts" so that the generic log statements from DeleteShortcutsCommon can be attributed to Chrome vs. Launcher. OR put a log statement in DeleteShortcutsCommon saying "Deleting shortcuts to " << target http://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chrom... File chrome/installer/util/chrome_app_host_distribution.cc (right): http://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chrom... chrome/installer/util/chrome_app_host_distribution.cc:53: return L"ChromeAppList"; Is a TODO required to use the correct App Model ID incorporating the user-data-dir, right?
high-level comment: this CL puts a buch of functionality intended for Chrome (e.g., ChromeShortcutOperation, ChromeShortcutProperties, CreateOrUpdateChromeShortcut) to use for something that isn't Chrome. at the very least, this makes the code somewhat misleading to read. if gab agrees with the overall approach, i suggest re-thinking the names of these types/methods so that they're no longer specific to Chrome-the-browser. http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:424: if (product.is_chrome_app_host()) { consider if it makes sense to add to Product and ProductOperation so conditionals like this aren't needed.
There was some thinking about not changing names to minimize merge problems if Gab's refactoring needs M24 repairs. But truth be told I don't know that it should really be an issue. I have looked through the definitions of those functions and, in general, they are pretty spot-on for what we need. So changing the names to reflect their now more generalized purpose seems right to me. On Wed, Oct 31, 2012 at 9:50 PM, <grt@chromium.org> wrote: > high-level comment: this CL puts a buch of functionality intended for > Chrome > (e.g., ChromeShortcutOperation, ChromeShortcutProperties, > CreateOrUpdateChromeShortcut) to use for something that isn't Chrome. at > the > very least, this makes the code somewhat misleading to read. if gab > agrees with > the overall approach, i suggest re-thinking the names of these > types/methods so > that they're no longer specific to Chrome-the-browser. > > > > http://codereview.chromium.**org/11359013/diff/2001/chrome/** > installer/setup/install.cc<http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/install.cc> > File chrome/installer/setup/**install.cc (right): > > http://codereview.chromium.**org/11359013/diff/2001/chrome/** > installer/setup/install.cc#**newcode424<http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/install.cc#newcode424> > chrome/installer/setup/**install.cc:424: if (product.is_chrome_app_host()) > { > consider if it makes sense to add to Product and ProductOperation so > conditionals like this aren't needed. > > http://codereview.chromium.**org/11359013/<http://codereview.chromium.org/113... >
I agree that everything that is no longer Chrome specific should be renamed and I'm fine with this going in now (I just want to get my two M24 fixes in first, but those will likely go in tomorrow anyways). The renames and refactoring required to make ShellUtil::ChromeShortcutProperties generic should happen in a separate CL first imo (in part because it would be great to get QA on it without any expected change in behavior) given we would be changing assumptions all over the place about these methods being for Chrome only. (I know this kind of contradicts what I said about committing changes in sync with their motivating factors on the previous CL, but in this case I expect the refactoring to be bigger than a simple function move and it would be nice to flush this out to get a clean actual CL after, you can even have both reviewed in parallel, referring to each other in their description). More comments on current CL below. Cheers, Gab https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:289: // based on state of installation and master preference. s/master preference/master_preferences https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:297: CommandLine(original_app_host_state->uninstall_command()). nit: '.' on the next line to make it more obvious it's continued from the line above. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:298: HasSwitch(installer::switches::kChromeAppLauncher); I feel like checking the presence of a flag on the previous uninstall string might be a bit risky, is there a better way to determine the current state of the app_launcher install? Also, I am not super-familiar with original_state; would this still work if registration of the uninstall string happened on the system before the shortcuts are installed? (don't have access to the code right now to double-check... i.e. order of registration/shortcut creation should be independent). https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:422: base_properties.set_chrome_exe(target); The reason the ShellUtil::ChromeShortcutProperties were made chrome only and not general (they were at first) is that it simplified giving defaults to non-set properties (see ShellUtil::GetShortcutPropertiesFromChromeShortcutProperties() or something like that). This will not work as the values you are not setting will default to Chrome's (i.e. app_id, icon, etc.). What you probably want to do is as we had discussed and you had suggested: make the mapping method in ShellUtil take a set of default properties and then use those to set the non-set properties, then you can call that method with different default properties based on the situation down in ShellUtil. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:431: base_properties.set_arguments(app_host_args.GetCommandLineString()); Things like this that are meant as a property for all of your shortcuts will probably end up in the set of default properties discussed in the comment above (unless this doesn't make sense in all app_launcher cases...?). https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:564: installer_state.UpdateStage(installer::CREATING_SHORTCUTS); Should we introduce a new stage constant? I think each constant is only mapped to 1 section of the code thus far, but I could be wrong.. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:262: // delete both. Finally, we delete the quick lauch shortcut. This is overly wordy, details about why we do which shortcut can go in the implementation if you feel it's necessary (it's actually already documented below). I feel something like this is sufficient: Deletes shortcuts at |install_level| from Start menu, Desktop, and Quick Launch. Only shortcuts pointing to |target| will be removed. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:289: // This method deletes Chrome shortcut folder from Windows Start menu. It Can you also make this cleaner while you're at it :)? // Deletes Chrome shortcuts as per DeleteShortcutsCommon() and removes // secondary tiles on the Start Screen (Win8+). https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:310: VLOG(1) << "Deleting Start Menu shortcuts."; This wasn't moved to the Common method above as the comment on it says... https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:320: // This method deletes App Host shortcuts. // Deletes App Host shortcuts. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:330: // has already been deleted in chrome_browser_main_win.cc::DoUninstallTasks(). This comment doesn't apply to the app_host (it was copied from the method above). https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:331: ShellUtil::ShellChange install_level = installer_state.system_install() ? Aren't your shortcuts always user-level? At least this is what install.cc does. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1081: if (is_app_host) { |is_app_host| can be inlined here, no need for a variable at the top imo. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1083: Append(installer::kChromeAppHostExe).value()); Bring '.' down to this line and indent 4 more spaces (probably doesn't fit) Or bring the whole thing down to this line, indented 4 spaces (I prefer that). https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1084: // First delete shortcuts from Start->Programs, Desktop & Quick Launch. -First (this is the only thing you do...) s/Start->Programs/Start menu (this is how it's called everywhere else). s/&/, and https://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:53: return L"ChromeAppList"; On 2012/11/01 01:21:50, erikwright wrote: > Is a TODO required to use the correct App Model ID incorporating the > user-data-dir, right? This is the "base AppId", i.e. the one to begin all the appids generated by other methods. See methods in shell_util.cc; this is correct (compare with google_chrome_distribution.cc. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:132: return true; I don't think this has any impact anymore (i.e. fine to change it but we should add to the installer fixit (https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH... to either make those have an impact again if they should or remove them (I feel the latter is correct now))
PTAL. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:289: // based on state of installation and master preference. On 2012/11/01 04:46:19, gab wrote: > s/master preference/master_preferences Done. Also s/master preference/master_preferences/g https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:297: CommandLine(original_app_host_state->uninstall_command()). On 2012/11/01 04:46:19, gab wrote: > nit: '.' on the next line to make it more obvious it's continued from the line > above. Done. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:298: HasSwitch(installer::switches::kChromeAppLauncher); I followed grt@'s recommendation regarding using uninstall string. As for original_state, this is passed down from wWinMain() before install occurred. The intent IS to check system status before the current install.exe run. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:415: // Shortcuts are always installed per-user unless specified. On 2012/11/01 01:21:50, erikwright wrote: > any reason for this moving from above? shortcut_level exists solely to initialize base_properties. I find it bizarre that the switch block lies between. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:422: base_properties.set_chrome_exe(target); I went through the flow, and found app_id is assigned by: GetShortcutPropertiesFromChromeShortcutProperties() => ShellUtil::GetBrowserModelId() => ChromeAppHostDistribution::GetBaseAppId() to retrieve L"ChromeAppList". Then it adds some user-specific registry suffix stuff, WHICH I ASSUMED is okay for both Chrome and App Host. Is this okay? https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:424: if (product.is_chrome_app_host()) { Eventually App Host and App Launcher will be the same thing. In https://codereview.chromium.org/11267023/ , the decision is to minimize divergence between App Host and App Launcher. For the case of Product, I thought is_chrome_app_launcher() would be redundant. This DCHECK can be removed once we have unification, but keeping it is also harmless. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:426: // Should consult AppListController::GetAppListCommandLine(). In AppListController::GetAppListCommandLine(), a similar command line is formed, but more stuff is added. Updating comment. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:428: // This is cleaner than appending strings, but as a result we'll get The extra space is really annoying. Can I add an extra static routine in CommandLine to e.g. render "--xxx" from "xxx", and then use it? https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:431: base_properties.set_arguments(app_host_args.GetCommandLineString()); The command line to run the App Launcher is app_host.exe --show-app-list I should get benwells@ to confirm. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:564: installer_state.UpdateStage(installer::CREATING_SHORTCUTS); Yeah, it's weird to have UpdateState((installer::CREATING_SHORTCUTS) to appear twice. Rearranging the logic to uniquify this. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:262: // delete both. Finally, we delete the quick lauch shortcut. On 2012/11/01 04:46:19, gab wrote: > This is overly wordy, details about why we do which shortcut can go in the > implementation if you feel it's necessary (it's actually already documented > below). > > I feel something like this is sufficient: > Deletes shortcuts at |install_level| from Start menu, Desktop, and Quick Launch. > Only shortcuts pointing to |target| will be removed. Done. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:289: // This method deletes Chrome shortcut folder from Windows Start menu. It On 2012/11/01 04:46:19, gab wrote: > Can you also make this cleaner while you're at it :)? > > // Deletes Chrome shortcuts as per DeleteShortcutsCommon() and removes > // secondary tiles on the Start Screen (Win8+). Done. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:307: BrowserDistribution* dist = product.distribution(); On 2012/11/01 01:21:50, erikwright wrote: > For a cleaner diff, move the declaration/assignment of 'dist' to its original > location. Done. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:308: DeleteShortcutsCommon(install_level, dist, chrome_exe); Using method #1, in case there are more product-specific stuff. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:310: VLOG(1) << "Deleting Start Menu shortcuts."; Moving all these into common, along with ShellUtil::RemoveChromeStartScreenShortcuts(). Also changed comments. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:320: // This method deletes App Host shortcuts. On 2012/11/01 04:46:19, gab wrote: > // Deletes App Host shortcuts. Done (same style as DeleteChromeShortcuts()). https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:330: // has already been deleted in chrome_browser_main_win.cc::DoUninstallTasks(). On 2012/11/01 04:46:19, gab wrote: > This comment doesn't apply to the app_host (it was copied from the method > above). Oops. Deleted! https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:331: ShellUtil::ShellChange install_level = installer_state.system_install() ? On 2012/11/01 04:46:19, gab wrote: > Aren't your shortcuts always user-level? At least this is what install.cc does. For now it is, but we want to future-proof. If setup.exe is run with --system-level with app-host or app-launcher, then failure should occur earlier on. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1081: if (is_app_host) { Done. The motivation was code symmetry, since is_chrome is also defined. But I guess this doesn't impact readability. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1083: Append(installer::kChromeAppHostExe).value()); The whole thing won't fit. Trying with keeping installer_state upstairs. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1084: // First delete shortcuts from Start->Programs, Desktop & Quick Launch. On 2012/11/01 04:46:19, gab wrote: > -First (this is the only thing you do...) > > s/Start->Programs/Start menu (this is how it's called everywhere else). > s/&/, and Done, also changed the comment that I copied this from. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:53: return L"ChromeAppList"; Expanded comment, and defined constant for this. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:132: return true; Will edit once I have access. Planning to add: - BrowserDistribution::CanCreateDesktopShortcuts() (and overrides) are not useful now. Either make it useful or remove it.
Now bugging benwells@, to notify/confirm with him regarding app id for shortcuts AND command line to run App Launcher, vs. AppListController::GetAppModelId() and AppListController::GetAppListCommandLine().
https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:298: HasSwitch(installer::switches::kChromeAppLauncher); On 2012/11/01 04:46:19, gab wrote: > I feel like checking the presence of a flag on the previous uninstall string > might be a bit risky, is there a better way to determine the current state of > the app_launcher install? This is standard practice. The uninstall string is the one-true source of installation state. > Also, I am not super-familiar with original_state; would this still work if > registration of the uninstall string happened on the system before the shortcuts > are installed? (don't have access to the code right now to double-check... i.e. > order of registration/shortcut creation should be independent). The original state is populated before the installation begins. It is a snapshot of the state of the system prior to installation commencing. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:422: base_properties.set_chrome_exe(target); On 2012/11/01 04:46:19, gab wrote: > The reason the ShellUtil::ChromeShortcutProperties were made chrome only and not > general (they were at first) is that it simplified giving defaults to non-set > properties (see ShellUtil::GetShortcutPropertiesFromChromeShortcutProperties() > or something like that). > > This will not work as the values you are not setting will default to Chrome's > (i.e. app_id, icon, etc.). > > What you probably want to do is as we had discussed and you had suggested: > make the mapping method in ShellUtil take a set of default properties and then > use those to set the non-set properties, then you can call that method with > different default properties based on the situation down in ShellUtil. We agreed F2F on the following: We will rename ShellUtil::ChromeShortcutProperties to ShellUtil::ShortcutProperties. We will rename ShellUtil::ChromeShortcutOperation to ShellUtil::ShortcutOperation. We will prefix all values of the ShellUtil::ShortcutOperation enum with SHELL_. We will expose a function from shell util: static ShellUtil::ShortcutProperties GetDefaultChromeShortcutProperties(BrowserDistribution* dist, ShellUtil::ShortcutOperation operation). We will modify GetShortcutPropertiesFromChromeShortcutProperties to no longer provide default values. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:424: if (product.is_chrome_app_host()) { On 2012/11/01 01:50:04, grt wrote: > consider if it makes sense to add to Product and ProductOperation so > conditionals like this aren't needed. Based on the above response to gab@, I suppose we could define: ShellUtil::ShortcutProperties Product::GetShortcutProperties(FilePath target_path, ShellUtil::ShortcutOperation) and a corresponding method on ProductOperations. The Chrome one would delegate to the static ShellUtil function mentioned earlier. SGTY, grt? https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:564: installer_state.UpdateStage(installer::CREATING_SHORTCUTS); On 2012/11/01 04:46:19, gab wrote: > Should we introduce a new stage constant? I think each constant is only mapped > to 1 section of the code thus far, but I could be wrong.. Let's just set this on line 558. It will encompass setting of shortcuts for both products, and if we don't actually need any shortcuts we will just transition in and transition out without doing any work. SGTY?
https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:424: if (product.is_chrome_app_host()) { On 2012/11/01 19:20:38, huangs1 wrote: > Eventually App Host and App Launcher will be the same thing. In > https://codereview.chromium.org/11267023/ , the decision is to minimize > divergence between App Host and App Launcher. For the case of Product, I > thought is_chrome_app_launcher() would be redundant. This DCHECK can be removed > once we have unification, but keeping it is also harmless. What grt meant was all the stuff inside the if () {} block. https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:428: // This is cleaner than appending strings, but as a result we'll get On 2012/11/01 19:20:38, huangs1 wrote: > The extra space is really annoying. Can I add an extra static routine in > CommandLine to e.g. render "--xxx" from "xxx", and then use it? Does it require code changes somewhere? If not, why is it annoying? If so, just trim whitespace from the result of GetCommandLineString. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/ins... chrome/installer/setup/install.cc:572: if (process_app_launcher_shortcuts || process_chrome_shortcuts) { personally I would say don't bother with this 'if'. Just update the state either way. If no shortcuts need to be created, it's no big deal to enter and exit the state without any work. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:55: // Should be same as AppListController::GetAppModelId(). Note that this will end up being suffixed with a user-specific suffix whereas the one generated by AppListController seems like it will not end up being suffixed. We should make sure that the one generated by AppListController is suffixed in the exact same way. I'll follow up with specific instructions there.
On 2012/11/01 19:23:01, huangs1 wrote: > Now bugging benwells@, to notify/confirm with him regarding app id for shortcuts > AND command line to run App Launcher, vs. AppListController::GetAppModelId() and > AppListController::GetAppListCommandLine(). So to be clear: - the command line in AppListController should use app_host.exe - the app model id should have the correct suffix FYI this patch should land soon: http://codereview.chromium.org/11367002/ which adds a shortcut with a flag. This is just to get people dogfooding quickly. About the command line: it should be updated once the whole opt in flow is working About the app model id: they should match. Note in the patch just uploaded i changed how i generated it as it wasn't including the user data directory in the profile folder properly. I thought it would get this as it goes through ShellIntegration::GetAppListAppModelIdForProfile. If not it should change (and maybe the web_app stuff should change too).
https://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:55: // Should be same as AppListController::GetAppModelId(). On 2012/11/01 20:43:36, erikwright wrote: > Note that this will end up being suffixed with a user-specific suffix whereas > the one generated by AppListController seems like it will not end up being > suffixed. > > We should make sure that the one generated by AppListController is suffixed in > the exact same way. > > I'll follow up with specific instructions there. I will do a small refactoring of the AppListController that should make it easy to fix this up properly. In the meantime, you should make sure to 'set_app_model_id' on the ShortcutProperties to the result of calling ShellUtil::BuildAppModelId() with a vector of components that contains _only_ kAppListId.
Comments below. Cheers! Gab http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:298: HasSwitch(installer::switches::kChromeAppLauncher); On 2012/11/01 20:02:20, erikwright wrote: > On 2012/11/01 04:46:19, gab wrote: > > I feel like checking the presence of a flag on the previous uninstall string > > might be a bit risky, is there a better way to determine the current state of > > the app_launcher install? > > This is standard practice. The uninstall string is the one-true source of > installation state. Ok, sgtm. > > > Also, I am not super-familiar with original_state; would this still work if > > registration of the uninstall string happened on the system before the > shortcuts > > are installed? (don't have access to the code right now to double-check... > i.e. > > order of registration/shortcut creation should be independent). > > The original state is populated before the installation begins. It is a snapshot > of the state of the system prior to installation commencing. Perfect, that's what I expected from the name, but just wanted to double-check. http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:415: // Shortcuts are always installed per-user unless specified. On 2012/11/01 19:20:38, huangs1 wrote: > On 2012/11/01 01:21:50, erikwright wrote: > > any reason for this moving from above? > > shortcut_level exists solely to initialize base_properties. I find it bizarre > that the switch block lies between. Ah right, it used to be used for other stuff that has been removed, makes sense to bring by |base_properties| now. http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:422: base_properties.set_chrome_exe(target); On 2012/11/01 20:02:20, erikwright wrote: > On 2012/11/01 04:46:19, gab wrote: > > The reason the ShellUtil::ChromeShortcutProperties were made chrome only and > not > > general (they were at first) is that it simplified giving defaults to non-set > > properties (see ShellUtil::GetShortcutPropertiesFromChromeShortcutProperties() > > or something like that). > > > > This will not work as the values you are not setting will default to Chrome's > > (i.e. app_id, icon, etc.). > > > > What you probably want to do is as we had discussed and you had suggested: > > make the mapping method in ShellUtil take a set of default properties and then > > use those to set the non-set properties, then you can call that method with > > different default properties based on the situation down in ShellUtil. > > We agreed F2F on the following: > > We will rename ShellUtil::ChromeShortcutProperties to > ShellUtil::ShortcutProperties. > > We will rename ShellUtil::ChromeShortcutOperation to > ShellUtil::ShortcutOperation. > > We will prefix all values of the ShellUtil::ShortcutOperation enum with SHELL_. > > We will expose a function from shell util: > > static ShellUtil::ShortcutProperties > GetDefaultChromeShortcutProperties(BrowserDistribution* dist, > ShellUtil::ShortcutOperation operation). > > We will modify GetShortcutPropertiesFromChromeShortcutProperties to no longer > provide default values. sgtm, though there are more methods than this that will need rename in ShellUtil (e.g. RemoveChromeShortcuts, CreateOrUpdateChromeShortcuts, etc.) http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:424: if (product.is_chrome_app_host()) { On 2012/11/01 20:02:20, erikwright wrote: > On 2012/11/01 01:50:04, grt wrote: > > consider if it makes sense to add to Product and ProductOperation so > > conditionals like this aren't needed. > > Based on the above response to gab@, I suppose we could define: > > ShellUtil::ShortcutProperties Product::GetShortcutProperties(FilePath > target_path, ShellUtil::ShortcutOperation) > > and a corresponding method on ProductOperations. > > The Chrome one would delegate to the static ShellUtil function mentioned > earlier. > > SGTY, grt? sgtm http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:428: // This is cleaner than appending strings, but as a result we'll get On 2012/11/01 20:43:36, erikwright wrote: > On 2012/11/01 19:20:38, huangs1 wrote: > > The extra space is really annoying. Can I add an extra static routine in > > CommandLine to e.g. render "--xxx" from "xxx", and then use it? > > Does it require code changes somewhere? If not, why is it annoying? > > If so, just trim whitespace from the result of GetCommandLineString. This is probably just a tiny bug in GetCommandLineString() which can be fixed in another CL, I also don't think it deserves a comment here, this paradigm is used all over the place already. http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/inst... chrome/installer/setup/install.cc:564: installer_state.UpdateStage(installer::CREATING_SHORTCUTS); On 2012/11/01 20:02:20, erikwright wrote: > On 2012/11/01 04:46:19, gab wrote: > > Should we introduce a new stage constant? I think each constant is only mapped > > to 1 section of the code thus far, but I could be wrong.. > > Let's just set this on line 558. It will encompass setting of shortcuts for both > products, and if we don't actually need any shortcuts we will just transition in > and transition out without doing any work. SGTY? SGTM, although it might be cleaner if we split the shortcut creation logic on its own. i.e. UpdateStage(installer::CREATING_SHORTCUTS) if (is app host) { shortcut app host... } else if (is_chrome) { shortcut for chrome... } if (is_chrome) { UpdateStage(installer::REGISTERING_CHROME) make default... (currently done before switching the stage but it makes more sense as part of the registration stage imo) other chrome registration... ... } ... http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:310: VLOG(1) << "Deleting Start Menu shortcuts."; On 2012/11/01 19:20:38, huangs1 wrote: > Moving all these into common, along with > ShellUtil::RemoveChromeStartScreenShortcuts(). > Also changed comments. Hmmm ShellUtil::RemoveChromeStartScreenShortcuts() is for secondary tiles will the app launcher use any secondary tiles? I'm also adding ShellUtil::RemoveChromeTaskbarShortcuts() in http://codereview.chromium.org/11368040/. This should be moved to the Common section when it lands and you merge. FYI, all of these methods made common should no longer have "Chrome" in them. http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/inst... chrome/installer/setup/install.cc:587: if (process_chrome_shortcuts) { I don't like the name "process_chrome_shortcuts" as this block does more than just shortcuts; you can keep it like this, but consider the structure I just replied on the same lines on the previous patch set. http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); I find this weird... if you want to keep the above logic for |install_level|, remove this DCHECK() -- otherwise you "support it", but the DCHECK prevents it from actually working in debug builds... (i.e. you can "implement" it by removing the DCHECK...). Otherwise make it: DCHECK(!installer_state.system_install()); ShellUtil::ShellChange install_level = ShellUtil::CURRENT_USER; http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:1077: .target_path().Append(installer::kChromeAppHostExe).value()); nit: style dictates that either you wrap at the paranthesis (which doesn't fit here, or you wrap everything 4 spaces. So in this case I think you'll have to do something like: const string16 app_host_exe( installer_state.target_path().Append(installer::kChromeAppHostExe) .value()); http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:1096: // First delete shortcuts from Start menu, Desktop, and Quick Launch. Again, remove "First" as this is the only thing you do here :). http://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chrom... File chrome/installer/util/chrome_app_host_distribution.cc (right): http://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chrom... chrome/installer/util/chrome_app_host_distribution.cc:26: const wchar_t kAppListId[] = L"ChromeAppList"; I don't feel a constant is necessary, I prefer inline below (this only forces the reader to look up in the file to know what to expect for no added benefit I feel. If you really want to keep the constant, at least make it first (i.e. sort them alphabetically).
http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); On 2012/11/02 04:19:58, gab wrote: > I find this weird... if you want to keep the above logic for |install_level|, > remove this DCHECK() -- otherwise you "support it", but the DCHECK prevents it > from actually working in debug builds... (i.e. you can "implement" it by > removing the DCHECK...). > > Otherwise make it: > DCHECK(!installer_state.system_install()); > ShellUtil::ShellChange install_level = ShellUtil::CURRENT_USER; There's presumably more code that would be needed if we wanted to support system level installs. For now we don't, and code elsewhere will bail if --system-level --app-launcher. I asked Sam to go ahead and pass through the install level (don't hard code it to user-level) but to DCHECK or NOTREACHED anywhere that additional code would be required so that we will run into these areas if/when we do implement system-level launcher installs.
http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); On 2012/11/02 04:31:01, erikwright wrote: > On 2012/11/02 04:19:58, gab wrote: > > I find this weird... if you want to keep the above logic for |install_level|, > > remove this DCHECK() -- otherwise you "support it", but the DCHECK prevents it > > from actually working in debug builds... (i.e. you can "implement" it by > > removing the DCHECK...). > > > > Otherwise make it: > > DCHECK(!installer_state.system_install()); > > ShellUtil::ShellChange install_level = ShellUtil::CURRENT_USER; > > There's presumably more code that would be needed if we wanted to support system > level installs. > > For now we don't, and code elsewhere will bail if --system-level --app-launcher. > > I asked Sam to go ahead and pass through the install level (don't hard code it > to user-level) but to DCHECK or NOTREACHED anywhere that additional code would > be required so that we will run into these areas if/when we do implement > system-level launcher installs. Well, no additional code would be required here that's the thing... Maybe a DCHECK(!installer_state.system_install) at the callsite would make more sense?
http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); On 2012/11/02 04:39:54, gab wrote: > On 2012/11/02 04:31:01, erikwright wrote: > > On 2012/11/02 04:19:58, gab wrote: > > > I find this weird... if you want to keep the above logic for > |install_level|, > > > remove this DCHECK() -- otherwise you "support it", but the DCHECK prevents > it > > > from actually working in debug builds... (i.e. you can "implement" it by > > > removing the DCHECK...). > > > > > > Otherwise make it: > > > DCHECK(!installer_state.system_install()); > > > ShellUtil::ShellChange install_level = ShellUtil::CURRENT_USER; > > > > There's presumably more code that would be needed if we wanted to support > system > > level installs. > > > > For now we don't, and code elsewhere will bail if --system-level > --app-launcher. > > > > I asked Sam to go ahead and pass through the install level (don't hard code it > > to user-level) but to DCHECK or NOTREACHED anywhere that additional code would > > be required so that we will run into these areas if/when we do implement > > system-level launcher installs. > > Well, no additional code would be required here that's the thing... > > Maybe a DCHECK(!installer_state.system_install) at the callsite would make more > sense? If that's the case (I hadn't looked, assumed it wasn't, but now I see that it seems to be) then yes, this DCHECK should go.
http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); On 2012/11/02 04:48:39, erikwright wrote: > On 2012/11/02 04:39:54, gab wrote: > > On 2012/11/02 04:31:01, erikwright wrote: > > > On 2012/11/02 04:19:58, gab wrote: > > > > I find this weird... if you want to keep the above logic for > > |install_level|, > > > > remove this DCHECK() -- otherwise you "support it", but the DCHECK > prevents > > it > > > > from actually working in debug builds... (i.e. you can "implement" it by > > > > removing the DCHECK...). > > > > > > > > Otherwise make it: > > > > DCHECK(!installer_state.system_install()); > > > > ShellUtil::ShellChange install_level = ShellUtil::CURRENT_USER; > > > > > > There's presumably more code that would be needed if we wanted to support > > system > > > level installs. > > > > > > For now we don't, and code elsewhere will bail if --system-level > > --app-launcher. > > > > > > I asked Sam to go ahead and pass through the install level (don't hard code > it > > > to user-level) but to DCHECK or NOTREACHED anywhere that additional code > would > > > be required so that we will run into these areas if/when we do implement > > > system-level launcher installs. > > > > Well, no additional code would be required here that's the thing... > > > > Maybe a DCHECK(!installer_state.system_install) at the callsite would make > more > > sense? > > If that's the case (I hadn't looked, assumed it wasn't, but now I see that it > seems to be) then yes, this DCHECK should go. Well, thinking more about it, when you do implement system-level installs you will likely want per-user shortcut as Chrome is now doing so that upon uninstall you'll still only need to remove per-user shortcuts. We need to also check for all-users shortcuts for Chrome as they used to exist, but if you never had them you don't need to check for that, so this here should always be CURRENT_USER (i.e. remove for uninstalling user, other users will eventually get a message from Windows telling them their shortcut now point to an invalid target with a YES/NO remove dialog).
https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:424: if (product.is_chrome_app_host()) { On 2012/11/01 20:02:20, erikwright wrote: > On 2012/11/01 01:50:04, grt wrote: > > consider if it makes sense to add to Product and ProductOperation so > > conditionals like this aren't needed. > > Based on the above response to gab@, I suppose we could define: > > ShellUtil::ShortcutProperties Product::GetShortcutProperties(FilePath > target_path, ShellUtil::ShortcutOperation) > > and a corresponding method on ProductOperations. > > The Chrome one would delegate to the static ShellUtil function mentioned > earlier. > > SGTY, grt? sure https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/ins... chrome/installer/setup/install.cc:572: if (process_app_launcher_shortcuts || process_chrome_shortcuts) { On 2012/11/01 20:43:36, erikwright wrote: > personally I would say don't bother with this 'if'. Just update the state either > way. If no shortcuts need to be created, it's no big deal to enter and exit the > state without any work. +1 https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); DCHECK_EQ
PTAL. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/ins... chrome/installer/setup/install.cc:572: if (process_app_launcher_shortcuts || process_chrome_shortcuts) { On 2012/11/01 20:43:36, erikwright wrote: > personally I would say don't bother with this 'if'. Just update the state either > way. If no shortcuts need to be created, it's no big deal to enter and exit the > state without any work. Done. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/ins... chrome/installer/setup/install.cc:587: if (process_chrome_shortcuts) { Got rid of the variable. The original comment was also misleading. So I broke this into separate blocks (with redundant ifs), and added a short comment each. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); Moved the check to the caller. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:329: DCHECK(install_level == ShellUtil::CURRENT_USER); Thanks (not applicable for this case any more though)! https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1077: .target_path().Append(installer::kChromeAppHostExe).value()); On 2012/11/02 04:19:58, gab wrote: > nit: style dictates that either you wrap at the paranthesis (which doesn't fit > here, or you wrap everything 4 spaces. > > So in this case I think you'll have to do something like: > const string16 app_host_exe( > installer_state.target_path().Append(installer::kChromeAppHostExe) > .value()); Done. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1096: // First delete shortcuts from Start menu, Desktop, and Quick Launch. On 2012/11/02 04:19:58, gab wrote: > Again, remove "First" as this is the only thing you do here :). Done. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:26: const wchar_t kAppListId[] = L"ChromeAppList"; Keeping the constant, so sorting. The const is added to maintain consistency with app_list_controller_win.cc. https://codereview.chromium.org/11359013/diff/3007/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:55: // Should be same as AppListController::GetAppModelId(). Done (code is in install.cc: CreateOrUpdateShortcuts()).
Made big merge. PTAL.
https://chromiumcodereview.appspot.com/11359013/diff/14001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/11359013/diff/14001/chrome/installer/s... chrome/installer/setup/install.cc:424: if (product.is_chrome()) { So lines 424 through 441 will be replaced with line 425. https://chromiumcodereview.appspot.com/11359013/diff/14001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/11359013/diff/14001/chrome/installer/s... chrome/installer/setup/uninstall.cc:259: // Deletes shortcuts at |install_level| from Start menu, Desktop, I guess this is to be removed?
PTAL. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:424: if (product.is_chrome()) { On 2012/11/07 21:15:30, erikwright wrote: > So lines 424 through 441 will be replaced with line 425. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:259: // Deletes shortcuts at |install_level| from Start menu, Desktop, On 2012/11/07 21:15:30, erikwright wrote: > I guess this is to be removed? I was trying to see if there's anything that can be salvaged. Removed.
Looks good, first comments post-merge. Cheers, Gab https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:144: VLOG(1) << "Failed to copy master_preferences from:" I don't think this should be modified. See it likes this, we have *the "master preferences"* and *the "master_preferences" file*. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:384: DCHECK(product.is_chrome() || product.is_chrome_app_host()); Now that this method has been made generic remove the DCHECK altogether. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:424: if (product.is_chrome()) { On 2012/11/07 21:15:30, erikwright wrote: > So lines 424 through 441 will be replaced with line 425. That's how I see it too. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:575: app_launcher_product->HasOption(kOptionAppHostIsLauncher)) { Indent -2 spaces. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:576: // Remove this check once we have system-level App Host. Put TODO in front of this comment. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:616: // If requested, make Chrome the default browser. Change this comment to: "// Register Chrome and, if requested, make Chrome the default browser." and move stage update for REGISTERING_CHROME here. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:632: // Register Chrome and configure auto-launch. Put auto-launch part of this comment right above the stage update for CONFIGURE_AUTO_LAUNCH below. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:633: installer_state.UpdateStage(installer::REGISTERING_CHROME); Move this to the top of this if block. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:259: // Deletes shortcuts at |install_level| from Start menu, Desktop, On 2012/11/07 21:15:30, erikwright wrote: > I guess this is to be removed? I'd say the opposite, add "taskbar" to this list and keep this comment. Remove the old comment on DeleteShortcuts(). https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:272: void DeleteShortcuts(const InstallerState& installer_state, And see, that's why we asked you not to commit your "simple addition of DeleteShortcutsCommon()" as part of your previous CL; to keep it in sync with the motivating changes (this CL). Because now the motivating changes don't need this anymore. You never know and there is no point doing something before you actually need it ;)! Cheers! https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1062: // Remove this check once we have system-level App Host. Add TODO in front of comment. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1074: if (is_chrome) { else if {} from block above, the product can't both be app_host and chrome. I think I actually prefer it to be flipped, i.e.: if (is_chrome) { ... } else if (is_app_host) { ... } but I'm fine either way. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1085: // Delete shortcuts from Start menu, Desktop, and Quick Launch. This comment is now also missing taskbar and start screen... in general I think it's overly verbose, the call itself is pretty implicit. I'd say just remove it. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/util/chr... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/util/chr... chrome/installer/util/chrome_app_host_distribution.cc:56: return kAppListId; Hard-coding the value here is fine, this is what's done for all other values across distributions. It's also more readable inline s the reader doesn't need to scroll up in the file to find the value. The constant would be good if you needed it more than once, but that's not the case.
PTAL. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:144: VLOG(1) << "Failed to copy master_preferences from:" On 2012/11/07 21:44:40, gab wrote: > I don't think this should be modified. See it likes this, we have *the "master > preferences"* and *the "master_preferences" file*. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:384: DCHECK(product.is_chrome() || product.is_chrome_app_host()); On 2012/11/07 21:44:40, gab wrote: > Now that this method has been made generic remove the DCHECK altogether. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:424: if (product.is_chrome()) { On 2012/11/07 21:44:40, gab wrote: > On 2012/11/07 21:15:30, erikwright wrote: > > So lines 424 through 441 will be replaced with line 425. > > That's how I see it too. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:575: app_launcher_product->HasOption(kOptionAppHostIsLauncher)) { On 2012/11/07 21:44:40, gab wrote: > Indent -2 spaces. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:576: // Remove this check once we have system-level App Host. On 2012/11/07 21:44:40, gab wrote: > Put TODO in front of this comment. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:616: // If requested, make Chrome the default browser. On 2012/11/07 21:44:40, gab wrote: > Change this comment to: > "// Register Chrome and, if requested, make Chrome the default browser." > and move stage update for REGISTERING_CHROME here. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:632: // Register Chrome and configure auto-launch. On 2012/11/07 21:44:40, gab wrote: > Put auto-launch part of this comment right above the stage update for > CONFIGURE_AUTO_LAUNCH below. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/in... chrome/installer/setup/install.cc:633: installer_state.UpdateStage(installer::REGISTERING_CHROME); On 2012/11/07 21:44:40, gab wrote: > Move this to the top of this if block. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:259: // Deletes shortcuts at |install_level| from Start menu, Desktop, On 2012/11/07 21:44:40, gab wrote: > On 2012/11/07 21:15:30, erikwright wrote: > > I guess this is to be removed? > > I'd say the opposite, add "taskbar" to this list and keep this comment. > Remove the old comment on DeleteShortcuts(). Done, replaced old comment with 3-line comment. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:272: void DeleteShortcuts(const InstallerState& installer_state, ! https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1062: // Remove this check once we have system-level App Host. On 2012/11/07 21:44:40, gab wrote: > Add TODO in front of comment. Done. https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1074: if (is_chrome) { On 2012/11/07 21:44:40, gab wrote: > else if {} from block above, the product can't both be app_host and chrome. > > I think I actually prefer it to be flipped, i.e.: > if (is_chrome) { > ... > } else if (is_app_host) { > ... > } > but I'm fine either way. Done (using else if; handling is_chrome first). https://codereview.chromium.org/11359013/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1085: // Delete shortcuts from Start menu, Desktop, and Quick Launch. On 2012/11/07 21:44:40, gab wrote: > This comment is now also missing taskbar and start screen... in general I think > it's overly verbose, the call itself is pretty implicit. I'd say just remove it. Done (removed both comments from the 2 callers). https://codereview.chromium.org/11359013/diff/14001/chrome/installer/util/chr... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359013/diff/14001/chrome/installer/util/chr... chrome/installer/util/chrome_app_host_distribution.cc:56: return kAppListId; On 2012/11/07 21:44:40, gab wrote: > Hard-coding the value here is fine, this is what's done for all other values > across distributions. > > It's also more readable inline s the reader doesn't need to scroll up in the > file to find the value. The constant would be good if you needed it more than > once, but that's not the case. Done.
Looks great, few details left. https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/ins... chrome/installer/setup/install.cc:614: // Register Chrome. nit: Remove this comment doesn't add any value (the name of the method itself is more explicit). https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1075: nit: Should be only one empty line between the end of this block and next statement. https://codereview.chromium.org/11359013/diff/6005/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_operations.cc (right): https://codereview.chromium.org/11359013/diff/6005/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_operations.cc:130: components.push_back(dist->GetBaseAppId()); Append ShellUtil::GetUserSpecificRegistrySuffix() to the base appid. I assume the app launcher shortcut won't be per-profile (i.e. that profile switching will be in the app, just as in Chrome); otherwise you should look into ShellIntegration::GetAppModelIdForProfile() although that doesn't yet handle user suffixes since it's only used as a runtime property now).
Updated. PTAL. https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/ins... chrome/installer/setup/install.cc:614: // Register Chrome. On 2012/11/08 00:10:58, gab wrote: > nit: Remove this comment doesn't add any value (the name of the method itself is > more explicit). Done. https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11359013/diff/6005/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1075: On 2012/11/08 00:10:58, gab wrote: > nit: Should be only one empty line between the end of this block and next > statement. Done. https://codereview.chromium.org/11359013/diff/6005/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_operations.cc (right): https://codereview.chromium.org/11359013/diff/6005/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_operations.cc:130: components.push_back(dist->GetBaseAppId()); From F2F, append => string contatenation (not push_back()). Done.
LGTM w/ nit below. FYI, I'm assuming this is always installed on a per-user basis, but if you need system-level installs (i.e. a per-user shortcut for every user), you'll need to plug into ActiveSetup ... either way, this is fine for now. https://codereview.chromium.org/11359013/diff/10003/chrome/installer/util/chr... File chrome/installer/util/chrome_app_host_operations.cc (right): https://codereview.chromium.org/11359013/diff/10003/chrome/installer/util/chr... chrome/installer/util/chrome_app_host_operations.cc:132: string16 baseAppId(dist->GetBaseAppId()); nit: base_app_id (always user lower case, separated by '_', names for variables). Pascal Casing for methods.
Done! https://codereview.chromium.org/11359013/diff/10003/chrome/installer/util/chr... File chrome/installer/util/chrome_app_host_operations.cc (right): https://codereview.chromium.org/11359013/diff/10003/chrome/installer/util/chr... chrome/installer/util/chrome_app_host_operations.cc:132: string16 baseAppId(dist->GetBaseAppId()); On 2012/11/08 00:40:07, gab wrote: > nit: base_app_id (always user lower case, separated by '_', names for > variables). > > Pascal Casing for methods. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/11359013/3009
On 2012/11/01 21:25:32, benwells wrote: > On 2012/11/01 19:23:01, huangs1 wrote: > > Now bugging benwells@, to notify/confirm with him regarding app id for > shortcuts > > AND command line to run App Launcher, vs. AppListController::GetAppModelId() > and > > AppListController::GetAppListCommandLine(). > > So to be clear: > - the command line in AppListController should use app_host.exe > - the app model id should have the correct suffix > > FYI this patch should land soon: http://codereview.chromium.org/11367002/ which > adds a shortcut with a flag. This is just to get people dogfooding quickly. So should the above-mentioned CL now be reverted? > > About the command line: it should be updated once the whole opt in flow is > working > About the app model id: they should match. Note in the patch just uploaded i > changed how i generated it as it wasn't including the user data directory in the > profile folder properly. I thought it would get this as it goes through > ShellIntegration::GetAppListAppModelIdForProfile. If not it should change (and > maybe the web_app stuff should change too).
Change committed as 166608 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
