Skip to content

Commit

Permalink
Fixed vpn button still has connected state for not purchased user(upl…
Browse files Browse the repository at this point in the history 
…ift to 1.60.x) (#21083)

* Merge pull request #20386 from brave/button-update

Remove brave vpn toolbar button flashes when reconnecting multiple times

* Merge pull request #20533 from brave/vpn_button_state_purchased_state

Fixed vpn button still has connected state for not purchased user
  • Loading branch information
@simonhong
simonhong authored Nov 23, 2023
1 parent dcc5ed6 commit 91128b5
Show file tree
Hide file tree
Showing 15 changed files with 414 additions and 70 deletions.
105 changes: 61 additions & 44 deletions browser/brave_vpn/brave_vpn_service_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,59 @@
#endif

namespace brave_vpn {
namespace {

std::unique_ptr<KeyedService> BuildVpnService(
content::BrowserContext* context) {
if (!brave_vpn::IsAllowedForContext(context)) {
return nullptr;
}

#if !BUILDFLAG(IS_ANDROID)
if (!g_brave_browser_process->brave_vpn_os_connection_api()) {
return nullptr;
}
#endif

auto* default_storage_partition = context->GetDefaultStoragePartition();
auto shared_url_loader_factory =
default_storage_partition->GetURLLoaderFactoryForBrowserProcess();
auto* local_state = g_browser_process->local_state();
brave_vpn::MigrateVPNSettings(user_prefs::UserPrefs::Get(context),
local_state);
auto callback = base::BindRepeating(
[](content::BrowserContext* context) {
return skus::SkusServiceFactory::GetForContext(context);
},
context);

std::unique_ptr<BraveVpnService> vpn_service =
std::make_unique<BraveVpnService>(
g_brave_browser_process->brave_vpn_os_connection_api(),
shared_url_loader_factory, local_state,
user_prefs::UserPrefs::Get(context), callback);
#if BUILDFLAG(IS_WIN)
if (brave_vpn::IsBraveVPNWireguardEnabled(g_browser_process->local_state())) {
auto* observer_service =
brave_vpn::BraveVpnWireguardObserverFactory::GetInstance()
->GetServiceForContext(context);
if (observer_service) {
observer_service->Observe(vpn_service.get());
}
} else {
auto* observer_service =
brave_vpn::BraveVpnDnsObserverFactory::GetInstance()
->GetServiceForContext(context);
if (observer_service) {
observer_service->Observe(vpn_service.get());
}
}
#endif

return vpn_service;
}

} // namespace

// static
BraveVpnServiceFactory* BraveVpnServiceFactory::GetInstance() {
Expand Down Expand Up @@ -73,52 +126,16 @@ BraveVpnServiceFactory::BraveVpnServiceFactory()

BraveVpnServiceFactory::~BraveVpnServiceFactory() = default;

KeyedService* BraveVpnServiceFactory::BuildServiceInstanceFor(
std::unique_ptr<KeyedService>
BraveVpnServiceFactory::BuildServiceInstanceForBrowserContext(
content::BrowserContext* context) const {
if (!brave_vpn::IsAllowedForContext(context)) {
return nullptr;
}

#if !BUILDFLAG(IS_ANDROID)
if (!g_brave_browser_process->brave_vpn_os_connection_api()) {
return nullptr;
}
#endif

auto* default_storage_partition = context->GetDefaultStoragePartition();
auto shared_url_loader_factory =
default_storage_partition->GetURLLoaderFactoryForBrowserProcess();
auto* local_state = g_browser_process->local_state();
brave_vpn::MigrateVPNSettings(user_prefs::UserPrefs::Get(context),
local_state);
auto callback = base::BindRepeating(
[](content::BrowserContext* context) {
return skus::SkusServiceFactory::GetForContext(context);
},
context);
return BuildVpnService(context);
}

auto* vpn_service = new BraveVpnService(
g_brave_browser_process->brave_vpn_os_connection_api(),
shared_url_loader_factory, local_state,
user_prefs::UserPrefs::Get(context), callback);
#if BUILDFLAG(IS_WIN)
if (brave_vpn::IsBraveVPNWireguardEnabled(g_browser_process->local_state())) {
auto* observer_service =
brave_vpn::BraveVpnWireguardObserverFactory::GetInstance()
->GetServiceForContext(context);
if (observer_service) {
observer_service->Observe(vpn_service);
}
} else {
auto* observer_service =
brave_vpn::BraveVpnDnsObserverFactory::GetInstance()
->GetServiceForContext(context);
if (observer_service) {
observer_service->Observe(vpn_service);
}
}
#endif
return vpn_service;
// static
BrowserContextKeyedServiceFactory::TestingFactory
BraveVpnServiceFactory::GetDefaultFactory() {
return base::BindRepeating(&BuildVpnService);
}

void BraveVpnServiceFactory::RegisterProfilePrefs(
Expand Down
7 changes: 6 additions & 1 deletion browser/brave_vpn/brave_vpn_service_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#ifndef BRAVE_BROWSER_BRAVE_VPN_BRAVE_VPN_SERVICE_FACTORY_H_
#define BRAVE_BROWSER_BRAVE_VPN_BRAVE_VPN_SERVICE_FACTORY_H_

#include <memory>

#include "brave/components/brave_vpn/common/mojom/brave_vpn.mojom.h"
#include "build/build_config.h"
#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
Expand Down Expand Up @@ -33,14 +35,17 @@ class BraveVpnServiceFactory : public BrowserContextKeyedServiceFactory {
content::BrowserContext* context,
mojo::PendingReceiver<brave_vpn::mojom::ServiceHandler> receiver);

// Returns the default factory, useful in tests.
static TestingFactory GetDefaultFactory();

private:
friend base::NoDestructor<BraveVpnServiceFactory>;

BraveVpnServiceFactory();
~BraveVpnServiceFactory() override;

// BrowserContextKeyedServiceFactory overrides:
KeyedService* BuildServiceInstanceFor(
std::unique_ptr<KeyedService> BuildServiceInstanceForBrowserContext(
content::BrowserContext* context) const override;
void RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) override;
Expand Down
10 changes: 10 additions & 0 deletions browser/brave_vpn/win/brave_vpn_wireguard_service/status_tray/status_tray_runner.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,16 @@ void StatusTrayRunner::UpdateConnectionState() {
if (current_state_ == state) {
return;
}
// Skip attempts to connect/disconnet if we had an error before and keep
// the icon in the error state until we get it clearly fixed.
bool should_skip_connection_attempt =
(current_state_ == brave_vpn::mojom::ConnectionState::CONNECT_FAILED &&
(state == brave_vpn::mojom::ConnectionState::CONNECTING ||
state == brave_vpn::mojom::ConnectionState::DISCONNECTING));
if (should_skip_connection_attempt) {
VLOG(1) << __func__ << " skip state: " << state;
return;
}
VLOG(1) << __func__ << ":" << state;
current_state_ = state;
SetIconState(GetStatusTrayIcon(state), GetStatusIconTooltip(state));
Expand Down
59 changes: 58 additions & 1 deletion browser/brave_vpn/win/brave_vpn_wireguard_service/status_tray/status_tray_runner_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ class StatusTrayRunnerTest : public testing::Test {
IDC_BRAVE_VPN_TRAY_DISCONNECT_VPN_ITEM));
}

void UpdateConnectionState() {
StatusTrayRunner::GetInstance()->UpdateConnectionState();
}

void WaitIconStateChangedTo(int expected_icon_id, int expected_tooltip_id) {
StatusTrayRunner::GetInstance()->SetIconStateCallbackForTesting(
base::BindLambdaForTesting([&](int icon_id, int tooltip_id) {
Expand All @@ -61,7 +65,7 @@ class StatusTrayRunnerTest : public testing::Test {
icon_state_updated_ = true;
}));
EXPECT_FALSE(IsIconStateUpdated());
StatusTrayRunner::GetInstance()->UpdateConnectionState();
UpdateConnectionState();
EXPECT_TRUE(IsIconStateUpdated());
ResetIconState();
EXPECT_FALSE(IsIconStateUpdated());
Expand Down Expand Up @@ -165,4 +169,57 @@ TEST_F(StatusTrayRunnerTest, UpdateConnectionState) {
static_cast<int>(brave_vpn::mojom::ConnectionState::CONNECTED));
}

TEST_F(StatusTrayRunnerTest, SkipAttemptsToConnectInFailedState) {
registry_util::RegistryOverrideManager registry_overrides;
registry_overrides.OverrideRegistry(HKEY_CURRENT_USER);

ui::NativeTheme::GetInstanceForNativeUi()->set_use_dark_colors(true);
EXPECT_TRUE(ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors());
// Tunnel service stopped, state disconnected, no info in registry.
StatusTrayRunner::GetInstance()->SetVPNConnectedForTesting(false);
WaitIconStateChangedTo(
IDR_BRAVE_VPN_TRAY_LIGHT,
IDS_BRAVE_VPN_WIREGUARD_TRAY_ICON_TOOLTIP_DISCONNECTED);
EXPECT_FALSE(brave_vpn::GetConnectionState().has_value());

// Tunnel service stopped, registry state as "connecting"
WriteConnectionState(
static_cast<int>(brave_vpn::mojom::ConnectionState::CONNECTING));
WaitIconStateChangedTo(IDR_BRAVE_VPN_TRAY_LIGHT_CONNECTING,
IDS_BRAVE_VPN_WIREGUARD_TRAY_ICON_TOOLTIP_CONNECTING);
EXPECT_EQ(brave_vpn::GetConnectionState().value(),
static_cast<int>(brave_vpn::mojom::ConnectionState::CONNECTING));

// Tunnel service stopped, registry state as "CONNECT_FAILED"
WriteConnectionState(
static_cast<int>(brave_vpn::mojom::ConnectionState::CONNECT_FAILED));
WaitIconStateChangedTo(IDR_BRAVE_VPN_TRAY_LIGHT_ERROR,
IDS_BRAVE_VPN_WIREGUARD_TRAY_ICON_TOOLTIP_ERROR);
EXPECT_EQ(
brave_vpn::GetConnectionState().value(),
static_cast<int>(brave_vpn::mojom::ConnectionState::CONNECT_FAILED));

// Tunnel service stopped, registry state as "connecting"
WriteConnectionState(
static_cast<int>(brave_vpn::mojom::ConnectionState::CONNECTING));
EXPECT_FALSE(IsIconStateUpdated());
UpdateConnectionState();
// Icon state should not be updated.
EXPECT_FALSE(IsIconStateUpdated());

// Tunnel service stopped, registry state as "disconnecting"
WriteConnectionState(
static_cast<int>(brave_vpn::mojom::ConnectionState::DISCONNECTING));
EXPECT_FALSE(IsIconStateUpdated());
UpdateConnectionState();
// Icon state should not be updated.
EXPECT_FALSE(IsIconStateUpdated());

// Service is working, state connected.
StatusTrayRunner::GetInstance()->SetVPNConnectedForTesting(true);
WaitIconStateChangedTo(IDR_BRAVE_VPN_TRAY_LIGHT_CONNECTED,
IDS_BRAVE_VPN_WIREGUARD_TRAY_ICON_TOOLTIP_CONNECTED);
EXPECT_EQ(brave_vpn::GetConnectionState().value(),
static_cast<int>(brave_vpn::mojom::ConnectionState::CONNECTED));
}
} // namespace brave_vpn
23 changes: 23 additions & 0 deletions browser/ui/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,29 @@ source_set("ui") {
}
}

source_set("unit_tests") {
if (!is_android) {
testonly = true
sources = []
deps = []

if (enable_brave_vpn) {
sources += [ "views/toolbar/brave_vpn_button_unittest.cc" ]
deps += [
"//base",
"//brave/components/brave_vpn/browser",
"//brave/components/brave_vpn/browser/connection:api",
"//brave/components/brave_vpn/browser/connection/ikev2:sim",
"//brave/components/skus/browser",
"//chrome/test:test_support",
"//skia",
"//testing/gtest",
"//third_party/abseil-cpp:absl",
]
}
}
}

source_set("browser_tests") {
if (!is_android) {
testonly = true
Expand Down
61 changes: 39 additions & 22 deletions browser/ui/views/toolbar/brave_vpn_button.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ BraveVPNButton::BraveVPNButton(Browser* browser)
browser_(browser),
service_(brave_vpn::BraveVpnServiceFactory::GetForProfile(
browser_->profile())) {
DCHECK(service_);
CHECK(service_);
UpdateButtonState();
Observe(service_);

// Replace ToolbarButton's highlight path generator.
Expand Down Expand Up @@ -199,15 +200,26 @@ BraveVPNButton::BraveVPNButton(Browser* browser)
BraveVPNButton::~BraveVPNButton() = default;

void BraveVPNButton::OnConnectionStateChanged(ConnectionState state) {
if (IsErrorState() && (state == ConnectionState::CONNECTING ||
state == ConnectionState::DISCONNECTING)) {
// Skip attempts to connect/disconnet if we had an error before and keep
// the button in the error state until we get it clearly fixed.
return;
}
UpdateButtonState();
UpdateColorsAndInsets();
}

void BraveVPNButton::UpdateButtonState() {
is_error_state_ = IsConnectError();
is_connected_ = IsConnected();
}

void BraveVPNButton::OnPurchasedStateChanged(
brave_vpn::mojom::PurchasedState state,
const absl::optional<std::string>& description) {
if (IsPurchased()) {
UpdateColorsAndInsets();
}
UpdateButtonState();
UpdateColorsAndInsets();
}

std::unique_ptr<views::Border> BraveVPNButton::GetBorder(
Expand All @@ -225,18 +237,16 @@ void BraveVPNButton::UpdateColorsAndInsets() {
if (!cp) {
return;
}
const bool is_connect_error = IsConnectError();
const bool is_connected = IsConnected();

const auto bg_color =
cp->GetColor(is_connect_error ? kColorBraveVpnButtonErrorBackgroundNormal
: kColorBraveVpnButtonBackgroundNormal);
cp->GetColor(is_error_state_ ? kColorBraveVpnButtonErrorBackgroundNormal
: kColorBraveVpnButtonBackgroundNormal);
SetBackground(views::CreateRoundedRectBackground(bg_color, kButtonRadius));

SetEnabledTextColors(cp->GetColor(is_connect_error
SetEnabledTextColors(cp->GetColor(is_error_state_
? kColorBraveVpnButtonTextError
: kColorBraveVpnButtonText));

if (is_connect_error) {
if (is_error_state_) {
SetImage(
views::Button::STATE_NORMAL,
gfx::CreateVectorIcon(kVpnIndicatorErrorIcon,
Expand All @@ -246,12 +256,12 @@ void BraveVPNButton::UpdateColorsAndInsets() {
image()->SetBackground(std::make_unique<ConnectErrorIconBackground>(
cp->GetColor(kColorBraveVpnButtonIconErrorInner)));
} else {
SetImage(
views::Button::STATE_NORMAL,
gfx::CreateVectorIcon(
is_connected ? kVpnIndicatorOnIcon : kVpnIndicatorOffIcon,
cp->GetColor(is_connected ? kColorBraveVpnButtonIconConnected
: kColorBraveVpnButtonIconDisconnected)));
SetImage(views::Button::STATE_NORMAL,
gfx::CreateVectorIcon(
is_connected_ ? kVpnIndicatorOnIcon : kVpnIndicatorOffIcon,
cp->GetColor(is_connected_
? kColorBraveVpnButtonIconConnected
: kColorBraveVpnButtonIconDisconnected)));

// Use background for inner color of button image.
// Adjusted border thickness to make invisible to the outside of the icon.
Expand All @@ -263,16 +273,16 @@ void BraveVPNButton::UpdateColorsAndInsets() {
// border color are mixed as both have alpha value.
// Draw border only for error state.
SetBorder(GetBorder(color_utils::GetResultingPaintColor(
cp->GetColor(is_connect_error ? kColorBraveVpnButtonErrorBorder
: kColorBraveVpnButtonBorder),
cp->GetColor(is_error_state_ ? kColorBraveVpnButtonErrorBorder
: kColorBraveVpnButtonBorder),
bg_color)));

auto* ink_drop_host = views::InkDrop::Get(this);

// Use different ink drop hover color for each themes.
auto target_base_color = color_utils::GetResultingPaintColor(
cp->GetColor(is_connect_error ? kColorBraveVpnButtonErrorBackgroundHover
: kColorBraveVpnButtonBorder),
cp->GetColor(is_error_state_ ? kColorBraveVpnButtonErrorBackgroundHover
: kColorBraveVpnButtonBorder),
bg_color);
bool need_ink_drop_color_update =
target_base_color != ink_drop_host->GetBaseColor();
Expand Down Expand Up @@ -312,8 +322,15 @@ bool BraveVPNButton::IsConnected() const {
return service_->IsConnected();
}

ConnectionState BraveVPNButton::GetVpnConnectionState() const {
if (connection_state_for_testing_) {
return connection_state_for_testing_.value();
}
return service_->GetConnectionState();
}

bool BraveVPNButton::IsConnectError() const {
const auto state = service_->GetConnectionState();
const auto state = GetVpnConnectionState();
return (state == ConnectionState::CONNECT_NOT_ALLOWED ||
state == ConnectionState::CONNECT_FAILED);
}
Expand Down
Loading

0 comments on commit 91128b5

Please sign in to comment.