New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 611271 link

Starred by 1 user
Status: Archived
Owner:
█ mgiuca@chromium.org
Long OOO (go/where-is-mgiuca)
Closed: Aug 14
Cc:
█ mgiuca@chromium.org
█ skuhne@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Canvas::DrawSolidFocusRect seems wrong (random +1 on one edge)

Project Member Reported by mgiuca@chromium.org, May 12 2016 
I was just looking at the code for Canvas::DrawSolidFocusRect and found this weird logic for drawing a rectangle:

(x1, x2, y1, y2 are the left, right, top and bottom edges of the rectangle, respectively.)

DrawLine(Point(x1, y1), Point(x2, y1), paint);
DrawLine(Point(x1, y2), Point(x2, y2), paint);
DrawLine(Point(x1, y1), Point(x1, y2), paint);
DrawLine(Point(x2, y1), Point(x2, y2 + 1), paint);

On that last line, why is it "y2 + 1". This would imply the right edge is going to extend 1px downwards past the bottom. Maybe there is a good reason for this (a hack around some drawing anomaly), but I couldn't find an explanation in the code, the CL that added it, or the bug. It should be documented or fixed.

skuhne, you added this code in https://codereview.chromium.org/78803002. Can you please explain it?

Comment 1 by skuhne@chromium.org, May 12 2016

Cc: skuhne@chromium.org
Owner: mgiuca@chromium.org
Wow, this must be really a long time ago. But if I am not mistaken the draw command does not draw the last point. As such you would end up with the corner pixel not being drawn. As such the last line needs to be one pixel longer.

Comment 2 by mgiuca@chromium.org, May 12 2016 

Yeah it was 2.5 years ago and was to get around bugs in hidpi displays. So maybe you did need to overshoot the corner in order to draw a pixel there. But wouldn't the same be true of the top-right and bottom-left corners?

Anyway, I stepped into the code and I don't see anything to indicate that DrawLine *doesn't* draw the last point (that would be really broken if that's the case). So I think this +1 is incorrect but I'll dig into it.

Comment 3 by skuhne@chromium.org, May 12 2016 

Well - back then the last pixel was not drawn. If it is now, you might shorten all lines by one pixel now.

Comment 4 by robliao@chromium.org, Aug 14

Status: Archived (was: Assigned)
Archiving old bugs that haven't been modified in over two years. 

If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!
►

Sign in to add a comment