[mac] App shims

[mac] App shims

The AppShimHostController uses an IPC::ChannelFactory to listen on a socket for
app shims connecting. App shims send a launch message when they start, so we
translate that into actually launching the app.

I changed chrome_main_app_mode_mac.mm to use the IPC channel instead of sending
an Apple Event as before, and added an event loop and an IO thread.

BUG=138733
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188675

[Mark Mentovai, 2013-03-08 16:50:17]

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:44: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:46:
AppShimController::AppShimController() {
There’s enough stuff in this constructor that could fail that you’re probably
better off either

1. Having a minimal constructor and putting all of the real work into a
properly-named initialization method.

or

2. Since you seem to just want to die on error (as in OnChannelError), handle
the error cases right here in the constructor by dying.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:81: @interface ReplyEventHandler :
NSObject {
Just because you’re defining this class in an .mm file instead of an .h file
doesn’t relieve you of the duty to provide proper documentation for methods and
member variables when they wouldn’t be obvious.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:85: + (void)pingProcess:(const
ProcessSerialNumber&)psn
For example, the memory management of ReplyEventHandler objects is a little
unconventional. You should say something about it.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:99: ReplyEventHandler* handler =
[[ReplyEventHandler alloc]
This would be easier to read if you broke the line after the =. It still takes
two lines that way, but it’s a more natural place to break it.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:107: if (self = [super init]) {
We always write
  if ((self = [super init]))
to avoid the compiler warning about it looking like you wanted == instead of =.
I’m surprised that this warning wasn’t triggered here.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:112: - (void)pingProcess:(const
ProcessSerialNumber&)psn {
Blank line between methods implementations, please.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:123: length:sizeof(ProcessSerialNumber)];
Use sizeof(variable) and not sizeof(type) whenever possible.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=sizeof...

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:134: AESendMessage(
Considering how much logging you did in the previous changelist, I’m surprised
to see things like this go unchecked. If you decide you want to log something
that returns an OSStatus or OSErr, you can use OSSTATUS_LOG from
"base/mac/mac_logging.h". PLOG : errno :: OSSTATUS_LOG : OSStatus.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:148: void Begin() {
This is unnamespaced and non-static. Putting something called “Begin” into the
global namespace is a bad idea.

This one should be in an anonymous namespace or should be marked static. (And it
should have a better name even if you do that.)

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:188: PLOG(ERROR) <<
"base::mac::FSRefFromPath failed for "
PLOG is wrong here, because base::mac::FSRefFromPath doesn’t have anything to do
with errno.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:221:
base::PlatformThread::SetName("CrAppShimMain");
I like that you’ve stuck with the appropriate naming convention.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.h:13: // singleton
You’re gonna need more docs than this.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.h:25: void
InitOnFile();
InitOnFile’s a funny name because (1) there is no file argument, the
implementation figures it out itself, and there’s no other way to initialize it,
and (2) it’s not really a file, it’s a UNIX-domain socket.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:34: class
AppShimHost : public IPC::Listener,
Some short comments about everything in this class, too. Again, the memory
management here is a little tricky, and you should say something about it.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:70: channel_ = NULL;
It’s not really necessary to zero things out in a destructor.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:95: // Only one app
launch message per channel.
{braces} around multi-line scopes, even if the scope takes multiple lines for a
single statement.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:121:
extension_misc::LAUNCH_NONE,
Weird indentation.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:181: void
CreateAppShimHost(const IPC::ChannelHandle& handle) {
Anonymous namespace or static.

https://codereview.chromium.org/12623005/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/12623005/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:2194:
'browser/web_applications/app_shim_host_controller.h',
Sort!

[jeremya, 2013-03-11 02:47:33]

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:44: };
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:46:
AppShimController::AppShimController() {
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> There’s enough stuff in this constructor that could fail that you’re probably
> better off either
> 
> 1. Having a minimal constructor and putting all of the real work into a
> properly-named initialization method.
> 
> or
> 
> 2. Since you seem to just want to die on error (as in OnChannelError), handle
> the error cases right here in the constructor by dying.

All the stuff that could fail here fails on a separate thread, so I can't die
straight away. I've split it out into an Init() method though.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:85: + (void)pingProcess:(const
ProcessSerialNumber&)psn
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> For example, the memory management of ReplyEventHandler objects is a little
> unconventional. You should say something about it.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:99: ReplyEventHandler* handler =
[[ReplyEventHandler alloc]
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> This would be easier to read if you broke the line after the =. It still takes
> two lines that way, but it’s a more natural place to break it.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:107: if (self = [super init]) {
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> We always write
>   if ((self = [super init]))
> to avoid the compiler warning about it looking like you wanted == instead of
=.
> I’m surprised that this warning wasn’t triggered here.

Done, I'm surprised too.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:112: - (void)pingProcess:(const
ProcessSerialNumber&)psn {
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> Blank line between methods implementations, please.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:123: length:sizeof(ProcessSerialNumber)];
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> Use sizeof(variable) and not sizeof(type) whenever possible.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=sizeof...

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:134: AESendMessage(
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> Considering how much logging you did in the previous changelist, I’m surprised
> to see things like this go unchecked. If you decide you want to log something
> that returns an OSStatus or OSErr, you can use OSSTATUS_LOG from
> "base/mac/mac_logging.h". PLOG : errno :: OSSTATUS_LOG : OSStatus.

Done, plus the thing dies if AESendMessage fails.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:148: void Begin() {
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> This is unnamespaced and non-static. Putting something called “Begin” into the
> global namespace is a bad idea.
> 
> This one should be in an anonymous namespace or should be marked static. (And
it
> should have a better name even if you do that.)

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:188: PLOG(ERROR) <<
"base::mac::FSRefFromPath failed for "
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> PLOG is wrong here, because base::mac::FSRefFromPath doesn’t have anything to
do
> with errno.

-> LOG

https://codereview.chromium.org/12623005/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:221:
base::PlatformThread::SetName("CrAppShimMain");
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> I like that you’ve stuck with the appropriate naming convention.

I did? Awesome!

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.h:13: // singleton
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> You’re gonna need more docs than this.

I've always liked concise documentation ;)

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.h:25: void
InitOnFile();
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> InitOnFile’s a funny name because (1) there is no file argument, the
> implementation figures it out itself, and there’s no other way to initialize
it,
> and (2) it’s not really a file, it’s a UNIX-domain socket.

InitOnFileThread make more sense? :)

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:34: class
AppShimHost : public IPC::Listener,
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> Some short comments about everything in this class, too. Again, the memory
> management here is a little tricky, and you should say something about it.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:70: channel_ = NULL;
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> It’s not really necessary to zero things out in a destructor.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:95: // Only one app
launch message per channel.
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> {braces} around multi-line scopes, even if the scope takes multiple lines for
a
> single statement.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:121:
extension_misc::LAUNCH_NONE,
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> Weird indentation.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.mm:181: void
CreateAppShimHost(const IPC::ChannelHandle& handle) {
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> Anonymous namespace or static.

Done.

https://codereview.chromium.org/12623005/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/12623005/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:2194:
'browser/web_applications/app_shim_host_controller.h',
On 2013/03/08 16:50:17, Mark Mentovai wrote:
> Sort!

Done.

[jeremy, 2013-03-11 06:49:59]

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:30: base::Thread* g_io_thread = NULL;
Wrap these in an anon namespace?

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:32: class AppShimController : public
IPC::Listener {
Class level comment + method comments.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:39: virtual bool OnMessageReceived(const
IPC::Message& message);
Add an
// IPC::Listener implementation.

comment

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:43: void
OnDidActivateApplicationNotification(NSNotification* notification);
Where is this implemented?

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:50:
AppShimController::AppShimController() {
channel_(NULL)

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:55:
PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
What do you do if this fails?

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:60: this,
g_io_thread->message_loop_proxy());
DCHECK(g_io_thread) somewhere ?

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:70: 
remove NL

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:78: [NSApp terminate:nil];
LOG(ERROR) ?

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:88: @interface ReplyEventHandler :
NSObject {
Can you add a header comment explaining why we need this class?

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:100: -
(id)initWithCallback:(base::Callback<void(bool)>)replyFn;
Add high level comments for methods.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:111: [[ReplyEventHandler alloc]
initWithCallback:replyFn];
Add a comment on where this is released.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:171: void OnPingChromeReply(bool success)
{
Function comment.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:304: scoped_ptr<AppShimHostController>
app_shim_host_controller_;
Comment.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.h:14: // socket
(|factory_|) and creates a helper object to manage the connection.
What kind of object is the helper object?
Also comments for methods below.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.h:23: virtual void
OnClientConnected(const IPC::ChannelHandle& handle) OVERRIDE;
// IPC::ChannelFactory::Delegate implementation. Also bellow.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:46: bool
OnMessageReceived(const IPC::Message& message);
* virtual
* OVERRIDE
* Inherited from comment.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:89: 
Remove NL?

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:134:
chrome::AppLaunchParams params(profile_, extension,
nit: one arg per line.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:191:
PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
What if this fails?

https://codereview.chromium.org/12623005/diff/8001/chrome/common/app_shim_mes...
File chrome/common/app_shim_messages.h (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/common/app_shim_mes...
chrome/common/app_shim_messages.h:11: #define IPC_MESSAGE_START AppShimMsgStart
Since we're dealing with new IPC messages can you double check that you're
validating all parameters defensively when receiving these messages?  Also,
might want to get someone from the security team to double-check this part of
the code review.

https://codereview.chromium.org/12623005/diff/8001/chrome/common/app_shim_mes...
chrome/common/app_shim_messages.h:13:
IPC_MESSAGE_CONTROL1(AppShimMsg_LaunchApp_Done,
Comments on what each message does.

[jeremya, 2013-03-11 08:42:26]

Also I've removed the WeakPtr stuff I had in AppShimHostController for now,
since I was getting some "called on the wrong thread" exceptions from the
WeakPtr destructors inside PostTask. I think the base::Unretained is unsafe
though, so I'm going to find a better way to ensure that everything stays alive
long enough to receive all the PostTasks.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:30: base::Thread* g_io_thread = NULL;
On 2013/03/11 06:49:59, jeremy wrote:
> Wrap these in an anon namespace?

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:32: class AppShimController : public
IPC::Listener {
On 2013/03/11 06:49:59, jeremy wrote:
> Class level comment + method comments.

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:39: virtual bool OnMessageReceived(const
IPC::Message& message);
On 2013/03/11 06:49:59, jeremy wrote:
> Add an
> // IPC::Listener implementation.
> 
> comment

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:43: void
OnDidActivateApplicationNotification(NSNotification* notification);
On 2013/03/11 06:49:59, jeremy wrote:
> Where is this implemented?

Ghost of a future patch. Removed.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:50:
AppShimController::AppShimController() {
On 2013/03/11 06:49:59, jeremy wrote:
> channel_(NULL)

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:55:
PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
On 2013/03/11 06:49:59, jeremy wrote:
> What do you do if this fails?

Let's just die. Didn't realise it could fail.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:60: this,
g_io_thread->message_loop_proxy());
On 2013/03/11 06:49:59, jeremy wrote:
> DCHECK(g_io_thread) somewhere ?

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:70: 
On 2013/03/11 06:49:59, jeremy wrote:
> remove NL

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:78: [NSApp terminate:nil];
On 2013/03/11 06:49:59, jeremy wrote:
> LOG(ERROR) ?

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:88: @interface ReplyEventHandler :
NSObject {
On 2013/03/11 06:49:59, jeremy wrote:
> Can you add a header comment explaining why we need this class?

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:100: -
(id)initWithCallback:(base::Callback<void(bool)>)replyFn;
On 2013/03/11 06:49:59, jeremy wrote:
> Add high level comments for methods.

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:111: [[ReplyEventHandler alloc]
initWithCallback:replyFn];
On 2013/03/11 06:49:59, jeremy wrote:
> Add a comment on where this is released.

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:171: void OnPingChromeReply(bool success)
{
On 2013/03/11 06:49:59, jeremy wrote:
> Function comment.

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:304: scoped_ptr<AppShimHostController>
app_shim_host_controller_;
On 2013/03/11 06:49:59, jeremy wrote:
> Comment.

Done, though I'm not sure how much info I should add here that's already in the
class comment in the AppShimHostController header.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.h:14: // socket
(|factory_|) and creates a helper object to manage the connection.
On 2013/03/11 06:49:59, jeremy wrote:
> What kind of object is the helper object?
> Also comments for methods below.

It's an AppShimHost, but that class is local to the .cc file... should I mention
it by name here anyway?

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.h:23: virtual void
OnClientConnected(const IPC::ChannelHandle& handle) OVERRIDE;
On 2013/03/11 06:49:59, jeremy wrote:
> // IPC::ChannelFactory::Delegate implementation. Also bellow.

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:46: bool
OnMessageReceived(const IPC::Message& message);
On 2013/03/11 06:49:59, jeremy wrote:
> * virtual
> * OVERRIDE
> * Inherited from comment.

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:89: 
On 2013/03/11 06:49:59, jeremy wrote:
> Remove NL?

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:134:
chrome::AppLaunchParams params(profile_, extension,
On 2013/03/11 06:49:59, jeremy wrote:
> nit: one arg per line.

Done.

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.mm:191:
PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
On 2013/03/11 06:49:59, jeremy wrote:
> What if this fails?

Then the controller can't run... not sure if we should retry after some amount
of time, but I've made it so it just fails to init (and the browser process will
clean up the object when it quits).

https://codereview.chromium.org/12623005/diff/8001/chrome/common/app_shim_mes...
File chrome/common/app_shim_messages.h (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/common/app_shim_mes...
chrome/common/app_shim_messages.h:11: #define IPC_MESSAGE_START AppShimMsgStart
On 2013/03/11 06:49:59, jeremy wrote:
> Since we're dealing with new IPC messages can you double check that you're
> validating all parameters defensively when receiving these messages?  Also,
> might want to get someone from the security team to double-check this part of
> the code review.

Good idea. I think palmer@ is a security fellow?

I'm not entirely sure how to validate the profile name...

https://codereview.chromium.org/12623005/diff/8001/chrome/common/app_shim_mes...
chrome/common/app_shim_messages.h:13:
IPC_MESSAGE_CONTROL1(AppShimMsg_LaunchApp_Done,
On 2013/03/11 06:49:59, jeremy wrote:
> Comments on what each message does.

Done.

[jeremy, 2013-03-11 09:32:04]

Is there a test that covers this somewhere?

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:47: virtual bool OnMessageReceived(const
IPC::Message& message);
OVERRIDE ?

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:116: // which will be destroyed once the
reply event has been received.
nit: will be destroyed -> will delete itself

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:128: // aevt/ansr message, then sends off
an Apple Event using AESendMessage.
How about:
// This function sends an apple event to Chrome and waits for a reply, as
documented elsewhere this provides an indication that Chrome has opened an IPC
channel we can connect to.

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:131: // This will be called when the
aevt/ansr message arrives.
how about:
// Called when Apple Event is returned from Chrome in response to the ping we
sent.

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:143: // This will be released when the
reply arrives, or possibly earlier if an
This will be releated -> The object will release itself

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:207: // has succeeded (or failed, if
|success| is false).
// Called when Chrome returns an Apple Event in response to our ping (or
failed...).

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.h:304: // Hosts the channel that App Shims
connect to on Mac.
nit: channel -> IPC channel

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.h:33: // the IO thread.
nit: How about adding something like "Called on the IO thread to listen for
messages?"

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
File chrome/browser/web_applications/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:111: if
(profile_dir.length() == 0) {
// profile_dir and app_id are received over IPC so we need to be extra defensive
here.

[Mark Mentovai, 2013-03-11 12:40:37]

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/app_shim_host_controller.h:25: void
InitOnFile();
jeremya wrote:
> On 2013/03/08 16:50:17, Mark Mentovai wrote:
> > InitOnFile’s a funny name because (1) there is no file argument, the
> > implementation figures it out itself, and there’s no other way to initialize
> it,
> > and (2) it’s not really a file, it’s a UNIX-domain socket.
> 
> InitOnFileThread make more sense? :)

Yeah, it does! See, I had no idea that “file” meant “file thread” instead of
“filesystem node.”

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/app_shim_host_controller.h:27: void
ListenOnIO();
Can you add the word “thread” to this one too?

[palmer, 2013-03-11 19:28:18]

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
File chrome/browser/web_applications/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:55: bool
LaunchAppImpl(const std::string& app_id, const std::string& profile_dir);
The app_id and profile[_dir] arguments are in reverse order compared to
OnLaunchApp. Seems slightly confusing to me.

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:67: 
NIT: Remove this blank line? Or is channel_ different enough from the other
members that it deserves its own stanza?

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:111: if
(profile_dir.length() == 0) {
This isn't enough validation. This could be an arbitrary pathname, which is bad.
I propose that we validate that it is a basename *only*, and provide the parent
ourselves. Call basename(3) or (some equivalent from base/ ?) on profile_dir, to
enforce that it really is a basename and not a tricky pathname. You should also
enforce some lexical sanity on basename(profile_dir), so that it isn't something
like "...      evil|silly____))))::\\\\ASDFASDF    ".

    profile_dir = basename(profile_dir);
    if (!LexicallySane(profile_dir)) {
        // fail
    }
    true_profile_dir = user_data_dir + "/" + profile_dir;

LexicallySane should be something really simple, like a regexp match on
/^\w{8,20}$/.

That way this is no longer an arbitrary pathname IPC, but a tightly-defined
basename IPC.

Obviously, below on line 121 you do my last line of pseudocode, so you just need
to put basename and LexicallySane above 121. There might already be some
definition of LexicallySane for profile directories somewhere? I don't know.

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:121: path =
profileManager->user_data_dir().Append(path);
Elsewhere, you use the expression PathService::Get(chrome::DIR_USER_DATA,
&user_data_dir). Is there a difference?

https://codereview.chromium.org/12623005/diff/16001/chrome/common/app_shim_me...
File chrome/common/app_shim_messages.h (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/common/app_shim_me...
chrome/common/app_shim_messages.h:21: std::string /* profile name */,
This is unsafe; see the .mm file for details.

[jeremya, 2013-03-12 03:45:35]

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:47: virtual bool OnMessageReceived(const
IPC::Message& message);
On 2013/03/11 09:32:04, jeremy wrote:
> OVERRIDE ?

Curious that the compiler isn't warning about that...

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:116: // which will be destroyed once the
reply event has been received.
On 2013/03/11 09:32:04, jeremy wrote:
> nit: will be destroyed -> will delete itself

Done.

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:128: // aevt/ansr message, then sends off
an Apple Event using AESendMessage.
On 2013/03/11 09:32:04, jeremy wrote:
> How about:
> // This function sends an apple event to Chrome and waits for a reply, as
> documented elsewhere this provides an indication that Chrome has opened an IPC
> channel we can connect to.

Technically this class isn't Chrome-specific -- it should work for any process.
But I've added a comment at the top level of this class to this effect.

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:131: // This will be called when the
aevt/ansr message arrives.
On 2013/03/11 09:32:04, jeremy wrote:
> how about:
> // Called when Apple Event is returned from Chrome in response to the ping we
> sent.

Tweaked slightly, but didn't mention Chrome (or use 'we', since some people seem
to be averse to pronouns in comments.)

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:143: // This will be released when the
reply arrives, or possibly earlier if an
On 2013/03/11 09:32:04, jeremy wrote:
> This will be releated -> The object will release itself

Done.

https://codereview.chromium.org/12623005/diff/16001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:207: // has succeeded (or failed, if
|success| is false).
On 2013/03/11 09:32:04, jeremy wrote:
> // Called when Chrome returns an Apple Event in response to our ping (or
> failed...).

Done.

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.h:304: // Hosts the channel that App Shims
connect to on Mac.
On 2013/03/11 09:32:04, jeremy wrote:
> nit: channel -> IPC channel

-> IPC channel factory

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
File chrome/browser/web_applications/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.h:33: // the IO thread.
On 2013/03/11 09:32:04, jeremy wrote:
> nit: How about adding something like "Called on the IO thread to listen for
> messages?"

Done.

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
File chrome/browser/web_applications/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:55: bool
LaunchAppImpl(const std::string& app_id, const std::string& profile_dir);
On 2013/03/11 19:28:18, Chris P. wrote:
> The app_id and profile[_dir] arguments are in reverse order compared to
> OnLaunchApp. Seems slightly confusing to me.

Haha, oops. The names were only wrong in the declaration.

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:67: 
On 2013/03/11 19:28:18, Chris P. wrote:
> NIT: Remove this blank line? Or is channel_ different enough from the other
> members that it deserves its own stanza?

Done.

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:111: if
(profile_dir.length() == 0) {
On 2013/03/11 19:28:18, Chris P. wrote:
> This isn't enough validation. This could be an arbitrary pathname, which is
bad.
> I propose that we validate that it is a basename *only*, and provide the
parent
> ourselves. Call basename(3) or (some equivalent from base/ ?) on profile_dir,
to
> enforce that it really is a basename and not a tricky pathname. You should
also
> enforce some lexical sanity on basename(profile_dir), so that it isn't
something
> like "...      evil|silly____))))::\\\\ASDFASDF    ".
> 
>     profile_dir = basename(profile_dir);
>     if (!LexicallySane(profile_dir)) {
>         // fail
>     }
>     true_profile_dir = user_data_dir + "/" + profile_dir;
> 
> LexicallySane should be something really simple, like a regexp match on
> /^\w{8,20}$/.
> 
> That way this is no longer an arbitrary pathname IPC, but a tightly-defined
> basename IPC.
> 
> Obviously, below on line 121 you do my last line of pseudocode, so you just
need
> to put basename and LexicallySane above 121. There might already be some
> definition of LexicallySane for profile directories somewhere? I don't know.

I see, I was hoping that ProfileManager::GetProfile would do that sanitisation,
but it doesn't seem to so far as I can tell.

- it also shouldn't be '..' or '.'
- ... how does anything ever manage to call this safely?

https://codereview.chromium.org/12623005/diff/16001/chrome/browser/web_applic...
chrome/browser/web_applications/app_shim_host_controller.mm:121: path =
profileManager->user_data_dir().Append(path);
On 2013/03/11 19:28:18, Chris P. wrote:
> Elsewhere, you use the expression PathService::Get(chrome::DIR_USER_DATA,
> &user_data_dir). Is there a difference?

Unless PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) can vary within a
single session of Chrome, no (see BrowserProcessImpl::CreateProfileManager()).

https://codereview.chromium.org/12623005/diff/16001/chrome/common/app_shim_me...
File chrome/common/app_shim_messages.h (right):

https://codereview.chromium.org/12623005/diff/16001/chrome/common/app_shim_me...
chrome/common/app_shim_messages.h:21: std::string /* profile name */,
On 2013/03/11 19:28:18, Chris P. wrote:
> This is unsafe; see the .mm file for details.

I wonder if there's something other than the profile name that we could put here
that would be inherently more safe?

[jeremya, 2013-03-12 04:39:27]

+thakis for chrome/browser/app_controller_mac.mm and
chrome/app/chrome_main_app_mode_mac.mm and general chrome/ OWNERS :)

[jeremya, 2013-03-12 04:40:16]

Also: made a change to use ProfileInfoCache to verify the path name that comes
off the IPC channel. PTAL :)

[jeremy, 2013-03-12 12:12:31]

LGTM with minor comments.

https://codereview.chromium.org/12623005/diff/36001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/36001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:48: virtual void OnChannelError();
Also OVERRIDE ?

https://codereview.chromium.org/12623005/diff/36001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:133: - (void)pingProcess:(const
ProcessSerialNumber&)psn;
how about:
// Sends an Apple Event ping to the Chrome process and registers to listen for a
reply.

https://codereview.chromium.org/12623005/diff/36001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:135: // Called when the aevt/ansr message
arrives in response to the ping sent.
How about:
Called when a response is received from Chrome for the ping sent by
pingProcess:.

https://codereview.chromium.org/12623005/diff/36001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:139: // Calls the reply function, passing
it |success| to specify whether the ping
nit: the reply function -> |onReply_|

[Mark Mentovai, 2013-03-12 20:30:40]

LGTM too.

https://codereview.chromium.org/12623005/diff/36001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/36001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:14: #include "base/threading/thread.h"
Sort.

[jeremya, 2013-03-12 23:51:44]

Comments addressed; though I left the ReplyEventHandler comments generic rather
than referring to the Chrome process specifically.

[jeremya, 2013-03-14 07:40:06]

Added a test and refactored. PTAL :)

[jeremya, 2013-03-14 08:32:49]

+koz for apps/ review.

[Nico, 2013-03-14 15:18:58]

lgtm!

[palmer, 2013-03-14 19:05:04]

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host.cc (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:56: Send(new AppShimMsg_LaunchApp_Done(success));
Please pardon my ignorance: How is this new object's lifetime managed?

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:87: base::FilePath path(profile_dir);
Can we therefore have |pfile_dir| originate as a FilePath? Hmm, I suppose since
we are doing our own IPC mechanism we'd have to serialize and de-serialize it
ourselves.. so yeah perhaps doing it as a string is best?

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:90: if (cache.GetIndexOfProfileWithPath(path) ==
std::string::npos) {
So this line is the true security check? Is it worth adding a comment saying so?

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:94: Profile* profile =
profileManager->GetProfile(path);
Is this kind of the same thing, effectively, as the GetIndexOfProfileWithPath
check? Or perhaps this is a better check — we should check that the client named
a profile that (a) exists as a pathname naming a directory on the filesystem,
and (b) that the directory is owned by the right user, and (c) that it really is
a Chrome profile and not /etc or something.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host.h (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.h:41: const std::string& app_id() { return app_id_;
}
The methods themselves, as well as their return values, could be const?

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.h:57: // done so the profile and app id are stored,
and all future messages from
NIT: I would say "|profile|" and "|app_id|".

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.h:64: // When the app closes, a signal is sent (by
way of closing the channel) to
Not sure what this means.

1. Is the signal sent by the Observe method? The passive voice ("is sent")
obscures the actor of the sending.

2. "by way of closing the channel" — does that mean that closing the channel
*is* the signal?

https://codereview.chromium.org/12623005/diff/59001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/59001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:78: channel_->Send(new
AppShimHostMsg_LaunchApp(
Similar question: Is this new object's lifetime managed? I supposed it gets
collected when the process terminates (in Quit, below)?

[jeremya, 2013-03-14 23:33:00]

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host.cc (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:56: Send(new AppShimMsg_LaunchApp_Done(success));
On 2013/03/14 19:05:04, Chris P. wrote:
> Please pardon my ignorance: How is this new object's lifetime managed?

From ipc/ipc_sender.h:
  // Sends the given IPC message.  The implementor takes ownership of the
  // given Message regardless of whether or not this method succeeds.  This
  // is done to make this method easier to use.  Returns true on success and
  // false otherwise.
  virtual bool Send(Message* msg) = 0;

i.e. channel_->Send() deletes it.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:87: base::FilePath path(profile_dir);
On 2013/03/14 19:05:04, Chris P. wrote:
> Can we therefore have |pfile_dir| originate as a FilePath? Hmm, I suppose
since
> we are doing our own IPC mechanism we'd have to serialize and de-serialize it
> ourselves.. so yeah perhaps doing it as a string is best?

Not sure if base::FilePaths can be serialized over IPC... but I'm also not sure
what advantage doing that would have over passing a string.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:90: if (cache.GetIndexOfProfileWithPath(path) ==
std::string::npos) {
On 2013/03/14 19:05:04, Chris P. wrote:
> So this line is the true security check? Is it worth adding a comment saying
so?

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:94: Profile* profile =
profileManager->GetProfile(path);
On 2013/03/14 19:05:04, Chris P. wrote:
> Is this kind of the same thing, effectively, as the GetIndexOfProfileWithPath
> check? Or perhaps this is a better check — we should check that the client
named
> a profile that (a) exists as a pathname naming a directory on the filesystem,
> and (b) that the directory is owned by the right user, and (c) that it really
is
> a Chrome profile and not /etc or something.

The ProfileInfoCache stores info about the profiles that Chrome knows about, and
I'm pretty sure it can only be updated by either editing the local prefs
manually (in which case we're hosed security-wise anyway) or by the normal ways
of adding a profile (i.e. from the settings page). So it already ensures all
those things.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host.h (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.h:41: const std::string& app_id() { return app_id_;
}
On 2013/03/14 19:05:04, Chris P. wrote:
> The methods themselves, as well as their return values, could be const?

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.h:57: // done so the profile and app id are stored,
and all future messages from
On 2013/03/14 19:05:04, Chris P. wrote:
> NIT: I would say "|profile|" and "|app_id|".

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.h:64: // When the app closes, a signal is sent (by
way of closing the channel) to
On 2013/03/14 19:05:04, Chris P. wrote:
> Not sure what this means.
> 
> 1. Is the signal sent by the Observe method? The passive voice ("is sent")
> obscures the actor of the sending.
> 
> 2. "by way of closing the channel" — does that mean that closing the channel
> *is* the signal?

Made a better comment.

https://codereview.chromium.org/12623005/diff/59001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12623005/diff/59001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:78: channel_->Send(new
AppShimHostMsg_LaunchApp(
On 2013/03/14 19:05:04, Chris P. wrote:
> Similar question: Is this new object's lifetime managed? I supposed it gets
> collected when the process terminates (in Quit, below)?

As above, Send() deletes it when it's done with it.

[koz (OOO until 15th September), 2013-03-14 23:51:05]

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host.cc (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:28: delete channel_;
scoped_ptr<> to hold channel_?

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:33: channel_ = new IPC::ChannelProxy(handle,
IPC::Channel::MODE_SERVER, this,
Is this method intended to be called multiple times? DCHECK(!channel_)
otherwise.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host.h (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.h:27: // connected to the app shim is closed.
Perhaps add something explaining that this services IPCs from AppShimController.
(Site note, AppShimController <-> AppShimHost seems asymmetrical, don't we
normally name things X <-> XHost?)

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.h:15: class AppShimHostController
As far as meaningless noun suffixes go, I think Manager would be more
appropriate here. XController to me says that it controls a single X, whereas
XManager means it manages many instances of X.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:23: host->ServeChannel(handle);
nit: make this one line?

Also, add a comment saying that AppShimHost deletes itself.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:28:
AppShimHostController::AppShimHostController() : factory_(NULL) {
DCHECK() to communicate what thread this is supposed to happen on.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:36: delete factory_;
scoped_ptr for factory_.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:63: // called on IO thread
DCHECK() that we're on the IO thread rather than this comment, or amend the
comment to say that we *may* be called on the IO thread.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_unittest.cc (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:54: bool
TestingAppShimHost::SendFakeMessage(IPC::Message* message) {
I think this would be better named as ReceiveFakeMessage(). Or even just
ReceiveMessage().

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:99: TestingAppShimHost* host_;
scoped_ptrs means you don't need a TearDown().

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:111: ASSERT_EQ(true,
LaunchWasSuccessful());
nit: ASSERT_TRUE

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:118: ASSERT_EQ(0u,
host()->app_id().length());
nit: host()->app_id().empty()

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:119: ASSERT_EQ(false,
LaunchWasSuccessful());
nit: ASSERT_FALSE

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:126: ASSERT_EQ(0u,
host()->app_id().length());
s/length/empty()

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:127: ASSERT_EQ(false,
LaunchWasSuccessful());
nit: ASSERT_FALSE

[palmer, 2013-03-15 01:41:27]

LGTM

[jeremya, 2013-03-15 02:24:14]

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host.cc (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:28: delete channel_;
On 2013/03/14 23:51:05, koz wrote:
> scoped_ptr<> to hold channel_?

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host.cc:33: channel_ = new IPC::ChannelProxy(handle,
IPC::Channel::MODE_SERVER, this,
On 2013/03/14 23:51:05, koz wrote:
> Is this method intended to be called multiple times? DCHECK(!channel_)
> otherwise.

Nope. Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_controller.h (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.h:15: class AppShimHostController
On 2013/03/14 23:51:05, koz wrote:
> As far as meaningless noun suffixes go, I think Manager would be more
> appropriate here. XController to me says that it controls a single X, whereas
> XManager means it manages many instances of X.

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_controller.mm (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:23: host->ServeChannel(handle);
On 2013/03/14 23:51:05, koz wrote:
> nit: make this one line?
> 
> Also, add a comment saying that AppShimHost deletes itself.

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:28:
AppShimHostController::AppShimHostController() : factory_(NULL) {
On 2013/03/14 23:51:05, koz wrote:
> DCHECK() to communicate what thread this is supposed to happen on.

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:36: delete factory_;
On 2013/03/14 23:51:05, koz wrote:
> scoped_ptr for factory_.

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_controller.mm:63: // called on IO thread
On 2013/03/14 23:51:05, koz wrote:
> DCHECK() that we're on the IO thread rather than this comment, or amend the
> comment to say that we *may* be called on the IO thread.

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_unittest.cc (right):

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:54: bool
TestingAppShimHost::SendFakeMessage(IPC::Message* message) {
On 2013/03/14 23:51:05, koz wrote:
> I think this would be better named as ReceiveFakeMessage(). Or even just
> ReceiveMessage().

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:99: TestingAppShimHost* host_;
On 2013/03/14 23:51:05, koz wrote:
> scoped_ptrs means you don't need a TearDown().

Like magic!

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:111: ASSERT_EQ(true,
LaunchWasSuccessful());
On 2013/03/14 23:51:05, koz wrote:
> nit: ASSERT_TRUE

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:118: ASSERT_EQ(0u,
host()->app_id().length());
On 2013/03/14 23:51:05, koz wrote:
> nit: host()->app_id().empty()

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:119: ASSERT_EQ(false,
LaunchWasSuccessful());
On 2013/03/14 23:51:05, koz wrote:
> nit: ASSERT_FALSE

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:126: ASSERT_EQ(0u,
host()->app_id().length());
On 2013/03/14 23:51:05, koz wrote:
> s/length/empty()

Done.

https://codereview.chromium.org/12623005/diff/59001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_unittest.cc:127: ASSERT_EQ(false,
LaunchWasSuccessful());
On 2013/03/14 23:51:05, koz wrote:
> nit: ASSERT_FALSE

Done.

[koz (OOO until 15th September), 2013-03-15 03:59:04]

lgtm

[commit-bot: I haz the power, 2013-03-18 02:19:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/12623005/78001

[commit-bot: I haz the power, 2013-03-18 05:54:52]

Change committed as 188675