Browse Source

fix: backport upstream fixes for color chooser dialogs (backport: 5-0-x) (#17254)

* fix: backport upstream fixes for color chooser dialogs

* chore: fix patches, Windows bad, linux good

* Update color_chooser_mac.patch

* Update color_chooser_win.patch
trop[bot] 6 years ago
parent
commit
78a2a2f18d

+ 2 - 1
patches/common/chromium/.patches

@@ -61,7 +61,6 @@ content_browser_main_loop.patch
 dump_syms.patch
 command-ismediakey.patch
 tts.patch
-color_chooser.patch
 printing.patch
 verbose_generate_breakpad_symbols.patch
 cross_site_document_resource_handler.patch
@@ -71,3 +70,5 @@ disable_color_correct_rendering.patch
 disable_time_ticks_dcheck.patch
 revert_build_swiftshader_for_arm32.patch
 fix_backup_includes_for_ptrace_get_thread_area_on_arm_arm64.patch
+color_chooser_mac.patch
+color_chooser_win.patch

+ 0 - 23
patches/common/chromium/color_chooser.patch

@@ -1,23 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Heilig Benedek <[email protected]>
-Date: Thu, 18 Oct 2018 17:08:13 -0700
-Subject: color_chooser.patch
-
-Disables a DCHECK that crashes the ColorChooser on Windows,
-that DCHECK most likely is an artifact that remained in chromium from a
-long time ago (last update of that part of the code was around 2012-2013,
-and this is purely UI, I don't think they have automated tests for it).
-
-diff --git a/chrome/browser/ui/views/color_chooser_win.cc b/chrome/browser/ui/views/color_chooser_win.cc
-index 434842ba0f81206a43fb26874930bf7309782890..93b4152003eaea05f0e16cd049687fbcbc672fb0 100644
---- a/chrome/browser/ui/views/color_chooser_win.cc
-+++ b/chrome/browser/ui/views/color_chooser_win.cc
-@@ -91,7 +91,7 @@ void ColorChooserWin::OnColorChooserDialogClosed() {
-     color_chooser_dialog_->ListenerDestroyed();
-     color_chooser_dialog_ = NULL;
-   }
--  DCHECK(current_color_chooser_ == this);
-+  // DCHECK(current_color_chooser_ == this);
-   current_color_chooser_ = NULL;
-   if (web_contents_)
-     web_contents_->DidEndColorChooser();

+ 186 - 0
patches/common/chromium/color_chooser_mac.patch

@@ -0,0 +1,186 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Benedek Heilig <[email protected]>
+Date: Thu, 21 Feb 2019 17:04:30 +0000
+Subject: fix a crash when using color chooser on macos
+
+Backports an upstream fix I made to the color chooser dialog on macos.
+This can be removed once that fix lands in a chromium version we use.
+Below is the original commit message:
+
+Fix crash when closing color chooser dialog on macos
+
+For some reason, closing the color chooser dialog caused a crash on
+macos. By using NSNotificationCenter instead of providing a delegate
+the crash goes away. I got the idea for this from the comment about the
+__target method already in the code.
+
+Change-Id: Ide35a455ff12decc9dd83b30c80b584ab376242b
+
+diff --git a/AUTHORS b/AUTHORS
+index 374b1a177f30d0c4b415d5c9c0fc4e4673414d39..436be8a093831020453285f0c476dcf9bd42fe8d 100644
+--- a/AUTHORS
++++ b/AUTHORS
+@@ -114,6 +114,7 @@ Ben Coe <[email protected]>
+ Ben Fiola <[email protected]>
+ Ben Karel <[email protected]>
+ Ben Noordhuis <[email protected]>
++Benedek Heilig <[email protected]>
+ Benjamin Dupont <[email protected]>
+ Benjamin Jemlich <[email protected]>
+ Bernard Cafarelli <[email protected]>
+diff --git a/chrome/browser/ui/cocoa/color_chooser_mac.h b/chrome/browser/ui/cocoa/color_chooser_mac.h
+index 511000dc7855938ceab31694149ddf9e2125e0e4..9dbcc9fe41786555f0fb16ac47c71ee8851c8dd5 100644
+--- a/chrome/browser/ui/cocoa/color_chooser_mac.h
++++ b/chrome/browser/ui/cocoa/color_chooser_mac.h
+@@ -15,7 +15,7 @@ class ColorChooserMac;
+ 
+ // A Listener class to act as a event target for NSColorPanel and send
+ // the results to the C++ class, ColorChooserMac.
+-@interface ColorPanelCocoa : NSObject<NSWindowDelegate> {
++@interface ColorPanelCocoa : NSObject {
+  @protected
+   // We don't call DidChooseColor if the change wasn't caused by the user
+   // interacting with the panel.
+@@ -26,6 +26,8 @@ class ColorChooserMac;
+ 
+ - (id)initWithChooser:(ColorChooserMac*)chooser;
+ 
++- (void)windowWillClose:(NSNotification*)notification;
++
+ // Called from NSColorPanel.
+ - (void)didChooseColor:(NSColorPanel*)panel;
+ 
+@@ -45,7 +47,7 @@ class ColorChooserMac : public content::ColorChooser {
+ 
+   // Called from ColorPanelCocoa.
+   void DidChooseColorInColorPanel(SkColor color);
+-  void DidCloseColorPabel();
++  void DidCloseColorPanel();
+ 
+   // Set the color programmatically.
+   void SetSelectedColor(SkColor color) override;
+diff --git a/chrome/browser/ui/cocoa/color_chooser_mac.mm b/chrome/browser/ui/cocoa/color_chooser_mac.mm
+index bf47154f0a880f1c6143065e5c6d060f1df73765..e7cdab7a23d96345cc6e8ec578a8616d132b7d51 100644
+--- a/chrome/browser/ui/cocoa/color_chooser_mac.mm
++++ b/chrome/browser/ui/cocoa/color_chooser_mac.mm
+@@ -39,16 +39,18 @@ void ColorChooserMac::DidChooseColorInColorPanel(SkColor color) {
+     web_contents_->DidChooseColorInColorChooser(color);
+ }
+ 
+-void ColorChooserMac::DidCloseColorPabel() {
++void ColorChooserMac::DidCloseColorPanel() {
+   End();
+ }
+ 
+ void ColorChooserMac::End() {
+-  panel_.reset();
+-  DCHECK(current_color_chooser_ == this);
+-  current_color_chooser_ = NULL;
+-  if (web_contents_)
++  if (panel_) {
++    panel_.reset();
++    DCHECK(current_color_chooser_ == this);
++    current_color_chooser_ = NULL;
++    if (web_contents_)
+       web_contents_->DidEndColorChooser();
++  }
+ }
+ 
+ void ColorChooserMac::SetSelectedColor(SkColor color) {
+@@ -67,9 +69,14 @@ void ColorChooserMac::SetSelectedColor(SkColor color) {
+     chooser_ = chooser;
+     NSColorPanel* panel = [NSColorPanel sharedColorPanel];
+     [panel setShowsAlpha:NO];
+-    [panel setDelegate:self];
+     [panel setTarget:self];
+     [panel setAction:@selector(didChooseColor:)];
++
++    [[NSNotificationCenter defaultCenter]
++        addObserver:self
++           selector:@selector(windowWillClose:)
++               name:NSWindowWillCloseNotification
++             object:panel];
+   }
+   return self;
+ }
+@@ -82,19 +89,21 @@ void ColorChooserMac::SetSelectedColor(SkColor color) {
+   // the ColorPanelCocoa is still the target.
+   BOOL respondsToPrivateTargetMethod =
+       [panel respondsToSelector:@selector(__target)];
+-
+-  if ([panel delegate] == self ||
+-      (respondsToPrivateTargetMethod && [panel __target] == self)) {
+-    [panel setDelegate:nil];
++  if (respondsToPrivateTargetMethod && [panel __target] == self) {
+     [panel setTarget:nil];
+     [panel setAction:nullptr];
+   }
+ 
++  [[NSNotificationCenter defaultCenter]
++      removeObserver:self
++                name:NSWindowWillCloseNotification
++              object:panel];
++
+   [super dealloc];
+ }
+ 
+ - (void)windowWillClose:(NSNotification*)notification {
+-  chooser_->DidCloseColorPabel();
++  chooser_->DidCloseColorPanel();
+   nonUserChange_ = NO;
+ }
+ 
+diff --git a/chrome/browser/ui/cocoa/color_panel_cocoa_unittest.mm b/chrome/browser/ui/cocoa/color_panel_cocoa_unittest.mm
+index 83185ceb9bbd29bf38fce7f72c23f3a5b15ba6c8..823365fdfc0bceceeed6f5edc00b3462715a4751 100644
+--- a/chrome/browser/ui/cocoa/color_panel_cocoa_unittest.mm
++++ b/chrome/browser/ui/cocoa/color_panel_cocoa_unittest.mm
+@@ -28,28 +28,6 @@ class ColorPanelCocoaTest : public CocoaTest {
+   }
+ };
+ 
+-TEST_F(ColorPanelCocoaTest, ClearTargetAndDelegateOnEnd) {
+-  NSColorPanel* nscolor_panel = [NSColorPanel sharedColorPanel];
+-  @autoreleasepool {
+-    EXPECT_TRUE([nscolor_panel respondsToSelector:@selector(__target)]);
+-
+-    // Create a ColorPanelCocoa.
+-    ColorChooserMac* color_chooser_mac =
+-        ColorChooserMac::Open(nullptr, SK_ColorBLACK);
+-
+-    // Confirm the NSColorPanel's configuration by the ColorChooserMac's
+-    // ColorPanelCocoa.
+-    EXPECT_TRUE([nscolor_panel delegate]);
+-    EXPECT_TRUE([nscolor_panel __target]);
+-
+-    // Release the ColorPanelCocoa and confirm it's no longer the NSColorPanel's
+-    // target or delegate.
+-    color_chooser_mac->End();
+-  }
+-  EXPECT_EQ([nscolor_panel delegate], nil);
+-  EXPECT_EQ([nscolor_panel __target], nil);
+-}
+-
+ TEST_F(ColorPanelCocoaTest, ClearTargetOnEnd) {
+   NSColorPanel* nscolor_panel = [NSColorPanel sharedColorPanel];
+   @autoreleasepool {
+@@ -61,19 +39,12 @@ TEST_F(ColorPanelCocoaTest, ClearTargetOnEnd) {
+ 
+     // Confirm the NSColorPanel's configuration by the ColorChooserMac's
+     // ColorPanelCocoa.
+-    EXPECT_TRUE([nscolor_panel delegate]);
+     EXPECT_TRUE([nscolor_panel __target]);
+ 
+-    // Clear the delegate and release the ColorPanelCocoa.
+-    [nscolor_panel setDelegate:nil];
+-
+     // Release the ColorPanelCocoa.
+     color_chooser_mac->End();
+   }
+-  // Confirm the ColorPanelCocoa is no longer the NSColorPanel's target or
+-  // delegate. Previously the ColorPanelCocoa would not clear the target if
+-  // the delegate had already been cleared.
+-  EXPECT_EQ([nscolor_panel delegate], nil);
++  // Confirm the ColorPanelCocoa is no longer the NSColorPanel's target
+   EXPECT_EQ([nscolor_panel __target], nil);
+ }
+ 

+ 92 - 0
patches/common/chromium/color_chooser_win.patch

@@ -0,0 +1,92 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Benedek Heilig <[email protected]>
+Date: Thu, 21 Feb 2019 17:16:33 +0000
+Subject: fix some DCHECKs on Windows when using color chooser dialog
+
+Backports an upstream fix I made to the color chooser dialog on
+Windows. This can be removed once that fix lands in a chromium version we
+use. Below is the original commit message:
+
+Fix DCHECKs being triggered while using color chooser dialog on Windows
+
+This fixes two DHCECKs being triggered on Windows when using the
+dialog in a debug build. The main source of these triggered DCHECKs was
+that when the user closes the dialog, OnColorChooserDialog is called,
+which calls WebContentsImpl::DidEndColorChooser, which calls End on the
+dialog, which calls OnColorChooserDialog again. This also happened on
+macos. The other DCHECK was the receiver->HasAtLeastOneRef() one,
+because ColorChooserDialog had a PostTask in it's constructor.
+
+Change-Id: I45ec32083a5850e006034073bc29d7ec4bb63189
+
+diff --git a/chrome/browser/ui/views/color_chooser_dialog.cc b/chrome/browser/ui/views/color_chooser_dialog.cc
+index c26be855120fcef78ce995d09fb3965f796746a9..1f568a2657250c5f21ab01da88950eedc6d4e177 100644
+--- a/chrome/browser/ui/views/color_chooser_dialog.cc
++++ b/chrome/browser/ui/views/color_chooser_dialog.cc
+@@ -24,12 +24,14 @@ using content::BrowserThread;
+ // static
+ COLORREF ColorChooserDialog::g_custom_colors[16];
+ 
+-ColorChooserDialog::ColorChooserDialog(views::ColorChooserListener* listener,
+-                                       SkColor initial_color,
+-                                       gfx::NativeWindow owning_window)
++ColorChooserDialog::ColorChooserDialog(views::ColorChooserListener* listener)
+     : listener_(listener) {
+   DCHECK(listener_);
+   CopyCustomColors(g_custom_colors, custom_colors_);
++}
++
++void ColorChooserDialog::Open(SkColor initial_color,
++                              gfx::NativeWindow owning_window) {
+   HWND owning_hwnd = views::HWNDForNativeWindow(owning_window);
+ 
+   std::unique_ptr<RunState> run_state = BeginRun(owning_hwnd);
+diff --git a/chrome/browser/ui/views/color_chooser_dialog.h b/chrome/browser/ui/views/color_chooser_dialog.h
+index 838e22dcd1a707714a098320d5bf8174fa60a2ea..b2558ce2a226ccde0f20de9712bf33051b21799c 100644
+--- a/chrome/browser/ui/views/color_chooser_dialog.h
++++ b/chrome/browser/ui/views/color_chooser_dialog.h
+@@ -23,9 +23,9 @@ class ColorChooserDialog
+       public ui::BaseShellDialog,
+       public ui::BaseShellDialogImpl {
+  public:
+-  ColorChooserDialog(views::ColorChooserListener* listener,
+-                     SkColor initial_color,
+-                     gfx::NativeWindow owning_window);
++  explicit ColorChooserDialog(views::ColorChooserListener* listener);
++
++  void Open(SkColor initial_color, gfx::NativeWindow owning_window);
+ 
+   // BaseShellDialog:
+   bool IsRunning(gfx::NativeWindow owning_window) const override;
+diff --git a/chrome/browser/ui/views/color_chooser_win.cc b/chrome/browser/ui/views/color_chooser_win.cc
+index 434842ba0f81206a43fb26874930bf7309782890..ffc58eec78dc36e21c2c04aa0b0cd9097541d2f0 100644
+--- a/chrome/browser/ui/views/color_chooser_win.cc
++++ b/chrome/browser/ui/views/color_chooser_win.cc
+@@ -61,9 +61,8 @@ ColorChooserWin::ColorChooserWin(content::WebContents* web_contents,
+                                      ->GetWidget()
+                                      ->GetView()
+                                      ->GetNativeView());
+-  color_chooser_dialog_ = new ColorChooserDialog(this,
+-                                                 initial_color,
+-                                                 owning_window);
++  color_chooser_dialog_ = new ColorChooserDialog(this);
++  color_chooser_dialog_->Open(initial_color, owning_window);
+ }
+ 
+ ColorChooserWin::~ColorChooserWin() {
+@@ -90,11 +89,11 @@ void ColorChooserWin::OnColorChooserDialogClosed() {
+   if (color_chooser_dialog_.get()) {
+     color_chooser_dialog_->ListenerDestroyed();
+     color_chooser_dialog_ = NULL;
++    DCHECK(current_color_chooser_ == this);
++    current_color_chooser_ = NULL;
++    if (web_contents_)
++      web_contents_->DidEndColorChooser();
+   }
+-  DCHECK(current_color_chooser_ == this);
+-  current_color_chooser_ = NULL;
+-  if (web_contents_)
+-    web_contents_->DidEndColorChooser();
+ }
+ 
+ namespace chrome {