Commit 06cbca85 authored by Sidney San Martín's avatar Sidney San Martín Committed by Commit Bot

[Mac] [Reland] On the Touch Bar, use individual back/forward buttons instead of semented control.

This relands the original change, fixing a use-after-free when
BrowserWindowDefaultTouchBar outlives its profile.

Original change's description:

> This is a design tweak which also enables the back and forward buttons
> to be placed separately. Existing customized Touch Bars which contain
> the grouped item still work; see the code comment for details.
>
> I used Cocoa bindings for the buttons' enabled states instead of holding
> pointers to them because I initially thought that the Touch Bar might allow the
> group and individual buttons to coexist. Turns out no, but I kept the KVO
> implementation because I like it.
>
> It also includes a couple of other changes that came up while writing
> this patch and during code review:
>
> - The unit tests no longer look for a hard-coded list of items in a
>   specific order. Instead, they verify that BrowserWindowDefaultTouchBar
>   can create the items it claims to be able to create, and that no
>   known-valid identifiers have stopped working.
>
> - BrowserWindowDefaultTouchBar returns nil if it doesn't know about an
>   identifier, instead of returning an empty Touch Bar item.
>
> Bug: 937935
> Change-Id: I8cdc6347bc667b0a24fa24fe2c6a56661747ac98
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1534238
> Commit-Queue: Sidney San Martín <sdy@chromium.org>
> Reviewed-by: Leonard Grey <lgrey@chromium.org>
> Auto-Submit: Sidney San Martín <sdy@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#649899}

Bug: 937935, 945772
Change-Id: I77bf9b6d14bef875f271f8fbd4468e6cdc30434e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1568846
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Reviewed-by: default avatarLeonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#651851}
parent 8f822dff
...@@ -14,10 +14,6 @@ class BookmarkTabHelperObserver; ...@@ -14,10 +14,6 @@ class BookmarkTabHelperObserver;
class Browser; class Browser;
@class BrowserWindowTouchBarController; @class BrowserWindowTouchBarController;
namespace content {
class WebContents;
}
// Provides a default touch bar for the browser window. This class implements // Provides a default touch bar for the browser window. This class implements
// the NSTouchBarDelegate and handles the items in the touch bar. // the NSTouchBarDelegate and handles the items in the touch bar.
API_AVAILABLE(macos(10.12.2)) API_AVAILABLE(macos(10.12.2))
...@@ -29,18 +25,18 @@ API_AVAILABLE(macos(10.12.2)) ...@@ -29,18 +25,18 @@ API_AVAILABLE(macos(10.12.2))
// True if the current page is starred. Used by star touch bar button. // True if the current page is starred. Used by star touch bar button.
@property(nonatomic, assign) BOOL isStarred; @property(nonatomic, assign) BOOL isStarred;
// Designated initializer. // True if the back button is enabled.
- (instancetype)initWithBrowser:(Browser*)browser @property(nonatomic, assign) BOOL canGoBack;
controller:(BrowserWindowTouchBarController*)controller;
// Creates and returns a touch bar for the browser window. // True if the forward button is enabled.
- (NSTouchBar*)makeTouchBar; @property(nonatomic, assign) BOOL canGoForward;
@property(nonatomic, assign) BrowserWindowTouchBarController* controller;
- (void)updateWebContents:(content::WebContents*)contents; @property(nonatomic) Browser* browser;
// Updates the back/forward button. Called when creating the touch bar or when // Creates and returns a touch bar for the browser window.
// the back and forward commands have changed. - (NSTouchBar*)makeTouchBar;
- (void)updateBackForwardControl;
- (BrowserWindowTouchBarController*)controller; - (BrowserWindowTouchBarController*)controller;
...@@ -49,6 +45,11 @@ API_AVAILABLE(macos(10.12.2)) ...@@ -49,6 +45,11 @@ API_AVAILABLE(macos(10.12.2))
// Private methods exposed for testing. // Private methods exposed for testing.
@interface BrowserWindowDefaultTouchBar (ExposedForTesting) @interface BrowserWindowDefaultTouchBar (ExposedForTesting)
@property(readonly, class) NSString* reloadOrStopItemIdentifier;
@property(readonly, class) NSString* backItemIdentifier;
@property(readonly, class) NSString* forwardItemIdentifier;
@property(readonly, class) NSString* fullscreenOriginItemIdentifier;
// Updates the reload/stop button. Called when creating the touch bar or the // Updates the reload/stop button. Called when creating the touch bar or the
// page load state has been updated. // page load state has been updated.
- (void)updateReloadStopButton; - (void)updateReloadStopButton;
...@@ -56,10 +57,6 @@ API_AVAILABLE(macos(10.12.2)) ...@@ -56,10 +57,6 @@ API_AVAILABLE(macos(10.12.2))
// Returns the reload/stop button on the touch bar. Creates it if it's null. // Returns the reload/stop button on the touch bar. Creates it if it's null.
- (NSButton*)reloadStopButton; - (NSButton*)reloadStopButton;
// Returns the back/forward segmented control on the touch bar. Creates it if
// it's null.
- (NSSegmentedControl*)backForwardControl;
// Returns the bridge object that BrowserWindowDefaultTouchBar uses to receive // Returns the bridge object that BrowserWindowDefaultTouchBar uses to receive
// notifications. // notifications.
- (BookmarkTabHelperObserver*)bookmarkTabObserver; - (BookmarkTabHelperObserver*)bookmarkTabObserver;
......
...@@ -32,9 +32,6 @@ API_AVAILABLE(macos(10.12.2)) ...@@ -32,9 +32,6 @@ API_AVAILABLE(macos(10.12.2))
// nil. // nil.
- (void)invalidateTouchBar; - (void)invalidateTouchBar;
- (void)updateWebContents:(content::WebContents*)contents;
- (content::WebContents*)webContents;
@end @end
@interface BrowserWindowTouchBarController (ExposedForTesting) @interface BrowserWindowTouchBarController (ExposedForTesting)
......
...@@ -19,66 +19,9 @@ ...@@ -19,66 +19,9 @@
#include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_observer.h"
#import "ui/base/cocoa/touch_bar_util.h" #import "ui/base/cocoa/touch_bar_util.h"
class API_AVAILABLE(macos(10.12.2)) WebContentsNotificationBridge
: public TabStripModelObserver,
public content::WebContentsObserver {
public:
WebContentsNotificationBridge(BrowserWindowTouchBarController* owner,
Browser* browser)
: owner_(owner), browser_(browser), contents_(nullptr) {
TabStripModel* model = browser_->tab_strip_model();
DCHECK(model);
model->AddObserver(this);
UpdateWebContents(model->GetActiveWebContents());
}
~WebContentsNotificationBridge() override {
TabStripModel* model = browser_->tab_strip_model();
if (model)
model->RemoveObserver(this);
}
void UpdateWebContents(content::WebContents* new_contents) {
contents_ = new_contents;
Observe(contents_);
[owner_ updateWebContents:contents_];
}
// TabStripModelObserver:
void OnTabStripModelChanged(
TabStripModel* tab_strip_model,
const TabStripModelChange& change,
const TabStripSelectionChange& selection) override {
if (tab_strip_model->empty() || !selection.active_tab_changed())
return;
UpdateWebContents(selection.new_contents);
contents_ = selection.new_contents;
}
content::WebContents* contents() const { return contents_; }
protected:
// WebContentsObserver:
void WebContentsDestroyed() override {
// Clean up if the web contents is being destroyed.
UpdateWebContents(nullptr);
}
private:
BrowserWindowTouchBarController* owner_; // Weak.
Browser* browser_; // Weak.
content::WebContents* contents_; // Weak.
};
@interface BrowserWindowTouchBarController () { @interface BrowserWindowTouchBarController () {
NSWindow* window_; // Weak. NSWindow* window_; // Weak.
// Used to receive and handle notifications.
std::unique_ptr<WebContentsNotificationBridge> notificationBridge_;
base::scoped_nsobject<BrowserWindowDefaultTouchBar> defaultTouchBar_; base::scoped_nsobject<BrowserWindowDefaultTouchBar> defaultTouchBar_;
base::scoped_nsobject<WebTextfieldTouchBarController> webTextfieldTouchBar_; base::scoped_nsobject<WebTextfieldTouchBarController> webTextfieldTouchBar_;
...@@ -92,12 +35,9 @@ class API_AVAILABLE(macos(10.12.2)) WebContentsNotificationBridge ...@@ -92,12 +35,9 @@ class API_AVAILABLE(macos(10.12.2)) WebContentsNotificationBridge
DCHECK(browser); DCHECK(browser);
window_ = window; window_ = window;
notificationBridge_ = defaultTouchBar_.reset([[BrowserWindowDefaultTouchBar alloc] init]);
std::make_unique<WebContentsNotificationBridge>(self, browser); defaultTouchBar_.get().controller = self;
defaultTouchBar_.get().browser = browser;
defaultTouchBar_.reset([[BrowserWindowDefaultTouchBar alloc]
initWithBrowser:browser
controller:self]);
webTextfieldTouchBar_.reset( webTextfieldTouchBar_.reset(
[[WebTextfieldTouchBarController alloc] initWithController:self]); [[WebTextfieldTouchBarController alloc] initWithController:self]);
} }
...@@ -105,6 +45,11 @@ class API_AVAILABLE(macos(10.12.2)) WebContentsNotificationBridge ...@@ -105,6 +45,11 @@ class API_AVAILABLE(macos(10.12.2)) WebContentsNotificationBridge
return self; return self;
} }
- (void)dealloc {
defaultTouchBar_.get().browser = nullptr;
[super dealloc];
}
- (void)invalidateTouchBar { - (void)invalidateTouchBar {
DCHECK([window_ respondsToSelector:@selector(setTouchBar:)]); DCHECK([window_ respondsToSelector:@selector(setTouchBar:)]);
[window_ performSelector:@selector(setTouchBar:) withObject:nil]; [window_ performSelector:@selector(setTouchBar:) withObject:nil];
...@@ -118,15 +63,6 @@ class API_AVAILABLE(macos(10.12.2)) WebContentsNotificationBridge ...@@ -118,15 +63,6 @@ class API_AVAILABLE(macos(10.12.2)) WebContentsNotificationBridge
return [defaultTouchBar_ makeTouchBar]; return [defaultTouchBar_ makeTouchBar];
} }
- (void)updateWebContents:(content::WebContents*)contents {
[defaultTouchBar_ updateWebContents:contents];
[self invalidateTouchBar];
}
- (content::WebContents*)webContents {
return notificationBridge_->web_contents();
}
@end @end
@implementation BrowserWindowTouchBarController (ExposedForTesting) @implementation BrowserWindowTouchBarController (ExposedForTesting)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment