Mac: Make it easier for views to correctly draw with a theme.

Mac: Make it easier for views to correctly draw with a theme.

Views that draw using the window's theme need to redraw when the theme
changed and when the window's active state changed.

Create a protocol ThemedWindowDrawing that views can adopt and easily
update in response to theme and active state changes.

Reduce inefficient use of cr_recursivelySetNeedsDisplay to minimize the
amount of redraw we do.

TabView, BookmarkButton, DownloadItemButton, and
DownloadShowAllButton were previously relying on BackgroundGradientView
calling cr_recursivelySetNeedsDisplay. Update them to adopt the new
protocol.

BUG=453996
Committed: https://crrev.com/7463e29b710429f78383a36adc1175169fa40188
Cr-Commit-Position: refs/heads/master@{#314446}

[Andre, 2015-02-03 19:00:20]

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

[Andre, 2015-02-03 19:00:21]

Robert, PTAL.

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (left):

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:624: [[self window]
setViewsNeedDisplay:YES];
I think there's a misunderstanding here.
setViewsNeedDisplay does not mark all views as dirty like
cr_recursivelySetNeedsDisplay.
It just tells the window that there is a view in it that is dirty.

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:628:
[floatingBarBackingView_ setNeedsDisplay:YES];  // Okay even if nil.
FloatingBarBackingView now adopts ThemedWindowDrawing.

[Robert Sesek, 2015-02-03 19:02:26]

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

[Robert Sesek, 2015-02-03 19:02:27]

I think Avi may be a better reviewer for this CL, since he did a lot of work
with themes initially.

[Andre, 2015-02-03 19:13:20]

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_strip_view.h (right):

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_view.h:20: @interface TabStripView :
NSView<ThemedWindowDrawing, URLDropTarget> {
See https://codereview.chromium.org/867273003/ for why the superclass is
changed.

[Andre, 2015-02-03 19:20:37]

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/back...
File chrome/browser/ui/cocoa/background_gradient_view.mm (right):

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/back...
chrome/browser/ui/cocoa/background_gradient_view.mm:114: -
(void)viewDidMoveToWindow {
Views usually call [[self window] isMainWindow] when drawing.
It seems better to call setNeedsDisplay after [self window] has been set.

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_strip_view.mm (right):

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_view.mm:68: [[self window] isMainWindow]
|| !themeProvider->UsingDefaultTheme();
I discussed this with Robert yesterday.
The browser window is never key but not main, only child windows can be.

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm (right):

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm:56: [self
cr_recursivelySetNeedsDisplay:YES];
This used to be done by BackgroundGradientView.

[Avi (use Gerrit), 2015-02-03 21:53:34]

I'm not sure I understand the need for both methods in the protocol. Are there
any situations where a view would need to handle just one, or would need to
handle them differently?

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm (right):

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm:58: 
Don't you need -windowDidChangeTheme too? In BrowserWindowController, it checks
to see if the protocol was adopted, but not if the individual methods were
implemented.

[Andre, 2015-02-03 22:34:05]

On 2015/02/03 21:53:34, Avi wrote:
> I'm not sure I understand the need for both methods in the protocol. Are there
> any situations where a view would need to handle just one, or would need to
> handle them differently?
> 
>
https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
> File chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm (right):
> 
>
https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm:58: 
> Don't you need -windowDidChangeTheme too? In BrowserWindowController, it
checks
> to see if the protocol was adopted, but not if the individual methods were
> implemented.

Some theme elements are the same whether active or inactive, so a view may
not want to redraw on -windowDidChangeActive but still need to redraw on
-windowDidChangeTheme.

[Andre, 2015-02-03 22:34:16]

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm (right):

https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm:58: 
On 2015/02/03 21:53:34, Avi wrote:
> Don't you need -windowDidChangeTheme too? In BrowserWindowController, it
checks
> to see if the protocol was adopted, but not if the individual methods were
> implemented.

-windowDidChangeTheme is a required method on a formal protocol, so we will get
a compiler warning if an implementation is missing. Therefore it should be safe
to not check for the individual methods at run time.
In this case, the BackgroundGradientView superclass provides an implementation
that we don't override.

[Avi (use Gerrit), 2015-02-04 00:20:50]

On 2015/02/03 22:34:16, Andre wrote:
>
https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
> File chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm (right):
> 
>
https://codereview.chromium.org/900613003/diff/1/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/toolbar_view_cocoa.mm:58: 
> On 2015/02/03 21:53:34, Avi wrote:
> > Don't you need -windowDidChangeTheme too? In BrowserWindowController, it
> checks
> > to see if the protocol was adopted, but not if the individual methods were
> > implemented.
> 
> -windowDidChangeTheme is a required method on a formal protocol, so we will
get
> a compiler warning if an implementation is missing. Therefore it should be
safe
> to not check for the individual methods at run time.
> In this case, the BackgroundGradientView superclass provides an implementation
> that we don't override.

Got it. LGTM

[Andre, 2015-02-04 00:28:53]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-02-04 00:29:36]

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

[commit-bot: I haz the power, 2015-02-04 00:33:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-04 00:34:18]

Patchset 1 (id:??) landed as
https://crrev.com/7463e29b710429f78383a36adc1175169fa40188
Cr-Commit-Position: refs/heads/master@{#314446}