Add option to open external links in new window

[mjlyons]

This change adds a PDFJS.openExternalLinkInNewWindow boolean, which opens links in a new window when enabled. The default now and previous to this change, clicking on an external link will open the link the same window (and navigate away from PDF.js).

[Snuffleupagus]

Won't this set target = '_blank' for both external and internal links, when the condition is true? You probably need to do something along these lines instead:

if (item.url && PDFJS.openExternalLinksInNewWindow) {

[mjlyons]

@Snuffleupagus - I didn't realize internal links would hit this codepath but make sense and added the url check. FWIW, when I created a link to another page (in Acrobat XI) it correctly jumped to the page even without this check, put perhaps this wasn't an "internal link"?

[Snuffleupagus]

If you look at annotations_layer_builder.js#L103, you will see that all links are fetched in the same way by the viewer code. Further down, at annotations_layer_builder.js#L126, you'll see the difference between the way internal/external links are handled.

With the current way that internal destinations are implemented, your first patch "worked" correctly more or less by accident :-)
But in order for this patch to work as advertised (i.e. only for external links) and to avoid any future issues (e.g. if we ever re-factor the link code for the viewer), we need to make sure that it only applies to external links. I hope this helps explain things, if not just ask!

[mjlyons]

Cool, totally make sense! Is there anything else for me to do? Not trying to be pushy, rather, I haven't contributed to pdf.js before and want to make sure you're not waiting on me =)

[Snuffleupagus]

Is there anything else for me to do? Not trying to be pushy, rather, I haven't contributed to pdf.js before and want to make sure you're not waiting on me =)

This seems fine to me; but given that adding a configuration option usually means that you'll have to support it forever, I figured it would be a good idea for one of the devs to sign off on this.

/cc @yurydelendik, @brendandahl Any feedback on this PR?

[yurydelendik]

but given that adding a configuration option... have to support it forever

yeah, eventually we have to move those properties for stateless objects. we can keep that where it is, but in the future add migration path to default those objects properties to set values from obsolete properties in the PDFJS.

Just a reminder for users of such properties to set them once before getDocument is invoked, to avoid confusion in the future.

Thanks for the patch.