Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(91)

Issue 2469213002: MacViews: TouchBar integration for toolkit-views dialogs. (Closed)

Created:
4 years, 1 month ago by tapted
Modified:
4 years, 1 month ago
CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MacViews: TouchBar integration for toolkit-views dialogs. Native Mac NSAlert dialogs show the dialog buttons on the TouchBar, we want similar behaviour for Chrome dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 Committed: https://crrev.com/c44c6a9a5b391a2081ed7259195ec1686482e2d0 Cr-Commit-Position: refs/heads/master@{#430209}

Patch Set 1 #

Patch Set 2 : no touch unless dialog #

Patch Set 3 : nit diff #

Patch Set 4 : fix upstream, comppile #

Patch Set 5 : I guess this is not needed either #

Patch Set 6 : it works! #

Patch Set 7 : Fix 10.10 SDK compile #

Patch Set 8 : nit comments #

Total comments: 3

Patch Set 9 : Split files. #

Patch Set 10 : close the client, not the delegate #

Total comments: 2

Patch Set 11 : update comment #

Total comments: 4

Patch Set 12 : : -> in #

Unified diffs Side-by-side diffs Delta from patch set Stats (+200 lines, -0 lines) Patch
M base/mac/sdk_forward_declarations.h View 1 2 3 4 5 6 7 8 1 chunk +14 lines, -0 lines 0 comments Download
M ui/base/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
A ui/base/cocoa/touch_bar_forward_declarations.h View 1 2 3 4 5 6 7 8 1 chunk +59 lines, -0 lines 0 comments Download
M ui/views/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
A ui/views/cocoa/bridged_content_view_touch_bar.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +125 lines, -0 lines 0 comments Download

Messages

Total messages: 51 (32 generated)
tapted
Hi Avi, am I holding it right? Probably I should put the crazy SDK-availability stuff ...
4 years, 1 month ago (2016-11-02 09:28:58 UTC) #8
pink (ping after 24hrs)
Are we going to be perpetually following around feature developers and adding this delegate method ...
4 years, 1 month ago (2016-11-02 14:13:59 UTC) #10
Avi (use Gerrit)
https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged_content_view.mm File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged_content_view.mm#newcode1527 ui/views/cocoa/bridged_content_view.mm:1527: [button setKeyEquivalent:@"\n"]; On 2016/11/02 09:28:57, tapted wrote: > I'd ...
4 years, 1 month ago (2016-11-02 14:50:41 UTC) #11
Avi (use Gerrit)
On 2016/11/02 09:28:58, tapted wrote: > Probably I should put the crazy SDK-availability stuff into ...
4 years, 1 month ago (2016-11-02 17:32:43 UTC) #12
tapted
I'll try firing up hopper today (although I'm build sheriff - maybe it will be ...
4 years, 1 month ago (2016-11-02 20:47:27 UTC) #13
tapted
PTAL - made touch_bar_forward_declarations.h and moved all the implementation into a category in bridged_content_view_touch_bar.mm -- ...
4 years, 1 month ago (2016-11-03 04:36:29 UTC) #17
tapted
On 2016/11/02 20:47:27, tapted wrote: > On 2016/11/02 14:13:59, pink wrote: > > Are we ...
4 years, 1 month ago (2016-11-03 08:04:58 UTC) #23
Avi (use Gerrit)
Rather cool, though I don't know the touchbar api or Views... https://codereview.chromium.org/2469213002/diff/180001/ui/views/cocoa/bridged_content_view_touch_bar.mm File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): ...
4 years, 1 month ago (2016-11-03 14:30:50 UTC) #24
tapted
+sky - this is all ObjC, but I want to make sure I'm using DialogDelegate/DialogClientView ...
4 years, 1 month ago (2016-11-03 23:43:58 UTC) #30
sky
Calling AcceptWindow/CancelWindow LGTM (I'm assuming the other reviewers are looking at the ObectiveC code).
4 years, 1 month ago (2016-11-03 23:53:52 UTC) #31
Avi (use Gerrit)
lgtm https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm#newcode57 ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ]) ...
4 years, 1 month ago (2016-11-04 16:41:25 UTC) #34
tapted
Thanks all! https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm#newcode57 ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ...
4 years, 1 month ago (2016-11-06 23:57:48 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2469213002/220001
4 years, 1 month ago (2016-11-06 23:58:20 UTC) #41
Avi (use Gerrit)
lgtm https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm#newcode57 ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ]) ...
4 years, 1 month ago (2016-11-07 00:59:54 UTC) #42
Avi (use Gerrit)
lgtm lgtm https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged_content_view_touch_bar.mm#newcode57 ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ...
4 years, 1 month ago (2016-11-07 00:59:54 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/326556)
4 years, 1 month ago (2016-11-07 01:14:59 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2469213002/220001
4 years, 1 month ago (2016-11-07 01:29:12 UTC) #47
commit-bot: I haz the power
Committed patchset #12 (id:220001)
4 years, 1 month ago (2016-11-07 02:30:09 UTC) #49
commit-bot: I haz the power
4 years, 1 month ago (2016-11-07 02:31:41 UTC) #51
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/c44c6a9a5b391a2081ed7259195ec1686482e2d0
Cr-Commit-Position: refs/heads/master@{#430209}

Powered by Google App Engine
This is Rietveld 408576698