platform_util::OpenItem fixes

platform_util OpenItem/ShowItemInFolder threading fixes.

OpenItem and ShowItemInFolder must now be called from the UI thread only - add
a comment to this effect in the header.
Dispatch the implementations to the FILE thread on chromeos/win/linux.
Fix usage of OpenItem/ShowItemInFolder in DownloadsDomHandler and 
ChromeContentBrowserClient.
Rename ShowFullTabUrl to ViewFolder and get rid of unused Profile* arg.
Use anonymous namespace for functions in file_manager_util.cc and platform_util_chromeos.cc
Replace NewRunnableFunction with Bind.
Add additional threading DCHECKs.
Fix lint errors.

BUG=chromium-os:21585
TEST=go to downloads page (ctrl-j), click on Open Downloads Folder.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109702

[achuithb, 2011-11-07 22:43:02]

This CL is in response to Asanka's comments in 
http://codereview.chromium.org/8439018/

Randy: Please feel free to add additional reviewers or remove yourself from the
reviewers list as necessary. Thanks!

[asanka, 2011-11-08 15:29:29]

Thanks for doing this!

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:84: base::Bind(&OpenItemOnFileThread,
full_path));
Would it make sense to have an OpenItemOnUIThread() instead and post a task
there if the call isn't already on the UI thread?  That will cut down on thread
hopping since FMU::ViewFolder and FMU::ViewItem want to be on the UI thread.

OpenItem() is currently called from the UI thread.

[achuithb, 2011-11-08 20:49:57]

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:84: base::Bind(&OpenItemOnFileThread,
full_path));
file_util::DirectoryExists needs to be called on the FILE thread, so we have to
make 1 trip to the file thread before deciding to call FMU::ViewFolder or
FMU::ViewItem on the UI thread.

Am I missing something?

[asanka, 2011-11-08 20:59:33]

LGTM.

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:84: base::Bind(&OpenItemOnFileThread,
full_path));
On 2011/11/08 20:49:57, achuith.bhandarkar wrote:
> file_util::DirectoryExists needs to be called on the FILE thread, so we have
to
> make 1 trip to the file thread before deciding to call FMU::ViewFolder or
> FMU::ViewItem on the UI thread.
> 
> Am I missing something?

You're correct. Sorry.

[achuithb, 2011-11-08 21:17:33]

Thanks for the review, Asanka.

Randy: Let me know if you'd like to review or pass.

[Randy Smith (Not in Mondays), 2011-11-08 22:12:35]

On 2011/11/08 21:17:33, achuith.bhandarkar wrote:
> Thanks for the review, Asanka.
> 
> Randy: Let me know if you'd like to review or pass.

Achuith: Sorry, I would like to review, just behind.  It's next on my queue, so
you should hear from me later tonight.

[Randy Smith (Not in Mondays), 2011-11-08 22:27:55]

Basically nits; if we're cleaning up the interface I'd like to be consistent and
document restrictions in the header file.

I do think it's important to have a platform owner (which neither Asanka or I
are) go over the code--I think this code is most often called from downloads,
but I don't think that's always the case, and regardless, being the primary code
customer doesn't give us the right to say how it should be written :-}.  Maybe
rsesek@ for mac, estade@ for the header & windows, and dhg@ for linux?  But use
your own judgement.

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
File chrome/browser/platform_util_linux.cc (right):

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
chrome/browser/platform_util_linux.cc:65:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
Should we add something into the header file indicating that this function must
be called on the FILE thread?  (I know it always needed to be called, but we
don't currently say so, and this cleanup seems like a good time to fix that.)

Or we could extend the pattern used elsewhere and make it callable on any thread
and do the redirects here.

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
File chrome/browser/platform_util_mac.mm (right):

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
chrome/browser/platform_util_mac.mm:37: void OpenItem(const FilePath& full_path)
{
So my interpretation of what I'm reading is that we're now counting on OpenItem
to always be called on the UI thread (some implementations redirect, but this
one doesn't, so the interface restriction is that we always call on the UI
thread).  If that's the case, can we update the commentary in platform_util.h to
say so?

[achuithb, 2011-11-09 00:26:27]

Sorry about the rebase. Only the platform_util* files, and
chrome_content_browser_client.cc have changed.

I've done some additional cleanup in platform_util_chromeos (moved the static
functions to anonymous namespace for consistency).
ShowItemInFolder can now be called from the UI thread without a
platform-specific #define, similar to OpenItem.

Asanka/Randy: PTAL.

I've added Mark to review this CL for the changes to platform_util. Mark, if
you're not the right person to review this, please let me know who I should ask
instead.

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
File chrome/browser/platform_util_linux.cc (right):

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
chrome/browser/platform_util_linux.cc:65:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2011/11/08 22:27:55, rdsmith wrote:
> Should we add something into the header file indicating that this function
must
> be called on the FILE thread?  (I know it always needed to be called, but we
> don't currently say so, and this cleanup seems like a good time to fix that.)
> 
> Or we could extend the pattern used elsewhere and make it callable on any
thread
> and do the redirects here.

Done.

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
File chrome/browser/platform_util_mac.mm (right):

http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
chrome/browser/platform_util_mac.mm:37: void OpenItem(const FilePath& full_path)
{
On 2011/11/08 22:27:55, rdsmith wrote:
> So my interpretation of what I'm reading is that we're now counting on
OpenItem
> to always be called on the UI thread (some implementations redirect, but this
> one doesn't, so the interface restriction is that we always call on the UI
> thread).  If that's the case, can we update the commentary in platform_util.h
to
> say so?

Done.

[Mark Mentovai, 2011-11-09 14:22:49]

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_util.h
File chrome/browser/platform_util.h (right):

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
chrome/browser/platform_util.h:20: // Must be called from the UI thread.
This doesn’t seem to be true.

It‘s definitely true of the Mac implementation. However, some of the other
implementations allow it to be called from any thread.

Perhaps those implementations could be refactored slightly so that the work that
needs to be done on the file thread is moved to a separate
(anonymous-namespaced?) helper function. Then, ShowItemInFolder and OpenItem can
DCHECK that they’re on the UI thread and post to the file thread; the helpers
can DCHECK that they’re on the file thread and do the work.

That seems substantially cleaner than having some platforms recycle
ShowItemInFolder and OpenItem for different purposes depending on which thread
they’re on, and having them implement different logic for checking thread
correctness and dealing with being called on an unexpected thread.

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:87:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
What’s the point of this if you ultimately need the work to be done on the UI
thread? OpenItemOnFileThread doesn’t seem to do any work that MUST be done on
the file thread. Why not jump straight to the UI thread? And if you start out on
the UI thread, why not just handle the request directly?

[Randy Smith (Not in Mondays), 2011-11-09 17:20:29]

On 2011/11/09 00:26:27, achuith.bhandarkar wrote:
> Sorry about the rebase. Only the platform_util* files, and
> chrome_content_browser_client.cc have changed.

What I sometimes do (when I remember!) is to upload the rebase and the
functional modifications as two separate patch sets.  (I have a background
thought of hacking Rietveld so if there's an upload that's also a rebase, to
automatically split it out into two patch sets marked differently.  But I'm
probably never going to get the right shape of tuit for that :-}).

> I've done some additional cleanup in platform_util_chromeos (moved the static
> functions to anonymous namespace for consistency).
> ShowItemInFolder can now be called from the UI thread without a
> platform-specific #define, similar to OpenItem.
> 
> Asanka/Randy: PTAL.

This all looks good to me (LGTM).  I might want to take another look after Mark
and you are done, but my guess is that Mark's going to be more picky than I, so
feel free to commit when you have Mark's LGTM unless there's something you'd
specifically like my opinion on.

> 
> I've added Mark to review this CL for the changes to platform_util. Mark, if
> you're not the right person to review this, please let me know who I should
ask
> instead.
> 
>
http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
> File chrome/browser/platform_util_linux.cc (right):
> 
>
http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
> chrome/browser/platform_util_linux.cc:65:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
> On 2011/11/08 22:27:55, rdsmith wrote:
> > Should we add something into the header file indicating that this function
> must
> > be called on the FILE thread?  (I know it always needed to be called, but we
> > don't currently say so, and this cleanup seems like a good time to fix
that.)
> > 
> > Or we could extend the pattern used elsewhere and make it callable on any
> thread
> > and do the redirects here.
> 
> Done.
> 
>
http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
> File chrome/browser/platform_util_mac.mm (right):
> 
>
http://codereview.chromium.org/8457004/diff/27002/chrome/browser/platform_uti...
> chrome/browser/platform_util_mac.mm:37: void OpenItem(const FilePath&
full_path)
> {
> On 2011/11/08 22:27:55, rdsmith wrote:
> > So my interpretation of what I'm reading is that we're now counting on
> OpenItem
> > to always be called on the UI thread (some implementations redirect, but
this
> > one doesn't, so the interface restriction is that we always call on the UI
> > thread).  If that's the case, can we update the commentary in
platform_util.h
> to
> > say so?
> 
> Done.

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:79: base::Bind(&ShowItemInFolder,
full_path));
nit, suggestion (i.e. up to you if you want to take it): The pattern I like
better here is to get the error/less important pathway out of the way at the
beginning, and leave the real code in the main function.  I.e.

if (!on file thread)
  Post task
  Return

Do file thread work


(I know that you may not keep the pattern of having a single function for both
off and on thread--this is just if you do.)

[achuithb, 2011-11-09 20:21:35]

Thanks for the comments, Mark. PTAL.

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_util.h
File chrome/browser/platform_util.h (right):

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
chrome/browser/platform_util.h:20: // Must be called from the UI thread.
On 2011/11/09 14:22:50, Mark Mentovai wrote:
> This doesn’t seem to be true.
> 
> It‘s definitely true of the Mac implementation. However, some of the other
> implementations allow it to be called from any thread.
> 
> Perhaps those implementations could be refactored slightly so that the work
that
> needs to be done on the file thread is moved to a separate
> (anonymous-namespaced?) helper function. Then, ShowItemInFolder and OpenItem
can
> DCHECK that they’re on the UI thread and post to the file thread; the helpers
> can DCHECK that they’re on the file thread and do the work.
> 
> That seems substantially cleaner than having some platforms recycle
> ShowItemInFolder and OpenItem for different purposes depending on which thread
> they’re on, and having them implement different logic for checking thread
> correctness and dealing with being called on an unexpected thread.

Done.

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:79: base::Bind(&ShowItemInFolder,
full_path));
On 2011/11/09 17:20:30, rdsmith wrote:
> nit, suggestion (i.e. up to you if you want to take it): The pattern I like
> better here is to get the error/less important pathway out of the way at the
> beginning, and leave the real code in the main function.  I.e.
> 
> if (!on file thread)
>   Post task
>   Return
> 
> Do file thread work
> 
> 
> (I know that you may not keep the pattern of having a single function for both
> off and on thread--this is just if you do.)

I had your suggestion in this CL and then switched to the way I was doing it
(using if/else instead of the early return). My rationale for switching was that
the if/else was more compact, and I was trying to avoid early returns in the
middle of functions. However, your suggestion is clearer, so I'll go with that
in the future. In this CL, I've split the function by thread.

http://codereview.chromium.org/8457004/diff/35002/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:87:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
On 2011/11/09 14:22:50, Mark Mentovai wrote:
> What’s the point of this if you ultimately need the work to be done on the UI
> thread? OpenItemOnFileThread doesn’t seem to do any work that MUST be done on
> the file thread. Why not jump straight to the UI thread? And if you start out
on
> the UI thread, why not just handle the request directly?

file_util::DirectoryExists must be called on the FILE thread.

[Mark Mentovai, 2011-11-09 20:41:18]

LGTM with these two changes.

http://codereview.chromium.org/8457004/diff/38004/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/38004/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:26: const std::string kGmailComposeUrl
=
There’s still a static constructor (and destructor) here. You should change this
to a const char[].

http://codereview.chromium.org/8457004/diff/38004/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:53: if
(file_util::DirectoryExists(dir))
Use {braces} because the conditioned statement takes up more than one line.

[achuithb, 2011-11-09 22:26:11]

Thanks for the feedback, Mark. I've fixed the last 2 issues.

Randy: I know you wanted to take another look. Please do so at your convenience.

Asanka: Let me know if the previous LGTM still stands - the only changes I've
made since your LGTM have been to platform_util code.

http://codereview.chromium.org/8457004/diff/38004/chrome/browser/platform_uti...
File chrome/browser/platform_util_chromeos.cc (right):

http://codereview.chromium.org/8457004/diff/38004/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:26: const std::string kGmailComposeUrl
=
On 2011/11/09 20:41:18, Mark Mentovai wrote:
> There’s still a static constructor (and destructor) here. You should change
this
> to a const char[].

Done.

http://codereview.chromium.org/8457004/diff/38004/chrome/browser/platform_uti...
chrome/browser/platform_util_chromeos.cc:53: if
(file_util::DirectoryExists(dir))
On 2011/11/09 20:41:18, Mark Mentovai wrote:
> Use {braces} because the conditioned statement takes up more than one line.

Done.

[Mark Mentovai, 2011-11-09 22:41:58]

LGTM

[asanka, 2011-11-09 22:46:37]

LGTM

[Randy Smith (Not in Mondays), 2011-11-09 22:52:27]

Still LGTM.