views::Label should paint borders in the overridden OnPaint() method.

views::Label should paint borders in the overridden OnPaint() method.

BUG=none
TEST=none


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

[sky, 2011-12-01 01:02:42]

http://codereview.chromium.org/8764005/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

http://codereview.chromium.org/8764005/diff/1/ui/views/controls/label.cc#newc...
ui/views/controls/label.cc:316: OnPaintFocusBorder(canvas);
Do these really need to be after painting the text? I'm wondering if we could
change 309 to View::OnPaint(canvas) to cover all this.
Also, you'll want to check subclasses to make sure they aren't effected.

[varunjain, 2011-12-01 05:12:03]

http://codereview.chromium.org/8764005/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

http://codereview.chromium.org/8764005/diff/1/ui/views/controls/label.cc#newc...
ui/views/controls/label.cc:316: OnPaintFocusBorder(canvas);
On 2011/12/01 01:02:42, sky wrote:
> Do these really need to be after painting the text? I'm wondering if we could
> change 309 to View::OnPaint(canvas) to cover all this.
> Also, you'll want to check subclasses to make sure they aren't effected.

You are correct that we can just do View::OnPaint() in the beginning. However,
as you prompted, I checked for some subclasses and some are doing fancy stuff
for focus. So I have removed the OnPaintFocusBorder call because views will
otherwise paint its focus border which may fiddle with the subclasses.
OnPaintBorder is ok because views doesnot do anything unless you explicitly set
a border on the view.

[sky, 2011-12-01 16:51:44]

Please add a comment as to why this doesn't invoke focus painting. LGTM
otherwise.

[varunjain, 2011-12-01 18:01:29]

On 2011/12/01 16:51:44, sky wrote:
> Please add a comment as to why this doesn't invoke focus painting. LGTM
> otherwise.

Done.

[commit-bot: I haz the power, 2011-12-01 18:01:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/8764005/8001

[commit-bot: I haz the power, 2011-12-01 19:59:09]

Change committed as 112502