[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 #192604|review?                     |review-
               Flag|                            |




--- Comment #6 from Alexey Proskuryakov <ap at webkit.org>  2013-03-11 22:10:03 PST ---
(From update of attachment 192604)
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.

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