Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9280639
or point your web browser to
http://mondrian/9280639
to review the following code:
Change 9280639 by [EMAIL PROTECTED] on 2008/12/05 19:48:11 *pending*
Provide aggregate metadata, for file drag and drop, on IE.
This is the IE analogue of Safari's 9262838 and Firefox's 9228479.
R=noel
[EMAIL PROTECTED]
DELTA=181 (77 added, 69 deleted, 35 changed)
OCL=9280639
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#2
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#9
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#4
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#3
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#15
edit
181 delta lines: 77 added, 69 deleted, 35 changed
Also consider running:
g4 lint -c 9280639
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 9280639 by [EMAIL PROTECTED] on 2008/12/05 19:48:11 *pending*
Provide aggregate metadata, for file drag and drop, on IE.
This is the IE analogue of Safari's 9262838 and Firefox's 9228479.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#2
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#9
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#4
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#3
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#15
edit
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#2
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
2008-12-05 19:45:16.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
2008-12-05 18:58:30.000000000 +1100
@@ -30,6 +30,14 @@
#include <vector>
#include "gears/base/common/js_types.h"
+enum DragAndDropEventType {
+ DRAG_AND_DROP_EVENT_DRAGENTER,
+ DRAG_AND_DROP_EVENT_DRAGOVER,
+ DRAG_AND_DROP_EVENT_DRAGLEAVE,
+ DRAG_AND_DROP_EVENT_DROP,
+ DRAG_AND_DROP_EVENT_INVALID
+};
+
class FileDragAndDropMetaData {
public:
FileDragAndDropMetaData();
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#9
-
c:\devel\srcwingears2/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-05 19:45:16.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-05 19:45:43.000000000 +1100
@@ -41,6 +41,7 @@
#include "gears/base/common/file.h"
#include "gears/base/common/mime_detect.h"
#include "gears/base/firefox/ns_file_utils.h"
+#include "gears/desktop/drag_and_drop_utils_common.h"
#include "gears/desktop/file_dialog.h"
@@ -104,15 +105,6 @@
return true;
#endif
}
-
-
-enum DragAndDropEventType {
- DRAG_AND_DROP_EVENT_DRAGENTER,
- DRAG_AND_DROP_EVENT_DRAGOVER,
- DRAG_AND_DROP_EVENT_DRAGLEAVE,
- DRAG_AND_DROP_EVENT_DROP,
- DRAG_AND_DROP_EVENT_INVALID
-};
// Note that some Mozilla event names differ from the HTML5 standard event
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#4
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2008-12-05 17:59:17.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2008-12-05 19:35:28.000000000 +1100
@@ -37,6 +37,7 @@
#include "gears/desktop/drag_and_drop_utils_ie.h"
#include "gears/base/ie/activex_utils.h"
+#include "gears/desktop/drag_and_drop_utils_common.h"
#include "gears/desktop/file_dialog.h"
@@ -81,9 +82,10 @@
}
-bool GetDroppedFiles(ModuleEnvironment *module_environment,
- JsArray *files_out,
- std::string16 *error_out) {
+bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
+ bool is_in_a_drop,
+ JsObject *data_out,
+ std::string16 *error_out) {
CComPtr<IHTMLWindow2> html_window_2;
if (FAILED(ActiveXUtils::GetHtmlWindow2(
module_environment->iunknown_site_, &html_window_2))) {
@@ -105,48 +107,49 @@
IID_IDataObject, &data_object);
if (FAILED(hr)) return false;
+ std::vector<std::string16> filenames;
FORMATETC desired_format_etc =
{ CF_HDROP, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL };
STGMEDIUM stg_medium;
hr = data_object->GetData(&desired_format_etc, &stg_medium);
- if (FAILED(hr)) return false;
-
- HDROP hdrop = static_cast<HDROP>(GlobalLock(stg_medium.hGlobal));
- if (hdrop == NULL) {
+ if (SUCCEEDED(hr)) {
+ HDROP hdrop = static_cast<HDROP>(GlobalLock(stg_medium.hGlobal));
+ if (hdrop == NULL) {
+ ReleaseStgMedium(&stg_medium);
+ return false;
+ }
+
+ UINT num_files = DragQueryFile(hdrop, -1, 0, 0);
+ TCHAR buffer[MAX_PATH + 1];
+ for (UINT i = 0; i < num_files; i++) {
+ DragQueryFile(hdrop, i, buffer, sizeof(buffer));
+ SHFILEINFO sh_file_info;
+ if (!SHGetFileInfo(buffer, 0, &sh_file_info, sizeof(sh_file_info),
+ SHGFI_ATTRIBUTES) ||
+ !(sh_file_info.dwAttributes & SFGAO_FILESYSTEM) ||
+ (sh_file_info.dwAttributes & SFGAO_FOLDER)) {
+ continue;
+ }
+ if ((sh_file_info.dwAttributes & SFGAO_LINK) &&
+ FAILED(ResolveShortcut(buffer))) {
+ continue;
+ }
+ filenames.push_back(std::string16(buffer));
+ }
+
+ GlobalUnlock(stg_medium.hGlobal);
ReleaseStgMedium(&stg_medium);
- return false;
- }
-
- std::vector<std::string16> filenames;
- UINT num_files = DragQueryFile(hdrop, -1, 0, 0);
- TCHAR buffer[MAX_PATH + 1];
- for (UINT i = 0; i < num_files; i++) {
- DragQueryFile(hdrop, i, buffer, sizeof(buffer));
- SHFILEINFO sh_file_info;
- if (!SHGetFileInfo(buffer, 0, &sh_file_info, sizeof(sh_file_info),
- SHGFI_ATTRIBUTES) ||
- !(sh_file_info.dwAttributes & SFGAO_FILESYSTEM) ||
- (sh_file_info.dwAttributes & SFGAO_FOLDER)) {
- continue;
- }
- if ((sh_file_info.dwAttributes & SFGAO_LINK) &&
- FAILED(ResolveShortcut(buffer))) {
- continue;
- }
- filenames.push_back(std::string16(buffer));
- }
-
- GlobalUnlock(stg_medium.hGlobal);
- ReleaseStgMedium(&stg_medium);
-
- return FileDialog::FilesToJsObjectArray(
- filenames, module_environment, files_out, error_out);
-}
-
-
-bool GetWindowEvent(ModuleEnvironment *module_environment,
- CComPtr<IHTMLEventObj> &window_event,
- CComBSTR &type) {
+ }
+
+ FileDragAndDropMetaData meta_data;
+ meta_data.SetFilenames(filenames);
+ return meta_data.ToJsObject(
+ module_environment, is_in_a_drop, data_out, error_out);
+}
+
+
+DragAndDropEventType GetWindowEvent(ModuleEnvironment *module_environment,
+ CComPtr<IHTMLEventObj> &window_event) {
// We ignore event_as_js_object, since Gears can access the IHTMLWindow2
// and its IHTMLEventObj directly, via COM, and that seems more trustworthy
// than event_as_js_object, which is supplied by (potentially malicious)
@@ -155,10 +158,28 @@
// IHTMLWindow2::get_event (different in that, querying both for their
// IUnknown's gives different pointers).
CComPtr<IHTMLWindow2> window;
- return SUCCEEDED(ActiveXUtils::GetHtmlWindow2(
- module_environment->iunknown_site_, &window)) &&
- SUCCEEDED(window->get_event(&window_event)) &&
- SUCCEEDED(window_event->get_type(&type));
+ CComBSTR type;
+ if (FAILED(ActiveXUtils::GetHtmlWindow2(
+ module_environment->iunknown_site_, &window)) ||
+ FAILED(window->get_event(&window_event)) ||
+ FAILED(window_event->get_type(&type))) {
+ // If we get here, then there is no window.event, so we are not in
+ // the browser's event dispatch.
+ return DRAG_AND_DROP_EVENT_INVALID;
+ }
+
+ if (type == CComBSTR(L"dragover")) {
+ return DRAG_AND_DROP_EVENT_DRAGOVER;
+ } else if (type == CComBSTR(L"dragenter")) {
+ return DRAG_AND_DROP_EVENT_DRAGENTER;
+ } else if (type == CComBSTR(L"dragleave")) {
+ return DRAG_AND_DROP_EVENT_DRAGLEAVE;
+ } else if (type == CComBSTR(L"drop")) {
+ return DRAG_AND_DROP_EVENT_DROP;
+ }
+ // If we get here, then we are in a non drag-and-drop event, such as a
+ // keypress event.
+ return DRAG_AND_DROP_EVENT_INVALID;
}
@@ -167,21 +188,13 @@
bool acceptance,
std::string16 *error_out) {
CComPtr<IHTMLEventObj> window_event;
- CComBSTR type;
- if (!GetWindowEvent(module_environment, window_event, type)) {
- // If we get here, then there is no window.event, so we are not in
- // the browser's event dispatch.
+ DragAndDropEventType type = GetWindowEvent(module_environment, window_event);
+ if (type == DRAG_AND_DROP_EVENT_INVALID) {
*error_out = STRING16(L"The drag-and-drop event is invalid.");
return;
}
- if (type == CComBSTR(L"drop")) {
- // Do nothing.
- return;
-
- } else if (type == CComBSTR(L"dragover") ||
- type == CComBSTR(L"dragenter") ||
- type == CComBSTR(L"dragleave")) {
+ if (type != DRAG_AND_DROP_EVENT_DROP) {
CComQIPtr<IHTMLEventObj2> window_event2(window_event);
CComPtr<IHTMLDataTransfer> data_transfer;
if (!window_event2 ||
@@ -192,11 +205,6 @@
*error_out = GET_INTERNAL_ERROR_MESSAGE();
return;
}
- return;
-
- } else {
- *error_out = STRING16(L"The drag-and-drop event is invalid.");
- return;
}
}
@@ -206,34 +214,17 @@
JsObject *data_out,
std::string16 *error_out) {
CComPtr<IHTMLEventObj> window_event;
- CComBSTR type;
- if (!GetWindowEvent(module_environment, window_event, type)) {
- // If we get here, then there is no window.event, so we are not in
- // the browser's event dispatch.
+ DragAndDropEventType type = GetWindowEvent(module_environment, window_event);
+ if (type == DRAG_AND_DROP_EVENT_INVALID) {
*error_out = STRING16(L"The drag-and-drop event is invalid.");
return;
}
- if (type == CComBSTR(L"drop")) {
- scoped_ptr<JsArray> file_array(
- module_environment->js_runner_->NewArray());
- if (!GetDroppedFiles(module_environment, file_array.get(), error_out)) {
- return;
- }
- data_out->SetPropertyArray(STRING16(L"files"), file_array.get());
- return;
-
- } else if (type == CComBSTR(L"dragover") ||
- type == CComBSTR(L"dragenter") ||
- type == CComBSTR(L"dragleave")) {
- // 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.
- return;
-
- } else {
- *error_out = STRING16(L"The drag-and-drop event is invalid.");
- return;
+ if (!AddFileDragAndDropData(module_environment,
+ type == DRAG_AND_DROP_EVENT_DROP,
+ data_out,
+ error_out)) {
+ assert(!error_out->empty());
}
}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#3
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
2008-12-05 17:59:17.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
2008-12-05 18:53:58.000000000 +1100
@@ -33,9 +33,10 @@
CComPtr<IHTMLEventObj> &html_event_obj,
CComPtr<IHTMLDataTransfer> &html_data_transfer);
-bool GetDroppedFiles(ModuleEnvironment *module_environment,
- JsArray *files_out,
- std::string16 *error_out);
+bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
+ bool is_in_a_drop,
+ JsObject *data_out,
+ std::string16 *error_out);
void AcceptDrag(ModuleEnvironment *module_environment,
JsObject *event,
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#15 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2008-12-05 18:57:25.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2008-12-05 19:43:21.000000000 +1100
@@ -166,6 +166,12 @@
scoped_ptr<JsObject> context_object(
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
+ std::string16 error;
+ AddFileDragAndDropData(module_environment_.get(), false,
+ context_object.get(), &error);
+ if (!error.empty()) {
+ return E_FAIL;
+ }
const int argc = 1;
JsParamToSend argv[argc] = {
{ JSPARAM_OBJECT, context_object.get() }
@@ -202,6 +208,12 @@
scoped_ptr<JsObject> context_object(
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
+ std::string16 error;
+ AddFileDragAndDropData(module_environment_.get(), false,
+ context_object.get(), &error);
+ if (!error.empty()) {
+ return E_FAIL;
+ }
const int argc = 1;
JsParamToSend argv[argc] = {
{ JSPARAM_OBJECT, context_object.get() }
@@ -234,6 +246,12 @@
scoped_ptr<JsObject> context_object(
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
+ std::string16 error;
+ AddFileDragAndDropData(module_environment_.get(), false,
+ context_object.get(), &error);
+ if (!error.empty()) {
+ return E_FAIL;
+ }
const int argc = 1;
JsParamToSend argv[argc] = {
{ JSPARAM_OBJECT, context_object.get() }
@@ -255,17 +273,15 @@
will_accept_drop_ = false;
if (on_drop_.get()) {
- std::string16 error;
- scoped_ptr<JsArray> file_array(
- module_environment_->js_runner_->NewArray());
- if (!GetDroppedFiles(module_environment_.get(), file_array.get(), &error))
{
- return E_FAIL;
- }
-
scoped_ptr<JsObject> context_object(
module_environment_->js_runner_->NewObject());
- context_object->SetPropertyArray(STRING16(L"files"), file_array.get());
AddEventToJsObject(context_object.get());
+ std::string16 error;
+ AddFileDragAndDropData(module_environment_.get(), true,
+ context_object.get(), &error);
+ if (!error.empty()) {
+ return E_FAIL;
+ }
const int argc = 1;
JsParamToSend argv[argc] = {
