drawFocusRing() function in CRC2D draws invalid focus ring.

drawFocusRing() function in CRC2D draws invalid focus ring.

The function draws a focus ring around the current default path or the given path if the given element is focused.
Currently, the focus ring is drawn incorrectly because it is larger than the given path region.
To solve this problem, we should add focus ring width to dirty rect.

BUG=352976
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169913

[zino, 2014-03-17 09:21:19]

I think my layout test is not good.
It seems to occurs repainting during image diff.
So, current result(before patch) is also passed.

Please let me know if you have a good way :)

[Justin Novosad, 2014-03-17 13:57:10]

If you can't write a test that reproduces the bug when executed as a layout
test, but you can reproduce the problem when running content_shell manually,
then you could submit the test as a manual test.

https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2288: const int
focusRingWidth = 1;
How come this value changed?  Seems unrelated to this bug.

[zino, 2014-03-18 10:14:01]

On 2014/03/17 13:57:10, junov wrote:
> If you can't write a test that reproduces the bug when executed as a layout
> test, but you can reproduce the problem when running content_shell manually,
> then you could submit the test as a manual test.
> 
>
https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:2288: const int
> focusRingWidth = 1;
> How come this value changed?  Seems unrelated to this bug.

It is my mistake. I wasn't considering Mac OSX. (other platforms are always
fixed as width = 1)
Anyway, I've modified it.

How can I create the manual test?
If the manual test you said is like "drag and drop auto scroll"
(fast/events/drag-and-drop-autoscroll.html),
I'm not sure but I think it is difficult.
Because I think we can't check result (true or false).

[Justin Novosad, 2014-03-18 12:00:43]

On 2014/03/18 10:14:01, zino wrote:
> On 2014/03/17 13:57:10, junov wrote:
> > If you can't write a test that reproduces the bug when executed as a layout
> > test, but you can reproduce the problem when running content_shell manually,
> > then you could submit the test as a manual test.
> > 
> >
>
https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
> > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > 
> >
>
https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2288: const int
> > focusRingWidth = 1;
> > How come this value changed?  Seems unrelated to this bug.
> 
> It is my mistake. I wasn't considering Mac OSX. (other platforms are always
> fixed as width = 1)
> Anyway, I've modified it.
> 
> How can I create the manual test?
> If the manual test you said is like "drag and drop auto scroll"
> (fast/events/drag-and-drop-autoscroll.html),
> I'm not sure but I think it is difficult.
> Because I think we can't check result (true or false).

The manual tests are simply tests that need to be validated by humans. It is
just a regular html test, usually with some text to describe the expected
output.
Example:
https://src.chromium.org/viewvc/blink?view=revision&revision=136262

We avoid these as much as possible because they are not automated, but some
times we have no choice for making regression tests for bugs that don't
reproduce in layout tests (and other reasons).

[zino, 2014-03-19 04:12:29]

On 2014/03/18 12:00:43, junov wrote:
> On 2014/03/18 10:14:01, zino wrote:
> > On 2014/03/17 13:57:10, junov wrote:
> > > If you can't write a test that reproduces the bug when executed as a
layout
> > > test, but you can reproduce the problem when running content_shell
manually,
> > > then you could submit the test as a manual test.
> > > 
> > >
> >
>
https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
> > > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/200283003/diff/40001/Source/core/html/canvas/...
> > > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2288: const int
> > > focusRingWidth = 1;
> > > How come this value changed?  Seems unrelated to this bug.
> > 
> > It is my mistake. I wasn't considering Mac OSX. (other platforms are always
> > fixed as width = 1)
> > Anyway, I've modified it.
> > 
> > How can I create the manual test?
> > If the manual test you said is like "drag and drop auto scroll"
> > (fast/events/drag-and-drop-autoscroll.html),
> > I'm not sure but I think it is difficult.
> > Because I think we can't check result (true or false).
> 
> The manual tests are simply tests that need to be validated by humans. It is
> just a regular html test, usually with some text to describe the expected
> output.
> Example:
> https://src.chromium.org/viewvc/blink?view=revision&revision=136262
> 
> We avoid these as much as possible because they are not automated, but some
> times we have no choice for making regression tests for bugs that don't
> reproduce in layout tests (and other reasons).

Good example!
Thanks to you, I could upload a manual test instead of a layout test.

[zino, 2014-03-20 16:04:57]

I addressed your comments.
Please review again.

[Rik, 2014-03-20 16:25:00]

On 2014/03/20 16:04:57, zino wrote:
> I addressed your comments.
> Please review again.

Firefox, IE and Dominic Mazzoni agreed to rename this API to drawFocusIfNeeded.
Could you make that change as well?

[Justin Novosad, 2014-03-20 19:23:54]

On 2014/03/20 16:25:00, Rik wrote:
> On 2014/03/20 16:04:57, zino wrote:
> > I addressed your comments.
> > Please review again.
> 
> Firefox, IE and Dominic Mazzoni agreed to rename this API to
drawFocusIfNeeded.
> Could you make that change as well?

We'll definitely make that change, but this bug and the new name are two
separate issues.
Let's keep this CL just about the bug. Tying the two together would mean that if
one is reverted or merged the other would follow.  Best to avoid that sort of
thing.

[Justin Novosad, 2014-03-20 19:27:38]

https://codereview.chromium.org/200283003/diff/80001/ManualTests/draw-system-...
File ManualTests/draw-system-focus-ring-dirty-rect.html (right):

https://codereview.chromium.org/200283003/diff/80001/ManualTests/draw-system-...
ManualTests/draw-system-focus-ring-dirty-rect.html:41: }, 500);
Does the timeout need to be this long?  If you want to make sure the draw
happens after the current frame is painted, then you should use
requestAnimationFrame instead of setTimeout.

[zino, 2014-03-22 08:51:08]

On 2014/03/20 19:27:38, junov wrote:
>
https://codereview.chromium.org/200283003/diff/80001/ManualTests/draw-system-...
> File ManualTests/draw-system-focus-ring-dirty-rect.html (right):
> 
>
https://codereview.chromium.org/200283003/diff/80001/ManualTests/draw-system-...
> ManualTests/draw-system-focus-ring-dirty-rect.html:41: }, 500);
> Does the timeout need to be this long?  If you want to make sure the draw
> happens after the current frame is painted, then you should use
> requestAnimationFrame instead of setTimeout.

You're right. I modified the manual test and renamed.
Thank you.

[Justin Novosad, 2014-03-24 14:49:56]

lgtm

[zino, 2014-03-25 03:10:07]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-03-25 03:10:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/200283003/100001

[commit-bot: I haz the power, 2014-03-25 03:13:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 03:13:00]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[zino, 2014-03-25 03:16:42]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-03-25 03:16:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/200283003/120001

[commit-bot: I haz the power, 2014-03-25 03:48:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 03:48:12]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[zino, 2014-03-25 04:28:56]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-03-25 04:29:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/200283003/120001

[commit-bot: I haz the power, 2014-03-25 04:30:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 04:30:26]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[zino, 2014-03-25 05:30:47]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-03-25 05:31:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/200283003/120001

[commit-bot: I haz the power, 2014-03-25 06:02:08]

Change committed as 169913