RESOLVED FIXED 177582
Request for PDF URL targets for specific pages, sections, etc.
https://bugs.webkit.org/show_bug.cgi?id=177582
Summary Request for PDF URL targets for specific pages, sections, etc.
Aishwarya Nirmal
Reported 2017-09-27 16:50:31 PDT
Attachments
Patch (2.37 KB, patch)
2017-09-27 16:57 PDT, Aishwarya Nirmal
no flags
Patch (2.48 KB, patch)
2017-09-28 10:32 PDT, Aishwarya Nirmal
no flags
Aishwarya Nirmal
Comment 1 2017-09-27 16:57:56 PDT
Tim Horton
Comment 2 2017-09-27 17:05:04 PDT
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 3 2017-09-27 17:07:30 PDT
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 4 2017-09-27 17:08:22 PDT
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
Comment 5 2017-09-28 10:05:28 PDT
(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
Comment 6 2017-09-28 10:32:06 PDT
Aishwarya Nirmal
Comment 7 2017-09-28 10:34:43 PDT
(In reply to Aishwarya Nirmal from comment #0) > <rdar://33496301> The original bug is <rdar://5692679>
Tim Horton
Comment 8 2017-09-28 11:13:32 PDT
The title from 5692679 would have been preferable (it makes more sense), for future reference.
WebKit Commit Bot
Comment 9 2017-09-28 11:40:45 PDT
Comment on attachment 322096 [details] Patch Clearing flags on attachment: 322096 Committed r222620: <http://trac.webkit.org/changeset/222620>
WebKit Commit Bot
Comment 10 2017-09-28 11:40:46 PDT
All reviewed patches have been landed. Closing bug.
Tim Horton
Comment 11 2017-09-28 12:35:37 PDT
Karim Ardalan
Comment 12 2022-05-12 08:46:44 PDT
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
Note You need to log in before you can comment on or make changes to this bug.