Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9147031:
Functions to return locations of various Chrome bundles. (Closed)
|
Created:
8 years, 11 months ago by jeremy Modified:
8 years, 11 months ago CC:
chromium-reviews, Avi (use Gerrit) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFunctions to return locations of various Chrome bundles.
TEST=None, code is currently not called.
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117293
Patch Set 1 #
Total comments: 30
Patch Set 2 : Fix review comments #
Total comments: 4
Patch Set 3 : Remove DCHECKs #
Messages
Total messages: 11 (0 generated)
Less polished than I'd like since I'm sheriff today, but I hope this is good enough shape to get your feedback. Just accessors for now, once these are checked in I'll start modifying calls.
http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h File base/mac/bundle_locations.h (right): http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:29: // - "Outer Bundle" - This is the main bundle for Chrome, it's what semicolon after 'Chrome' rather than comma http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:33: // usually this should be the same as the outer bundle except when Chrome "Usually this should" is pleonastic. Try This will be the same as the outer bundle except when Chrome is launched via an app shortcut, in which case... http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:37: // - "Framework Bundle" - Bundle corresponding to the Chrome framework. First two items in the list were "This is the bundle..."; do the same here. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:41: // - If the choice is between the Outer or Launch bundles then please chose s/chose/choose/ http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:48: BASE_EXPORT FilePath LaunchBundleBundlePath(); Why two 'Bundle's here but one in OuterBundlePath? http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:51: BASE_EXPORT NSBundle* FrameworkBundle(); FrameworkBundlePath isn't needed? http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:53: // Set the bundle that MainAppBundle will return, overriding the default value There is no MainAppBundle nor SetOverrideAppBundle. Also, just make this two sentences. Perhaps Set the bundles that the preceding functions will return, overriding the default values. Restore the default by passing in |nil|. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm File base/mac/bundle_locations.mm (right): http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:10: // No threading worries since NSBundle isn't thread safe. Why do I find this sentence funny? :) Is it worth checking the calling thread in DCHECKs? Do scoped_nsobjects have evil static initializers? Can we use them here?
http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h File base/mac/bundle_locations.h (right): http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:57: BASE_EXPORT void SetOverrideOuterBundle(NSBundle* bundle); This is replacing base/mac/foundation_util base::mac::SetOverrideAppBundle, correct? base/mac/foundation_util also has base::mac::SetOverrideAppBundlePath, which accepts a FilePath argument. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:58: Maybe we should just have // In the .h: enum WhichBundle { BUNDLE_OUTER_APP, BUNDLE_LAUNCHED_APP, BUNDLE_FRAMEWORK, }; NSBundle* GetBundle(WhichBundle); FilePath GetBundlePath(WhichBundle); void SetBundleOverride(WhichBundle, NSBundle*); void SetBundleOverridePath(WhichBundle, const FilePath&); // In the .mm: namespace { NSBundle* g_override_bundles[BUNDLE_FRAMEWORK + 1]; } // namespace except that outside of base::mac, the calls would look like base::mac::GetBundlePath(base::mac::BUNDLE_LAUNCHED_APP), which may be a little verbose. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:62: #endif // #ifndef BASE_MAC_BUNDLE_LOCATIONS_H_ No #ifndef in the comment. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm File base/mac/bundle_locations.mm (right): http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:10: // No threading worries since NSBundle isn't thread safe. Avi wrote: > Do scoped_nsobjects have evil static initializers? Can we use them here? Nope, they’ve still got destructors. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:15: NSBundle* LaunchBundle() { This name sounds like it’s going to launch a bundle. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:21: FilePath LaunchBundleBundlePath() { This name sounds like you’re stuttering. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:37: NSBundle* FrameworkBundle() { If we have convenience wrappers for the other two calls (launch-bundle and outer-bundle) to get their FilePaths, why isn’t there one for this? Does nobody ever want the framework bundle’s FilePath?
* All review comments addressed. * Renamed LaunchBundle -> MainBundle per Mark's comments, open to other suggestions. * Removed setters for MainBundle since I don't think they'll be needed. If it turns out I'm wrong then I'll add them in a followup CL. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h File base/mac/bundle_locations.h (right): http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:29: // - "Outer Bundle" - This is the main bundle for Chrome, it's what On 2012/01/10 15:37:13, Avi wrote: > semicolon after 'Chrome' rather than comma Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:33: // usually this should be the same as the outer bundle except when Chrome On 2012/01/10 15:37:13, Avi wrote: > "Usually this should" is pleonastic. Try > > This will be the same as the outer bundle except when Chrome is launched via an > app shortcut, in which case... Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:37: // - "Framework Bundle" - Bundle corresponding to the Chrome framework. On 2012/01/10 15:37:13, Avi wrote: > First two items in the list were "This is the bundle..."; do the same here. Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:41: // - If the choice is between the Outer or Launch bundles then please chose On 2012/01/10 15:37:13, Avi wrote: > s/chose/choose/ Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:48: BASE_EXPORT FilePath LaunchBundleBundlePath(); On 2012/01/10 15:37:13, Avi wrote: > Why two 'Bundle's here but one in OuterBundlePath? Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:51: BASE_EXPORT NSBundle* FrameworkBundle(); On 2012/01/10 15:37:13, Avi wrote: > FrameworkBundlePath isn't needed? Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:53: // Set the bundle that MainAppBundle will return, overriding the default value On 2012/01/10 15:37:13, Avi wrote: > There is no MainAppBundle nor SetOverrideAppBundle. Also, just make this two > sentences. Perhaps > > Set the bundles that the preceding functions will return, overriding the default > values. Restore the default by passing in |nil|. Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:57: BASE_EXPORT void SetOverrideOuterBundle(NSBundle* bundle); On 2012/01/10 22:09:35, Mark Mentovai wrote: > This is replacing base/mac/foundation_util base::mac::SetOverrideAppBundle, > correct? > > base/mac/foundation_util also has base::mac::SetOverrideAppBundlePath, which > accepts a FilePath argument. Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:58: Personally I prefer the method in this patch to the verbosity of an enum. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.h#new... base/mac/bundle_locations.h:62: #endif // #ifndef BASE_MAC_BUNDLE_LOCATIONS_H_ On 2012/01/10 22:09:35, Mark Mentovai wrote: > No #ifndef in the comment. Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm File base/mac/bundle_locations.mm (right): http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:10: // No threading worries since NSBundle isn't thread safe. This comment is copied from foundation_utils.h , I've added DCHECKs in all of the relevant functions - does that seem ok to you? http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:15: NSBundle* LaunchBundle() { Yes it does, changing to MainBundle() - other suggestions welcome :) http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:21: FilePath LaunchBundleBundlePath() { On 2012/01/10 22:09:35, Mark Mentovai wrote: > This name sounds like you’re stuttering. Done. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:37: NSBundle* FrameworkBundle() { On 2012/01/10 22:09:35, Mark Mentovai wrote: > If we have convenience wrappers for the other two calls (launch-bundle and > outer-bundle) to get their FilePaths, why isn’t there one for this? Does nobody > ever want the framework bundle’s FilePath? Done.
This lgtm. http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm File base/mac/bundle_locations.mm (right): http://codereview.chromium.org/9147031/diff/1/base/mac/bundle_locations.mm#ne... base/mac/bundle_locations.mm:10: // No threading worries since NSBundle isn't thread safe. On 2012/01/11 07:12:44, jeremy wrote: > This comment is copied from foundation_utils.h , I've added DCHECKs in all of > the relevant functions - does that seem ok to you? Oh, that's fine. It struck me as absurd that there are no threading concerns because something is thread un-safe. I know what you mean, but still...
Mark: I need an owner lgtm, thanks!
http://codereview.chromium.org/9147031/diff/7001/base/mac/bundle_locations.mm File base/mac/bundle_locations.mm (right): http://codereview.chromium.org/9147031/diff/7001/base/mac/bundle_locations.mm... base/mac/bundle_locations.mm:19: DCHECK([NSThread isMainThread]); I don’t know if these DCHECKs are a good idea. http://codereview.chromium.org/9147031/diff/7001/base/mac/bundle_locations.mm... base/mac/bundle_locations.mm:24: DCHECK([NSThread isMainThread]); Don’t do this here, MainBundle does it anyway. Same in the other *Path functions. http://codereview.chromium.org/9147031/diff/7001/base/mac/bundle_locations.mm... base/mac/bundle_locations.mm:56: NSBundle* override_bundle) { I don’t think this does what you think it does. How does this ever write into g_override_framework_bundle or g_override_outer_bundle without accepting an NSBundle** parameter? http://codereview.chromium.org/9147031/diff/7001/base/mac/bundle_locations.mm... base/mac/bundle_locations.mm:72: DCHECK([NSThread isMainThread]); Again, not sure about these DCHCEKs, but if you are to DCHECK, you should probably just have one inside AssignOverrideBundle.
* Removed DCHECKs * Fixed indescribably moronic pointer bug :o/
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremy@chromium.org/9147031/11002
Change committed as 117293 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
