Consolidate bubble border code.

Consolidate bubble border code.

Per 135200, we should only use image based code.
- Add two new sets of bubble border images;
- Use the new bubble border images for app menu and launcher tooltip;
- Get rid of BubbleBorder2 since it's no longer needed;

BUG=135200
TEST=Verify app list, launcher bar tooltip bubbles use shadow based on image and no artifacts.

R=msw@chromium.org,sky@chromium.org,oshima@chromium.org

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

[xiyuan, 2012-09-17 21:39:30]

Suggested split:
msw for ui/views/bubble/*
sky for ash/*, app_list/*
oshima for ui_resources.grd

Thanks.

[sky, 2012-09-17 22:44:04]

http://codereview.chromium.org/10905311/diff/1020/ui/app_list/app_list_backgr...
File ui/app_list/app_list_background.h (right):

http://codereview.chromium.org/10905311/diff/1020/ui/app_list/app_list_backgr...
ui/app_list/app_list_background.h:29: int corner_radius_;
const

[xiyuan, 2012-09-18 16:19:06]

http://codereview.chromium.org/10905311/diff/1020/ui/app_list/app_list_backgr...
File ui/app_list/app_list_background.h (right):

http://codereview.chromium.org/10905311/diff/1020/ui/app_list/app_list_backgr...
ui/app_list/app_list_background.h:29: int corner_radius_;
On 2012/09/17 22:44:04, sky wrote:
> const

Done.

[xiyuan, 2012-09-18 17:20:20]

+jennyz, stevenjb

[msw, 2012-09-18 19:33:42]

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:55: // Offsets of where arrow tip pixel is in
corresponding image assets.
nit: consider "Location of the arrow tip pixels in corr..."

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:56: gfx::Point left_arrow_tip;
We can derive the tip position from the image and |arrow_interior_height|.
Defining these points should not be necessary.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:62: int left_border_padding;
We can derive the image padding from the image and |arrow_interior_height|.
Defining these sizes should not be necessary.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:86: // Calculate horizontal and vertical insets
for arrow by ensuring that
Should this be moved into CalculateArrowOffset?

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:89:
std::max(std::max(images_->top_arrow_tip.x(),
It seems like you could assume that the arrow images are symmetrical with the
arrow in the center and avoid the new std::max you're adding here.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:260: offset = SanitizeArrowOffset(offset,
border_size);
Nix this, its redundant with the call in CalculateArrowOffset.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:270: if (images == NULL) {
Can you invert this check to make it an early return?
Smaller indenting below might make more one-line statements.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:274: switch (shadow) {
This is getting unwieldy, perhaps BorderImages needs a ctor?
Also, consider defining a static array of IDR arrays or similar.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:284: images->left_arrow = new gfx::ImageSkia();
These seem to be leaked (prior to this CL, not your fault).
Please add a TODO (for yourself, the original author, or me).

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:288: images->arrow_interior_height = 7;
This group has no arrow, why is arrow_interior_height 7?

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:393: case SHADOW_COUNT:
Make this (or add a similar) default: case.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:619: int BubbleBorder::SanitizeArrowOffset(int
arrow_offset,
After removing the call in SetArrowOffset, inline this in CalculateArrowOffset.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:628: if (override_arrow_offset_)
Add braces here to match the else case.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:47: SHADOW_COUNT
nit: add a comma (same for ArrowLocation if you don't mind).

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:61: // TODO(xiyuan): Get rid of this since it's
part of BorderImages now?
Yeah, GetCornerRadius and GetBorderCornerRadius are redundant.
Did you mean to address this now or later? (later is ok w/me)

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:110: void set_relative_arrow_offset(int
relative_arrow_offset) {
nit: fits on one line if you s/relative_arrow_offset/offset/

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:125: // Returns the corner radius of current
image set.
nit: "*the* current"

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:190: // Relative offset to move current arrow.
It is used for move bubble relative
nit: grammar "to move the" twice.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:192: int relative_arrow_offset_;
Instead of adding a new offset (in addition to |arrow_offset_| and
|override_arrow_offset_|), why not set |override_arrow_offset_| in
BubbleFrameView when the bubble is offscreen?

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:204: bool center_arrow_;
This state should be merged into the ArrowLocation enum.
TOP_CENTER is clearer than TOP_[LEFT|RIGHT] + center_arrow.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:71: void
set_border_shadow(BubbleBorder::Shadow border_shadow) {
nit: this fits on one line if you s/|border_shadow|/|shadow|/

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:95: bool try_fit_bubble() const { return
try_fit_bubble_; }
This name seems worse; consider adjust_if_offscreen or similar.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:206: // If true (defaults to true), the arrow
may be mirrored and moved o fit the
nit: s/o/to

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:207: // bubble on screen better. It would be a
no-op if bubble has no arrow.
nit: "*the* bubble"

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.cc (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:89: OffsetArrowIfOffScreen(anchor_rect,
client_size);
This is only useful for centered arrows, right? Limit calls to that?

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:137: // Get desired bubble bounds without
adjustment.
grammar nit: "Get the..."

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:138: const int old_adjust =
bubble_border_->relative_arrow_offset();
Use the existing |override_arrow_offset_|; nix new |relative_arrow_offset_|.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:146: // Calculate off screen adjustment
nit: hyphen off-screen or single word; add a period.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.h (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.h:66: // Move arrow (by setting its offset) to
make bubble fit in the monitor
nit: grammar; consider: "Adjust the bubble's arrow offsets if the generated
window bounds don't fit in the monitor bounds."

[msw, 2012-09-18 19:36:13]

Also, why did we decide to go exclusively with image borders?
The bug doesn't actually say why, and I think Skia drawing commands would be
preferable to a variety of image assets.

[xiyuan, 2012-09-18 19:51:16]

On 2012/09/18 19:36:13, msw wrote:
> Also, why did we decide to go exclusively with image borders?
> The bug doesn't actually say why, and I think Skia drawing commands would be
> preferable to a variety of image assets.

The skia drawing code is no longer in use after app menu and launcher tooltip
switches to use image based bubble border. My impression is that unused code
should be deleted since we could resurrect them from history if needed.

The reason to switch to image based border for app menu and launcher tooltip is
that we want crispy 1px border on HiDPI system. Right now, we draw in a canvas
with scale factor 2 on such system and it would tricky to draw 1px thin lines.
Not sure if setting stroke width to 0.5 would work. And even if that works, then
we need to differentiate the underlying system in drawing code.

[msw, 2012-09-18 19:57:31]

On 2012/09/18 19:51:16, xiyuan wrote:
> On 2012/09/18 19:36:13, msw wrote:
> > Also, why did we decide to go exclusively with image borders?
> > The bug doesn't actually say why, and I think Skia drawing commands would be
> > preferable to a variety of image assets.
> 
> The skia drawing code is no longer in use after app menu and launcher tooltip
> switches to use image based bubble border. My impression is that unused code
> should be deleted since we could resurrect them from history if needed.
> 
> The reason to switch to image based border for app menu and launcher tooltip
is
> that we want crispy 1px border on HiDPI system. Right now, we draw in a canvas
> with scale factor 2 on such system and it would tricky to draw 1px thin lines.
> Not sure if setting stroke width to 0.5 would work. And even if that works,
then
> we need to differentiate the underlying system in drawing code.

Ah, using images for HiDPI support seems reasonable (for now), and I definitely
agree that we can resurrect the code later if needed. +cc varunjain who wrote
bubble_border_2 for fyi.

[xiyuan, 2012-09-19 18:20:09]

I have updated to address all comments. The arrow images are changed to be
symmetric so that we could revert to previous code that assumes arrow tip is in
middle of image.

For oshima, the images are from UX and updated on drive as well.

Please take another look. Thanks.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:55: // Offsets of where arrow tip pixel is in
corresponding image assets.
On 2012/09/18 19:33:42, msw wrote:
> nit: consider "Location of the arrow tip pixels in corr..."

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:56: gfx::Point left_arrow_tip;
On 2012/09/18 19:33:42, msw wrote:
> We can derive the tip position from the image and |arrow_interior_height|.
> Defining these points should not be necessary.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:62: int left_border_padding;
On 2012/09/18 19:33:42, msw wrote:
> We can derive the image padding from the image and |arrow_interior_height|.
> Defining these sizes should not be necessary.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:86: // Calculate horizontal and vertical insets
for arrow by ensuring that
On 2012/09/18 19:33:42, msw wrote:
> Should this be moved into CalculateArrowOffset?

|arrow_offset_| is needed to get minimum bubble size in Getbounds. We might want
to keep it here to set |arrow_offset_|.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:89:
std::max(std::max(images_->top_arrow_tip.x(),
On 2012/09/18 19:33:42, msw wrote:
> It seems like you could assume that the arrow images are symmetrical with the
> arrow in the center and avoid the new std::max you're adding here.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:260: offset = SanitizeArrowOffset(offset,
border_size);
On 2012/09/18 19:33:42, msw wrote:
> Nix this, its redundant with the call in CalculateArrowOffset.

Done. And made SetArrowOffset an inline accessor.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:270: if (images == NULL) {
On 2012/09/18 19:33:42, msw wrote:
> Can you invert this check to make it an early return?
> Smaller indenting below might make more one-line statements.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:274: switch (shadow) {
On 2012/09/18 19:33:42, msw wrote:
> This is getting unwieldy, perhaps BorderImages needs a ctor?
> Also, consider defining a static array of IDR arrays or similar.

Done. Added IDR arrays and pass it to BorderImage's ctor to load image
resources.

Change resource loading function from GetImageSkiaNamed to
GetImageName().ToImageSkia because the later is the preferred way.

Also changed ImageSkia member from pointer to object. Assigning ImageSkia is
cheap similar to SkBitmap, it is a add ref op on its internal storage.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:284: images->left_arrow = new gfx::ImageSkia();
On 2012/09/18 19:33:42, msw wrote:
> These seem to be leaked (prior to this CL, not your fault).
> Please add a TODO (for yourself, the original author, or me).

Resolved as we use ImageSkia object instead of pointer and no longer allocates
these dummy ImageSkias.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:288: images->arrow_interior_height = 7;
On 2012/09/18 19:33:42, msw wrote:
> This group has no arrow, why is arrow_interior_height 7?

Removed since it's not used and we should be okay with default 0.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:393: case SHADOW_COUNT:
On 2012/09/18 19:33:42, msw wrote:
> Make this (or add a similar) default: case.

Done.

I found not using "default" for enum based switch has a benefit to catch missing
values at compile time. But I am okay either way.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:619: int BubbleBorder::SanitizeArrowOffset(int
arrow_offset,
On 2012/09/18 19:33:42, msw wrote:
> After removing the call in SetArrowOffset, inline this in
CalculateArrowOffset.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:628: if (override_arrow_offset_)
On 2012/09/18 19:33:42, msw wrote:
> Add braces here to match the else case.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:47: SHADOW_COUNT
On 2012/09/18 19:33:42, msw wrote:
> nit: add a comma (same for ArrowLocation if you don't mind).

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:61: // TODO(xiyuan): Get rid of this since it's
part of BorderImages now?
On 2012/09/18 19:33:42, msw wrote:
> Yeah, GetCornerRadius and GetBorderCornerRadius are redundant.
> Did you mean to address this now or later? (later is ok w/me)

I'll do it later in a clean up CL. Right now, this function is used like a theme
param and used in places that are not related to bubble at all, such as omnibox
popup and balloon view. We probably want this const to defined somewhere else
instead of here.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:110: void set_relative_arrow_offset(int
relative_arrow_offset) {
On 2012/09/18 19:33:42, msw wrote:
> nit: fits on one line if you s/relative_arrow_offset/offset/

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:125: // Returns the corner radius of current
image set.
On 2012/09/18 19:33:42, msw wrote:
> nit: "*the* current"

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:190: // Relative offset to move current arrow.
It is used for move bubble relative
On 2012/09/18 19:33:42, msw wrote:
> nit: grammar "to move the" twice.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:192: int relative_arrow_offset_;
On 2012/09/18 19:33:42, msw wrote:
> Instead of adding a new offset (in addition to |arrow_offset_| and
> |override_arrow_offset_|), why not set |override_arrow_offset_| in
> BubbleFrameView when the bubble is offscreen?

Initially my concern is that if we set |override_arrow_offset_|, we lose it and
would not be able to revert to original value.

e.g. if user sets |override_arrow_offset_| to some value and the bubble
initially could not fit on screen, so we change |override_arrow_offset_|. Then,
things change (anchor moved, screen size changed etc) and the bubble could now
fit. And we would not be able to restore to user set value.

Then I checked that the only user of override_arrow_offset_ is in launcher
Overflow bubble code which does offscreen adjustment by itself. So the concern
is not valid, at least not with the current trunk code.

Removed as suggested.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:204: bool center_arrow_;
On 2012/09/18 19:33:42, msw wrote:
> This state should be merged into the ArrowLocation enum.
> TOP_CENTER is clearer than TOP_[LEFT|RIGHT] + center_arrow.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:71: void
set_border_shadow(BubbleBorder::Shadow border_shadow) {
On 2012/09/18 19:33:42, msw wrote:
> nit: this fits on one line if you s/|border_shadow|/|shadow|/

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:95: bool try_fit_bubble() const { return
try_fit_bubble_; }
On 2012/09/18 19:33:42, msw wrote:
> This name seems worse; consider adjust_if_offscreen or similar.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:206: // If true (defaults to true), the arrow
may be mirrored and moved o fit the
On 2012/09/18 19:33:42, msw wrote:
> nit: s/o/to

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:207: // bubble on screen better. It would be a
no-op if bubble has no arrow.
On 2012/09/18 19:33:42, msw wrote:
> nit: "*the* bubble"

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.cc (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:89: OffsetArrowIfOffScreen(anchor_rect,
client_size);
On 2012/09/18 19:33:42, msw wrote:
> This is only useful for centered arrows, right? Limit calls to that?

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:137: // Get desired bubble bounds without
adjustment.
On 2012/09/18 19:33:42, msw wrote:
> grammar nit: "Get the..."

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:138: const int old_adjust =
bubble_border_->relative_arrow_offset();
On 2012/09/18 19:33:42, msw wrote:
> Use the existing |override_arrow_offset_|; nix new |relative_arrow_offset_|.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:146: // Calculate off screen adjustment
On 2012/09/18 19:33:42, msw wrote:
> nit: hyphen off-screen or single word; add a period.

Done.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.h (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.h:66: // Move arrow (by setting its offset) to
make bubble fit in the monitor
On 2012/09/18 19:33:42, msw wrote:
> nit: grammar; consider: "Adjust the bubble's arrow offsets if the generated
> window bounds don't fit in the monitor bounds."

Done.

[msw, 2012-09-19 20:12:31]

Patch Set 4's bubble changes are significantly better; thank you for going the
extra mile there! (still have some comments)

Please update/add unit tests for the new centered arrow types/helpers as well as
for the offset adjustment/validation.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:86: // Calculate horizontal and vertical insets
for arrow by ensuring that
On 2012/09/19 18:20:09, xiyuan wrote:
> On 2012/09/18 19:33:42, msw wrote:
> > Should this be moved into CalculateArrowOffset?
> 
> |arrow_offset_| is needed to get minimum bubble size in Getbounds. We might
want
> to keep it here to set |arrow_offset_|.

ok.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:260: offset = SanitizeArrowOffset(offset,
border_size);
On 2012/09/19 18:20:09, xiyuan wrote:
> On 2012/09/18 19:33:42, msw wrote:
> > Nix this, its redundant with the call in CalculateArrowOffset.
> 
> Done. And made SetArrowOffset an inline accessor.

Nice.

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:274: switch (shadow) {
On 2012/09/19 18:20:09, xiyuan wrote:
> On 2012/09/18 19:33:42, msw wrote:
> > This is getting unwieldy, perhaps BorderImages needs a ctor?
> > Also, consider defining a static array of IDR arrays or similar.
> 
> Done. Added IDR arrays and pass it to BorderImage's ctor to load image
> resources.
> 
> Change resource loading function from GetImageSkiaNamed to
> GetImageName().ToImageSkia because the later is the preferred way.
> 
> Also changed ImageSkia member from pointer to object. Assigning ImageSkia is
> cheap similar to SkBitmap, it is a add ref op on its internal storage.
> 

All excellent (though GetImageSkiaNamed was probably ok).

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:284: images->left_arrow = new gfx::ImageSkia();
On 2012/09/19 18:20:09, xiyuan wrote:
> On 2012/09/18 19:33:42, msw wrote:
> > These seem to be leaked (prior to this CL, not your fault).
> > Please add a TODO (for yourself, the original author, or me).
> 
> Resolved as we use ImageSkia object instead of pointer and no longer allocates
> these dummy ImageSkias.

Great!

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:393: case SHADOW_COUNT:
On 2012/09/19 18:20:09, xiyuan wrote:
> On 2012/09/18 19:33:42, msw wrote:
> > Make this (or add a similar) default: case.
> 
> Done.
> 
> I found not using "default" for enum based switch has a benefit to catch
missing
> values at compile time. But I am okay either way.

Good point, feel free to switch it back; thanks for the tip!

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:61: // TODO(xiyuan): Get rid of this since it's
part of BorderImages now?
On 2012/09/19 18:20:09, xiyuan wrote:
> On 2012/09/18 19:33:42, msw wrote:
> > Yeah, GetCornerRadius and GetBorderCornerRadius are redundant.
> > Did you mean to address this now or later? (later is ok w/me)
> 
> I'll do it later in a clean up CL. Right now, this function is used like a
theme
> param and used in places that are not related to bubble at all, such as
omnibox
> popup and balloon view. We probably want this const to defined somewhere else
> instead of here.

sgtm

http://codereview.chromium.org/10905311/diff/10001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:192: int relative_arrow_offset_;
On 2012/09/19 18:20:09, xiyuan wrote:
> On 2012/09/18 19:33:42, msw wrote:
> > Instead of adding a new offset (in addition to |arrow_offset_| and
> > |override_arrow_offset_|), why not set |override_arrow_offset_| in
> > BubbleFrameView when the bubble is offscreen?
> 
> Initially my concern is that if we set |override_arrow_offset_|, we lose it
and
> would not be able to revert to original value.
> 
> e.g. if user sets |override_arrow_offset_| to some value and the bubble
> initially could not fit on screen, so we change |override_arrow_offset_|.
Then,
> things change (anchor moved, screen size changed etc) and the bubble could now
> fit. And we would not be able to restore to user set value.
> 
> Then I checked that the only user of override_arrow_offset_ is in launcher
> Overflow bubble code which does offscreen adjustment by itself. So the concern
> is not valid, at least not with the current trunk code.
> 
> Removed as suggested.

Great! Thanks for being thorough!

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:136: BubbleBorder::border_images_[SHADOW_COUNT]
= { NULL };
nit: indent by 4 spaces.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:254: kBorderStrokeSize;
nit: indent four more spaces.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:336: return std::max(arrow_offset_,
nit: add a comment on what clamping/validation this does.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:338: border_size.width() - arrow_offset_:
nit: space before colon.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:344: CHECK(shadow < SHADOW_COUNT);
nit: CHECK_LT

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:27: // 3 bit specifies whether arrow at the
center of its residing edge.
grammar nit: "*the* arrow *is*"

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:89: return (loc == LEFT_CENTER || loc ==
BOTTOM_CENTER || loc >= NONE) ?
s/BOTTOM_CENTER/RIGHT_CENTER/

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:123: // location to make that happen.
good explanation!

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.cc (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:160: if
(BubbleBorder::is_arrow_at_center(arrow) ||
After diagramming some cases I think |offscreen_adjust| should *always* be
negated; that tries to move the bubble onscreen, though some resulting offsets
will be clamped correctly by the border. If that's correct, please remove this
block and either flip the calculations above, or subtract |offscreen_adjust|
from the existing offset below.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:170: int arrow_offset =
bubble_border_->GetArrowOffset(window_bounds.size()) +
nit: send the result to set_arrow_offset directly; nix local.

[sky, 2012-09-19 21:18:05]

LGTM for the non ui/views/bubble code. Wait for Mike on the ui/views/bubble
part.

[xiyuan, 2012-09-19 22:51:35]

Addressed comments in #4 and added tests for arrow types/helpers and offscreen
adjustment. 

The tests actually found some unexpected return values from helpers for the new
arrow types and fixed them by assigning better values to the enum.

Please take another look. Thanks.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:136: BubbleBorder::border_images_[SHADOW_COUNT]
= { NULL };
On 2012/09/19 20:12:32, msw wrote:
> nit: indent by 4 spaces.

Done.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:254: kBorderStrokeSize;
On 2012/09/19 20:12:32, msw wrote:
> nit: indent four more spaces.

Done.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:336: return std::max(arrow_offset_,
On 2012/09/19 20:12:32, msw wrote:
> nit: add a comment on what clamping/validation this does.

Done.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:338: border_size.width() - arrow_offset_:
On 2012/09/19 20:12:32, msw wrote:
> nit: space before colon.

I changed the expression in the wrong way (did not notice the border_size part
is wrapped in a bracket). The original code is correct. Reverted my change in
this line. And the nit is obsolete because of that.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:344: CHECK(shadow < SHADOW_COUNT);
On 2012/09/19 20:12:32, msw wrote:
> nit: CHECK_LT

Done.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:27: // 3 bit specifies whether arrow at the
center of its residing edge.
On 2012/09/19 20:12:32, msw wrote:
> grammar nit: "*the* arrow *is*"

Done.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:89: return (loc == LEFT_CENTER || loc ==
BOTTOM_CENTER || loc >= NONE) ?
On 2012/09/19 20:12:32, msw wrote:
> s/BOTTOM_CENTER/RIGHT_CENTER/

Oops. Fixed now. :p

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:123: // location to make that happen.
On 2012/09/19 20:12:32, msw wrote:
> good explanation!

hehe... :) I wish I could write something this good.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.cc (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:160: if
(BubbleBorder::is_arrow_at_center(arrow) ||
On 2012/09/19 20:12:32, msw wrote:
> After diagramming some cases I think |offscreen_adjust| should *always* be
> negated; that tries to move the bubble onscreen, though some resulting offsets
> will be clamped correctly by the border. If that's correct, please remove this
> block and either flip the calculations above, or subtract |offscreen_adjust|
> from the existing offset below.

Since we only run this function for center arrows, yes, it should always be
negated.

Before I was planning to use the function for right and bottom arrows, e.g.
TOP_RIGHT case. In this situation, if bubble goes off left screen edge,
|offscreen_adjust| is positive and we need to move arrow to left. But because
arrow offset is defined as distance from starting edge, which is right for
TOP_RIGHT case. We actually need to increase arrow offset, that is keep the
positive value and add to existing arrow offset.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:170: int arrow_offset =
bubble_border_->GetArrowOffset(window_bounds.size()) +
On 2012/09/19 20:12:32, msw wrote:
> nit: send the result to set_arrow_offset directly; nix local.

Done.

[msw, 2012-09-20 00:07:23]

LGTM; very nice work! All my comments are nits/optional.

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.cc (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:160: if
(BubbleBorder::is_arrow_at_center(arrow) ||
On 2012/09/19 22:51:36, xiyuan wrote:
> On 2012/09/19 20:12:32, msw wrote:
> > After diagramming some cases I think |offscreen_adjust| should *always* be
> > negated; that tries to move the bubble onscreen, though some resulting
offsets
> > will be clamped correctly by the border. If that's correct, please remove
this
> > block and either flip the calculations above, or subtract |offscreen_adjust|
> > from the existing offset below.
> 
> Since we only run this function for center arrows, yes, it should always be
> negated.
> 
> Before I was planning to use the function for right and bottom arrows, e.g.
> TOP_RIGHT case. In this situation, if bubble goes off left screen edge,
> |offscreen_adjust| is positive and we need to move arrow to left. But because
> arrow offset is defined as distance from starting edge, which is right for
> TOP_RIGHT case. We actually need to increase arrow offset, that is keep the
> positive value and add to existing arrow offset.

Ahhh, I mistakenly thought the offset always used canonical x/y signs, but
you're right, sorry. Feel free to revert for future-proofing, or leave as-is
since its sufficient for centered arrows, up to you.

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:414: !is_arrow_on_left(arrow_location_)) {
nit: indent one more space.

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:37: TOP_CENTER    = 9,
Would it make more sense to specify these with bitwise ops?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view_unittest.cc (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:24: const int kIgnore = -1;
nit: remove

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:291: EXPECT_GT(window_bounds.x() +
window_bounds.width() / 2, 125);
GT 125 seems too loose, it's EQ 250, right?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:303: EXPECT_GT(window_bounds.x() +
window_bounds.width() / 2, 125);
ditto: GT 125 seems too loose, it's EQ 250, right?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:315: EXPECT_LT(window_bounds.x() +
window_bounds.width() / 2, 925);
similar: LT 925 seems too loose, it's EQ 750, right?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:327: EXPECT_LT(window_bounds.x() +
window_bounds.width() / 2, 925);
ditto: LT 925 seems too loose, it's EQ 750, right?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:339: EXPECT_GT(window_bounds.y() +
window_bounds.height() / 2, 125);
similar: GT 125 seems too loose, it's EQ 250, right?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:351: EXPECT_GT(window_bounds.y() +
window_bounds.height() / 2, 125);
ditto: GT 125 seems too loose, it's EQ 250, right?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:363: EXPECT_LT(window_bounds.y() +
window_bounds.height() / 2, 925);
similar: LT 925 seems too loose, it's EQ 750, right?

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:375: EXPECT_LT(window_bounds.y() +
window_bounds.height() / 2, 925);
ditto: LT 925 seems too loose, it's EQ 750, right?

[xiyuan, 2012-09-20 16:56:51]

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.cc (right):

http://codereview.chromium.org/10905311/diff/22002/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.cc:160: if
(BubbleBorder::is_arrow_at_center(arrow) ||
On 2012/09/20 00:07:23, msw wrote:
> On 2012/09/19 22:51:36, xiyuan wrote:
> > On 2012/09/19 20:12:32, msw wrote:
> > > After diagramming some cases I think |offscreen_adjust| should *always* be
> > > negated; that tries to move the bubble onscreen, though some resulting
> offsets
> > > will be clamped correctly by the border. If that's correct, please remove
> this
> > > block and either flip the calculations above, or subtract
|offscreen_adjust|
> > > from the existing offset below.
> > 
> > Since we only run this function for center arrows, yes, it should always be
> > negated.
> > 
> > Before I was planning to use the function for right and bottom arrows, e.g.
> > TOP_RIGHT case. In this situation, if bubble goes off left screen edge,
> > |offscreen_adjust| is positive and we need to move arrow to left. But
because
> > arrow offset is defined as distance from starting edge, which is right for
> > TOP_RIGHT case. We actually need to increase arrow offset, that is keep the
> > positive value and add to existing arrow offset.
> 
> Ahhh, I mistakenly thought the offset always used canonical x/y signs, but
> you're right, sorry. Feel free to revert for future-proofing, or leave as-is
> since its sufficient for centered arrows, up to you.

I'll keep it as it is now. I have a DCHECK at the entry to make sure the logic
here only runs for center arrows. We could always add it back when needed later.

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:414: !is_arrow_on_left(arrow_location_)) {
On 2012/09/20 00:07:23, msw wrote:
> nit: indent one more space.

Done.

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view_unittest.cc (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:24: const int kIgnore = -1;
On 2012/09/20 00:07:23, msw wrote:
> nit: remove

Done.

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view_unittest.cc:291: EXPECT_GT(window_bounds.x() +
window_bounds.width() / 2, 125);
On 2012/09/20 00:07:23, msw wrote:
> GT 125 seems too loose, it's EQ 250, right?

This is intended to verify that bubble's center is to the right of anchor's
center because of the adjustment.

But seem unnecessary since we already checked that bubble's left is at the
monitor's left edge before this statement and bubble's arrow is pointing at
anchor's center after this.

Removed this check.

[msw, 2012-09-20 17:03:33]

One more nit.

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:98: left =
*rb.GetImageNamed(image_ids[0]).ToImageSkia();
I know the ResourceBundle header says GetImageNamed is preferred for
cross-platform portability, but in Views-exclusive code, ImageSkia /
GetImageSkiaNamed is actually preferred (and more concise). See this relevant
chromium-dev thread:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/IXPJs131xCc/dis...

[oshima, 2012-09-20 17:17:47]

ui_resources lgtm

[xiyuan, 2012-09-20 17:30:56]

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:98: left =
*rb.GetImageNamed(image_ids[0]).ToImageSkia();
On 2012/09/20 17:03:34, msw wrote:
> I know the ResourceBundle header says GetImageNamed is preferred for
> cross-platform portability, but in Views-exclusive code, ImageSkia /
> GetImageSkiaNamed is actually preferred (and more concise). See this relevant
> chromium-dev thread:
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/IXPJs131xCc/dis...
> 

Aha... Thanks for the tip. Reverted back to use GetImageSkiaNamed.

[msw, 2012-09-20 17:40:09]

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10905311/diff/24034/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.cc:98: left =
*rb.GetImageNamed(image_ids[0]).ToImageSkia();
On 2012/09/20 17:30:56, xiyuan wrote:
> On 2012/09/20 17:03:34, msw wrote:
> > I know the ResourceBundle header says GetImageNamed is preferred for
> > cross-platform portability, but in Views-exclusive code, ImageSkia /
> > GetImageSkiaNamed is actually preferred (and more concise). See this
relevant
> > chromium-dev thread:
> >
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/IXPJs131xCc/dis...
> > 
> 
> Aha... Thanks for the tip. Reverted back to use GetImageSkiaNamed.

np, I'm updating the ambiguous comment at
https://codereview.chromium.org/10970018/

http://codereview.chromium.org/10905311/diff/38003/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/38003/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:55: BIG_SHADOW,
Why did you remove the #if defined(USE_AURA) here?
kBigShadowImages and kSmalleShadowImages are still Aura-only...

[xiyuan, 2012-09-20 17:58:29]

http://codereview.chromium.org/10905311/diff/38003/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10905311/diff/38003/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:55: BIG_SHADOW,
On 2012/09/20 17:40:09, msw wrote:
> Why did you remove the #if defined(USE_AURA) here?
> kBigShadowImages and kSmalleShadowImages are still Aura-only...

Right now app list is in progress of porting to Windows and is using the deleted
BubbleBorder2. So the big shadow images are needed on Windows.

Small shadow images are not needed on other platforms at the moment. When we
clearly know that they are not needed on other platforms, we need to move the
resource somewhere else (probably into ash_resources) instead of here. I am
thinking of doing that later when things are more clear and we need to move
them. Think that requires to further refactor bubble border to allow passing
images from external since views code should not directly depends on
ash_resources.

And yes, I forgot to clear that up. :(

[msw, 2012-09-20 18:05:33]

LGTM