Hello zork,
I'd like you to do a code review. Please execute
g4 diff -c 8549551
or point your web browser to
http://mondrian/8549551
(this changelist has been uploaded to Mondrian)
to review the following code:
Change 8549551 by [EMAIL PROTECTED] on 2008/10/09 15:23:04 *pending*
Show the security origin in the openFiles() title bar.
Otherwise, on a page with multiple gadgets, a user cannot tell
which site he/she is giving file access to.
PRESUBMIT=passed
R=zork
[EMAIL PROTECTED]
DELTA=51 (30 added, 3 deleted, 18 changed)
OCL=8549551
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#51 edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog.cc#13 edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog.h#7 edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_gtk.cc#8
edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_osx.cc#10
edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_win32.cc#9
edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_win32.h#7
edit
... //depot/googleclient/gears/opensource/gears/ui/common/i18n_strings.cc#1 edit
... //depot/googleclient/gears/opensource/gears/ui/common/i18n_strings.h#1 edit
51 delta lines: 30 added, 3 deleted, 18 changed
Also consider running:
g4 lint -c 8549551
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 8549551 by [EMAIL PROTECTED] on 2008/10/09 15:23:04 *pending*
Show the security origin in the openFiles() title bar.
Otherwise, on a page with multiple gadgets, a user cannot tell
which site he/she is giving file access to.
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#51 edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog.cc#13 edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog.h#7 edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_gtk.cc#8
edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_osx.cc#10
edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_win32.cc#9
edit
... //depot/googleclient/gears/opensource/gears/desktop/file_dialog_win32.h#7
edit
... //depot/googleclient/gears/opensource/gears/ui/common/i18n_strings.cc#1 edit
... //depot/googleclient/gears/opensource/gears/ui/common/i18n_strings.h#1 edit
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#51 -
C:\src\0L/googleclient/gears/opensource/gears/desktop/desktop.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.cc 2008-10-09
18:49:28.000000000 -0700
+++ googleclient/gears/opensource/gears/desktop/desktop.cc 2008-10-09
17:37:21.000000000 -0700
@@ -430,16 +430,17 @@
// TODO(cdevries): set focus to tab where this function was called
+ scoped_refptr<ModuleEnvironment> environment;
+ GetModuleEnvironment(&environment);
+
FileDialog::Options options;
if (argv[1].was_specified) {
- if (!FileDialog::ParseOptions(context, options_map, &options)) {
+ if (!FileDialog::ParseOptions(context, *environment, options_map,
+ &options)) {
assert(context->is_exception_set());
return;
}
}
-
- scoped_refptr<ModuleEnvironment> environment;
- GetModuleEnvironment(&environment);
std::string16 error;
scoped_ptr<FileDialog> dialog(FileDialog::Create(environment.get()));
==== //depot/googleclient/gears/opensource/gears/desktop/file_dialog.cc#13 -
C:\src\0L/googleclient/gears/opensource/gears/desktop/file_dialog.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/file_dialog.cc 2008-10-09
16:17:38.000000000 -0700
+++ googleclient/gears/opensource/gears/desktop/file_dialog.cc 2008-10-09
17:40:11.000000000 -0700
@@ -31,6 +31,7 @@
#include "gears/base/common/string_utils.h"
#include "gears/blob/blob.h"
#include "gears/blob/file_blob.h"
+#include "gears/ui/common/i18n_strings.h"
#if defined(WIN32)
#include "gears/desktop/file_dialog_win32.h"
@@ -240,8 +241,9 @@
return true;
}
-bool FileDialog::ParseOptions(JsCallContext* context, const JsObject& map,
- Options* options) {
+bool FileDialog::ParseOptions(JsCallContext* context,
+ const ModuleEnvironment& module_environment,
+ const JsObject& map, Options* options) {
// options.filter = [ ".txt", "text/html", "text/*" ];
if (map.GetPropertyType(kFilter) != JSPARAM_UNDEFINED) {
bool success = true;
@@ -274,6 +276,7 @@
return false;
}
}
+
// options.singleFile = true;
if (map.GetPropertyType(kSingleFile) != JSPARAM_UNDEFINED) {
bool singleFile = false;
@@ -286,6 +289,18 @@
}
options->mode = singleFile ? SINGLE_FILE : MULTIPLE_FILES;
}
+
+ // Also fill out a dialog_title field. It's not a caller-defined option,
+ // but this the most convenient place to set it.
+ //
+ // We show only the host. The scheme and port are less interesting for
+ // end users making decisions about what access to allow, and they clutter
+ // the important part (the host).
+ options->dialog_title = GetLocalString(OPEN_FILES_STRING);
+ options->dialog_title += STRING16(L" (");
+ options->dialog_title += module_environment.security_origin_.host();
+ options->dialog_title += STRING16(L")");
+
return true;
}
==== //depot/googleclient/gears/opensource/gears/desktop/file_dialog.h#7 -
C:\src\0L/googleclient/gears/opensource/gears/desktop/file_dialog.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/file_dialog.h 2008-10-09
16:17:38.000000000 -0700
+++ googleclient/gears/opensource/gears/desktop/file_dialog.h 2008-10-09
17:35:45.000000000 -0700
@@ -33,6 +33,7 @@
#include "gears/base/common/js_types.h"
#include "gears/base/common/mutex.h"
#include "gears/base/common/scoped_refptr.h"
+#include "gears/base/common/string16.h"
#include "gears/ui/common/window_utils.h"
#include "third_party/scoped_ptr/scoped_ptr.h"
@@ -54,6 +55,7 @@
Options() : mode(MULTIPLE_FILES) { }
Mode mode;
+ std::string16 dialog_title;
// filter is a vector of Internet Media Types (eg, text/plain) and
// filename extensions (eg, .txt). If non-empty, the file dialog will
// filter the selectable files by this criteria.
@@ -81,8 +83,9 @@
// Prematurely terminates the dialog selection.
void Cancel();
- static bool ParseOptions(JsCallContext* context, const JsObject& map,
- Options* options);
+ static bool ParseOptions(JsCallContext* context,
+ const ModuleEnvironment& module_environment,
+ const JsObject& map, Options* options);
// Creates an array of javascript objects from files.
// Each javascript object has the following properties.
==== //depot/googleclient/gears/opensource/gears/desktop/file_dialog_gtk.cc#8 -
C:\src\0L/googleclient/gears/opensource/gears/desktop/file_dialog_gtk.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/file_dialog_gtk.cc
2008-10-09 15:22:51.000000000 -0700
+++ googleclient/gears/opensource/gears/desktop/file_dialog_gtk.cc
2008-10-09 16:22:13.000000000 -0700
@@ -103,7 +103,7 @@
const FileDialog::Options& options,
std::string16* error) {
dialog_.reset(gtk_file_chooser_dialog_new(
- String16ToUTF8(GetLocalString(SK_OpenFile)).c_str(), parent,
+ String16ToUTF8(options.dialog_title).c_str(), parent,
GTK_FILE_CHOOSER_ACTION_OPEN,
GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
GTK_STOCK_OPEN, GTK_RESPONSE_ACCEPT,
@@ -129,8 +129,9 @@
continue;
if (!gtk_filter) {
gtk_filter = gtk_file_filter_new();
- gtk_file_filter_set_name(gtk_filter,
- String16ToUTF8(GetLocalString(SK_AllReadableDocuments)).c_str());
+ gtk_file_filter_set_name(gtk_filter,
+ String16ToUTF8(GetLocalString(
+ RECOMMENDED_FILE_TYPES_STRING)).c_str());
}
if ('.' == filter_item[0]) {
std::string pattern("*");
@@ -148,7 +149,7 @@
// Always include an unrestricted filter that the user may select.
gtk_filter = gtk_file_filter_new();
gtk_file_filter_set_name(gtk_filter,
- String16ToUTF8(GetLocalString(SK_AllDocuments)).c_str());
+ String16ToUTF8(GetLocalString(ALL_FILE_TYPES_STRING)).c_str());
gtk_file_filter_add_custom(gtk_filter, static_cast<GtkFileFilterFlags>(0),
&AnyFileFilter, NULL, &AnyFileFilterDestroy);
gtk_file_chooser_add_filter(GTK_FILE_CHOOSER(dialog_.get()), gtk_filter);
==== //depot/googleclient/gears/opensource/gears/desktop/file_dialog_osx.cc#10
- C:\src\0L/googleclient/gears/opensource/gears/desktop/file_dialog_osx.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/file_dialog_osx.cc
2008-10-09 15:22:52.000000000 -0700
+++ googleclient/gears/opensource/gears/desktop/file_dialog_osx.cc
2008-10-09 16:22:45.000000000 -0700
@@ -180,6 +180,8 @@
*error = STRING16(L"Failed to create dialog options.");
return false;
}
+ dialog_options.windowTitle = CFStringCreateWithString16(
+ options.dialog_title.c_str());
if (parent) {
dialog_options.parentWindow = parent;
dialog_options.modality = kWindowModalityWindowModal;
@@ -243,9 +245,9 @@
// Add entries to the filter drop-down. The first entry displays any file
// that conforms to a UTI in utis_. The second entry displays all files.
scoped_CFString default_label(CFStringCreateWithString16(
- GetLocalString(SK_AllReadableDocuments).c_str()));
+ GetLocalString(RECOMMENDED_FILE_TYPES_STRING).c_str()));
scoped_CFString all_label(CFStringCreateWithString16(
- GetLocalString(SK_AllDocuments).c_str()));
+ GetLocalString(ALL_FILE_TYPES_STRING).c_str()));
const void* kFilterNames[] = {
default_label.get(),
all_label.get()
==== //depot/googleclient/gears/opensource/gears/desktop/file_dialog_win32.cc#9
- C:\src\0L/googleclient/gears/opensource/gears/desktop/file_dialog_win32.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/file_dialog_win32.cc
2008-10-09 15:22:53.000000000 -0700
+++ googleclient/gears/opensource/gears/desktop/file_dialog_win32.cc
2008-10-09 16:21:13.000000000 -0700
@@ -166,12 +166,14 @@
// Initialize an open file dialog to open multiple files.
void FileDialogWin32::InitDialog(NativeWindowPtr parent,
const FileDialog::Options& options) {
+ dialog_title_ = options.dialog_title;
filename_buffer_.resize(kFilenameBufferSize);
// Initialize OPENFILENAME
memset(&ofn_, 0, sizeof(ofn_));
ofn_.lStructSize = sizeof(ofn_);
ofn_.hwndOwner = parent;
+ ofn_.lpstrTitle = dialog_title_.c_str();
ofn_.lpstrFile = &filename_buffer_[0];
ofn_.lpstrFile[0] = '\0';
ofn_.nMaxFile = kFilenameBufferSize;
@@ -243,7 +245,8 @@
if (default_filter.empty())
return true;
- filter_.append(GetLocalString(SK_AllReadableDocuments));
+ filter_.clear();
+ filter_.append(GetLocalString(RECOMMENDED_FILE_TYPES_STRING));
filter_.push_back('\0');
// Append everything but the first character, which is always ';'.
filter_.insert(filter_.end(), default_filter.begin() + 1,
@@ -252,7 +255,7 @@
// Always include an unrestricted filter that the user may select.
// On Win32, *.* matches everything, even files with no extension.
- filter_.append(GetLocalString(SK_AllDocuments));
+ filter_.append(GetLocalString(ALL_FILE_TYPES_STRING));
filter_.push_back('\0');
filter_.append(STRING16(L"*.*"));
filter_.push_back('\0');
==== //depot/googleclient/gears/opensource/gears/desktop/file_dialog_win32.h#7
- C:\src\0L/googleclient/gears/opensource/gears/desktop/file_dialog_win32.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/file_dialog_win32.h
2008-10-09 16:17:38.000000000 -0700
+++ googleclient/gears/opensource/gears/desktop/file_dialog_win32.h
2008-10-09 15:29:42.000000000 -0700
@@ -79,6 +79,8 @@
OPENFILENAME ofn_;
// Provides the backing memory for ofn_.lpstrFilter.
std::string16 filter_;
+ // Provides the backing memory for ofn_.lpstrTitle.
+ std::string16 dialog_title_;
// Provides the backing memory for ofn_.lpstrFile.
std::vector<TCHAR> filename_buffer_;
// MessageService topic for communication from worker to main thread.
==== //depot/googleclient/gears/opensource/gears/ui/common/i18n_strings.cc#1 -
C:\src\0L/googleclient/gears/opensource/gears/ui/common/i18n_strings.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/i18n_strings.cc
2008-10-09 15:22:53.000000000 -0700
+++ googleclient/gears/opensource/gears/ui/common/i18n_strings.cc
2008-10-09 16:20:41.000000000 -0700
@@ -28,9 +28,9 @@
const char16* const kLocalStrings[] = {
// TODO(zork): Grab these strings from i18n_string_db.cc.stab
- STRING16(L"All Readable Documents"),
- STRING16(L"All Documents"),
- STRING16(L"Open File"),
+ STRING16(L"Open Files"), // used in dialog title bar
+ STRING16(L"Recommended File Types"), // filetype filter
+ STRING16(L"All Files"), // filetype filter
};
std::string16 GetLocalString(I18NStringKey key) {
==== //depot/googleclient/gears/opensource/gears/ui/common/i18n_strings.h#1 -
C:\src\0L/googleclient/gears/opensource/gears/ui/common/i18n_strings.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/i18n_strings.h
2008-10-09 15:22:53.000000000 -0700
+++ googleclient/gears/opensource/gears/ui/common/i18n_strings.h
2008-10-09 16:20:06.000000000 -0700
@@ -29,9 +29,9 @@
#include "gears/base/common/string16.h"
enum I18NStringKey {
- SK_AllReadableDocuments,
- SK_AllDocuments,
- SK_OpenFile,
+ OPEN_FILES_STRING,
+ RECOMMENDED_FILE_TYPES_STRING,
+ ALL_FILE_TYPES_STRING,
};
// Returns the localized string identified by key.
