Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15724019:
Recreate shortcuts on app update. (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. |
DescriptionRecreate shortcuts on app update.
At the moment, if an app's name or icon changes, the shim is not updated.
This change updates the shim by deleting existing app shims and recreating
them.
Also launch the internal copy of a shim if the one in the applications directory
does not exist.
BUG=168080
TEST=Load an unbundled app.
Create a shortcut for it.
Change the app's name and reload it.
The app shim's display name should be updated.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207481
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #
Total comments: 10
Patch Set 3 : Address comments #Patch Set 4 : Apps folder is app_path.DirName(), not BaseName() #
Total comments: 9
Patch Set 5 : Address comments #Patch Set 6 : Include profile dir in bundle id. #
Total comments: 20
Patch Set 7 : Address comments. Sync and rebase. #Patch Set 8 : Sync and rebase #
Total comments: 11
Patch Set 9 : Missed a bit in the rebase. #Patch Set 10 : Address comments #
Total comments: 16
Patch Set 11 : Address comments #Patch Set 12 : Rebase for http://crrev.com/17202004/ #
Total comments: 2
Patch Set 13 : Sync and rebase #
Messages
Total messages: 26 (0 generated)
https://codereview.chromium.org/15724019/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:451: file_util::Delete(shortcut_creator.GetShortcutPath(), true); web_app_win.cc also checks whether the `Chrome Apps` folder becomes empty. Let's do the same. https://codereview.chromium.org/15724019/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:461: shortcut_creator.CreateShortcut(); I wonder if there are any downsides to copying over it in-place. E.g. web_app_win will delete things first. And Mac does complain sometimes. E.g. If I try to rename an applications that's running, I get a "Are you sure you want to change the name of foo while it's open" dialog. I can't really think of a downside right now.. But if/when we switch to writing multiple locales in the display name, then you can end up leaving stray files around if an extension deletes a locale in an update. Maybe that's subtle enough that we will forget. How much uglier is it to delete-first? (and is OSX OK with that?). I think chrome installs new versions to a subfolder of its .app and then atomically switches over somehow.
Also made it only reveal in finder when creating a new shortcut, not for updates. https://codereview.chromium.org/15724019/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:451: file_util::Delete(shortcut_creator.GetShortcutPath(), true); On 2013/06/11 06:40:08, tapted wrote: > web_app_win.cc also checks whether the `Chrome Apps` folder becomes empty. Let's > do the same. Done. https://codereview.chromium.org/15724019/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:461: shortcut_creator.CreateShortcut(); Seems to be fine to delete first. The app is restarted on update, so the shim shuts down anyway.
Let's dress up the CL description a bit too. For non-trivial changes, they should always have a bit of "why" it's being done, as well as some "how". i.e. currently, if an extension updates its name/icon, the shim will not be updated. how=reuse shortcut creation code after deleting the old shim. Also needs a TEST= https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:453: base::FilePath app_path = shortcut_creator.GetShortcutPath(); We should check if this is empty. (say... both Applications folders became write-protected since it installed). otherwise... https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:455: base::FilePath apps_folder = app_path.BaseName(); ... this would become "." ... https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:457: file_util::Delete(apps_folder, false); ... and this could try to delete some random folder. Maybe. Also, do things typically check the return value of file_util::Delete? I guess web_app_win.cc doesn't, so maybe it's fine. https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:462: const string16& /*old_app_title*/, nit: I like it better without this commented out ;). -Wshadow isn't coming to chrome any time soon.. https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:467: file_util::Delete(shortcut_creator.GetShortcutPath(), true); Check for empty here too -- passing empty strings to Delete sounds scary.
https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:453: base::FilePath app_path = shortcut_creator.GetShortcutPath(); On 2013/06/12 08:17:52, tapted wrote: > We should check if this is empty. (say... both Applications folders became > write-protected since it installed). otherwise... Done. https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:455: base::FilePath apps_folder = app_path.BaseName(); On 2013/06/12 08:17:52, tapted wrote: > ... this would become "." ... Done. https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:457: file_util::Delete(apps_folder, false); On 2013/06/12 08:17:52, tapted wrote: > ... and this could try to delete some random folder. Maybe. > > Also, do things typically check the return value of file_util::Delete? I guess > web_app_win.cc doesn't, so maybe it's fine. There's not much we can do if it did not succeed. https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:462: const string16& /*old_app_title*/, On 2013/06/12 08:17:52, tapted wrote: > nit: I like it better without this commented out ;). -Wshadow isn't coming to > chrome any time soon.. Done. https://codereview.chromium.org/15724019/diff/5001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:467: file_util::Delete(shortcut_creator.GetShortcutPath(), true); On 2013/06/12 08:17:52, tapted wrote: > Check for empty here too -- passing empty strings to Delete sounds scary. Done.
lgtm
benwells, could you please review for OWNERS?
https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (left): https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:451: base::FilePath bundle_path = GetAppBundleByExtensionId(info.extension_id); What is the effect of replacing GetAppBundleByExtensionId with GetShortcutPath? The previous function seemed to do some sort of search (LSFindApplicationForInfo), does that mean it could update shims which had been moved? https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:375: selectFile:base::mac::FilePathToNSString(GetShortcutPath()) GetShortcutPath can fail, so this should check it's result. https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:414: base::FilePath GetAppBundleByExtensionId(std::string extension_id) { This function doesn't appear to be used anymore. https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:467: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); Make sure you handle the shim created in the web_app path as well. (i.e. update after https://codereview.chromium.org/16304005/ lands, or update that patch after this lands).
https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (left): https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:451: base::FilePath bundle_path = GetAppBundleByExtensionId(info.extension_id); On 2013/06/13 00:09:07, benwells wrote: > What is the effect of replacing GetAppBundleByExtensionId with GetShortcutPath? > The previous function seemed to do some sort of search > (LSFindApplicationForInfo), does that mean it could update shims which had been > moved? LSFindApplicationForInfo finds one app bundle that matches the bundle ID. I couldn't find a similar function that returns all app bundles matching that ID. Since we are going to have multiple copies of the same bundle, and they have unique paths, it seems better to find them by path. If the user moves or renames one, we stop updating it. An alternative would be to give each bundle a unique ID, e.g. by appending the profile dir, and whether it's the visible or hidden bundle. https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:375: selectFile:base::mac::FilePathToNSString(GetShortcutPath()) On 2013/06/13 00:09:07, benwells wrote: > GetShortcutPath can fail, so this should check it's result. Done. https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:414: base::FilePath GetAppBundleByExtensionId(std::string extension_id) { On 2013/06/13 00:09:07, benwells wrote: > This function doesn't appear to be used anymore. Removed. https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:467: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); On 2013/06/13 00:09:07, benwells wrote: > Make sure you handle the shim created in the web_app path as well. (i.e. update > after https://codereview.chromium.org/16304005/ lands, or update that patch > after this lands). I'll merge the two CLs since they touch the same files.
On 2013/06/13 01:47:41, jackhou1 wrote: > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_mac.mm (left): > > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:451: base::FilePath bundle_path = > GetAppBundleByExtensionId(info.extension_id); > On 2013/06/13 00:09:07, benwells wrote: > > What is the effect of replacing GetAppBundleByExtensionId with > GetShortcutPath? > > The previous function seemed to do some sort of search > > (LSFindApplicationForInfo), does that mean it could update shims which had > been > > moved? > > LSFindApplicationForInfo finds one app bundle that matches the bundle ID. I > couldn't find a similar function that returns all app bundles matching that ID. > Since we are going to have multiple copies of the same bundle, and they have > unique paths, it seems better to find them by path. If the user moves or renames > one, we stop updating it. An alternative would be to give each bundle a unique > ID, e.g. by appending the profile dir, and whether it's the visible or hidden > bundle. > > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_mac.mm (right): > > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:375: > selectFile:base::mac::FilePathToNSString(GetShortcutPath()) > On 2013/06/13 00:09:07, benwells wrote: > > GetShortcutPath can fail, so this should check it's result. > > Done. > > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:414: base::FilePath > GetAppBundleByExtensionId(std::string extension_id) { > On 2013/06/13 00:09:07, benwells wrote: > > This function doesn't appear to be used anymore. > > Removed. > > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:467: > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); > On 2013/06/13 00:09:07, benwells wrote: > > Make sure you handle the shim created in the web_app path as well. (i.e. > update > > after https://codereview.chromium.org/16304005/ lands, or update that patch > > after this lands). > > I'll merge the two CLs since they touch the same files. PTAL. Not merging them. I'll let the two CLs fight it out on the commit arena and update the loser.
https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (left): https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:451: base::FilePath bundle_path = GetAppBundleByExtensionId(info.extension_id); On 2013/06/13 01:47:42, jackhou1 wrote: > On 2013/06/13 00:09:07, benwells wrote: > > What is the effect of replacing GetAppBundleByExtensionId with > GetShortcutPath? > > The previous function seemed to do some sort of search > > (LSFindApplicationForInfo), does that mean it could update shims which had > been > > moved? > > LSFindApplicationForInfo finds one app bundle that matches the bundle ID. I > couldn't find a similar function that returns all app bundles matching that ID. > Since we are going to have multiple copies of the same bundle, and they have > unique paths, it seems better to find them by path. If the user moves or renames > one, we stop updating it. An alternative would be to give each bundle a unique > ID, e.g. by appending the profile dir, and whether it's the visible or hidden > bundle. Your alternative sounds pretty good. Any reason not to do this? It would have the advantage of updates being more robust, and removing shims after uninstall being more robust as well.
On 2013/06/13 02:19:31, benwells wrote: > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_mac.mm (left): > > https://codereview.chromium.org/15724019/diff/16001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_mac.mm:451: base::FilePath bundle_path = > GetAppBundleByExtensionId(info.extension_id); > On 2013/06/13 01:47:42, jackhou1 wrote: > > On 2013/06/13 00:09:07, benwells wrote: > > > What is the effect of replacing GetAppBundleByExtensionId with > > GetShortcutPath? > > > The previous function seemed to do some sort of search > > > (LSFindApplicationForInfo), does that mean it could update shims which had > > been > > > moved? > > > > LSFindApplicationForInfo finds one app bundle that matches the bundle ID. I > > couldn't find a similar function that returns all app bundles matching that > ID. > > Since we are going to have multiple copies of the same bundle, and they have > > unique paths, it seems better to find them by path. If the user moves or > renames > > one, we stop updating it. An alternative would be to give each bundle a unique > > ID, e.g. by appending the profile dir, and whether it's the visible or hidden > > bundle. > > Your alternative sounds pretty good. Any reason not to do this? It would have > the advantage of updates being more robust, and removing shims after uninstall > being more robust as well. I'm not sure if anything else depends on the bundle id being what it is now. But I think it makes sense to do this, and update anything that breaks. It's only able to find one bundle, but it will update and delete a bundle that has been moved from the Chrome Apps directory.
PTAL
On 2013/06/17 04:44:11, jackhou1 wrote: > PTAL tapted, are you gonna look at this again? It has changed quite a bit since your lg.
https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:50: // Returns a path to the Chrome Apps folder. maybe mention that it's a subfolder of /Applications or ~/Applications since there will be multiple destination paths https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:53: // Creates shortcuts for the given shortcut info. I don't think these comments on FooShortcut[s]() are adding anything https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:54: bool CreateShortcut(); make this plural to be consistent? (also, it's true now ;) https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:74: // Here so it can be mocked out for testing. nit: "Here" -> Protected and virtual https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:95: base::FilePath user_data_dir_; has the change that moved this landed?. rebase? https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:162: void DeleteShortcut(base::FilePath app_path) { const reference https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:162: void DeleteShortcut(base::FilePath app_path) { Maybe a more descriptive name, to distinguish it from the member function. E.g. DeletePathAndParentIfEmpty https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:250: } nit: blank line after for early return https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:422: base::mac::ScopedCFTypeRef<CFURLRef> url(url_ref); move this below checking status != noErr? https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac_unittest.mm (right): https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac_unittest.mm:123: EXPECT_CALL(shortcut_creator, GetAppBundleById(expected_bundle_id)) Can you test the failure case, with GetAppBundleById returning an empty base::FilePath() ?
https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:50: // Returns a path to the Chrome Apps folder. On 2013/06/18 06:54:59, tapted wrote: > maybe mention that it's a subfolder of /Applications or ~/Applications since > there will be multiple destination paths Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:53: // Creates shortcuts for the given shortcut info. On 2013/06/18 06:54:59, tapted wrote: > I don't think these comments on FooShortcut[s]() are adding anything Removed. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:54: bool CreateShortcut(); On 2013/06/18 06:54:59, tapted wrote: > make this plural to be consistent? (also, it's true now ;) Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:74: // Here so it can be mocked out for testing. On 2013/06/18 06:54:59, tapted wrote: > nit: "Here" -> Protected and virtual Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.h:95: base::FilePath user_data_dir_; On 2013/06/18 06:54:59, tapted wrote: > has the change that moved this landed?. rebase? Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:162: void DeleteShortcut(base::FilePath app_path) { On 2013/06/18 06:54:59, tapted wrote: > const reference Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:162: void DeleteShortcut(base::FilePath app_path) { On 2013/06/18 06:54:59, tapted wrote: > Maybe a more descriptive name, to distinguish it from the member function. E.g. > DeletePathAndParentIfEmpty Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:250: } On 2013/06/18 06:54:59, tapted wrote: > nit: blank line after for early return Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:422: base::mac::ScopedCFTypeRef<CFURLRef> url(url_ref); On 2013/06/18 06:54:59, tapted wrote: > move this below checking status != noErr? Done. https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac_unittest.mm (right): https://codereview.chromium.org/15724019/diff/24001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac_unittest.mm:123: EXPECT_CALL(shortcut_creator, GetAppBundleById(expected_bundle_id)) On 2013/06/18 06:54:59, tapted wrote: > Can you test the failure case, with GetAppBundleById returning an empty > base::FilePath() ? Done.
https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:299: } this curly needs to be unindented one space https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:327: return false; What if app_data_path_ succeeds and app_path.DirName() fails? Do we still need to call UpdateInternalBundleIdentifier(); ? (same in UpdateShortcuts()) https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:339: DeletePathAndParentIfEmpty(GetAppBundleById(GetBundleIdentifier())); Hmm.. Unfortunately, if they've moved it, we shouldn't delete its parent folder if that folder becomes empty. I think this case can be inline here. (as a bonus, I think then the app_path.empty() check in DeletePathAndParentIfEmpty() can turned into a DCHECK). https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:354: return false; Can we still update the copy in app_data_path_ ? (and should we file_util::Delete(app_data_path_.Append(stuff)), too?) https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:500: return [plist writeToFile:plist_path atomically:YES]; nit: atomically:YES on a new line and align colons
https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:299: } On 2013/06/19 00:58:35, tapted wrote: > this curly needs to be unindented one space Done. https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:327: return false; On 2013/06/19 00:58:35, tapted wrote: > What if app_data_path_ succeeds and app_path.DirName() fails? Do we still need > to call UpdateInternalBundleIdentifier(); ? (same in UpdateShortcuts()) Done. https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:339: DeletePathAndParentIfEmpty(GetAppBundleById(GetBundleIdentifier())); On 2013/06/19 00:58:35, tapted wrote: > Hmm.. Unfortunately, if they've moved it, we shouldn't delete its parent folder > if that folder becomes empty. I think this case can be inline here. (as a bonus, > I think then the app_path.empty() check in DeletePathAndParentIfEmpty() can > turned into a DCHECK). Done. https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:354: return false; On 2013/06/19 00:58:35, tapted wrote: > Can we still update the copy in app_data_path_ ? > > (and should we file_util::Delete(app_data_path_.Append(stuff)), too?) Done. https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:500: return [plist writeToFile:plist_path atomically:YES]; On 2013/06/19 00:58:35, tapted wrote: > nit: atomically:YES on a new line and align colons Done.
lg - mostly straightforward fixes except for the question about first deleting app_data_path_ in UpdateShortcuts() https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/42001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:354: return false; On 2013/06/19 00:58:35, tapted wrote: > (and should we file_util::Delete(app_data_path_.Append(stuff)), too?) sorry this ^^ was hidden in a comment on the last patchset. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:290: return false; false -> 0 https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:296: return false; false -> 0 https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:304: return false; this should be `return succeeded` https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:337: size_t succeeded = CreateShortcutsIn(paths); nit: maybe num_succeeded or success_count, to make it less booly https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:338: if (!succeeded) nit: I think it's typically preferred to vs 0 for numbers (i.e. succeeded == 0) https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:374: paths.push_back(app_data_path_); Do we need to delete this folder, before updating it, like we do for app_path? https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:381: size_t succeeded = CreateShortcutsIn(paths); nit: less booly https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:382: if (!succeeded) nit: less booly
https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:290: return false; On 2013/06/19 08:29:40, tapted wrote: > false -> 0 Done. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:296: return false; On 2013/06/19 08:29:40, tapted wrote: > false -> 0 Done. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:304: return false; On 2013/06/19 08:29:40, tapted wrote: > this should be `return succeeded` Done. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:337: size_t succeeded = CreateShortcutsIn(paths); On 2013/06/19 08:29:40, tapted wrote: > nit: maybe num_succeeded or success_count, to make it less booly Done. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:338: if (!succeeded) On 2013/06/19 08:29:40, tapted wrote: > nit: I think it's typically preferred to vs 0 for numbers (i.e. succeeded == 0) Done. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:374: paths.push_back(app_data_path_); On 2013/06/19 08:29:40, tapted wrote: > Do we need to delete this folder, before updating it, like we do for app_path? I think whatever reasons for deleting the other one apply to this too. I.e. making sure we delete any files that should no longer be in the bundle. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:381: size_t succeeded = CreateShortcutsIn(paths); On 2013/06/19 08:29:40, tapted wrote: > nit: less booly Done. https://codereview.chromium.org/15724019/diff/45002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:382: if (!succeeded) On 2013/06/19 08:29:40, tapted wrote: > nit: less booly Done.
lgtm
benwells, PTAL
lgtm https://codereview.chromium.org/15724019/diff/63001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/63001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:171: // The user may have deleted the copy in the Applications folder, use the This feels unrelated to the CL description. Please update the description to mention it.
https://codereview.chromium.org/15724019/diff/63001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/15724019/diff/63001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_mac.mm:171: // The user may have deleted the copy in the Applications folder, use the On 2013/06/20 08:34:52, benwells wrote: > This feels unrelated to the CL description. Please update the description to > mention it. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/15724019/69001
Message was sent while issue was closed.
Change committed as 207481 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
