Created attachment 230863 [details] Patch

Comment on attachment 230863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230863&action=review > Source/WebCore/ChangeLog:8 > + No new tests. Why are there no tests? Seems like it could be emulated, no? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1969 > +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element) It would definitely be great to have tests for that! > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1978 > + if (element->focused()) { Early return: if (!element->focused()) return;

Comment on attachment 230863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230863&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1969 >> +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element) > > It would definitely be great to have tests for that! How would I create a test for this? I could manually draw the ring but that might not match what the browser does. Maybe I should just check if something changed? >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1978 >> + if (element->focused()) { > > Early return: > if (!element->focused()) > return; will do.

(In reply to comment #3) > (From update of attachment 230863 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230863&action=review > > >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1969 > >> +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element) > > > > It would definitely be great to have tests for that! > > How would I create a test for this? > I could manually draw the ring but that might not match what the browser does. Maybe I should just check if something changed? That would be the easiest way probably. Why doesn't it work to draw the focus ring of the element in canvas in one test and without the canvas in another HTML file? > > >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1978 > >> + if (element->focused()) { > > > > Early return: > > if (!element->focused()) > > return; > > will do.

Created attachment 231029 [details] Patch

Comment on attachment 231029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231029&action=review Not sure I can review+ this. Dean Jackson should probably take a look. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1974 > +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element) Since this is exposed to the DOM, it needs to handle null element pointers. Probably just need to return early if it’s null. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1983 > + if (element->focused()) { Why not early return for this check too? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1984 > + GraphicsContext* c = drawingContext(); Would you be willing to consider use a word instead of a letter for this local variable? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1985 > + if (c) Again, why not an early return for this too?

Created attachment 231037 [details] Patch

Comment on attachment 231037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231037&action=review r- because I asked for the changes last review. Patch itself looks good though. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1985 > + if (!element) > + return; > + if (!state().m_hasInvertibleTransform) > + return; > + if (m_path.isEmpty()) > + return; > + if (!element->isDescendantOf(canvas())) > + return; > + > + if (element->focused()) { you can collapse those into one if clause. And (as said in previous review!) make it if (!element->focused()) return > LayoutTests/fast/canvas/draw-focus-if-needed.html:3 > + <button id="button1"></button> Change all tabs to spaces. > LayoutTests/fast/canvas/draw-focus-if-needed.html:23 > +else > + { else { > LayoutTests/fast/canvas/draw-focus-if-needed.html:34 > + } no indentation

Comment on attachment 231037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231037&action=review > Source/WebCore/ChangeLog:7 > + Add some lines what is actually expected by the function. "When called, draws focus ring for descendants of canvas element." or so.

Created attachment 231062 [details] Patch

Comment on attachment 231037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231037&action=review >> Source/WebCore/ChangeLog:7 >> + > > Add some lines what is actually expected by the function. "When called, draws focus ring for descendants of canvas element." or so. done >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1985 >> + if (element->focused()) { > > you can collapse those into one if clause. And (as said in previous review!) make it if (!element->focused()) return done. Sorry that I missed that. >> LayoutTests/fast/canvas/draw-focus-if-needed.html:3 >> + <button id="button1"></button> > > Change all tabs to spaces. done

Comment on attachment 231062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231062&action=review Snippet in changelog. Otherwise looks good to me. Please give Dean time to look at it before landing. > Source/WebCore/ChangeLog:8 > + This API will draw a focus ring if the element is focused. "the element"? What kind of element? It is a child of <canvas>, right? And it is also just drawn when the element is focused and the function is called.

Created attachment 231064 [details] Patch

Comment on attachment 231064 [details] Patch Rejecting attachment 231064 [details] from review queue. cabanier@adobe.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

Comment on attachment 231064 [details] Patch Please wait for dino this time before submitting.

Comment on attachment 231064 [details] Patch Clearing flags on attachment: 231064 Committed r168476: <http://trac.webkit.org/changeset/168476>

All reviewed patches have been landed. Closing bug.