[NaCl SDK] Expose Google Drive to nacl_io.

native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h

Index: native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h
diff --git a/native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h b/native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h
new file mode 100644
index 0000000000000000000000000000000000000000..850986cbb10382fd59aea3064fbed5c8a93b5460
--- /dev/null
+++ b/native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h
@@ -0,0 +1,87 @@
+// Copyright (c) 2016 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 LIBRARIES_NACL_IO_GOOGLEDRIVEFS_UTIL_H_
+#define LIBRARIES_NACL_IO_GOOGLEDRIVEFS_UTIL_H_
+
+#include <stdlib.h>
+
+#include "nacl_io/error.h"
+#include "nacl_io/filesystem.h"
+#include "nacl_io/path.h"
+#include "nacl_io/pepper_interface.h"
+
+namespace nacl_io {
+
+static const char FOLDER_MIME_TYPE[] = "application/vnd.google-apps.folder";

[binji, 2016/09/20 01:22:05]

you shouldn't define static variables in a header file; they'll be duplicated in
every translation unit that includes the header. Instead do this:

something.h:
extern const char DRIVE_URL[];

something.cc
const char DRIVE_URL[] = "...";

[binji, 2016/09/20 01:22:05]

All of these values and functions has relatively generic names for being
GoogleDriveFS-specific. I'd suggest moving it into a class, or wrapping it in a
namespace.

[chanpatorikku, 2016/09/21 16:41:09]

~250 bytes are going to be duplicated.  
Code changes may have to be there for the hundreds of bytes, so the static
variables are changed to extern variables.

[chanpatorikku, 2016/09/21 16:41:09]

There're variables specific to a class in other files in the nacl_io directory.

socket/socket_node.h:

static const size_t MAX_SOCK_TRANSFER = 65536;

Would you still like to change the code?

+
+static const char DRIVE_URL[] = "https://www.googleapis.com/drive/v3/files";
+static const char UPLOAD_DRIVE_URL[] =
+    "https://www.googleapis.com/upload/drive/v3/files";
+static const char DOWNLOAD_DRIVE_URL[] =
+    "https://www.googleapis.com/download/drive/v3/files";
+
+struct RequestUrlParams {
+  std::string url;
+  std::string method;
+  std::string headers;
+  std::string body;
+};
+
+std::string ParentEqualClause(const std::string& parent_dir_id);
+std::string NameEqualClause(const std::string& name);
+
+int ExtractYearFromRFC3339(const std::string& date_time);

[binji, 2016/09/20 01:22:05]

are these ever used independently from ConvertDateTimeToEpochTime? If not, I'd
suggest not exposing these functions as part of the API.

[chanpatorikku, 2016/09/21 16:41:09]

No, these are not ever used independently from ConvertDateTimeToEpochTime.
Further, ConvertDateTimeToEpochTime is only used in
GoogleDriveFsNode::GetModifiedTime.

When only one source file uses a function, some of the functions are written in
an unnamed namespace.
When more than one source file uses the function, however, the functions
probably are refactored to shared code.
The functions can be exposed, so exposing them rather than not exposing them and
sharing them later seems to be another idea.

Moreover, review time is increased when there're more code changes.

Would you still like not to expose the functions?

+int ExtractMonthFromRFC3339(const std::string& date_time);
+int ExtractDayFromRFC3339(const std::string& date_time);
+int ExtractHourFromRFC3339(const std::string& date_time);
+int ExtractMinuteFromRFC3339(const std::string& date_time);
+int ExtractSecondFromRFC3339(const std::string& date_time);
+
+time_t ConvertDateTimeToEpochTime(int year,
+                                  int month,
+                                  int day,
+                                  int hour,
+                                  int minute,
+                                  int second);
+
+void AddUrlPath(const std::string& path, std::string* out_url);

[binji, 2016/09/20 01:22:05]

As mentioned before, a nicer API would be to make RequestUrlParams a class and
have these functions use it directly (without having to pass in the URL as an
out param).

[chanpatorikku, 2016/09/21 16:41:09]

The reply of this message was done for another message of the same aim.
Please look at the other reply.

+void AddUrlQAttributeValue(std::string* array,
+                           size_t size,
+                           std::string* out_q_attribute_value);
+void AddUrlFirstQueryParameter(const std::string& attribute,
+                               const std::string& value,
+                               std::string* out_url);
+void AddUrlNextQueryParameter(const std::string& attribute,
+                              const std::string& value,
+                              std::string* out_url);
+void AddHeaders(const std::string& header_field_key,
+                const std::string& header_field_value,
+                std::string* out_headers);
+void AddBody(const std::string& data, std::string* out_body);
+Error LoadUrl(PepperInterface* ppapi,
+              const RequestUrlParams& params,
+              ScopedResource* out_url_response_info_resource);
+Error ReadResponseBody(PepperInterface* ppapi,
+                       PP_Resource url_response_info_object,
+                       int32_t bytes_to_read,
+                       std::string* out_output);
+int32_t ReadStatusCode(PepperInterface* ppapi,
+                       PP_Resource url_response_info_object);
+Error GetValueStringAndValuePos(const std::string& json,
+                                const std::string& key,
+                                size_t key_search_pos,
+                                std::string* out_value_string,
+                                size_t* out_value_pos);
+Error RequestItemIdAndItemType(const std::string& parent_dir_id,

[binji, 2016/09/20 01:22:05]

why aren't these functions on GoogleFilesystemFS? Having them pass in a
Filesystem* is confusing as it won't work with any other filesystem.

[chanpatorikku, 2016/09/21 16:41:09]

If what you mean is that why aren't these functions defined as static function
on a class, the reason is that the calls of static functions of a class and the
calls of a global function in a certain namespace are made in the same way.

Filesystem* changed to GoogleDriveFs* since Filesystem* is confusing.

+                               const std::string& item_name,
+                               Filesystem* filesystem,
+                               std::string* out_item_id,
+                               bool* out_is_dir_type);
+Error RequestParentDirId(const Path& path,
+                         Filesystem* filesystem,
+                         std::string* out_parent_dir_id);
+}  // namespace nacl_io
+
+#endif  // LIBRARIES_NACL_IO_GOOGLEDRIVEFS_UTIL_H_