[webkit-reviews] review denied: [Bug 112081] Printing to PDF should produce internal links when HTML has internal links : [Attachment 192604] 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 192604: Patch
https://bugs.webkit.org/attachment.cgi?id=192604&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=192604&action=review


Looks fine in general.

> Source/WebCore/ChangeLog:8
> +	   This change doesn't have any tests because PDF generation doesn't
have any existing tests.

How did you test manually?

> Source/WebCore/platform/graphics/GraphicsContext.h:271
> +	   void setShouldOutputInternalLinks(bool);

This function is never called in this patch.

> Source/WebCore/platform/graphics/GraphicsContext.h:431
>	   void setURLForRect(const KURL&, const IntRect&);
>  
> +	   void setLinkToDestination(const char*, const IntRect&);

This first parameter needs a name, it's not easily deducible from type. The
parameter should be a String, it's the job of GraphicsContext to convert it to
a type suitable for a specific platform.

It would be better to have these functions follow a common style. I'd name the
new one "setDestinationForRect".

> Source/WebCore/platform/graphics/GraphicsContext.h:432
> +	   void addDestination(const String&, IntPoint);

addDestinationAtPoint? Also, we normally pass IntPoint as const reference, too.


> Source/WebCore/rendering/RenderObject.cpp:1118
> +    KURL url = n->document()->completeURL(href);
> +    CString urlString = url.string().utf8();

The URL may be invalid, which needs to be accounted for.

> Source/WebCore/rendering/RenderObject.cpp:1120
> +	   context->setLinkToDestination(strchr(urlString.data(), '#') + 1,
pixelSnappedIntRect(rect));

A better way to determine what the identifier is is would be via
KURL::fragmentIdentifier().

> Source/WebCore/rendering/RenderObject.cpp:1127
> +    Node* n = this->node();

The reason to say "this->node()" is to avoid conflicts with a local variable of
the same name, which you don't have here.

But using abbreviations is discouraged in WebKit code, so you should.

> Source/WebCore/rendering/RenderObject.cpp:1139
> +    this->addDestination(graphicsContext);

No reason for "this->" here.

> Source/WebCore/rendering/RenderObject.cpp:1142
> +    while (child) {
> +	   child->addDestinationsRecursive(graphicsContext);

Wow, adding a destination for every element with an id is quite some work. You
explained the reason in the bug, but I'm not convinced. If there is already an
HTML document on the Web, other pages should link to it, not to a PDF. And if
it's not on the Web, a real authoring tool should be used to produce the file
to publish, not a browser's "Save As" command.

Also, anchors link to names, not to ids.

> Source/WebCore/rendering/RenderObject.h:983
> +    void addDestination(GraphicsContext*);

This function is never called from outside, and should be private.