Publish app shortcuts on Mac

Publish app shortcuts on Mac

With this CL when installing a platform app we do the following
  - copy the stub App Mode Loader.app bundle to a tmp folder with the correct app name
  - update the info plist
  - copy the final app to /Applications
  - if we don't have write permission to /Application then we copy the app to ~/Application instead

Note, for now I'm putting this feature behind a --enable-platform-apps flag.

BUG=112651
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121613

[sail, 2012-02-07 07:07:40]

Note, to properly test this change you need jeremy's CL that builds the App Mode
Launcher.app bundle. This can still be checked in independently since everything
is currently behind a flag.

Please take a look.

Thanks

[jeremy, 2012-02-07 08:29:27]

Can you add some unit tests for app shortcut creation?

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
File chrome/browser/web_applications/web_app.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
chrome/browser/web_applications/web_app.cc:86: std::string file_name =
UTF16ToUTF8(name);
Do we do this somewhere else in the code? Can you ask Avi to look this over to
see if there might be any corner cases around encoding that we're missing?

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.mm:22: stringByAppendingString:@"
App Mode Loader.app"];
This CL is based on top of the temporary CL I sent you, right? Because the code
in the tree currently doesn't generate a .app for the loader.

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.mm:26: NSString*
FilePathToNSString(const FilePath& file_path) {
Can this live somewhere in base/mac/ ? I've got an in-flight cl that uses this
as well...

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/common/mac/ap...
File chrome/common/mac/app_mode_common.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.mm:12: NSString * const
kCrAppModeShortcutIDKey = @"CrAppModeShortcutID";
Can you add comments for these.

[sail, 2012-02-07 18:17:05]

> Can you add some unit tests for app shortcut creation?
I'm working on unit tests right now. Mind if I do this in a separate CL?

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
File chrome/browser/web_applications/web_app.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
chrome/browser/web_applications/web_app.cc:86: std::string file_name =
UTF16ToUTF8(name);
On 2012/02/07 08:29:27, jeremy wrote:
> Do we do this somewhere else in the code? Can you ask Avi to look this over to
> see if there might be any corner cases around encoding that we're missing?

This is used in a few places for linux and cross platform code. I think it
should be ok to use this since there are very few restrictions on legal Mac file
names. Adding avi to look this over.

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.mm:22: stringByAppendingString:@"
App Mode Loader.app"];
On 2012/02/07 08:29:27, jeremy wrote:
> This CL is based on top of the temporary CL I sent you, right? Because the
code
> in the tree currently doesn't generate a .app for the loader.

Yea, currently this code will fail but it should be ok to checkin since
everything is behind a flag.

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.mm:26: NSString*
FilePathToNSString(const FilePath& file_path) {
On 2012/02/07 08:29:27, jeremy wrote:
> Can this live somewhere in base/mac/ ? I've got an in-flight cl that uses this
> as well...

Done.

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/common/mac/ap...
File chrome/common/mac/app_mode_common.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/1012/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.mm:12: NSString * const
kCrAppModeShortcutIDKey = @"CrAppModeShortcutID";
On 2012/02/07 08:29:27, jeremy wrote:
> Can you add comments for these.

Actually there are already comments in the header file.

[sail, 2012-02-07 22:58:56]

On 2012/02/07 18:17:05, sail wrote:
> > Can you add some unit tests for app shortcut creation?
> I'm working on unit tests right now. Mind if I do this in a separate CL?

Unit tests out for review:
https://chromiumcodereview.appspot.com/9362001/

[Robert Sesek, 2012-02-08 00:59:38]

I didn't review the windows part.

http://codereview.chromium.org/9346013/diff/2006/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/9346013/diff/2006/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:292: NSString* FilePathToNSString(const FilePath&
path);
Need comments. And tests: success, nil NSString, empty path, and empty str.

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:76: void CreateShortcutTask(const
FilePath& web_app_path,
Comment.

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:22: stringByAppendingString:@"
App Mode Loader.app"];
Localization?

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:23: return
base::mac::PathForFrameworkBundleResource((CFStringRef)app_loader);
base::mac::NSToCFCast

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:31: FilePath GetDstPath() {
What does daylight saving time has to do with this?

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:102:
selectFile:base::mac::FilePathToNSString(dst_path)
nit: just indent 4

http://codereview.chromium.org/9346013/diff/2006/chrome/common/mac/app_mode_c...
File chrome/common/mac/app_mode_common.h (right):

http://codereview.chromium.org/9346013/diff/2006/chrome/common/mac/app_mode_c...
chrome/common/mac/app_mode_common.h:25: extern NSString * const
kCrAppModeShortcutIDKey;
nit: no space before *

[sail, 2012-02-08 01:47:20]

http://codereview.chromium.org/9346013/diff/2006/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/9346013/diff/2006/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:292: NSString* FilePathToNSString(const FilePath&
path);
On 2012/02/08 00:59:38, rsesek wrote:
> Need comments. And tests: success, nil NSString, empty path, and empty str.

Done.

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:76: void CreateShortcutTask(const
FilePath& web_app_path,
On 2012/02/08 00:59:38, rsesek wrote:
> Comment.

Done.

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:22: stringByAppendingString:@"
App Mode Loader.app"];
On 2012/02/08 00:59:38, rsesek wrote:
> Localization?

This is the hardcoded app name. It's not visible to the user since we rename it
before copying it into their /Applications folder.

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:23: return
base::mac::PathForFrameworkBundleResource((CFStringRef)app_loader);
On 2012/02/08 00:59:38, rsesek wrote:
> base::mac::NSToCFCast

Done.

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:31: FilePath GetDstPath() {
On 2012/02/08 00:59:38, rsesek wrote:
> What does daylight saving time has to do with this?

Done.

http://codereview.chromium.org/9346013/diff/2006/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:102:
selectFile:base::mac::FilePathToNSString(dst_path)
On 2012/02/08 00:59:38, rsesek wrote:
> nit: just indent 4

Done.

http://codereview.chromium.org/9346013/diff/2006/chrome/common/mac/app_mode_c...
File chrome/common/mac/app_mode_common.h (right):

http://codereview.chromium.org/9346013/diff/2006/chrome/common/mac/app_mode_c...
chrome/common/mac/app_mode_common.h:25: extern NSString * const
kCrAppModeShortcutIDKey;
On 2012/02/08 00:59:38, rsesek wrote:
> nit: no space before *

Done.

[sail, 2012-02-08 01:48:03]

mark: base/* OWNERS review

[Mark Mentovai, 2012-02-08 04:29:04]

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
base/mac/foundation_util.h:292: // Converts |path| to a NSString.
State that the NSString will be autoreleased. State that the return value will
be nil if path is empty.

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
base/mac/foundation_util.mm:334:
stringWithFileSystemRepresentation:path.value().c_str()
This is overkill. +[NSString stringWithUTF8String:] is fine. The reason is that
the filesystem representation is always UTF-8. Since it’s a stricter subset of
UTF-8, you can’t get a filesystem representation with -[NSString UTF8String] and
need to use -[NSString fileSystemEncoding], but to go the other way,
+stringWithUTF8String: will accept any UTF-8 including the specific UTF-8 of the
filesystem encoding. It is cheaper and more direct.

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
base/mac/foundation_util.mm:339: if (!str || ![str length])
You can simplify this to (![str length]).

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
File base/mac/foundation_util_unittest.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
base/mac/foundation_util_unittest.mm:325: TEST(FoundationUtilTest,
FilePathToNSString) {
Thanks for providing tests!

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:7: #include
"chrome/browser/web_applications/web_app.h"
Shouldn’t this come first, before <Cocoa/Cocoa.h>?

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:9: #include <Cocoa/Cocoa.h>
This one is #imported, not #included.

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:18: extern const CFStringRef
kBrowserBundleIDKey;
Why do you mix CFStringRefs and…

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:25: extern NSString* const
kCrAppModeShortcutIDKey;
…NSStrings?

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
File chrome/common/mac/app_mode_common.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
chrome/common/mac/app_mode_common.mm:5: #include
"chrome/common/mac/app_mode_common.h"
If the .h is becoming an Objective-C header, this (and other #include sites)
should become an #import.

[sail, 2012-02-08 05:21:38]

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
base/mac/foundation_util.h:292: // Converts |path| to a NSString.
On 2012/02/08 04:29:04, Mark Mentovai wrote:
> State that the NSString will be autoreleased. State that the return value will
> be nil if path is empty.

Done.

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
base/mac/foundation_util.mm:334:
stringWithFileSystemRepresentation:path.value().c_str()
On 2012/02/08 04:29:04, Mark Mentovai wrote:
> This is overkill. +[NSString stringWithUTF8String:] is fine. The reason is
that
> the filesystem representation is always UTF-8. Since it’s a stricter subset of
> UTF-8, you can’t get a filesystem representation with -[NSString UTF8String]
and
> need to use -[NSString fileSystemEncoding], but to go the other way,
> +stringWithUTF8String: will accept any UTF-8 including the specific UTF-8 of
the
> filesystem encoding. It is cheaper and more direct.

Done.

https://chromiumcodereview.appspot.com/9346013/diff/11001/base/mac/foundation...
base/mac/foundation_util.mm:339: if (!str || ![str length])
On 2012/02/08 04:29:04, Mark Mentovai wrote:
> You can simplify this to (![str length]).

Done.

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:7: #include
"chrome/browser/web_applications/web_app.h"
On 2012/02/08 04:29:04, Mark Mentovai wrote:
> Shouldn’t this come first, before <Cocoa/Cocoa.h>?

Done.

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:9: #include <Cocoa/Cocoa.h>
On 2012/02/08 04:29:04, Mark Mentovai wrote:
> This one is #imported, not #included.

Done.

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:18: extern const CFStringRef
kBrowserBundleIDKey;
On 2012/02/08 04:29:04, Mark Mentovai wrote:
> Why do you mix CFStringRefs and…

Switched to NSString since that's what we use mostly anyways.

https://chromiumcodereview.appspot.com/9346013/diff/11001/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:25: extern NSString* const
kCrAppModeShortcutIDKey;
On 2012/02/08 04:29:04, Mark Mentovai wrote:
> …NSStrings?

Done.

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/app/app_mode...
File chrome/app/app_mode_loader_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/app/app_mode...
chrome/app/app_mode_loader_mac.mm:75:
(CFStringRef)app_mode::kLastRunAppBundlePathPrefsKey,
Note, these C style casts are fixed (or will be fixed) in Jeremy's CL.

[Mark Mentovai, 2012-02-08 05:53:26]

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/exte...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/exte...
chrome/browser/extensions/extension_service.cc:2639: // TODO(sail): For now only
install shortcuts for if enable platform apps
Revise grammar (get rid of “for?”)

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
File chrome/browser/web_applications/web_app_linux.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
chrome/browser/web_applications/web_app_linux.cc:27: return;  // assuming always
success.
No need to return as the last statement of a void-typed function. (And the
comment doesn’t make sense in a void-typed function, either.)

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:58: return [dict
writeToFile:plist_path atomically:NO];
atomically:NO is bad.

I don’t like rewriting Info.plist at all, actually. Can we stuff the things we
need into some other file that isn’t used for any purpose other than the our
own?

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:75: FilePath app_name =
GetSanitizedFileName(shortcut_info.title);
This would almost certainly be better if you built up the staging app in the
same directory where it’s supposed to wind up, but as a dot-file or something.
Then you can just move it into place instead of having to copy it.

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/common/mac/a...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:9: #import <Cocoa/Cocoa.h>
Just use <Foundation/Foundation.h> because all you need is NSString.

[Mark Mentovai, 2012-02-08 05:55:19]

LGTM if you’d like to check this in as-is with the minor things taken
care of, addressing the following in a follow-up:

> I don’t like rewriting Info.plist at all, actually. Can we stuff the
> things we need into some other file that isn’t used for any purpose
> other than the our own?

> This would almost certainly be better if you built up the staging app in
> the same directory where it’s supposed to wind up, but as a dot-file or
> something. Then you can just move it into place instead of having to
> copy it.

[sail, 2012-02-08 06:12:51]

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/exte...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/exte...
chrome/browser/extensions/extension_service.cc:2639: // TODO(sail): For now only
install shortcuts for if enable platform apps
On 2012/02/08 05:53:26, Mark Mentovai wrote:
> Revise grammar (get rid of “for?”)

Done.

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
File chrome/browser/web_applications/web_app_linux.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
chrome/browser/web_applications/web_app_linux.cc:27: return;  // assuming always
success.
On 2012/02/08 05:53:26, Mark Mentovai wrote:
> No need to return as the last statement of a void-typed function. (And the
> comment doesn’t make sense in a void-typed function, either.)

Done.

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:58: return [dict
writeToFile:plist_path atomically:NO];
On 2012/02/08 05:53:26, Mark Mentovai wrote:
> atomically:NO is bad.
> 
> I don’t like rewriting Info.plist at all, actually. Can we stuff the things we
> need into some other file that isn’t used for any purpose other than the our
> own?

Unfortunately I think there are some standard Info.plist entries that we'll have
to customize at publish time. For example:
  - bundle identifier
  - bundle display name
  - document types/handlers

https://chromiumcodereview.appspot.com/9346013/diff/12004/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:75: FilePath app_name =
GetSanitizedFileName(shortcut_info.title);
On 2012/02/08 05:53:26, Mark Mentovai wrote:
> This would almost certainly be better if you built up the staging app in the
> same directory where it’s supposed to wind up, but as a dot-file or something.
> Then you can just move it into place instead of having to copy it.

Ahh, I didn't realize I was doing a copy. I think I could do a move from the tmp
directory. There should be a function that lets you create a temp folder on the
same volume as the target folder. Looking...

[Mark Mentovai, 2012-02-08 06:27:25]

Don’t do a move straight from the temporary directory. It may be on a different
filesystem. That’s why I suggested staging in the same directory.

I think it’s fine to have to set some keys in Info.plist when you create the
shortcut initially, but the structure of the code made it look like Info.plist
would also be subject to being updated subsequent to shortcut creation. That’s
what I’d like to avoid.

[sail, 2012-02-08 06:30:39]

On 2012/02/08 06:27:25, Mark Mentovai wrote:
> Don’t do a move straight from the temporary directory. It may be on a
different
> filesystem. That’s why I suggested staging in the same directory.

So it looks like FSPathGetTemporaryDirectoryForReplaceObject() will do what you
want. I'll send a follow up CL to do this.

> I think it’s fine to have to set some keys in Info.plist when you create the
> shortcut initially, but the structure of the code made it look like Info.plist
> would also be subject to being updated subsequent to shortcut creation. That’s
> what I’d like to avoid.

Ahh, yea, currently this is only meant to run when publishing the shortcut.

[jeremy, 2012-02-08 09:10:45]

https://chromiumcodereview.appspot.com/9346013/diff/15005/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/base/mac/foundation...
base/mac/foundation_util.h:295: // Converts |str| to a FilePath.
Add to comment:
if |str| is nil then an empty file path is returned ?

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.h:76: // Does the actual job of creating
a shortcut (see CreateShortcut above).
nit: "CreateShortcut()"

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.h:82: // Returns sanitized name that
could be used as a file name
How about:
Sanitizes |name| and returns a version of it that is safe to use as an on-disk
file name .

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app_linux.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_linux.cc:11: 
nit: do we need this newline?

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:22: stringByAppendingString:@"
App Mode Loader.app"];
IMHO this should be indented 4 past [l10n_util , same for the
PathForFramework..() line below.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:27: bool IsWritable(const
FilePath& path) {
Should this live in base/file_util.h ?

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:32: FilePath GetDestinationPath()
{
Comment for this function.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:48:
app_path.Append("Contents").Append("Info.plist"));
nit: IMHO splitting this into 2 lines would make it a bit easier to read.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:68: namespace web_app {
no newline between nested namespaces.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/common/mac/a...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/common/mac/a...
chrome/common/mac/app_mode_common.h:27: // Key for the app's shortcut.
nit: *shortcut name.

[---DO-NOT-USE---rsesek1, 2012-02-08 12:24:33]

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/app/app_mode...
File chrome/app/app_mode_loader_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/app/app_mode...
chrome/app/app_mode_loader_mac.mm:74: NSString* cr_bundle_path =
[(NSString*)CFPreferencesCopyAppValue(
You could fix these up to use the NSToCFCast and CFToNSCast functions.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.cc:136: web_app::GetDataDir(data_dir),
nit: this should be indented 2 more

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.h:76: // Does the actual job of creating
a shortcut (see CreateShortcut above).
Does this have to only be called on the FILE thread?

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:22: stringByAppendingString:@"
App Mode Loader.app"];
On 2012/02/08 09:10:45, jeremy wrote:
> IMHO this should be indented 4 past [l10n_util , same for the
> PathForFramework..() line below.

This is correct as-is.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:48:
app_path.Append("Contents").Append("Info.plist"));
On 2012/02/08 09:10:45, jeremy wrote:
> nit: IMHO splitting this into 2 lines would make it a bit easier to read.

I think this is OK.

[sail, 2012-02-08 19:09:15]

https://chromiumcodereview.appspot.com/9346013/diff/15005/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/base/mac/foundation...
base/mac/foundation_util.h:295: // Converts |str| to a FilePath.
On 2012/02/08 09:10:45, jeremy wrote:
> Add to comment:
> if |str| is nil then an empty file path is returned ?

Done.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/app/app_mode...
File chrome/app/app_mode_loader_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/app/app_mode...
chrome/app/app_mode_loader_mac.mm:74: NSString* cr_bundle_path =
[(NSString*)CFPreferencesCopyAppValue(
On 2012/02/08 12:24:33, rsesek1 wrote:
> You could fix these up to use the NSToCFCast and CFToNSCast functions.

Currently this code doesn't use base/*. This is fixed in jeremy's CL.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.cc:136: web_app::GetDataDir(data_dir),
On 2012/02/08 12:24:33, rsesek1 wrote:
> nit: this should be indented 2 more

Done.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.h:76: // Does the actual job of creating
a shortcut (see CreateShortcut above).
On 2012/02/08 09:10:45, jeremy wrote:
> nit: "CreateShortcut()"

Done.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.h:76: // Does the actual job of creating
a shortcut (see CreateShortcut above).
On 2012/02/08 12:24:33, rsesek1 wrote:
> Does this have to only be called on the FILE thread?

Yea. Ahh I forgot to add the DCHECK() on the Mac version. fixed.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.h:76: // Does the actual job of creating
a shortcut (see CreateShortcut above).
On 2012/02/08 12:24:33, rsesek1 wrote:
> Does this have to only be called on the FILE thread?

Yea. Ahh I forgot to add the DCHECK() on the Mac version. fixed.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app.h:82: // Returns sanitized name that
could be used as a file name
On 2012/02/08 09:10:45, jeremy wrote:
> How about:
> Sanitizes |name| and returns a version of it that is safe to use as an on-disk
> file name .

Done.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app_linux.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_linux.cc:11: 
On 2012/02/08 09:10:45, jeremy wrote:
> nit: do we need this newline?

Done.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:27: bool IsWritable(const
FilePath& path) {
On 2012/02/08 09:10:45, jeremy wrote:
> Should this live in base/file_util.h ?

Removed, this was already in base.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:32: FilePath GetDestinationPath()
{
On 2012/02/08 09:10:45, jeremy wrote:
> Comment for this function.

Done.

https://chromiumcodereview.appspot.com/9346013/diff/15005/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:68: namespace web_app {
On 2012/02/08 09:10:45, jeremy wrote:
> no newline between nested namespaces.

Done.

[Mihai Parparita -not on Chrome, 2012-02-08 23:03:01]

LGTM

https://chromiumcodereview.appspot.com/9346013/diff/7011/chrome/browser/exten...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/7011/chrome/browser/exten...
chrome/browser/extensions/extension_service.cc:2636: void
ExtensionService::StartInstallApplicationShortcut(
This code is moving in http://codereview.chromium.org/9310079/ (currently in the
commit queue), so you may want to wait for that to land and put it in
AppShortcutManager.

https://chromiumcodereview.appspot.com/9346013/diff/7011/chrome/browser/exten...
chrome/browser/extensions/extension_service.cc:2641: if
(!extension->is_platform_app() ||
This is already checked before StartInstallApplicationShortcut is called.

[Robert Sesek, 2012-02-09 02:03:32]

lgtm

https://chromiumcodereview.appspot.com/9346013/diff/7011/chrome/browser/web_a...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9346013/diff/7011/chrome/browser/web_a...
chrome/browser/web_applications/web_app.h:83: // on-disk file name .
nit: extra space before .

[jeremy, 2012-02-09 08:47:01]

lgtm

[sail, 2012-02-09 22:23:52]

rebased and moved code to the new app_shortcut_manager class.
also fixed wstring presubmit errors by switching shell_integration to string16

https://chromiumcodereview.appspot.com/9346013/diff/7011/chrome/browser/exten...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9346013/diff/7011/chrome/browser/exten...
chrome/browser/extensions/extension_service.cc:2641: if
(!extension->is_platform_app() ||
On 2012/02/08 23:03:02, Mihai Parparita wrote:
> This is already checked before StartInstallApplicationShortcut is called.

Done.

[commit-bot: I haz the power, 2012-02-09 22:24:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9346013/16003

[commit-bot: I haz the power, 2012-02-09 22:24:32]

Can't apply patch for file chrome/browser/web_applications/web_app.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/web_applications/web_app.cc
Hunk #1 FAILED at 4.
Hunk #3 FAILED at 101.
2 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/web_applications/web_app.cc.rej

[commit-bot: I haz the power, 2012-02-10 08:11:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9346013/17005

[commit-bot: I haz the power, 2012-02-10 09:15:21]

Try job failure for 9346013-17005 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-10 23:50:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9346013/25001

[commit-bot: I haz the power, 2012-02-11 02:04:15]

Change committed as 121613