Skip to content

Commit

Permalink
CrOS: Eliminate CaptivePortalWindowProxyDelegate
Browse files Browse the repository at this point in the history 
CaptivePortalWindowProxyDelegate is currently used to directly
call into NetworkStateInformer when the captive portal window
navigates away from the portal URL.

This is incorrect and will break when Shill portal detection
becomes primary. A redirect should trigger a new portal
detection cycle instead.

BUG=b:243405151
TEST=unit_tests, browser_tests, manual testing

Change-Id: I766ff82380cb9d483463b6b3ea055e75f650903c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866837
Reviewed-by: Roman Sorokin <rsorokin@google.com>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1042181}
  • Loading branch information
@stevenjb
stevenjb authored and Chromium LUCI CQ committed Sep 1, 2022
1 parent e6c3b47 commit f32987a
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 82 deletions.
4 changes: 2 additions & 2 deletions chrome/browser/ash/login/screens/error_screen.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ base::CallbackListSubscription ErrorScreen::RegisterConnectRequestCallback(
void ErrorScreen::MaybeInitCaptivePortalWindowProxy(
content::WebContents* web_contents) {
if (!captive_portal_window_proxy_.get()) {
captive_portal_window_proxy_ = std::make_unique<CaptivePortalWindowProxy>(
network_state_informer_.get(), web_contents);
captive_portal_window_proxy_ =
std::make_unique<CaptivePortalWindowProxy>(web_contents);
}
}

Expand Down
79 changes: 31 additions & 48 deletions chrome/browser/ash/login/ui/captive_portal_window_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,30 +27,9 @@
#include "chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chromeos/ash/components/dbus/shill/fake_shill_manager_client.h"
#include "chromeos/ash/components/network/portal_detector/network_portal_detector.h"
#include "content/public/test/browser_test.h"

namespace ash {
namespace {

// Stub implementation of CaptivePortalWindowProxyDelegate, does
// nothing and used to instantiate CaptivePortalWindowProxy.
class CaptivePortalWindowProxyStubDelegate
: public CaptivePortalWindowProxyDelegate {
public:
CaptivePortalWindowProxyStubDelegate() : num_portal_notifications_(0) {}

~CaptivePortalWindowProxyStubDelegate() override {}

void OnPortalDetected() override { ++num_portal_notifications_; }

int num_portal_notifications() const { return num_portal_notifications_; }

private:
int num_portal_notifications_;
};

} // namespace

class CaptivePortalWindowTest : public InProcessBrowserTest {
protected:
Expand All @@ -66,11 +45,12 @@ class CaptivePortalWindowTest : public InProcessBrowserTest {
captive_portal_window_proxy_->OnOriginalURLLoaded();
}

void CheckState(bool is_shown, int num_portal_notifications) {
void CheckState(bool is_shown, bool in_progress) {
bool actual_is_shown = (CaptivePortalWindowProxy::STATE_DISPLAYED ==
captive_portal_window_proxy_->GetState());
ASSERT_EQ(is_shown, actual_is_shown);
ASSERT_EQ(num_portal_notifications, delegate_.num_portal_notifications());
ASSERT_EQ(in_progress,
network_portal_detector_->portal_detection_in_progress());
}

void SetUpCommandLine(base::CommandLine* command_line) override {
Expand All @@ -83,99 +63,102 @@ class CaptivePortalWindowTest : public InProcessBrowserTest {
content::WebContents* web_contents =
LoginDisplayHost::default_host()->GetOobeWebContents();
captive_portal_window_proxy_ =
std::make_unique<CaptivePortalWindowProxy>(&delegate_, web_contents);
std::make_unique<CaptivePortalWindowProxy>(web_contents);
}

void TearDownOnMainThread() override {
captive_portal_window_proxy_.reset();
void SetUpInProcessBrowserTestFixture() override {
network_portal_detector_ = new NetworkPortalDetectorTestImpl();
network_portal_detector::InitializeForTesting(network_portal_detector_);
}

void TearDownOnMainThread() override { captive_portal_window_proxy_.reset(); }

private:
std::unique_ptr<CaptivePortalWindowProxy> captive_portal_window_proxy_;
CaptivePortalWindowProxyStubDelegate delegate_;
NetworkPortalDetectorTestImpl* network_portal_detector_;
};

IN_PROC_BROWSER_TEST_F(CaptivePortalWindowTest, Show) {
Show();
}

IN_PROC_BROWSER_TEST_F(CaptivePortalWindowTest, ShowClose) {
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

Show();
CheckState(true, 0);
CheckState(/*is_shown=*/true, /*in_progress=*/false);

Close();
// Wait for widget to be destroyed
base::RunLoop().RunUntilIdle();
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);
}

IN_PROC_BROWSER_TEST_F(CaptivePortalWindowTest, OnRedirected) {
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

ShowIfRedirected();
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

OnRedirected();
CheckState(true, 1);
CheckState(/*is_shown=*/true, /*in_progress=*/true);

Close();
// Wait for widget to be destroyed
base::RunLoop().RunUntilIdle();
CheckState(false, 1);
CheckState(/*is_shown=*/false, /*in_progress=*/true);
}

IN_PROC_BROWSER_TEST_F(CaptivePortalWindowTest, OnOriginalURLLoaded) {
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

ShowIfRedirected();
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

OnRedirected();
CheckState(true, 1);
CheckState(/*is_shown=*/true, /*in_progress=*/true);

OnOriginalURLLoaded();
// Wait for widget to be destroyed
base::RunLoop().RunUntilIdle();
CheckState(false, 1);
CheckState(/*is_shown=*/false, /*in_progress=*/true);
}

IN_PROC_BROWSER_TEST_F(CaptivePortalWindowTest, MultipleCalls) {
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

ShowIfRedirected();
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

Show();
CheckState(true, 0);
CheckState(/*is_shown=*/true, /*in_progress=*/false);

Close();
// Wait for widget to be destroyed
base::RunLoop().RunUntilIdle();
CheckState(false, 0);
CheckState(/*is_shown=*/false, /*in_progress=*/false);

OnRedirected();
CheckState(false, 1);
CheckState(/*is_shown=*/false, /*in_progress=*/true);

OnOriginalURLLoaded();
// Wait for widget to be destroyed
base::RunLoop().RunUntilIdle();
CheckState(false, 1);
CheckState(/*is_shown=*/false, /*in_progress=*/true);

Show();
CheckState(true, 1);
CheckState(/*is_shown=*/true, /*in_progress=*/true);

OnRedirected();
CheckState(true, 2);
CheckState(/*is_shown=*/true, /*in_progress=*/true);

Close();
// Wait for widget to be destroyed
base::RunLoop().RunUntilIdle();
CheckState(false, 2);
CheckState(/*is_shown=*/false, /*in_progress=*/true);

OnOriginalURLLoaded();
CheckState(false, 2);
CheckState(/*is_shown=*/false, /*in_progress=*/true);
}

class CaptivePortalWindowCtorDtorTest : public LoginManagerTest {
Expand Down
8 changes: 5 additions & 3 deletions chrome/browser/ash/login/ui/captive_portal_window_proxy.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "chrome/browser/themes/custom_theme_supplier.h"
#include "chrome/browser/themes/theme_service.h"
#include "chrome/browser/ui/webui/chromeos/internet_detail_dialog.h"
#include "chromeos/ash/components/network/portal_detector/network_portal_detector.h"
#include "components/constrained_window/constrained_window_views.h"
#include "components/web_modal/web_contents_modal_dialog_host.h"
#include "components/web_modal/web_contents_modal_dialog_manager.h"
Expand Down Expand Up @@ -71,9 +72,8 @@ views::Widget* CreateWindowAsFramelessChild(
} // namespace

CaptivePortalWindowProxy::CaptivePortalWindowProxy(
Delegate* delegate,
content::WebContents* web_contents)
: delegate_(delegate), web_contents_(web_contents) {
: web_contents_(web_contents) {
DCHECK_EQ(STATE_IDLE, GetState());
}

Expand Down Expand Up @@ -133,7 +133,9 @@ void CaptivePortalWindowProxy::OnRedirected() {
if (GetState() == STATE_WAITING_FOR_REDIRECTION) {
Show();
}
delegate_->OnPortalDetected();
NetworkPortalDetector* detector = network_portal_detector::GetInstance();
if (detector)
detector->StartPortalDetection();
}

void CaptivePortalWindowProxy::OnOriginalURLLoaded() {
Expand Down
22 changes: 1 addition & 21 deletions chrome/browser/ash/login/ui/captive_portal_window_proxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,6 @@ class Widget;
namespace ash {
class CaptivePortalView;

// Delegate interface for CaptivePortalWindowProxy.
class CaptivePortalWindowProxyDelegate {
public:
// Called when a captive portal is detected.
virtual void OnPortalDetected() = 0;

protected:
virtual ~CaptivePortalWindowProxyDelegate() = default;
};

// Proxy which manages showing of the window for CaptivePortal sign-in.
class CaptivePortalWindowProxy : public views::WidgetObserver {
public:
Expand All @@ -48,10 +38,7 @@ class CaptivePortalWindowProxy : public views::WidgetObserver {
virtual void OnAfterCaptivePortalHidden() {}
};

using Delegate = CaptivePortalWindowProxyDelegate;

CaptivePortalWindowProxy(Delegate* delegate,
content::WebContents* web_contents);
explicit CaptivePortalWindowProxy(content::WebContents* web_contents);
CaptivePortalWindowProxy(const CaptivePortalWindowProxy&) = delete;
CaptivePortalWindowProxy& operator=(const CaptivePortalWindowProxy&) = delete;
~CaptivePortalWindowProxy() override;
Expand Down Expand Up @@ -120,7 +107,6 @@ class CaptivePortalWindowProxy : public views::WidgetObserver {
}

Profile* profile_ = ProfileHelper::GetSigninProfile();
Delegate* delegate_;
content::WebContents* web_contents_;
views::Widget* widget_ = nullptr;

Expand All @@ -132,10 +118,4 @@ class CaptivePortalWindowProxy : public views::WidgetObserver {

} // namespace ash

// TODO(https://crbug.com/1164001): remove after the //chrome/browser/chromeos
// source migration is finished.
namespace chromeos {
using ::ash::CaptivePortalWindowProxyDelegate;
}

#endif // CHROME_BROWSER_ASH_LOGIN_UI_CAPTIVE_PORTAL_WINDOW_PROXY_H_
4 changes: 0 additions & 4 deletions chrome/browser/ui/webui/chromeos/login/network_state_informer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,6 @@ void NetworkStateInformer::OnPortalDetectionCompleted(
UpdateStateAndNotify();
}

void NetworkStateInformer::OnPortalDetected() {
UpdateStateAndNotify();
}

// static
const char* NetworkStateInformer::StatusString(State state) {
switch (state) {
Expand Down
4 changes: 0 additions & 4 deletions chrome/browser/ui/webui/chromeos/login/network_state_informer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ namespace chromeos {
// changed. Also, it answers to the requests about current network state.
class NetworkStateInformer : public chromeos::NetworkStateHandlerObserver,
public chromeos::NetworkPortalDetector::Observer,
public CaptivePortalWindowProxyDelegate,
public base::RefCounted<NetworkStateInformer> {
public:
enum State {
Expand Down Expand Up @@ -70,9 +69,6 @@ class NetworkStateInformer : public chromeos::NetworkStateHandlerObserver,
const NetworkState* network,
const NetworkPortalDetector::CaptivePortalStatus status) override;

// CaptivePortalWindowProxyDelegate implementation:
void OnPortalDetected() override;

State state() const { return state_; }
std::string network_path() const { return network_path_; }

Expand Down

0 comments on commit f32987a

Please sign in to comment.