Fix darkly-drawn Show All button in Downloads Shelf.

Fix darkly-drawn Show All button in Downloads Shelf.

When the download shelf appears the Show All button at the far right is
very dark, almost as if its been drawn several times on top of itself
(see the original bug report for a screenshot of the problem).

The problem is that the Show All button is layer backed (because the
shelf is), so it caches its contents. The Show All button also declares
itself to be opaque but wants the shelf background texture to show
through. It accomplishes this by simulating a lock focus onto the shelf
from within its drawRect:, and calling the shelf's drawRect:.

Unfortunately, when the button first draws (into its layer) the shelf
is hidden and so the shelf's height is 0.0. The shelf's drawing routines
restrict themselves to the shelf's bounds rect - that works most of the
time because if the shelf has 0 height there should be no drawing to be
done. This optimization doesn't work for the Show All button when it
uses the zero-height shelf to draw its background.

The proposed fix is to detect the zero-height shelf from within
drawRect:, and to use a frame-changed notification from the shelf as
the signal to request a redraw.

BUG=485782
Committed: https://crrev.com/890dcc45af511f04eeb72462849aa78434f07e59
Cr-Commit-Position: refs/heads/master@{#330120}

[shrike, 2015-05-07 23:00:55]

shrike@chromium.org changed reviewers:
+ asanka@chromium.org

[shrike, 2015-05-07 23:03:50]

Hello asanka,

This is a bug I've seen before, but ran into it repeatedly while working on the
context menu problem, so I decided to track it down and fix it. PTAL.

[asanka, 2015-05-08 17:48:36]

lgtm assuming this works well with shelf animation.

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_show_all_button.mm (right):

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_show_all_button.mm:81: } else {
Do we need the observer if there was a successful paint?

[asanka, 2015-05-08 17:49:39]

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_show_all_button.mm (right):

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_show_all_button.mm:74: [[NSColor
colorWithCalibratedWhite:0.8 alpha:1.0] set];
Is there a way to determine this at runtime without hardcoding?

[shrike, 2015-05-08 18:27:48]

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_show_all_button.mm (right):

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_show_all_button.mm:74: [[NSColor
colorWithCalibratedWhite:0.8 alpha:1.0] set];
On 2015/05/08 17:49:39, asanka wrote:
> Is there a way to determine this at runtime without hardcoding?

Not cleanly. The code that we need lives in [BackgroundGradientView
drawBackground:], but to copy/paste that into DownloadShowAllButton requires
exposing a private method from BackgroundGradientView (backgroundImageColor).

Calling [BackgroundGradientView drawBackground:] directly can't be done because
it relies on a static varliable set up in cr_drawUsingAncestor:inRect:.

In my testing the hard-coded background color version comes onscreen for a small
fraction of a second before being replaced by the real one.

It might be possible to say

- (BOOL)isOpaque {
  return [downloadShelfView bounds].size.height > 0.0;
}

and skip trying to draw the background in drawRect: (but still wait for the
shelf to grow in height and request a redraw).

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_show_all_button.mm:81: } else {
On 2015/05/08 17:48:36, asanka wrote:
> Do we need the observer if there was a successful paint?

No, we should not. Are you asking because you see an issue? There should be no
observer at this point.

[asanka, 2015-05-12 00:20:52]

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_show_all_button.mm (right):

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_show_all_button.mm:74: [[NSColor
colorWithCalibratedWhite:0.8 alpha:1.0] set];
On 2015/05/08 at 18:27:48, shrike wrote:
> On 2015/05/08 17:49:39, asanka wrote:
> > Is there a way to determine this at runtime without hardcoding?
> 
> Not cleanly. The code that we need lives in [BackgroundGradientView
drawBackground:], but to copy/paste that into DownloadShowAllButton requires
exposing a private method from BackgroundGradientView (backgroundImageColor).
> 
> Calling [BackgroundGradientView drawBackground:] directly can't be done
because it relies on a static varliable set up in cr_drawUsingAncestor:inRect:.
> 
> In my testing the hard-coded background color version comes onscreen for a
small fraction of a second before being replaced by the real one.
> 
> It might be possible to say
> 
> - (BOOL)isOpaque {
>   return [downloadShelfView bounds].size.height > 0.0;
> }
> 
> and skip trying to draw the background in drawRect: (but still wait for the
shelf to grow in height and request a redraw).

The only thing I'm worried about is whether this would result in a visible flash
if there's a big difference in contrast between the theme background and the
color we are choosing. It would be great if we can find a solution that avoids
this possibility.

https://codereview.chromium.org/1125363005/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_show_all_button.mm:81: } else {
On 2015/05/08 at 18:27:48, shrike wrote:
> On 2015/05/08 17:48:36, asanka wrote:
> > Do we need the observer if there was a successful paint?
> 
> No, we should not. Are you asking because you see an issue? There should be no
observer at this point.

Right. Misread.

[shrike, 2015-05-12 21:26:59]

shrike@chromium.org changed reviewers:
+ rsesek@chromium.org

[shrike, 2015-05-12 21:36:07]

Hello asanka,

I understand and agree with your concern about the Show All button background
flashing. Here is a variant of the cl that ensures the background always gets
drawn using the shelf's gradient code. There is also no need to ask for a redraw
at a later time, as I had previously implemented.

However, this change comes at the cost of adding a variant of
cr_drawUsingAncestor:inRect: to ui/base/cocoa/nsview_additions. I tried to
choose a reasonable name for the new method. I also chose to keep two variants
of the method around vs. just my new one and changing all places in the code to
use the new variant. I don't know what the preferred choice is for such a
decision in Chrome. Looping rsesek in to get approval/recommendations.

PTAL

[Robert Sesek, 2015-05-13 18:38:55]

I don't have any input here; I'm not too familiar with this bit of code. Maybe
Avi would be?

[shrike, 2015-05-13 21:04:14]

shrike@chromium.org changed reviewers:
+ avi@chromium.org
- rsesek@chromium.org

[shrike, 2015-05-13 21:07:07]

Hello avi,

My current proposal for fixing this bug involves adding a variant of
cr_drawUsingAncestor:inRect: to ui/base/cocoa/nsview_additions. I tried to
choose a reasonable name for the new method. I also chose to keep two variants
of the method around vs. just my new one and changing all places in the code to
use the new variant. I don't know what the preferred choice is for Chrome.
Please approve/recommend a different course.

[Avi (use Gerrit), 2015-05-13 21:49:40]

lgtm

This is fine.

[asanka, 2015-05-14 22:09:56]

On 2015/05/13 at 21:49:40, avi wrote:
> lgtm
> 
> This is fine.

lgtm from me as well.

[shrike, 2015-05-15 16:40:23]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2015-05-15 16:41:33]

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

[commit-bot: I haz the power, 2015-05-15 17:29:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-15 17:30:32]

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