Be explicit about target type in platform_util::OpenItem()

chrome/browser/platform_util.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_PLATFORM_UTIL_H_
 #define CHROME_BROWSER_PLATFORM_UTIL_H_
 
 #include <string>
 
 #include "base/strings/string16.h"
 #include "ui/gfx/native_widget_types.h"
 
 class GURL;
 class Profile;
 
 namespace base {
 class FilePath;
 }
 
 namespace platform_util {
 
 // Show the given file in a file manager. If possible, select the file.
 // The |profile| is used to determine the running profile of file manager app
 // in Chrome OS only. Not used in other platforms.
-// Must be called from the UI thread.
+// Must be called on the UI thread.
 void ShowItemInFolder(Profile* profile, const base::FilePath& full_path);
 
-// Open the given file in the desktop's default manner.
-// Must be called from the UI thread.
-void OpenItem(Profile* profile, const base::FilePath& full_path);
+// Opens the given file in the desktop's default manner. |full_path| must point
+// to an existing file. If |full_path| doesn't refer to an existing file, then

[James Hawkins, 2014/09/29 21:09:14]

This secondary condition "If ... doesn't refer to an existing file" is confusing
because the statement preceding it says it shouldn't be possible.  When the
documentation says 'must', we must assert that the condition is true.  Any other
behavior must be undocumented.

[asanka, 2014/09/30 23:07:46]

Yup. I updated the comments for ShowItemInFolder(), OpenFile(), and
OpenFolder(). I relaxed the requirement that |full_path| must exist and point to
files/folders since that's often not practical for the call site to determine
prior to calling these functions.

They are also currently fire-and-forget with no feedback of whether the
operation succeeded. This behavior reflects the nature of the platform level
APIs that are used to implement them. So I only documented the restrictions that
are enforced within Chrome before the request is passed down to the underlying
platform.

+// no action will be taken. Must be called on the UI thread.
+void OpenFile(Profile* profile, const base::FilePath& full_path);
+
+// Opens the given folder in the file manager. No action will be taken if

[James Hawkins, 2014/09/29 21:09:14]

Why do we not say anything about |full_path| pointing to an existing folder, as
we do for OpenFile?

[James Hawkins, 2014/09/29 21:09:14]

What is the 'file manager'?  Perhaps we should be less-specific, as is the case
for OpenFile where we refer to the desktop's default manner.

[asanka, 2014/09/30 23:07:46]

Comment updated.

[asanka, 2014/09/30 23:07:46]

Comment updated.

+// |full_path| does not refer to an existing folder. Must be called on the UI
+// thread.
+void OpenFolder(Profile* profile, const base::FilePath& full_path);
 
 // Open the given external protocol URL in the desktop's default manner.
 // (For example, mailto: URLs in the default mail user agent.)
 // Must be called from the UI thread.
 void OpenExternal(Profile* profile, const GURL& url);
 
 // Get the top level window for the native view. This can return NULL.
 gfx::NativeWindow GetTopLevel(gfx::NativeView view);
 
 // Get the direct parent of |view|, may return NULL.
@@ skipping 13 matching lines @@
 #if defined(OS_MACOSX)
 // On 10.7+, back and forward swipe gestures can be triggered using a scroll
 // gesture, if enabled in System Preferences. This function returns true if
 // the feature is supported and enabled, and false otherwise.
 bool IsSwipeTrackingFromScrollEventsEnabled();
 #endif
 
 }  // namespace platform_util
 
 #endif  // CHROME_BROWSER_PLATFORM_UTIL_H_