1.50.x Wrap clean link to a feature flag (uplift #17673) (#17680)

Wrap clean link to a feature flag
brave · Mar 22, 2023 · 1f00c17 · 1f00c17
1 parent 0defe0d
commit 1f00c17
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 13 deletions.
diff --git a/browser/about_flags.cc b/browser/about_flags.cc
@@ -6,6 +6,7 @@
 #include "chrome/browser/about_flags.h"
 
 #include "base/strings/string_util.h"
+#include "brave/browser/brave_browser_features.h"
 #include "brave/browser/brave_features_internal_names.h"
 #include "brave/browser/ethereum_remote_client/buildflags/buildflags.h"
 #include "brave/browser/ethereum_remote_client/features.h"
@@ -689,6 +690,12 @@ constexpr char kRestrictEventSourcePoolDescription[] =
      flag_descriptions::kUseDevUpdaterUrlName,                              \
      flag_descriptions::kUseDevUpdaterUrlDescription, kOsAll,               \
      FEATURE_VALUE_TYPE(brave_component_updater::kUseDevUpdaterUrl)},       \
+    {"brave-copy-clean-link-by-default",                                    \
+     "Override default copy hotkey with copy clean link",                   \
+     "Sanitize url before copying, replaces default ctrl+c hotkey for "     \
+     "url ",                                                                \
+     kOsWin | kOsLinux | kOsMac,                                            \
+     FEATURE_VALUE_TYPE(features::kBraveCopyCleanLinkByDefault)},           \
     {"allow-certain-client-hints",                                          \
       flag_descriptions::kAllowCertainClientHintsName,                      \
       flag_descriptions::kAllowCertainClientHintsDescription, kOsAll,       \

diff --git a/browser/brave_app_controller_mac.mm b/browser/brave_app_controller_mac.mm
@@ -6,6 +6,7 @@
 #import "brave/browser/brave_app_controller_mac.h"
 
 #include "brave/app/brave_command_ids.h"
+#include "brave/browser/brave_browser_features.h"
 #include "brave/browser/ui/browser_commands.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/ui/browser.h"
@@ -60,7 +61,11 @@ - (void)menuNeedsUpdate:(NSMenu*)menu {
   }
   if ([self shouldShowCleanLinkItem]) {
     [_copyCleanLinkMenuItem setHidden:NO];
-    [self setKeyEquivalentToItem:_copyCleanLinkMenuItem];
+    if (base::FeatureList::IsEnabled(features::kBraveCopyCleanLinkByDefault)) {
+      [self setKeyEquivalentToItem:_copyCleanLinkMenuItem];
+    } else {
+      [self setKeyEquivalentToItem:_copyMenuItem];
+    }
   } else {
     [_copyCleanLinkMenuItem setHidden:YES];
     [self setKeyEquivalentToItem:_copyMenuItem];

diff --git a/browser/brave_app_controller_mac_browsertest.mm b/browser/brave_app_controller_mac_browsertest.mm
@@ -13,8 +13,10 @@
 #include "base/mac/foundation_util.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/mac/scoped_objc_class_swizzler.h"
+#include "base/test/scoped_feature_list.h"
 #include "brave/app/brave_command_ids.h"
 #include "brave/browser/brave_app_controller_mac.h"
+#include "brave/browser/brave_browser_features.h"
 #include "brave/browser/ui/views/frame/brave_browser_view.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/ui/browser.h"
@@ -32,6 +34,58 @@
 
 using BraveAppControllerBrowserTest = InProcessBrowserTest;
 
+class BraveAppControllerCleanLinkFeatureDisabledBrowserTest
+    : public InProcessBrowserTest {
+ public:
+  BraveAppControllerCleanLinkFeatureDisabledBrowserTest() {
+    features_.InitWithFeatureState(features::kBraveCopyCleanLinkByDefault,
+                                   false);
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+};
+
+IN_PROC_BROWSER_TEST_F(BraveAppControllerCleanLinkFeatureDisabledBrowserTest,
+                       CopyLinkItemVisible) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url = embedded_test_server()->GetURL(kTestingPage);
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url));
+  EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
+
+  BraveBrowserView* browser_view = static_cast<BraveBrowserView*>(
+      BraveBrowserView::GetBrowserViewForBrowser(browser()));
+  OmniboxView* omnibox_view = browser_view->GetLocationBar()->GetOmniboxView();
+  omnibox_view->SetFocus(true);
+  omnibox_view->SelectAll(false);
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(BraveBrowserWindow::From(browser()->window())->HasSelectedURL());
+
+  BraveAppController* ac = base::mac::ObjCCastStrict<BraveAppController>(
+      [[NSApplication sharedApplication] delegate]);
+  ASSERT_TRUE(ac);
+  base::scoped_nsobject<NSMenu> edit_submenu(
+      [[[NSApp mainMenu] itemWithTag:IDC_EDIT_MENU] submenu],
+      base::scoped_policy::RETAIN);
+
+  base::scoped_nsobject<NSMenuItem> copy_item(
+      [edit_submenu itemWithTag:IDC_CONTENT_CONTEXT_COPY],
+      base::scoped_policy::RETAIN);
+
+  base::scoped_nsobject<NSMenuItem> clean_link_menu_item(
+      [edit_submenu itemWithTag:IDC_COPY_CLEAN_LINK],
+      base::scoped_policy::RETAIN);
+
+  [ac menuNeedsUpdate:[clean_link_menu_item menu]];
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE([clean_link_menu_item isHidden]);
+
+  EXPECT_TRUE([[clean_link_menu_item keyEquivalent] isEqualToString:@""]);
+
+  EXPECT_TRUE([[copy_item keyEquivalent] isEqualToString:@"c"]);
+  EXPECT_EQ([copy_item keyEquivalentModifierMask], NSEventModifierFlagCommand);
+}
+
 IN_PROC_BROWSER_TEST_F(BraveAppControllerBrowserTest, CopyLinkItemVisible) {
   ASSERT_TRUE(embedded_test_server()->Start());
   GURL url = embedded_test_server()->GetURL(kTestingPage);

diff --git a/browser/brave_browser_features.cc b/browser/brave_browser_features.cc
@@ -12,4 +12,9 @@ BASE_FEATURE(kBraveCleanupSessionCookiesOnSessionRestore,
              "BraveCleanupSessionCookiesOnSessionRestore",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
+// Sanitize url before copying, replaces default ctrl+c hotkey for urls.
+BASE_FEATURE(kBraveCopyCleanLinkByDefault,
+             "brave-copy-clean-link-by-default",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 }  // namespace features
diff --git a/browser/brave_browser_features.h b/browser/brave_browser_features.h
@@ -11,6 +11,7 @@
 namespace features {
 
 BASE_DECLARE_FEATURE(kBraveCleanupSessionCookiesOnSessionRestore);
+BASE_DECLARE_FEATURE(kBraveCopyCleanLinkByDefault);
 
 }  // namespace features
 

diff --git a/browser/ui/views/omnibox/brave_omnibox_view_views.cc b/browser/ui/views/omnibox/brave_omnibox_view_views.cc
@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "brave/app/brave_command_ids.h"
+#include "brave/browser/brave_browser_features.h"
 #include "brave/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -63,6 +64,10 @@ void BraveOmniboxViewViews::CopySanitizedURL(const GURL& url) {
 #if BUILDFLAG(IS_WIN)
 bool BraveOmniboxViewViews::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
+  if (!base::FeatureList::IsEnabled(features::kBraveCopyCleanLinkByDefault)) {
+    return OmniboxViewViews::AcceleratorPressed(accelerator);
+  }
+
   ui::KeyEvent event(
       accelerator.key_state() == ui::Accelerator::KeyState::PRESSED
           ? ui::ET_KEY_PRESSED
@@ -85,14 +90,16 @@ bool BraveOmniboxViewViews::AcceleratorPressed(
 bool BraveOmniboxViewViews::GetAcceleratorForCommandId(
     int command_id,
     ui::Accelerator* accelerator) const {
-  bool is_url = const_cast<BraveOmniboxViewViews*>(this)->SelectedTextIsURL();
-  if (is_url) {
-    if (command_id == kCopy) {
-      return false;
-    }
-    if (command_id == IDC_COPY_CLEAN_LINK) {
-      *accelerator = ui::Accelerator(ui::VKEY_C, ui::EF_PLATFORM_ACCELERATOR);
-      return true;
+  if (base::FeatureList::IsEnabled(features::kBraveCopyCleanLinkByDefault)) {
+    bool is_url = const_cast<BraveOmniboxViewViews*>(this)->SelectedTextIsURL();
+    if (is_url) {
+      if (command_id == kCopy) {
+        return false;
+      }
+      if (command_id == IDC_COPY_CLEAN_LINK) {
+        *accelerator = ui::Accelerator(ui::VKEY_C, ui::EF_PLATFORM_ACCELERATOR);
+        return true;
+      }
     }
   }
   return OmniboxViewViews::GetAcceleratorForCommandId(command_id, accelerator);
@@ -101,10 +108,12 @@ bool BraveOmniboxViewViews::GetAcceleratorForCommandId(
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_MAC)
 void BraveOmniboxViewViews::ExecuteTextEditCommand(
     ui::TextEditCommand command) {
-  auto url_to_copy = GetURLToCopy();
-  if (command == ui::TextEditCommand::COPY && url_to_copy.has_value()) {
-    CopySanitizedURL(url_to_copy.value());
-    return;
+  if (base::FeatureList::IsEnabled(features::kBraveCopyCleanLinkByDefault)) {
+    auto url_to_copy = GetURLToCopy();
+    if (command == ui::TextEditCommand::COPY && url_to_copy.has_value()) {
+      CopySanitizedURL(url_to_copy.value());
+      return;
+    }
   }
   OmniboxViewViews::ExecuteTextEditCommand(command);
 }