[Mac][Material Design] Fix bookmark chevron button focus ring location.

[Mac][Material Design] Fix bookmark chevron button focus ring location.

This cl adjusts the bookmark bar's chevron button's focus ring, which
is 2pt too low.

R=tapted@chromium.org
BUG=617511
Committed: https://crrev.com/ce0f71bffa40beb738e2579efd91d3a0260dd010
Cr-Commit-Position: refs/heads/master@{#400309}

[shrike, 2016-06-15 21:58:16]

PTAL

[tapted, 2016-06-16 05:53:19]

I think there's a nice way to avoid the rtti. And.. it's probably fine, but is
there an explanation for why the focus rings are shifted in MD?

https://codereview.chromium.org/2074503002/diff/1/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2074503002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:380:
bookmarkBarViewControllerFromControlView:(NSView*)controlView {
having the button cell know much about its parents, and using rtti to find it,
probably breaks some kind of rule. I think the nice way to do this is to add a
property like `isOffTheSideButton()`, and have it return YES -- either by
setting a data member in bookmark_button_cell.mm's

// Used by the off-the-side menu, the only case where a
// BookmarkButtonCell is loaded from a nib.
- (void)awakeFromNib {
  [self configureBookmarkButtonCell];
}

(somewhat hacky)

OR having -[BookmarkBarController
configureOffTheSideButtonContentsAndVisibility] set it

OR creating a new subclass, just in bookmark_button_cell.mm like:

@interface OffTheSideButtonCell : BookmarkButtonCell
@end
@implementation
- (BOOL)isOffTheSideButton {
  return YES;
}
@end

then open BookmarkBar.xib in a text editor and change
customClass="BookmarkButtonCell" on line 25 to OffTheSideButtonCell

That's essentially the approach currently used by isFolderButtonCell - the only
downside is editing the .xib by hand, but this should be safe.

[shrike, 2016-06-16 15:19:09]

PTAL

https://codereview.chromium.org/2074503002/diff/1/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2074503002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:380:
bookmarkBarViewControllerFromControlView:(NSView*)controlView {
On 2016/06/16 05:53:19, tapted wrote:
> OR creating a new subclass, just in bookmark_button_cell.mm like:
> 
> @interface OffTheSideButtonCell : BookmarkButtonCell
> @end
> @implementation
> - (BOOL)isOffTheSideButton {
>   return YES;
> }
> @end
> 
> then open BookmarkBar.xib in a text editor and change
> customClass="BookmarkButtonCell" on line 25 to OffTheSideButtonCell
> 
> That's essentially the approach currently used by isFolderButtonCell - the
only
> downside is editing the .xib by hand, but this should be safe.

Great! My original preference was to change the cell class but I didn't think of
a way to do that outside of IB.

I am also unsure why we need a 2pt shift. My understanding of how the focus ring
drawing works is that the Appkit builds some kind of path based on the graphics
commands you execute during a certain window of time. That means the focus ring
should follow the bitmap.

[tapted, 2016-06-16 21:04:27]

lgtm % nit

https://codereview.chromium.org/2074503002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2074503002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:11: #import
"chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h"
nit: remove these 2 added imports

[shrike, 2016-06-16 22:52:03]

https://codereview.chromium.org/2074503002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2074503002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:11: #import
"chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h"
On 2016/06/16 21:04:26, tapted wrote:
> nit: remove these 2 added imports

Done.

[shrike, 2016-06-16 22:52:07]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-06-16 22:52:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2074503002/#ps60001
(title: "Remove unneeded includes.")

[commit-bot: I haz the power, 2016-06-16 22:52:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2074503002/60001

[commit-bot: I haz the power, 2016-06-17 00:15:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-17 00:16:17]

Description was changed from

==========
[Mac][Material Design] Fix bookmark chevron button focus ring location.

This cl adjusts the bookmark bar's chevron button's focus ring, which
is 2pt too low.

R=tapted@chromium.org
BUG=617511
==========

to

==========
[Mac][Material Design] Fix bookmark chevron button focus ring location.

This cl adjusts the bookmark bar's chevron button's focus ring, which
is 2pt too low.

R=tapted@chromium.org
BUG=617511

Committed: https://crrev.com/ce0f71bffa40beb738e2579efd91d3a0260dd010
Cr-Commit-Position: refs/heads/master@{#400309}
==========

[commit-bot: I haz the power, 2016-06-17 00:16:18]

Patchset 4 (id:??) landed as
https://crrev.com/ce0f71bffa40beb738e2579efd91d3a0260dd010
Cr-Commit-Position: refs/heads/master@{#400309}