Issue 8359029: ui/gfx: Convert Canvas::DrawFocusRect() to use gfx::Rect.

Issue 8359029: ui/gfx: Convert Canvas::DrawFocusRect() to use gfx::Rect. (Closed)

Created:
9 years, 2 months ago by tfarina

Modified:
9 years, 2 months ago

Reviewers:
Peter Kasting

CC:
chromium-reviews, asanka, nkostylev+watch_chromium.org, tfarina, Randy Smith (Not in Mondays), stevenjb+watch_chromium.org, davemoore+watch_chromium.org, dhollowa

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

ui/gfx: Convert Canvas::DrawFocusRect() to use gfx::Rect. BUG=100898 R=pkasting@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107050

Patch Set 2 : address Peter's review #

Created: 9 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+29 lines, -30 lines)			Patch
M	chrome/browser/chromeos/drop_shadow_label.cc	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/chromeos/views/dropdown_button.cc	View	1	1 chunk	+5 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/views/download/download_shelf_view.cc	View	1	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/location_bar/location_bar_view.cc	View	1	1 chunk	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/dragged_tab_view.cc	View		1 chunk	+4 lines, -3 lines	0 comments	Download
M	ui/gfx/canvas.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ui/gfx/canvas_direct2d.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ui/gfx/canvas_direct2d.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ui/gfx/canvas_skia.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ui/gfx/canvas_skia.cc	View	1	3 chunks	+5 lines, -7 lines	0 comments	Download
M	views/controls/button/checkbox.cc	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	views/controls/button/text_button.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	views/controls/label.cc	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	views/view.cc	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Peter Kasting

LGTM http://codereview.chromium.org/8359029/diff/1/chrome/browser/chromeos/views/dropdown_button.cc File chrome/browser/chromeos/views/dropdown_button.cc (right): http://codereview.chromium.org/8359029/diff/1/chrome/browser/chromeos/views/dropdown_button.cc#newcode74 chrome/browser/chromeos/views/dropdown_button.cc:74: if (HasFocus() && (IsFocusable() || IsAccessibilityFocusableInRootView())) Nit: While ...

9 years, 2 months ago (2011-10-24 22:15:13 UTC) #2

tfarina

9 years, 2 months ago (2011-10-25 02:04:53 UTC) #3

http://codereview.chromium.org/8359029/diff/1/chrome/browser/chromeos/views/d...
File chrome/browser/chromeos/views/dropdown_button.cc (right):

http://codereview.chromium.org/8359029/diff/1/chrome/browser/chromeos/views/d...
chrome/browser/chromeos/views/dropdown_button.cc:74: if (HasFocus() &&
(IsFocusable() || IsAccessibilityFocusableInRootView()))
On 2011/10/24 22:15:13, Peter Kasting wrote:
> Nit: While here, add {}

Done.

http://codereview.chromium.org/8359029/diff/1/chrome/browser/ui/views/downloa...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

http://codereview.chromium.org/8359029/diff/1/chrome/browser/ui/views/downloa...
chrome/browser/ui/views/download/download_shelf_view.cc:166:
canvas->DrawFocusRect(gfx::Rect(r.x(), r.y(), r.width(), r.height() - 1));
On 2011/10/24 22:15:13, Peter Kasting wrote:
> Nit: Longer, but less verbose:
> 
> r.Inset(0, 0, 0, 1);
> canvas->DrawFocusRect(r);

Done.

http://codereview.chromium.org/8359029/diff/1/chrome/browser/ui/views/locatio...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/8359029/diff/1/chrome/browser/ui/views/locatio...
chrome/browser/ui/views/location_bar/location_bar_view.cc:758: gfx::Rect
rect(r.x() - 1, r.y() - 1, r.width() + 2, r.height() + 2);
On 2011/10/24 22:15:13, Peter Kasting wrote:
> Nit: Simpler:
> 
> #if defined(OS_WIN)
>     r.Inset(-1, -1);
> #else
>     r.Inset(-1, 0);
> #endif
>     canvas->DrawFocusRect(r);

Done.

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

http://codereview.chromium.org/8359029/diff/1/ui/gfx/canvas_skia.cc#newcode209
ui/gfx/canvas_skia.cc:209: // First the horizontal lines.
On 2011/10/24 22:15:13, Peter Kasting wrote:
> Nit: While you're here, remove this comment.

Done.

http://codereview.chromium.org/8359029/diff/1/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/8359029/diff/1/views/view.cc#newcode1082
views/view.cc:1082: canvas->DrawFocusRect(gfx::Rect(0, 0, width(), height()));
On 2011/10/24 22:15:13, Peter Kasting wrote:
> Nit: Just pass GetLocalBounds() instead.
> 
> (While you're in this file, feel free to fix that function to return
> "gfx::Rect(gfx::Point(), size());".)

Done.

Expand Messages | Collapse Messages