Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814683005:
[Mac] Support for Touch Bar Customization (Closed)
|
Created:
3 years, 8 months ago by spqchan Modified:
3 years, 8 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, srahim+watch_chromium.org, danakj+watch_chromium.org, mac-reviews_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Support for Touch Bar Customization
Implement customization support for the default touch
bar. Changed the item identifiers to reverse-DNS style format.
BUG=710100
Review-Url: https://codereview.chromium.org/2814683005
Cr-Commit-Position: refs/heads/master@{#464967}
Committed: https://chromium.googlesource.com/chromium/src/+/1ad3c00f56fbf19ecba52bdf0591ca7ad1098d3a
Patch Set 1 : . #Patch Set 2 : Fixed the search button width #Patch Set 3 : Cleaned up #
Total comments: 5
Patch Set 4 : Fix for rsesek #
Total comments: 4
Patch Set 5 : Fix for rsesek #Patch Set 6 : Use BaseBundleId() #
Total comments: 10
Patch Set 7 : Fix for rsesek #Patch Set 8 : Fix the identifier format #
Messages
Total messages: 52 (33 generated)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Mac] Customization Support for Touch Bar BUG= ========== to ========== [Mac] Support for Touch Bar Customization BUG=710100 ==========
Description was changed from ========== [Mac] Support for Touch Bar Customization BUG=710100 ========== to ========== [Mac] Support for Touch Bar Customization Implement customization support for the default touch bar. Changed the item identifiers to reverse-DNS style format. BUG=710100 ==========
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:745: [NSApplication sharedApplication].automaticCustomizeTouchBarMenuItemEnabled = Won't this crash on anything <10.12.1? https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:57: @"com.google.chrome.browser-window"; Why the change to reverse-domain notation?
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:745: [NSApplication sharedApplication].automaticCustomizeTouchBarMenuItemEnabled = On 2017/04/13 18:45:32, Robert Sesek wrote: > Won't this crash on anything <10.12.1? Good point, I added in the fix https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:57: @"com.google.chrome.browser-window"; On 2017/04/13 18:45:32, Robert Sesek wrote: > Why the change to reverse-domain notation? In this link: https://developer.apple.com/reference/appkit/nstouchbar "To make an NSTouchBar object eligible for customization, assign it a globally-unique customizationIdentifier identifier. For the identifier string, use reverse-DNS style, such as “com.company-name.app-name.alphanumeric-ID”."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2814683005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:57: @"com.google.chrome.browser-window"; On 2017/04/13 21:13:50, spqchan wrote: > On 2017/04/13 18:45:32, Robert Sesek wrote: > > Why the change to reverse-domain notation? > > In this link: https://developer.apple.com/reference/appkit/nstouchbar > > "To make an NSTouchBar object eligible for customization, assign it a > globally-unique customizationIdentifier identifier. For the identifier string, > use reverse-DNS style, such as “com.company-name.app-name.alphanumeric-ID”." Okay, then I'd actually recommend formatting with base::mac::BaseBundleID() instead of hard-coding the bundle ID. https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:747: (setAutomaticCustomizeTouchBarMenuItemEnabled:)]) { Did clang-format do this? It should *really* be broken after the :
I'm not familiar with formatting with base::mac::BaseBundleID(). Do you mean retrieving the bundle id string and then concatenating to it? https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:747: (setAutomaticCustomizeTouchBarMenuItemEnabled:)]) { On 2017/04/14 16:56:20, Robert Sesek wrote: > Did clang-format do this? It should *really* be broken after the : Yes it did, is there a way for us to fix clang-format for this case? I fixed the format
On 2017/04/14 18:02:19, spqchan wrote: > I'm not familiar with formatting with base::mac::BaseBundleID(). Do you mean > retrieving the bundle id string and then concatenating to it? Yes, sorry for not being clear. Concatenate or string-format the bundle ID with the identifiers. https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:747: (setAutomaticCustomizeTouchBarMenuItemEnabled:)]) { On 2017/04/14 18:02:19, spqchan wrote: > On 2017/04/14 16:56:20, Robert Sesek wrote: > > Did clang-format do this? It should *really* be broken after the : > > Yes it did, is there a way for us to fix clang-format for this case? I fixed the > format See the "Reporting problems" link here: https://chromium.googlesource.com/chromium/src/+/7cb9933f3452fb5dc17a74189b59...
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2814683005/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:747: (setAutomaticCustomizeTouchBarMenuItemEnabled:)]) { On 2017/04/14 18:22:17, Robert Sesek wrote: > On 2017/04/14 18:02:19, spqchan wrote: > > On 2017/04/14 16:56:20, Robert Sesek wrote: > > > Did clang-format do this? It should *really* be broken after the : > > > > Yes it did, is there a way for us to fix clang-format for this case? I fixed > the > > format > > See the "Reporting problems" link here: > https://chromium.googlesource.com/chromium/src/+/7cb9933f3452fb5dc17a74189b59... thanks! https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:104: NSString* CreateTouchBarId(NSString* const touch_bar_id) { I'm putting these methods here for now, but I think they would be better in mac_util. WDYT?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ nits https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:60: NSString* const kBackForwardTouchId = @"BACK-FWD"; CamelCase instead of all upper? https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:104: NSString* CreateTouchBarId(NSString* const touch_bar_id) { On 2017/04/14 22:46:57, spqchan wrote: > I'm putting these methods here for now, but I think they would be better in > mac_util. WDYT? Keeping them local to where they're used is definitely better than a util file. https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:104: NSString* CreateTouchBarId(NSString* const touch_bar_id) { Naming nit: The word "Create" implies strong ownership being returned, so I'd maybe name this and the function below "Get" to make it clear that the result is autoreleased. https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:107: return [NSString stringWithFormat:@"%@-%@", chrome_bundle_id, touch_bar_id]; Use dots instead of dashes? And on line 113.
https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:60: NSString* const kBackForwardTouchId = @"BACK-FWD"; On 2017/04/17 16:56:23, Robert Sesek wrote: > CamelCase instead of all upper? It's supposed to be upper, since the format is “com.company-name.app-name.alphanumeric-ID”. https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:104: NSString* CreateTouchBarId(NSString* const touch_bar_id) { On 2017/04/17 16:56:23, Robert Sesek wrote: > Naming nit: The word "Create" implies strong ownership being returned, so I'd > maybe name this and the function below "Get" to make it clear that the result is > autoreleased. Done. https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:104: NSString* CreateTouchBarId(NSString* const touch_bar_id) { On 2017/04/17 16:56:23, Robert Sesek wrote: > On 2017/04/14 22:46:57, spqchan wrote: > > I'm putting these methods here for now, but I think they would be better in > > mac_util. WDYT? > > Keeping them local to where they're used is definitely better than a util file. I was planning on using it in other files in the future, but that's a bit of an overhead right now. https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:107: return [NSString stringWithFormat:@"%@-%@", chrome_bundle_id, touch_bar_id]; On 2017/04/17 16:56:23, Robert Sesek wrote: > Use dots instead of dashes? And on line 113. The format is “com.company-name.app-name.alphanumeric-ID”, so what's beyond the base bundle need to be in dashes
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2814683005/#ps160001 (title: "Fix for rsesek")
https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:107: return [NSString stringWithFormat:@"%@-%@", chrome_bundle_id, touch_bar_id]; On 2017/04/17 17:56:37, spqchan wrote: > On 2017/04/17 16:56:23, Robert Sesek wrote: > > Use dots instead of dashes? And on line 113. > > The format is “com.company-name.app-name.alphanumeric-ID”, so what's beyond the > base bundle need to be in dashes Is there a dot between the base bundle ID and the touchbar ID?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by rsesek@chromium.org
On 2017/04/17 17:57:54, Robert Sesek wrote: > https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): > > https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:107: return [NSString > stringWithFormat:@"%@-%@", chrome_bundle_id, touch_bar_id]; > On 2017/04/17 17:56:37, spqchan wrote: > > On 2017/04/17 16:56:23, Robert Sesek wrote: > > > Use dots instead of dashes? And on line 113. > > > > The format is “com.company-name.app-name.alphanumeric-ID”, so what's beyond > the > > base bundle need to be in dashes > > Is there a dot between the base bundle ID and the touchbar ID? Ah, I see what you mean. Thanks for catching it! I'll go ahead and fix it
On 2017/04/17 18:13:13, spqchan wrote: > On 2017/04/17 17:57:54, Robert Sesek wrote: > > > https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... > > File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): > > > > > https://codereview.chromium.org/2814683005/diff/140001/chrome/browser/ui/coco... > > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:107: return [NSString > > stringWithFormat:@"%@-%@", chrome_bundle_id, touch_bar_id]; > > On 2017/04/17 17:56:37, spqchan wrote: > > > On 2017/04/17 16:56:23, Robert Sesek wrote: > > > > Use dots instead of dashes? And on line 113. > > > > > > The format is “com.company-name.app-name.alphanumeric-ID”, so what's beyond > > the > > > base bundle need to be in dashes > > > > Is there a dot between the base bundle ID and the touchbar ID? > > Ah, I see what you mean. Thanks for catching it! I'll go ahead and fix it PTAL
lgtm
On 2017/04/17 18:22:23, Robert Sesek wrote: > lgtm thanks!
The CQ bit was checked by spqchan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492453524840560,
"parent_rev": "faf4c1d6b393e0d8e0b15d970d23294f0b7aa8b5", "commit_rev":
"1ad3c00f56fbf19ecba52bdf0591ca7ad1098d3a"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Support for Touch Bar Customization Implement customization support for the default touch bar. Changed the item identifiers to reverse-DNS style format. BUG=710100 ========== to ========== [Mac] Support for Touch Bar Customization Implement customization support for the default touch bar. Changed the item identifiers to reverse-DNS style format. BUG=710100 Review-Url: https://codereview.chromium.org/2814683005 Cr-Commit-Position: refs/heads/master@{#464967} Committed: https://chromium.googlesource.com/chromium/src/+/1ad3c00f56fbf19ecba52bdf0591... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/1ad3c00f56fbf19ecba52bdf0591...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:180001) has been created in https://codereview.chromium.org/2831713002/ by spqchan@chromium.org. The reason for reverting is: Broke the 10.12 build.
Message was sent while issue was closed.
I believe this broke non-sierra builds. I'm on 10.11.6 and get the following: ../../chrome/browser/ui/cocoa/browser_window_touch_bar.mm:237:32: error: 'NSTouchBarItemIdentifierFlexibleSpace' is only available on macOS 10_12_2 or newer [-Werror,-Wunguarded-availability] [customIdentifiers addObject:NSTouchBarItemIdentifierFlexibleSpace]; |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
