ui/gfx: Convert Canvas::FillRectInt() to use gfx::Rect.

ui/gfx: Convert Canvas::FillRectInt() to use gfx::Rect.

BUG=100898
R=pkasting@chromium.org

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

[tfarina, 2011-10-27 15:13:20]

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
File views/window/custom_frame_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
views/window/custom_frame_view.cc:349: gfx::Rect(0, 0, width(),
frame_image->height()));
Peter, using gfx::Rect here is causing views_unittests to assert on a negative
height (-99) because of the check in Size::set_height(). So the unittests are
not passing.

[Peter Kasting, 2011-10-27 19:17:29]

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/chromeos/pane...
File chrome/browser/chromeos/panels/panel_scroller_header.cc (right):

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/chromeos/pane...
chrome/browser/chromeos/panels/panel_scroller_header.cc:43:
canvas->FillRect(0xFFFFFFFF, gfx::Rect(GetLocalBounds()));
Nit: GetLocalBounds() already returns a rect, you don't need to wrap it with
another one.

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/thumbnail_generator_unittest.cc (right):

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/thumbnail_generator_unittest.cc:223:
canvas.FillRect(kBlack, gfx::Rect(0, 0, kSize.width(), kSize.height()));
Nit: Shorter: gfx::Rect(gfx::Point(), kSize); (2 places)

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/dragged_tab_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/dragged_tab_view.cc:147: int image_x =
kDragFrameBorderSize;
Nit: Simpler:

  gfx::Rect image_rect(kDragFrameBorderSize, tab_height,
      ps.width() - kTwiceDragFrameBorderSize, contents_size_.height());
  scale_canvas.FillRect(SK_ColorBLACK, image_rect);
  photobooth_->PaintScreenshotIntoCanvas(&scale_canvas, image_rect);

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/canvas_skia_linux.cc
File ui/gfx/canvas_skia_linux.cc (right):

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/canvas_skia_linux.cc#...
ui/gfx/canvas_skia_linux.cc:122: static_cast<SkColor>(0),
Nit: OK to leave this on previous line

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/compositor/layer_unit...
File ui/gfx/compositor/layer_unittest.cc (right):

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/compositor/layer_unit...
ui/gfx/compositor/layer_unittest.cc:130:
canvas->FillRect(colors_.at(color_index_),
Nit: While here, change at() to []

http://codereview.chromium.org/8405002/diff/7001/views/desktop/desktop_backgr...
File views/desktop/desktop_background.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/desktop/desktop_backgr...
views/desktop/desktop_background.cc:22: canvas->FillRect(SK_ColorCYAN,
gfx::Rect(0, 0, view->width(), view->width()));
I think the double-use of width() here is a bug (Ben agrees).  I think you
should pass view->GetLocalBounds() here.

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
File views/window/custom_frame_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
views/window/custom_frame_view.cc:349: gfx::Rect(0, 0, width(),
frame_image->height()));
On 2011/10/27 15:13:20, tfarina wrote:
> Peter, using gfx::Rect here is causing views_unittests to assert on a negative
> height (-99) because of the check in Size::set_height(). So the unittests are
> not passing.

How is it that we're somehow getting a frame_image with height -99 in unit
tests?  We should fix that.

http://codereview.chromium.org/8405002/diff/7001/views/window/dialog_client_v...
File views/window/dialog_client_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/dialog_client_v...
views/window/dialog_client_view.cc:59: canvas->FillRect(sk_color,
gfx::Rect(gfx::Point(), view->size()));
Nit: view->GetLocalBounds()

[tfarina, 2011-10-27 20:46:03]

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/chromeos/pane...
File chrome/browser/chromeos/panels/panel_scroller_header.cc (right):

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/chromeos/pane...
chrome/browser/chromeos/panels/panel_scroller_header.cc:43:
canvas->FillRect(0xFFFFFFFF, gfx::Rect(GetLocalBounds()));
On 2011/10/27 19:17:29, Peter Kasting wrote:
> Nit: GetLocalBounds() already returns a rect, you don't need to wrap it with
> another one.

Done.

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/thumbnail_generator_unittest.cc (right):

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/thumbnail_generator_unittest.cc:223:
canvas.FillRect(kBlack, gfx::Rect(0, 0, kSize.width(), kSize.height()));
On 2011/10/27 19:17:29, Peter Kasting wrote:
> Nit: Shorter: gfx::Rect(gfx::Point(), kSize); (2 places)

Done.

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/dragged_tab_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/dragged_tab_view.cc:147: int image_x =
kDragFrameBorderSize;
On 2011/10/27 19:17:29, Peter Kasting wrote:
> Nit: Simpler:
> 
>   gfx::Rect image_rect(kDragFrameBorderSize, tab_height,
>       ps.width() - kTwiceDragFrameBorderSize, contents_size_.height());
>   scale_canvas.FillRect(SK_ColorBLACK, image_rect);
>   photobooth_->PaintScreenshotIntoCanvas(&scale_canvas, image_rect);

Done.

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/canvas_skia_linux.cc
File ui/gfx/canvas_skia_linux.cc (right):

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/canvas_skia_linux.cc#...
ui/gfx/canvas_skia_linux.cc:122: static_cast<SkColor>(0),
On 2011/10/27 19:17:29, Peter Kasting wrote:
> Nit: OK to leave this on previous line

Done.

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/compositor/layer_unit...
File ui/gfx/compositor/layer_unittest.cc (right):

http://codereview.chromium.org/8405002/diff/7001/ui/gfx/compositor/layer_unit...
ui/gfx/compositor/layer_unittest.cc:130:
canvas->FillRect(colors_.at(color_index_),
On 2011/10/27 19:17:29, Peter Kasting wrote:
> Nit: While here, change at() to []

Done.

http://codereview.chromium.org/8405002/diff/7001/views/desktop/desktop_backgr...
File views/desktop/desktop_background.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/desktop/desktop_backgr...
views/desktop/desktop_background.cc:22: canvas->FillRect(SK_ColorCYAN,
gfx::Rect(0, 0, view->width(), view->width()));
On 2011/10/27 19:17:29, Peter Kasting wrote:
> I think the double-use of width() here is a bug (Ben agrees).  I think you
> should pass view->GetLocalBounds() here.

Done.

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
File views/window/custom_frame_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
views/window/custom_frame_view.cc:349: gfx::Rect(0, 0, width(),
frame_image->height()));
On 2011/10/27 19:17:29, Peter Kasting wrote:
> On 2011/10/27 15:13:20, tfarina wrote:
> > Peter, using gfx::Rect here is causing views_unittests to assert on a
negative
> > height (-99) because of the check in Size::set_height(). So the unittests
are
> > not passing.
> 
> How is it that we're somehow getting a frame_image with height -99 in unit
> tests?  We should fix that.

Yeah, I was going to send you the try bot urls with the failing tests but they
seem to have expired. Plus some interactive_ui_tests are also failing for the
same reason.

http://codereview.chromium.org/8405002/diff/7001/views/window/dialog_client_v...
File views/window/dialog_client_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/dialog_client_v...
views/window/dialog_client_view.cc:59: canvas->FillRect(sk_color,
gfx::Rect(gfx::Point(), view->size()));
On 2011/10/27 19:17:29, Peter Kasting wrote:
> Nit: view->GetLocalBounds()

Done.

[Peter Kasting, 2011-10-27 20:50:34]

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
File views/window/custom_frame_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
views/window/custom_frame_view.cc:349: gfx::Rect(0, 0, width(),
frame_image->height()));
On 2011/10/27 20:46:03, tfarina wrote:
> On 2011/10/27 19:17:29, Peter Kasting wrote:
> > On 2011/10/27 15:13:20, tfarina wrote:
> > > Peter, using gfx::Rect here is causing views_unittests to assert on a
> negative
> > > height (-99) because of the check in Size::set_height(). So the unittests
> are
> > > not passing.
> > 
> > How is it that we're somehow getting a frame_image with height -99 in unit
> > tests?  We should fix that.
> 
> Yeah, I was going to send you the try bot urls with the failing tests but they
> seem to have expired. Plus some interactive_ui_tests are also failing for the
> same reason.

Can you figure out how to fix this?

[tfarina, 2011-10-27 21:31:30]

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
File views/window/custom_frame_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
views/window/custom_frame_view.cc:354: height() - frame_image->height()));
On 2011/10/27 20:50:34, Peter Kasting wrote:
> Can you figure out how to fix this?

Hard to say. But probably I can help identify the cause.
 
gdb says that this->height() is 29 and frame_image->height() is 128. That ends
up in -99. So for some reason we have this tiny height().

[Peter Kasting, 2011-10-27 21:49:18]

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
File views/window/custom_frame_view.cc (right):

http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
views/window/custom_frame_view.cc:354: height() - frame_image->height()));
On 2011/10/27 21:31:30, tfarina wrote:
> On 2011/10/27 20:50:34, Peter Kasting wrote:
> > Can you figure out how to fix this?
> 
> Hard to say. But probably I can help identify the cause.
>  
> gdb says that this->height() is 29 and frame_image->height() is 128. That ends
> up in -99. So for some reason we have this tiny height().

Oh!  I misinterpreted what you were saying was negative.

This one is easy:

  int remaining_height = height() - frame_image->height();
  if (remaining_height > 0) {
    // Now fill down the sides.
    canvas->FillRect(frame_color, gfx::Rect(0, frame_image->height(),
        left_edge->width(), remaining_height));
    canvas->FillRect(frame_color, gfx::Rect(width() - right_edge->width(),
        frame_image->height(), right_edge->width(), remaining_height));
    // Now fill the bottom area.
    int center_width = width() - left_edge->width() - right_edge->width();
    if (center_width > 0) {
      canvas->FillRect(frame_color, gfx::Rect(left_edge->width(),
          height() - bottom_edge->height(), center_width,
          bottom_edge->height()));
    }
  }

We should probably also do similar checks in other files with this pattern.

[tfarina, 2011-10-27 22:34:28]

On 2011/10/27 21:49:18, Peter Kasting wrote:
>
http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
> File views/window/custom_frame_view.cc (right):
> 
>
http://codereview.chromium.org/8405002/diff/7001/views/window/custom_frame_vi...
> views/window/custom_frame_view.cc:354: height() - frame_image->height()));
> On 2011/10/27 21:31:30, tfarina wrote:
> > On 2011/10/27 20:50:34, Peter Kasting wrote:
> > > Can you figure out how to fix this?
> > 
> > Hard to say. But probably I can help identify the cause.
> >  
> > gdb says that this->height() is 29 and frame_image->height() is 128. That
ends
> > up in -99. So for some reason we have this tiny height().
> 
> Oh!  I misinterpreted what you were saying was negative.
> 
> This one is easy:
> 
>   int remaining_height = height() - frame_image->height();
>   if (remaining_height > 0) {
>     // Now fill down the sides.
>     canvas->FillRect(frame_color, gfx::Rect(0, frame_image->height(),
>         left_edge->width(), remaining_height));
>     canvas->FillRect(frame_color, gfx::Rect(width() - right_edge->width(),
>         frame_image->height(), right_edge->width(), remaining_height));
>     // Now fill the bottom area.
>     int center_width = width() - left_edge->width() - right_edge->width();
>     if (center_width > 0) {
>       canvas->FillRect(frame_color, gfx::Rect(left_edge->width(),
>           height() - bottom_edge->height(), center_width,
>           bottom_edge->height()));
>     }
>   }
> 
Done. Thanks!

> We should probably also do similar checks in other files with this pattern.
I'll look into the interactive_ui_tests soon.

[Peter Kasting, 2011-10-27 23:42:22]

BTW, general comment about line wrapping.  You can line wrap almost any way you
please, so nothing you're doing is wrong.  But in general a lot of our code
prefers to collapse arguments to save lines, a la:

Foo(Bar(a, b, c,
        d, e));

or

Function(arg1, arg2,
    very_long_arg3, arg4);

As opposed to putting one argument on each line or disallowing args on the
initial line if subsequent lines are indented 4.  So if you have code that can
be made noticeably more compact you're welcome to do so.

As for other problematic areas that may need checks added, some examples are the
xxx_browser_frame_view.cc files.

[tfarina, 2011-10-28 00:20:40]

On 2011/10/27 23:42:22, Peter Kasting wrote:
> BTW, general comment about line wrapping.  You can line wrap almost any way
you
> please, so nothing you're doing is wrong.  But in general a lot of our code
> prefers to collapse arguments to save lines, a la:
> 
> Foo(Bar(a, b, c,
>         d, e));
> 
> or
> 
> Function(arg1, arg2,
>     very_long_arg3, arg4);
> 
> As opposed to putting one argument on each line or disallowing args on the
> initial line if subsequent lines are indented 4.  So if you have code that can
> be made noticeably more compact you're welcome to do so.
> 
Done.

> As for other problematic areas that may need checks added, some examples are
the
> xxx_browser_frame_view.cc files.

Done.

interactive_ui_tests were fixed by your change!

[Peter Kasting, 2011-10-28 00:46:44]

LGTM