Move Android to use new Popup Blocker API.

chrome/browser/android/chrome_web_contents_delegate_android.cc

Index: chrome/browser/android/chrome_web_contents_delegate_android.cc
diff --git a/chrome/browser/android/chrome_web_contents_delegate_android.cc b/chrome/browser/android/chrome_web_contents_delegate_android.cc
index 0eb1157977c23a3530cc23ff026a91a460529b1a..9caa5ab3015fb7f547615488aa9347c0653bac95 100644
--- a/chrome/browser/android/chrome_web_contents_delegate_android.cc
+++ b/chrome/browser/android/chrome_web_contents_delegate_android.cc
@@ -5,18 +5,27 @@
 #include "chrome/browser/android/chrome_web_contents_delegate_android.h"
 
 #include "base/android/jni_android.h"
+#include "base/command_line.h"
+#include "chrome/browser/android/tab_android.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/file_select_helper.h"
 #include "chrome/browser/media/media_capture_devices_dispatcher.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.h"
+#include "chrome/browser/ui/blocked_content/blocked_content_tab_helper.h"
+#include "chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h"
+#include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/find_bar/find_notification_details.h"
 #include "chrome/browser/ui/find_bar/find_tab_helper.h"
+#include "chrome/common/chrome_switches.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/file_chooser_params.h"
 #include "jni/ChromeWebContentsDelegateAndroid_jni.h"
+#include "third_party/WebKit/public/web/WebWindowFeatures.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/rect_f.h"
 
@@ -217,6 +226,90 @@ bool ChromeWebContentsDelegateAndroid::RequestPpapiBrokerPermission(
 #endif
 }
 
+WebContents* ChromeWebContentsDelegateAndroid::OpenURLFromTab(
+    WebContents* source,
+    const content::OpenURLParams& params) {
+  WindowOpenDisposition disposition = params.disposition;
+  if (!source || (disposition != CURRENT_TAB &&

[Yaron, 2013/08/21 19:07:58]

Can source be NULL? Won't we just crash later?

[David Trainor- moved to gerrit, 2013/08/21 20:21:27]

The NULL check for source is done everywhere by browser, so I assumed it was a
valid case.

+                  disposition != NEW_FOREGROUND_TAB &&
+                  disposition != NEW_BACKGROUND_TAB &&
+                  disposition != OFF_THE_RECORD &&
+                  disposition != NEW_POPUP &&
+                  disposition != NEW_WINDOW)) {
+    // We can't handle this here.  Give the parent a chance.
+    return WebContentsDelegateAndroid::OpenURLFromTab(source, params);
+  }
+
+  Profile* profile = Profile::FromBrowserContext(source->GetBrowserContext());
+  chrome::NavigateParams nav_params(profile,
+                                    params.url,
+                                    params.transition);
+  FillNavigateParamsFromOpenURLParams(&nav_params, params);
+  nav_params.source_contents = source;
+  nav_params.window_action = chrome::NavigateParams::SHOW_WINDOW;
+  nav_params.user_gesture = params.user_gesture;
+
+  PopupBlockerTabHelper* popup_blocker_helper = NULL;
+  if (source)

[Yaron, 2013/08/21 19:07:58]

You've already de-refed source on L243.

[David Trainor- moved to gerrit, 2013/08/21 20:21:27]

Done.

+    popup_blocker_helper = PopupBlockerTabHelper::FromWebContents(source);
+
+  DCHECK(popup_blocker_helper);
+
+  if (popup_blocker_helper) {

[Yaron, 2013/08/21 19:07:58]

No need for if after DCHECK

[David Trainor- moved to gerrit, 2013/08/21 20:21:27]

Done.

[Ted C, 2013/08/21 21:06:15]

Really?  I thought you had to fail gracefully for DCHECK, but this will just
crash down below now.

[Yaron, 2013/08/22 00:36:30]

Nope. From
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
:
"Use DCHECK() or NOTREACHED() as assertions, e.g. to document pre- and
post-conditions. A DCHECK() means "this condition must always be true", not
"this condition is normally true, but perhaps not in exceptional cases."  A
consequence of this is that you should not handle DCHECK() failures, even if
failure would result in a crash. Attempting to handle a DCHECK() failure is a
statement that the DCHECK() can fail, which contradicts the point of writing the
DCHECK("

+    if ((params.disposition == NEW_POPUP ||
+        params.disposition == NEW_FOREGROUND_TAB ||
+        params.disposition == NEW_BACKGROUND_TAB ||
+        params.disposition == NEW_WINDOW) &&
+        !params.user_gesture &&
+        !CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kDisablePopupBlocking)) {
+      if (popup_blocker_helper->MaybeBlockPopup(nav_params,
+                                                WebKit::WebWindowFeatures())) {
+        return NULL;
+      }
+    }
+  }
+
+  return WebContentsDelegateAndroid::OpenURLFromTab(source, params);
+}
+
+void ChromeWebContentsDelegateAndroid::AddNewContents(

[jochen (gone - plz use gerrit), 2013/08/21 08:05:25]

this isn't really related to popup blocking, but for instant search

[David Trainor- moved to gerrit, 2013/08/21 17:27:03]

We were using this code path downstream for popups as well as handling things
like instant (even though we don't use it for that reason).  I just moved it up
here and removed it from the popup pipeline so that it's all upstream.

+    WebContents* source,
+    WebContents* new_contents,
+    WindowOpenDisposition disposition,
+    const gfx::Rect& initial_pos,
+    bool user_gesture,
+    bool* was_blocked) {
+  // No code for this yet.
+  DCHECK(disposition != SAVE_TO_DISK);

[Yaron, 2013/08/21 19:07:58]

DCHECK_NE

[David Trainor- moved to gerrit, 2013/08/21 20:21:27]

Done.

+  // Can't create a new contents for the current tab - invalid case.
+  DCHECK(disposition != CURRENT_TAB);

[Yaron, 2013/08/21 19:07:58]

DCHECK_NE

[David Trainor- moved to gerrit, 2013/08/21 20:21:27]

Done.

+
+  BlockedContentTabHelper* source_blocked_content = NULL;
+  if (source)

[Yaron, 2013/08/21 19:07:58]

Indentation off and this may be unnecessary?

[David Trainor- moved to gerrit, 2013/08/21 20:21:27]

The code this was built off of checks for source both here and in
OpenURLFromTab.  I think it's a valid case to have a NULL source web contents.

+  source_blocked_content = BlockedContentTabHelper::FromWebContents(source);
+
+  TabAndroid::InitTabHelpers(new_contents);
+
+  if (source_blocked_content) {
+    if (source_blocked_content->all_contents_blocked()) {
+      source_blocked_content->AddWebContents(
+          new_contents, disposition, initial_pos, user_gesture);
+      if (was_blocked)
+        *was_blocked = true;
+      return;
+    }
+
+    new_contents->GetRenderViewHost()->DisassociateFromPopupCount();
+  }
+
+  WebContentsDelegateAndroid::AddNewContents(source,
+                                             new_contents,
+                                             disposition,
+                                             initial_pos,
+                                             user_gesture,
+                                             was_blocked);
+}
 
 }  // namespace android
 }  // namespace chrome