[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

This removes base::mac::FSRefFromPath usage from
OpenApplicationWithPath. |FSRefFromPath| is relying on deprecated API
and should be removed.

BUG=650854
Committed: https://crrev.com/7eb9c14b4f3c992d2d66f4b6f56e8a42671abd87
Cr-Commit-Position: refs/heads/master@{#423391}

[Eugene But (OOO till 7-30), 2016-10-04 04:17:36]

Description was changed from

==========
[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

BUG=650854
==========

to

==========
[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

This removes base::mac::FSRefFromPath usage from
OpenApplicationWithPath. |FSRefFromPath| is relying on deprecated API
and should be removed.

BUG=650854
==========

[Eugene But (OOO till 7-30), 2016-10-04 04:18:43]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 04:18:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eugene But (OOO till 7-30), 2016-10-04 04:57:34]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[Eugene But (OOO till 7-30), 2016-10-04 04:57:49]

eugenebut@chromium.org changed reviewers:
+ thakis@chromium.org

[commit-bot: I haz the power, 2016-10-04 04:57:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 05:50:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 05:50:31]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-04 05:54:08]

tapted@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2016-10-04 05:54:09]

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
File base/mac/launch_services_util.h (right):

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.h:14: struct ProcessSerialNumber;
remove?

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.h:26: BASE_EXPORT bool OpenApplicationWithPath(
Since the API is changing anyway... returning a base::Process might be a nicer
API

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
File base/mac/launch_services_util.mm (right):

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:19: if (!bundle_url) {
apart from OOM, can this fail? Perhaps just DCHECK(bundle_url) so we can trim
some string data from the binary.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:25: NSMutableArray* launch_args =
[NSMutableArray arrayWithCapacity:argc - 1];
NSWorkspaceLaunchConfigurationArguments says it's an NSArray of NSStrings passed
into the argv parameter, so should argv[0] be the command anyway? That would
allow for new-school for loops below too :). Otherwise we should comment here to
say why we need to remove it...

But LSApplicationParameters::argv seems to have the same documentation... so I'm
not sure how this ever worked :|

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:33: };
That // TODO(jeremya): this opens a new browser window if Chrome is already ...
probably still belongs here. 

And, not for this CL, but I think the TODO can actually be resolved by adding
`NSWorkspaceLaunchNewInstance` -- the code here was written before Chrome
supported process singletons, but we since added support for that, so Chrome now
*does* support launching multiple times, and we use IPC over a domain socket to
pass the args -- that would probably fix some bugs, since
NSWorkspaceLaunchConfigurationArguments is ignored when the app is already
running.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:42: *out_pid = app.processIdentifier;
[app processIdentifier]

But also launchApplicationAtURL will return nil on error - I guess this is still
the right thing to do, but it looks a bit weird.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:48: DLOG(ERROR) <<
base::SysNSStringToUTF8(error_description);
There's probably a good argument for making this a LOG rather than DLOG. But
also I'm not sure whether a localized string from userInfo has the best thing to
log. Does NSLog do something reasonable? Maybe that will give something with a
consistent format that we can look for on message boards/logs/bugs

[Matt Giuca, 2016-10-04 05:58:42]

mgiuca@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2016-10-04 05:58:43]

Not sure why I am CC'd... is there something you wanted me to look at?

[Eugene But (OOO till 7-30), 2016-10-04 06:59:41]

On 2016/10/04 05:58:43, Matt Giuca (OOO til Oct 4) wrote:
> Not sure why I am CC'd... is there something you wanted me to look at?
I don't know. Maybe some kind presubmit hook CCed you. I did not do that on
purpose.

[Eugene But (OOO till 7-30), 2016-10-04 07:02:49]

PTAL

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
File base/mac/launch_services_util.h (right):

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.h:14: struct ProcessSerialNumber;
On 2016/10/04 05:54:08, tapted wrote:
> remove?

Done.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.h:26: BASE_EXPORT bool OpenApplicationWithPath(
On 2016/10/04 05:54:08, tapted wrote:
> Since the API is changing anyway... returning a base::Process might be a nicer
> API

Done.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
File base/mac/launch_services_util.mm (right):

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:19: if (!bundle_url) {
On 2016/10/04 05:54:08, tapted wrote:
> apart from OOM, can this fail? Perhaps just DCHECK(bundle_url) so we can trim
> some string data from the binary. 
It can if bundle_path.value() is an empty string. I also missed early return
which was in the originally code. w/o early return launchApplicationAtURL: would
crash for nil URL, so I added it back.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:25: NSMutableArray* launch_args =
[NSMutableArray arrayWithCapacity:argc - 1];
On 2016/10/04 05:54:08, tapted wrote:
> NSWorkspaceLaunchConfigurationArguments says it's an NSArray of NSStrings
passed
> into the argv parameter, so should argv[0] be the command anyway? That would
> allow for new-school for loops below too :). Otherwise we should comment here
to
> say why we need to remove it...
> 
> But LSApplicationParameters::argv seems to have the same documentation... so
I'm
> not sure how this ever worked :|
argv[0] is a binary path, which automatically added by NSWorkspace (I checked).
Added comments to explain this.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:33: };
On 2016/10/04 05:54:08, tapted wrote:
> That // TODO(jeremya): this opens a new browser window if Chrome is already
...
> probably still belongs here. 
> 
> And, not for this CL, but I think the TODO can actually be resolved by adding
> `NSWorkspaceLaunchNewInstance` -- the code here was written before Chrome
> supported process singletons, but we since added support for that, so Chrome
now
> *does* support launching multiple times, and we use IPC over a domain socket
to
> pass the args -- that would probably fix some bugs, since
> NSWorkspaceLaunchConfigurationArguments is ignored when the app is already
> running.
Added back TODO.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:42: *out_pid = app.processIdentifier;
On 2016/10/04 05:54:08, tapted wrote:
> [app processIdentifier]
> 
> But also launchApplicationAtURL will return nil on error - I guess this is
still
> the right thing to do, but it looks a bit weird.
Done. New code now correctly handles nil case.

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:48: DLOG(ERROR) <<
base::SysNSStringToUTF8(error_description);
On 2016/10/04 05:54:08, tapted wrote:
> There's probably a good argument for making this a LOG rather than DLOG. But
> also I'm not sure whether a localized string from userInfo has the best thing
to
> log. Does NSLog do something reasonable? Maybe that will give something with a
> consistent format that we can look for on message boards/logs/bugs
Localized description prints this: 

The application “NameOfTheapp.app” could not be launched because it was not
found.

Which sounds like a very good message. NSLog logs error description, which can
be logged as follows:
LOG(ERROR) << base::SysNSStringToUTF8([launch_error description]);

Do you think it is any better than localized description?

[Matt Giuca, 2016-10-05 00:43:19]

mgiuca@chromium.org changed reviewers:
- mgiuca@chromium.org

[tapted, 2016-10-05 01:40:47]

lgtm

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
File base/mac/launch_services_util.mm (right):

https://codereview.chromium.org/2390953003/diff/20001/base/mac/launch_service...
base/mac/launch_services_util.mm:48: DLOG(ERROR) <<
base::SysNSStringToUTF8(error_description);
On 2016/10/04 07:02:49, Eugene But wrote:
> On 2016/10/04 05:54:08, tapted wrote:
> > There's probably a good argument for making this a LOG rather than DLOG. But
> > also I'm not sure whether a localized string from userInfo has the best
thing
> to
> > log. Does NSLog do something reasonable? Maybe that will give something with
a
> > consistent format that we can look for on message boards/logs/bugs
> Localized description prints this: 
> 
> The application “NameOfTheapp.app” could not be launched because it was not
> found.
> 
> Which sounds like a very good message. NSLog logs error description, which can
> be logged as follows:
> LOG(ERROR) << base::SysNSStringToUTF8([launch_error description]);
> 
> Do you think it is any better than localized description?

The localized description should be OK - I agree it's nicer than a fixed string.
The typical thinking for LOG output is that a user is very unlikely to see it
(better to just crash so we get a report). If a user does see it, they might
plug it into Google, in which case localizing it could result in fewer hits.
Chromium developers might see it locally or in tryjob output - in that case
localizing probably doesn't matter.

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
File base/mac/launch_services_util.h (right):

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
base/mac/launch_services_util.h:22: // first entry. Returns a valid process if
the app was successfully launched.
"without first entry" is a bit ambiguous here. I think we can remove that bit of
the sentence - callers can just assume it does the "right thing" as far as argv
is concerned.

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
File base/mac/launch_services_util.mm (right):

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
base/mac/launch_services_util.mm:48: return Process([app processIdentifier]);
nit: perhaps DCHECK(app) before this, to enforce the assumption that if
launchApplicationAtURL returns nil, it is guaranteed to also set launch_error
(otherwise, we won't get a crash here and this case won't be distinguishable
from `return Process();`

[Eugene But (OOO till 7-30), 2016-10-05 01:49:51]

Thanks!

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
File base/mac/launch_services_util.h (right):

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
base/mac/launch_services_util.h:22: // first entry. Returns a valid process if
the app was successfully launched.
On 2016/10/05 01:40:47, tapted wrote:
> "without first entry" is a bit ambiguous here. I think we can remove that bit
of
> the sentence - callers can just assume it does the "right thing" as far as
argv
> is concerned.
Alright. I actually added this because you were not sure why first entry was
removed.

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
File base/mac/launch_services_util.mm (right):

https://codereview.chromium.org/2390953003/diff/60001/base/mac/launch_service...
base/mac/launch_services_util.mm:48: return Process([app processIdentifier]);
On 2016/10/05 01:40:47, tapted wrote:
> nit: perhaps DCHECK(app) before this, to enforce the assumption that if
> launchApplicationAtURL returns nil, it is guaranteed to also set launch_error
> (otherwise, we won't get a crash here and this case won't be distinguishable
> from `return Process();`

Done.

[Eugene But (OOO till 7-30), 2016-10-05 01:50:14]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-10-05 01:50:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2390953003/#ps80001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2016-10-05 01:50:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 02:00:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 02:00:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eugene But (OOO till 7-30), 2016-10-05 02:17:38]

Nico?

[Nico, 2016-10-05 14:23:18]

lgtm, thanks!

https://codereview.chromium.org/2390953003/diff/80001/base/mac/launch_service...
File base/mac/launch_services_util.mm (right):

https://codereview.chromium.org/2390953003/diff/80001/base/mac/launch_service...
base/mac/launch_services_util.mm:29: const std::string& arg(argv[i]);
You don't need this variable, just pass argv[i] on the next line directly

[Eugene But (OOO till 7-30), 2016-10-06 00:43:49]

Thanks!

[Eugene But (OOO till 7-30), 2016-10-06 00:43:54]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-10-06 00:43:55]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2390953003/#ps100001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2016-10-06 00:44:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 01:56:04]

Description was changed from

==========
[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

This removes base::mac::FSRefFromPath usage from
OpenApplicationWithPath. |FSRefFromPath| is relying on deprecated API
and should be removed.

BUG=650854
==========

to

==========
[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

This removes base::mac::FSRefFromPath usage from
OpenApplicationWithPath. |FSRefFromPath| is relying on deprecated API
and should be removed.

BUG=650854
==========

[commit-bot: I haz the power, 2016-10-06 01:56:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-06 01:59:00]

Description was changed from

==========
[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

This removes base::mac::FSRefFromPath usage from
OpenApplicationWithPath. |FSRefFromPath| is relying on deprecated API
and should be removed.

BUG=650854
==========

to

==========
[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

This removes base::mac::FSRefFromPath usage from
OpenApplicationWithPath. |FSRefFromPath| is relying on deprecated API
and should be removed.

BUG=650854

Committed: https://crrev.com/7eb9c14b4f3c992d2d66f4b6f56e8a42671abd87
Cr-Commit-Position: refs/heads/master@{#423391}
==========

[commit-bot: I haz the power, 2016-10-06 01:59:01]

Patchset 6 (id:??) landed as
https://crrev.com/7eb9c14b4f3c992d2d66f4b6f56e8a42671abd87
Cr-Commit-Position: refs/heads/master@{#423391}