Add a drop shadow to the fullscreen exit bubble on windows.

Add a drop shadow to the fullscreen exit bubble on windows.

BUG=100879
TEST=Visually confirm that when entering fullscreen, the exit bubble has a drop shadow.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110465

[alicet1, 2011-11-15 02:04:22]

I think you have to seperate out the png files into a change and check that in
first. then your grd and code change together. the trybots are not friendly with
new binary files I think.

will add comments to the rest.

thanx,
alice

[koz (OOO until 15th September), 2011-11-15 03:20:46]

On 2011/11/15 02:04:22, alicet1 wrote:
> I think you have to seperate out the png files into a change and check that in
> first. then your grd and code change together. the trybots are not friendly
with
> new binary files I think.
> 
> will add comments to the rest.
> 
> thanx,
> alice

Thanks for the tip, Alice. I've uploaded a separate code review for the images
here: http://codereview.chromium.org/8567019.

[sky, 2011-11-15 05:06:37]

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.cc
File views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.c...
views/bubble/bubble_border.cc:219: arrow_offset_ = std::max(offset_x, offset_y);
This method is poorly named now that it sets both class and non-class state.
Refactor it into:
static BorderImages* GetBorderImages(Shadow shadow_type);
that returns (creating if necessary) the BorderImages*. Move the instance
specific state into the constructor and don't inline the constructor.

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h
File views/bubble/bubble_border.h (right):

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:16: struct BorderImages {
Forward declare this as a struct in BubbleBorder. Also, add a constructor to
NULL everything out.

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:30: 
nit: remove one of these lines.

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:167: SkBitmap* left_;
It's weird to have this and the struct. How about adding a field of type
BorderImages* that is one of normal_images_ or shadow_images_. Or cache Shadow
and look up the BorderImages when needed.

[koz (OOO until 15th September), 2011-11-15 06:35:43]

Thanks for the comments, Scott.

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.cc
File views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.c...
views/bubble/bubble_border.cc:219: arrow_offset_ = std::max(offset_x, offset_y);
On 2011/11/15 05:06:37, sky wrote:
> This method is poorly named now that it sets both class and non-class state.
> Refactor it into:
> static BorderImages* GetBorderImages(Shadow shadow_type);
> that returns (creating if necessary) the BorderImages*. Move the instance
> specific state into the constructor and don't inline the constructor.

Done.

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h
File views/bubble/bubble_border.h (right):

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:16: struct BorderImages {
On 2011/11/15 05:06:37, sky wrote:
> Forward declare this as a struct in BubbleBorder. Also, add a constructor to
> NULL everything out.

I'm not sure I've understood you here - I've added the constructor and moved the
declaration of this struct into BubbleBorder. Is that right?

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:30: 
On 2011/11/15 05:06:37, sky wrote:
> nit: remove one of these lines.

Done.

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:167: SkBitmap* left_;
On 2011/11/15 05:06:37, sky wrote:
> It's weird to have this and the struct. How about adding a field of type
> BorderImages* that is one of normal_images_ or shadow_images_. Or cache Shadow
> and look up the BorderImages when needed.

Done.

[sky, 2011-11-15 16:36:33]

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h
File views/bubble/bubble_border.h (right):

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:16: struct BorderImages {
On 2011/11/15 06:35:43, koz wrote:
> On 2011/11/15 05:06:37, sky wrote:
> > Forward declare this as a struct in BubbleBorder. Also, add a constructor to
> > NULL everything out.
> 
> I'm not sure I've understood you here - I've added the constructor and moved
the
> declaration of this struct into BubbleBorder. Is that right?

Close. Since you don't actually need the implementation in the header I'm saying
forward declare it in the header (private section), then put the implementation
in the .cc.

http://codereview.chromium.org/8565034/diff/2008/views/bubble/bubble_border.cc
File views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/8565034/diff/2008/views/bubble/bubble_border.c...
views/bubble/bubble_border.cc:28: : override_arrow_offset_(0),
spacing is off.

[alicet1, 2011-11-15 16:51:12]

LGTM

http://codereview.chromium.org/8565034/diff/2008/views/bubble/border_contents...
File views/bubble/border_contents_view.cc (right):

http://codereview.chromium.org/8565034/diff/2008/views/bubble/border_contents...
views/bubble/border_contents_view.cc:81: bubble_border_ = new
BubbleBorder(arrow_location,
can you add a todo for me that we need to expose shadow option in
BorderContentsView when we move your bubble over?

[koz (OOO until 15th September), 2011-11-15 22:45:39]

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h
File views/bubble/bubble_border.h (right):

http://codereview.chromium.org/8565034/diff/6002/views/bubble/bubble_border.h...
views/bubble/bubble_border.h:16: struct BorderImages {
On 2011/11/15 16:36:33, sky wrote:
> On 2011/11/15 06:35:43, koz wrote:
> > On 2011/11/15 05:06:37, sky wrote:
> > > Forward declare this as a struct in BubbleBorder. Also, add a constructor
to
> > > NULL everything out.
> > 
> > I'm not sure I've understood you here - I've added the constructor and moved
> the
> > declaration of this struct into BubbleBorder. Is that right?
> 
> Close. Since you don't actually need the implementation in the header I'm
saying
> forward declare it in the header (private section), then put the
implementation
> in the .cc.

Thanks for the clarification. Done.

http://codereview.chromium.org/8565034/diff/2008/views/bubble/border_contents...
File views/bubble/border_contents_view.cc (right):

http://codereview.chromium.org/8565034/diff/2008/views/bubble/border_contents...
views/bubble/border_contents_view.cc:81: bubble_border_ = new
BubbleBorder(arrow_location,
On 2011/11/15 16:51:12, alicet1 wrote:
> can you add a todo for me that we need to expose shadow option in
> BorderContentsView when we move your bubble over? 

Done.

http://codereview.chromium.org/8565034/diff/2008/views/bubble/bubble_border.cc
File views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/8565034/diff/2008/views/bubble/bubble_border.c...
views/bubble/bubble_border.cc:28: : override_arrow_offset_(0),
On 2011/11/15 16:36:34, sky wrote:
> spacing is off.

Done.

[sky, 2011-11-16 02:29:51]

LGTM

[commit-bot: I haz the power, 2011-11-17 00:57:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/8565034/8007

[commit-bot: I haz the power, 2011-11-17 06:10:30]

Can't apply patch for file
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/fullscreen_exit_bubble_views.cc
Hunk #1 FAILED at 73.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc.rej

[commit-bot: I haz the power, 2011-11-17 06:22:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/8565034/16002

[commit-bot: I haz the power, 2011-11-17 07:33:34]

Change committed as 110465