[Mac] Fix rough-looking profile picker text, especially with a dark theme.

[Mac] Fix rough-looking profile picker text, especially with a dark theme.

Issue 593835 shows the AvatarButton's text having jagged, rainbow tinted
edges. It turns out that the text cell gets a hint about anti-aliasing
from the NSVisualEffect view below it, but the hint is wrong because the
NSVisualEffectView is itself below the theme background.

This only happens when the NSVisualEffectView's blendingMode is set to
NSVisualEffectBlendingModeBehindWindow (which makes the content behind
the window show through with a blur effect). To create the effect, the
view cuts a hole in the window and generates a mask which the window
server uses to render a translucent material behind the window.

Because the translucent parts of the window are invisible to the app,
the text can't render itself with correct subpixel anti-aliasing, which
needs information about the background. To help, NSVisualEffectView
implements two undocumented methods:
`-shouldSetFontSmoothingBackgroundColor` and
`-_backgroundColorForFontSmoothing`. It provides the base color of the
translucent material, and for the purpose of antialiasing the text cell
pretends that it's over an opaque background of that color. Because the
translucency effect is subtle, it's not obvious that the antialiasing is
done with a fixed color.

NSView implements these methods to first search its immediate subviews
and return the first YES/non-nil answer, or return NO/nil if
`self.opaque == YES`, then tail call the same method on its superview.

A quirk of this strategy is that, to provide a background color to a
text view, an NSVisualEffectView must be either a superview or an
immediate subview of a superview. So, the AvatarButton's text does get a
background color hint with the current hierarchy:

    - NSThemeFrame
       |- NSVisualEffectView
       |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

…but, if the NSVisualEffectView were wrapped in another view, it doesn't
(because each step of the search just looks at immediate subviews):

    - NSThemeFrame
       |- NSView
       | |- NSVisualEffectView
       | |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

Anyway, back on topic, our issue is that the text cell accepts the
hinted background color, which is never right since Chrome always
draws a tint over the visual effect view, and in some cases the hint is
drastically wrong.

Ideally, we'd provide a fake background for the text to perform subpixel
anti-aliasing against, but it's unclear how to do that. For now, the
simplest solution is to wrap it in another view.

This CL sucks all of the tab strip background drawing into
TabStripBackgroundView, and makes it so that other views don't need to
know about the NSVisualEffectView at all.

It also only creates the NSVisualEffectView when it's used, and gets rid
of it when it's not used (like in full screen, or when a theme is
installed).

BUG=593835
Review-Url: https://codereview.chromium.org/2430493002
Cr-Commit-Position: refs/heads/master@{#443707}
Committed: https://chromium.googlesource.com/chromium/src/+/a5fd3c415f3cc4f8813ba9772d838bee41040ae8

[Sidney San Martín, 2017-01-09 16:18:08]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 16:18:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 17:02:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 17:02:37]

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_...)

[Sidney San Martín, 2017-01-09 17:41:03]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 17:41:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-01-09 17:42:53]

Description was changed from

==========
[Mac] Fix rough profile picker text when a dark theme is in use

The linked bug shows the AvatarButton's text with jagged, rainbow-tinted
edges. It turns out that an NSVisualEffect view underneath the button is
giving its text cell a hint about the background behind it, but the hint
is wrong because the NSVisualEffectView is covered by the theme
background.

This only happens when the NSVisualEffectView's blendingMode is set to
NSVisualEffectBlendingModeBehindWindow. This mode makes it translucent:
the content behind the window shows through, blurred. To create the
effect, the view cuts a hole in the window and generates a mask which
the window server uses to render a translucent material behind it.

Because the translucent parts of the window are clear from the app's
point of view, the text can't automatically be rendered with subpixel
anti-aliasing (which uses information about the background). So,
NSVisualEffectView implements two undocumented methods:
`-shouldSetFontSmoothingBackgroundColor` and
`-_backgroundColorForFontSmoothing`. It returns the base color of the
material, and the text cell renders itself opaquely over that color.
Because the effect is subtle, it's not noticable that the antialiasing
is done with a static color.

The base implementations of these methods return the first YES/non-nil
answer from the view's immediate subviews, then return NO/nil if
`self.opaque` is `YES` or recurse to its superview otherwise.

A quirk of this strategy is that an NSVisualEffectView must be either a
superview of a text view or an immediate subview of its superview. So,
the AvatarButton's text gets a background with the current hierarchy:

    - NSThemeFrame
       |- NSVisualEffectView
       |  |- TabBackgroundView # draws the theme
       |- FullSizeContentView
          |- AvatarButton

…but, if the NSVisualEffectView were wrapped in any other view, it
wouldn't:

    - NSThemeFrame
       |- NSView
       | |- NSVisualEffectView
       | |  |- TabBackgroundView
       |- FullSizeContentView
          |- AvatarButton

Anyway, back on topic, our issue is that the text cell accepts the
suggested background color as the truth. That's never right, since
Chrome always overlays the visual effect view with some tint, and in some
cases

[TODO]

BUG=593835
==========

to

==========
[Mac] Fix rough-looking profile picker text, especially with a dark theme.

The linked bug shows the AvatarButton's text with jagged, rainbow-tinted
edges. It turns out that an NSVisualEffect view underneath the button is
giving its text cell a hint about the background behind it, but the hint
is wrong because the NSVisualEffectView is covered by the theme
background.

This only happens when the NSVisualEffectView's blendingMode is set to
NSVisualEffectBlendingModeBehindWindow. This mode makes it translucent:
the content behind the window shows through, blurred. To create the
effect, the view cuts a hole in the window and generates a mask which
the window server uses to render a translucent material behind it.

Because the translucent parts of the window are clear from the app's
point of view, the text can't automatically be rendered with subpixel
anti-aliasing (which uses information about the background). So,
NSVisualEffectView implements two undocumented methods:
`-shouldSetFontSmoothingBackgroundColor` and
`-_backgroundColorForFontSmoothing`. It returns the base color of the
material, and the text cell renders itself opaquely over that color.
Because the effect is subtle, it's not noticable that the antialiasing
is done with a static color.

The base implementations of these methods return the first YES/non-nil
answer from the view's immediate subviews, then return NO/nil if
`self.opaque` is `YES` or recurse to its superview otherwise.

A quirk of this strategy is that an NSVisualEffectView must be either a
superview of a text view or an immediate subview of its superview. So,
the AvatarButton's text gets a background with the current hierarchy:

    - NSThemeFrame
       |- NSVisualEffectView
       |  |- TabBackgroundView # draws the theme
       |- FullSizeContentView
          |- AvatarButton

…but, if the NSVisualEffectView were wrapped in any other view, it
wouldn't:

    - NSThemeFrame
       |- NSView
       | |- NSVisualEffectView
       | |  |- TabBackgroundView
       |- FullSizeContentView
          |- AvatarButton

Anyway, back on topic, our issue is that the text cell accepts the
suggested background color as the truth. That's never right, since
Chrome always overlays the visual effect view with some tint, and in some
cases

[TODO]

BUG=593835
==========

[commit-bot: I haz the power, 2017-01-09 19:13:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 19:13:07]

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_...)

[Sidney San Martín, 2017-01-10 22:38:05]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 22:39:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-01-10 23:02:58]

Description was changed from

==========
[Mac] Fix rough-looking profile picker text, especially with a dark theme.

The linked bug shows the AvatarButton's text with jagged, rainbow-tinted
edges. It turns out that an NSVisualEffect view underneath the button is
giving its text cell a hint about the background behind it, but the hint
is wrong because the NSVisualEffectView is covered by the theme
background.

This only happens when the NSVisualEffectView's blendingMode is set to
NSVisualEffectBlendingModeBehindWindow. This mode makes it translucent:
the content behind the window shows through, blurred. To create the
effect, the view cuts a hole in the window and generates a mask which
the window server uses to render a translucent material behind it.

Because the translucent parts of the window are clear from the app's
point of view, the text can't automatically be rendered with subpixel
anti-aliasing (which uses information about the background). So,
NSVisualEffectView implements two undocumented methods:
`-shouldSetFontSmoothingBackgroundColor` and
`-_backgroundColorForFontSmoothing`. It returns the base color of the
material, and the text cell renders itself opaquely over that color.
Because the effect is subtle, it's not noticable that the antialiasing
is done with a static color.

The base implementations of these methods return the first YES/non-nil
answer from the view's immediate subviews, then return NO/nil if
`self.opaque` is `YES` or recurse to its superview otherwise.

A quirk of this strategy is that an NSVisualEffectView must be either a
superview of a text view or an immediate subview of its superview. So,
the AvatarButton's text gets a background with the current hierarchy:

    - NSThemeFrame
       |- NSVisualEffectView
       |  |- TabBackgroundView # draws the theme
       |- FullSizeContentView
          |- AvatarButton

…but, if the NSVisualEffectView were wrapped in any other view, it
wouldn't:

    - NSThemeFrame
       |- NSView
       | |- NSVisualEffectView
       | |  |- TabBackgroundView
       |- FullSizeContentView
          |- AvatarButton

Anyway, back on topic, our issue is that the text cell accepts the
suggested background color as the truth. That's never right, since
Chrome always overlays the visual effect view with some tint, and in some
cases

[TODO]

BUG=593835
==========

to

==========
[Mac] Fix rough-looking profile picker text, especially with a dark theme.

Issue 593835 shows the AvatarButton's text having jagged, rainbow tinted
edges. It turns out that the text cell gets a hint about anti-aliasing
from the NSVisualEffect view below it, but the hint is wrong because the
NSVisualEffectView is itself below the theme background.

This only happens when the NSVisualEffectView's blendingMode is set to
NSVisualEffectBlendingModeBehindWindow (which makes the content behind
the window show through with a blur effect). To create the effect, the
view cuts a hole in the window and generates a mask which the window
server uses to render a translucent material behind the window.

Because the translucent parts of the window are invisible to the app,
the text can't render itself with correct subpixel anti-aliasing, which
needs information about the background. To help, NSVisualEffectView
implements two undocumented methods:
`-shouldSetFontSmoothingBackgroundColor` and
`-_backgroundColorForFontSmoothing`. It provides the base color of the
translucent material, and for the purpose of antialiasing the text cell
pretends that it's over an opaque background of that color. Because the
translucency effect is subtle, it's not obvious that the antialiasing is
done with a fixed color.

NSView implements these methods to first search its immediate subviews
and return the first YES/non-nil answer, or return NO/nil if
`self.opaque == YES`, then tail call the same method on its superview.

A quirk of this strategy is that, to provide a background color to a
text view, an NSVisualEffectView must be either a superview or an
immediate subview of a superview. So, the AvatarButton's text does get a
background color hint with the current hierarchy:

    - NSThemeFrame
       |- NSVisualEffectView
       |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

…but, if the NSVisualEffectView were wrapped in another view, it doesn't
(because each step of the search just looks at immediate subviews):

    - NSThemeFrame
       |- NSView
       | |- NSVisualEffectView
       | |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

Anyway, back on topic, our issue is that the text cell accepts the
hinted background color, which is never right since Chrome always
draws a tint over the visual effect view, and in some cases the hint is
drastically wrong.

Ideally, we'd provide a fake background for the text to perform subpixel
anti-aliasing against, but it's unclear how to do that. For now, the
simplest solution is to wrap it in another view.

This CL sucks all of the tab strip background drawing into
TabStripBackgroundView, and makes it so that other views don't need to
know about the NSVisualEffectView at all.

It also only creates the NSVisualEffectView when it's used, and gets rid
of it when it's not used (like in full screen, or when a theme is
installed).

BUG=593835
==========

[Sidney San Martín, 2017-01-10 23:12:19]

sdy@chromium.org changed reviewers:
+ avi@chromium.org, spqchan@chromium.org

[Sidney San Martín, 2017-01-10 23:12:20]

Beware, the CL description is fairly long, but it mostly just boils down to a
refactor.

avi@, can you review for sanity and OWNERship?

spqchan@, mind taking a look at how if affects full screen? The only difference
I notice is that the tab strip background color no longer changes in full screen
on OSs that support NSVisualEffectView, but I think that was a bug.

[commit-bot: I haz the power, 2017-01-11 00:03:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 00:03:24]

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_...)

[Sidney San Martín, 2017-01-11 06:57:26]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 06:57:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 07:58:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 07:58:05]

Dry run: This issue passed the CQ dry run.

[Sidney San Martín, 2017-01-11 17:55:48]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 17:56:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-01-11 17:56:59]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 17:57:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-01-11 19:03:42]

lgtm

Looks Sane To Me.

[commit-bot: I haz the power, 2017-01-11 19:15:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:15:43]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-01-11 19:23:38]

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

[Robert Sesek, 2017-01-11 19:23:40]

Mostly style comments :-)

https://codereview.chromium.org/2430493002/diff/100001/base/mac/sdk_forward_d...
File base/mac/sdk_forward_declarations.h (left):

https://codereview.chromium.org/2430493002/diff/100001/base/mac/sdk_forward_d...
base/mac/sdk_forward_declarations.h:79: BASE_EXPORT extern NSString* const
NSAppearanceNameVibrantDark;
Why are you removing these from here?

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view.h (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.h:10: @class
TabStripBackgroundView;
Not used?

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:18: @synthesize
inATabDraggingOverlayWindow = _inATabDraggingOverlayWindow;
Chrome uses trailingUnderscores_ for ObjC ivars.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:95:
TabStripThemeBackgroundView* _themeBackgroundView;
Here too use trailing underscores.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:102:
initWithFrame:self.bounds] autorelease];
Who owns this? Needs a comment in the ivar.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:119: -
(void)updateVisualEffectView {
Does this need to be gated by a runtime version check?

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:156: [self.window
removeObserver:self forKeyPath:@"styleMask"];
Do you need to remove the observer in dealloc, or does AppKit call this on
window teardown too?

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:165: DCHECK(object ==
self.window);
DCHECK_EQ

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_private.h (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_private.h:5: #ifndef
CHROME_BROWSER_UI_COCOA_TABS_TAB_STRIP_BACKGROUND_VIEW_PRIVATE_H_
Not sure I understand the point of this file.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_private.h:13: extern
NSString* const NSAppearanceNameVibrantDark;
Why did these get moved out of sdk_forward_declarations.h?

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:25: const
base::scoped_nsobject<TabStripBackgroundView> tabStripBackgroundView_{
naming: use under_scores_ in C++ members

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:38: bool
_usingSystemTheme = true;
naming: under_scores_

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:48:
@synthesize themeProvider = _themeProvider;
nit: Use trailing underscore.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:52: Class
NSVisualEffectView_class = [NSVisualEffectView class];
Leave a comment explaining what this is doing.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:56: auto
hasVisualEffectView = [&]() {
naming: under_scores

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:65:
MockThemeProvider themeProvider;
naming: under_scores

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:18: @interface NSWindow
(TitlebarAppearsTransparent)
Is this a SPI or is this a forward declaration (in which case, it goes into
sdk_forward_declarations.h)?

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:188:
tabStripBackgroundView_.get().inATabDraggingOverlayWindow = YES;
Dot notation is not really used in this method. Be consistent with the code
around here and use a method invocation.

[spqchan, 2017-01-11 19:33:57]

The fullscreen changes LGTM

I just have some nits

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:24: bool isFullScreen
= (self.window.styleMask & NSFullScreenWindowMask) != 0;
nit: Change to isFullscreen

We want to use "Fullscreen" instead of "Full Screen" to make things more
consistent

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:124: // Visual effects
cause higher power consumption in full screen.
nit: "full screen" -> "fullscreen"

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:17: for
(NSView* subview in view.subviews)
nit: multiple statement for loops need curly braces

[Sidney San Martín, 2017-01-11 22:00:59]

Still working on the other comments.

https://codereview.chromium.org/2430493002/diff/100001/base/mac/sdk_forward_d...
File base/mac/sdk_forward_declarations.h (left):

https://codereview.chromium.org/2430493002/diff/100001/base/mac/sdk_forward_d...
base/mac/sdk_forward_declarations.h:79: BASE_EXPORT extern NSString* const
NSAppearanceNameVibrantDark;
On 2017/01/11 19:23:39, Robert Sesek wrote:
> Why are you removing these from here?

Maybe we can chat about this in person with thakis@? I think that if the intent
of -Wunguarded-availability is for you to not use partially-available symbols by
accident, then the number of symbols in sdk_forward_declarations.h is inversely
correlated with the diagnostic being useful.

I wanted to avoid putting new related symbols in this header, especially ones
that are only used in one file, so I put them all in the same place.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_private.h (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_private.h:5: #ifndef
CHROME_BROWSER_UI_COCOA_TABS_TAB_STRIP_BACKGROUND_VIEW_PRIVATE_H_
On 2017/01/11 19:23:40, Robert Sesek wrote:
> Not sure I understand the point of this file.

It's so that tab_strip_background_view_unittest.mm has access to private details
like <ThemedWindowDrawing> protocol conformance. Maybe it should be named
…_testing.h? I'd love to get rid of it if there's a better way.

[Robert Sesek, 2017-01-11 22:09:54]

https://codereview.chromium.org/2430493002/diff/100001/base/mac/sdk_forward_d...
File base/mac/sdk_forward_declarations.h (left):

https://codereview.chromium.org/2430493002/diff/100001/base/mac/sdk_forward_d...
base/mac/sdk_forward_declarations.h:79: BASE_EXPORT extern NSString* const
NSAppearanceNameVibrantDark;
On 2017/01/11 22:00:58, sdy wrote:
> On 2017/01/11 19:23:39, Robert Sesek wrote:
> > Why are you removing these from here?
> 
> Maybe we can chat about this in person with thakis@? I think that if the
intent
> of -Wunguarded-availability is for you to not use partially-available symbols
by
> accident, then the number of symbols in sdk_forward_declarations.h is
inversely
> correlated with the diagnostic being useful.
> 
> I wanted to avoid putting new related symbols in this header, especially ones
> that are only used in one file, so I put them all in the same place.

Hm yes let's chat in person.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_private.h (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_private.h:5: #ifndef
CHROME_BROWSER_UI_COCOA_TABS_TAB_STRIP_BACKGROUND_VIEW_PRIVATE_H_
On 2017/01/11 22:00:58, sdy wrote:
> On 2017/01/11 19:23:40, Robert Sesek wrote:
> > Not sure I understand the point of this file.
> 
> It's so that tab_strip_background_view_unittest.mm has access to private
details
> like <ThemedWindowDrawing> protocol conformance. Maybe it should be named
> …_testing.h? I'd love to get rid of it if there's a better way.

I think I view that as a public detail (it's certainly public for all existing
conformers).

[Sidney San Martín, 2017-01-12 23:58:27]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 23:59:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-01-13 00:10:08]

This should address all comments so far.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view.h (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.h:10: @class
TabStripBackgroundView;
On 2017/01/11 19:23:39, Robert Sesek wrote:
> Not used?

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:18: @synthesize
inATabDraggingOverlayWindow = _inATabDraggingOverlayWindow;
On 2017/01/11 19:23:39, Robert Sesek wrote:
> Chrome uses trailingUnderscores_ for ObjC ivars.

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:24: bool isFullScreen
= (self.window.styleMask & NSFullScreenWindowMask) != 0;
On 2017/01/11 19:33:56, spqchan wrote:
> nit: Change to isFullscreen
> 
> We want to use "Fullscreen" instead of "Full Screen" to make things more
> consistent

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:95:
TabStripThemeBackgroundView* _themeBackgroundView;
On 2017/01/11 19:23:39, Robert Sesek wrote:
> Here too use trailing underscores.

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:102:
initWithFrame:self.bounds] autorelease];
On 2017/01/11 19:23:39, Robert Sesek wrote:
> Who owns this? Needs a comment in the ivar.

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:119: -
(void)updateVisualEffectView {
On 2017/01/11 19:23:40, Robert Sesek wrote:
> Does this need to be gated by a runtime version check?

It doesn't _need_ to be. NSVisualEffectView is a weak symbol so this method will
bail at line 130. I could optimize by, for example, doing a runtime check in
+alloc and returning a different class without any NSVisualEffectView logic, but
I'm not sure if it's worth it.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:124: // Visual effects
cause higher power consumption in full screen.
On 2017/01/11 19:33:56, spqchan wrote:
> nit: "full screen" -> "fullscreen"

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:156: [self.window
removeObserver:self forKeyPath:@"styleMask"];
On 2017/01/11 19:23:39, Robert Sesek wrote:
> Do you need to remove the observer in dealloc, or does AppKit call this on
> window teardown too?

AppKit takes care of it:

> When a window is deallocated, AppKit calls this method for each view in the
window, passing nil for the newWindow parameter.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:165: DCHECK(object ==
self.window);
On 2017/01/11 19:23:40, Robert Sesek wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:17: for
(NSView* subview in view.subviews)
On 2017/01/11 19:33:56, spqchan wrote:
> nit: multiple statement for loops need curly braces

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:25: const
base::scoped_nsobject<TabStripBackgroundView> tabStripBackgroundView_{
On 2017/01/11 19:23:40, Robert Sesek wrote:
> naming: use under_scores_ in C++ members

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:38: bool
_usingSystemTheme = true;
On 2017/01/11 19:23:40, Robert Sesek wrote:
> naming: under_scores_

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:48:
@synthesize themeProvider = _themeProvider;
On 2017/01/11 19:23:40, Robert Sesek wrote:
> nit: Use trailing underscore.

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:52: Class
NSVisualEffectView_class = [NSVisualEffectView class];
On 2017/01/11 19:23:40, Robert Sesek wrote:
> Leave a comment explaining what this is doing.

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:56: auto
hasVisualEffectView = [&]() {
On 2017/01/11 19:23:40, Robert Sesek wrote:
> naming: under_scores

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:65:
MockThemeProvider themeProvider;
On 2017/01/11 19:23:40, Robert Sesek wrote:
> naming: under_scores

Done.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right):

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:18: @interface NSWindow
(TitlebarAppearsTransparent)
On 2017/01/11 19:23:40, Robert Sesek wrote:
> Is this a SPI or is this a forward declaration (in which case, it goes into
> sdk_forward_declarations.h)?

It's a redeclaration of a partially-available property. I added a comment.

https://codereview.chromium.org/2430493002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:188:
tabStripBackgroundView_.get().inATabDraggingOverlayWindow = YES;
On 2017/01/11 19:23:40, Robert Sesek wrote:
> Dot notation is not really used in this method. Be consistent with the code
> around here and use a method invocation.

Done.

[commit-bot: I haz the power, 2017-01-13 00:52:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 00:52:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Robert Sesek, 2017-01-13 19:39:42]

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:12: // TODO: Remove
once we no longer support 10.9 (-Wunguarded-availability)
nit: Format is TODO(xxx) where "xxx" is a crbug URL or your username. Also fix
punctuation.

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:35: void
SetUsingSystemTheme(bool usingSystemTheme) {
naming: using_system_theme

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:54: // TODO:
Remove once we no longer support 10.9
nit: punctuation

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:67:
[[[TabStripBackgroundViewTestWindow alloc] init] autorelease];
Why not scoped_nsobject?

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:19: // TODO: Remove once
we no longer support 10.9 (-Wunguarded-availability)
Same comment.

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:106: [[window contentView]
addSubview:tabStripBackgroundView_];
Use the windowView local here and on line 108 instead.

[Sidney San Martín, 2017-01-13 19:47:44]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 19:48:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-01-13 19:54:02]

The CQ bit was unchecked by sdy@chromium.org

[Sidney San Martín, 2017-01-13 20:03:41]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 20:05:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-01-13 20:05:17]

This should address all of these comments except the scoped_nsobject one (see
below).

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm:12: // TODO: Remove
once we no longer support 10.9 (-Wunguarded-availability)
On 2017/01/13 19:39:41, Robert Sesek wrote:
> nit: Format is TODO(xxx) where "xxx" is a crbug URL or your username. Also fix
> punctuation.

Done.

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:35: void
SetUsingSystemTheme(bool usingSystemTheme) {
On 2017/01/13 19:39:42, Robert Sesek wrote:
> naming: using_system_theme

Done.

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:54: // TODO:
Remove once we no longer support 10.9
On 2017/01/13 19:39:41, Robert Sesek wrote:
> nit: punctuation

Done.

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:67:
[[[TabStripBackgroundViewTestWindow alloc] init] autorelease];
On 2017/01/13 19:39:42, Robert Sesek wrote:
> Why not scoped_nsobject?

Because I can't use properties with scoped_nsobject.

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:19: // TODO: Remove once
we no longer support 10.9 (-Wunguarded-availability)
On 2017/01/13 19:39:42, Robert Sesek wrote:
> Same comment.

Done.

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:106: [[window contentView]
addSubview:tabStripBackgroundView_];
On 2017/01/13 19:39:42, Robert Sesek wrote:
> Use the windowView local here and on line 108 instead.

I renamed it and moved it up to use in more places; let me know how that looks.

[Sidney San Martín, 2017-01-13 20:06:19]

Patchset #8 (id:140001) has been deleted

[Robert Sesek, 2017-01-13 20:12:36]

LGTM

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:67:
[[[TabStripBackgroundViewTestWindow alloc] init] autorelease];
On 2017/01/13 20:05:17, sdy wrote:
> On 2017/01/13 19:39:42, Robert Sesek wrote:
> > Why not scoped_nsobject?
> 
> Because I can't use properties with scoped_nsobject.

Hm. I'd probably do this (optional):

scoped_nsobject<TabStripBackgroundViewTestWindow> owned_window(…);
TabStripBackgroundViewTestWindow* window = owned_window.get();

[Sidney San Martín, 2017-01-13 20:16:45]

The CQ bit was unchecked by sdy@chromium.org

[Sidney San Martín, 2017-01-13 20:17:15]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[Sidney San Martín, 2017-01-13 20:17:34]

The CQ bit was unchecked by sdy@chromium.org

[Sidney San Martín, 2017-01-13 20:17:51]

The CQ bit was checked by sdy@chromium.org

[Sidney San Martín, 2017-01-13 20:17:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
spqchan@chromium.org, avi@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2430493002/#ps180001
(title: "Scopify!")

[commit-bot: I haz the power, 2017-01-13 20:19:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-01-13 20:30:42]

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm (right):

https://codereview.chromium.org/2430493002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm:67:
[[[TabStripBackgroundViewTestWindow alloc] init] autorelease];
On 2017/01/13 20:12:36, Robert Sesek wrote:
> On 2017/01/13 20:05:17, sdy wrote:
> > On 2017/01/13 19:39:42, Robert Sesek wrote:
> > > Why not scoped_nsobject?
> > 
> > Because I can't use properties with scoped_nsobject.
> 
> Hm. I'd probably do this (optional):
> 
> scoped_nsobject<TabStripBackgroundViewTestWindow> owned_window(…);
> TabStripBackgroundViewTestWindow* window = owned_window.get();

Done (with slightly different naming).

[commit-bot: I haz the power, 2017-01-13 22:51:29]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484338671736080,
"parent_rev": "720c8a0a62833dc3b0a0f3b45906d7996702c31b", "commit_rev":
"a5fd3c415f3cc4f8813ba9772d838bee41040ae8"}

[commit-bot: I haz the power, 2017-01-13 22:52:00]

Description was changed from

==========
[Mac] Fix rough-looking profile picker text, especially with a dark theme.

Issue 593835 shows the AvatarButton's text having jagged, rainbow tinted
edges. It turns out that the text cell gets a hint about anti-aliasing
from the NSVisualEffect view below it, but the hint is wrong because the
NSVisualEffectView is itself below the theme background.

This only happens when the NSVisualEffectView's blendingMode is set to
NSVisualEffectBlendingModeBehindWindow (which makes the content behind
the window show through with a blur effect). To create the effect, the
view cuts a hole in the window and generates a mask which the window
server uses to render a translucent material behind the window.

Because the translucent parts of the window are invisible to the app,
the text can't render itself with correct subpixel anti-aliasing, which
needs information about the background. To help, NSVisualEffectView
implements two undocumented methods:
`-shouldSetFontSmoothingBackgroundColor` and
`-_backgroundColorForFontSmoothing`. It provides the base color of the
translucent material, and for the purpose of antialiasing the text cell
pretends that it's over an opaque background of that color. Because the
translucency effect is subtle, it's not obvious that the antialiasing is
done with a fixed color.

NSView implements these methods to first search its immediate subviews
and return the first YES/non-nil answer, or return NO/nil if
`self.opaque == YES`, then tail call the same method on its superview.

A quirk of this strategy is that, to provide a background color to a
text view, an NSVisualEffectView must be either a superview or an
immediate subview of a superview. So, the AvatarButton's text does get a
background color hint with the current hierarchy:

    - NSThemeFrame
       |- NSVisualEffectView
       |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

…but, if the NSVisualEffectView were wrapped in another view, it doesn't
(because each step of the search just looks at immediate subviews):

    - NSThemeFrame
       |- NSView
       | |- NSVisualEffectView
       | |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

Anyway, back on topic, our issue is that the text cell accepts the
hinted background color, which is never right since Chrome always
draws a tint over the visual effect view, and in some cases the hint is
drastically wrong.

Ideally, we'd provide a fake background for the text to perform subpixel
anti-aliasing against, but it's unclear how to do that. For now, the
simplest solution is to wrap it in another view.

This CL sucks all of the tab strip background drawing into
TabStripBackgroundView, and makes it so that other views don't need to
know about the NSVisualEffectView at all.

It also only creates the NSVisualEffectView when it's used, and gets rid
of it when it's not used (like in full screen, or when a theme is
installed).

BUG=593835
==========

to

==========
[Mac] Fix rough-looking profile picker text, especially with a dark theme.

Issue 593835 shows the AvatarButton's text having jagged, rainbow tinted
edges. It turns out that the text cell gets a hint about anti-aliasing
from the NSVisualEffect view below it, but the hint is wrong because the
NSVisualEffectView is itself below the theme background.

This only happens when the NSVisualEffectView's blendingMode is set to
NSVisualEffectBlendingModeBehindWindow (which makes the content behind
the window show through with a blur effect). To create the effect, the
view cuts a hole in the window and generates a mask which the window
server uses to render a translucent material behind the window.

Because the translucent parts of the window are invisible to the app,
the text can't render itself with correct subpixel anti-aliasing, which
needs information about the background. To help, NSVisualEffectView
implements two undocumented methods:
`-shouldSetFontSmoothingBackgroundColor` and
`-_backgroundColorForFontSmoothing`. It provides the base color of the
translucent material, and for the purpose of antialiasing the text cell
pretends that it's over an opaque background of that color. Because the
translucency effect is subtle, it's not obvious that the antialiasing is
done with a fixed color.

NSView implements these methods to first search its immediate subviews
and return the first YES/non-nil answer, or return NO/nil if
`self.opaque == YES`, then tail call the same method on its superview.

A quirk of this strategy is that, to provide a background color to a
text view, an NSVisualEffectView must be either a superview or an
immediate subview of a superview. So, the AvatarButton's text does get a
background color hint with the current hierarchy:

    - NSThemeFrame
       |- NSVisualEffectView
       |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

…but, if the NSVisualEffectView were wrapped in another view, it doesn't
(because each step of the search just looks at immediate subviews):

    - NSThemeFrame
       |- NSView
       | |- NSVisualEffectView
       | |  |- TabStripBackgroundView
       |- FullSizeContentView
          |- AvatarButton

Anyway, back on topic, our issue is that the text cell accepts the
hinted background color, which is never right since Chrome always
draws a tint over the visual effect view, and in some cases the hint is
drastically wrong.

Ideally, we'd provide a fake background for the text to perform subpixel
anti-aliasing against, but it's unclear how to do that. For now, the
simplest solution is to wrap it in another view.

This CL sucks all of the tab strip background drawing into
TabStripBackgroundView, and makes it so that other views don't need to
know about the NSVisualEffectView at all.

It also only creates the NSVisualEffectView when it's used, and gets rid
of it when it's not used (like in full screen, or when a theme is
installed).

BUG=593835

Review-Url: https://codereview.chromium.org/2430493002
Cr-Commit-Position: refs/heads/master@{#443707}
Committed:
https://chromium.googlesource.com/chromium/src/+/a5fd3c415f3cc4f8813ba9772d83...
==========

[commit-bot: I haz the power, 2017-01-13 22:52:02]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a5fd3c415f3cc4f8813ba9772d83...

[pkalinnikov, 2017-01-17 09:54:40]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/2638873002/ by pkalinnikov@chromium.org.

The reason for reverting is: FullscreenControllerTest.FullscreenOnFileURL from
browser_tests flakes chromium.mac/Mac10.10 Tests builder..