A cockroach-sized code review (wangxianzhu localrev 1276)

[phni...@gmail.com]

Hello james.su,

I'd like you to do a code review. Please review the following patch:

----------------------------------------------------------------------
r1276: (no author) | 2009-11-20 12:42:25 +0800

Add support of customized net error action for mozgtk browser element.
----------------------------------------------------------------------

=== extensions/gtkmoz_browser_element/browser_element.cc
==================================================================
--- extensions/gtkmoz_browser_element/browser_element.cc (revision 1275)
+++ extensions/gtkmoz_browser_element/browser_element.cc (revision 1276)
@@ -1089,6 +1089,13 @@
} else {
result = OpenURL(params[2]) ? '1' : '0';
}
+ } else if (strcmp(type, kNetErrorFeedback) == 0) {
+ if (param_count != 3) {
+ LOG("%s feedback needs 3 parameters, but only %zu is given",
+ kNetErrorFeedback, param_count);
+ } else {
+ result = onerror_signal_(params[2]) ? '1' : '0';
+ }
} else {
LOG("Unknown feedback: %s", type);
}
@@ -1128,6 +1135,7 @@
Connection *minimized_connection_, *restored_connection_,
*popout_connection_, *popin_connection_,
*dock_connection_, *undock_connection_;
+ Signal1<bool, const char *> onerror_signal_;
};

BrowserController *BrowserController::instance_ = NULL;
@@ -1164,14 +1172,9 @@
}
}

-class BrowserElement::Impl : public BrowserElementImpl {
- public:
- Impl(BrowserElement *owner) : BrowserElementImpl(owner) { }
-};
-
BrowserElement::BrowserElement(View *view, const char *name)
: BasicElement(view, "browser", name, true),
- impl_(new Impl(this)) {
+ impl_(new BrowserElementImpl(this)) {
SetEnabled(true);
}

@@ -1187,6 +1190,8 @@
RegisterProperty("alwaysOpenNewWindow",
NewSlot(&BrowserElement::IsAlwaysOpenNewWindow),
NewSlot(&BrowserElement::SetAlwaysOpenNewWindow));
+ RegisterClassSignal("onerror", &BrowserElementImpl::onerror_signal_,
+ &BrowserElement::impl_);
}

BrowserElement::~BrowserElement() {
=== extensions/gtkmoz_browser_element/browser_element.h
==================================================================
--- extensions/gtkmoz_browser_element/browser_element.h (revision 1275)
+++ extensions/gtkmoz_browser_element/browser_element.h (revision 1276)
@@ -23,6 +23,8 @@
namespace ggadget {
namespace gtkmoz {

+class BrowserElementImpl;
+
class BrowserElement : public BasicElement {
public:
DEFINE_CLASS_ID(0xa4fae95864ae4d89, BasicElement);
@@ -69,8 +71,7 @@
private:
DISALLOW_EVIL_CONSTRUCTORS(BrowserElement);

- class Impl;
- Impl *impl_;
+ BrowserElementImpl *impl_;
};

} // namespace gtkmoz
=== extensions/gtkmoz_browser_element/browser_child.cc
==================================================================
--- extensions/gtkmoz_browser_element/browser_child.cc (revision 1275)
+++ extensions/gtkmoz_browser_element/browser_child.cc (revision 1276)
@@ -559,6 +559,9 @@
window = do_QueryInterface(view);
}

+ nsCOMPtr<nsIDOMWindow> top_window;
+ window->GetTop(getter_AddRefs(top_window));
+
*is_loading = PR_FALSE;
for (BrowserMap::iterator it = g_browsers.begin(); it != g_browsers.end();
++it) {
@@ -567,7 +570,7 @@
nsCOMPtr<nsIDOMWindow> window1;
rv = browser->GetContentDOMWindow(getter_AddRefs(window1));
NS_ENSURE_SUCCESS(rv, NULL);
- if (window == window1) {
+ if (window == window1 || top_window == window1) {
nsCOMPtr<nsIWebProgress> progress(do_GetInterface(browser));
if (progress)
progress->GetIsLoadingDocument(is_loading);
@@ -611,10 +614,21 @@
PRBool is_loading = PR_FALSE;
BrowserInfo *browser_info = FindBrowserByContentPolicyContext(
context, &is_loading);
- if (!browser_info || !browser_info->always_open_new_window)
+ if (!browser_info) {
return NS_OK;
+ }

- if (content_type == TYPE_DOCUMENT || content_type == TYPE_SUBDOCUMENT) {
+ if (strncmp(url_spec.get(), "about:neterror", 14) == 0) {
+ std::string r = SendFeedback(browser_info->browser_id,
+ kNetErrorFeedback, url_spec.get(),
+ NULL);
+ if (r[0] != '0')
+ *retval = REJECT_OTHER;
+ return NS_OK;
+ }
+
+ if (!browser_info->always_open_new_window &&
+ (content_type == TYPE_DOCUMENT || content_type == TYPE_SUBDOCUMENT)) {
if (request_origin)
request_origin->GetSpec(origin_spec);

=== extensions/gtkmoz_browser_element/browser_child.h
==================================================================
--- extensions/gtkmoz_browser_element/browser_child.h (revision 1275)
+++ extensions/gtkmoz_browser_element/browser_child.h (revision 1276)
@@ -247,7 +247,7 @@
* The browser child tells the controller that the browser is about to open
* an URL.
*
- * Messsage format:
+ * Message format:
* <code>
* OPEN\n
* Browser ID (size_t)\n
@@ -255,11 +255,31 @@
* """EOM"""\n
* </code>
*
- * The controller must immediately reply a message containing only kReplyPrefix.
+ * The controller must immediately reply a message containing kReplyPrefix and
+ * the return value "1" or "0" indicating if the URL has been/will be opened or
+ * not opened by the controller respectively.
*/
const char kOpenURLFeedback[] = "OPEN";

/**
+ * The browser child tells the controller that the browser has encountered a
+ * network error.
+ *
+ * Message format:
+ * <code>
+ * NETERR\n
+ * Browser ID (size_t)\n
+ * Error URL (normal string, about:neterror?...)\n
+ * """EOM"""\n
+ *
+ * The controller must immediately reply a message containing kReplyPrefix and
+ * the return value "1" or "0" indicating if the error has been/will be handled
+ * or not handled by the controller respectively.
+ * </code>
+ */
+const char kNetErrorFeedback[] = "ERR";
+
+/**
* The browser child periodically pings the controller to check if the
* controller died.
*
=== gadgets/igoogle/main.js
==================================================================
--- gadgets/igoogle/main.js (revision 1275)
+++ gadgets/igoogle/main.js (revision 1276)
@@ -47,6 +47,7 @@
}
};

+ browser.onerror = OnBrowserError;
plugin.onAddCustomMenuItems = OnAddCustomMenuItems;
SetUpCommonParams();
LoadGadget();
@@ -62,6 +63,14 @@
menu.AddItem(strings.GADGET_UNLOAD, unload_flags, UnloadMenuHandler);
}

+function OnBrowserError(url) {
+ alert(url);
+ view.setTimeout(function() {
+ DisplayMessage(strings.GADGET_ERROR);
+ }, 100);
+ return true;
+}
+
function RefreshMenuHandler(item_text) {
RefreshGadget();
}

This is a semiautomated message from "svkmail". Complaints or suggestions?
Mail edy...@gmail.com.

[James Su]

LGTM.