File Manager: fix an unsafe const reference (intermittent crash).

File Manager: fix an unsafe const reference (intermittent crash).

Regression started by (http://crrev.com/148492).

The FilePath.BaseName().value() is a local temp object and it is not safe to
assign it to a const string reference. Use the display_name instead.

BUG=139736
TEST=lumpy

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

Landed as a separate CL: Add ChromeOS-specific OWNERS for extensions.

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

[satorux1, 2012-07-31 00:57:56]

LGTM. Thanks!

[tbarzic, 2012-07-31 01:00:24]

http://codereview.chromium.org/10828086/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10828086/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1156:
files[0].file_path.BaseName().value();
yeah, this is my fault.. I forgot to undo some experimentation I did in
http://crrev.com/148492

I think files[0].display_name should work (even with the reference).

[hshi1, 2012-07-31 01:04:41]

http://codereview.chromium.org/10828086/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10828086/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1156:
files[0].file_path.BaseName().value();
Ah I see so it IS a regression after all... would Patch#2 work for you? Thanks.

On 2012/07/31 01:00:25, tbarzic wrote:
> yeah, this is my fault.. I forgot to undo some experimentation I did in
> http://crrev.com/148492
> 
> I think files[0].display_name should work (even with the reference).

[tbarzic, 2012-07-31 01:07:17]

yep, it is a regression :)

LGTM

thanks for fixing this

[commit-bot: I haz the power, 2012-07-31 01:13:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10828086/2003

[commit-bot: I haz the power, 2012-07-31 01:13:23]

Presubmit check for 10828086-2003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[hshi1, 2012-07-31 01:16:23]

+@ben,@sky - could you please rubber-stamp LGTM please?

For some reason this directory has no OWNER set so it defaults to chrome/OWNER.

[hshi1, 2012-07-31 17:37:22]

+darin@ to review.

The chrome/browser/chromeos/extensions folder has no OWNER defined and we used
to be able to freely land patches. Recently the chrome/ OWNER file has set
noparent hence it would require one of the owners of the entire chrome/ folder
to approve.

I propose that we add a OWNER file for chrome/browser/chromeos/extensions so
that in the future we won't need this.

[Mihai Parparita -not on Chrome, 2012-07-31 17:55:51]

http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/OWNERS (right):

http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/OWNERS:1: achuith@chromium.org
BTW, Bryan is adding this file too: http://codereview.chromium.org/10832086/

Might make sense to have a ChromeOS-specific section, in addition to the copy of
chrome/browser/extensions/OWNERS.

[bryeung, 2012-07-31 18:01:48]

FYI: my change is landed @149221.

Bryan

On Tue, Jul 31, 2012 at 1:55 PM,  <mihaip@chromium.org> wrote:
>
>
http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/OWNERS (right):
>
>
http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/OWNERS:1: achuith@chromium.org
> BTW, Bryan is adding this file too:
> http://codereview.chromium.org/10832086/
>
> Might make sense to have a ChromeOS-specific section, in addition to the
> copy of chrome/browser/extensions/OWNERS.
>
> http://codereview.chromium.org/10828086/

[darin (slow to review), 2012-07-31 18:09:27]

LGTM

[hshi1, 2012-07-31 18:11:19]

Mihai, this adds ChromeOS-specific owners after rebase to revision 149221 (with
the new OWNER file). Does this work for you? Thanks.

http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/OWNERS (right):

http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/OWNERS:1: achuith@chromium.org
Done.

On 2012/07/31 17:55:51, Mihai Parparita wrote:
> BTW, Bryan is adding this file too: http://codereview.chromium.org/10832086/
> 
> Might make sense to have a ChromeOS-specific section, in addition to the copy
of
> chrome/browser/extensions/OWNERS.

[commit-bot: I haz the power, 2012-07-31 18:19:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10828086/3003

[tbarzic, 2012-07-31 18:21:11]

On 2012/07/31 18:11:19, hshi1 wrote:
> Mihai, this adds ChromeOS-specific owners after rebase to revision 149221
(with
> the new OWNER file). Does this work for you? Thanks.
> 
>
http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/OWNERS (right):
> 
>
http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/OWNERS:1: mailto:achuith@chromium.org
> Done.
> 
> On 2012/07/31 17:55:51, Mihai Parparita wrote:
> > BTW, Bryan is adding this file too: http://codereview.chromium.org/10832086/
> > 
> > Might make sense to have a ChromeOS-specific section, in addition to the
copy
> of
> > chrome/browser/extensions/OWNERS.

it may be a good idea to add this in a separate cl?

[Mihai Parparita -not on Chrome, 2012-07-31 18:23:01]

LGTM

[hshi1, 2012-07-31 18:24:16]

On 2012/07/31 18:21:11, tbarzic wrote:
> On 2012/07/31 18:11:19, hshi1 wrote:
> > Mihai, this adds ChromeOS-specific owners after rebase to revision 149221
> (with
> > the new OWNER file). Does this work for you? Thanks.
> > 
> >
>
http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
> > File chrome/browser/chromeos/extensions/OWNERS (right):
> > 
> >
>
http://codereview.chromium.org/10828086/diff/13001/chrome/browser/chromeos/ex...
> > chrome/browser/chromeos/extensions/OWNERS:1: mailto:achuith@chromium.org
> > Done.
> > 
> > On 2012/07/31 17:55:51, Mihai Parparita wrote:
> > > BTW, Bryan is adding this file too:
http://codereview.chromium.org/10832086/
> > > 
> > > Might make sense to have a ChromeOS-specific section, in addition to the
> copy
> > of
> > > chrome/browser/extensions/OWNERS.
> 
> it may be a good idea to add this in a separate cl?

Makes sense. I'll land the fix first, then add the ChromeOS-specific owners in a
separate CL.