Add Query() support to FileRef

webkit/plugins/ppapi/ppb_file_ref_impl.cc

 // 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.
 
 #include "webkit/plugins/ppapi/ppb_file_ref_impl.h"
 
+#include "base/platform_file.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
 #include "ppapi/c/pp_errors.h"
+#include "ppapi/shared_impl/file_type_conversion.h"
+#include "ppapi/shared_impl/time_conversion.h"
+#include "ppapi/shared_impl/var.h"
 #include "ppapi/thunk/enter.h"
 #include "ppapi/thunk/ppb_file_system_api.h"
-#include "ppapi/shared_impl/time_conversion.h"
-#include "ppapi/shared_impl/var.h"
 #include "webkit/plugins/ppapi/common.h"
 #include "webkit/plugins/ppapi/file_callbacks.h"
 #include "webkit/plugins/ppapi/plugin_delegate.h"
 #include "webkit/plugins/ppapi/plugin_module.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/ppb_file_system_impl.h"
 #include "webkit/plugins/ppapi/resource_helper.h"
 
 using ppapi::HostResource;
 using ppapi::PPB_FileRef_CreateInfo;
@@ skipping 59 matching lines @@
   CHECK(pos != std::string::npos);
   return path.substr(pos + 1);
 }
 
 }  // namespace
 
 PPB_FileRef_Impl::PPB_FileRef_Impl(const PPB_FileRef_CreateInfo& info,
                                    PPB_FileSystem_Impl* file_system)
     : PPB_FileRef_Shared(::ppapi::OBJECT_IS_IMPL, info),
       file_system_(file_system),
-      external_file_system_path_() {
+      external_file_system_path_(),
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 PPB_FileRef_Impl::PPB_FileRef_Impl(const PPB_FileRef_CreateInfo& info,
                                    const base::FilePath& external_file_path)
     : PPB_FileRef_Shared(::ppapi::OBJECT_IS_IMPL, info),
       file_system_(),
-      external_file_system_path_(external_file_path) {
+      external_file_system_path_(external_file_path),
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 PPB_FileRef_Impl::~PPB_FileRef_Impl() {
 }
 
 // static
 PPB_FileRef_Impl* PPB_FileRef_Impl::CreateInternal(PP_Resource pp_file_system,
                                                    const std::string& path) {
   EnterResourceNoLock<PPB_FileSystem_API> enter(pp_file_system, true);
   if (enter.failed())
@@ skipping 173 matching lines @@
 
 bool PPB_FileRef_Impl::HasValidFileSystem() const {
   return file_system_ && file_system_->opened();
 }
 
 bool PPB_FileRef_Impl::IsValidNonExternalFileSystem() const {
   return file_system_ && file_system_->opened() &&
       file_system_->type() != PP_FILESYSTEMTYPE_EXTERNAL;
 }
 
+int32_t PPB_FileRef_Impl::Query(PP_FileInfo* info,
+                                scoped_refptr<TrackedCallback> callback) {
+  PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
+  if (!plugin_instance)
+    return PP_ERROR_FAILED;
+
+  if (!file_system_) {
+    // External filesystem
+    // We have to do something totally different for external filesystems.

[yzshen1, 2013/03/26 23:32:42]

nit: file systems.

+    if (!plugin_instance->delegate()->AsyncOpenFile(
+        GetSystemPath(), base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
+        base::Bind(&PPB_FileRef_Impl::QueryCallback,
+                   weak_factory_.GetWeakPtr(), info, callback)))
+      return PP_ERROR_FAILED;
+  } else {
+    // Non-external filesystem

[yzshen1, 2013/03/26 23:32:42]

Just out of curiosity: I wonder why we have IsValidNonExternalFileSystem(), if
(file_system.get() != NULL) ==> non-external filesystem.
(That method has a check about file_system_->type().)

+    if (!HasValidFileSystem())
+      return PP_ERROR_NOACCESS;
+
+    if (!plugin_instance->delegate()->Query(
+            GetFileSystemURL(),
+            new FileCallbacks(this, callback, info, file_system_)))
+      return PP_ERROR_FAILED;
+
+  }
+  return PP_OK_COMPLETIONPENDING;
+}
+
+void PPB_FileRef_Impl::QueryCallback(PP_FileInfo* info,
+                                     scoped_refptr<TrackedCallback> callback,
+                                     base::PlatformFileError error_code,
+                                     base::PassPlatformFile passed_file) {
+  int32_t pp_error = ::ppapi::PlatformFileErrorToPepperError(error_code);

[yzshen1, 2013/03/26 23:32:42]

Please test that whether |callback| is still pending. If not you could return
early.

+  if (pp_error != PP_OK)
+    callback->Run(pp_error);
+  base::PlatformFile file = passed_file.ReleaseValue();

[yzshen1, 2013/03/26 23:32:42]

It seems the file handle is leaked?
(Besides, because you are using weakptr factory, it is not reliable to close the
file handle in GetFileInfoCallback().)

+
+  PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
+  if (!plugin_instance)
+    callback->Run(PP_ERROR_FAILED);
+
+  if (!base::FileUtilProxy::GetFileInfoFromPlatformFile(
+      plugin_instance->delegate()->GetFileThreadMessageLoopProxy(), file,
+      base::Bind(&PPB_FileRef_Impl::GetFileInfoCallback,
+                 weak_factory_.GetWeakPtr(), info, callback)))
+    callback->Run(PP_ERROR_FAILED);
+}
+
+void PPB_FileRef_Impl::GetFileInfoCallback(
+    PP_FileInfo* info,
+    scoped_refptr<TrackedCallback> callback,
+    base::PlatformFileError error_code,
+    const base::PlatformFileInfo& file_info) {
+  int32_t pp_error = ::ppapi::PlatformFileErrorToPepperError(error_code);

[yzshen1, 2013/03/26 23:32:42]

Same here, please test whether |callback| is still pending, if not, you
shouldn't touch |info|. It may be invalid.

+  if (pp_error != PP_OK)
+    callback->Run(pp_error);
+
+  info->size = file_info.size;
+  if (file_info.is_symbolic_link)
+    info->type = PP_FILETYPE_OTHER;
+  else if (file_info.is_directory)
+    info->type = PP_FILETYPE_DIRECTORY;
+  else
+    info->type = PP_FILETYPE_REGULAR;
+
+  info->system_type = PP_FILESYSTEMTYPE_EXTERNAL;
+  info->creation_time = file_info.creation_time.ToDoubleT();
+  info->last_access_time = file_info.last_accessed.ToDoubleT();
+  info->last_modified_time = file_info.last_modified.ToDoubleT();
+  callback->Run(PP_OK);
+}
+
+
 }  // namespace ppapi
 }  // namespace webkit