[webkit-reviews] review denied: [Bug 112081] Printing to PDF should produce internal links when HTML has internal links : [Attachment 192865] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied David Lattimore
<dml+webkit at google.com>'s request for review:
Bug 112081: Printing to PDF should produce internal links when HTML has
internal links
https://bugs.webkit.org/show_bug.cgi?id=112081

Attachment 192865: Patch
https://bugs.webkit.org/attachment.cgi?id=192865&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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)"?