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

[bugzilla-daemon at webkit.org]

Tim Horton <timothy_horton at apple.com> 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 194177: Patch
https://bugs.webkit.org/attachment.cgi?id=194177&action=review

------- Additional Comments from Tim Horton <timothy_horton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=194177&action=review


> Source/WebCore/page/FrameView.cpp:3490
> +    if (!m_linkedDestinationsValid) {
> +	   collectLinkedDestinations(node);
> +	   m_linkedDestinationsValid = true;
> +    }

What if you print, the page is mutated via JavaScript, and then you print
again? m_linkedDestinations will be stale, right? And contain stale pointers to
Elements? Which you will then dereference below?

> Source/WebCore/page/FrameView.cpp:3495
> +	   IntRect bb = it->value->renderer()->absoluteBoundingBoxRect();
> +	   IntPoint point(bb.x(), bb.y());

Please expand "bb".

> Source/WebCore/page/FrameView.cpp:3496
> +	   if (pageRect.contains(point))

What if the bounds are shoved off the page but the bounding box intersects the
page? You probably want "intersection of pageRect and 'bb' is not empty", no?

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:1113
> +    return true;

Shouldn't this only return true if the graphics context is backed by e.g. a
PDF? Certainly you don't want to do all this work for a bitmap Skia context...