Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12663023:
[mac] Make app shims look in the correct user data dir on Canary. (Closed)
|
Created:
7 years, 9 months ago by jeremya Modified:
7 years, 8 months ago CC:
chromium-reviews, sail+watch_chromium.org, playmobil Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[mac] Make app shims look in the correct user data dir on Canary.
On Canary, the user data directory is affected by
[[NSBundle mainBundle] objectForInfoDictionaryKey:@"CrProductDirName"].
Unfortunately in app shims, [NSBundle mainBundle] refers to the app shim's
bundle, not Chrome's bundle. This patch parameterises the
user-data-directory-figuring-out code on the NSBundle, so that app shims can
pass an NSBundle for the real Chrome bundle.
R=thakis@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192644
Patch Set 1 #
Total comments: 16
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : comments #Patch Set 4 : fix compile #Patch Set 5 : fix compile again #
Messages
Total messages: 19 (0 generated)
CC jeremy who might be interested
Thanks sail@! I am interested, but it's the weekend here - will review properly on Sunday. On Fri, Mar 22, 2013 at 3:50 AM, <sail@chromium.org> wrote: > CC jeremy who might be interested > > https://codereview.chromium.**org/12663023/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... File chrome/app/chrome_main_app_mode_mac.mm (left): https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... chrome/app/chrome_main_app_mode_mac.mm:256: chrome::RegisterPathProvider(); As long as we’re positive that nothing else needs the path provider. Hey, does anyone need the AtExitManager while we’re in here? https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... File chrome/app/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... chrome/app/chrome_main_app_mode_mac.mm:75: if (!chrome::GetUserDataDirectoryForBundle(chrome_bundle, &user_data_dir)) { It’s more cumbersome, but …ForBrowserAppBundle would answer the “which bundle?” question. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_in... File chrome/common/chrome_paths_internal.h (right): https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_in... chrome/common/chrome_paths_internal.h:98: // therein. This returns a bool. Document what the bool means. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... File chrome/common/chrome_paths_mac.mm (right): https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:107: bool GetDefaultUserDataDirectoryForProduct(const std::string& product_dir, This belongs in the anonymous namespace a few lines up. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:231: char* product_dir_name = ProductDirNameInternal(bundle); I don’t like calling *Internal functions from something other than the associated function that wraps it, because it’s messy. *Internal functions just look like broken-off implementation details of the one function that they support, and calling it directly from elsewhere can be a confusing pattern. I can’t come up with a better way to restructure things, though. Notably, the strdup thing is a royal pain that makes it a little more cumbersome, but we can’t put a std::string into a static (or rely on anything about lifetimes of const char*s we didn’t make ourselves), so we had to do it. But maybe at least a rename of the “internal” function is in order now. ProductDirNameForBundle? https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:231: char* product_dir_name = ProductDirNameInternal(bundle); You can stick the result into a scoped_ptr_malloc which is a little more self-documenting and makes sure the right thing happens if this function grows to have early returns.
https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_in... File chrome/common/chrome_paths_internal.h (right): https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_in... chrome/common/chrome_paths_internal.h:98: // therein. Also, can you please document what |bundle| is supposed to be exactly (What +[NSBundle mainBundle] would return if Chrome were launched normally)? https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... File chrome/common/chrome_paths_mac.mm (right): https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:59: // Use OuterAppBundle() to get the main app's bundle. This key needs to live Can you please update this comment?
https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... File chrome/app/chrome_main_app_mode_mac.mm (left): https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... chrome/app/chrome_main_app_mode_mac.mm:256: chrome::RegisterPathProvider(); On 2013/03/22 18:08:41, Mark Mentovai wrote: > As long as we’re positive that nothing else needs the path provider. Hey, does > anyone need the AtExitManager while we’re in here? Actually we might still need it for DIR_APP_DATA https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... File chrome/app/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/12663023/diff/1/chrome/app/chrome_main_app_mo... chrome/app/chrome_main_app_mode_mac.mm:75: if (!chrome::GetUserDataDirectoryForBundle(chrome_bundle, &user_data_dir)) { On 2013/03/22 18:08:41, Mark Mentovai wrote: > It’s more cumbersome, but …ForBrowserAppBundle would answer the “which bundle?” > question. I went for "...ForBrowserBundle". I think that's sufficiently descriptive. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_in... File chrome/common/chrome_paths_internal.h (right): https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_in... chrome/common/chrome_paths_internal.h:98: // therein. On 2013/03/24 14:17:13, jeremy wrote: > Also, can you please document what |bundle| is supposed to be exactly (What > +[NSBundle mainBundle] would return if Chrome were launched normally)? Done. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_in... chrome/common/chrome_paths_internal.h:98: // therein. On 2013/03/22 18:08:41, Mark Mentovai wrote: > This returns a bool. Document what the bool means. Done. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... File chrome/common/chrome_paths_mac.mm (right): https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:59: // Use OuterAppBundle() to get the main app's bundle. This key needs to live On 2013/03/24 14:17:13, jeremy wrote: > Can you please update this comment? Moved it to where the call to OuterAppBundle went. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:107: bool GetDefaultUserDataDirectoryForProduct(const std::string& product_dir, On 2013/03/22 18:08:41, Mark Mentovai wrote: > This belongs in the anonymous namespace a few lines up. Done. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:231: char* product_dir_name = ProductDirNameInternal(bundle); On 2013/03/22 18:08:41, Mark Mentovai wrote: > I don’t like calling *Internal functions from something other than the > associated function that wraps it, because it’s messy. *Internal functions just > look like broken-off implementation details of the one function that they > support, and calling it directly from elsewhere can be a confusing pattern. > > I can’t come up with a better way to restructure things, though. Notably, the > strdup thing is a royal pain that makes it a little more cumbersome, but we > can’t put a std::string into a static (or rely on anything about lifetimes of > const char*s we didn’t make ourselves), so we had to do it. > > But maybe at least a rename of the “internal” function is in order now. > ProductDirNameForBundle? Done. https://codereview.chromium.org/12663023/diff/1/chrome/common/chrome_paths_ma... chrome/common/chrome_paths_mac.mm:231: char* product_dir_name = ProductDirNameInternal(bundle); On 2013/03/22 18:08:41, Mark Mentovai wrote: > You can stick the result into a scoped_ptr_malloc which is a little more > self-documenting and makes sure the right thing happens if this function grows > to have early returns. Done.
Ping :)
Sorry about that. LGTM with these minor changes. https://codereview.chromium.org/12663023/diff/9001/chrome/app/chrome_main_app... File chrome/app/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/12663023/diff/9001/chrome/app/chrome_main_app... chrome/app/chrome_main_app_mode_mac.mm:75: if (!chrome::GetUserDataDirectoryForBrowserBundle( This kind of thing reads a tiny bit better formatted as if (!chrome::GetUserDataDirectoryForBrowserBundle(chrome_bundle, &user_data_dir)) { with the arguments lined up, don’t you agree? https://codereview.chromium.org/12663023/diff/9001/chrome/common/chrome_paths... File chrome/common/chrome_paths_internal.h (right): https://codereview.chromium.org/12663023/diff/9001/chrome/common/chrome_paths... chrome/common/chrome_paths_internal.h:102: NSBundle* bundle, base::FilePath* result); Likewise. https://codereview.chromium.org/12663023/diff/9001/chrome/common/chrome_paths... File chrome/common/chrome_paths_mac.mm (right): https://codereview.chromium.org/12663023/diff/9001/chrome/common/chrome_paths... chrome/common/chrome_paths_mac.mm:62: #endif #else DCHECK(!chrome_bundle) to ensure that nobody does anything with the argument on iOS thinking that it has an effect? https://codereview.chromium.org/12663023/diff/9001/chrome/common/chrome_paths... chrome/common/chrome_paths_mac.mm:231: NSBundle* bundle, base::FilePath* result) { Same thing with the wrapping on this line.
lgtm stamp
All done. Thanks :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12663023/19001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12663023/19001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12663023/19001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12663023/46001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12663023/52001
Message was sent while issue was closed.
Change committed as 192644 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
