fix: backport upstream fixes for color chooser dialogs (#17227)

* 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
This commit is contained in:
Heilig Benedek 2019-03-07 02:55:15 +01:00 коммит произвёл Robo
Родитель 2733697819
Коммит 5422fd9941
4 изменённых файлов: 280 добавлений и 24 удалений

Просмотреть файл

@ -60,7 +60,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_time_ticks_dcheck.patch
autofill_size_calculation.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

Просмотреть файл

@ -1,23 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Heilig Benedek <benecene@gmail.com>
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();

Просмотреть файл

@ -0,0 +1,186 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Benedek Heilig <benecene@gmail.com>
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 <bencoe@gmail.com>
Ben Fiola <benfiola@gmail.com>
Ben Karel <eschew@gmail.com>
Ben Noordhuis <ben@strongloop.com>
+Benedek Heilig <benecene@gmail.com>
Benjamin Dupont <bedupont@cisco.com>
Benjamin Jemlich <pcgod99@gmail.com>
Bernard Cafarelli <voyageur@gentoo.org>
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);
}

Просмотреть файл

@ -0,0 +1,92 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Benedek Heilig <benecene@gmail.com>
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 {