|
|
Created:
7 years, 10 months ago by jeremya Modified:
7 years, 10 months ago CC:
chromium-reviews, Aaron Boodman, sail+watch_chromium.org, chromium-apps-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[mac] Delete app shortcuts on app uninstall.
When an app is uninstalled, find the .app bundle it's associated with using
Launch Services, and delete it.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182077
Patch Set 1 #
Total comments: 16
Patch Set 2 : comments #
Total comments: 7
Patch Set 3 : comments #
Total comments: 6
Patch Set 4 : nits #Patch Set 5 : rebase #Patch Set 6 : fix rebase #
Messages
Total messages: 17 (0 generated)
benwells- apps-related review and OWNERS avi- for the mac stuff, specifically the LS usage
https://codereview.chromium.org/12178030/diff/1/chrome/browser/extensions/app... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://codereview.chromium.org/12178030/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_shortcut_manager.cc:86: #endif Can you label some of these? It's hard to keep track of what ends here... https://codereview.chromium.org/12178030/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_shortcut_manager.cc:88: #endif ...and here. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:262: CFURLRef* kDontWantURL = NULL; This isn't a constant so don't name it as such. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:265: base::mac::BaseBundleID() + std::string(".app.") + extension_id; This isn't Objective-C code. This (and other variable names) should_be_named_like_this. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:267: CFStringCreateWithCString(NULL, bundleID.c_str(), kCFStringEncodingUTF8); SysUTF8ToCFStringRef. Also, don't leak this string, please. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:269: NULL, &ref, kDontWantURL); I don't understand. Why do you pass NULL for the inName parameter but use a fake constant (kDontWantURL) for the last? FSRefs are long obsolete. I'd much rather you get a CFURLRef back and convert it rather than use an FSRef. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:273: return FilePath(base::mac::PathFromFSRef(ref)); PathFromFSRef... ew. If we have to deal with APIs that use FSRefs then find, but here we have an option, so let's not use it.
https://codereview.chromium.org/12178030/diff/1/chrome/browser/extensions/app... File chrome/browser/extensions/app_shortcut_manager.cc (right): https://codereview.chromium.org/12178030/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_shortcut_manager.cc:86: #endif On 2013/02/05 00:19:25, Avi wrote: > Can you label some of these? It's hard to keep track of what ends here... Done. https://codereview.chromium.org/12178030/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_shortcut_manager.cc:88: #endif On 2013/02/05 00:19:25, Avi wrote: > ...and here. Done. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:265: base::mac::BaseBundleID() + std::string(".app.") + extension_id; On 2013/02/05 00:19:25, Avi wrote: > This isn't Objective-C code. This (and other variable names) > should_be_named_like_this. Done. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:267: CFStringCreateWithCString(NULL, bundleID.c_str(), kCFStringEncodingUTF8); On 2013/02/05 00:19:25, Avi wrote: > SysUTF8ToCFStringRef. Also, don't leak this string, please. Done. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:269: NULL, &ref, kDontWantURL); On 2013/02/05 00:19:25, Avi wrote: > I don't understand. Why do you pass NULL for the inName parameter but use a fake > constant (kDontWantURL) for the last? > > FSRefs are long obsolete. I'd much rather you get a CFURLRef back and convert it > rather than use an FSRef. I cargo-culted from chrome_service_application_mac.mm. Fixed :) https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:273: return FilePath(base::mac::PathFromFSRef(ref)); On 2013/02/05 00:19:25, Avi wrote: > PathFromFSRef... ew. If we have to deal with APIs that use FSRefs then find, but > here we have an option, so let's not use it. Done.
https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:269: NULL, &ref, kDontWantURL); Would you be so kind as to fix it there too? :) https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:264: base::mac::BaseBundleID() + std::string(".app.") + extension_id; BTW, do we use this bundle id elsewhere? Surely we need to build it when we spawn the app bundle. This smells like this should be a utility function somewhere called by both the creator and deleter of app bundles. https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:266: base::SysUTF8ToCFStringRef(bundle_id.c_str())); Why c_str()? SysUTF8ToCFStringRef takes a std::string&. https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:275: NSString *path_string = [(NSURL *)url.get() path]; CFToNSCast. And spaces go after the *s.
c/b/e lgtm
+ajwong for chrome/service OWNERS. Looks like the original author of that code has left google. Also I have no idea how to test that it still works, but it compiles. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:269: NULL, &ref, kDontWantURL); On 2013/02/05 04:01:40, Avi wrote: > Would you be so kind as to fix it there too? :) I fixed some things there, but it needs an FSRef for LSOpenApplication. https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:264: base::mac::BaseBundleID() + std::string(".app.") + extension_id; On 2013/02/05 04:01:40, Avi wrote: > BTW, do we use this bundle id elsewhere? Surely we need to build it when we > spawn the app bundle. This smells like this should be a utility function > somewhere called by both the creator and deleter of app bundles. We load up a string with a @PLACEHOLDER@ from a plist on disk when we spawn the app bundle. I figured that wasn't really necessary here since it's got a static form. Perhaps it should be extracted so the other code doesn't do the @PLACEHOLDER@ replace and instead uses this logic. https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:266: base::SysUTF8ToCFStringRef(bundle_id.c_str())); On 2013/02/05 04:01:40, Avi wrote: > Why c_str()? SysUTF8ToCFStringRef takes a std::string&. Fixed https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:275: NSString *path_string = [(NSURL *)url.get() path]; On 2013/02/05 04:01:40, Avi wrote: > CFToNSCast. And spaces go after the *s. Done.
lgtm Who wrote that cloud print code? It's leaky as hell. https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/1/chrome/browser/web_applicatio... chrome/browser/web_applications/web_app_mac.mm:269: NULL, &ref, kDontWantURL); Yeah, that falls under the "no non-deprecated way to do it". Apple's not immune to that :) https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/12178030/diff/6001/chrome/browser/web_applica... chrome/browser/web_applications/web_app_mac.mm:264: base::mac::BaseBundleID() + std::string(".app.") + extension_id; Huh. Eventually we should unify things, but not in this CL. https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... File chrome/service/chrome_service_application_mac.mm (left): https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... chrome/service/chrome_service_application_mac.mm:33: CFStringCreateWithCString(NULL, silent.c_str(), kCFStringEncodingUTF8); Yikes. This leaked... https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... chrome/service/chrome_service_application_mac.mm:37: CFArrayCreate(NULL, (const void**) flags, 1, &kCFTypeArrayCallBacks); Did this leak? What does LSApplicationParameters require? Offhand, I don't know what's correct. https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... chrome/service/chrome_service_application_mac.mm:43: CFStringCreateWithCString(NULL, bundleID.c_str(), kCFStringEncodingUTF8); .. and this clearly leaked. https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... File chrome/service/chrome_service_application_mac.mm (right): https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... chrome/service/chrome_service_application_mac.mm:47: NULL, &ref, kDontWantURL); Put a null in this last parameter rather than this fake constant.
according to git blame, abeera@google.com wrote it, but they don't seem to be working at Google any more. https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... File chrome/service/chrome_service_application_mac.mm (left): https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... chrome/service/chrome_service_application_mac.mm:37: CFArrayCreate(NULL, (const void**) flags, 1, &kCFTypeArrayCallBacks); On 2013/02/05 05:15:49, Avi wrote: > Did this leak? What does LSApplicationParameters require? Offhand, I don't know > what's correct. I think so. I can't find any documentation saying that LSApplicationParameters/LSOpenApplication takes ownership of the array. https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... File chrome/service/chrome_service_application_mac.mm (right): https://codereview.chromium.org/12178030/diff/12001/chrome/service/chrome_ser... chrome/service/chrome_service_application_mac.mm:47: NULL, &ref, kDontWantURL); On 2013/02/05 05:15:49, Avi wrote: > Put a null in this last parameter rather than this fake constant. Done.
My "who wrote this" question was more rhetorical, but oh well. SLGTM
service rubberstamp lgtm AFAICT, this is just fixing a pretty obviously looking memory leak and doesn't affect any real behavior (not, I don't know this part of the code either). Let me know if it warrant more deep inspection.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12178030/17001
Failed to apply patch for chrome/browser/web_applications/web_app_mac.mm: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file chrome/browser/web_applications/web_app_mac.mm Hunk #2 succeeded at 258 with fuzz 2 (offset 1 line). Hunk #3 FAILED at 288. 1 out of 3 hunks FAILED -- saving rejects to file chrome/browser/web_applications/web_app_mac.mm.rej Patch: chrome/browser/web_applications/web_app_mac.mm Index: chrome/browser/web_applications/web_app_mac.mm diff --git a/chrome/browser/web_applications/web_app_mac.mm b/chrome/browser/web_applications/web_app_mac.mm index be02bc52af382199effa6cfb1d79ab4e380be2e1..f51d0d7d9f8690183edfd22585930162ad80e390 100644 --- a/chrome/browser/web_applications/web_app_mac.mm +++ b/chrome/browser/web_applications/web_app_mac.mm @@ -10,7 +10,9 @@ #include "base/files/scoped_temp_dir.h" #include "base/mac/bundle_locations.h" #include "base/mac/foundation_util.h" +#include "base/mac/mac_logging.h" #include "base/mac/mac_util.h" +#include "base/mac/scoped_cftyperef.h" #include "base/memory/scoped_nsobject.h" #include "base/sys_string_conversions.h" #include "base/utf_string_conversions.h" @@ -255,6 +257,25 @@ namespace web_app { namespace internals { +FilePath GetAppBundleByExtensionId(std::string extension_id) { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); + // This matches APP_MODE_APP_BUNDLE_ID in chrome/chrome.gyp. + std::string bundle_id = + base::mac::BaseBundleID() + std::string(".app.") + extension_id; + base::mac::ScopedCFTypeRef<CFStringRef> bundle_id_cf( + base::SysUTF8ToCFStringRef(bundle_id)); + CFURLRef url_ref = NULL; + OSStatus status = LSFindApplicationForInfo( + kLSUnknownCreator, bundle_id_cf.get(), NULL, NULL, &url_ref); + base::mac::ScopedCFTypeRef<CFURLRef> url(url_ref); + + if (status != noErr) + return FilePath(); + + NSString* path_string = [base::mac::CFToNSCast(url.get()) path]; + return FilePath([path_string fileSystemRepresentation]); +} + bool CreatePlatformShortcuts( const FilePath& web_app_path, const ShellIntegration::ShortcutInfo& shortcut_info) { @@ -267,9 +288,11 @@ bool CreatePlatformShortcuts( void DeletePlatformShortcuts( const FilePath& web_app_path, - const ShellIntegration::ShortcutInfo& shortcut_info) { - // TODO(benwells): Implement this when shortcuts / weblings are enabled on - // mac. + const ShellIntegration::ShortcutInfo& info) { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); + + FilePath bundle_path = GetAppBundleByExtensionId(info.extension_id); + file_util::Delete(bundle_path, true); } void UpdatePlatformShortcuts(
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12178030/8003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12178030/19003
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12178030/19003
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... |