Bug 132638 – Clean up the difference between painting focus rings and adding PDF annotations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Clean up the difference between painting focus rings and adding PDF annotations

[Dean Jackson]

Created attachment 230963 [details]
Patch

[Simon Fraser (smfr)]

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.

[WebKit Commit Bot]

Comment on attachment 230963 [details]
Patch

Clearing flags on attachment: 230963

Committed r168404: <http://trac.webkit.org/changeset/168404>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?

[Dean Jackson]

(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.

[Dean Jackson]

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.

[Darin Adler]

(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.

[Dean Jackson]

Committed followup r168437: <http://trac.webkit.org/changeset/168437>