Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9374009:
Install platform apps into a separate data directory (Closed)
|
Created:
8 years, 10 months ago by sail Modified:
8 years, 10 months ago CC:
chromium-reviews, mihaip+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd extension path field to Mac platform apps
This CL adds a new extension path field to Mac platform apps. This path is used to load the extension for Mac platform apps.
BUG=112651
TEST=Installed a platform app. Verified that it could run side by side with Chromium.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123443
Patch Set 1 #Patch Set 2 : fix comment #
Total comments: 10
Patch Set 3 : rebase, address review comments #
Total comments: 9
Patch Set 4 : address review comments #Patch Set 5 : fix test #
Total comments: 12
Patch Set 6 : remove duplicate extension #
Total comments: 4
Patch Set 7 : address review comments #
Total comments: 2
Patch Set 8 : address review comments #
Total comments: 4
Patch Set 9 : address review comments #
Total comments: 5
Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : address review comments #
Messages
Total messages: 36 (0 generated)
Can you add unit tests. Also, can you add whoever wrote the extension install code as reviewer if they're not already reviewing this CL? https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/web_a... File chrome/browser/web_applications/web_app_mac.h (right): https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/web_a... chrome/browser/web_applications/web_app_mac.h:30: FilePath web_app_path_; Please comment this member.
Is the diffbase for the CL old? https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:164: return false; Leave a TODO here. https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/exten... File chrome/browser/extensions/extension_service.h (right): https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/exten... chrome/browser/extensions/extension_service.h:711: // Sets up preferences for a newly installed extension. Document the page_ordinal and initial_enable params. https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/web_a... File chrome/browser/web_applications/web_app_mac.mm (right): https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/browser/web_a... chrome/browser/web_applications/web_app_mac.mm:106: forKey:app_mode::kCrAppModeUserDataDirKey]; nit: align colons https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/common/extens... File chrome/common/extensions/extension_file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/2001/chrome/common/extens... chrome/common/extensions/extension_file_util_unittest.cc:45: true); /*should_delete=*/true); and on line 60
Are there any design documents about what the long-term plan is here? This seems to violate many deep assumptions ... I'm just wondering what the path forward is.
On 2012/02/09 22:00:57, Aaron Boodman wrote: > Are there any design documents about what the long-term plan is here? This seems > to violate many deep assumptions ... I'm just wondering what the path forward > is. Hi Aaron, this is just a proof of concept so far. Sharing a profile would be a lot of work on the Mac. We're experimenting with using separate profiles. If there are any specific things you want me to look into please let me know.
On 2012/02/09 09:27:00, jeremy wrote: > Can you add unit tests. Since this is behind a flag would it be ok to add a unit test after this has landed? Since there are a few unlanded changes things are a bit broken right now. > Also, can you add whoever wrote the extension install code as reviewer if > they're not already reviewing this CL? Sure. Adding benwells, jstritar, aa
Rebased off tip of tree + latest patches. Please take another look. Also added more extension folks. http://codereview.chromium.org/9374009/diff/2001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/9374009/diff/2001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.cc:164: return false; On 2012/02/09 18:43:59, rsesek wrote: > Leave a TODO here. I'm not sure what the plans are for other platforms. I added a comment that currently we only plan to support this on Mac. http://codereview.chromium.org/9374009/diff/2001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_service.h (right): http://codereview.chromium.org/9374009/diff/2001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.h:711: // Sets up preferences for a newly installed extension. On 2012/02/09 18:43:59, rsesek wrote: > Document the page_ordinal and initial_enable params. Done. http://codereview.chromium.org/9374009/diff/2001/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.h (right): http://codereview.chromium.org/9374009/diff/2001/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.h:30: FilePath web_app_path_; On 2012/02/09 09:27:00, jeremy wrote: > Please comment this member. Done. http://codereview.chromium.org/9374009/diff/2001/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.mm (right): http://codereview.chromium.org/9374009/diff/2001/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:106: forKey:app_mode::kCrAppModeUserDataDirKey]; On 2012/02/09 18:43:59, rsesek wrote: > nit: align colons Done. http://codereview.chromium.org/9374009/diff/2001/chrome/common/extensions/ext... File chrome/common/extensions/extension_file_util_unittest.cc (right): http://codereview.chromium.org/9374009/diff/2001/chrome/common/extensions/ext... chrome/common/extensions/extension_file_util_unittest.cc:45: true); On 2012/02/09 18:43:59, rsesek wrote: > /*should_delete=*/true); and on line 60 Done.
Possible alternate implementations of this that involve fewer changes in current extension code: Option 1 1. Change CrxInstaller to take install_directory as a construtor parameter (instead of reading it from ExtensionService). 2. In AppShortcutManager, when getting NOTIFICATION_EXTENSION_INSTALLED 3. Kick off the another CrxInstaller that installs the extension into the app's profile directory (I'm not sure what you can use as the source file. Is the .crx. still around? Perhaps if you listen to a lower level notification like NOTIFICATION_CRX_INSTALLER_DONE) Option 2 Use external extensions ( http://code.google.com/chrome/extensions/trunk/external_extensions.html) in the app's profile directory, pointed at a copy of the .crx that's been copied in its user data directory. That way, when the app binary gets launched next, it'll install the app in its profile (see http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/e... how that works for default apps, since the use case is similar). Mihai On Thu, Feb 9, 2012 at 2:03 PM, <sail@chromium.org> wrote: > On 2012/02/09 22:00:57, Aaron Boodman wrote: > >> Are there any design documents about what the long-term plan is here? This >> > seems > >> to violate many deep assumptions ... I'm just wondering what the path >> forward >> is. >> > > Hi Aaron, this is just a proof of concept so far. Sharing a profile would > be a > lot of work on the Mac. We're experimenting with using separate profiles. > If > there are any specific things you want me to look into please let me know. > > https://chromiumcodereview.**appspot.com/9374009/<https://chromiumcodereview.... >
https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/crx_installer.cc:523: /*should_delete=*/true); In Chrome code, I usually see the format: true); // delete source dir Even better, use a named constant or define an enum in extension_file_util to make this more self-documenting. https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (left): https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:2235: // overridden, don't reset the value. Seems like these could also be moved into ExtensionPrefs::OnExtensionInstalled? https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:158: bool ExtensionService::PlatformAppRequiresSeparateDataDirectory() { I think this would make more sense as a member of Extension: bool Extension::RequiesSeparateUserDataDirectory() const { #if defined(OS_MACOSX) return is_platform_app(); #else return false; } https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:2672: FilePath data_dir = web_app::GetWebAppDataDirectory( These few lines of code shows up in a couple places. Refactor into Extension::GetPlatformAppUserDataDirectory()?
https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/crx_installer.cc:523: /*should_delete=*/true); On 2012/02/10 20:09:46, Aaron Boodman wrote: > In Chrome code, I usually see the format: > > true); // delete source dir > > Even better, use a named constant or define an enum in extension_file_util to > make this more self-documenting. We do /*var=*/ a lot: http://code.google.com/codesearch#search/&exact_package=chromium&q=%3D%5C*%2F... +1 to an enum, though.
https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/crx_installer.cc:523: /*should_delete=*/true); On 2012/02/10 20:16:02, rsesek wrote: > On 2012/02/10 20:09:46, Aaron Boodman wrote: > > In Chrome code, I usually see the format: > > > > true); // delete source dir > > > > Even better, use a named constant or define an enum in extension_file_util to > > make this more self-documenting. > > We do /*var=*/ a lot: > http://code.google.com/codesearch#search/&exact_package=chromium&q=%253D%255C... > > +1 to an enum, though. Done. Switched to enum. https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (left): https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:2235: // overridden, don't reset the value. On 2012/02/10 20:09:46, Aaron Boodman wrote: > Seems like these could also be moved into ExtensionPrefs::OnExtensionInstalled? Done. https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:158: bool ExtensionService::PlatformAppRequiresSeparateDataDirectory() { On 2012/02/10 20:09:46, Aaron Boodman wrote: > I think this would make more sense as a member of Extension: > > bool Extension::RequiesSeparateUserDataDirectory() const { > #if defined(OS_MACOSX) > return is_platform_app(); > #else > return false; > } Done. https://chromiumcodereview.appspot.com/9374009/diff/2003/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:2672: FilePath data_dir = web_app::GetWebAppDataDirectory( On 2012/02/10 20:09:46, Aaron Boodman wrote: > These few lines of code shows up in a couple places. Refactor into > Extension::GetPlatformAppUserDataDirectory()? This would involve moving several functions from web_app.cc to extension.cc. It would aslo require a profile path to be passed in since the extension doesn't have a profile reference. To simplify things I added a new GetWebAppDataDirectory() overload that takes a profile and an extension. I also cleaned up the existing code. Currently there are 3 functions with very similar names: GetWebAppDataDirectory GetWebAppDir GetDataDir I consolidated the last two into the first.
On 2012/02/10 00:14:41, Mihai Parparita wrote: > Possible alternate implementations of this that involve fewer changes in > current extension code: Hi Mihai. Would you mind I did this in a separate CL. Currently it's not clear if we want to have a duplicate copy of the extension or not. Once we try out this feature on Canary we can make a decision and move this code to a better place.
Ping!
On Fri, Feb 10, 2012 at 2:08 AM, <sail@chromium.org> wrote: > On 2012/02/09 09:27:00, jeremy wrote: > >> Can you add unit tests. >> > > Since this is behind a flag would it be ok to add a unit test after this > has > landed? Since there are a few unlanded changes things are a bit broken > right > now. I'd prefer that this land with tests, as long as they pass for you locally we can just land them as FAILS_ for now and enable them when the dependent CLs land...
https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:494: bool separate_data_dir_required = Could you add a comment explaining when this case kicks in. It should be clear to someone reading this function, under what conditions the flag will be true. https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:518: extension_file_util::InstallExtension( What if this call fails? https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... File chrome/browser/extensions/extension_service.h (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/extension_service.h:698: // directory. |page_ordinal| is order the extension should show up in the NTP. nit: *is -> is the https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/web_... File chrome/browser/web_applications/web_app_mac.h (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/web_... chrome/browser/web_applications/web_app_mac.h:19: // The shortcut store its data directory in |web_app_path|. nit: store -> stores https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/common/exten... File chrome/common/extensions/extension.h (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/common/exten... chrome/common/extensions/extension.h:602: // directory. Can you expand the comment as to when this is the case. https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/common/exten... File chrome/common/extensions/extension_file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/common/exten... chrome/common/extensions/extension_file_util_unittest.cc:42: src, IMHO this should be indented 4 from the start of the function call column e.g. in relation to extension_file_util , also below.
Review comments are dependent on my understanding what this cl is for, exactly. My understanding is this cl will: - when a platform app is being installed at a file level, also install it into a new data dir - after extensions are installed and their preferences are being set up, also set them up in the new data dir - once that is done you can manually run the platform app by starting a new instance of chrome pointing at the new data dir If that isn't correct let me know. https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... File chrome/browser/extensions/crx_installer.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:143: profile_path_(profile_->GetPath()), Do you need this new field? Can you just call profile_->GetPath() in the one place it is used? https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:512: if (separate_data_dir_required) { What about unpacked extensions? Can this move into extension_file_util::InstallExtension? https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/crx_installer.cc:535: version_dir, Should this reload the extension from the new data directory or the original extension? I think at the moment we are just creating a copy of the extension in another folder, so it can be run manually from there. In which case the call to InstallExtension above can fail and not affect the flow of this function. If that's the case, maybe a comment would help. https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... File chrome/browser/extensions/extension_service.h (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/extension_service.h:698: // directory. |page_ordinal| is order the extension should show up in the NTP. Some info about what happens to the preferences in the original data directory would be nice. I.e. is the final result two copies of the extension preferences? https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/extension_service.h:698: // directory. |page_ordinal| is order the extension should show up in the NTP. Can this be done elsewhere? Moving it into AppShortcutManager to happen on the NOTIFICATION_EXTENSION_INSTALLED should be pretty trivial, if it works.
> Hi Mihai. Would you mind I did this in a separate CL. Currently it's not clear > if we want to have a duplicate copy of the extension or not. Once we try out > this feature on Canary we can make a decision and move this code to a better > place. I agree that it'd be good to validate the user-facing behavior of this change. However, we also need to validate how such an approach could be implemented. IMO, the code as it currently stands is spread-out enough and brittle enough that I'm not comfortable with it landing with the current design. Of the two that I suggested, I'm leaning towards the external extension one. I also thought of a variant of it. Aaron, please let know if you think it's gross: Use external extensions, but instead of specifying via the external extension file a .crx file that gets installed or an update URL, specify a path where the extension can be loaded as unpacked (the LOAD Extension::Location enum value). For that path, use the path that the extension gets installed into in the regular profile. That way, the extension doesn't have to get installed into two locations that we have to keep in sync. https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9374009/diff/12002/chrome/browser/exte... chrome/browser/extensions/extension_service.cc:2655: extension_prefs.AddGrantedPermissions(id, extension->GetActivePermissions()); Is this a (partial) duplication of what CrxInstaller::ReportSuccessFromUIThread and ExtensionService::AddExtension do? It seems like it'll end up being quite brittle.
I like the idea of having the extension files only installed in the parent profile, and somehow having the child Chrome instance load them from there. One issue I am not sure of: Traditionally, we have had the invariant that only the single Chrome browser process touches the files in the user data directory. Is it OK to have the 'parent' browser process modifying the parent user data directory and the 'child' browser process modifying the child user data directory concurrently? Assuming that is OK... it seems like there are a number of approaches to telling the child process to load the extension in question. Probably the easiest is a new command line flag. We have already pretty nicely factored ExtensionService so that you can just randomly call AddExtension with any Extension object. There's no reason we couldn't add code to create an Extension instance pointing at a directory in the parent profile.
On 2012/02/14 00:41:03, Aaron Boodman wrote: > Traditionally, we have had the invariant that only the single Chrome browser > process touches the files in the user data directory. Is it OK to have the > 'parent' browser process modifying the parent user data directory and the > 'child' browser process modifying the child user data directory concurrently? Would the child process be touching the data at all? In general, once an extension is unpacked, we don't do anything to its directory, right? The only problem I see is the parent touching the files for auto-update. > Assuming that is OK... it seems like there are a number of approaches to telling > the child process to load the extension in question. Probably the easiest is a > new command line flag. We have already pretty nicely factored ExtensionService > so that you can just randomly call AddExtension with any Extension object. > There's no reason we couldn't add code to create an Extension instance pointing > at a directory in the parent profile. Is this just a matter of using --load-extension (http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profiles/pro...) in the stub binary (i.e. passing it in ChromeAppModeStart)? Mihai
On Mon, Feb 13, 2012 at 4:46 PM, <mihaip@chromium.org> wrote: > On 2012/02/14 00:41:03, Aaron Boodman wrote: >> >> Traditionally, we have had the invariant that only the single Chrome >> browser >> process touches the files in the user data directory. Is it OK to have the >> 'parent' browser process modifying the parent user data directory and the >> 'child' browser process modifying the child user data directory >> concurrently? > > > Would the child process be touching the data at all? In general, once an > extension is unpacked, we don't do anything to its directory, right? The > only > problem I see is the parent touching the files for auto-update. We use versioned directories, so even autoupdate should be fine. I can't think of problem with this approach. >> Assuming that is OK... it seems like there are a number of approaches to > > telling >> >> the child process to load the extension in question. Probably the easiest >> is a >> new command line flag. We have already pretty nicely factored >> ExtensionService >> so that you can just randomly call AddExtension with any Extension object. >> There's no reason we couldn't add code to create an Extension instance > > pointing >> >> at a directory in the parent profile. > > > Is this just a matter of using --load-extension > (http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profiles/pro...) > in the stub binary (i.e. passing it in ChromeAppModeStart)? I was a little concerned about using --load-extension because I believe that the LOAD location still implies some special behavior that you would not want. For example, I think that LOAD extensions are loaded synchronously at startup off disk. Maybe that has changed now that we remember unpacked extensions in the preferences...? I'm not sure. But you'd want to look through the references to location() carefully to make sure there aren't others. Creating a new isolated code path for this use case that just does ExtensionService::AddExtension() seems easier/better in some ways. In general 'LOAD' means 'developer mode'. So it's a bit of an abuse of the path to use it for apps this way, and I could see confusion creeping back in over time where people assume that location() == LOAD means developer mode. Seems better to avoid that.
This CL is much simpler now. I made the following changes: - non-extension related plumbing for the --user-data-dir field has been moved to a separate CL (http://codereview.chromium.org/9423048/) - deleted all the code that made a duplicate copy of the extension in the platform app's user data directory - added plumbing for a new "extension path" field so that the platform app can find the extension For now I'm using --load-extension to load the extension from the extension path. I'll change this to a new switch in a separate CL.
> I was a little concerned about using --load-extension because I > believe that the LOAD location still implies some special behavior > that you would not want. Hi Aron, it sounds like I need to introduce a new switch to specify that the platform app's extension should be loaded from a directory. Do you mind if I do this in a separate change?
Nice, much simpler indeed. http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... File chrome/app/chrome_main_app_mode_mac.mm (left): http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... chrome/app/chrome_main_app_mode_mac.mm:51: // TODO(viettrungluu): do something intelligent with data So we're blessing this hack? It seems like we'd want to use the constant names for the flags, and possibly CommandLine::Init, AppendSwitch, etc. http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... File chrome/app/chrome_main_app_mode_mac.mm (right): http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... chrome/app/chrome_main_app_mode_mac.mm:55: std::string argv3("--load-extension=" + info->extension_path.value()); Add a TODO about switching to a different flag that doesn't imply Location::LOAD for the extension
http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... File chrome/app/chrome_main_app_mode_mac.mm (left): http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... chrome/app/chrome_main_app_mode_mac.mm:51: // TODO(viettrungluu): do something intelligent with data On 2012/02/21 20:41:02, Mihai Parparita wrote: > So we're blessing this hack? It seems like we'd want to use the constant names > for the flags, and possibly CommandLine::Init, AppendSwitch, etc. Done. http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... File chrome/app/chrome_main_app_mode_mac.mm (right): http://codereview.chromium.org/9374009/diff/19001/chrome/app/chrome_main_app_... chrome/app/chrome_main_app_mode_mac.mm:55: std::string argv3("--load-extension=" + info->extension_path.value()); On 2012/02/21 20:41:02, Mihai Parparita wrote: > Add a TODO about switching to a different flag that doesn't imply Location::LOAD > for the extension Done.
LGTM http://codereview.chromium.org/9374009/diff/24001/chrome/common/mac/app_mode_... File chrome/common/mac/app_mode_common.h (right): http://codereview.chromium.org/9374009/diff/24001/chrome/common/mac/app_mode_... chrome/common/mac/app_mode_common.h:36: extern NSString * const kCrAppModeExtensionPathKey; Nit: no space before *.
http://codereview.chromium.org/9374009/diff/24001/chrome/common/mac/app_mode_... File chrome/common/mac/app_mode_common.h (right): http://codereview.chromium.org/9374009/diff/24001/chrome/common/mac/app_mode_... chrome/common/mac/app_mode_common.h:36: extern NSString * const kCrAppModeExtensionPathKey; On 2012/02/23 00:25:19, Mihai Parparita wrote: > Nit: no space before *. Done.
http://codereview.chromium.org/9374009/diff/29001/chrome/app/chrome_main_app_... File chrome/app/chrome_main_app_mode_mac.mm (right): http://codereview.chromium.org/9374009/diff/29001/chrome/app/chrome_main_app_... chrome/app/chrome_main_app_mode_mac.mm:65: argv[i] = const_cast<char*>(command_line.argv()[i].c_str()); Why do you need this const cast? Can't the array be of const char? http://codereview.chromium.org/9374009/diff/29001/chrome/browser/web_applicat... File chrome/browser/web_applications/web_app_mac.mm (right): http://codereview.chromium.org/9374009/diff/29001/chrome/browser/web_applicat... chrome/browser/web_applications/web_app_mac.mm:179: forKey:app_mode::kCrAppModeExtensionPathKey]; nit: align colons
http://codereview.chromium.org/9374009/diff/29001/chrome/app/chrome_main_app_... File chrome/app/chrome_main_app_mode_mac.mm (right): http://codereview.chromium.org/9374009/diff/29001/chrome/app/chrome_main_app_... chrome/app/chrome_main_app_mode_mac.mm:65: argv[i] = const_cast<char*>(command_line.argv()[i].c_str()); On 2012/02/23 01:23:47, rsesek wrote: > Why do you need this const cast? Can't the array be of const char? Currently all ChromeMain and ContentMain accept (int argc, char** argv). It would probably be a good idea to extend it to take CommandLine directly but I think that's should be a separate change. http://codereview.chromium.org/9374009/diff/29001/chrome/browser/web_applicat... File chrome/browser/web_applications/web_app_mac.mm (right): http://codereview.chromium.org/9374009/diff/29001/chrome/browser/web_applicat... chrome/browser/web_applications/web_app_mac.mm:179: forKey:app_mode::kCrAppModeExtensionPathKey]; On 2012/02/23 01:23:47, rsesek wrote: > nit: align colons Done.
lgtm
http://codereview.chromium.org/9374009/diff/30003/chrome/app/app_mode_loader_... File chrome/app/app_mode_loader_mac.mm (right): http://codereview.chromium.org/9374009/diff/30003/chrome/app/app_mode_loader_... chrome/app/app_mode_loader_mac.mm:99: [info_plist objectForKey:app_mode::kCrAppModeExtensionPathKey]); What happens if the extension is missing? http://codereview.chromium.org/9374009/diff/30003/chrome/app/chrome_main_app_... File chrome/app/chrome_main_app_mode_mac.mm (right): http://codereview.chromium.org/9374009/diff/30003/chrome/app/chrome_main_app_... chrome/app/chrome_main_app_mode_mac.mm:54: command_line.AppendSwitch(info->argv[0]); We should really change this so the communication isn't happening via the command line. No need to do anything now, but I'll change it in my next cl...
http://codereview.chromium.org/9374009/diff/30003/chrome/app/app_mode_loader_... File chrome/app/app_mode_loader_mac.mm (right): http://codereview.chromium.org/9374009/diff/30003/chrome/app/app_mode_loader_... chrome/app/app_mode_loader_mac.mm:99: [info_plist objectForKey:app_mode::kCrAppModeExtensionPathKey]); On 2012/02/23 19:46:48, jeremy wrote: > What happens if the extension is missing? This will be handled by the browser code that loads extensions. Once this feature is stable we'll have to add a bunch of error UI. For example, what happens if the user data dir is locked, etc...
lgtm
lgtm for shortcut stuff, with nit. http://codereview.chromium.org/9374009/diff/30003/chrome/browser/shell_integr... File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/9374009/diff/30003/chrome/browser/shell_integr... chrome/browser/shell_integration.h:19: class FilePath; Nit: this forward decl can be removed now.
https://chromiumcodereview.appspot.com/9374009/diff/30003/chrome/browser/shel... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/9374009/diff/30003/chrome/browser/shel... chrome/browser/shell_integration.h:19: class FilePath; On 2012/02/24 00:10:42, benwells wrote: > Nit: this forward decl can be removed now. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/9374009/36014
Change committed as 123443 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
