Bug 177582 – Request for PDF URL targets for specific pages, sections, etc.

• Component: PDF
• Status: RESOLVED FIXED

[Aishwarya Nirmal]

<rdar://33496301>

[Aishwarya Nirmal]

Created attachment 322043 [details]
Patch

[Tim Horton]

Comment on attachment 322043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322043&action=review

> Source/WebKit/ChangeLog:5
> +        <rdar://problem/33496301>

This should be 5692679; the one you mentioned is a dupe.

> Source/WebKit/WebProcess/Plugins/PDF/PDFLayerControllerSPI.h:71
> +@property (nonatomic, strong) NSString* URLFragment;

The asterisk is on the wrong side; we put them on the left for C++ types and on the right for ObjC types (it's weird, I know).

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1039
> +        m_pdfLayerController.get().URLFragment = pdfURL.substring(index);

This should get wrapped in an __MAC_OS_X_VERSION_MIN_REQUIRED ifdef, because it's not available in shipping versions of macOS.

[Tim Horton]

Comment on attachment 322043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322043&action=review

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1037
> +    size_t index = pdfURL.reverseFind('#');

I bet our URL parser has a better way to do this! We should ask Alex.

>> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1039
>> +        m_pdfLayerController.get().URLFragment = pdfURL.substring(index);
> 
> This should get wrapped in an __MAC_OS_X_VERSION_MIN_REQUIRED ifdef, because it's not available in shipping versions of macOS.

Also I generally prefer the [m_pdfLayerController setURLFragment:] variant instead of dot notation in the case where you've got a RetainPtr. (.get() is so ugly)

[Tim Horton]

Comment on attachment 322043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322043&action=review

>> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1037
>> +    size_t index = pdfURL.reverseFind('#');
> 
> I bet our URL parser has a better way to do this! We should ask Alex.

Actually, I don't even think we need to ask Alex; URL has fragmentIdentifier()

[Wenson Hsieh]

(In reply to Tim Horton from comment #2)
> Comment on attachment 322043 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322043&action=review
> 
> > Source/WebKit/ChangeLog:5
> > +        <rdar://problem/33496301>
> 
> This should be 5692679; the one you mentioned is a dupe.
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFLayerControllerSPI.h:71
> > +@property (nonatomic, strong) NSString* URLFragment;
> 
> The asterisk is on the wrong side; we put them on the left for C++ types and
> on the right for ObjC types (it's weird, I know).
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1039
> > +        m_pdfLayerController.get().URLFragment = pdfURL.substring(index);
> 
> This should get wrapped in an __MAC_OS_X_VERSION_MIN_REQUIRED ifdef, because
> it's not available in shipping versions of macOS.

If I understand correctly, this should be a MIN_REQUIRED check in the implementation file and a MAX_ALLOWED check in PDFLayerControllerSPI.h?

[Aishwarya Nirmal]

Created attachment 322096 [details]
Patch

[Aishwarya Nirmal]

(In reply to Aishwarya Nirmal from comment #0)
> <rdar://33496301>

The original bug is <rdar://5692679>

[Tim Horton]

The title from 5692679 would have been preferable (it makes more sense), for future reference.

[WebKit Commit Bot]

Comment on attachment 322096 [details]
Patch

Clearing flags on attachment: 322096

Committed r222620: <http://trac.webkit.org/changeset/222620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Minor follow up in https://trac.webkit.org/changeset/222624/webkit

[Karim Ardalan]

This feature is working in Safari for MacOS but still isn't working for Safari on iOS/iPadOS. Could you please fix this?

https://helpx.adobe.com/acrobat/kb/link-html-pdf-page-acrobat.html