Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695493002:
[Mac] Touch Bar support for default browser window state (Closed)
Description[Mac] Touch Bar support for default browser window state
BUG=675254
Review-Url: https://codereview.chromium.org/2695493002
Cr-Commit-Position: refs/heads/master@{#451448}
Committed: https://chromium.googlesource.com/chromium/src/+/4ad0a609fea4c17f2b2d71e446bfae18c956b243
Patch Set 1 #Patch Set 2 : fixes and nits #
Total comments: 31
Patch Set 3 : Fix for rsesek and added a test #Patch Set 4 : Rebased #Patch Set 5 : fixed build error #Patch Set 6 : Remove test #
Total comments: 2
Patch Set 7 : Fix for rsesek #Patch Set 8 : Tests are back~ #
Total comments: 6
Patch Set 9 : Fix for rsesek 3 #
Total comments: 6
Patch Set 10 : Renamed vector icons #
Total comments: 4
Patch Set 11 : Renamed icons #
Total comments: 6
Patch Set 12 : Fixes for estade, remove the google search icon #
Total comments: 16
Patch Set 13 : Fix for estade 2 #
Total comments: 2
Patch Set 14 : Rebased #Messages
Total messages: 125 (87 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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
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] Default Touch Bar BUG= ========== to ========== [Mac] Touch Bar support for default browser window state BUG=675254 ==========
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
Patchset #2 (id:80001) has been deleted
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2695493002/diff/100001/chrome/app/vector_icon... File chrome/app/vector_icons/new_tab.icon (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/app/vector_icon... chrome/app/vector_icons/new_tab.icon:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:390: touchBar_.reset([[BrowserWindowTouchBar alloc] initWithBrowser:browser]); Can we avoid creating this if it's not necessary? Maybe a +newWithBrowser: method that can return nil? https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.h (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.h:4: Header guards. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.h:5: #include <Cocoa/Cocoa.h> #import https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. How about a unit test? https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:76: } // end namespace Just "namespace" https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:174: TemplateURLService* template_url_service = naming: templateUrlService https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:187: gfx::CreateVectorIcon(iconId, kTouchBarIconSize, Should this use CreateNSImageFromId? https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:213: NSButton* button = (NSButton*)sender; No C-style casts. Use ObjCCast or just do [button tag] instead. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/framed_browser_window.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/framed_browser_window.mm:506: static_cast<BrowserWindowController*>([self windowController]); Use ObjCCastStrict instead. https://codereview.chromium.org/2695493002/diff/100001/ui/gfx/vector_icons/g_... File ui/gfx/vector_icons/g_search.icon (right): https://codereview.chromium.org/2695493002/diff/100001/ui/gfx/vector_icons/g_... ui/gfx/vector_icons/g_search.icon:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2695493002/diff/100001/chrome/app/vector_icon... File chrome/app/vector_icons/new_tab.icon (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/app/vector_icon... chrome/app/vector_icons/new_tab.icon:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/14 17:32:50, Robert Sesek wrote: > 2017 Done. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:390: touchBar_.reset([[BrowserWindowTouchBar alloc] initWithBrowser:browser]); On 2017/02/14 17:32:50, Robert Sesek wrote: > Can we avoid creating this if it's not necessary? Maybe a +newWithBrowser: > method that can return nil? I moved things around so it's only created when it's used. I'm not too sure what you mean with the +newWithBrowser: method. Can you clarify? https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.h (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/14 17:32:51, Robert Sesek wrote: > 2017 Done. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.h:4: On 2017/02/14 17:32:50, Robert Sesek wrote: > Header guards. Done. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.h:5: #include <Cocoa/Cocoa.h> On 2017/02/14 17:32:51, Robert Sesek wrote: > #import Done. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/14 17:32:51, Robert Sesek wrote: > How about a unit test? I tried to write some tests, but they're currently flaky. Do you mind if I add them in a follow up CL? I want to get this landed before the feature freeze https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:76: } // end namespace On 2017/02/14 17:32:51, Robert Sesek wrote: > Just "namespace" Done. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:174: TemplateURLService* template_url_service = On 2017/02/14 17:32:51, Robert Sesek wrote: > naming: templateUrlService Done. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:187: gfx::CreateVectorIcon(iconId, kTouchBarIconSize, On 2017/02/14 17:32:51, Robert Sesek wrote: > Should this use CreateNSImageFromId? No, iconId is type VectorIconId, which is different from VectorIcon https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:213: NSButton* button = (NSButton*)sender; On 2017/02/14 17:32:51, Robert Sesek wrote: > No C-style casts. Use ObjCCast or just do [button tag] instead. Done. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/framed_browser_window.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/framed_browser_window.mm:506: static_cast<BrowserWindowController*>([self windowController]); On 2017/02/14 17:32:51, Robert Sesek wrote: > Use ObjCCastStrict instead. Done. https://codereview.chromium.org/2695493002/diff/100001/ui/gfx/vector_icons/g_... File ui/gfx/vector_icons/g_search.icon (right): https://codereview.chromium.org/2695493002/diff/100001/ui/gfx/vector_icons/g_... ui/gfx/vector_icons/g_search.icon:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/14 17:32:51, Robert Sesek wrote: > 2017 Done.
https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:390: touchBar_.reset([[BrowserWindowTouchBar alloc] initWithBrowser:browser]); On 2017/02/16 15:25:51, spqchan wrote: > On 2017/02/14 17:32:50, Robert Sesek wrote: > > Can we avoid creating this if it's not necessary? Maybe a +newWithBrowser: > > method that can return nil? > > I moved things around so it's only created when it's used. > I'm not too sure what you mean with the +newWithBrowser: method. Can you > clarify? I meant create a convenience class constructor rather than use a dedicated initializer. But what you did is also fine. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/16 15:25:51, spqchan wrote: > On 2017/02/14 17:32:51, Robert Sesek wrote: > > How about a unit test? > > I tried to write some tests, but they're currently flaky. Do you mind if I add > them in a follow up CL? I want to get this landed before the feature freeze If the test is flaky, doesn't that mean that the underlying code could have a bug in it? https://codereview.chromium.org/2695493002/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2695493002/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:1009: if (touchBar_) { Omit the nil checks. Messaging nil is fine.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/16 18:19:38, Robert Sesek wrote: > On 2017/02/16 15:25:51, spqchan wrote: > > On 2017/02/14 17:32:51, Robert Sesek wrote: > > > How about a unit test? > > > > I tried to write some tests, but they're currently flaky. Do you mind if I add > > them in a follow up CL? I want to get this landed before the feature freeze > > If the test is flaky, doesn't that mean that the underlying code could have a > bug in it? Sorry, I should've clarified. It's more like I'm trying to get the tests to only run on > OSX 10.12.1. For some reason, the tests are running on 10.9. All tests fail on that because NSTouchBar isn't available there, which causes everything to return nil. I'm most likely doing something dumb though so it probably won't take too long to figure out the issue, but I want to prioritize landing this CL in ASAP before tomorrow's feature freeze. https://codereview.chromium.org/2695493002/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2695493002/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:1009: if (touchBar_) { On 2017/02/16 18:19:39, Robert Sesek wrote: > Omit the nil checks. Messaging nil is fine. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/16 18:35:42, spqchan wrote: > On 2017/02/16 18:19:38, Robert Sesek wrote: > > On 2017/02/16 15:25:51, spqchan wrote: > > > On 2017/02/14 17:32:51, Robert Sesek wrote: > > > > How about a unit test? > > > > > > I tried to write some tests, but they're currently flaky. Do you mind if I > add > > > them in a follow up CL? I want to get this landed before the feature freeze > > > > If the test is flaky, doesn't that mean that the underlying code could have a > > bug in it? > > Sorry, I should've clarified. It's more like I'm trying to get the tests to only > run on > OSX 10.12.1. For some reason, the tests are running on 10.9. All tests > fail on that because NSTouchBar isn't available there, which causes everything > to return nil. > > I'm most likely doing something dumb though so it probably won't take too long > to figure out the issue, but I want to prioritize landing this CL in ASAP before > tomorrow's feature freeze. Looking at the trybot failures from PS5, this is the error: "+[NSButton buttonWithImage:target:action:]: unrecognized selector sent to class 0x7fff761eef28". And that's because in -touchBar:makeItemForIdentifier:, while touchBarItem is nil on 10.9, some of the branches can still call CreateTouchBarButton(). That method on NSButton is only in the 10.12 SDK. You could either not use that NSButton method, or just add an early return to -touchBar:makeItemForIdentifier: if touchBarItem is nil. Also, you can gate the test body on base::mac::IsAtLeastOS10_12().
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/16 18:43:22, Robert Sesek wrote: > On 2017/02/16 18:35:42, spqchan wrote: > > On 2017/02/16 18:19:38, Robert Sesek wrote: > > > On 2017/02/16 15:25:51, spqchan wrote: > > > > On 2017/02/14 17:32:51, Robert Sesek wrote: > > > > > How about a unit test? > > > > > > > > I tried to write some tests, but they're currently flaky. Do you mind if I > > add > > > > them in a follow up CL? I want to get this landed before the feature > freeze > > > > > > If the test is flaky, doesn't that mean that the underlying code could have > a > > > bug in it? > > > > Sorry, I should've clarified. It's more like I'm trying to get the tests to > only > > run on > OSX 10.12.1. For some reason, the tests are running on 10.9. All > tests > > fail on that because NSTouchBar isn't available there, which causes everything > > to return nil. > > > > I'm most likely doing something dumb though so it probably won't take too long > > to figure out the issue, but I want to prioritize landing this CL in ASAP > before > > tomorrow's feature freeze. > > Looking at the trybot failures from PS5, this is the error: "+[NSButton > buttonWithImage:target:action:]: unrecognized selector sent to class > 0x7fff761eef28". And that's because in -touchBar:makeItemForIdentifier:, while > touchBarItem is nil on 10.9, some of the branches can still call > CreateTouchBarButton(). That method on NSButton is only in the 10.12 SDK. > > You could either not use that NSButton method, or just add an early return to > -touchBar:makeItemForIdentifier: if touchBarItem is nil. > > Also, you can gate the test body on base::mac::IsAtLeastOS10_12(). Thanks! I was trying to use that a while ago, but there wasn't a method for IsAtLeastOS10_12_1(). For some reason, MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_12_1 didn't work either However, I just realized that I can just get around that by using !IsAtMostOS10_12() :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for re-adding the test. https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/16 19:05:23, spqchan wrote: > On 2017/02/16 18:43:22, Robert Sesek wrote: > > On 2017/02/16 18:35:42, spqchan wrote: > > > On 2017/02/16 18:19:38, Robert Sesek wrote: > > > > On 2017/02/16 15:25:51, spqchan wrote: > > > > > On 2017/02/14 17:32:51, Robert Sesek wrote: > > > > > > How about a unit test? > > > > > > > > > > I tried to write some tests, but they're currently flaky. Do you mind if > I > > > add > > > > > them in a follow up CL? I want to get this landed before the feature > > freeze > > > > > > > > If the test is flaky, doesn't that mean that the underlying code could > have > > a > > > > bug in it? > > > > > > Sorry, I should've clarified. It's more like I'm trying to get the tests to > > only > > > run on > OSX 10.12.1. For some reason, the tests are running on 10.9. All > > tests > > > fail on that because NSTouchBar isn't available there, which causes > everything > > > to return nil. > > > > > > I'm most likely doing something dumb though so it probably won't take too > long > > > to figure out the issue, but I want to prioritize landing this CL in ASAP > > before > > > tomorrow's feature freeze. > > > > Looking at the trybot failures from PS5, this is the error: "+[NSButton > > buttonWithImage:target:action:]: unrecognized selector sent to class > > 0x7fff761eef28". And that's because in -touchBar:makeItemForIdentifier:, while > > touchBarItem is nil on 10.9, some of the branches can still call > > CreateTouchBarButton(). That method on NSButton is only in the 10.12 SDK. > > > > You could either not use that NSButton method, or just add an early return to > > -touchBar:makeItemForIdentifier: if touchBarItem is nil. > > > > Also, you can gate the test body on base::mac::IsAtLeastOS10_12(). > > Thanks! I was trying to use that a while ago, but there wasn't a method for > IsAtLeastOS10_12_1(). > For some reason, MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_12_1 > didn't work either > However, I just realized that I can just get around that by using > !IsAtMostOS10_12() :) Does the IsAtMostOS10_12() check let the test run on 10.12? I'd probably just use IsAtLeastOS10_12() and not worry about 10.12.0. https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm (right): https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm:34: DCHECK([touchBarItemIds containsObject:@"BackForwardTouchId"]); Change DCHECK -> EXPECT_TRUE https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm:51: DCHECK_EQ(IDC_RELOAD, [[item view] tag]); DCHECK_EQ -> EXPECT_EQ https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm:57: DCHECK_EQ(IDC_STOP, [[item view] tag]); Same.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/16 19:13:38, Robert Sesek wrote: > On 2017/02/16 19:05:23, spqchan wrote: > > On 2017/02/16 18:43:22, Robert Sesek wrote: > > > On 2017/02/16 18:35:42, spqchan wrote: > > > > On 2017/02/16 18:19:38, Robert Sesek wrote: > > > > > On 2017/02/16 15:25:51, spqchan wrote: > > > > > > On 2017/02/14 17:32:51, Robert Sesek wrote: > > > > > > > How about a unit test? > > > > > > > > > > > > I tried to write some tests, but they're currently flaky. Do you mind > if > > I > > > > add > > > > > > them in a follow up CL? I want to get this landed before the feature > > > freeze > > > > > > > > > > If the test is flaky, doesn't that mean that the underlying code could > > have > > > a > > > > > bug in it? > > > > > > > > Sorry, I should've clarified. It's more like I'm trying to get the tests > to > > > only > > > > run on > OSX 10.12.1. For some reason, the tests are running on 10.9. All > > > tests > > > > fail on that because NSTouchBar isn't available there, which causes > > everything > > > > to return nil. > > > > > > > > I'm most likely doing something dumb though so it probably won't take too > > long > > > > to figure out the issue, but I want to prioritize landing this CL in ASAP > > > before > > > > tomorrow's feature freeze. > > > > > > Looking at the trybot failures from PS5, this is the error: "+[NSButton > > > buttonWithImage:target:action:]: unrecognized selector sent to class > > > 0x7fff761eef28". And that's because in -touchBar:makeItemForIdentifier:, > while > > > touchBarItem is nil on 10.9, some of the branches can still call > > > CreateTouchBarButton(). That method on NSButton is only in the 10.12 SDK. > > > > > > You could either not use that NSButton method, or just add an early return > to > > > -touchBar:makeItemForIdentifier: if touchBarItem is nil. > > > > > > Also, you can gate the test body on base::mac::IsAtLeastOS10_12(). > > > > Thanks! I was trying to use that a while ago, but there wasn't a method for > > IsAtLeastOS10_12_1(). > > For some reason, MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_12_1 > > didn't work either > > However, I just realized that I can just get around that by using > > !IsAtMostOS10_12() :) > > Does the IsAtMostOS10_12() check let the test run on 10.12? I'd probably just > use IsAtLeastOS10_12() and not worry about 10.12.0. Done. https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm (right): https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm:34: DCHECK([touchBarItemIds containsObject:@"BackForwardTouchId"]); On 2017/02/16 19:13:39, Robert Sesek wrote: > Change DCHECK -> EXPECT_TRUE Done. https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm:51: DCHECK_EQ(IDC_RELOAD, [[item view] tag]); On 2017/02/16 19:13:38, Robert Sesek wrote: > DCHECK_EQ -> EXPECT_EQ Done. https://codereview.chromium.org/2695493002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm:57: DCHECK_EQ(IDC_STOP, [[item view] tag]); On 2017/02/16 19:13:39, Robert Sesek wrote: > Same. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
On 2017/02/16 19:32:50, Robert Sesek wrote: > LGTM thanks!
The CQ bit was unchecked by spqchan@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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...
On 2017/02/16 20:36:50, commit-bot: I haz the power wrote: > 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...) I think you need OWNERS for the new vector_icons.
On 2017/02/16 21:50:07, Robert Sesek wrote: > On 2017/02/16 20:36:50, commit-bot: I haz the power wrote: > > 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...) > > I think you need OWNERS for the new vector_icons. Augh right. Thanks!
spqchan@chromium.org changed reviewers: + ben@chromium.org
+ben for chrome/app/vector_icons/ and ui/gfx/vector_icons/
spqchan@chromium.org changed reviewers: + sky@chromium.org - ben@chromium.org
+sky for chrome/app/vector_icons/ and ui/gfx/vector_icons/
The CQ bit was unchecked by commit-bot@chromium.org
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...)
sky@chromium.org changed reviewers: + estade@chromium.org
+estade https://codereview.chromium.org/2695493002/diff/240001/ui/gfx/vector_icons/g_... File ui/gfx/vector_icons/g_search.icon (right): https://codereview.chromium.org/2695493002/diff/240001/ui/gfx/vector_icons/g_... ui/gfx/vector_icons/g_search.icon:5: CANVAS_DIMENSIONS, 36, The name of this file isn't clear. What is g_search? If this is google specific artwork, then I suspect it should be in src-internal. Evan was going to figure that out though.
https://codereview.chromium.org/2695493002/diff/240001/chrome/app/vector_icon... File chrome/app/vector_icons/new_tab.icon (right): https://codereview.chromium.org/2695493002/diff/240001/chrome/app/vector_icon... chrome/app/vector_icons/new_tab.icon:5: CANVAS_DIMENSIONS, 36, Sorry, one more question. Is this the new tab icon in the tabstrip? If not, what is it? Maybe the name should make it clearer.
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/2695493002/diff/240001/chrome/app/vector_icon... File chrome/app/vector_icons/new_tab.icon (right): https://codereview.chromium.org/2695493002/diff/240001/chrome/app/vector_icon... chrome/app/vector_icons/new_tab.icon:5: CANVAS_DIMENSIONS, 36, On 2017/02/17 16:52:03, sky wrote: > Sorry, one more question. Is this the new tab icon in the tabstrip? If not, what > is it? Maybe the name should make it clearer. This new tab icon is for the Mac's touch bar. Clicking on it will open a new tab. I renamed it so hopefully it's a bit better https://codereview.chromium.org/2695493002/diff/240001/ui/gfx/vector_icons/g_... File ui/gfx/vector_icons/g_search.icon (right): https://codereview.chromium.org/2695493002/diff/240001/ui/gfx/vector_icons/g_... ui/gfx/vector_icons/g_search.icon:5: CANVAS_DIMENSIONS, 36, On 2017/02/17 16:50:59, sky wrote: > The name of this file isn't clear. What is g_search? If this is google specific > artwork, then I suspect it should be in src-internal. Evan was going to figure > that out though. I renamed it to google_search_touchbar. This is used for the "Search" touch bar button when the default search engine is Google
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
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...
https://codereview.chromium.org/2695493002/diff/240001/chrome/app/vector_icon... File chrome/app/vector_icons/new_tab.icon (right): https://codereview.chromium.org/2695493002/diff/240001/chrome/app/vector_icon... chrome/app/vector_icons/new_tab.icon:5: CANVAS_DIMENSIONS, 36, On 2017/02/17 17:50:31, spqchan wrote: > On 2017/02/17 16:52:03, sky wrote: > > Sorry, one more question. Is this the new tab icon in the tabstrip? If not, > what > > is it? Maybe the name should make it clearer. > > This new tab icon is for the Mac's touch bar. Clicking on it will open a new > tab. I renamed it so hopefully it's a bit better Please rename this so that's clear, e.g. touch_bar_new_tab or new_tab_touchbar. https://codereview.chromium.org/2695493002/diff/240001/ui/gfx/vector_icons/g_... File ui/gfx/vector_icons/g_search.icon (right): https://codereview.chromium.org/2695493002/diff/240001/ui/gfx/vector_icons/g_... ui/gfx/vector_icons/g_search.icon:5: CANVAS_DIMENSIONS, 36, On 2017/02/17 17:50:31, spqchan wrote: > On 2017/02/17 16:50:59, sky wrote: > > The name of this file isn't clear. What is g_search? If this is google > specific > > artwork, then I suspect it should be in src-internal. Evan was going to figure > > that out though. > > I renamed it to google_search_touchbar. This is used for the "Search" touch bar > button when the default search engine is Google Thanks for the rename, that makes it clearer. One more question though. Why do you need this in ui? AFAICT it's only used in chrome. https://codereview.chromium.org/2695493002/diff/260001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/260001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:38: "new_tab_touchbar.icon", Include only on mac? https://codereview.chromium.org/2695493002/diff/260001/ui/gfx/vector_icons/BU... File ui/gfx/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/260001/ui/gfx/vector_icons/BU... ui/gfx/vector_icons/BUILD.gn:35: "google_search_touchbar.icon", Include only on mac?
PTAL https://codereview.chromium.org/2695493002/diff/260001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/260001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:38: "new_tab_touchbar.icon", On 2017/02/17 19:25:19, sky wrote: > Include only on mac? Done. https://codereview.chromium.org/2695493002/diff/260001/ui/gfx/vector_icons/BU... File ui/gfx/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/260001/ui/gfx/vector_icons/BU... ui/gfx/vector_icons/BUILD.gn:35: "google_search_touchbar.icon", On 2017/02/17 19:25:19, sky wrote: > Include only on mac? Done.
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...
https://codereview.chromium.org/2695493002/diff/280001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/280001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:38: "new_tab_mac_touchbar.icon", please only add if is_mac https://codereview.chromium.org/2695493002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:190: : gfx::VectorIconId::OMNIBOX_SEARCH; please use omnibox::kSearchIcon for this (it's the same icon --- OMNIBOX_SEARCH is only there temporarily until I can clean up a reference to it that's in ash). https://codereview.chromium.org/2695493002/diff/280001/ui/gfx/vector_icons/BU... File ui/gfx/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/280001/ui/gfx/vector_icons/BU... ui/gfx/vector_icons/BUILD.gn:35: "google_search_mac_touchbar.icon", this belongs next to the other one you're adding, i.e. in chrome/app, unless I'm missing something. wrt whether it should go in src-internal, I'm not sure --- haven't quite sorted that out yet. Sorry. It probably wouldn't hurt for you to add it in src-internal for now and we can move it back to src later if need be. You could also punt on this by just using the normal search icon always and then adding the g-specific one later (since you're worried about the feature freeze).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2695493002/diff/280001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/280001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:38: "new_tab_mac_touchbar.icon", On 2017/02/17 20:11:39, Evan Stade wrote: > please only add if is_mac Done. https://codereview.chromium.org/2695493002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:190: : gfx::VectorIconId::OMNIBOX_SEARCH; On 2017/02/17 20:11:39, Evan Stade wrote: > please use omnibox::kSearchIcon for this (it's the same icon --- OMNIBOX_SEARCH > is only there temporarily until I can clean up a reference to it that's in ash). Done. https://codereview.chromium.org/2695493002/diff/280001/ui/gfx/vector_icons/BU... File ui/gfx/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/280001/ui/gfx/vector_icons/BU... ui/gfx/vector_icons/BUILD.gn:35: "google_search_mac_touchbar.icon", On 2017/02/17 20:11:39, Evan Stade wrote: > this belongs next to the other one you're adding, i.e. in chrome/app, unless I'm > missing something. > > wrt whether it should go in src-internal, I'm not sure --- haven't quite sorted > that out yet. Sorry. It probably wouldn't hurt for you to add it in src-internal > for now and we can move it back to src later if need be. You could also punt on > this by just using the normal search icon always and then adding the g-specific > one later (since you're worried about the feature freeze). Sure thing, I'll just stick with the normal search icon for now.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
some nits https://codereview.chromium.org/2695493002/diff/300001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/300001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:38: nit: remove newline https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:28: #include "ui/gfx/vector_icons_public.h" don't think you need this https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:29: #include "ui/vector_icons/vector_icons.h" or this https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:50: const SkColor kTouchBarStarIconColor = gfx::kGoogleBlue500; You may want to pull this one from the native theme. I don't think it's a coincidence that this is the same blue as focus indicators and prominent buttons. The star that's in the omnibox does this with theme->GetSystemColor(ui::NativeTheme::kColorId_ProminentButtonColor) https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/bub... https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:53: const int kTouchBarIconSize = 16; this shouldn't be necessary. Aren't all the icons you're using already defined as 16dp (hence this would be the default if you left out that argument)? Generally, icons that are this size should not be scaled down, and if you were indeed using a larger icon and scaling down this would look bad. https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:59: NSImage* CreateNSImageFromId(const gfx::VectorIcon* icon, const ref also, the name (referencing "Id") no longer makes sense, I'd use CreateNSImageFromVectorIcon https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:67: NSButton* CreateTouchBarButton(const gfx::VectorIcon* icon, const ref https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:138: const gfx::VectorIcon* icon = const ref
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...
Patchset #13 (id:320001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2695493002/diff/300001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2695493002/diff/300001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:38: On 2017/02/17 20:52:57, Evan Stade wrote: > nit: remove newline Done. https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:28: #include "ui/gfx/vector_icons_public.h" On 2017/02/17 20:52:58, Evan Stade wrote: > don't think you need this Done. https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:29: #include "ui/vector_icons/vector_icons.h" On 2017/02/17 20:52:58, Evan Stade wrote: > or this I need it for kBackArrowIcon, etc https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:50: const SkColor kTouchBarStarIconColor = gfx::kGoogleBlue500; On 2017/02/17 20:52:58, Evan Stade wrote: > You may want to pull this one from the native theme. I don't think it's a > coincidence that this is the same blue as focus indicators and prominent > buttons. The star that's in the omnibox does this with > > theme->GetSystemColor(ui::NativeTheme::kColorId_ProminentButtonColor) > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/bub... Done. https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:53: const int kTouchBarIconSize = 16; On 2017/02/17 20:52:58, Evan Stade wrote: > this shouldn't be necessary. Aren't all the icons you're using already defined > as 16dp (hence this would be the default if you left out that argument)? > Generally, icons that are this size should not be scaled down, and if you were > indeed using a larger icon and scaling down this would look bad. It looks like I need this, otherwise the new tab and search icons get really big. https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:59: NSImage* CreateNSImageFromId(const gfx::VectorIcon* icon, On 2017/02/17 20:52:57, Evan Stade wrote: > const ref > > also, the name (referencing "Id") no longer makes sense, I'd use > CreateNSImageFromVectorIcon Done. https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:67: NSButton* CreateTouchBarButton(const gfx::VectorIcon* icon, On 2017/02/17 20:52:57, Evan Stade wrote: > const ref Done. https://codereview.chromium.org/2695493002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:138: const gfx::VectorIcon* icon = On 2017/02/17 20:52:57, Evan Stade wrote: > const ref Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Now that the ui change is gone, I only looked at chrome/app/vector_icons. LGTM
On 2017/02/17 22:36:35, sky wrote: > Now that the ui change is gone, I only looked at chrome/app/vector_icons. LGTM Thanks!
The CQ bit was unchecked by spqchan@chromium.org
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/2695493002/#ps340001 (title: "Fix for estade 2")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2695493002/diff/340001/chrome/app/vector_icon... File chrome/app/vector_icons/new_tab_mac_touchbar.icon (right): https://codereview.chromium.org/2695493002/diff/340001/chrome/app/vector_icon... chrome/app/vector_icons/new_tab_mac_touchbar.icon:5: CANVAS_DIMENSIONS, 36, the new tab icon that I see on the bug was provided in a 60x60 version. Where did the 36 come from? Did you get this from go/icons ("add circle")? If so, the convention is to give it the generic name ("add_circle.icon"). But I don't think this is right, because you're scaling this down from 36 to 16 dip (32px for retina -- I don't know what scale factor the touchbar screen uses). All the other icons you're using are optimized for display at exactly 16/32px. This one is going to wind up with blurry lines because vector graphics scale well but don't guarantee that strokes are aligned to pixel boundaries. I think you need/want a designer to create a version optimized for 32px.
https://codereview.chromium.org/2695493002/diff/340001/chrome/app/vector_icon... File chrome/app/vector_icons/new_tab_mac_touchbar.icon (right): https://codereview.chromium.org/2695493002/diff/340001/chrome/app/vector_icon... chrome/app/vector_icons/new_tab_mac_touchbar.icon:5: CANVAS_DIMENSIONS, 36, On 2017/02/18 00:01:44, Evan Stade wrote: > the new tab icon that I see on the bug was provided in a 60x60 version. Where > did the 36 come from? Did you get this from go/icons ("add circle")? If so, the > convention is to give it the generic name ("add_circle.icon"). > > But I don't think this is right, because you're scaling this down from 36 to 16 > dip (32px for retina -- I don't know what scale factor the touchbar screen > uses). All the other icons you're using are optimized for display at exactly > 16/32px. This one is going to wind up with blurry lines because vector graphics > scale well but don't guarantee that strokes are aligned to pixel boundaries. I > think you need/want a designer to create a version optimized for 32px. The icon was strange. It was 60x60 vector, but the icon was actually 36x36 with enough padding to fill out the rest of the 60x60x. I got rid of the spacing and just ended up with this size. I'll reach for the designer and then get this fixed in a follow up CL. Thanks for letting me know :)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/app/vector_icons/BUILD.gn:
While running git apply --index -p1;
error: patch failed: chrome/app/vector_icons/BUILD.gn:57
error: chrome/app/vector_icons/BUILD.gn: patch does not apply
Patch: chrome/app/vector_icons/BUILD.gn
Index: chrome/app/vector_icons/BUILD.gn
diff --git a/chrome/app/vector_icons/BUILD.gn b/chrome/app/vector_icons/BUILD.gn
index
12f028a2205396e3e1a6bf0f8c2a9be0b50b4fd4..be30fe3a0d2f2ce874a1a5c29b3ef6053e7f2409
100644
--- a/chrome/app/vector_icons/BUILD.gn
+++ b/chrome/app/vector_icons/BUILD.gn
@@ -57,6 +57,10 @@ aggregate_vector_icons("chrome_vector_icons") {
"zoom_minus.icon",
"zoom_plus.icon",
]
+
+ if (is_mac) {
+ icons += [ "new_tab_mac_touchbar.icon" ]
+ }
}
source_set("vector_icons") {
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, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2695493002/#ps360001 (title: "Rebased")
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": 360001, "attempt_start_ts": 1487402340514170,
"parent_rev": "ecdfdf5b38b44e99023608dac72afa4f6d293d8a", "commit_rev":
"4ad0a609fea4c17f2b2d71e446bfae18c956b243"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Touch Bar support for default browser window state BUG=675254 ========== to ========== [Mac] Touch Bar support for default browser window state BUG=675254 Review-Url: https://codereview.chromium.org/2695493002 Cr-Commit-Position: refs/heads/master@{#451448} Committed: https://chromium.googlesource.com/chromium/src/+/4ad0a609fea4c17f2b2d71e446bf... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:360001) as https://chromium.googlesource.com/chromium/src/+/4ad0a609fea4c17f2b2d71e446bf... |
