New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 856391 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Port WebTextfieldTouchBarController to MacViews

Project Member Reported by spqc...@chromium.org, Jun 26 2018

Issue description

Currently, only the default touch bar is available in MacViews. We'll need to port over credit card support and textfield suggestions
 
Labels: -Pri-1 Target-70 Pri-3
This is being punted to M70.
Labels: Group-Touch_Bar
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e95b7292bae44c8e6fb9af5801f102ae28587d6c

commit e95b7292bae44c8e6fb9af5801f102ae28587d6c
Author: spqchan <spqchan@chromium.org>
Date: Thu Jul 12 20:05:49 2018

[Mac] Refactor BrowserWindowTouchBarMac

Separate BrowserWindowTouchBarMac into two classes:
BrowserWindowDefaultTouchBar and
BrowserWindowTouchBarController.

BrowserWindowTouchBarController determines what
touch bar should be used for browser window.
BrowserWindowDefaultTouchBar creates a default
touch bar for the browser.

These changes are split from a bigger change to
hook up touch bar support for web textfield in
MacViews:

https://chromium-review.googlesource.com/c/chromium/src/+/1132597

Bug:  856391 
Change-Id: I9be31c65bd0acc2ec4d39b5e2f6b06fc30e36bf7
Reviewed-on: https://chromium-review.googlesource.com/1133705
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574693}
[modify] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/BUILD.gn
[delete] https://crrev.com/6678ca5e07d42a16c0e63b123543672ab94c3c29/chrome/browser/ui/browser_window_touch_bar_mac.h
[modify] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/browser_window_controller.h
[modify] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/browser_window_controller.mm
[modify] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/framed_browser_window.mm
[add] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/touchbar/browser_window_default_touch_bar.h
[rename] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/touchbar/browser_window_default_touch_bar.mm
[rename] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/touchbar/browser_window_default_touch_bar_unittest.mm
[add] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.h
[add] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.mm
[rename] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller_browsertest.mm
[modify] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/browser/ui/views/frame/browser_frame_mac.mm
[modify] https://crrev.com/e95b7292bae44c8e6fb9af5801f102ae28587d6c/chrome/test/BUILD.gn

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9da90b4c8ed7a7cc823c9bc3012658ca187b393

commit f9da90b4c8ed7a7cc823c9bc3012658ca187b393
Author: spqchan <spqchan@chromium.org>
Date: Wed Jul 25 16:49:54 2018

[MacViews] Hook Web Content Textfield Touch Bar Support

Remove Cocoa dependency from WebTextfieldTouchBar by moving
it from TabContentsController to BrowserWindowTouchBarController.

Hook up BrowserWindowTouchBarController to
AutofillPopupControllerImplMac on MacViews so that the
credit card touch bar can be updated.

Move the the logic that listens to WebContent changes to the
BrowserWindowTouchBarController. When the WebContents has
changed, BrowserWindowTouchBarController will update the
WebContents in BrowserWindowDefaultTouchBar and
WebTextfieldTouchBarController.

Modified SuggestedTextTouchBarController so that when its
WebContents has changed, it'll observe the new WebContents.

Bug:  856391 
Change-Id: I9c29a371a0b38eb608bf8e32521abe22d5c664e0
Reviewed-on: https://chromium-review.googlesource.com/1135829
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577940}
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/autofill/autofill_popup_controller_impl_mac.mm
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/tabs/tab_strip_controller.h
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/touchbar/browser_window_default_touch_bar.mm
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.h
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.mm
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.h
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.mm
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.h
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.mm
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/views/frame/browser_frame_mac.h
[modify] https://crrev.com/f9da90b4c8ed7a7cc823c9bc3012658ca187b393/chrome/browser/ui/views/frame/browser_frame_mac.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98d49376a8fa7f03e07aa2c6a3ee202b6214db54

commit 98d49376a8fa7f03e07aa2c6a3ee202b6214db54
Author: Derek Cheng <imcheng@chromium.org>
Date: Wed Jul 25 19:29:37 2018

Revert "[MacViews] Hook Web Content Textfield Touch Bar Support"

This reverts commit f9da90b4c8ed7a7cc823c9bc3012658ca187b393.

Reason for revert: Broke Mac10.11 Tests: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.11%20Tests/28261

Test failures:

BrowserWindowTouchBarControllerTest.SearchEngineChanges
BrowserWindowTouchBarControllerTest.DestroyNotificationBridge
BrowserWindowTouchBarControllerTest.TabChanges



Original change's description:
> [MacViews] Hook Web Content Textfield Touch Bar Support
> 
> Remove Cocoa dependency from WebTextfieldTouchBar by moving
> it from TabContentsController to BrowserWindowTouchBarController.
> 
> Hook up BrowserWindowTouchBarController to
> AutofillPopupControllerImplMac on MacViews so that the
> credit card touch bar can be updated.
> 
> Move the the logic that listens to WebContent changes to the
> BrowserWindowTouchBarController. When the WebContents has
> changed, BrowserWindowTouchBarController will update the
> WebContents in BrowserWindowDefaultTouchBar and
> WebTextfieldTouchBarController.
> 
> Modified SuggestedTextTouchBarController so that when its
> WebContents has changed, it'll observe the new WebContents.
> 
> Bug:  856391 
> Change-Id: I9c29a371a0b38eb608bf8e32521abe22d5c664e0
> Reviewed-on: https://chromium-review.googlesource.com/1135829
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Sarah Chan <spqchan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577940}

TBR=avi@chromium.org,spqchan@chromium.org

Change-Id: Ifffb93394919b40f05942eebbc3e87c07c69d7d5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  856391 
Reviewed-on: https://chromium-review.googlesource.com/1150248
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578015}
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/autofill/autofill_popup_controller_impl_mac.mm
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/tabs/tab_strip_controller.h
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/touchbar/browser_window_default_touch_bar.mm
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.h
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.mm
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.h
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.mm
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.h
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.mm
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/views/frame/browser_frame_mac.h
[modify] https://crrev.com/98d49376a8fa7f03e07aa2c6a3ee202b6214db54/chrome/browser/ui/views/frame/browser_frame_mac.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38ca7b11c3df03e49e29d49375492b32c30040b0

commit 38ca7b11c3df03e49e29d49375492b32c30040b0
Author: spqchan <spqchan@chromium.org>
Date: Thu Jul 26 18:59:47 2018

[MacViews] Hook Web Content Textfield Touch Bar Support

Reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1135829

The previous CL got reverted because the touch bar is initialized
in the BrowserWindowTouchBarControllerTest, which causes issues
if the touch bar API is not available. This CL addresses that issue
by only creating the BrowserWindowTouchBarController when
the API is available.

Remove Cocoa dependency from WebTextfieldTouchBar by moving
it from TabContentsController to BrowserWindowTouchBarController.

Hook up BrowserWindowTouchBarController to
AutofillPopupControllerImplMac on MacViews so that the
credit card touch bar can be updated.

Move the the logic that listens to WebContent changes to the
BrowserWindowTouchBarController. When the WebContents has
changed, BrowserWindowTouchBarController will update the
WebContents in BrowserWindowDefaultTouchBar and
WebTextfieldTouchBarController.

Modified SuggestedTextTouchBarController so that when its
WebContents has changed, it'll observe the new WebContents.

Bug:  856391 
Change-Id: Ic3dfb002ecec6574502c3f1a4cdd32e9edb713f4
Reviewed-on: https://chromium-review.googlesource.com/1135829
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#577940}
Reviewed-on: https://chromium-review.googlesource.com/1151613
Cr-Commit-Position: refs/heads/master@{#578390}
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/autofill/autofill_popup_controller_impl_mac.mm
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/tabs/tab_strip_controller.h
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/browser_window_default_touch_bar.mm
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.h
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.mm
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller_browsertest.mm
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.h
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.mm
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.h
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.mm
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/views/frame/browser_frame_mac.h
[modify] https://crrev.com/38ca7b11c3df03e49e29d49375492b32c30040b0/chrome/browser/ui/views/frame/browser_frame_mac.mm

Labels: Merge-Request-69
Tested on Canary and it's safe to merge
Project Member

Comment 8 by sheriffbot@chromium.org, Aug 1

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This P3, targeted to M70. Could you pls justify the merge for M69?
Also which CL you're requesting a merge to M69 for?
CL #6. This is a feature that is currently missing in MacViews. It would be great if we can have it with the MacViews launch
Cc: robliao@chromium.org ellyjo...@chromium.org
Thank you Sarah.

+ elly (also +rob per comment #1), are you comfortable taking this merge in for M69?

Summary for further triage:

Candidate Change: 38ca7b11c3df03e49e29d49375492b32c30040b0
Regression from M68: Yes

Behavior in M68 Cocoa: Credit Cards will show up in the touchbar
Behavior in M69 MacViews: Default touchbar items get displayed.

Usage of the feature in M68: Low

I'm inclined to let this arrive in M70, but will defer to ellyjones@ for the final decision. If there is additional data to support an M69 merge, I'm all ears.
Labels: -Merge-Review-69 Merge-Rejected-69
Mac TL declines merge: the change is too large and the feature is not critical enough.
Status: Fixed (was: Assigned)

Sign in to add a comment