ChromeOS: bypass proxy for captive portal authorization.

chrome/browser/chromeos/net/network_portal_notification_controller.cc

Index: chrome/browser/chromeos/net/network_portal_notification_controller.cc
diff --git a/chrome/browser/chromeos/net/network_portal_notification_controller.cc b/chrome/browser/chromeos/net/network_portal_notification_controller.cc
index 17ad3559ba1da68cc749bb1557051dd739856ecd..ef15955036cf280ef73f6994e57663913fcf56a4 100644
--- a/chrome/browser/chromeos/net/network_portal_notification_controller.cc
+++ b/chrome/browser/chromeos/net/network_portal_notification_controller.cc
@@ -16,7 +16,10 @@
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/mobile/mobile_activator.h"
+#include "chrome/browser/chromeos/net/network_portal_web_dialog.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/grit/generated_resources.h"
@@ -51,7 +54,9 @@ void CloseNotification() {
 class NetworkPortalNotificationControllerDelegate
     : public message_center::NotificationDelegate {
  public:
-  NetworkPortalNotificationControllerDelegate(): clicked_(false) {}
+  NetworkPortalNotificationControllerDelegate(
+      NetworkPortalNotificationController* controller)
+      : clicked_(false), controller_(controller->AsWeakPtr()) {}
 
   // Overridden from message_center::NotificationDelegate:
   virtual void Display() override;
@@ -59,10 +64,10 @@ class NetworkPortalNotificationControllerDelegate
   virtual void Click() override;
 
  private:
-  virtual ~NetworkPortalNotificationControllerDelegate() {}
-
   bool clicked_;
 
+  base::WeakPtr<NetworkPortalNotificationController> controller_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkPortalNotificationControllerDelegate);
 };
 
@@ -93,13 +98,19 @@ void NetworkPortalNotificationControllerDelegate::Click() {
       NetworkPortalNotificationController::USER_ACTION_METRIC_CLICKED,
       NetworkPortalNotificationController::USER_ACTION_METRIC_COUNT);
 
-  Profile* profile = ProfileManager::GetActiveUserProfile();
-  if (!profile)
-    return;
-  chrome::ScopedTabbedBrowserDisplayer displayer(profile,
-                                                 chrome::HOST_DESKTOP_TYPE_ASH);
-  GURL url(captive_portal::CaptivePortalDetector::kDefaultURL);
-  chrome::ShowSingletonTab(displayer.browser(), url);
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kEnableCaptivePortalBypassProxy)) {
+    if (controller_)
+      controller_->ShowDialog();

[ygorshenin1, 2014/11/24 15:02:07]

I'm proposing to add a return statement at the end to the first branch and get
rid of the else clause.

[Alexander Alekseev, 2014/11/26 00:26:28]

Done.

[ygorshenin1, 2014/11/26 16:33:50]

Sorry, I missed CloseNotification() call. Could you please restore your code for
the function back?

+  } else {
+    Profile* profile = ProfileManager::GetActiveUserProfile();
+    if (!profile)
+      return;
+    chrome::ScopedTabbedBrowserDisplayer displayer(
+        profile, chrome::HOST_DESKTOP_TYPE_ASH);
+    GURL url(captive_portal::CaptivePortalDetector::kDefaultURL);
+    chrome::ShowSingletonTab(displayer.browser(), url);
+  }
 
   CloseNotification();
 }
@@ -118,7 +129,9 @@ const char NetworkPortalNotificationController::kNotificationMetric[] =
 const char NetworkPortalNotificationController::kUserActionMetric[] =
     "CaptivePortal.Notification.UserAction";
 
-NetworkPortalNotificationController::NetworkPortalNotificationController() {}
+NetworkPortalNotificationController::NetworkPortalNotificationController()
+    : dialog_(NULL) {

[ygorshenin1, 2014/11/24 15:02:07]

nit: s/NULL/nullptr/g

[Alexander Alekseev, 2014/11/26 00:26:28]

Done.

+}
 
 NetworkPortalNotificationController::~NetworkPortalNotificationController() {}
 
@@ -131,6 +144,10 @@ void NetworkPortalNotificationController::OnPortalDetectionCompleted(
   if (!network ||
       state.status != NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL) {
     last_network_path_.clear();
+
+    if (dialog_)
+      dialog_->Close();
+
     CloseNotification();
     return;
   }
@@ -152,16 +169,13 @@ void NetworkPortalNotificationController::OnPortalDetectionCompleted(
       ash::system_notifier::kNotifierNetworkPortalDetector);
 
   scoped_ptr<Notification> notification(new Notification(
-      message_center::NOTIFICATION_TYPE_SIMPLE,
-      kNotificationId,
+      message_center::NOTIFICATION_TYPE_SIMPLE, kNotificationId,
       l10n_util::GetStringUTF16(IDS_PORTAL_DETECTION_NOTIFICATION_TITLE),
       l10n_util::GetStringFUTF16(IDS_PORTAL_DETECTION_NOTIFICATION_MESSAGE,
                                  base::UTF8ToUTF16(network->name())),
-      icon,
-      base::string16() /* display_source */,
-      notifier_id,
+      icon, base::string16() /* display_source */, notifier_id,
       message_center::RichNotificationData(),
-      new NetworkPortalNotificationControllerDelegate()));
+      new NetworkPortalNotificationControllerDelegate(this)));
   notification->SetSystemPriority();
 
   if (ash::Shell::HasInstance()) {
@@ -173,4 +187,21 @@ void NetworkPortalNotificationController::OnPortalDetectionCompleted(
   message_center::MessageCenter::Get()->AddNotification(notification.Pass());
 }
 
+void NetworkPortalNotificationController::ShowDialog() {
+  if (!dialog_) {

[ygorshenin1, 2014/11/24 15:02:07]

nit: I'm proposing to change the code in the following way:
if (dialog_)
  return;
...

[Alexander Alekseev, 2014/11/26 00:26:28]

Done.

+    Profile* signin_profile = ProfileHelper::GetSigninProfile();
+    dialog_ = new NetworkPortalWebDialog(this);
+    dialog_->SetWidget(views::Widget::GetWidgetForNativeWindow(
+        chrome::ShowWebDialog(NULL, signin_profile, dialog_)));
+  }
+}
+
+void NetworkPortalNotificationController::OnDialogDestroyed(
+    const NetworkPortalWebDialog* dialog) {
+  if (dialog == dialog_) {

[ygorshenin1, 2014/11/24 15:02:08]

When dialog_ is not equal to dialog? Could you please describe such situations?
If dialog_ always equals to dialog, could you please add a CHECK_EQ at the
beginning of the method?

[Alexander Alekseev, 2014/11/26 00:26:28]

Dialog close and destroy are asynchronous. So it is possible for the first
dialog instance to be already closed, but not yet destroyed, when second one is
already instantiated.

+    dialog_ = NULL;
+    ProfileHelper::Get()->ClearSigninProfile(base::Closure());
+  }
+}
+
 }  // namespace chromeos