Bug 103389 - PDFPlugin: "Show Definition" context menu item should be implemented
Summary: PDFPlugin: "Show Definition" context menu item should be implemented
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: PDF (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 103496
Blocks:
  Show dependency tree / graph
 
Reported: 2012-11-27 03:51 PST by Tim Horton
Modified: 2012-11-30 12:51 PST (History)
4 users (show)

See Also:

Attachments
patch (4.73 KB, patch)
2012-11-27 04:08 PST, Tim Horton 		no flags Details | Formatted Diff | Diff
patch (much simplified) (3.89 KB, patch)
2012-11-28 17:40 PST, Tim Horton 		mitz: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-11-27 03:51:37 PST 
<rdar://problem/12710751>
Comment 1 Tim Horton 2012-11-27 04:08:55 PST 
Created attachment 176229 [details]
patch
Comment 2 WebKit Review Bot 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.
Comment 3 Tim Horton 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.
Comment 4 Alexey Proskuryakov 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?
Comment 5 Tim Horton 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.
Comment 6 Tim Horton 2012-11-28 17:40:05 PST 
Created attachment 176614 [details]
patch (much simplified)
Comment 7 mitz 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?
Comment 8 Tim Horton 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