Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9228479
or point your web browser to
http://mondrian/9228479
to review the following code:
Change 9228479 by [EMAIL PROTECTED] on 2008/12/02 22:11:48 *pending*
Provide aggregate metadata, for file drag and drop. Specifically:
(1) MIME types, (2) file extensions, (3) total file size in bytes,
and (4) file count.
This CL only implements this feature for Firefox, and future CLs
will implement for IE and Safari.
PRESUBMIT=passed
R=noel
[EMAIL PROTECTED]
DELTA=126 (101 added, 10 deleted, 15 changed)
OCL=9228479
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#3
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#15
edit
126 delta lines: 101 added, 10 deleted, 15 changed
Also consider running:
g4 lint -c 9228479
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 9228479 by [EMAIL PROTECTED] on 2008/12/02 22:11:48 *pending*
Provide aggregate metadata, for file drag and drop. Specifically:
(1) MIME types, (2) file extensions, (3) total file size in bytes,
and (4) file count.
This CL only implements this feature for Firefox, and future CLs
will implement for IE and Safari.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#3
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#15
edit
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#6
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-02 16:11:08.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-02 22:14:33.000000000 +1100
@@ -38,6 +38,8 @@
#include "gears/desktop/drag_and_drop_utils_ff.h"
+#include "gears/base/common/file.h"
+#include "gears/base/common/mime_detect.h"
#include "gears/base/firefox/ns_file_utils.h"
#include "gears/desktop/file_dialog.h"
@@ -305,6 +307,22 @@
}
+static void JsObjectSetPropertyStringArray(
+ JsObject *data_out,
+ ModuleEnvironment *module_environment,
+ const char16 *property_key,
+ std::set<std::string16> &strings) {
+ scoped_ptr<JsArray> array(module_environment->js_runner_->NewArray());
+ int i = 0;
+ for (std::set<std::string16>::iterator iter = strings.begin();
+ iter != strings.end();
+ ++iter) {
+ array->SetElementString(i++, *iter);
+ }
+ data_out->SetPropertyArray(property_key, array.get());
+}
+
+
void GetDragData(ModuleEnvironment *module_environment,
JsObject *event_as_js_object,
JsObject *data_out,
@@ -317,6 +335,11 @@
return;
}
+ // TODO(nigeltao): Should we early exit for DRAG_AND_DROP_EVENT_DRAGLEAVE
+ // (and not provide e.g. fileCount, fileTotalBytes, etcetera)? The answer
+ // probably depends on what is feasible (e.g. can we distinguish a dragover
+ // and a dragleave on Safari) on other browser/OS combinations.
+
nsCOMPtr<nsIDragService> drag_service =
do_GetService("@mozilla.org/widget/dragservice;1");
if (!drag_service) {
@@ -331,27 +354,60 @@
return;
}
+ // TODO(nigeltao): cache the filenames (and their MIME types, extensions,
+ // etcetera), to avoid hitting the file system on every DnD event.
+ // This probably requires being able to reliably distinguish when a
+ // drag session ends and a new one begins, which might not be trivial if
+ // you get the same nsIDragSession pointer (since it's just the singleton
+ // DragService that's been QueryInterface'd) for two separate sessions.
+ std::vector<std::string16> filenames;
+ std::set<std::string16> file_extensions;
+ std::set<std::string16> file_mime_types;
+ int64 fileTotalBytes = 0;
+ if (!GetDroppedFiles(module_environment,
+ drag_session.get(),
+ &filenames,
+ &file_extensions,
+ &file_mime_types,
+ &fileTotalBytes)) {
+ // If GetDroppedFiles fails, then GetDroppedFiles may have added partial
+ // results to the filenames vector, and other accumulators, which we
+ // should clear out.
+ // TODO(nigeltao): Should we throw an error here, or just happily return
+ // empty? How should we behave if the user drags and drops Text or a URL?
+ filenames.clear();
+ file_extensions.clear();
+ file_mime_types.clear();
+ fileTotalBytes = 0;
+ }
+
+ data_out->SetPropertyInt(STRING16(L"fileCount"), filenames.size());
+ data_out->SetPropertyDouble(STRING16(L"fileTotalBytes"), fileTotalBytes);
+ JsObjectSetPropertyStringArray(data_out, module_environment,
+ STRING16(L"fileExtensions"), file_extensions);
+ JsObjectSetPropertyStringArray(data_out, module_environment,
+ STRING16(L"fileMimeTypes"), file_mime_types);
+
if (type == DRAG_AND_DROP_EVENT_DROP) {
scoped_ptr<JsArray> file_array(
module_environment->js_runner_->NewArray());
- if (!GetDroppedFiles(module_environment, drag_session.get(),
- file_array.get(), error_out)) {
+ if (!FileDialog::FilesToJsObjectArray(filenames, module_environment,
+ file_array.get(), error_out)) {
+ // FilesToJsObjectArray will set error_out.
return;
}
data_out->SetPropertyArray(STRING16(L"files"), file_array.get());
}
-
- // TODO(nigeltao): For all DnD events (including the non-drop events
- // dragenter and dragover), we still should provide (1) MIME types,
- // (2) file extensions, (3) total file size, and (4) file count.
}
bool GetDroppedFiles(
ModuleEnvironment *module_environment,
nsIDragSession *drag_session,
- JsArray *files_out,
- std::string16 *error_out) {
+ std::vector<std::string16> *filenames_out,
+ std::set<std::string16> *file_extensions_out,
+ std::set<std::string16> *file_mime_types_out,
+ int64 *file_total_bytes_out) {
#if defined(LINUX) && !defined(OS_MACOSX)
// Although Firefox's underlying XPCOM widget library aims to present a
// consistent cross-platform interface, there are still significant
@@ -374,7 +430,6 @@
nsresult nr = drag_session->GetNumDropItems(&num_drop_items);
if (NS_FAILED(nr) || num_drop_items <= 0) { return false; }
- std::vector<std::string16> filenames;
for (int i = 0; i < static_cast<int>(num_drop_items); i++) {
nsCOMPtr<nsITransferable> transferable =
do_CreateInstance("@mozilla.org/widget/transferable;1", &nr);
@@ -402,6 +457,9 @@
nsString filename;
nr = file->GetPath(filename);
if (NS_FAILED(nr)) { return false; }
+ PRInt64 file_size;
+ nr = file->GetFileSize(&file_size);
+ if (NS_FAILED(nr)) { return false; }
#else
nsCOMPtr<nsIFile> file(do_QueryInterface(data));
if (!file) { return false; }
@@ -420,6 +478,9 @@
nr = local_file->SetFollowLinks(PR_TRUE);
if (NS_FAILED(nr)) { return false; }
nr = local_file->InitWithPath(path);
+ if (NS_FAILED(nr)) { return false; }
+ PRInt64 file_size;
+ nr = local_file->GetFileSize(&file_size);
if (NS_FAILED(nr)) { return false; }
nr = NS_ERROR_FAILURE;
@@ -432,9 +493,16 @@
if (NS_FAILED(nr)) { return false; }
#endif
- filenames.push_back(std::string16(filename.get()));
- }
-
- return FileDialog::FilesToJsObjectArray(
- filenames, module_environment, files_out, error_out);
-}
+ filenames_out->push_back(std::string16(filename.get()));
+ // TODO(nigeltao): Decide whether we should insert ".txt" or "txt" -
+ // that is, does the file extension include the dot at the start.
+ file_extensions_out->insert(File::GetFileExtension(filename.get()));
+ // TODO(nigeltao): Should we also keep an array of per-file MIME types,
+ // not just the overall set of MIME types. If so, the mimeType should
+ // probably be a property of the file JavaScript object (i.e. the thing
+ // with a name and blob property), not a separate array to the files array.
+ file_mime_types_out->insert(DetectMimeTypeOfFile(filename.get()));
+ *file_total_bytes_out += file_size;
+ }
+ return true;
+}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#3
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2008-12-02 16:11:09.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2008-12-02 22:12:58.000000000 +1100
@@ -26,14 +26,20 @@
#ifndef GEARS_DESKTOP_DRAG_AND_DROP_UTILS_FF_H__
#define GEARS_DESKTOP_DRAG_AND_DROP_UTILS_FF_H__
+#include <set>
+#include <vector>
+
#include "gears/base/common/base_class.h"
class nsIDragSession;
+// The xxx_out parameters are presumed to be initialized to empty (or zero).
bool GetDroppedFiles(ModuleEnvironment *module_environment,
nsIDragSession *drag_session,
- JsArray *files_out,
- std::string16 *error_out);
+ std::vector<std::string16> *filenames_out,
+ std::set<std::string16> *file_extensions_out,
+ std::set<std::string16> *file_mime_types_out,
+ int64 *file_total_bytes_out);
void AcceptDrag(ModuleEnvironment *module_environment,
JsObject *event,
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#15 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-12-02 16:11:09.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-12-02 22:05:39.000000000 +1100
@@ -227,22 +227,39 @@
ProvideDebugVisualFeedback(false);
if (will_accept_drop_) {
will_accept_drop_ = false;
- std::string16 error;
+ std::vector<std::string16> filenames;
+ std::set<std::string16> file_extensions;
+ std::set<std::string16> file_mime_types;
+ int64 file_total_bytes;
+ // TODO(nigeltao): We should call GetDroppedFiles (and provide the
+ // aggregate file metadata) during dragenter, dragover and dragleave,
+ // not just during drop.
+ if (!GetDroppedFiles(module_environment_.get(),
+ drag_session.get(),
+ &filenames,
+ &file_extensions,
+ &file_mime_types,
+ &file_total_bytes)) {
+ return NS_ERROR_FAILURE;
+ }
+
+ scoped_ptr<JsObject> context_object(
+ module_environment_->js_runner_->NewObject());
scoped_ptr<JsArray> file_array(
module_environment_->js_runner_->NewArray());
- if (!GetDroppedFiles(module_environment_.get(), drag_session.get(),
- file_array.get(), &error)) {
+ std::string16 ignored;
+ if (!FileDialog::FilesToJsObjectArray(filenames,
+ module_environment_.get(),
+ file_array.get(),
+ &ignored)) {
return NS_ERROR_FAILURE;
}
+ context_object->SetPropertyArray(STRING16(L"files"), file_array.get());
+ AddEventToJsObject(context_object.get(), event);
// Prevent the default browser behavior of navigating away from the
// current page to the file being dropped.
event->StopPropagation();
-
- scoped_ptr<JsObject> context_object(
- module_environment_->js_runner_->NewObject());
- context_object->SetPropertyArray(STRING16(L"files"), file_array.get());
- AddEventToJsObject(context_object.get(), event);
const int argc = 1;
JsParamToSend argv[argc] = {
