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

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

BUG=100898
R=pkasting@chromium.org

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

[Peter Kasting, 2011-11-05 01:10:10]

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/autocom...
File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/autocom...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:179:
bg_painter_->Paint(width() - kOptInBackgroundHInset * 2,
Nit: Shorter:

    gfx::Rect paint_rect(GetLocalBounds());
    paint_rect.Inset(kOptInBackgroundHInset, kOptInBackgroundVInset);
    canvas->Save();
    canvas->Translate(paint_rect.origin());
    bg_painter_->Paint(paint_rect.width(), paint_rect.height(), canvas);
    canvas->Restore();
    canvas->DrawRect(ResourceBundle::toolbar_separator_color, paint_rect);

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
chrome/browser/ui/views/tabs/tab.cc:586:
hover_canvas.DrawRect(gfx::Rect(hover_point_.x() - radius,
Nit: Seems cleaner:

    gfx::Rect paint_rect(hover_point_, gfx::Size());
    paint_rect.Inset(-radius, -radius);
    hover_canvas.DrawRect(paint_rect, paint);

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
chrome/browser/ui/views/tabs/tab_strip.cc:337: height() - 2),  // Visible region
that overlaps the toolbar.
Nit: I'd hoist this comment above the line as "// The tabstrip area overlaps the
toolbar area by 2 px.", then all the code can go on one line.

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas.h#newcode125
ui/gfx/canvas.h:125: virtual void DrawRect(const SkColor& color, const
gfx::Rect& rect) = 0;
Nit: Kinda drives me crazy that some of these functions put colors or brushes or
paints first, some last, etc.  Maybe we can standardize all the rect and line
drawing/filling functions to put coordinates first, other stuff after?

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia.cc#newcode167
ui/gfx/canvas_skia.cc:167: SkIRect rc = { rect.x(), rect.y(), rect.right(),
rect.bottom() };
Nit: Maybe we should just add a RectToSkIRect() function to skia_util.h?

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia_win.cc
File ui/gfx/canvas_skia_win.cc (right):

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia_win.cc#newco...
ui/gfx/canvas_skia_win.cc:418: CanvasSkia text_canvas(w + 2, h + 2, true);
Nit: We could probably create a rect with this size and then use its width and
height below... especially if skia::MakeOpaque() also gets converted to take a
rect.

[tfarina, 2011-11-05 02:23:24]

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/autocom...
File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/autocom...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:179:
bg_painter_->Paint(width() - kOptInBackgroundHInset * 2,
On 2011/11/05 01:10:10, Peter Kasting wrote:
> Nit: Shorter:
> 
>     gfx::Rect paint_rect(GetLocalBounds());
>     paint_rect.Inset(kOptInBackgroundHInset, kOptInBackgroundVInset);
>     canvas->Save();
>     canvas->Translate(paint_rect.origin());
>     bg_painter_->Paint(paint_rect.width(), paint_rect.height(), canvas);
>     canvas->Restore();
>     canvas->DrawRect(ResourceBundle::toolbar_separator_color, paint_rect);

Done.

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
chrome/browser/ui/views/tabs/tab.cc:586:
hover_canvas.DrawRect(gfx::Rect(hover_point_.x() - radius,
On 2011/11/05 01:10:10, Peter Kasting wrote:
> Nit: Seems cleaner:
> 
>     gfx::Rect paint_rect(hover_point_, gfx::Size());
>     paint_rect.Inset(-radius, -radius);
>     hover_canvas.DrawRect(paint_rect, paint);

Done.

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/8476019/diff/1/chrome/browser/ui/views/tabs/ta...
chrome/browser/ui/views/tabs/tab_strip.cc:337: height() - 2),  // Visible region
that overlaps the toolbar.
On 2011/11/05 01:10:10, Peter Kasting wrote:
> Nit: I'd hoist this comment above the line as "// The tabstrip area overlaps
the
> toolbar area by 2 px.", then all the code can go on one line.

Done.

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas.h#newcode125
ui/gfx/canvas.h:125: virtual void DrawRect(const SkColor& color, const
gfx::Rect& rect) = 0;
On 2011/11/05 01:10:10, Peter Kasting wrote:
> Nit: Kinda drives me crazy that some of these functions put colors or brushes
or
> paints first, some last, etc.  Maybe we can standardize all the rect and line
> drawing/filling functions to put coordinates first, other stuff after?

Me too. Addressed for DrawRect. Can I fix the other functions in a follow up CL?

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia.cc#newcode167
ui/gfx/canvas_skia.cc:167: SkIRect rc = { rect.x(), rect.y(), rect.right(),
rect.bottom() };
On 2011/11/05 01:10:10, Peter Kasting wrote:
> Nit: Maybe we should just add a RectToSkIRect() function to skia_util.h?

Done.

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia_win.cc
File ui/gfx/canvas_skia_win.cc (right):

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas_skia_win.cc#newco...
ui/gfx/canvas_skia_win.cc:418: CanvasSkia text_canvas(w + 2, h + 2, true);
On 2011/11/05 01:10:10, Peter Kasting wrote:
> Nit: We could probably create a rect with this size and then use its width and
> height below... especially if skia::MakeOpaque() also gets converted to take a
> rect.

Done.

[tfarina, 2011-11-10 12:30:42]

ping?

[Peter Kasting, 2011-11-14 21:17:50]

I'm sorry I've not gotten to this.  I was unexpectedly OOO several days last
week and am WebKit sheriff this week.  I will try to get to this as soon as I
can.

[tfarina, 2011-11-18 22:38:28]

On 2011/11/14 21:17:50, Peter Kasting wrote:
> I'm sorry I've not gotten to this.  I was unexpectedly OOO several days last
> week and am WebKit sheriff this week.  I will try to get to this as soon as I
> can.

Peter, friendly ping. In case you forgot about this.

[tfarina, 2011-11-18 22:39:15]

Sending again as gmail sent this to spam, and you may not have got this. :/

[Peter Kasting, 2011-11-21 19:26:02]

LGTM

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/8476019/diff/1/ui/gfx/canvas.h#newcode125
ui/gfx/canvas.h:125: virtual void DrawRect(const SkColor& color, const
gfx::Rect& rect) = 0;
On 2011/11/05 02:23:24, tfarina wrote:
> On 2011/11/05 01:10:10, Peter Kasting wrote:
> > Nit: Kinda drives me crazy that some of these functions put colors or
brushes
> or
> > paints first, some last, etc.  Maybe we can standardize all the rect and
line
> > drawing/filling functions to put coordinates first, other stuff after?
> 
> Me too. Addressed for DrawRect. Can I fix the other functions in a follow up
CL?

Sure.

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas.h#newcode121
ui/gfx/canvas.h:121: // Draws the given rectangle with the given paint's
parameters.
Nit: I'd leave both the declaration and definition of this one where they were
before, as specifying an SkPaint seems more specialized than specifying a color.

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas_skia.h
File ui/gfx/canvas_skia.h (right):

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas_skia.h#newcode112
ui/gfx/canvas_skia.h:112: virtual void DrawRect(const gfx::Rect& rect, const
SkPaint& paint) OVERRIDE;
Nit: Same comment about order

[tfarina, 2011-11-22 18:39:01]

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas.h#newcode121
ui/gfx/canvas.h:121: // Draws the given rectangle with the given paint's
parameters.
On 2011/11/21 19:26:03, Peter Kasting wrote:
> Nit: I'd leave both the declaration and definition of this one where they were
> before, as specifying an SkPaint seems more specialized than specifying a
color.

Done.

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas_skia.h
File ui/gfx/canvas_skia.h (right):

http://codereview.chromium.org/8476019/diff/16/ui/gfx/canvas_skia.h#newcode112
ui/gfx/canvas_skia.h:112: virtual void DrawRect(const gfx::Rect& rect, const
SkPaint& paint) OVERRIDE;
On 2011/11/21 19:26:03, Peter Kasting wrote:
> Nit: Same comment about order

Done.