Hello andreip.mpcomplete,
I'd like you to do a code review. Please execute
g4 diff -c 8986863
or point your web browser to
http://mondrian/8986863
to review the following code:
Change 8986863 by [EMAIL PROTECTED] on 2008/11/13 11:02:30 *pending*
Switches 'void *platform_data' parameter of HtmlDialog constructor and
SettingsDialog::Run to 'BrowsingContext *browsing_context'. This parameter is
currently only used by Chrome, which passes BrowsingContext*. Also added a
corresponding BrowsingContext* parameter to PermissionsDialog::Prompt, as this
will be required by Opera.
Note that browser-specific users of BrowsingContext (eg HtmlDialog)
still need to cast the BrowsingContext* pointer to the browser-specific
implementation class in order to use the object. We can add virtual methods to
BrowsingContext to avoid this once the design of BrowsingContext has been
finalised.
R=andreip.mpcomplete
[EMAIL PROTECTED]
DELTA=30 (9 added, 3 deleted, 18 changed)
OCL=8986863
Affected files ...
...
//depot/googleclient/gears/opensource/gears/base/common/permissions_manager.cc#2
edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#56 edit
... //depot/googleclient/gears/opensource/gears/ui/chrome/html_dialog_cr.cc#1
edit
... //depot/googleclient/gears/opensource/gears/ui/common/alert_dialog.cc#3 edit
... //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.h#7 edit
...
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc#9
edit
...
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.h#2
edit
... //depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.cc#11
edit
... //depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.h#5
edit
30 delta lines: 9 added, 3 deleted, 18 changed
Also consider running:
g4 lint -c 8986863
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 8986863 by [EMAIL PROTECTED] on 2008/11/13 11:02:30 *pending*
Switches 'void *platform_data' parameter of HtmlDialog constructor and
SettingsDialog::Run to 'BrowsingContext *browsing_context'. This parameter is
currently only used by Chrome, which passes BrowsingContext*. Also added a
corresponding BrowsingContext* parameter to PermissionsDialog::Prompt, as this
will be required by Opera.
Note that browser-specific users of BrowsingContext (eg HtmlDialog)
still need to cast the BrowsingContext* pointer to the browser-specific
implementation class in order to use the object. We can add virtual methods to
BrowsingContext to avoid this once the design of BrowsingContext has been
finalised.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/base/common/permissions_manager.cc#2
edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#56 edit
... //depot/googleclient/gears/opensource/gears/ui/chrome/html_dialog_cr.cc#1
edit
... //depot/googleclient/gears/opensource/gears/ui/common/alert_dialog.cc#3 edit
... //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.h#7 edit
...
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc#9
edit
...
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.h#2
edit
... //depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.cc#11
edit
... //depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.h#5
edit
====
//depot/googleclient/gears/opensource/gears/base/common/permissions_manager.cc#2
-
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/base/common/permissions_manager.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/permissions_manager.cc
2008-11-10 14:29:57.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/permissions_manager.cc
2008-11-13 11:07:22.000000000 +0000
@@ -50,7 +50,8 @@
// Could not find a prior decision. Ask the user.
permission_state_[type] = PermissionsDialog::Prompt(security_origin_,
type,
- custom);
+ custom,
+ NULL);
}
// Return the boolean decision.
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#56 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/desktop/desktop.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.cc 2008-11-11
11:16:49.000000000 +0000
+++ googleclient/gears/opensource/gears/desktop/desktop.cc 2008-11-13
11:04:45.000000000 +0000
@@ -362,7 +362,7 @@
const int kShortcutsDialogWidth = 360;
const int kShortcutsDialogHeight = 320;
- HtmlDialog shortcuts_dialog;
+ HtmlDialog shortcuts_dialog(NULL);
if (desktop.InitializeDialog(&shortcut_info, &shortcuts_dialog,
Desktop::DIALOG_STYLE_STANDARD)) {
HtmlDialogReturnValue dialog_result = shortcuts_dialog.DoModal(
==== //depot/googleclient/gears/opensource/gears/ui/chrome/html_dialog_cr.cc#1
-
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/ui/chrome/html_dialog_cr.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/chrome/html_dialog_cr.cc
2008-10-08 09:50:53.000000000 +0100
+++ googleclient/gears/opensource/gears/ui/chrome/html_dialog_cr.cc
2008-11-13 11:19:31.000000000 +0000
@@ -77,16 +77,17 @@
if (!String16ToUTF8(arguments_string, &arguments_utf8))
return false;
- scoped_refptr<CRBrowsingContext> browsing_context(
- static_cast<CRBrowsingContext*>(platform_data_));
- platform_data_ = NULL; // browsing_context may be deleted at end of scope
+ scoped_refptr<CRBrowsingContext> scoped_browsing_context(
+ static_cast<CRBrowsingContext*>(browsing_context_));
+ // scoped_browsing_context may be deleted at end of scope
+ browsing_context_ = NULL;
// Note: we pass a callback object as the plugin_context. When the dialog
// closes, we get a notification with a pointer to that plugin_context, which
// we use to handle the results.
void *plugin_context = new HtmlDialogCallback(this, callback, closure);
bool rv = g_cpbrowser_funcs.show_html_dialog(
- g_cpid, browsing_context->context, url_utf8.c_str(), width, height,
+ g_cpid, scoped_browsing_context->context, url_utf8.c_str(), width,
height,
arguments_utf8.c_str(), plugin_context) == CPERR_SUCCESS;
return rv;
}
==== //depot/googleclient/gears/opensource/gears/ui/common/alert_dialog.cc#3 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/ui/common/alert_dialog.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/alert_dialog.cc
2008-11-13 12:18:04.000000000 +0000
+++ googleclient/gears/opensource/gears/ui/common/alert_dialog.cc
2008-11-13 11:42:16.000000000 +0000
@@ -52,7 +52,7 @@
// static
void AlertDialog::ShowModal(AlertMessageId id) {
- HtmlDialog dialog;
+ HtmlDialog dialog(NULL);
AlertBeep();
dialog.arguments["messageId"] = Json::Value(GetMessageIdString(id));
dialog.DoModal(STRING16(L"alert_dialog.html"), kDialogWidth, kDialogHeight);
==== //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.h#7 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/ui/common/html_dialog.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/html_dialog.h 2008-11-13
12:18:04.000000000 +0000
+++ googleclient/gears/opensource/gears/ui/common/html_dialog.h 2008-11-13
11:04:21.000000000 +0000
@@ -29,6 +29,8 @@
#include "gears/base/common/string16.h"
#include "third_party/jsoncpp/json.h"
+class BrowsingContext;
+
// Return values for showing dialogs. If the dialog returns SUPRESSED, it means
// that the SupressDialog preference in PermissionsDB is enabled for automated
// testing.
@@ -55,12 +57,10 @@
void *closure);
public:
// Constructor.
- HtmlDialog()
- : arguments(Json::objectValue), result(Json::nullValue),
- platform_data_(NULL) {}
- HtmlDialog(void *platform_data)
- : arguments(Json::objectValue), result(Json::nullValue),
- platform_data_(platform_data) {}
+ HtmlDialog(BrowsingContext *browsing_context)
+ : arguments(Json::objectValue),
+ result(Json::nullValue),
+ browsing_context_(browsing_context) {}
// Open the dialog.
HtmlDialogReturnValue DoModal(const char16 *html_filename, int width,
@@ -94,8 +94,7 @@
// GetLocale() is in the browser-specific HtmlDialog code.
bool GetLocale(std::string16 *locale);
- // Platform-specific data.
- void *platform_data_;
+ BrowsingContext *browsing_context_;
};
#endif // GEARS_UI_COMMON_HTML_DIALOG_H__
====
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc#9 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc
2008-11-10 14:29:59.000000000 +0000
+++ googleclient/gears/opensource/gears/ui/common/permissions_dialog.cc
2008-11-13 11:08:21.000000000 +0000
@@ -52,11 +52,12 @@
PermissionState PermissionsDialog::Prompt(
const SecurityOrigin &origin,
PermissionsDB::PermissionType type,
- const CustomContent *custom_content) {
+ const CustomContent *custom_content,
+ BrowsingContext *browsing_context) {
// Note: Arguments and results are coupled to the values that
// permissions_dialog.html.m4 is expecting.
const char16* dialog_type = DialogTypeForPermission(type);
- HtmlDialog dialog;
+ HtmlDialog dialog(browsing_context);
const char16 *custom_icon = NULL;
const char16 *custom_name = NULL;
const char16 *custom_message = NULL;
====
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.h#2 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/ui/common/permissions_dialog.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/permissions_dialog.h
2008-11-10 14:29:59.000000000 +0000
+++ googleclient/gears/opensource/gears/ui/common/permissions_dialog.h
2008-11-13 11:06:06.000000000 +0000
@@ -28,6 +28,7 @@
#include "gears/base/common/security_model.h"
+class BrowsingContext;
enum PermissionState;
class JsCallContext;
@@ -58,7 +59,8 @@
// message will be shown.
static PermissionState Prompt(const SecurityOrigin &origin,
PermissionsDB::PermissionType type,
- const CustomContent *custom_content);
+ const CustomContent *custom_content,
+ BrowsingContext *browsing_context);
private:
// Private constructor. Use static Prompt() methods instead.
PermissionsDialog() {}
====
//depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.cc#11 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/ui/common/settings_dialog.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/settings_dialog.cc
2008-11-13 12:18:04.000000000 +0000
+++ googleclient/gears/opensource/gears/ui/common/settings_dialog.cc
2008-11-13 10:59:07.000000000 +0000
@@ -54,8 +54,8 @@
}
#endif
-void SettingsDialog::Run(void *platform_data) {
- scoped_ptr<HtmlDialog> settings_dialog(new HtmlDialog(platform_data));
+void SettingsDialog::Run(BrowsingContext *browsing_context) {
+ scoped_ptr<HtmlDialog> settings_dialog(new HtmlDialog(browsing_context));
// Populate the arguments property.
settings_dialog->arguments[kPermissionsString] =
==== //depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.h#5
-
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/ui/common/settings_dialog.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/settings_dialog.h
2008-11-13 12:18:04.000000000 +0000
+++ googleclient/gears/opensource/gears/ui/common/settings_dialog.h
2008-11-13 10:58:23.000000000 +0000
@@ -29,12 +29,14 @@
#include "gears/base/common/permissions_db.h"
#include "third_party/jsoncpp/json.h"
+class BrowsingContext;
+
// For now this is very simple and only gives a way to revoke decisions made
// with the "remember me" checkbox on the capabilities prompt.
class SettingsDialog {
public:
// Show the settings dialog and apply any changes the user makes.
- static void Run(void *platform_data);
+ static void Run(BrowsingContext *browsing_context);
static bool IsVisible() { return visible_; }
