Bug 132638

Summary: Clean up the difference between painting focus rings and adding PDF annotations
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, kondapallykalyan, sam, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Dean Jackson 2014-05-06 19:08:51 PDT
Clean up the difference between painting focus rings and adding PDF annotations
Comment 1 Dean Jackson 2014-05-06 19:13:07 PDT
Created attachment 230963 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-05-06 19:24:37 PDT
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 3 WebKit Commit Bot 2014-05-06 19:59:08 PDT
Comment on attachment 230963 [details]
Patch

Clearing flags on attachment: 230963

Committed r168404: <http://trac.webkit.org/changeset/168404>
Comment 4 WebKit Commit Bot 2014-05-06 19:59:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2014-05-06 20:11:29 PDT
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?
Comment 6 Dean Jackson 2014-05-06 20:43:52 PDT
(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.
Comment 7 Dean Jackson 2014-05-06 20:46:29 PDT
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.
Comment 8 Darin Adler 2014-05-06 20:49:58 PDT
(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.
Comment 9 Dean Jackson 2014-05-07 13:04:29 PDT
Committed followup r168437: <http://trac.webkit.org/changeset/168437>