Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11054006:
Make application shortcuts point to app_host.exe, install App Host during app installation. (Closed)
|
Created:
8 years, 2 months ago by huangs Modified:
8 years, 1 month ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, grt+watch_chromium.org, gab, miket_OOO Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMake application shortcuts point to app_host.exe, install App Host during app installation.
Platform apps need app_host.exe. 2 parts to this change are:
(A) App shortcut now call "app_host.exe" instead of "chrome.exe".
(B) On platform app install, check the presence of app_host.exe. If it's absent, call quick-enable-application-host to install app_host.exe. Wait for completion, then proceeding to install app.
(B) should be executed in the FILE thread. However, some installation tasks may need to wait until quick-enable-application-host finishes (had CRX and unpacked app install in mind, but that's no longer the case). To avoid blocking the UI thread, we:
- Create AppHostInstaller class, implementing EnsureAppHostInstalled().
- When (in AppShortcutManager::Observe) we want to ensure App Host is installed, we call the routine and specify a callback runction that takes a bool |success| flag.
- AppHostInstaller handles the complexities of switching threads, checking presence of app_host.exe, grabs quick-enable-application-host command, calling it, listening to completion, and then notifies the caller via the callback.
BUG=138319
TEST=install, then install an app. Verify App Host installed. Right-click on an app in NTP, create shortcut. Verify shortcut points to app_host.exe
TBR=brettw
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166640
Patch Set 1 #
Total comments: 40
Patch Set 2 : Moving quick-enable-app-host feature; adding App Host install code for unpacked extension installer. #
Total comments: 20
Patch Set 3 : Rearranged code for unpacked_installer.cc; moved App Host code from chrome_launch_support to app_ho… #
Total comments: 58
Patch Set 4 : Changed AppHostInstaller interface; handled thread compilcation in AppHostInstaller; added error me… #
Total comments: 33
Patch Set 5 : Extracting app_host_installer_impl_win.*; proposing simplified flow to switch between threads for A… #
Total comments: 16
Patch Set 6 : Making AppHostInstallerImpl() manage its own destruction. #
Total comments: 16
Patch Set 7 : Changing code entry from class to function. #
Total comments: 26
Patch Set 8 : Refactoring AppHostInstallerImpl: checking for is_platform_app() before flow; entry now done by sta… #
Total comments: 26
Patch Set 9 : Fixing comments. #
Total comments: 11
Patch Set 10 : Synched; comments and #include fixes. #Patch Set 11 : Updating dependency, since code is Windows-specific. #Patch Set 12 : Fixing crashing ExtensionServiceTest.LoadExtensionsWithPlugins unit test, caused by usage of const … #
Total comments: 18
Patch Set 13 : Stylistic changes: typedef and function renames. #
Total comments: 9
Patch Set 14 : Now installing App Host in AppShortcutManager, instead of CrxInstaller / UnpackedInstaller. #
Total comments: 8
Patch Set 15 : Refactoring; moving App Host install errors to logs. #
Total comments: 4
Patch Set 16 : Adding app_host_installer_win.*; using WeakPtrFactory in AppShortcutManager. #
Total comments: 12
Patch Set 17 : Fixing headers. #
Total comments: 4
Patch Set 18 : Updating dependencies; fixing Linux build breaks. #Patch Set 19 : Updating dependencies; fixing Linux build breaks. #Patch Set 20 : Giving AppShortcutManager explicit out-of-line destructor to make linux_clang happy. #Patch Set 21 : Also need virtual (but without OVERRIDE). #Messages
Total messages: 75 (0 generated)
This is a work in progress. Currently the code does not work due to Error 740: The requested operation requires elevation. when we execute the quick-enable-application-host commandline from Google Update.
Sam, is that error because you are building using ninja, and thus don't have manifests embedded? On Tue, Oct 2, 2012 at 6:57 PM, <huangs@chromium.org> wrote: > Reviewers: erikwright, > > Message: > This is a work in progress. Currently the code does not work due to > > Error 740: The requested operation requires elevation. > > when we execute the quick-enable-application-host commandline from Google > Update. > > Description: > Making application shortcurts point to app_host.exe, and implementing > quick-enable-application-host. > > (Work in progress). > > BUG=138319 > > > Please review this at https://codereview.chromium.**org/11054006/<https://codereview.chromium.org/1... > > SVN Base: http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr... > > Affected files: > A chrome/browser/extensions/app_**host_installer_win.h > A chrome/browser/extensions/app_**host_installer_win.cc > M chrome/browser/extensions/crx_**installer.h > M chrome/browser/extensions/crx_**installer.cc > M chrome/browser/web_**applications/web_app_win.cc > M chrome/chrome_browser.gypi > M chrome/chrome_browser_**extensions.gypi > M chrome/installer/launcher_**support/chrome_launcher_**support.h > M chrome/installer/launcher_**support/chrome_launcher_**support.cc > > >
Yes, that appears to be the case.
I know you said work in progress, but here is some early feedback. https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app... File chrome/browser/extensions/app_host_installer_win.cc (right): https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_host_installer_win.cc:7: #include <windows.h> Some of these includes are in the .h https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_host_installer_win.cc:15: namespace { Keep the anonymous namespace out of the extensions namespace. https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_host_installer_win.cc:46: } // namespace Nit: Blank line before end of namespace. https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_host_installer_win.cc:57: DCHECK(!running_); Is running_ just for this DCHECK? https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app... File chrome/browser/extensions/app_host_installer_win.h (right): https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_host_installer_win.h:18: void EnsureAppHostPresentAndCall(const base::Closure& on_success, Please add a comment explaining that this can only be run on the FILE thread. Also, I think it might be better to have something like: bool IsAppHostInstallRequired(Extension*) as well as void InstallAppHost(...); Usage would be: ....... if (IsAppHostInstallRequired(extension)) { InstallAppHost(callbacks); return; } CompleteIt(); } That way the common case of not needing to install has a nice simple flow in the program. Whether you have two closures or one callback with a bool parameter indicating success is probably a matter of opinion; I would go with one. https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/crx... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/crx... chrome/browser/extensions/crx_installer.cc:515: #if defined(OS_WIN) This part of the codebase is platform independent. We can get rid of these #if checks and have this code all generic with some small tweaks. This would then let us add a similar concept to linux and / or mac later on. To do this app_host_installer_win.h should become app_host_installer.h, with it's implementation changing based on what platform we are on (either via #if or the linker, but #if should be fine for now). https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/crx... chrome/browser/extensions/crx_installer.cc:581: string16 error = L"Some random error message"; Ah, you did say work in progress.... https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/crx... File chrome/browser/extensions/crx_installer.h (right): https://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/crx... chrome/browser/extensions/crx_installer.h:227: void CompleteExtensionInstall(); Please come up with a better name for this function. There is CompleteInstall and CompleteExtensionInstall, how are they different? From a quick glance, maybe CompleteInstall should be CheckEnvironmentAndInstall, and CompleteExtensionInstall CompleteInstall. Or maybe you can come up with something better.
http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app_... File chrome/browser/extensions/app_host_installer_win.cc (right): http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app_... chrome/browser/extensions/app_host_installer_win.cc:33: on_failure_.Run(); Use process_util::GetTerminationStatus instead of GetExitCodeProcess and be sure to check the TerminationStatus. Only TERMINATION_STATUS_NORMAL_TERMINATION should call success handler. All others call the failure handler and LOG(ERROR) with the TerminationStatus and the exit code (if appropriate). http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app_... chrome/browser/extensions/app_host_installer_win.cc:49: : running_(false), process_(INVALID_HANDLE_VALUE), delegate_(NULL) { process_ and delegate_ are objects whose default constructors do the right thing in this case. No need to initialize them. http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app_... chrome/browser/extensions/app_host_installer_win.cc:57: DCHECK(!running_); On 2012/10/03 05:08:43, benwells wrote: > Is running_ just for this DCHECK? It could have been used to allow the AppHostInstaller instance to be re-used, but I don't think this is worthwhile. I would just DCHECK(!delegate_.get()) before line 69. Then make sure the class comments indicate that each instance should only be used once. http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app_... File chrome/browser/extensions/app_host_installer_win.h (right): http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/app_... chrome/browser/extensions/app_host_installer_win.h:18: void EnsureAppHostPresentAndCall(const base::Closure& on_success, On 2012/10/03 05:08:43, benwells wrote: > Please add a comment explaining that this can only be run on the FILE thread. > > Also, I think it might be better to have something like: > bool IsAppHostInstallRequired(Extension*) > > as well as > void InstallAppHost(...); > > Usage would be: > > ....... > if (IsAppHostInstallRequired(extension)) { > InstallAppHost(callbacks); > return; > } > > CompleteIt(); > } > > That way the common case of not needing to install has a nice simple flow in the > program. > > Whether you have two closures or one callback with a bool parameter indicating > success is probably a matter of opinion; I would go with one. Sure, a static IsAppHostRequired(Extension*) seems a good idea. http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/crx_... File chrome/browser/extensions/crx_installer.h (right): http://codereview.chromium.org/11054006/diff/1/chrome/browser/extensions/crx_... chrome/browser/extensions/crx_installer.h:227: void CompleteExtensionInstall(); On 2012/10/03 05:08:43, benwells wrote: > Please come up with a better name for this function. There is CompleteInstall > and CompleteExtensionInstall, how are they different? > > From a quick glance, maybe CompleteInstall should be CheckEnvironmentAndInstall, > and CompleteExtensionInstall CompleteInstall. Or maybe you can come up with > something better. Naming is hell. How about CompleteInstall->BeginInstall CompleteExtensionInstall->CompleteInstall http://codereview.chromium.org/11054006/diff/1/chrome/chrome_browser_extensio... File chrome/chrome_browser_extensions.gypi (right): http://codereview.chromium.org/11054006/diff/1/chrome/chrome_browser_extensio... chrome/chrome_browser_extensions.gypi:742: ['exclude', '^browser/extensions/app_host_installer_win.cc'], NB that you probably don't need to exclude these (I think _win.{h,cc} are auto-excluded on other platforms). The stuff below seems to indicate that you are right to add this, but I wonder if (a) I'm wrong or (b) someone else also unnecessarily added these. http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... chrome/installer/launcher_support/chrome_launcher_support.cc:136: FilePath setup_exe_path(GetSetupExeForInstallationLevel(level)); inline setup_exe_path http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... chrome/installer/launcher_support/chrome_launcher_support.cc:141: FilePath setup_exe_path(GetSetupExeFromRegistry(level, kAppHostAppId)); inline setup_exe_path http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... chrome/installer/launcher_support/chrome_launcher_support.cc:171: bool LaunchQuickEnableAppHost(base::win::ScopedHandle* process) { This doesn't really belong here. Move it into app_host_installer_win.cc http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... File chrome/installer/launcher_support/chrome_launcher_support.h (right): http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... chrome/installer/launcher_support/chrome_launcher_support.h:8: #include "base/win/scoped_handle.h" forward-decl http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... chrome/installer/launcher_support/chrome_launcher_support.h:45: FilePath GetAnyAppHostPath(); Add behaviour analogous to GetAnyChromePath's: // In non-official builds, to ease development, this will first look for a // chrome.exe in the same directory as the current executable. http://codereview.chromium.org/11054006/diff/1/chrome/installer/launcher_supp... chrome/installer/launcher_support/chrome_launcher_support.h:47: // Returns true if App Host is installed (system-level or user-level). or in the same directory as the current executable.
Still WIP, but please review. Some main changes: - Implemented code for the 2 other entry points in unpacked_installer.cc - Moved OS_WIN-specific changes within app_host_installer.* - Made decision regarding usage of base::Unretained(). TODO: - Testing code and writing tests. - Error messages on failed App Host install. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... File chrome/browser/extensions/app_host_installer_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.cc:7: #include <windows.h> On 2012/10/03 05:08:43, benwells wrote: > Some of these includes are in the .h I thought a .cc file should not assume transitive inclusion? Please confirm, thanks! https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.cc:15: namespace { On 2012/10/03 05:08:43, benwells wrote: > Keep the anonymous namespace out of the extensions namespace. Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.cc:33: on_failure_.Run(); On 2012/10/03 15:58:35, erikwright wrote: > Use process_util::GetTerminationStatus instead of GetExitCodeProcess and be sure > to check the TerminationStatus. > > Only TERMINATION_STATUS_NORMAL_TERMINATION should call success handler. All > others call the failure handler and LOG(ERROR) with the TerminationStatus and > the exit code (if appropriate). Done. I think TerminationStatus is sufficient; we don't need the exit_code. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.cc:46: } // namespace On 2012/10/03 05:08:43, benwells wrote: > Nit: Blank line before end of namespace. Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.cc:49: : running_(false), process_(INVALID_HANDLE_VALUE), delegate_(NULL) { On 2012/10/03 15:58:35, erikwright wrote: > process_ and delegate_ are objects whose default constructors do the right thing > in this case. No need to initialize them. Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.cc:57: DCHECK(!running_); On 2012/10/03 05:08:43, benwells wrote: > Is running_ just for this DCHECK? I'm worried about the case where user installs app A, and before quick-enable-app-host runs, installs app B again! Possible outcomes: - There is already built-in mechanism that only allows one app install at a time? (Don't know). - We will have block this explicitly. - Installer is smart enough to handle simultaneous installs? (Don't know). We'll need to revisit this later. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.cc:57: DCHECK(!running_); On 2012/10/03 15:58:35, erikwright wrote: > On 2012/10/03 05:08:43, benwells wrote: > > Is running_ just for this DCHECK? > > It could have been used to allow the AppHostInstaller instance to be re-used, > but I don't think this is worthwhile. > > I would just DCHECK(!delegate_.get()) before line 69. > > Then make sure the class comments indicate that each instance should only be > used once. Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... File chrome/browser/extensions/app_host_installer_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.h:18: void EnsureAppHostPresentAndCall(const base::Closure& on_success, On 2012/10/03 05:08:43, benwells wrote: > Please add a comment explaining that this can only be run on the FILE thread. > > Also, I think it might be better to have something like: > bool IsAppHostInstallRequired(Extension*) > > as well as > void InstallAppHost(...); > > Usage would be: > > ....... > if (IsAppHostInstallRequired(extension)) { > InstallAppHost(callbacks); > return; > } > > CompleteIt(); > } > > That way the common case of not needing to install has a nice simple flow in the > program. > > Whether you have two closures or one callback with a bool parameter indicating > success is probably a matter of opinion; I would go with one. Done, but added another function OnAppHostInstallation(bool) as callback. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host_installer_win.h:18: void EnsureAppHostPresentAndCall(const base::Closure& on_success, On 2012/10/03 15:58:35, erikwright wrote: > On 2012/10/03 05:08:43, benwells wrote: > > Please add a comment explaining that this can only be run on the FILE thread. > > > > Also, I think it might be better to have something like: > > bool IsAppHostInstallRequired(Extension*) > > > > as well as > > void InstallAppHost(...); > > > > Usage would be: > > > > ....... > > if (IsAppHostInstallRequired(extension)) { > > InstallAppHost(callbacks); > > return; > > } > > > > CompleteIt(); > > } > > > > That way the common case of not needing to install has a nice simple flow in > the > > program. > > > > Whether you have two closures or one callback with a bool parameter indicating > > success is probably a matter of opinion; I would go with one. > > Sure, a static IsAppHostRequired(Extension*) seems a good idea. Done (Made static). https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/crx_installer.cc:515: #if defined(OS_WIN) On 2012/10/03 05:08:43, benwells wrote: > This part of the codebase is platform independent. We can get rid of these #if > checks and have this code all generic with some small tweaks. This would then > let us add a similar concept to linux and / or mac later on. > > To do this app_host_installer_win.h should become app_host_installer.h, with > it's implementation changing based on what platform we are on (either via #if or > the linker, but #if should be fine for now). > Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/crx_installer.cc:581: string16 error = L"Some random error message"; On 2012/10/03 05:08:43, benwells wrote: > Ah, you did say work in progress.... Yup. I moved this routine. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... File chrome/browser/extensions/crx_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/crx_installer.h:227: void CompleteExtensionInstall(); On 2012/10/03 15:58:35, erikwright wrote: > On 2012/10/03 05:08:43, benwells wrote: > > Please come up with a better name for this function. There is CompleteInstall > > and CompleteExtensionInstall, how are they different? > > > > From a quick glance, maybe CompleteInstall should be > CheckEnvironmentAndInstall, > > and CompleteExtensionInstall CompleteInstall. Or maybe you can come up with > > something better. > > Naming is hell. > > How about > > CompleteInstall->BeginInstall > CompleteExtensionInstall->CompleteInstall Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/browser/extensi... chrome/browser/extensions/crx_installer.h:227: void CompleteExtensionInstall(); On 2012/10/03 15:58:35, erikwright wrote: > On 2012/10/03 05:08:43, benwells wrote: > > Please come up with a better name for this function. There is CompleteInstall > > and CompleteExtensionInstall, how are they different? > > > > From a quick glance, maybe CompleteInstall should be > CheckEnvironmentAndInstall, > > and CompleteExtensionInstall CompleteInstall. Or maybe you can come up with > > something better. > > Naming is hell. > > How about > > CompleteInstall->BeginInstall > CompleteExtensionInstall->CompleteInstall Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/chrome_browser_... File chrome/chrome_browser_extensions.gypi (right): https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/chrome_browser_... chrome/chrome_browser_extensions.gypi:742: ['exclude', '^browser/extensions/app_host_installer_win.cc'], On 2012/10/03 15:58:35, erikwright wrote: > NB that you probably don't need to exclude these (I think _win.{h,cc} are > auto-excluded on other platforms). > > The stuff below seems to indicate that you are right to add this, but I wonder > if (a) I'm wrong or (b) someone else also unnecessarily added these. Since we're make this non-specific to window, this is removed regardless. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:136: FilePath setup_exe_path(GetSetupExeForInstallationLevel(level)); On 2012/10/03 15:58:35, erikwright wrote: > inline setup_exe_path Done, but now FindExeRelativeToSetupExe() requires pass-by-value. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:141: FilePath setup_exe_path(GetSetupExeFromRegistry(level, kAppHostAppId)); On 2012/10/03 15:58:35, erikwright wrote: > inline setup_exe_path Done. Same as above. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:171: bool LaunchQuickEnableAppHost(base::win::ScopedHandle* process) { On 2012/10/03 15:58:35, erikwright wrote: > This doesn't really belong here. > > Move it into app_host_installer_win.cc Done, and removed useless #include's, but now GetQuickEnableAppHostCommandFromRegistry() is exposed. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.h:8: #include "base/win/scoped_handle.h" On 2012/10/03 15:58:35, erikwright wrote: > forward-decl The definition was typedef GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle; and there does not seem to be instances of forward declaration (searched for "ScopedHandle;"). But this is moot now anyway (moved LaunchQuickEnableAppHost()) https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.h:45: FilePath GetAnyAppHostPath(); On 2012/10/03 15:58:35, erikwright wrote: > Add behaviour analogous to GetAnyChromePath's: > > // In non-official builds, to ease development, this will first look for a > // chrome.exe in the same directory as the current executable. Done. https://chromiumcodereview.appspot.com/11054006/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.h:47: // Returns true if App Host is installed (system-level or user-level). On 2012/10/03 15:58:35, erikwright wrote: > or in the same directory as the current executable. Done.
https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:7: #include <windows.h> #ifdef OS_WIN https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:38: LOG(ERROR) << "App Host install failed, status = " << status; log the exit code if appropriate https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:93: using content::BrowserThread; is the using necessary? https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:105: callback.Run(true); NOTREACHED() << "Attempt to install the app host on an unsupported platform."; callback.Run(false); https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:8: #include "base/bind.h" include callback_fwd.h instead https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:32: scoped_ptr<base::win::ObjectWatcher::Delegate> delegate_; include scoped_ptr https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:35: DISALLOW_COPY_AND_ASSIGN(AppHostInstaller); include basictypes.h https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:513: AddRef(); // Balanced in OnAppHostInstallation(). I don't think that this is required. You're binding a ref_ptr to this in the callback, so you are already AddRef'd. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/unpacked_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/unpacked_installer.h:91: // Callback founction for AppHostInstaller. founction https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:159: string16 GetQuickEnableAppHostCommand(InstallationLevel level) { This does not belong here. Where you are calling from (in Chrome) you should be able to use installer::AppCommands::Initialize (chrome/installer/util/app_commands.h)
Please check again. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:7: #include <windows.h> On 2012/10/03 20:57:07, erikwright wrote: > #ifdef OS_WIN Done. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:38: LOG(ERROR) << "App Host install failed, status = " << status; On 2012/10/03 20:57:07, erikwright wrote: > log the exit code if appropriate Done. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:93: using content::BrowserThread; On 2012/10/03 20:57:07, erikwright wrote: > is the using necessary? Done (removed, added content::). https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.cc:105: callback.Run(true); On 2012/10/03 20:57:07, erikwright wrote: > NOTREACHED() << "Attempt to install the app host on an unsupported platform."; > callback.Run(false); Done. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:8: #include "base/bind.h" On 2012/10/03 20:57:07, erikwright wrote: > include callback_fwd.h instead Done. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:32: scoped_ptr<base::win::ObjectWatcher::Delegate> delegate_; On 2012/10/03 20:57:07, erikwright wrote: > include scoped_ptr Done. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:35: DISALLOW_COPY_AND_ASSIGN(AppHostInstaller); On 2012/10/03 20:57:07, erikwright wrote: > include basictypes.h Done. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:513: AddRef(); // Balanced in OnAppHostInstallation(). On 2012/10/03 20:57:07, erikwright wrote: > I don't think that this is required. You're binding a ref_ptr to this in the > callback, so you are already AddRef'd. Done; deleted corresponding Release(), too. https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... File chrome/browser/extensions/unpacked_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/browser/exte... chrome/browser/extensions/unpacked_installer.h:91: // Callback founction for AppHostInstaller. On 2012/10/03 20:57:07, erikwright wrote: > founction Done (oops). https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/3002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:159: string16 GetQuickEnableAppHostCommand(InstallationLevel level) { On 2012/10/03 20:57:07, erikwright wrote: > This does not belong here. > > Where you are calling from (in Chrome) you should be able to use > installer::AppCommands::Initialize (chrome/installer/util/app_commands.h) Moved code to app_host_installer.cc, but using AppCommands only increases complexity with no gain (should discuss this).
https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:47: void OnObjectSignaled(HANDLE object) { virtual ... OVERRIDE #include compiler_specific https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:68: // App Host's "ClientState" registry key. "returns an empty string if the path does not exist or cannot be read." https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:130: if (FAILED(LaunchQuickEnableAppHost(&process_))) { FAILED is for HRESULTS. if (!Launch...()) {...} https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:135: delegate_.reset(new QuickEnableWatcher(callback)); Although I think it's technically OK, I'd like to see the watcher_ stopped before delegate_ is reset. At a glance, it looks as though the watcher could reference the freed delegate somewhere between 135 and 136. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:11: #include "base/win/object_watcher.h" You'll need #if's for base/win/* https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:520: void CrxInstaller::OnAppHostInstallation(bool success) { OnAppHostInstallationComplete https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:219: // proceeds to post CompleteExtensionInstall(). CompleteInstall https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:222: // Callback founction for AppHostInstaller. founction https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/unpacked_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:77: if (service_weak_.get()) { Remove this check, remove the extension_service member and constructor parameter. See my comment below to add the check. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:298: PermissionsUpdater perms_updater(service_weak_->profile()); Do a check on service_weak_ around lines 298-303. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:298: PermissionsUpdater perms_updater(service_weak_->profile()); This used to all happen on the UI thread. Have you checked that it is safe to do on the file thread too? https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:135: // For development mode, app_host.exe should be in same dir as the stub. "as the stub" -> "as chrome.exe" https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.h:8: #include "base/string16.h" no longer required.
https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:12: #include "chrome/common/extensions/extension.h" Nit: Move includes like string16, logging, process_util and browser_thread into the #if. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:43: : callback_(callback) { Nit: {} (on one line) https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:49: base::TerminationStatus status( I'm not at all familiar withe ObjectWatcher. What happens if the process just hangs and doesn't terminate? Is there some sort of timeout? https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:112: AppHostInstaller::AppHostInstaller() { Nit: {} https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:11: #include "base/win/object_watcher.h" On 2012/10/04 01:28:16, erikwright wrote: > You'll need #if's for base/win/* You also don't need to include scoped_ptr unless it's win. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:23: static bool IsAppHostInstallRequired(const Extension& extension); If these functions need to be called on the FILE thread, please mention it. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:514: base::Bind(&CrxInstaller::OnAppHostInstallation, this)); early return here and remove the else https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:522: CompleteInstall(); Early return and remove the else https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:524: // TODO(huangs): Error message. I take it you'll remove this TODO / fix the error before committing? https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:218: // Runs on File thread. Ensures that App Host is installed, and then Nit: File -> FILE. Nit: This function does more than what the comment describes (it also checks minimum version), so the comment is a little misleading as is. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:224: Nit: File -> FILE https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/unpacked_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:280: base::Bind(&UnpackedInstaller::OnAppHostInstallation, this)); early return and remove the else. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:288: CompleteInstall(); early return and remove the else. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/unpacked_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.h:94: // Runs on File thread. Ensures that App Host is installed, and then Nit: File -> FILE https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.h:101: // Runs on File thread. Installs the unpacked extension into the profile and Nit: File -> FILE https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/chrome_brow... File chrome/chrome_browser_extensions.gypi (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/chrome_brow... chrome/chrome_browser_extensions.gypi:319: 'browser/extensions/app_host_installer.h', BTW Erik was right, _win files are only included in win builds.
Thanks to erikwright@, for laying out the code for app_host_installer.cc for me! Main changes: - Changed to single interface: InstallAppHostIfNecessaryThenCall(). This is because registry calls need to be in FILE thread, but we don't want the callers to deal with this. The callback is in caller thread now, too. - Added error strings and messages. - Documented app_host_installer.cc. Tested this on UI, and it appears to invoke quick-enable-application-host correctly (although I still need MSBuild to work). unpacked_installer has ugly error message UI though. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:12: #include "chrome/common/extensions/extension.h" On 2012/10/04 07:53:01, benwells wrote: > Nit: Move includes like string16, logging, process_util and browser_thread into > the #if. Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:43: : callback_(callback) { On 2012/10/04 07:53:01, benwells wrote: > Nit: {} (on one line) Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:47: void OnObjectSignaled(HANDLE object) { On 2012/10/04 01:28:16, erikwright wrote: > virtual ... OVERRIDE > #include compiler_specific Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:49: base::TerminationStatus status( On 2012/10/04 07:53:01, benwells wrote: > I'm not at all familiar withe ObjectWatcher. What happens if the process just > hangs and doesn't terminate? Is there some sort of timeout? If process crashes, then it will return. If it runs indefinitely, then there is no timeout. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:68: // App Host's "ClientState" registry key. On 2012/10/04 01:28:16, erikwright wrote: > "returns an empty string if the path does not exist or cannot be read." Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:112: AppHostInstaller::AppHostInstaller() { On 2012/10/04 07:53:01, benwells wrote: > Nit: {} Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:130: if (FAILED(LaunchQuickEnableAppHost(&process_))) { On 2012/10/04 01:28:16, erikwright wrote: > FAILED is for HRESULTS. > > if (!Launch...()) {...} Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:135: delegate_.reset(new QuickEnableWatcher(callback)); On 2012/10/04 01:28:16, erikwright wrote: > Although I think it's technically OK, I'd like to see the watcher_ stopped > before delegate_ is reset. At a glance, it looks as though the watcher could > reference the freed delegate somewhere between 135 and 136. Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:11: #include "base/win/object_watcher.h" On 2012/10/04 01:28:16, erikwright wrote: > You'll need #if's for base/win/* Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:11: #include "base/win/object_watcher.h" On 2012/10/04 07:53:01, benwells wrote: > On 2012/10/04 01:28:16, erikwright wrote: > > You'll need #if's for base/win/* > > You also don't need to include scoped_ptr unless it's win. Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:23: static bool IsAppHostInstallRequired(const Extension& extension); On 2012/10/04 07:53:01, benwells wrote: > If these functions need to be called on the FILE thread, please mention it. Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:514: base::Bind(&CrxInstaller::OnAppHostInstallation, this)); On 2012/10/04 07:53:01, benwells wrote: > early return here and remove the else Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:520: void CrxInstaller::OnAppHostInstallation(bool success) { On 2012/10/04 01:28:16, erikwright wrote: > OnAppHostInstallationComplete Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:522: CompleteInstall(); On 2012/10/04 07:53:01, benwells wrote: > Early return and remove the else Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:524: // TODO(huangs): Error message. On 2012/10/04 07:53:01, benwells wrote: > I take it you'll remove this TODO / fix the error before committing? Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:218: // Runs on File thread. Ensures that App Host is installed, and then On 2012/10/04 07:53:01, benwells wrote: > Nit: File -> FILE. > > Nit: This function does more than what the comment describes (it also checks > minimum version), so the comment is a little misleading as is. Done. Also fixed every instance of "file thread" in file. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:219: // proceeds to post CompleteExtensionInstall(). On 2012/10/04 01:28:16, erikwright wrote: > CompleteInstall Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:222: // Callback founction for AppHostInstaller. On 2012/10/04 01:28:16, erikwright wrote: > founction Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/crx_installer.h:224: On 2012/10/04 07:53:01, benwells wrote: > Nit: File -> FILE Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/unpacked_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:77: if (service_weak_.get()) { On 2012/10/04 01:28:16, erikwright wrote: > Remove this check, remove the extension_service member and constructor > parameter. > > See my comment below to add the check. Keeping this the way it was, now that we changed interface to AppHostInstaller. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:280: base::Bind(&UnpackedInstaller::OnAppHostInstallation, this)); On 2012/10/04 07:53:01, benwells wrote: > early return and remove the else. Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:288: CompleteInstall(); On 2012/10/04 07:53:01, benwells wrote: > early return and remove the else. Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:298: PermissionsUpdater perms_updater(service_weak_->profile()); On 2012/10/04 01:28:16, erikwright wrote: > Do a check on service_weak_ around lines 298-303. No check, in light of changed interface to AppHostInstaller. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:298: PermissionsUpdater perms_updater(service_weak_->profile()); On 2012/10/04 01:28:16, erikwright wrote: > This used to all happen on the UI thread. Have you checked that it is safe to do > on the file thread too? Changing interface to AppHostInstaller, so still in UI thread here. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... File chrome/browser/extensions/unpacked_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.h:94: // Runs on File thread. Ensures that App Host is installed, and then On 2012/10/04 07:53:01, benwells wrote: > Nit: File -> FILE Done (for the file). https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.h:101: // Runs on File thread. Installs the unpacked extension into the profile and On 2012/10/04 07:53:01, benwells wrote: > Nit: File -> FILE Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/chrome_brow... File chrome/chrome_browser_extensions.gypi (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/chrome_brow... chrome/chrome_browser_extensions.gypi:319: 'browser/extensions/app_host_installer.h', On 2012/10/04 07:53:01, benwells wrote: > BTW Erik was right, _win files are only included in win builds. Okay (no change). https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:135: // For development mode, app_host.exe should be in same dir as the stub. On 2012/10/04 01:28:16, erikwright wrote: > "as the stub" -> "as chrome.exe" Done. https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://chromiumcodereview.appspot.com/11054006/diff/13001/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.h:8: #include "base/string16.h" On 2012/10/04 01:28:16, erikwright wrote: > no longer required. Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/app/generat... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/app/generat... chrome/app/generated_resources.grd:207: + New site permissions settings will take effect after reloading the page. Trailing white spaces were trimmed by my editor. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/app/generat... chrome/app/generated_resources.grd:4984: + <message name="IDS_EXTENSION_APP_HOST_INSTALL_ERROR" desc="Message for when an App Host fails to install."> This is the only main change. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:31: AppHostInstallerImpl* impl_; Not using scoped_ptr for this, since AppHostInstallerImpl is hidden (in particular, no destructor).
https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:126: class AppHostInstallerImpl { Move AppHostInstallerImpl to app_host_installer_impl_win.{h,cc} https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:131: void InstallAppHostIfNecessaryThenCall( Rename function and parameter. "Implementation of" -> Implements https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:136: remove blank line https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:138: void InstallAppHostIfNecessaryOnFileThreadThenCall( rename as elsewhere. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:147: void PostResultToCallerThread(BrowserThread::ID thread_id, This method and the next one could actually be free functions in a private namespace to simplify the class declaration. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:148: const base::Callback<void(bool)>& final_step, final_step -> original_completion_callback https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:176: base::Bind(&AppHostInstallerImpl::PostResultToCallerThread, Need a comment to explain why this callback can never be invoked after AppHostInstallerImpl is destroyed. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:236: class AppHostInstallerImpl { No impl required in the else case - see below... https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:255: AppHostInstaller::AppHostInstaller() : impl_(new AppHostInstallerImpl()) {} #if win, new AppHostInstallerImpl #else 0 https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:258: delete impl_; if impl_ delete impl_ https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:264: impl_->InstallAppHostIfNecessaryThenCall(extension, final_step); if impl_ impl_->... else final_step.Run(true) https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:21: // Installs App Host, then calls (directly or via post) |next_step| Installs the App Host if it is required and not present for the given extension on the current platform. Calls |completion_callback| after checks and installation (if necessary) are complete, with a boolean which will be false iff the installation was required but failed. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:25: void AppHostInstaller::InstallAppHostIfNecessaryThenCall( Remove the "ThenCall". The presence of a callback in the signature, appropriately named and commented, will be sufficient. Rename 'final_step' to callback or completion_callback.
minor drive-by q's. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:61: subkey.append(1, L'\\').append(app_guid); this is here for the sake of the change on line 24. why is this a good thing? https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:149: return !app_host_exe.empty() && file_util::PathExists(app_host_exe); is this call to PathExists not superfluous?
Please take another look. I implemented a new flow in app_host_installer_impl_win.cc (old flow was in previous revision of app_host_installer.cc). The main idea is that each member function moves to the right thread on-demand (by pseudo-self-recursion). This way, each member function need not prepare for the other member functions' behaviours, thus significantly simplifying the code. gab@ and grt@ pointed out that caveats may include: - Using BrowserThread to dictate control flow is usual. - Would this require significant overhead? - What about unit tests? https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:126: class AppHostInstallerImpl { On 2012/10/05 02:21:56, erikwright wrote: > Move AppHostInstallerImpl to app_host_installer_impl_win.{h,cc} Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:131: void InstallAppHostIfNecessaryThenCall( On 2012/10/05 02:21:56, erikwright wrote: > Rename function and parameter. > > "Implementation of" -> Implements Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:136: On 2012/10/05 02:21:56, erikwright wrote: > remove blank line Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:138: void InstallAppHostIfNecessaryOnFileThreadThenCall( On 2012/10/05 02:21:56, erikwright wrote: > rename as elsewhere. Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:147: void PostResultToCallerThread(BrowserThread::ID thread_id, On 2012/10/05 02:21:56, erikwright wrote: > This method and the next one could actually be free functions in a private > namespace to simplify the class declaration. Yes, but now it needs to be move elsewhere, which breaks the linear reading flow. But the function is removed anyway in the new flow. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:148: const base::Callback<void(bool)>& final_step, On 2012/10/05 02:21:56, erikwright wrote: > final_step -> original_completion_callback Moot in the new flow, since there is only one completion_callback now. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:176: base::Bind(&AppHostInstallerImpl::PostResultToCallerThread, On 2012/10/05 02:21:56, erikwright wrote: > Need a comment to explain why this callback can never be invoked after > AppHostInstallerImpl is destroyed. Moot, since this is unused in the new flow. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:236: class AppHostInstallerImpl { On 2012/10/05 02:21:56, erikwright wrote: > No impl required in the else case - see below... Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:255: AppHostInstaller::AppHostInstaller() : impl_(new AppHostInstallerImpl()) {} On 2012/10/05 02:21:56, erikwright wrote: > #if win, new AppHostInstallerImpl > #else 0 Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:258: delete impl_; On 2012/10/05 02:21:56, erikwright wrote: > if impl_ > delete impl_ Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:264: impl_->InstallAppHostIfNecessaryThenCall(extension, final_step); On 2012/10/05 02:21:56, erikwright wrote: > if impl_ > impl_->... > else > final_step.Run(true) Done, but need to use #if define(OS_WIN), since we eliminated the stub implementation. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:21: // Installs App Host, then calls (directly or via post) |next_step| On 2012/10/05 02:21:56, erikwright wrote: > Installs the App Host if it is required and not present for the given extension > on the current platform. > > Calls |completion_callback| after checks and installation (if necessary) are > complete, with a boolean which will be false iff the installation was required > but failed. Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:25: void AppHostInstaller::InstallAppHostIfNecessaryThenCall( On 2012/10/05 02:21:56, erikwright wrote: > Remove the "ThenCall". > > The presence of a callback in the signature, appropriately named and commented, > will be sufficient. > > Rename 'final_step' to callback or completion_callback. Done. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:61: subkey.append(1, L'\\').append(app_guid); On 2012/10/05 02:53:05, grt wrote: > this is here for the sake of the change on line 24. why is this a good thing? I think k*Key[] variables should not end with '\\'. The .append(1, L'\\') exists all over anyway. https://chromiumcodereview.appspot.com/11054006/diff/16002/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:149: return !app_host_exe.empty() && file_util::PathExists(app_host_exe); On 2012/10/05 02:53:05, grt wrote: > is this call to PathExists not superfluous? Yes it is. Removed!
Just a quick look at the main area for risk. I think you risk use-after-frees if the AppHostInstaller is freed up while in action. I think the AppHostInstallerImpl should probably have a private WeakPtrFactory for itself. It should bind its callbacks to itself using a weak ptr, and thus the callbacks become safely NULL if it (and the WeakPtrFactory) is deleted. But you should double-check the thread from Will Chan about WeakPtr usage and see if this was one of the cases he endorsed or recommended against. https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:7: #include "base/bind.h" not required. https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:9: #include "base/compiler_specific.h" not required https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:10: #include "chrome/common/extensions/extension.h" not required https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:37: class QuickEnableWatcher : public base::win::ObjectWatcher::Delegate { Add class comments. https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:115: if (!BrowserThread::GetCurrentThreadIdentifier(&caller_thread_id_)) { DCHECK completion_callback_.IsNull() https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:137: base::Unretained(this))); Can't the impl potentially be deleted before this callback is invoked? Then you'll have a use-after-free? https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:151: base::Bind(&AppHostInstallerImpl::Finish, base::Unretained(this)))); Here I think the Unretained is probably safe, but you need to check and document that if it stays this way. https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:161: &AppHostInstallerImpl::Finish, base::Unretained(this), success)); Same concern here about Unretained.
Can we make AppHostInstallerImpl manage its own lifecycle instead, i.e. gets allocated InstallAppHostIfNecessary() and "forgotten", but upon completion (which we know occurs in AppHostInstallerImpl::Finish), it deletes itself? SimpleExtensionLoadPrompt in unpacked_installer.cc has a similar pattern.
Please check again! https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:7: #include "base/bind.h" On 2012/10/05 21:18:23, erikwright wrote: > not required. Done. https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:9: #include "base/compiler_specific.h" On 2012/10/05 21:18:23, erikwright wrote: > not required Done. https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:10: #include "chrome/common/extensions/extension.h" On 2012/10/05 21:18:23, erikwright wrote: > not required Done (forward declared). https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:37: class QuickEnableWatcher : public base::win::ObjectWatcher::Delegate { On 2012/10/05 21:18:23, erikwright wrote: > Add class comments. Done. https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:115: if (!BrowserThread::GetCurrentThreadIdentifier(&caller_thread_id_)) { On 2012/10/05 21:18:23, erikwright wrote: > DCHECK completion_callback_.IsNull() I don't understand this (callback.h has no IsNull()); please clarify! https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:137: base::Unretained(this))); On 2012/10/05 21:18:23, erikwright wrote: > Can't the impl potentially be deleted before this callback is invoked? Then > you'll have a use-after-free? I'm letting AppHostInstallerImpl manage its own lifecycle; we'll iterate on this! https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:151: base::Bind(&AppHostInstallerImpl::Finish, base::Unretained(this)))); On 2012/10/05 21:18:23, erikwright wrote: > Here I think the Unretained is probably safe, but you need to check and document > that if it stays this way. If AppHostInstallerImpl deletes itself, then this is safe. Note that both callers (CrxInstaller and UnpackedInstaller) extend base::RefCountedThreadSafe<>, so I think they are guaranteed to live while AppHostInstallerImpl is running (please verify). https://chromiumcodereview.appspot.com/11054006/diff/24003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:161: &AppHostInstallerImpl::Finish, base::Unretained(this), success)); On 2012/10/05 21:18:23, erikwright wrote: > Same concern here about Unretained. Ditto.
http://codereview.chromium.org/11054006/diff/8013/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/11054006/diff/8013/chrome/app/generated_resour... chrome/app/generated_resources.grd:4985: + Could not install App Host. This seems a bit terse / technical. Will users know what the App Host is? Is there any action they can take? http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/a... File chrome/browser/extensions/app_host_installer.h (right): http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/a... chrome/browser/extensions/app_host_installer.h:24: void AppHostInstaller::InstallAppHostIfNecessary( Can this just be a static function in a namespace? If not, lose the AppHostInstaller:: from the declaration. http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/a... chrome/browser/extensions/app_host_installer.h:26: const base::Callback<void(bool)>& completion_callback); I know I suggested adding the extension here, but it feels a bit wrong somehow. If it wasn't here, this file and the implementation could move somewhere else, which would make me happy. It does not feel right in extensions. http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/c... File chrome/browser/extensions/crx_installer.cc (right): http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/c... chrome/browser/extensions/crx_installer.cc:512: app_host_installer_.InstallAppHostIfNecessary(*extension_, Always going through this function and its callbacks obfuscates the flow of installation for all extensions - most of which don't need the app host. This is the main reason I suggested having the IsAppHostRequired function (the other reason is that something similar will be needed for the app launcher). Putting something back like: if (something) { do this stuff with callbacks return; } CompleteInstall(); will make the flow nice and simple again for extensions which don't need the app host. "something" could be calling into the app_host_installation code, or maybe even better could just call a new function extension->RequiresAppHost(), keeping the app host installation code completely decoupled from extensions. http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/c... chrome/browser/extensions/crx_installer.cc:523: } Blank line here after early return. http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/u... File chrome/browser/extensions/unpacked_installer.cc (right): http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/u... chrome/browser/extensions/unpacked_installer.cc:278: } Nit: Blank line here after early return.
http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/a... File chrome/browser/extensions/app_host_installer.h (right): http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/a... chrome/browser/extensions/app_host_installer.h:26: const base::Callback<void(bool)>& completion_callback); On 2012/10/08 06:14:33, benwells wrote: > I know I suggested adding the extension here, but it feels a bit wrong somehow. > If it wasn't here, this file and the implementation could move somewhere else, > which would make me happy. It does not feel right in extensions. We can push the is_platform_app() check to call sites, but that doesn't seem right either. Currently the app_host_installer is in two parts - the basic outside part and the platform-specific part. The check for is_platform_app() could be kept in the outside part, and that could stay here as a static helper function. The platform-specific part could then be moved somewhere else. I'd like to defer the actual question of where, while I think about it some more. http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/c... File chrome/browser/extensions/crx_installer.cc (right): http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/c... chrome/browser/extensions/crx_installer.cc:512: app_host_installer_.InstallAppHostIfNecessary(*extension_, On 2012/10/08 06:14:33, benwells wrote: > Always going through this function and its callbacks obfuscates the flow of > installation for all extensions - most of which don't need the app host. This is > the main reason I suggested having the IsAppHostRequired function (the other > reason is that something similar will be needed for the app launcher). > > Putting something back like: > if (something) { > do this stuff with callbacks > return; > } > > CompleteInstall(); > > will make the flow nice and simple again for extensions which don't need the app > host. > > "something" could be calling into the app_host_installation code, or maybe even > better could just call a new function extension->RequiresAppHost(), keeping the > app host installation code completely decoupled from extensions. Part of the checking has to occur on the FILE thread. It's true that for non-Windows platforms we don't need to go to the FILE thread, but I think it is more appropriate to encapsulate this detail in the AppHost specific code, rather than requiring each of the clients to be aware of it. The AppHost code Does The Right Thing, which is to say it doesn't bounce across threads unnecessarily on unsupported platforms.
https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/app/generate... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/app/generate... chrome/app/generated_resources.grd:4985: + Could not install App Host. On 2012/10/08 06:14:33, benwells wrote: > This seems a bit terse / technical. Will users know what the App Host is? Is > there any action they can take? Changed this a bit. Will need to iterate on this, still. https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:24: void AppHostInstaller::InstallAppHostIfNecessary( On 2012/10/08 06:14:33, benwells wrote: > Can this just be a static function in a namespace? > > If not, lose the AppHostInstaller:: from the declaration. Done. https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... chrome/browser/extensions/app_host_installer.h:26: const base::Callback<void(bool)>& completion_callback); On 2012/10/09 15:54:53, erikwright wrote: > On 2012/10/08 06:14:33, benwells wrote: > > I know I suggested adding the extension here, but it feels a bit wrong > somehow. > > If it wasn't here, this file and the implementation could move somewhere else, > > which would make me happy. It does not feel right in extensions. > > We can push the is_platform_app() check to call sites, but that doesn't seem > right either. > > Currently the app_host_installer is in two parts - the basic outside part and > the platform-specific part. > > The check for is_platform_app() could be kept in the outside part, and that > could stay here as a static helper function. > > The platform-specific part could then be moved somewhere else. > > I'd like to defer the actual question of where, while I think about it some > more. Deferring. Once we have a decision, will move the file as part of the final change, to not disrupt diffs. https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:512: app_host_installer_.InstallAppHostIfNecessary(*extension_, On 2012/10/09 15:54:53, erikwright wrote: > On 2012/10/08 06:14:33, benwells wrote: > > Always going through this function and its callbacks obfuscates the flow of > > installation for all extensions - most of which don't need the app host. This > is > > the main reason I suggested having the IsAppHostRequired function (the other > > reason is that something similar will be needed for the app launcher). > > > > Putting something back like: > > if (something) { > > do this stuff with callbacks > > return; > > } > > > > CompleteInstall(); > > > > will make the flow nice and simple again for extensions which don't need the > app > > host. > > > > "something" could be calling into the app_host_installation code, or maybe > even > > better could just call a new function extension->RequiresAppHost(), keeping > the > > app host installation code completely decoupled from extensions. > > Part of the checking has to occur on the FILE thread. > > It's true that for non-Windows platforms we don't need to go to the FILE thread, > but I think it is more appropriate to encapsulate this detail in the AppHost > specific code, rather than requiring each of the clients to be aware of it. > > The AppHost code Does The Right Thing, which is to say it doesn't bounce across > threads unnecessarily on unsupported platforms. Some minor rearrangement for style; otherwise not changing the interface. https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:523: } On 2012/10/08 06:14:33, benwells wrote: > Blank line here after early return. Done. https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... File chrome/browser/extensions/unpacked_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/8013/chrome/browser/exte... chrome/browser/extensions/unpacked_installer.cc:278: } On 2012/10/08 06:14:33, benwells wrote: > Nit: Blank line here after early return. Done (also for OnLoaded()).
https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/app/generat... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/app/generat... chrome/app/generated_resources.grd:4984: + <message name="IDS_EXTENSION_APP_HOST_INSTALL_ERROR" desc="Message for when an App Host fails to install."> There are a couple options here: (1) Really generic message such as "Application installation failed". (2) Less generic message such as "Application Launcher installation failed" or "Application Runtime installation failed." (3) One of the above with a link to a knowledge-base article. (4) Actually capture the installer error string and/or error code from the App Launcher setup.exe process. I would suggest (1) or (2) for now, a histogram to track failures in the field, and log a task for (4). If the histogram actually indicates a non-trivial number of failures, a knowledge-base article can be written and linked to from the failure message. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:22: // The implementation. Go ahead and do the is_platform_app() check here, and then don't pass extension to impl https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:23: AppHostInstallerImpl* impl = new AppHostInstallerImpl(); Make the constructor and destructor of AppHostInstallerImpl private, make InstallAppHostIfNecessary a static function. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:8: #include "base/basictypes.h" not needed, right? https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:19: // App Host installation is necessary if This level of detail is inappropriate for the declaration. Use a declarative style, and stick to the details clients care about: // Determines if the App Host is installed and, if not, installs it. Other details can be moved to the implementation. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:30: AppHostInstallerImpl(); private https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:32: // Implements AppHostInstaller::InstallAppHostIfNecessary(). // Asynchronously checks the system state and installs the App Host if not present. |completion_callback| will be notified on the caller's thread when the installation process completes. No timeout is used - thus |completion_callback| may be held for an arbitrarily long time, including past browser shutdown. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:33: void InstallAppHostIfNecessary( static, and rename to EnsureAppHostInstalled, since we are now doing the "IfNecessary" bit elsewhere. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:40: // Moves to the FILE thread. Continues InstallAppHostIfNecessary(). // Checks the system state on the FILE thread. Will trigger "InstallAppHost" if App Host is not installed. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:41: void InstallAppHostIfNecessaryInternal(); Rename to "EnsureAppHostInstalledInternal" https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:43: // Runs on the FILE thread. Installs App Host. // Asynchronously triggers the App Host installation. Will call "Finish" upon completion (successful or otherwise). https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:47: void Finish(bool success); // Passes |success| to |completion_callback| on the original caller thread. Deletes the AppHostInstallerImpl. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:517: if (!success) { Add a DCHECK for current thread == FILE
Please take another look. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/app/generat... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/app/generat... chrome/app/generated_resources.grd:4984: + <message name="IDS_EXTENSION_APP_HOST_INSTALL_ERROR" desc="Message for when an App Host fails to install."> On 2012/10/09 19:24:48, erikwright wrote: > There are a couple options here: > > (1) Really generic message such as "Application installation failed". > (2) Less generic message such as "Application Launcher installation failed" or > "Application Runtime installation failed." > (3) One of the above with a link to a knowledge-base article. > (4) Actually capture the installer error string and/or error code from the App > Launcher setup.exe process. > > I would suggest (1) or (2) for now, a histogram to track failures in the field, > and log a task for (4). > > If the histogram actually indicates a non-trivial number of failures, a > knowledge-base article can be written and linked to from the failure message. Done (going with "Application Runtime installation failed."). https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:22: // The implementation. On 2012/10/09 19:24:48, erikwright wrote: > Go ahead and do the is_platform_app() check here, and then don't pass extension > to impl Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:23: AppHostInstallerImpl* impl = new AppHostInstallerImpl(); On 2012/10/09 19:24:48, erikwright wrote: > Make the constructor and destructor of AppHostInstallerImpl private, make > InstallAppHostIfNecessary a static function. Nice. Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:8: #include "base/basictypes.h" On 2012/10/09 19:24:48, erikwright wrote: > not needed, right? Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:19: // App Host installation is necessary if On 2012/10/09 19:24:48, erikwright wrote: > This level of detail is inappropriate for the declaration. Use a declarative > style, and stick to the details clients care about: > > // Determines if the App Host is installed and, if not, installs it. > > Other details can be moved to the implementation. Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:30: AppHostInstallerImpl(); On 2012/10/09 19:24:48, erikwright wrote: > private Done. Added two parameters. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:32: // Implements AppHostInstaller::InstallAppHostIfNecessary(). On 2012/10/09 19:24:48, erikwright wrote: > // Asynchronously checks the system state and installs the App Host if not > present. |completion_callback| will be notified on the caller's thread when the > installation process completes. No timeout is used - thus |completion_callback| > may be held for an arbitrarily long time, including past browser shutdown. Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:33: void InstallAppHostIfNecessary( On 2012/10/09 19:24:48, erikwright wrote: > static, and rename to EnsureAppHostInstalled, since we are now doing the > "IfNecessary" bit elsewhere. Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:40: // Moves to the FILE thread. Continues InstallAppHostIfNecessary(). On 2012/10/09 19:24:48, erikwright wrote: > // Checks the system state on the FILE thread. Will trigger "InstallAppHost" if > App Host is not installed. Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:41: void InstallAppHostIfNecessaryInternal(); On 2012/10/09 19:24:48, erikwright wrote: > Rename to "EnsureAppHostInstalledInternal" Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:43: // Runs on the FILE thread. Installs App Host. On 2012/10/09 19:24:48, erikwright wrote: > // Asynchronously triggers the App Host installation. Will call "Finish" upon > completion (successful or otherwise). Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:47: void Finish(bool success); On 2012/10/09 19:24:48, erikwright wrote: > // Passes |success| to |completion_callback| on the original caller thread. > Deletes the AppHostInstallerImpl. Done. https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/35001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:517: if (!success) { On 2012/10/09 19:24:48, erikwright wrote: > Add a DCHECK for current thread == FILE Done.
Please make sure you have read the Google C++ style guide (and Chromium style guide) sections on comments. Particularly, what types of comments are required, optional, and discouraged in class declarations and implementations. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:18: // Calls |completion_callback| after checks and installation (if necessary) ... on the original calling thread after checks ... https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:114: // static function. just "// static" https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:123: AppHostInstallerImpl *impl = the assignment to a local variable is unnecessary. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:126: // impl will delete itself. // AppHostInstalerImpl will delete itself new AppHostInstallerImpl(...)->EnsureAppHostInstalledInternal(); https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:129: // AppHostInstallerImpl checks the presence of app_host.exe, and launches This comment should probably be at the top of this file. Fully-qualify Finish (AppHostInstallerImpl::Finish) and instead of |completion_callback| just say "which notifies the caller via a completion callback on the original calling thread and then destroys the AppHostInstallerImpl instance." https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:137: AppHostInstallerImpl::AppHostInstallerImpl( "// Private constructor." is not necessary. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:143: // Private destructor. not necessary. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:146: // Checks the system state on the FILE thread. Will trigger InstallAppHost() Unless you have implementation details to add beyond what is in the header, no comment is necessary here. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:165: // Asynchronously triggers the App Host installation. Will call "Finish" upon Ditto. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:182: // Passes |success| to |completion_callback| on the original caller thread. Ditto. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:33: AppHostInstallerImpl(const base::Callback<void(bool)>& completion_callback, // Constructs an AppHostInstallerImpl which will call |completion_callback| on the specified thread upon installation completion. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:49: // Callback that should be invoked on install completion on the caller thread. My personal opinion is that comments are unnecessary and possibly reduce readability for the private member variables. I will defer to Ben's opinion, though.
Made the suggested changes on comments, but not uploading yet (as more comments might come in). Now I'm wondering, perhaps AppHostInstallerImpl should hide completely inside app_host_installer_impl.win.cc, and the interface is just a subroutine instead of a static member? https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:18: // Calls |completion_callback| after checks and installation (if necessary) On 2012/10/09 21:51:52, erikwright wrote: > ... on the original calling thread after checks ... Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:114: // static function. On 2012/10/09 21:51:52, erikwright wrote: > just "// static" Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:123: AppHostInstallerImpl *impl = On 2012/10/09 21:51:52, erikwright wrote: > the assignment to a local variable is unnecessary. Done. Needed (new ...)->... to make compiler happy. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:126: // impl will delete itself. On 2012/10/09 21:51:52, erikwright wrote: > // AppHostInstalerImpl will delete itself > new AppHostInstallerImpl(...)->EnsureAppHostInstalledInternal(); Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:129: // AppHostInstallerImpl checks the presence of app_host.exe, and launches On 2012/10/09 21:51:52, erikwright wrote: > This comment should probably be at the top of this file. > > Fully-qualify Finish (AppHostInstallerImpl::Finish) and instead of > |completion_callback| just say "which notifies the caller via a completion > callback on the original calling thread and then destroys the > AppHostInstallerImpl instance." Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:137: AppHostInstallerImpl::AppHostInstallerImpl( On 2012/10/09 21:51:52, erikwright wrote: > "// Private constructor." is not necessary. Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:143: // Private destructor. On 2012/10/09 21:51:52, erikwright wrote: > not necessary. Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:146: // Checks the system state on the FILE thread. Will trigger InstallAppHost() On 2012/10/09 21:51:52, erikwright wrote: > Unless you have implementation details to add beyond what is in the header, no > comment is necessary here. Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:165: // Asynchronously triggers the App Host installation. Will call "Finish" upon On 2012/10/09 21:51:52, erikwright wrote: > Ditto. Done. Moved the comment to the .h file. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:182: // Passes |success| to |completion_callback| on the original caller thread. On 2012/10/09 21:51:52, erikwright wrote: > Ditto. Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:33: AppHostInstallerImpl(const base::Callback<void(bool)>& completion_callback, On 2012/10/09 21:51:52, erikwright wrote: > // Constructs an AppHostInstallerImpl which will call |completion_callback| on > the specified thread upon installation completion. Done. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:49: // Callback that should be invoked on install completion on the caller thread. On 2012/10/09 21:51:52, erikwright wrote: > My personal opinion is that comments are unnecessary and possibly reduce > readability for the private member variables. > > I will defer to Ben's opinion, though. Okay, I'll wait. I realized that crx_installer.h has a bunch of comments, so I decide to imitate.
http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/a... File chrome/browser/extensions/app_host_installer.h (right): http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/a... chrome/browser/extensions/app_host_installer.h:26: const base::Callback<void(bool)>& completion_callback); On 2012/10/09 18:25:37, huangs1 wrote: > On 2012/10/09 15:54:53, erikwright wrote: > > On 2012/10/08 06:14:33, benwells wrote: > > > I know I suggested adding the extension here, but it feels a bit wrong > > somehow. > > > If it wasn't here, this file and the implementation could move somewhere > else, > > > which would make me happy. It does not feel right in extensions. > > > > We can push the is_platform_app() check to call sites, but that doesn't seem > > right either. > > > > Currently the app_host_installer is in two parts - the basic outside part and > > the platform-specific part. > > > > The check for is_platform_app() could be kept in the outside part, and that > > could stay here as a static helper function. > > > > The platform-specific part could then be moved somewhere else. > > > > I'd like to defer the actual question of where, while I think about it some > > more. > > Deferring. Once we have a decision, will move the file as part of the final > change, to not disrupt diffs. My main concerns are (1) keeping all this code out of the extensions area, as it has little relation to the other code there. One sign of this is that it should be reviewed by installer folk, not extension folk. (2) keep the flow of extension install simple for non-app-host-requiring situations. This will help the extension folk who have to debug problems with extension install and don't know anything about the app host. Erik's suggestion has improved (2). Or, add something to extension.cc along the lines of: bool Extension::requires_app_host() { return is_platform_app(); } http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/c... File chrome/browser/extensions/crx_installer.cc (right): http://codereview.chromium.org/11054006/diff/8013/chrome/browser/extensions/c... chrome/browser/extensions/crx_installer.cc:512: app_host_installer_.InstallAppHostIfNecessary(*extension_, On 2012/10/09 18:25:37, huangs1 wrote: > On 2012/10/09 15:54:53, erikwright wrote: > > On 2012/10/08 06:14:33, benwells wrote: > > > Always going through this function and its callbacks obfuscates the flow of > > > installation for all extensions - most of which don't need the app host. > This > > is > > > the main reason I suggested having the IsAppHostRequired function (the other > > > reason is that something similar will be needed for the app launcher). > > > > > > Putting something back like: > > > if (something) { > > > do this stuff with callbacks > > > return; > > > } > > > > > > CompleteInstall(); > > > > > > will make the flow nice and simple again for extensions which don't need the > > app > > > host. > > > > > > "something" could be calling into the app_host_installation code, or maybe > > even > > > better could just call a new function extension->RequiresAppHost(), keeping > > the > > > app host installation code completely decoupled from extensions. > > > > Part of the checking has to occur on the FILE thread. > > > > It's true that for non-Windows platforms we don't need to go to the FILE > thread, > > but I think it is more appropriate to encapsulate this detail in the AppHost > > specific code, rather than requiring each of the clients to be aware of it. > > > > The AppHost code Does The Right Thing, which is to say it doesn't bounce > across > > threads unnecessarily on unsupported platforms. > > Some minor rearrangement for style; otherwise not changing the interface. I should have been clearer. The "something" would only be part of the check, the is_platform_app part. But having that scattered everywhere would be yuck so it would be better instead encapsulated on extension. Again, the objective is to keep the install flow simpler for the majority of extensions that are not platform apps. http://codereview.chromium.org/11054006/diff/34002/chrome/browser/extensions/... File chrome/browser/extensions/app_host_installer_impl_win.h (right): http://codereview.chromium.org/11054006/diff/34002/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer_impl_win.h:49: // Callback that should be invoked on install completion on the caller thread. On 2012/10/09 22:44:26, huangs1 wrote: > On 2012/10/09 21:51:52, erikwright wrote: > > My personal opinion is that comments are unnecessary and possibly reduce > > readability for the private member variables. > > > > I will defer to Ben's opinion, though. > > Okay, I'll wait. I realized that crx_installer.h has a bunch of comments, so I > decide to imitate. I agree with Erik, commenting private member variables adds noise and has a tendency to mislead over time. If the variables are well named it is unnecessary. So, feel free to remove the comments :)
Please check again. https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/34002/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:49: // Callback that should be invoked on install completion on the caller thread. On 2012/10/10 03:22:04, benwells wrote: > On 2012/10/09 22:44:26, huangs1 wrote: > > On 2012/10/09 21:51:52, erikwright wrote: > > > My personal opinion is that comments are unnecessary and possibly reduce > > > readability for the private member variables. > > > > > > I will defer to Ben's opinion, though. > > > > Okay, I'll wait. I realized that crx_installer.h has a bunch of comments, so > I > > decide to imitate. > > I agree with Erik, commenting private member variables adds noise and has a > tendency to mislead over time. If the variables are well named it is > unnecessary. So, feel free to remove the comments :) Done.
I'm going to drum up a suggestion for the final file locations. https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:14: #include "base/win/object_watcher.h" not required (in header). https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:16: #include "chrome/browser/extensions/app_host_installer.h" not required. https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:18: #include "content/public/browser/browser_thread.h" not required (in header). https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:20: // AppHostInstallerImpl checks the presence of app_host.exe, and launches I believe file comments are required to go before includes, per Google C++ Style Guide http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_C... https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:54: remove blank lines between member variables.
PTAL https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:14: #include "base/win/object_watcher.h" On 2012/10/10 16:32:24, erikwright wrote: > not required (in header). Done (I thought we're not supposed to rely on transitive inclusions?). https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:16: #include "chrome/browser/extensions/app_host_installer.h" On 2012/10/10 16:32:24, erikwright wrote: > not required. Done. https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:18: #include "content/public/browser/browser_thread.h" On 2012/10/10 16:32:24, erikwright wrote: > not required (in header). Done. https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:20: // AppHostInstallerImpl checks the presence of app_host.exe, and launches On 2012/10/10 16:32:24, erikwright wrote: > I believe file comments are required to go before includes, per Google C++ Style > Guide > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_C... Done. https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:54: On 2012/10/10 16:32:24, erikwright wrote: > remove blank lines between member variables. Done.
Just a little insightful comment for Sam :). https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/43016/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:14: #include "base/win/object_watcher.h" On 2012/10/10 17:46:42, huangs1 wrote: > On 2012/10/10 16:32:24, erikwright wrote: > > not required (in header). > > Done (I thought we're not supposed to rely on transitive inclusions?). Except when the inclusion comes from your parent header (i.e. .cc of the .h with same name). The reason to include-what-you-use is not to depend on the implementation of other headers (i.e. removing an include in a.h shouldn't break b.cc); however this doesn't apply to a.h vs a.cc. Hope this makes sense.
Thanks gab@!
Only changed chrome/chrome_browser.gypi. PTAL.
Single line fix! PTAL.
first pass. http://codereview.chromium.org/11054006/diff/53001/chrome/browser/extensions/... File chrome/browser/extensions/app_host_installer.cc (right): http://codereview.chromium.org/11054006/diff/53001/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer.cc:18: void InstallAppHostIfNecessary(const Extension& extension, same wrapping here. http://codereview.chromium.org/11054006/diff/53001/chrome/browser/extensions/... File chrome/browser/extensions/app_host_installer.h (right): http://codereview.chromium.org/11054006/diff/53001/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer.h:21: void InstallAppHostIfNecessary(const Extension& extension, need to wrap both parameters if you can't indent the second one (you can't). So line-break after ( http://codereview.chromium.org/11054006/diff/53001/chrome/browser/extensions/... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): http://codereview.chromium.org/11054006/diff/53001/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer_impl_win.cc:176: if (!BrowserThread::PostTask(caller_thread_id_, FROM_HERE, base::Bind( same wrapping comment
partial review here. some nits, some open for discussion https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:14: namespace app_host_installer { this doesn't follow the chromium namespace convention. if this code belongs in the app_host_installer namespace, then it should also be in the app_host_installer directory under the extensions directory. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/ZLes1... https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:48: QuickEnableDelegate(const base::Callback<void(bool)>& callback) style disallows all inlining like this. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:50: add out-of-line dtor https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:25: // present. |completion_callback| will be notified on the caller's thread what does the bool arg for the completion_callback mean? consider making a typedef for the type base::Callback<void(bool)> where you document this. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:47: void InstallAppHost(); nit: There's a convention to suffix funcs that must be called on a particular thread with "OnFooThread", so please rename this to InstallAppHostOnFileThread. Perhaps other funcs should be similarly changed. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:525: ReportFailureFromFileThread( nit: use less vertical space: ReportFailureFromFileThread(CrxInstallerError( l10n_util::GetStringUTF16(IDS_EXTENSION_APP_HOST_INSTALL_ERROR)));
On 2012/10/24 15:59:18, grt wrote: > partial review here. some nits, some open for discussion > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > File chrome/browser/extensions/app_host_installer.h (right): > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > chrome/browser/extensions/app_host_installer.h:14: namespace app_host_installer > { > this doesn't follow the chromium namespace convention. if this code belongs in > the app_host_installer namespace, then it should also be in the > app_host_installer directory under the extensions directory. > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/ZLes1... > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > File chrome/browser/extensions/app_host_installer_impl_win.cc (right): > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > chrome/browser/extensions/app_host_installer_impl_win.cc:48: > QuickEnableDelegate(const base::Callback<void(bool)>& callback) > style disallows all inlining like this. > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > chrome/browser/extensions/app_host_installer_impl_win.cc:50: > add out-of-line dtor > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > File chrome/browser/extensions/app_host_installer_impl_win.h (right): > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > chrome/browser/extensions/app_host_installer_impl_win.h:25: // present. > |completion_callback| will be notified on the caller's thread > what does the bool arg for the completion_callback mean? consider making a > typedef for the type base::Callback<void(bool)> where you document this. > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > chrome/browser/extensions/app_host_installer_impl_win.h:47: void > InstallAppHost(); > nit: There's a convention to suffix funcs that must be called on a particular > thread with "OnFooThread", so please rename this to InstallAppHostOnFileThread. > Perhaps other funcs should be similarly changed. > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > File chrome/browser/extensions/crx_installer.cc (right): > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > chrome/browser/extensions/crx_installer.cc:525: ReportFailureFromFileThread( > nit: use less vertical space: > ReportFailureFromFileThread(CrxInstallerError( > l10n_util::GetStringUTF16(IDS_EXTENSION_APP_HOST_INSTALL_ERROR))); benwells, regarding where to put this code: It doesn't belong in chrome/installer, because that is the Windows installer only and there could quite conceivably be other platform versions of this code. I suggest: chrome/browser/extensions/app_host/installer Does that suit you? grt: I believe that the namespace app_host_installer would be kosher in that location.
PTAL. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:18: void InstallAppHostIfNecessary(const Extension& extension, On 2012/10/24 15:33:28, erikwright wrote: > same wrapping here. Done. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:14: namespace app_host_installer { We still need to find a good place to place these, so not changing this yet (at least keeping as placeholder). https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:21: void InstallAppHostIfNecessary(const Extension& extension, On 2012/10/24 15:33:28, erikwright wrote: > need to wrap both parameters if you can't indent the second one (you can't). > > So line-break after ( Done. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:48: QuickEnableDelegate(const base::Callback<void(bool)>& callback) On 2012/10/24 15:59:18, grt wrote: > style disallows all inlining like this. Done. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:50: On 2012/10/24 15:59:18, grt wrote: > add out-of-line dtor Done. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.cc:176: if (!BrowserThread::PostTask(caller_thread_id_, FROM_HERE, base::Bind( On 2012/10/24 15:33:28, erikwright wrote: > same wrapping comment Done. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_impl_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:25: // present. |completion_callback| will be notified on the caller's thread Done. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/app_host_installer_impl_win.h:47: void InstallAppHost(); Done. Changing my code only though. https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:525: ReportFailureFromFileThread( On 2012/10/24 15:59:18, grt wrote: > nit: use less vertical space: > ReportFailureFromFileThread(CrxInstallerError( > l10n_util::GetStringUTF16(IDS_EXTENSION_APP_HOST_INSTALL_ERROR))); Done.
On 2012/10/24 16:50:29, erikwright wrote: > On 2012/10/24 15:59:18, grt wrote: > > partial review here. some nits, some open for discussion > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > File chrome/browser/extensions/app_host_installer.h (right): > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > chrome/browser/extensions/app_host_installer.h:14: namespace > app_host_installer > > { > > this doesn't follow the chromium namespace convention. if this code belongs in > > the app_host_installer namespace, then it should also be in the > > app_host_installer directory under the extensions directory. > > > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/ZLes1... > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > File chrome/browser/extensions/app_host_installer_impl_win.cc (right): > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > chrome/browser/extensions/app_host_installer_impl_win.cc:48: > > QuickEnableDelegate(const base::Callback<void(bool)>& callback) > > style disallows all inlining like this. > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > chrome/browser/extensions/app_host_installer_impl_win.cc:50: > > add out-of-line dtor > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > File chrome/browser/extensions/app_host_installer_impl_win.h (right): > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > chrome/browser/extensions/app_host_installer_impl_win.h:25: // present. > > |completion_callback| will be notified on the caller's thread > > what does the bool arg for the completion_callback mean? consider making a > > typedef for the type base::Callback<void(bool)> where you document this. > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > chrome/browser/extensions/app_host_installer_impl_win.h:47: void > > InstallAppHost(); > > nit: There's a convention to suffix funcs that must be called on a particular > > thread with "OnFooThread", so please rename this to > InstallAppHostOnFileThread. > > Perhaps other funcs should be similarly changed. > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > File chrome/browser/extensions/crx_installer.cc (right): > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > chrome/browser/extensions/crx_installer.cc:525: ReportFailureFromFileThread( > > nit: use less vertical space: > > ReportFailureFromFileThread(CrxInstallerError( > > l10n_util::GetStringUTF16(IDS_EXTENSION_APP_HOST_INSTALL_ERROR))); > > benwells, regarding where to put this code: > > It doesn't belong in chrome/installer, because that is the Windows installer > only and there could quite conceivably be other platform versions of this code. > > I suggest: > chrome/browser/extensions/app_host/installer > > Does that suit you? > > grt: I believe that the namespace app_host_installer would be kosher in that > location. grt and I spoke about this f2f. We think it should go in chrome/browser/app_host_util Reasoning: 1) the current contents of chrome/browser/app_host are actually not part of the browser but a separate executable. Therefore they should go up to the top-level (src/app_host) 2) The AppHostInstaller will shortly be joined by some state-querying methods, so it should not just be app_host_installer/ (thus app_host_util/) 3) The code is only used by extensions at the moment, and thus, although it could technically go in chrome/common if you just consider its dependencies, it doesn't make sense to pollute these higher parts of the hierarchy with extensions-specific code. benwells?
On 2012/10/24 19:16:05, erikwright wrote: > On 2012/10/24 16:50:29, erikwright wrote: > > On 2012/10/24 15:59:18, grt wrote: > > > partial review here. some nits, some open for discussion > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > File chrome/browser/extensions/app_host_installer.h (right): > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > chrome/browser/extensions/app_host_installer.h:14: namespace > > app_host_installer > > > { > > > this doesn't follow the chromium namespace convention. if this code belongs > in > > > the app_host_installer namespace, then it should also be in the > > > app_host_installer directory under the extensions directory. > > > > > > > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/ZLes1... > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > File chrome/browser/extensions/app_host_installer_impl_win.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > chrome/browser/extensions/app_host_installer_impl_win.cc:48: > > > QuickEnableDelegate(const base::Callback<void(bool)>& callback) > > > style disallows all inlining like this. > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > chrome/browser/extensions/app_host_installer_impl_win.cc:50: > > > add out-of-line dtor > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > File chrome/browser/extensions/app_host_installer_impl_win.h (right): > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > chrome/browser/extensions/app_host_installer_impl_win.h:25: // present. > > > |completion_callback| will be notified on the caller's thread > > > what does the bool arg for the completion_callback mean? consider making a > > > typedef for the type base::Callback<void(bool)> where you document this. > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > chrome/browser/extensions/app_host_installer_impl_win.h:47: void > > > InstallAppHost(); > > > nit: There's a convention to suffix funcs that must be called on a > particular > > > thread with "OnFooThread", so please rename this to > > InstallAppHostOnFileThread. > > > Perhaps other funcs should be similarly changed. > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > File chrome/browser/extensions/crx_installer.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/11054006/diff/53001/chrome/browser/ext... > > > chrome/browser/extensions/crx_installer.cc:525: ReportFailureFromFileThread( > > > nit: use less vertical space: > > > ReportFailureFromFileThread(CrxInstallerError( > > > l10n_util::GetStringUTF16(IDS_EXTENSION_APP_HOST_INSTALL_ERROR))); > > > > benwells, regarding where to put this code: > > > > It doesn't belong in chrome/installer, because that is the Windows installer > > only and there could quite conceivably be other platform versions of this > code. > > > > I suggest: > > chrome/browser/extensions/app_host/installer > > > > Does that suit you? > > > > grt: I believe that the namespace app_host_installer would be kosher in that > > location. > > grt and I spoke about this f2f. > > We think it should go in chrome/browser/app_host_util > > Reasoning: > 1) the current contents of chrome/browser/app_host are actually not part of the > browser but a separate executable. Therefore they should go up to the top-level > (src/app_host) > 2) The AppHostInstaller will shortly be joined by some state-querying methods, > so it should not just be app_host_installer/ (thus app_host_util/) > 3) The code is only used by extensions at the moment, and thus, although it > could technically go in chrome/common if you just consider its dependencies, it > doesn't make sense to pollute these higher parts of the hierarchy with > extensions-specific code. > > benwells? I like chrome/browser/app_host_util
> I like chrome/browser/app_host_util Another possibility is to have chrome/browser/apps, which could have packaged apps related stuff in general. This might work better than just app_host_util, which would never have much stuff in it. There are lots of types of extensions, most aren't platform apps and a lot of the people working on extensions don't know all the ins and outs of platform apps, which is why extensions isn't too good. But apps avoids this. mihaip@, miket@: what say you to chrome/browser/apps? aa@ and I had a quick chat about it, it would be logically dependent on chrome/browser/extensions and not the other way (but not through gyp rules, just through careful stewardship).
http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... File chrome/browser/extensions/crx_installer.cc (right): http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... chrome/browser/extensions/crx_installer.cc:518: app_host_installer::InstallAppHostIfNecessary( This seems out of place and like a layering violation. Conceptually, apps should depend on extensions, but not the reverse. Eventually, we will make this real with DEPS, but for now we should not introduce more issues. CRXInstaller should be about installing packages, that's it. It shouldn't know about the details of what's in those packages. Clients of CRXInstaller should use an abstract interface to begin installation and to register for notifications of its progress. So I think that you should abstract out some kind of OnInstallCompleteOnFileThread() API that other clients could also use. http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... chrome/browser/extensions/crx_installer.cc:520: base::Bind(&CrxInstaller::OnAppHostInstallationComplete, this)); I didn't have time to read all the code behind InstallAppHostIfNecessary, but it wasn't immediately clear why it is asynchronous. We are already on the FILE thread here. Can InstallAppHostIfNecessary just block? If so that would make the client API to CRXInstaller somewhat simpler.
http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... File chrome/browser/extensions/unpacked_installer.cc (right): http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... chrome/browser/extensions/unpacked_installer.cc:271: app_host_installer::InstallAppHostIfNecessary( Same thing here - this should be done via an abstract interface somehow.
response to aa@. http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... File chrome/browser/extensions/crx_installer.cc (right): http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... chrome/browser/extensions/crx_installer.cc:518: app_host_installer::InstallAppHostIfNecessary( On 2012/10/25 04:39:41, Aaron Boodman wrote: > This seems out of place and like a layering violation. > > Conceptually, apps should depend on extensions, but not the reverse. Eventually, > we will make this real with DEPS, but for now we should not introduce more > issues. > > CRXInstaller should be about installing packages, that's it. It shouldn't know > about the details of what's in those packages. Clients of CRXInstaller should > use an abstract interface to begin installation and to register for > notifications of its progress. > > So I think that you should abstract out some kind of > OnInstallCompleteOnFileThread() API that other clients could also use. OK. It would be an API similar to the following: class ExtensionInstallDelegate { public: // Called after permissions have been granted and the install is ready to proceed. The delegate should call |completion_callback| with true to complete the installation or false to abort. virtual void OnReadyToInstall(Extension *, base::Callback<void(bool)> completion_callback) = 0; }; And every CrxInstaller that could theoretically be used with a platform app needs to have our implementation of the delegate. So it would presumably make sense to centralize the instantiation of CrxInstallers (and UnpackedInstallers) in ExtensionService. Also, it seems that you mean you don't even want this functionality in chrome/browser/extensions. Is that right? If so, it would imply having some way to register the listeners on the ExtensionService, and some other part of Chrome would need to implement the logic to install the launcher (which part?). And if I'm right that you don't want this in c/b/extensions, presumably that also applies to AppShortcutManager? http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... chrome/browser/extensions/crx_installer.cc:520: base::Bind(&CrxInstaller::OnAppHostInstallationComplete, this)); On 2012/10/25 04:39:41, Aaron Boodman wrote: > I didn't have time to read all the code behind InstallAppHostIfNecessary, but it > wasn't immediately clear why it is asynchronous. We are already on the FILE > thread here. Can InstallAppHostIfNecessary just block? If so that would make the > client API to CRXInstaller somewhat simpler. We launch an external process, which can take an arbitrary length of time. I think it's best to be asynchronous.
http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... File chrome/browser/extensions/crx_installer.cc (right): http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... chrome/browser/extensions/crx_installer.cc:518: app_host_installer::InstallAppHostIfNecessary( On 2012/10/26 20:32:30, erikwright wrote: > On 2012/10/25 04:39:41, Aaron Boodman wrote: > > This seems out of place and like a layering violation. > > > > Conceptually, apps should depend on extensions, but not the reverse. > Eventually, > > we will make this real with DEPS, but for now we should not introduce more > > issues. > > > > CRXInstaller should be about installing packages, that's it. It shouldn't know > > about the details of what's in those packages. Clients of CRXInstaller should > > use an abstract interface to begin installation and to register for > > notifications of its progress. > > > > So I think that you should abstract out some kind of > > OnInstallCompleteOnFileThread() API that other clients could also use. > > OK. It would be an API similar to the following: > > class ExtensionInstallDelegate { > public: > // Called after permissions have been granted and the install is ready to > proceed. The delegate should call |completion_callback| with true to complete > the installation or false to abort. > virtual void OnReadyToInstall(Extension *, base::Callback<void(bool)> > completion_callback) = 0; > }; > > And every CrxInstaller that could theoretically be used with a platform app > needs to have our implementation of the delegate. So it would presumably make > sense to centralize the instantiation of CrxInstallers (and UnpackedInstallers) > in ExtensionService. Imagine a world after: mv /chrome/browser/extensions/api /chrome/browser/extension_apis mv /chrome/browser/extensions/*app* /apps/* mv /chrome/browser/extensions /extensions /apps depends on /extensions /chrome/browser depends on both extensions and apps That is the world we want to move toward. It doesn't make sense for ExtensionService to create the ExtensionInstallDelegateForApps class in such a world, because it doesn't know anything about apps. It sounds like this centralization is needed somewhere in chrome/browser. Conceptually, something like "Browser::InitiateExtensionInstall()". > Also, it seems that you mean you don't even want this functionality in > chrome/browser/extensions. Is that right? If so, it would imply having some way > to register the listeners on the ExtensionService, and some other part of Chrome > would need to implement the logic to install the launcher (which part?). > > And if I'm right that you don't want this in c/b/extensions, presumably that > also applies to AppShortcutManager? I don't feel as strongly about the directories right now, but if there's an obvious home for this (/c/b/apps/shortcut_manager?) then I think it probably makes sense to move it there so that people see it and have a better chance of doing the right thing in the future. http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... chrome/browser/extensions/crx_installer.cc:520: base::Bind(&CrxInstaller::OnAppHostInstallationComplete, this)); On 2012/10/26 20:32:30, erikwright wrote: > On 2012/10/25 04:39:41, Aaron Boodman wrote: > > I didn't have time to read all the code behind InstallAppHostIfNecessary, but > it > > wasn't immediately clear why it is asynchronous. We are already on the FILE > > thread here. Can InstallAppHostIfNecessary just block? If so that would make > the > > client API to CRXInstaller somewhat simpler. > > We launch an external process, which can take an arbitrary length of time. I > think it's best to be asynchronous. You could move CrxInstaller to use the BlockingTaskPool, so that it's not a big deal if this takes a long time. That way we don't need another resume API on CrxInstaller. OTOH, if you're going to keep this async, consider putting the callback to the interface on the UI thread, since that would be more generally useful.
On Mon, Oct 29, 2012 at 4:37 PM, <aa@chromium.org> wrote: > It sounds like this centralization is needed somewhere in > chrome/browser. Conceptually, something like > "Browser::InitiateExtensionInstall()". Note, I don't literally mean this should go on Browser - just something at that level. - a
It feels like the same problem with the delegate in http://codereview.chromium.org/11117011/ We need some way to register delegate factories ... or something ... I'd be happy to take this on, it would be awesome if it didn't hold up these patches. On Tue, Oct 30, 2012 at 11:01 AM, Aaron Boodman <aa@chromium.org> wrote: > On Mon, Oct 29, 2012 at 4:37 PM, <aa@chromium.org> wrote: > > It sounds like this centralization is needed somewhere in > > chrome/browser. Conceptually, something like > > "Browser::InitiateExtensionInstall()". > > Note, I don't literally mean this should go on Browser - just > something at that level. > > - a >
On 2012/10/29 23:37:29, Aaron Boodman wrote: > http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... > File chrome/browser/extensions/crx_installer.cc (right): > > http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... > chrome/browser/extensions/crx_installer.cc:518: > app_host_installer::InstallAppHostIfNecessary( > On 2012/10/26 20:32:30, erikwright wrote: > > On 2012/10/25 04:39:41, Aaron Boodman wrote: > > > This seems out of place and like a layering violation. > > > > > > Conceptually, apps should depend on extensions, but not the reverse. > > Eventually, > > > we will make this real with DEPS, but for now we should not introduce more > > > issues. > > > > > > CRXInstaller should be about installing packages, that's it. It shouldn't > know > > > about the details of what's in those packages. Clients of CRXInstaller > should > > > use an abstract interface to begin installation and to register for > > > notifications of its progress. > > > > > > So I think that you should abstract out some kind of > > > OnInstallCompleteOnFileThread() API that other clients could also use. > > > > OK. It would be an API similar to the following: > > > > class ExtensionInstallDelegate { > > public: > > // Called after permissions have been granted and the install is ready to > > proceed. The delegate should call |completion_callback| with true to complete > > the installation or false to abort. > > virtual void OnReadyToInstall(Extension *, base::Callback<void(bool)> > > completion_callback) = 0; > > }; > > > > And every CrxInstaller that could theoretically be used with a platform app > > needs to have our implementation of the delegate. So it would presumably make > > sense to centralize the instantiation of CrxInstallers (and > UnpackedInstallers) > > in ExtensionService. > > Imagine a world after: > > mv /chrome/browser/extensions/api /chrome/browser/extension_apis > mv /chrome/browser/extensions/*app* /apps/* > mv /chrome/browser/extensions /extensions > > /apps depends on /extensions > /chrome/browser depends on both extensions and apps > > That is the world we want to move toward. It doesn't make sense for > ExtensionService to create the ExtensionInstallDelegateForApps class in such a > world, because it doesn't know anything about apps. > > It sounds like this centralization is needed somewhere in chrome/browser. > Conceptually, something like "Browser::InitiateExtensionInstall()". > > > Also, it seems that you mean you don't even want this functionality in > > chrome/browser/extensions. Is that right? If so, it would imply having some > way > > to register the listeners on the ExtensionService, and some other part of > Chrome > > would need to implement the logic to install the launcher (which part?). > > > > And if I'm right that you don't want this in c/b/extensions, presumably that > > also applies to AppShortcutManager? > > I don't feel as strongly about the directories right now, but if there's an > obvious home for this (/c/b/apps/shortcut_manager?) then I think it probably > makes sense to move it there so that people see it and have a better chance of > doing the right thing in the future. OK, for now I think we agree on (1) defining the delegate interface I sketched out (2) adding a method on ExtensionService to register delegate(s) (3) These delegates are automatically added to all {Crx,Unpacked}Installers. (4) The delegate implementation that installs the launcher will go in chrome/browser/apps/launcher_util And for now, I propose putting the code to create and register this delegate in ProfileImpl::ProfileImpl(). Does that suit you for the moment? > > http://codereview.chromium.org/11054006/diff/54017/chrome/browser/extensions/... > chrome/browser/extensions/crx_installer.cc:520: > base::Bind(&CrxInstaller::OnAppHostInstallationComplete, this)); > On 2012/10/26 20:32:30, erikwright wrote: > > On 2012/10/25 04:39:41, Aaron Boodman wrote: > > > I didn't have time to read all the code behind InstallAppHostIfNecessary, > but > > it > > > wasn't immediately clear why it is asynchronous. We are already on the FILE > > > thread here. Can InstallAppHostIfNecessary just block? If so that would make > > the > > > client API to CRXInstaller somewhat simpler. > > > > We launch an external process, which can take an arbitrary length of time. I > > think it's best to be asynchronous. > > You could move CrxInstaller to use the BlockingTaskPool, so that it's not a big > deal if this takes a long time. That way we don't need another resume API on > CrxInstaller. > > OTOH, if you're going to keep this async, consider putting the callback to the > interface on the UI thread, since that would be more generally useful. I'm not really keen to dramatically alter the threading model of the extensions system. I'd prefer to leave the API async, but continuation can be done via a callback to avoid adding new public methods to {Crx,Unpacked}Installer. I don't mind doing the callback on UI thread, but it's worth noting that, since we start on File thread, this will imply an extra and unnecessary bounce for things that have to occur on the file thread.
This is a work-in-progress upload. Key points: - Changes in crx_installer.* are abandoned. - Changes in unpacked_installer.* are abandoned. - Moving App Host installation to app_shortcut_manager.* Installation is tested for both CRX and unpacked extension flows. Caveats: - App icon still appear before App Host installation. - There is no error message if App Host installation fails. https://chromiumcodereview.appspot.com/11054006/diff/54017/chrome/browser/ext... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/54017/chrome/browser/ext... chrome/browser/extensions/crx_installer.cc:520: base::Bind(&CrxInstaller::OnAppHostInstallationComplete, this)); Per our discussion, moving installation to AppShortcutManager. https://chromiumcodereview.appspot.com/11054006/diff/54017/chrome/browser/ext... File chrome/browser/extensions/unpacked_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/54017/chrome/browser/ext... chrome/browser/extensions/unpacked_installer.cc:271: app_host_installer::InstallAppHostIfNecessary( Per our discussion, moving installation to AppShortcutManager.
I like this approach a LOT. Great compromise. Just some small things and it should be ready. https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:21: #if defined(OS_WIN) I think this platform specific pattern is a little differnt to the pattern typically used. Can you do something like: - in app_host_installer.cc only have the #if !defined(OS_WIN) case - in app_host_installer_win.cc have the complete #if defined(OS_WIN) case (note, no _impl in the file name). Then you can also move Impl class as it is into an anonymous namespace in the _win file. https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:25: const Extension& extension, We don't need this here now. Let's assume the caller knows it is relevant for the extension.
https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:21: #if defined(OS_WIN) On 2012/11/01 23:11:46, benwells wrote: > I think this platform specific pattern is a little differnt to the pattern > typically used. > > Can you do something like: > - in app_host_installer.cc only have the #if !defined(OS_WIN) case > - in app_host_installer_win.cc have the complete #if defined(OS_WIN) case (note, > no _impl in the file name). > > Then you can also move Impl class as it is into an anonymous namespace in the > _win file. By default, I think x_win.cc will only be built on Windows, but x.cc will still be built on all platforms. So we will need to exclude it using .gyp rules on other platforms (or #ifdef out the implementation in the x.cc file for Windows). I don't really mind doing that, but it's not exactly the pattern used elsewhere, either (usually, there is a version for each platform, plus an optional cross-platform file that is compiled everywhere). I made an alternate suggestion in app_shortcut_manager.cc . PTAL in your Friday and if it suits you we'll do that. Otherwise, I prefer #ifdef option above (x.cc will be #ifdef'd out on Windows, but not excluded in .gyp). https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_shortcut_manager.cc:87: UpdateApplicationShortcuts(extension); Perhaps we can just put, here: #ifdef OS_WIN AppHostInstaller::EnsureAppHostInstalled( base::Bind(&AppShortcutManager::UpdateApplicationShortcuts, base::Unretained(this), extension)); #else UpdateApplicationShortcuts(extension) #endif Then we can put an #ifdef'd include at the top of this file and rename app_host_installer_impl_win.{cc,h} to app_host_installer_win.{cc,h}.
https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:21: #if defined(OS_WIN) On 2012/11/02 01:07:26, erikwright wrote: > On 2012/11/01 23:11:46, benwells wrote: > > I think this platform specific pattern is a little differnt to the pattern > > typically used. > > > > Can you do something like: > > - in app_host_installer.cc only have the #if !defined(OS_WIN) case > > - in app_host_installer_win.cc have the complete #if defined(OS_WIN) case > (note, > > no _impl in the file name). > > > > Then you can also move Impl class as it is into an anonymous namespace in the > > _win file. > > By default, I think x_win.cc will only be built on Windows, but x.cc will still > be built on all platforms. So we will need to exclude it using .gyp rules on > other platforms (or #ifdef out the implementation in the x.cc file for Windows). > > I don't really mind doing that, but it's not exactly the pattern used elsewhere, > either (usually, there is a version for each platform, plus an optional > cross-platform file that is compiled everywhere). > > I made an alternate suggestion in app_shortcut_manager.cc . PTAL in your Friday > and if it suits you we'll do that. Otherwise, I prefer #ifdef option above (x.cc > will be #ifdef'd out on Windows, but not excluded in .gyp). Yes, x_win.cc is only built on Windows. You do not need to exclude it using .gyp rules, just the implementation of the function using #if. The suggestion is your 'otherwise, i prefer...' option but without the separate exposed impl class, and with a _win.cc instead of _impl_win.cc. This matches what is done elsewhere e.g. base\file_util.h, base\process_util.h. But, I don't mind too much. As we only have one call site now your other suggestion is good too. If you think there will ever be InstallAppHostIfNecessary on linux and mac, we should do it like this, otherwise an #if in the shortcut manager is good.
PTAL. Notes: - Removed the localized error messages, since App Host installation failure will only be logged. - Caveat still exists: Chrome app icons appear before App Host install. Looking into this next. https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.cc:21: #if defined(OS_WIN) Done. So: -- app_host_installer.cc -- app_host_installer.h -- app_host_installer_impl_win.cc -- app_host_installer_impl_win.h ++ app_host_installer_win.cc ++ app_host_installer_win.h - app_host_installer_win.h is included iff OS_WIN. - Renamed AppHostInstallerImpl to AppHostInstaller. - class AppHostsInstaller exposed. - Main entry is AppHostInstaller::EnsureAppHostInstalled(). https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_host_installer.h (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_host_installer.h:25: const Extension& extension, - Removed parameter. - We no longer test extension.is_platform_app() ourselves. https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/77001/chrome/browser/ext... chrome/browser/extensions/app_shortcut_manager.cc:87: UpdateApplicationShortcuts(extension); On 2012/11/02 01:07:26, erikwright wrote: > Perhaps we can just put, here: > > #ifdef OS_WIN > AppHostInstaller::EnsureAppHostInstalled( > base::Bind(&AppShortcutManager::UpdateApplicationShortcuts, > base::Unretained(this), extension)); > #else > UpdateApplicationShortcuts(extension) > #endif > > Then we can put an #ifdef'd include at the top of this file and rename > app_host_installer_impl_win.{cc,h} to app_host_installer_win.{cc,h}. Done.
http://codereview.chromium.org/11054006/diff/72003/chrome/browser/extensions/... File chrome/browser/extensions/app_shortcut_manager.cc (right): http://codereview.chromium.org/11054006/diff/72003/chrome/browser/extensions/... chrome/browser/extensions/app_shortcut_manager.cc:94: base::Unretained(this), extension)); For safety the instance pointer should be a weak pointer to 'this'. Add a WeakPtrFactory<AppShortcutManager> member, initialize it with 'this', and then use it to create a weak pointer.
http://codereview.chromium.org/11054006/diff/72003/chrome/chrome_browser_exte... File chrome/chrome_browser_extensions.gypi (right): http://codereview.chromium.org/11054006/diff/72003/chrome/chrome_browser_exte... chrome/chrome_browser_extensions.gypi:344: 'browser/extensions/app_host_installer_win.cc', are these files gone? i don't see them in the latest patchset.
PTAL. I forgot to "git add" in the last CL. https://chromiumcodereview.appspot.com/11054006/diff/72003/chrome/browser/ext... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/72003/chrome/browser/ext... chrome/browser/extensions/app_shortcut_manager.cc:94: base::Unretained(this), extension)); On 2012/11/02 18:56:38, erikwright wrote: > For safety the instance pointer should be a weak pointer to 'this'. > > Add a WeakPtrFactory<AppShortcutManager> member, initialize it with 'this', and > then use it to create a weak pointer. Done. https://chromiumcodereview.appspot.com/11054006/diff/72003/chrome/chrome_brow... File chrome/chrome_browser_extensions.gypi (right): https://chromiumcodereview.appspot.com/11054006/diff/72003/chrome/chrome_brow... chrome/chrome_browser_extensions.gypi:344: 'browser/extensions/app_host_installer_win.cc', Doh, I forgot to do git add!
LGTM. https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_win.h:61: base::win::ScopedHandle process_; #include "base/win/scoped_handle.h" https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_win.h:62: scoped_ptr<base::win::ObjectWatcher::Delegate> delegate_; #include "base/memory/scoped_ptr.h" https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_shortcut_manager.cc:54: : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)), #include "base/compiler_specific.h"
aa: PTAL.
http://codereview.chromium.org/11054006/diff/76003/chrome/browser/extensions/... File chrome/browser/extensions/app_host_installer_win.h (right): http://codereview.chromium.org/11054006/diff/76003/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer_win.h:18: using content::BrowserThread; "using" in a header like this is banned. http://codereview.chromium.org/11054006/diff/76003/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer_win.h:26: class AppHostInstaller { why is this class public? consider putting EnsureAppHostInstalled in namespace scope and making this whole class an implementation detail in the .cc file.
http://codereview.chromium.org/11054006/diff/76003/chrome/browser/extensions/... File chrome/browser/extensions/app_host_installer_win.h (right): http://codereview.chromium.org/11054006/diff/76003/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer_win.h:26: class AppHostInstaller { On 2012/11/02 20:44:42, grt wrote: > why is this class public? consider putting EnsureAppHostInstalled in namespace > scope and making this whole class an implementation detail in the .cc file. It's lacking unit tests in the current CL. I would like to add some, and for that it will need to have a header.
PTAL. https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... File chrome/browser/extensions/app_host_installer_win.h (right): https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_win.h:18: using content::BrowserThread; On 2012/11/02 20:44:42, grt wrote: > "using" in a header like this is banned. Done (deleted). https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_win.h:26: class AppHostInstaller { Leaving it the way it is (for now). https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_win.h:61: base::win::ScopedHandle process_; On 2012/11/02 20:40:59, erikwright wrote: > #include "base/win/scoped_handle.h" Done. https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_host_installer_win.h:62: scoped_ptr<base::win::ObjectWatcher::Delegate> delegate_; On 2012/11/02 20:40:59, erikwright wrote: > #include "base/memory/scoped_ptr.h" Done. https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/76003/chrome/browser/ext... chrome/browser/extensions/app_shortcut_manager.cc:54: : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)), On 2012/11/02 20:40:59, erikwright wrote: > #include "base/compiler_specific.h" Done.
http://codereview.chromium.org/11054006/diff/76003/chrome/browser/extensions/... File chrome/browser/extensions/app_host_installer_win.h (right): http://codereview.chromium.org/11054006/diff/76003/chrome/browser/extensions/... chrome/browser/extensions/app_host_installer_win.h:26: class AppHostInstaller { On 2012/11/02 22:15:33, huangs1 wrote: > Leaving it the way it is (for now). I'd prefer this to all be hidden, and when the tests come make what you need public then. http://codereview.chromium.org/11054006/diff/72004/chrome/browser/extensions/... File chrome/browser/extensions/app_shortcut_manager.cc (right): http://codereview.chromium.org/11054006/diff/72004/chrome/browser/extensions/... chrome/browser/extensions/app_shortcut_manager.cc:96: weak_factory_.GetWeakPtr(), extension)); What happens if the app shortcut manager is freed while one of these requests is in progress?
lgtm. Would prefer the installer implementation hidden but that can come in a clean up patch, if the tests don't come soon. http://codereview.chromium.org/11054006/diff/72004/chrome/browser/extensions/... File chrome/browser/extensions/app_shortcut_manager.cc (right): http://codereview.chromium.org/11054006/diff/72004/chrome/browser/extensions/... chrome/browser/extensions/app_shortcut_manager.cc:96: weak_factory_.GetWeakPtr(), extension)); On 2012/11/05 07:44:33, benwells wrote: > What happens if the app shortcut manager is freed while one of these requests is > in progress? Sorry, my mistake, got mixed up with Unretained. Carry on.
lgtm http://codereview.chromium.org/11054006/diff/72004/chrome/browser/extensions/... File chrome/browser/extensions/app_shortcut_manager.cc (right): http://codereview.chromium.org/11054006/diff/72004/chrome/browser/extensions/... chrome/browser/extensions/app_shortcut_manager.cc:55: : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)), move this after tracker_'s initialization to fix linux builder break
lgtm
PTAL. https://chromiumcodereview.appspot.com/11054006/diff/72004/chrome/browser/ext... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://chromiumcodereview.appspot.com/11054006/diff/72004/chrome/browser/ext... chrome/browser/extensions/app_shortcut_manager.cc:55: : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)), On 2012/11/05 16:58:30, grt wrote: > move this after tracker_'s initialization to fix linux builder break Done. Thanks!!
brettw: PTAL for chrome_browser_extensions.gypi .
TBR'ing brettw for simple .gypi change. Feel free to uncheck CQ if you wish strongly to review this.
TBR'ing brettw for simple .gypi change. Feel free to uncheck CQ if you wish strongly to review this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/11054006/72013
Retried try job too often for step(s) compile
huangs: Please add this explicit out-of-line destructor. From linux_clang: In file included from ../../chrome/browser/extensions/extension_service.h:22: ../../chrome/browser/extensions/app_shortcut_manager.h:20:1:error: [chromium-style] Complex class/struct needs an explicit out-of-line destructor. class AppShortcutManager : public ImageLoadingTracker::Observer,
@erikwright: Thanks for looking into this. Updated, but will run try job first.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/11054006/79004
Change committed as 166640 |
