Cleanup BubbleFrameView and BubbleBorder construction.

Cleanup BubbleFrameView and BubbleBorder construction.

Pass custom color to BubbleBorder ctor (or SK_ColorWHITE default).
Call BubbleFrameView::SetBubbleBorder instead and remove border ctor arg.
Misc refactoring and cleanup.

TODO(followup): Update bubbles to use BubbleDelegateView where possible.
TODO(followup): Use theme colors for misc borders instead of SK_ColorWHITE.

BUG=166075
TEST=No observable changes.
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180090

[msw, 2013-01-31 18:33:24]

Hey Scott, please take a look; thanks!

[sky, 2013-01-31 19:31:16]

https://codereview.chromium.org/12096084/diff/8002/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

https://codereview.chromium.org/12096084/diff/8002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:68: BubbleBorder(ArrowLocation arrow, Shadow
shadow, SkColor color);
Is there a compelling reason you're adding this? With the current approach it's
easy to get the right thing (white), but with the new approach folks have to
think about what color they should use and possibly get it wrong.

[msw, 2013-01-31 20:11:40]

https://codereview.chromium.org/12096084/diff/8002/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

https://codereview.chromium.org/12096084/diff/8002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:68: BubbleBorder(ArrowLocation arrow, Shadow
shadow, SkColor color);
On 2013/01/31 19:31:16, sky wrote:
> Is there a compelling reason you're adding this? With the current approach
it's
> easy to get the right thing (white), but with the new approach folks have to
> think about what color they should use and possibly get it wrong.

My thought was that hardcoded white is wrong, and most/all bubbles should use
theme colors. Most/all bubble should use BubbleDelegateView, which makes it easy
to use theme colors (or override with a custom color). I hope to convert a few
outliers to BubbleDelegateView in followups; others should know their color.

[sky, 2013-01-31 21:16:53]

On Thu, Jan 31, 2013 at 12:11 PM,  <msw@chromium.org> wrote:
>
>
https://codereview.chromium.org/12096084/diff/8002/ui/views/bubble/bubble_bor...
> File ui/views/bubble/bubble_border.h (right):
>
>
https://codereview.chromium.org/12096084/diff/8002/ui/views/bubble/bubble_bor...
> ui/views/bubble/bubble_border.h:68: BubbleBorder(ArrowLocation arrow,
> Shadow shadow, SkColor color);
> On 2013/01/31 19:31:16, sky wrote:
>>
>> Is there a compelling reason you're adding this? With the current
>
> approach it's
>>
>> easy to get the right thing (white), but with the new approach folks
>
> have to
>>
>> think about what color they should use and possibly get it wrong.
>
>
> My thought was that hardcoded white is wrong, and most/all bubbles
> should use theme colors. Most/all bubble should use BubbleDelegateView,
> which makes it easy to use theme colors (or override with a custom
> color). I hope to convert a few outliers to BubbleDelegateView in
> followups; others should know their color.

Shouldn't they all lookup the same theme value?

  -Scott

[msw, 2013-01-31 22:18:37]

> Shouldn't they all lookup the same theme value?

Yes, for the most part, but I think this is a good change, because:

1) It helps the exceptions that shouldn't use kColorId_WindowBackground:
a) DialogFrameView uses kColorId_DialogBackground instead.
b) MenuScrollViewContainer should probably use kColorId_MenuBackgroundColor
instead.
c) MaximizeBubbleController is blackish.

2) It just surfaces the incorrect usage of views that should be
BubbleDelegateViews:
a) FullscreenExitBubble
b) BalloonViewImpl
c) OmniboxPopupContentView

Also, TrayBubbleBorder seems like a mostly useless subclass that should be
trashed.

That said, if you disagree, I'm happy to restore the default value as white or
the window color (though the border doesn't have NativeTheme context on
construction), and let the exceptions continue to set a color after
construction.

[sky, 2013-02-01 00:07:03]

Since its even split, LGTM

[commit-bot: I haz the power, 2013-02-01 00:24:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/12096084/8002

[commit-bot: I haz the power, 2013-02-01 05:47:48]

Change committed as 180090