Bug 132638 - Clean up the difference between painting focus rings and adding PDF annotations
Summary: Clean up the difference between painting focus rings and adding PDF annotations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-06 19:08 PDT by Dean Jackson
Modified: 2014-05-07 13:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.52 KB, patch)
2014-05-06 19:13 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>