[Mac] Fix BMB chevron button being black on Chrome open.

[Mac] Fix BMB chevron button being black on Chrome open.

It seems the issue is caused by the button being drawn into
a layer early on and being cached. Adding a -setNeedsDisplay:
when we actually determine whether it should be visible fixes
the problem - so that the first actual paint to the screen
has the correct appearance.

I'm not sure why Cocoa draws it to a layer earlier than this
(this doesn't happen for other buttons), nor why it's background
color is black when that happens. However, this change does fix
the bad appearance.

BUG=563395
Committed: https://crrev.com/bf571b6dcf58bd8a28fdd5e15b4428e5c2a0b44f
Cr-Commit-Position: refs/heads/master@{#371344}

[Alexei Svitkine (slow), 2016-01-25 00:09:14]

asvitkine@chromium.org changed reviewers:
+ erikchen@chromium.org

[erikchen, 2016-01-25 14:51:05]

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

https://codereview.chromium.org/1630453002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:868:
[offTheSideButton_ setNeedsDisplay:YES];
Does your CL still fix the problem if we only call setNeedsDisplay:  when hidden
changes from YES to NO? I'd prefer to minimize the extra redrawing that we do.

[Alexei Svitkine (slow), 2016-01-25 17:34:59]

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

https://codereview.chromium.org/1630453002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:868:
[offTheSideButton_ setNeedsDisplay:YES];
On 2016/01/25 14:51:05, erikchen wrote:
> Does your CL still fix the problem if we only call setNeedsDisplay:  when
hidden
> changes from YES to NO? I'd prefer to minimize the extra redrawing that we do.

Unfortunately, that doesn't work (on its own). The control starts visible, so
the initial time this happens the setHidden: above is a no-op.

However, I could set it to hidden in the initialization code above - i.e. after
line 429 and then the you suggest check can be added here. Would you like me to
do that?

[erikchen, 2016-01-25 19:48:57]

On 2016/01/25 17:34:59, Alexei Svitkine (very slow) wrote:
>
https://codereview.chromium.org/1630453002/diff/1/chrome/browser/ui/cocoa/boo...
> File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):
> 
>
https://codereview.chromium.org/1630453002/diff/1/chrome/browser/ui/cocoa/boo...
> chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:868:
> [offTheSideButton_ setNeedsDisplay:YES];
> On 2016/01/25 14:51:05, erikchen wrote:
> > Does your CL still fix the problem if we only call setNeedsDisplay:  when
> hidden
> > changes from YES to NO? I'd prefer to minimize the extra redrawing that we
do.
> 
> Unfortunately, that doesn't work (on its own). The control starts visible, so
> the initial time this happens the setHidden: above is a no-op.
> 
> However, I could set it to hidden in the initialization code above - i.e.
after
> line 429 and then the you suggest check can be added here. Would you like me
to
> do that?

lgtm.

This is not my preferred solution because this has the potential to
inappropriately cause too many redraws. That being said, the bookmark bar is
already poorly optimized with regards to that, so I don't see too much harm in
using the setNeedsDisplay: hammer to solve this problem. If you choose to land
the current logic, please add a comment linking to this discussion and/or the
crbug.

If you want to do more digging, my suspicion is that the first draw occurs when
BookmarkButton is not-layer backed, and later on it acquires a layer, but hasn't
redrawn appropriately. I suspect that if you give "Off the side button" a layer
in IB under the "Core Animation Layer" pane, that the problem may also go away.

[Alexei Svitkine (slow), 2016-01-25 22:10:03]

Adding a layer via IB didn't seem to help. Added a comment for context and
landing as-is otherwise so we fix this for our users. If we find a better way to
do this in the future, we can clean this up later. Thanks!

[Alexei Svitkine (slow), 2016-01-25 22:10:08]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-01-25 22:10:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1630453002/#ps20001
(title: "Add a comment.")

[commit-bot: I haz the power, 2016-01-25 22:12:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1630453002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1630453002/20001

[commit-bot: I haz the power, 2016-01-25 23:12:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-25 23:14:43]

Description was changed from

==========
[Mac] Fix BMB chevron button being black on Chrome open.

It seems the issue is caused by the button being drawn into
a layer early on and being cached. Adding a -setNeedsDisplay:
when we actually determine whether it should be visible fixes
the problem - so that the first actual paint to the screen
has the correct appearance.

I'm not sure why Cocoa draws it to a layer earlier than this
(this doesn't happen for other buttons), nor why it's background
color is black when that happens. However, this change does fix
the bad appearance.

BUG=563395
==========

to

==========
[Mac] Fix BMB chevron button being black on Chrome open.

It seems the issue is caused by the button being drawn into
a layer early on and being cached. Adding a -setNeedsDisplay:
when we actually determine whether it should be visible fixes
the problem - so that the first actual paint to the screen
has the correct appearance.

I'm not sure why Cocoa draws it to a layer earlier than this
(this doesn't happen for other buttons), nor why it's background
color is black when that happens. However, this change does fix
the bad appearance.

BUG=563395

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

[commit-bot: I haz the power, 2016-01-25 23:14:43]

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