[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





--- Comment #10 from David Lattimore <dml+webkit at google.com>  2013-03-12 21:13:38 PST ---
(From update of attachment 192604)
View in context: https://bugs.webkit.org/attachment.cgi?id=192604&action=review

>> 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?

I wrote a test HTML file with various different elements (<h1>, <p>, <a>, <div>, <td>). These had IDs. One of the <a> elements had a name instead. I then had links to each of these. I also had an element with an ID but no link to it and a link to something that didn't exist. I generated PDFs from this HTML using both a headless version of WebKit and file->print->save as PDF in Chromium. I then opened these PDFs in a text editor to make sure the links and the destinations looked right (and to make sure there was no destination defined for the element with no link to it). I then opened the PDFs in Evince, Acroread and the PDF viewer on Mac OS and confirmed that clicking the links scrolled to the appropriate page (or in the case of Evince to the actual element itself).

>> Source/WebCore/platform/graphics/GraphicsContext.h:271
>> +        void setShouldOutputInternalLinks(bool);
> 
> This function is never called in this patch.

Was thinking of providing a way to disable it in case some users of WebKit didn't want it. Probably less needed now that I'm only outputting named destinations that are used. Removed this.

>> Source/WebCore/platform/graphics/GraphicsContext.h:431
>> +        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".

Done

>> Source/WebCore/platform/graphics/GraphicsContext.h:432
>> +        void addDestination(const String&, IntPoint);
> 
> addDestinationAtPoint? Also, we normally pass IntPoint as const reference, too.

Done

>> Source/WebCore/rendering/RenderObject.cpp:1118
>> +    CString urlString = url.string().utf8();
> 
> The URL may be invalid, which needs to be accounted for.

Done

>> 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().

Done

>> 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.

This code has now been removed.

>> Source/WebCore/rendering/RenderObject.cpp:1139
>> +    this->addDestination(graphicsContext);
> 
> No reason for "this->" here.

Code removed.

>> Source/WebCore/rendering/RenderObject.cpp:1142
>> +        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.

IDs are checked first. Names are checked second and only for <a> elements. http://www.w3.org/html/wg/drafts/html/master/browsers.html#scroll-to-fragid

Fortunately I was able to take advantage of Document::findAnchor, so this patch no longer has to worry about those details.

I've changed the patch to:
1) only output links to named destinations where such a named destination exists.
2) only output named destinations if something links to it.

>> Source/WebCore/rendering/RenderObject.h:983
>> +    void addDestination(GraphicsContext*);
> 
> This function is never called from outside, and should be private.

Removed method.

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