ui/gfx: Make the first version of Canvas::TileImageInt take a gfx::Rect.

ui/gfx: Make the first version of Canvas::TileImageInt take a gfx::Rect.

BUG=100898
R=pkasting@chromium.org
TBR=sky@chromium.org

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

[Peter Kasting, 2012-02-06 22:32:39]

LGTM

http://codereview.chromium.org/9332006/diff/2001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

http://codereview.chromium.org/9332006/diff/2001/ash/launcher/launcher.cc#new...
ash/launcher/launcher.cc:34: virtual void Paint(int w, int h, gfx::Canvas*
canvas) OVERRIDE {
Nit: At some point we should make this Paint function take a gfx::Size()

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:806: 0, width() -
frame_edges.top_right->width(), frame_edges.top->height()));
Nit: While here, this has a bug; third arg should be "width() -
frame_edges.top_left->width() - frame_edges.top_right->width()"

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/abou...
File chrome/browser/ui/views/about_chrome_view.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/abou...
chrome/browser/ui/views/about_chrome_view.cc:437:
ui::ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont);
Nit: Why add this qualifier if it works without it?

You also did this in some previous patch and I forgot to ask why or suggest not
doing it.

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/app_panel_browser_frame_view.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_panel_browser_frame_view.cc:365: width() -
top_right_corner->width(), top_edge->height()));
Nit: While here, this has a bug; third arg should be "width() -
top_left_corner_width() - top_right_corner->width()"

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_panel_browser_frame_view.cc:392:
ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
Nit: Same nit about qualifiers

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab.cc:489:
canvas->TileImage(*tab_inactive_image->image_c,
Nit: Shorter:

  gfx::Rect tile_rect(GetLocalBounds());
  tile_rect.Inset(tab_inactive_image->l_width, 0, tab_inactive_image->r_width,
                  0);
  canvas->TileImage(*tab_inactive_image->image_c, tile_rect);
  canvas->DrawBitmapInt(*tab_inactive_image->image_r, tile_rect.right(), 0);

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab.cc:534: canvas->TileImage(*tab_image->image_c,
gfx::Rect(tab_image->l_width, 0,
Nit: Possibly clearer:

  gfx::Rect tile_rect(GetLocalBounds());
  tile_rect.Inset(tab_image->l_width, 0, tab_image->r_width, 0);
  canvas->TileImage(*tab_image->image_c, tile_rect);
  canvas->DrawBitmapInt(*tab_image->image_r, tile_rect.right(), 0);

http://codereview.chromium.org/9332006/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_action.cc:149:
SkScalarFloor(SkIntToScalar(bounds.x()) +
Nit: While here, convert SkIntToScalar(SkScalarFloor(...)) to
SkScalarFloorToScalar(...)

http://codereview.chromium.org/9332006/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_action.cc:175:
canvas->TileImage(*gradient_center,
Nit: Simpler:

  gfx::Rect tile_rect(gfx::SkRectToRect(rect));
  tile_rect.Inset(gradient_left->width(), 0, gradient_right->width(), 0);
  canvas->TileImage(*gradient_center, tile_rect);

http://codereview.chromium.org/9332006/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/9332006/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_win.cc:1136:
canvas.TileImage(background_,
Nit: Simpler:

  dc_rect.set_origin(gfx::Point());
  canvas.TileImage(background_, dc_rect);

http://codereview.chromium.org/9332006/diff/2001/ui/views/controls/scrollbar/...
File ui/views/controls/scrollbar/bitmap_scroll_bar.cc (right):

http://codereview.chromium.org/9332006/diff/2001/ui/views/controls/scrollbar/...
ui/views/controls/scrollbar/bitmap_scroll_bar.cc:297:
gfx::Rect(track_bounds.x(), track_bounds.y(),
Nit: Just pass GetTrackBounds() directly and drop the temp and the rect
construction.

http://codereview.chromium.org/9332006/diff/2001/ui/views/window/frame_backgr...
File ui/views/window/frame_background.cc (right):

http://codereview.chromium.org/9332006/diff/2001/ui/views/window/frame_backgr...
ui/views/window/frame_background.cc:120: - bottom_right_corner_->width(),
Nit: While here, '-' goes on end of prior line

[tfarina, 2012-02-06 23:12:16]

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:806: 0, width() -
frame_edges.top_right->width(), frame_edges.top->height()));
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: While here, this has a bug; third arg should be "width() -
> frame_edges.top_left->width() - frame_edges.top_right->width()"

Done.

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/abou...
File chrome/browser/ui/views/about_chrome_view.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/abou...
chrome/browser/ui/views/about_chrome_view.cc:437:
ui::ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont);
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: Why add this qualifier if it works without it?
> 
> You also did this in some previous patch and I forgot to ask why or suggest
not
> doing it.

It is because ResourceBundle was moved into ui namespace, and there is a TODO
about this in resource_bundle.h.

ui/base/resource/resource_bundle.h +320

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/app_panel_browser_frame_view.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_panel_browser_frame_view.cc:365: width() -
top_right_corner->width(), top_edge->height()));
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: While here, this has a bug; third arg should be "width() -
> top_left_corner_width() - top_right_corner->width()"

Done.

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab.cc:489:
canvas->TileImage(*tab_inactive_image->image_c,
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: Shorter:
> 
>   gfx::Rect tile_rect(GetLocalBounds());
>   tile_rect.Inset(tab_inactive_image->l_width, 0, tab_inactive_image->r_width,
>                   0);
>   canvas->TileImage(*tab_inactive_image->image_c, tile_rect);
>   canvas->DrawBitmapInt(*tab_inactive_image->image_r, tile_rect.right(), 0);

Done.

http://codereview.chromium.org/9332006/diff/2001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab.cc:534: canvas->TileImage(*tab_image->image_c,
gfx::Rect(tab_image->l_width, 0,
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: Possibly clearer:
> 
>   gfx::Rect tile_rect(GetLocalBounds());
>   tile_rect.Inset(tab_image->l_width, 0, tab_image->r_width, 0);
>   canvas->TileImage(*tab_image->image_c, tile_rect);
>   canvas->DrawBitmapInt(*tab_image->image_r, tile_rect.right(), 0);

Done.

http://codereview.chromium.org/9332006/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/9332006/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_action.cc:149:
SkScalarFloor(SkIntToScalar(bounds.x()) +
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: While here, convert SkIntToScalar(SkScalarFloor(...)) to
> SkScalarFloorToScalar(...)

Could you take another look, to see if I get this right?

http://codereview.chromium.org/9332006/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/9332006/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_win.cc:1136:
canvas.TileImage(background_,
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: Simpler:
> 
>   dc_rect.set_origin(gfx::Point());
>   canvas.TileImage(background_, dc_rect);

Done.

http://codereview.chromium.org/9332006/diff/2001/ui/views/controls/scrollbar/...
File ui/views/controls/scrollbar/bitmap_scroll_bar.cc (right):

http://codereview.chromium.org/9332006/diff/2001/ui/views/controls/scrollbar/...
ui/views/controls/scrollbar/bitmap_scroll_bar.cc:297:
gfx::Rect(track_bounds.x(), track_bounds.y(),
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: Just pass GetTrackBounds() directly and drop the temp and the rect
> construction.

Done.

http://codereview.chromium.org/9332006/diff/2001/ui/views/window/frame_backgr...
File ui/views/window/frame_background.cc (right):

http://codereview.chromium.org/9332006/diff/2001/ui/views/window/frame_backgr...
ui/views/window/frame_background.cc:120: - bottom_right_corner_->width(),
On 2012/02/06 22:32:39, Peter Kasting wrote:
> Nit: While here, '-' goes on end of prior line

Done.

[Peter Kasting, 2012-02-06 23:21:50]

http://codereview.chromium.org/9332006/diff/7003/chrome/common/extensions/ext...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/9332006/diff/7003/chrome/common/extensions/ext...
chrome/common/extensions/extension_action.cc:148: rect.fLeft =
SkScalarFloorToScalar(SkIntToScalar(bounds.x()) +
Looks right to me

[tfarina, 2012-02-08 19:45:15]

https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
File ui/views/window/frame_background.cc (right):

https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
ui/views/window/frame_background.cc:90: view->width() -
top_left_corner_->width() - top_right_corner_->width(),
Peter, this calculation is resulting in -2.

10 - 6 - 6

Thus, WidgetTest.ChangeActivation is failing.

[Peter Kasting, 2012-02-08 20:54:57]

https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
File ui/views/window/frame_background.cc (right):

https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
ui/views/window/frame_background.cc:90: view->width() -
top_left_corner_->width() - top_right_corner_->width(),
On 2012/02/08 19:45:15, tfarina wrote:
> Peter, this calculation is resulting in -2.
> 
> 10 - 6 - 6
> 
> Thus, WidgetTest.ChangeActivation is failing.

Why is the view allowed to reach as small as 10 px in width?  It seems like
GetMinimumSize() somewhere should be preventing that.

If not, and we need to handle this here, we should be deciding how to scale or
clip the images in question.  For example:

  int top_left_width = top_left_corner_->width();
  int top_right_width = top_right_corner_->width();
  int center_width = view->width() - top_left_width - top_right_width;
  if (center_width > 0) {
    canvas->TileImage(*top_edge_, gfx::Rect(top_left_width, 0, center_width,
                                            top_edge_->height()));
  } else if (center_width < 0) {
    // Not enough space for the two corners.  Remove an equal number of pixels
    // from the inside edges of each (unless this would shrink a corner beyond 0
    // width).
    int top_right_clip = std::min(-center_width / 2, top_right_width);
    top_left_width += center_width + top_right_clip;
    DCHECK_GE(top_left_width, 0);
    top_right_width -= top_right_clip;
  }
  if (top_left_width) {
    canvas->DrawBitmapInt(*top_left_corner_, 0, 0, top_left_width,
        top_left_height, 0, 0, top_left_width, top_left_height, false);
  }
  if (top_right_width) {
    canvas->DrawBitmapInt(*top_right_corner_,
        top_right_corner_->width() - top_right_width, 0, top_right_width,
        top_right_height, view->width() - top_right_width, 0, top_right_width,
        top_right_height, false);
  }

However, I don't think we should need to do this if we're setting the minimum
size appropriately.

[tfarina, 2012-02-08 21:09:47]

On 2012/02/08 20:54:57, Peter Kasting wrote:
>
https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
> File ui/views/window/frame_background.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
> ui/views/window/frame_background.cc:90: view->width() -
> top_left_corner_->width() - top_right_corner_->width(),
> On 2012/02/08 19:45:15, tfarina wrote:
> > Peter, this calculation is resulting in -2.
> > 
> > 10 - 6 - 6
> > 
> > Thus, WidgetTest.ChangeActivation is failing.
> 
> Why is the view allowed to reach as small as 10 px in width?  It seems like
> GetMinimumSize() somewhere should be preventing that.
> 
> If not, and we need to handle this here, we should be deciding how to scale or
> clip the images in question.  For example:
> 
>   int top_left_width = top_left_corner_->width();
>   int top_right_width = top_right_corner_->width();
>   int center_width = view->width() - top_left_width - top_right_width;
>   if (center_width > 0) {
>     canvas->TileImage(*top_edge_, gfx::Rect(top_left_width, 0, center_width,
>                                             top_edge_->height()));
>   } else if (center_width < 0) {
>     // Not enough space for the two corners.  Remove an equal number of pixels
>     // from the inside edges of each (unless this would shrink a corner beyond
0
>     // width).
>     int top_right_clip = std::min(-center_width / 2, top_right_width);
>     top_left_width += center_width + top_right_clip;
>     DCHECK_GE(top_left_width, 0);
>     top_right_width -= top_right_clip;
>   }
>   if (top_left_width) {
>     canvas->DrawBitmapInt(*top_left_corner_, 0, 0, top_left_width,
>         top_left_height, 0, 0, top_left_width, top_left_height, false);
>   }
>   if (top_right_width) {
>     canvas->DrawBitmapInt(*top_right_corner_,
>         top_right_corner_->width() - top_right_width, 0, top_right_width,
>         top_right_height, view->width() - top_right_width, 0, top_right_width,
>         top_right_height, false);
>   }
> 
> However, I don't think we should need to do this if we're setting the minimum
> size appropriately.

|view| is CustomFrameView, and CustomFrameView just overrides GetPreferredSize,
should we override GetMinimumSize and to something there? |view| can also be
ConstrainedWindowViews and OpaqueBrowserFrameView.

[Peter Kasting, 2012-02-08 21:18:33]

On 2012/02/08 21:09:47, tfarina wrote:
> |view| is CustomFrameView, and CustomFrameView just overrides
GetPreferredSize,
> should we override GetMinimumSize and to something there? |view| can also be
> ConstrainedWindowViews and OpaqueBrowserFrameView.

Normally GetMinimumSize() returns GetPreferredSize() when not overridden.  This
leaves us with several questions:

Can windows with a CustomFrameView be user-resized?  If so, then we need to
override GetMinimumSize() for it and do something smart, the way
OpaqueBrowserFrameView() already does.  If not, then we should just see who's
creating such a small frame view, fix all such places, and then probably add
some DCHECKs to the class's construction flow so that we ensure the created size
is large enough to display the various images.

The same questions apply for ConstrainedWindowViews.

[tfarina, 2012-02-08 22:17:41]

On Wed, Feb 8, 2012 at 7:18 PM,  <pkasting@chromium.org> wrote:
> On 2012/02/08 21:09:47, tfarina wrote:
>>
>> |view| is CustomFrameView, and CustomFrameView just overrides
>
> GetPreferredSize,
>>
>> should we override GetMinimumSize and to something there? |view| can also
>> be
>> ConstrainedWindowViews and OpaqueBrowserFrameView.
>
>
> Normally GetMinimumSize() returns GetPreferredSize() when not overridden.
>  This
> leaves us with several questions:
>
> Can windows with a CustomFrameView be user-resized?
Yes, views_examples window is an example.

>  If so, then we need to
> override GetMinimumSize() for it and do something smart, the way
> OpaqueBrowserFrameView() already does.  If not, then we should just see
> who's
> creating such a small frame view, fix all such places, and then probably add
> some DCHECKs to the class's construction flow so that we ensure the created
> size
> is large enough to display the various images.
>
> The same questions apply for ConstrainedWindowViews.
>

I didn't trigger ConstrainedWindowViews, because on gtk we still
having some problems like
[20938:20938:165196827772:FATAL:native_theme_gtk.cc(75)] Check failed:
false. Invalid color_id: 8 when running chrome (with use_aura=1). But
I suppose it can't be user-resized.

-- 
Thiago

[tfarina, 2012-02-09 23:42:37]

On Wed, Feb 8, 2012 at 8:17 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Wed, Feb 8, 2012 at 7:18 PM,  <pkasting@chromium.org> wrote:
>> On 2012/02/08 21:09:47, tfarina wrote:
>>>
>>> |view| is CustomFrameView, and CustomFrameView just overrides
>>
>> GetPreferredSize,
>>>
>>> should we override GetMinimumSize and to something there? |view| can also
>>> be
>>> ConstrainedWindowViews and OpaqueBrowserFrameView.
>>
>>
>> Normally GetMinimumSize() returns GetPreferredSize() when not overridden.
>>  This
>> leaves us with several questions:
>>
>> Can windows with a CustomFrameView be user-resized?
> Yes, views_examples window is an example.
>
>>  If so, then we need to
>> override GetMinimumSize() for it and do something smart, the way
>> OpaqueBrowserFrameView() already does.  If not, then we should just see
>> who's
>> creating such a small frame view, fix all such places, and then probably add
>> some DCHECKs to the class's construction flow so that we ensure the created
>> size
>> is large enough to display the various images.
>>
>> The same questions apply for ConstrainedWindowViews.
>>
>
> I didn't trigger ConstrainedWindowViews, because on gtk we still
> having some problems like
> [20938:20938:165196827772:FATAL:native_theme_gtk.cc(75)] Check failed:
> false. Invalid color_id: 8 when running chrome (with use_aura=1). But
> I suppose it can't be user-resized.
>
Peter, any comments here?

-- 
Thiago

[Peter Kasting, 2012-02-09 23:45:42]

On 2012/02/09 23:42:37, tfarina wrote:
> Peter, any comments here?

I'm not sure what you want feedback on.  I'm also not sure what state the tree
is in.  This patch set is Closed, but I assume was rolled out due to the
problems you describe?  If so maybe we should be discussing on a new changeset?

My previous email said everything I had to say regarding how I thought we should
proceed.  In particular, we should implement CustomFrameView::GetMinimumSize()
and ensure it's enforced sufficiently to eliminate the problems you're seeing.

[tfarina, 2012-02-09 23:56:41]

On 2012/02/09 23:45:42, Peter Kasting wrote:
> On 2012/02/09 23:42:37, tfarina wrote:
> > Peter, any comments here?
> 
> I'm not sure what you want feedback on.  I'm also not sure what state the tree
> is in.  This patch set is Closed, but I assume was rolled out due to the
> problems you describe?
Yeah, hbono reverted it because WidgetTest.ChangeActivation was crashing
views_unittests.

> If so maybe we should be discussing on a new changeset?
> 
Yup, I'll open a new CL.

> My previous email said everything I had to say regarding how I thought we
should
> proceed.  In particular, we should implement CustomFrameView::GetMinimumSize()
> and ensure it's enforced sufficiently to eliminate the problems you're seeing.

Right. I'll try to implement it.

[tfarina, 2012-04-04 19:28:46]

On 2012/02/08 19:45:15, tfarina wrote:
>
https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
> File ui/views/window/frame_background.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9332006/diff/14002/ui/views/window/fra...
> ui/views/window/frame_background.cc:90: view->width() -
> top_left_corner_->width() - top_right_corner_->width(),
> Peter, this calculation is resulting in -2.
> 
> 10 - 6 - 6
> 
> Thus, WidgetTest.ChangeActivation is failing.

I'm going to file a bug for this. Would Scott provide some input here? I'll copy
him in the bug.