Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
612093007:
Remove kCmdInstallExtension and kCmdInstallApp from Installer code (Closed)
|
Created:
6 years, 2 months ago by Devlin Modified:
6 years, 2 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org, huangs Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemove kCmdInstallExtension and kCmdInstallApp from Installer code
Remove the code to add work items for kCmdInstallExtension and kCmdInstallApp
in the installer code, and add code to remove any old registry links to them.
Also remove validation code for these.
This is required in order to remove the --(limited)-install-from-webstore switches.
BUG=297647
Committed: https://crrev.com/aa895c2b0d1c1b6a3e5dc4e79195ef4911179ab9
Cr-Commit-Position: refs/heads/master@{#297664}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Messages
Total messages: 20 (7 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + grt@chromium.org
Greg, please take a look when you can. Thanks! :) https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:395: AddLegacyRemovalItem(installer_state, p, kLegacyCmdInstallApp, list); I think in your email you recommended adding this removal item for all products, but it was only added for the chrome_app_host product, and the cmd key is generated based on the product distribution - should we still add it for any/all products? https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... File chrome/installer/util/installation_validator.cc (left): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... chrome/installer/util/installation_validator.cc:206: void InstallationValidator::ValidateInstallCommand( My understanding is that, since these should never be attached as commands anymore, there's no reason to validate them. Right?
awesome. thanks for taking care of this. https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:238: void AddLegacyRemovalItem(const InstallerState& installer_state, since this specifically removes an app command, how about AddLegacyAppCommandRemovalItem? https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:242: base::string16 full_cmd_key( nit: get rid of this local var and call GetRegCommandKey in-line below https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:244: work_item_list->AddDeleteRegKeyWorkItem( please add a comment stating that failures are ignored since this is a clean-up operation that shouldn't block an install/update on failure. https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:395: AddLegacyRemovalItem(installer_state, p, kLegacyCmdInstallApp, list); On 2014/09/30 18:03:01, Devlin wrote: > I think in your email you recommended adding this removal item for all products, > but it was only added for the chrome_app_host product, and the cmd key is > generated based on the product distribution - should we still add it for any/all > products? To be safe, I think so. I can't say for sure that it's possible for the kCmdInstallApp command to be registered without the app host product being in the set of products. It's safe to remove it regardless. https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... File chrome/installer/util/installation_validator.cc (left): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... chrome/installer/util/installation_validator.cc:206: void InstallationValidator::ValidateInstallCommand( On 2014/09/30 18:03:01, Devlin wrote: > My understanding is that, since these should never be attached as commands > anymore, there's no reason to validate them. Right? Could you make it validate that the commands aren't present?
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:238: void AddLegacyRemovalItem(const InstallerState& installer_state, On 2014/09/30 18:30:32, grt wrote: > since this specifically removes an app command, how about > AddLegacyAppCommandRemovalItem? It actually removes anything that's passed in via |name| (currently App or Extension) so saying AppCommand is a bit misleading, no? https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:242: base::string16 full_cmd_key( On 2014/09/30 18:30:32, grt wrote: > nit: get rid of this local var and call GetRegCommandKey in-line below Done. https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:244: work_item_list->AddDeleteRegKeyWorkItem( On 2014/09/30 18:30:32, grt wrote: > please add a comment stating that failures are ignored since this is a clean-up > operation that shouldn't block an install/update on failure. Done. https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... File chrome/installer/util/installation_validator.cc (left): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... chrome/installer/util/installation_validator.cc:206: void InstallationValidator::ValidateInstallCommand( On 2014/09/30 18:30:33, grt wrote: > On 2014/09/30 18:03:01, Devlin wrote: > > My understanding is that, since these should never be attached as commands > > anymore, there's no reason to validate them. Right? > > Could you make it validate that the commands aren't present? I *think* (still very new to this code, so could be very wrong) that they will be implicitly checked via ValidateAppCommandExpectations(), which, on lines 313 - 316 of the new version (originally lines 371 - 374) will throw an error and fail validation if an unexpected command is found. Since we remove our expectations for these commands in ValidateAppCommands(), this will do the trick, right?
https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:238: void AddLegacyRemovalItem(const InstallerState& installer_state, On 2014/09/30 18:51:01, Devlin wrote: > On 2014/09/30 18:30:32, grt wrote: > > since this specifically removes an app command, how about > > AddLegacyAppCommandRemovalItem? > > It actually removes anything that's passed in via |name| (currently App or > Extension) so saying AppCommand is a bit misleading, no? name collision! "AppCommand" is the generic term for these types of commands that are registered with Google Update, and is orthogonal to Chrome Apps or Chrome Extensions. it is a coincidence that the two AppCommands you're removing are called "install-extension" and "install-application". https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... File chrome/installer/util/installation_validator.cc (left): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/util/in... chrome/installer/util/installation_validator.cc:206: void InstallationValidator::ValidateInstallCommand( On 2014/09/30 18:51:02, Devlin wrote: > On 2014/09/30 18:30:33, grt wrote: > > On 2014/09/30 18:03:01, Devlin wrote: > > > My understanding is that, since these should never be attached as commands > > > anymore, there's no reason to validate them. Right? > > > > Could you make it validate that the commands aren't present? > > I *think* (still very new to this code, so could be very wrong) that they will > be implicitly checked via ValidateAppCommandExpectations(), which, on lines 313 > - 316 of the new version (originally lines 371 - 374) will throw an error and > fail validation if an unexpected command is found. Since we remove our > expectations for these commands in ValidateAppCommands(), this will do the > trick, right? i think you're right, thanks. https://codereview.chromium.org/612093007/diff/80001/chrome/installer/setup/i... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/80001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:1336: // Remove the legacy "install-application" commmand from all products. this isn't quite right. the kCmdInstallApp will only be registered under the app hosts's appguid. its removal should be unconditional, but its location is deterministic. instead of AddLegacyRemovalItem taking a Product*, make it take a BrowserDistribution and pass in the app host's.
https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/40001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:238: void AddLegacyRemovalItem(const InstallerState& installer_state, On 2014/09/30 18:59:26, grt wrote: > On 2014/09/30 18:51:01, Devlin wrote: > > On 2014/09/30 18:30:32, grt wrote: > > > since this specifically removes an app command, how about > > > AddLegacyAppCommandRemovalItem? > > > > It actually removes anything that's passed in via |name| (currently App or > > Extension) so saying AppCommand is a bit misleading, no? > > name collision! "AppCommand" is the generic term for these types of commands > that are registered with Google Update, and is orthogonal to Chrome Apps or > Chrome Extensions. it is a coincidence that the two AppCommands you're removing > are called "install-extension" and "install-application". Ahh, gotcha. Done. https://codereview.chromium.org/612093007/diff/80001/chrome/installer/setup/i... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/80001/chrome/installer/setup/i... chrome/installer/setup/install_worker.cc:1336: // Remove the legacy "install-application" commmand from all products. On 2014/09/30 18:59:26, grt wrote: > this isn't quite right. the kCmdInstallApp will only be registered under the app > hosts's appguid. its removal should be unconditional, but its location is > deterministic. instead of AddLegacyRemovalItem taking a Product*, make it take a > BrowserDistribution and pass in the app host's. Done.
https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:246: GetRegCommandKey(product.distribution(), name), product.distribution() -> distribution https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:1334: // Remove the legacy "install-application" commmand from all products. suggestion: // Unconditionally remove the "install-application" command from the app // hosts's key. https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:1338: BrowserDistribution::CHROME_APP_HOST, BrowserDistribution::CHROME_APP_HOST, -> BrowserDistribution::CHROME_APP_HOST),
Patchset #4 (id:30007) has been deleted
https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:246: GetRegCommandKey(product.distribution(), name), On 2014/09/30 19:57:24, grt wrote: > product.distribution() -> distribution Whoops, sorry. Done. https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:1334: // Remove the legacy "install-application" commmand from all products. On 2014/09/30 19:57:24, grt wrote: > suggestion: > // Unconditionally remove the "install-application" command from the app > // hosts's key. Done. https://codereview.chromium.org/612093007/diff/100001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:1338: BrowserDistribution::CHROME_APP_HOST, On 2014/09/30 19:57:24, grt wrote: > BrowserDistribution::CHROME_APP_HOST, -> BrowserDistribution::CHROME_APP_HOST), D'oh. Working on Linux, so compilation means a try bot run. Apparently, it would have been worth it. Sorry :/
https://codereview.chromium.org/612093007/diff/130001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/130001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:1335: AddLegacyAppCommandRemovalItem( oh dear. i just noticed that there isn't a Chromium-branded variant of the CHROME_APP_HOST distribution, so this is actually the wrong thing for non-Google Chrome builds and for canary. fortunately, BrowserDistribution::AppHostIsSupported() will return true only for the Google Chrome CHROME_BROWSER distribution (and false for Chromium and Google Chrome Canary). given that, i think the shortest-path to getting this right is to put it into the is_chrome() block of AddProductSpecificWorkItems like so: if (p.is_chrome()) { ... if (p.distribution()->AppHostIsSupported()) { // Unconditionally remove the "install-application" command from the // app hosts's key. ... } } apologies for not noticing this sooner.
Patchset #5 (id:150001) has been deleted
https://codereview.chromium.org/612093007/diff/130001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/130001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:1335: AddLegacyAppCommandRemovalItem( On 2014/09/30 20:39:24, grt wrote: > oh dear. i just noticed that there isn't a Chromium-branded variant of the > CHROME_APP_HOST distribution, so this is actually the wrong thing for non-Google > Chrome builds and for canary. fortunately, > BrowserDistribution::AppHostIsSupported() will return true only for the Google > Chrome CHROME_BROWSER distribution (and false for Chromium and Google Chrome > Canary). given that, i think the shortest-path to getting this right is to put > it into the is_chrome() block of AddProductSpecificWorkItems like so: > > if (p.is_chrome()) { > ... > if (p.distribution()->AppHostIsSupported()) { > // Unconditionally remove the "install-application" command from the > // app hosts's key. > ... > } > } > > apologies for not noticing this sooner. Done. To confirm, this does mean we will never remove the key if product.is_chrome_app_host() (the only time the key was previously added). Is that okay?
LGTM https://codereview.chromium.org/612093007/diff/130001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/612093007/diff/130001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:1335: AddLegacyAppCommandRemovalItem( On 2014/09/30 20:55:50, Devlin wrote: > On 2014/09/30 20:39:24, grt wrote: > > oh dear. i just noticed that there isn't a Chromium-branded variant of the > > CHROME_APP_HOST distribution, so this is actually the wrong thing for > non-Google > > Chrome builds and for canary. fortunately, > > BrowserDistribution::AppHostIsSupported() will return true only for the Google > > Chrome CHROME_BROWSER distribution (and false for Chromium and Google Chrome > > Canary). given that, i think the shortest-path to getting this right is to put > > it into the is_chrome() block of AddProductSpecificWorkItems like so: > > > > if (p.is_chrome()) { > > ... > > if (p.distribution()->AppHostIsSupported()) { > > // Unconditionally remove the "install-application" command from the > > // app hosts's key. > > ... > > } > > } > > > > apologies for not noticing this sooner. > > Done. > > To confirm, this does mean we will never remove the key if > product.is_chrome_app_host() (the only time the key was previously added). Is > that okay? Yes. The app host was generally installed along with Google Chrome, so installer_state.products() most likely contains Chrome. There were exceptions, but I don't think we need to be concerned with them now.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/612093007/170001
Message was sent while issue was closed.
Committed patchset #5 (id:170001) as b3a01a46b16fa4bfd7156b4b5dd09a2540a0c7d3
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/aa895c2b0d1c1b6a3e5dc4e79195ef4911179ab9 Cr-Commit-Position: refs/heads/master@{#297664} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
