Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16304005:
Create a copy of each app's shim bundle in the app's app_data_path. (Closed)
|
Created:
7 years, 6 months ago by jackhou1 Modified:
7 years, 6 months ago CC:
chromium-reviews, sail+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreate a copy of each app's shim bundle in the app's app_data_path.
This allows app shims to be deleted from the Applications directory by
the user.
BUG=168080, 229086
TEST=Create a shortcut for a platform app.
Delete the shortcut.
Start the app, an app shim should appear in the dock.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206679
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments #
Total comments: 4
Patch Set 3 : Address comments. #
Total comments: 8
Patch Set 4 : Address comments #
Total comments: 7
Patch Set 5 : Address comments #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:199: LOG(ERROR) << "Creating user_data_dir " << user_data_dir_.value() I think this block can be omitted -- user_data_dir_ should always exist. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:233: LOG(ERROR) << "Copying app to user_data_dir: " << user_data_dir_.value() Here, I think `NOTREACHED(); return false;` -- web_app_win.cc behaves like this in its `CreateShortcutsInPaths`. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:240: LOG(ERROR) << "Copying app to applications directory: " << dst_path.value() Logging probably isn't desirable here either -- nobody will see it. `return false` seems to be the technique from web_app_win.cc. I think if we start getting bug reports, or add some UMA to check the return value of CreateShortcutsOnFileThread (rather than base::IngoreResult), and see problems, then we can propagate up an error string to a dialog the way extension install does. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:400: if (shim_path.empty()) can this still be true for cases we can recover from (and still care about)? E.g. /Applications and ~/Applications not writeable, but the user data dir is? I think we can just merge this into the if statement below. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:404: // user_data_dir. nit: maybe more correct to say "the web app's data dir". And, this might be clearer inside the `if` block which essentially says the first and last parts of the comment for you ;). E.g. if (..) { // User may have deleted the copy in the Applications folder. base::.. } https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac_unittest.mm (right): https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac_unittest.mm:36: const base::FilePath user_data_dir, pass as const-reference?
+chrome-apps-syd-reviews
https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:199: LOG(ERROR) << "Creating user_data_dir " << user_data_dir_.value() On 2013/06/11 06:15:12, tapted wrote: > I think this block can be omitted -- user_data_dir_ should always exist. The user_data_dir_ here belongs to the app. I.e. <user_data_dir>/Web Applications/_crx_<extension_id>/. I don't think anything creates it on Mac at the moment. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:233: LOG(ERROR) << "Copying app to user_data_dir: " << user_data_dir_.value() On 2013/06/11 06:15:12, tapted wrote: > Here, I think `NOTREACHED(); return false;` -- web_app_win.cc behaves like this > in its `CreateShortcutsInPaths`. Done. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:240: LOG(ERROR) << "Copying app to applications directory: " << dst_path.value() On 2013/06/11 06:15:12, tapted wrote: > Logging probably isn't desirable here either -- nobody will see it. `return > false` seems to be the technique from web_app_win.cc. > > I think if we start getting bug reports, or add some UMA to check the return > value of CreateShortcutsOnFileThread (rather than base::IngoreResult), and see > problems, then we can propagate up an error string to a dialog the way extension > install does. Done. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:400: if (shim_path.empty()) On 2013/06/11 06:15:12, tapted wrote: > can this still be true for cases we can recover from (and still care about)? > E.g. /Applications and ~/Applications not writeable, but the user data dir is? I > think we can just merge this into the if statement below. Done. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:404: // user_data_dir. On 2013/06/11 06:15:12, tapted wrote: > nit: maybe more correct to say "the web app's data dir". And, this might be > clearer inside the `if` block which essentially says the first and last parts of > the comment for you ;). E.g. > > if (..) { > // User may have deleted the copy in the Applications folder. > base::.. > } Done. https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac_unittest.mm (right): https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac_unittest.mm:36: const base::FilePath user_data_dir, On 2013/06/11 06:15:12, tapted wrote: > pass as const-reference? Done.
https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:199: LOG(ERROR) << "Creating user_data_dir " << user_data_dir_.value() On 2013/06/12 06:54:46, jackhou1 wrote: > On 2013/06/11 06:15:12, tapted wrote: > > I think this block can be omitted -- user_data_dir_ should always exist. > > The user_data_dir_ here belongs to the app. I.e. <user_data_dir>/Web > Applications/_crx_<extension_id>/. I don't think anything creates it on Mac at > the moment. Eep. Let's fix the name. And the header comment -- it starts off OK, and then says "Note, the user data directory is the parent of the profile directory.". Which is actually what means 'intrinsically', so the name shouldn't be used for other things. The variable is also in the wrong place -- it should come after all the functions. Maybe web_app_path_ -- this is what the argument is in the {Create,Delete,Update}PlatformShortcuts functions. Don't forget to update how it's referred to in the CL description, too.
https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:398: if (!file_util::PathExists(shim_path)) { This should be if (shim_path.empty() || !file_util::PathExists(shim_path)) https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac_unittest.mm (right): https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac_unittest.mm:36: const base::FilePath& user_data_dir, web_app_path ;). More below.
https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:199: LOG(ERROR) << "Creating user_data_dir " << user_data_dir_.value() On 2013/06/12 08:26:39, tapted wrote: > On 2013/06/12 06:54:46, jackhou1 wrote: > > On 2013/06/11 06:15:12, tapted wrote: > > > I think this block can be omitted -- user_data_dir_ should always exist. > > > > The user_data_dir_ here belongs to the app. I.e. <user_data_dir>/Web > > Applications/_crx_<extension_id>/. I don't think anything creates it on Mac at > > the moment. > > Eep. Let's fix the name. And the header comment -- it starts off OK, and then > says "Note, the user data directory is the parent of the profile directory.". > Which is actually what means 'intrinsically', so the name shouldn't be used for > other things. > > The variable is also in the wrong place -- it should come after all the > functions. > > Maybe web_app_path_ -- this is what the argument is in the > {Create,Delete,Update}PlatformShortcuts functions. > > Don't forget to update how it's referred to in the CL description, too. Done. https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:398: if (!file_util::PathExists(shim_path)) { On 2013/06/12 08:37:56, tapted wrote: > This should be > > if (shim_path.empty() || !file_util::PathExists(shim_path)) Done. https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac_unittest.mm (right): https://codereview.chromium.org/16304005/diff/7001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac_unittest.mm:36: const base::FilePath& user_data_dir, On 2013/06/12 08:37:56, tapted wrote: > web_app_path ;). More below. Done.
lgtm Although I wonder if we should just purge all the logging from these files.. maybe after fishfood, or adding uma for shortcut creation.
benwells, could you please review for OWNERS
https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:36: // The shortcut stores its user data directory in |web_app_path|. This comment doesn't make sense, it was from an earlier era where we ran apps in their own user data directory. Update it to describe what web_app_path is now used for. https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:81: // Path to the app's user data directory. For example: Ditto. This isn't the app's user data directory. https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:230: // Put one copy in the app's web_app_path so we can still run it if the user What happens if the shortcut is being created in response to the create shortcuts dialog? The shim might already exist in the web app path.
https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:36: // The shortcut stores its user data directory in |web_app_path|. On 2013/06/13 00:13:31, benwells wrote: > This comment doesn't make sense, it was from an earlier era where we ran apps in > their own user data directory. > > Update it to describe what web_app_path is now used for. Done. https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:81: // Path to the app's user data directory. For example: On 2013/06/13 00:13:31, benwells wrote: > Ditto. This isn't the app's user data directory. Done. https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:230: // Put one copy in the app's web_app_path so we can still run it if the user On 2013/06/13 00:13:31, benwells wrote: > What happens if the shortcut is being created in response to the create > shortcuts dialog? The shim might already exist in the web app path. It will replace the shim with the same files (though some contents of those files change). E.g. the profile name could have changed in the mean time. Though I realised in the update case, we don't want to create a new shortcut in the apps folder if it has been deleted. I'll update the other CL.
https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:230: // Put one copy in the app's web_app_path so we can still run it if the user On 2013/06/13 01:33:43, jackhou1 wrote: > On 2013/06/13 00:13:31, benwells wrote: > > What happens if the shortcut is being created in response to the create > > shortcuts dialog? The shim might already exist in the web app path. > > It will replace the shim with the same files (though some contents of those > files change). E.g. the profile name could have changed in the mean time. > > Though I realised in the update case, we don't want to create a new shortcut in > the apps folder if it has been deleted. I'll update the other CL. On other platforms we have used the shortcut location struct as an argument to all these functions to control where we put shortcuts. It feels like this would be nicer if we did the same here. Then when a shortcut is added by the user we just add it in the applications folder. When we are adding it automatically we add it to both. The more commonality between the platforms' code the better. WDYT? https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:242: Nit: is the new blank line needed? https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:298: forKey:app_mode::kCrAppModeUserDataDirKey]; Is this property in the plist still needed? I have a hunch it can be removed.
forgot this one ... https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:39: const base::FilePath& web_app_path, nit: web_app_path is a bad name. These aren't web apps, they are local apps. Maybe rename web_app_path -> app_data_path.
https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:230: // Put one copy in the app's web_app_path so we can still run it if the user On 2013/06/13 03:57:43, benwells wrote: > On 2013/06/13 01:33:43, jackhou1 wrote: > > On 2013/06/13 00:13:31, benwells wrote: > > > What happens if the shortcut is being created in response to the create > > > shortcuts dialog? The shim might already exist in the web app path. > > > > It will replace the shim with the same files (though some contents of those > > files change). E.g. the profile name could have changed in the mean time. > > > > Though I realised in the update case, we don't want to create a new shortcut > in > > the apps folder if it has been deleted. I'll update the other CL. > > On other platforms we have used the shortcut location struct as an argument to > all these functions to control where we put shortcuts. It feels like this would > be nicer if we did the same here. Then when a shortcut is added by the user we > just add it in the applications folder. When we are adding it automatically we > add it to both. > > The more commonality between the platforms' code the better. > > WDYT? At the moment, there's no dialog to choose where to create shortcuts. So as far as the user can tell, we're just making a shortcut in the Chrome Apps folder. In terms of not creating it in the app_data_path, I think it would be better to do that when app shims are enabled for good. At the moment, we don't know if there is a copy in the app_data_path, or if that copy is up to date. So I'd prefer that "Create shortcuts" recreates all shortcuts to a consistent state. https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:39: const base::FilePath& web_app_path, On 2013/06/13 04:40:33, benwells wrote: > nit: web_app_path is a bad name. These aren't web apps, they are local apps. > Maybe rename web_app_path -> app_data_path. Done. https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:242: On 2013/06/13 03:57:43, benwells wrote: > Nit: is the new blank line needed? Done. https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:298: forKey:app_mode::kCrAppModeUserDataDirKey]; On 2013/06/13 03:57:43, benwells wrote: > Is this property in the plist still needed? I have a hunch it can be removed. Right now, most of the kCrAppMode* entries are unused. app_mode_loader_mac.mm reads all of them and passes them into ChromeAppModeStart in chrome_main_app_mode_mac.mm. I think they're there in case we want to use them in future.
https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:298: forKey:app_mode::kCrAppModeUserDataDirKey]; On 2013/06/14 03:31:27, jackhou1 wrote: > On 2013/06/13 03:57:43, benwells wrote: > > Is this property in the plist still needed? I have a hunch it can be removed. > > Right now, most of the kCrAppMode* entries are unused. app_mode_loader_mac.mm > reads all of them and passes them into ChromeAppModeStart in > chrome_main_app_mode_mac.mm. I think they're there in case we want to use them > in future. We should review them and remove any that aren't used, like this one. This was useful in a previous incarnation of app shims, but now it is just a historical artifact.
On 2013/06/14 03:31:27, jackhou1 wrote: > https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_mac.mm (right): > > https://codereview.chromium.org/16304005/diff/14001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:230: // Put one copy in the app's > web_app_path so we can still run it if the user > On 2013/06/13 03:57:43, benwells wrote: > > On 2013/06/13 01:33:43, jackhou1 wrote: > > > On 2013/06/13 00:13:31, benwells wrote: > > > > What happens if the shortcut is being created in response to the create > > > > shortcuts dialog? The shim might already exist in the web app path. > > > > > > It will replace the shim with the same files (though some contents of those > > > files change). E.g. the profile name could have changed in the mean time. > > > > > > Though I realised in the update case, we don't want to create a new shortcut > > in > > > the apps folder if it has been deleted. I'll update the other CL. > > > > On other platforms we have used the shortcut location struct as an argument to > > all these functions to control where we put shortcuts. It feels like this > would > > be nicer if we did the same here. Then when a shortcut is added by the user we > > just add it in the applications folder. When we are adding it automatically we > > add it to both. > > > > The more commonality between the platforms' code the better. > > > > WDYT? > > At the moment, there's no dialog to choose where to create shortcuts. So as far > as the user can tell, we're just making a shortcut in the Chrome Apps folder. > > In terms of not creating it in the app_data_path, I think it would be better to > do that when app shims are enabled for good. At the moment, we don't know if > there is a copy in the app_data_path, or if that copy is up to date. So I'd > prefer that "Create shortcuts" recreates all shortcuts to a consistent state. > Hmmmm. OK, I don't quite agree but don't think it matters at the moment. > https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_mac.h (right): > > https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.h:39: const base::FilePath& > web_app_path, > On 2013/06/13 04:40:33, benwells wrote: > > nit: web_app_path is a bad name. These aren't web apps, they are local apps. > > Maybe rename web_app_path -> app_data_path. > > Done. > > https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_mac.mm (right): > > https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:242: > On 2013/06/13 03:57:43, benwells wrote: > > Nit: is the new blank line needed? > > Done. > > https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:298: > forKey:app_mode::kCrAppModeUserDataDirKey]; > On 2013/06/13 03:57:43, benwells wrote: > > Is this property in the plist still needed? I have a hunch it can be removed. > > Right now, most of the kCrAppMode* entries are unused. app_mode_loader_mac.mm > reads all of them and passes them into ChromeAppModeStart in > chrome_main_app_mode_mac.mm. I think they're there in case we want to use them > in future.
On 2013/06/14 03:36:40, benwells wrote: > https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_mac.mm (right): > > https://codereview.chromium.org/16304005/diff/22001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:298: > forKey:app_mode::kCrAppModeUserDataDirKey]; > On 2013/06/14 03:31:27, jackhou1 wrote: > > On 2013/06/13 03:57:43, benwells wrote: > > > Is this property in the plist still needed? I have a hunch it can be > removed. > > > > Right now, most of the kCrAppMode* entries are unused. app_mode_loader_mac.mm > > reads all of them and passes them into ChromeAppModeStart in > > chrome_main_app_mode_mac.mm. I think they're there in case we want to use them > > in future. > > We should review them and remove any that aren't used, like this one. This was > useful in a previous incarnation of app shims, but now it is just a historical > artifact. I'll put together a CL to clean them up.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/16304005/28001
Message was sent while issue was closed.
Change committed as 206679 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
