[Webkit-unassigned] [Bug 112081] Printing to PDF should produce internal links when HTML has internal links

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=112081


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #192865|review?                     |review-
               Flag|                            |




--- Comment #12 from Alexey Proskuryakov <ap at webkit.org>  2013-03-12 22:31:51 PST ---
(From update of attachment 192865)
View in context: https://bugs.webkit.org/attachment.cgi?id=192865&action=review

Thank you, this looks much better. r- for leaving a dangling pointer.

> Source/WebCore/ChangeLog:9
> +        This change doesn't have any tests because PDF generation doesn't
> +        have any existing tests. Manual testing was performed in Chromium

That said, adding a way to test PDF generation would be a valuable project.

> Source/WebCore/ChangeLog:17
> +        (WebCore):

It's just a bug of prepare-ChangeLog that this cruft is added. The purpose of generating file and name links is to simplify adding per-function comments to ChangeLogs, and unhelpful garbage should be removed (yes, a lot of people don't do that, sadly).

> Source/WebCore/dom/Document.h:1200
> +    HashMap<String, Element*>& usedAnchors() { return m_usedAnchors; }

Looks like this could be const (on both sides). But see other comments.

> Source/WebCore/dom/Document.h:1465
> +    HashMap<String, Element*> m_usedAnchors;

This is very surprising to have as a permanent member of Document. This causes a serious bug: if one generates PDF twice, and an element is removed in between the calls, a dangling pointer is left in the HashMap, and then dereferenced.

The HashMap should at least be cleared when no longer needed, but I think that I have a better suggestion below.

> Source/WebCore/page/FrameView.cpp:3434
> +            p->addDestinationAtPoint(i->key, IntPoint(0, y));

Why no horizontal offset? The user can have the document zoomed in, and then scrolling to (0, y) may not reveal the element.

Can the result of absoluteBoundingBoxRect() have negative coordinates? Perhaps we should clamp to 0. Ditto for being out of page bounds on the other side.

> Source/WebCore/rendering/RenderObject.cpp:1124
> +            document()->addUsedAnchor(name, element);

Would it work if we just called addDestinationAtPoint here? The same destination could be added several times, but I don't see any reason why that would be a problem. We are just using an intermediate map instead of one in PDF context.

> Source/WebCore/rendering/RenderObject.cpp:1129
> +            return;
> +        }
> +    }
> +    context->setURLForRect(url, pixelSnappedIntRect(rect));

Unsure if it's meaningful to add a same-document URL if anchor couldn't be found. Perhaps the early return should be outside "if (element)"?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.