Bug 103389

Summary: PDFPlugin: "Show Definition" context menu item should be implemented
Product: WebKit Reporter: Tim Horton <thorton>
Component: PDFAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, mitz, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 103496    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch (much simplified) mitz: review+

Tim Horton
Reported 2012-11-27 03:51:37 PST
<rdar://problem/12710751>
Attachments
patch (4.73 KB, patch)
2012-11-27 04:08 PST, Tim Horton 		no flags
DetailsFormatted DiffDiff
patch (much simplified) (3.89 KB, patch)
2012-11-28 17:40 PST, Tim Horton 		mitz: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tim Horton
Comment 1 2012-11-27 04:08:55 PST
Created attachment 176229 [details] patch
WebKit Review Bot
Comment 2 2012-11-27 04:13:17 PST
Attachment 176229 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:70: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:70: The parameter name "point" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Tim Horton
Comment 3 2012-11-27 04:24:22 PST
Comment on attachment 176229 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=176229&action=review > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:725 > + NSMutableDictionary *fontDescriptorAttributes = [[[font fontDescriptor] fontAttributes] mutableCopy]; Whoops, this leaks. Will fix tomorrow.
Alexey Proskuryakov
Comment 4 2012-11-27 08:38:19 PST
Comment on attachment 176229 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=176229&action=review > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:718 > + NSDictionary *attributes = [string attributesAtIndex:0 effectiveRange:0]; What makes attributes at this particular index special?
Tim Horton
Comment 5 2012-11-27 13:51:51 PST
Comment on attachment 176229 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=176229&action=review >> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:718 >> + NSDictionary *attributes = [string attributesAtIndex:0 effectiveRange:0]; > > What makes attributes at this particular index special? We can only have one font for the dictionary popup, so why not the font at the beginning of the attributed string? I believe this is effectively identical to what WebPageMac does with its Ranges for web content.
Tim Horton
Comment 6 2012-11-28 17:40:05 PST
Created attachment 176614 [details] patch (much simplified)
mitz
Comment 7 2012-11-30 07:19:28 PST
Comment on attachment 176614 [details] patch (much simplified) View in context: https://bugs.webkit.org/attachment.cgi?id=176614&action=review > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:780 > + dictionaryPopupInfo.origin = convertFromPDFViewToRootView(IntPoint(point)); Is the IntPoint() necessary?
Tim Horton
Comment 8 2012-11-30 12:51:25 PST
(In reply to comment #7) > (From update of attachment 176614 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176614&action=review > > > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:780 > > + dictionaryPopupInfo.origin = convertFromPDFViewToRootView(IntPoint(point)); > > Is the IntPoint() necessary? It is. Thanks, Dan! http://trac.webkit.org/changeset/136260
Note You need to log in before you can comment on or make changes to this bug.