Hello noel, dsymonds,
I'd like you to do a code review. Please execute
g4 diff -c 11170799
or point your web browser to
http://mondrian/11170799
to review the following code:
Change 11170799 by nigel...@nigeltao-srcgears2 on 2009/05/18 20:14:21 *pending*
In file drag-and-drop, let FileDragAndDropMetaData distinguish
between not having any files in the clipboard (or pasteboard), and
having unreadable files (e.g. directories) in the clipboard.
Previously, both states were indicated by filenames_ being an
empty array. Now, there is another has_files_ member variable.
PRESUBMIT=passed
R=noel,dsymonds
[email protected]
DELTA=56 (27 added, 5 deleted, 24 changed)
OCL=11170799
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#8
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#18
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#17
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#13
edit
56 delta lines: 27 added, 5 deleted, 24 changed
Also consider running:
g4 lint -c 11170799
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 11170799 by nigel...@nigeltao-srcgears2 on 2009/05/18 20:14:21 *pending*
In file drag-and-drop, let FileDragAndDropMetaData distinguish
between not having any files in the clipboard (or pasteboard), and
having unreadable files (e.g. directories) in the clipboard.
Previously, both states were indicated by filenames_ being an
empty array. Now, there is another has_files_ member variable.
OCL=11170799
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#8
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#18
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#17
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#13
edit
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#6
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
2009-05-18 20:14:28.000000000 +1000
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
2009-05-18 20:58:54.000000000 +1000
@@ -58,11 +58,13 @@
extensions_.clear();
mime_types_.clear();
total_bytes_ = 0;
+ has_files_ = false;
}
void FileDragAndDropMetaData::SetFilenames(std::vector<std::string16> &names) {
assert(IsEmpty());
+ has_files_ = true;
filenames_.swap(names);
for (std::vector<std::string16>::iterator i = filenames_.begin();
i != filenames_.end(); ++i) {
@@ -81,6 +83,13 @@
JsObject *object_out,
std::string16 *error_out) {
JsRunnerInterface *runner = module_environment->js_runner_;
+ if (!has_files_)
+ return false;
+
+ // TODO(nigeltao): Suppose the user is dragging a directory. Do we want to
+ // return null (in JavaScript's desktop.getDragData) or do we instead want
+ // to return something with count 0? For the former, we want to do the two
+ // lines below. For the latter, we want to delete the two lines below.
if (IsEmpty())
return false;
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#8
-
/home/nigeltao/srcgears2/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
2009-05-18 20:14:28.000000000 +1000
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
2009-05-18 20:58:14.000000000 +1000
@@ -77,6 +77,12 @@
std::set<std::string16> mime_types_;
int64 total_bytes_;
+ // Setting has_files to true means that the clipboard had files on it,
+ // whether or not those files were readable (e.g. they weren't directories).
+ // If they weren't readable, then has_files_ will be true but filenames_
+ // will be empty.
+ bool has_files_;
+
DISALLOW_EVIL_CONSTRUCTORS(FileDragAndDropMetaData);
};
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#18
-
/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
2009-05-18 20:14:28.000000000 +1000
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2009-05-18 21:15:27.000000000 +1000
@@ -422,7 +422,6 @@
// Convert from file:// URLs to an actual filename (which also involves
// decoding %20s into spaces, for example).
- bool success = true;
std::vector<std::string16> filenames;
for (std::vector<std::string>::iterator i = filenames_as_ascii_urls.begin();
i != filenames_as_ascii_urls.end(); ++i) {
@@ -433,23 +432,21 @@
nsCOMPtr<nsIFile> file;
nsresult nr = NSFileUtils::GetFileFromURLSpec(
filename_nsstring, getter_AddRefs(file));
- if (NS_FAILED(nr)) { success = false; break; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
nsString filename;
nr = file->GetPath(filename);
- if (NS_FAILED(nr)) { success = false; break; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
PRBool bool_result;
nr = file->IsDirectory(&bool_result);
- if (NS_FAILED(nr) || bool_result) { success = false; break; }
+ if (NS_FAILED(nr) || bool_result) { filenames.clear(); break; }
nr = file->IsSpecial(&bool_result);
- if (NS_FAILED(nr) || bool_result) { success = false; break; }
+ if (NS_FAILED(nr) || bool_result) { filenames.clear(); break; }
filenames.push_back(std::string16(filename.get()));
}
assert(g_file_drag_and_drop_meta_data->IsEmpty());
- if (success) {
- g_file_drag_and_drop_meta_data->SetFilenames(filenames);
- }
+ g_file_drag_and_drop_meta_data->SetFilenames(filenames);
return TRUE;
}
@@ -576,11 +573,12 @@
nsresult nr = drag_session->GetNumDropItems(&num_drop_items);
if (NS_FAILED(nr) || num_drop_items <= 0) { return false; }
+ bool has_files = 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);
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
transferable->AddDataFlavor(kFileMime);
drag_session->GetData(transferable, i);
@@ -588,29 +586,33 @@
PRUint32 data_len;
nr = transferable->GetTransferData(
kFileMime, getter_AddRefs(data), &data_len);
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
nsCOMPtr<nsIFile> file(do_QueryInterface(data));
- if (!file) { return false; }
+ if (!file) { filenames.clear(); break;; }
nsString path;
nr = file->GetPath(path);
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
+
+ has_files = true;
PRBool bool_result = false;
if (NS_FAILED(file->IsDirectory(&bool_result)) || bool_result) {
- return false;
+ filenames.clear();
+ break;
}
if (NS_FAILED(file->IsSpecial(&bool_result)) || bool_result) {
- return false;
+ filenames.clear();
+ break;
}
nsCOMPtr<nsILocalFile> local_file =
do_CreateInstance("@mozilla.org/file/local;1", &nr);
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
nr = local_file->SetFollowLinks(PR_TRUE);
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
nr = local_file->InitWithPath(path);
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
nr = NS_ERROR_FAILURE;
nsString filename;
@@ -619,7 +621,7 @@
} else if (NS_SUCCEEDED(file->IsFile(&bool_result)) && bool_result) {
nr = local_file->GetPath(filename);
}
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr)) { filenames.clear(); break; }
filenames.push_back(std::string16(filename.get()));
}
@@ -632,7 +634,9 @@
#else
FileDragAndDropMetaData file_drag_and_drop_meta_data;
#endif
- file_drag_and_drop_meta_data.SetFilenames(filenames);
+ if (has_files) {
+ file_drag_and_drop_meta_data.SetFilenames(filenames);
+ }
return file_drag_and_drop_meta_data.ToJsObject(
module_environment,
type == DRAG_AND_DROP_EVENT_DROP,
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#17
-
/home/nigeltao/srcgears2/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
2009-05-18 20:14:29.000000000 +1000
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2009-05-18 21:07:36.000000000 +1000
@@ -157,6 +157,7 @@
IID_IDataObject, &data_object);
if (FAILED(hr)) return false;
+ bool has_files = false;
std::vector<std::string16> filenames;
FORMATETC desired_format_etc =
{ CF_HDROP, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL };
@@ -170,19 +171,18 @@
}
UINT num_files = DragQueryFile(hdrop, -1, 0, 0);
+ has_files = num_files > 0;
for (UINT i = 0; i < num_files; ++i) {
WCHAR path[MAX_PATH];
if (!DragQueryFile(hdrop, i, path, MAX_PATH)) {
- GlobalUnlock(stg_medium.hGlobal);
- ReleaseStgMedium(&stg_medium);
- return false;
+ filenames.clear();
+ break;
}
FileAttributes file(path);
if (!file.Found() || file.IsDirectory()) {
- GlobalUnlock(stg_medium.hGlobal);
- ReleaseStgMedium(&stg_medium);
- return false;
+ filenames.clear();
+ break;
}
LOG16((L" found %s\n", path));
@@ -193,7 +193,9 @@
ReleaseStgMedium(&stg_medium);
}
- meta_data_out->SetFilenames(filenames);
+ if (has_files) {
+ meta_data_out->SetFilenames(filenames);
+ }
return true;
}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#13
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
2009-05-18 20:14:29.000000000 +1000
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
2009-05-18 21:08:51.000000000 +1000
@@ -148,11 +148,12 @@
BOOL is_drag_from_another_application =
([draggingInfo draggingSource] == nil);
- BOOL success = YES;
+ BOOL has_files = NO;
if (is_drag_from_another_application) {
std::vector<std::string16> filenames;
NSPasteboard *pboard = [draggingInfo draggingPasteboard];
if ([[pboard types] containsObject:NSFilenamesPboardType]) {
+ has_files = YES;
NSArray *files = [pboard propertyListForType:NSFilenamesPboardType];
NSEnumerator *enumerator = [files objectEnumerator];
NSFileManager *fileManager = [NSFileManager defaultManager];
@@ -163,13 +164,13 @@
BOOL isDir;
if ([fileManager fileExistsAtPath:ns_string isDirectory:&isDir] &&
isDir) {
- success = NO;
+ filenames.clear();
break;
}
filenames.push_back(std_string);
}
}
- if (success) {
+ if (has_files) {
g_file_drag_and_drop_meta_data->SetFilenames(filenames);
}
}
