Use file thread when converting paths to absolute when loading apps.

Use file thread when converting paths to absolute when loading apps.

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

[benwells, 2014-05-09 11:39:56]

Note this doesn't need new tests as it is covered by existing tests, they just
didn't have the thread check in the function that expands relative paths.

[tapted, 2014-05-12 00:07:51]

My only real concern is the raw Extension* in GetAbsolutePathFromCommandLine..

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode319
apps/launcher.cc:319: const Extension* extension_;
should this be a scoped_refptr?

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode351
apps/launcher.cc:351: if (!command_line.GetArgs().size()) {
nit: if (command_line.GetArgs().empty()) {

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode357
apps/launcher.cc:357: // launcher will be freed when nothing has a reference to
it. The message
nit: this comment seems like it should be on the constructor rather than
repeated each call.. here I'd maybe say `|launcher| manages its own lifetime.`
to direct someone to more precise comments on the class.

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode362
apps/launcher.cc:362: launcher->LaunchWithRelativePath(current_directory);
You could also do

  base::PostTaskAndReplyWithResult(
      BrowserThread::GetBlockingPool(),
      FROM_HERE,
      base::Bind(GetAbsolutePathFromCommandLine,
                 command_line,
                 current_directory),
      base::Bind(LaunchPlatformAppWithPath, profile, extension));

It would even properly wrap the refptr. You'd just need to change
GetAbsolutePathFromCommandLine to return a base::FilePath

But also I think I like making PlatformAppPathLauncher more of a thing.

[benwells, 2014-05-12 00:18:51]

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode319
apps/launcher.cc:319: const Extension* extension_;
On 2014/05/12 00:07:51, tapted wrote:
> should this be a scoped_refptr?

I don't think so. My understanding is that extension is not meant to be used as
a ref ptr except internally.

To protect from any extension destruction while a launch with command line data
is going on, we'd have to store the id and look it up all the time.

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode357
apps/launcher.cc:357: // launcher will be freed when nothing has a reference to
it. The message
On 2014/05/12 00:07:51, tapted wrote:
> nit: this comment seems like it should be on the constructor rather than
> repeated each call.. here I'd maybe say `|launcher| manages its own lifetime.`
> to direct someone to more precise comments on the class.

Yep, I'll move it. I remember adding this comment when I had just figured out
how this stuff worked.

[tapted, 2014-05-12 00:27:44]

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode149
apps/launcher.cc:149: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
oh! I forgot - there's now also DCHECK_CURRENTLY_ON for this

[benwells, 2014-05-12 05:51:05]

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode149
apps/launcher.cc:149: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2014/05/12 00:27:45, tapted wrote:
> oh! I forgot - there's now also DCHECK_CURRENTLY_ON for this

Nice, I didn't know about that.

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode319
apps/launcher.cc:319: const Extension* extension_;
On 2014/05/12 00:18:51, benwells wrote:
> On 2014/05/12 00:07:51, tapted wrote:
> > should this be a scoped_refptr?
> 
> I don't think so. My understanding is that extension is not meant to be used
as
> a ref ptr except internally.
> 
> To protect from any extension destruction while a launch with command line
data
> is going on, we'd have to store the id and look it up all the time.

As discussed, added a TODO for this.

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode351
apps/launcher.cc:351: if (!command_line.GetArgs().size()) {
On 2014/05/12 00:07:51, tapted wrote:
> nit: if (command_line.GetArgs().empty()) {

Done.

https://codereview.chromium.org/277873002/diff/1/apps/launcher.cc#newcode357
apps/launcher.cc:357: // launcher will be freed when nothing has a reference to
it. The message
On 2014/05/12 00:18:51, benwells wrote:
> On 2014/05/12 00:07:51, tapted wrote:
> > nit: this comment seems like it should be on the constructor rather than
> > repeated each call.. here I'd maybe say `|launcher| manages its own
lifetime.`
> > to direct someone to more precise comments on the class.
> 
> Yep, I'll move it. I remember adding this comment when I had just figured out
> how this stuff worked.

Actually there is already a comment there :)

[tapted, 2014-05-12 06:03:38]

lgtm

[benwells, 2014-05-12 06:24:59]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-12 06:25:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/277873002/40001

[commit-bot: I haz the power, 2014-05-12 08:21:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-12 11:49:45]

Change committed as 269768