Clean up the difference between painting focus rings and adding PDF annotations
Created attachment 230963 [details] Patch
Comment on attachment 230963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230963&action=review > Source/WebCore/rendering/RenderObject.cpp:1016 > +void RenderObject::addPDFURLRect(PaintInfo& paintInfo, const LayoutPoint& paintOffset) This function needs to DIAF.
Comment on attachment 230963 [details] Patch Clearing flags on attachment: 230963 Committed r168404: <http://trac.webkit.org/changeset/168404>
All reviewed patches have been landed. Closing bug.
Comment on attachment 230963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230963&action=review > Source/WebCore/rendering/RenderInline.cpp:1499 > + else if (hasOutlineAnnotation() && !theme().supportsFocusRing(&styleToUse)) > + addPDFURLRect(paintInfo, paintOffset); Why is this else? Why not an independent if?
(In reply to comment #5) > (From update of attachment 230963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230963&action=review > > > Source/WebCore/rendering/RenderInline.cpp:1499 > > + else if (hasOutlineAnnotation() && !theme().supportsFocusRing(&styleToUse)) > > + addPDFURLRect(paintInfo, paintOffset); > > Why is this else? Why not an independent if? Because the original logic was: if (A || B) { if (C) paintFocusRing(); } void paintFocusRing() { if (A) // actually paint focus ring else PDFRect() } So the new PDF path still has to check !A (the code that was in paintFocusRing). I could have done that in the separate if, but I figured the else branch was cleaner. New code is: if (A && C) paintFocusRing() else if (B && C) PDFRect() The "else if" knows that either A is false (which is what we need) or C is false (in which case it will fail itself). The alternative would be: if (A && C) paintFocusRing if (!A && B && C) PDFRect() Now that I've written it, what I committed is pretty subtle, so I'll fix it.
BTW, I left the "if (A)" in paintFocusRing (outlineStyleIsAuto) even though these are the only two callers. They will only call when A is true, but who knows what someone else will do in the future.
(In reply to comment #7) > BTW, I left the "if (A)" in paintFocusRing (outlineStyleIsAuto) even though these are the only two callers. They will only call when A is true, but who knows what someone else will do in the future. We can document such preconditions and include assertions if we prefer to do that instead of doing redundant checks in the production code.
Committed followup r168437: <http://trac.webkit.org/changeset/168437>