Bug 116563 - PDFPlugin: Main-frame PDFPlugin Accessibility
Summary: PDFPlugin: Main-frame PDFPlugin Accessibility
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:
Blocks:
 
Reported: 2013-05-21 11:05 PDT by Tim Horton
Modified: 2013-05-27 12:52 PDT (History)
9 users (show)

See Also:

Attachments
patch (23.42 KB, patch)
2013-05-21 13:01 PDT, Tim Horton 		eflews.bot: commit-queue-
Details | Formatted Diff | Diff
patch (25.32 KB, patch)
2013-05-24 11:40 PDT, Tim Horton 		no flags Details | Formatted Diff | Diff
patch (25.26 KB, patch)
2013-05-24 11:42 PDT, Tim Horton 		no flags Details | Formatted Diff | Diff
patch (25.25 KB, patch)
2013-05-24 11:44 PDT, Tim Horton 		sam: review+
Details | Formatted Diff | Diff
Show Obsolete (3) 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 2013-05-21 11:05:59 PDT 
<rdar://problem/13458534>
Comment 1 Tim Horton 2013-05-21 13:01:28 PDT 
Created attachment 202461 [details]
patch
Comment 2 WebKit Commit Bot 2013-05-21 13:02:38 PDT 
Attachment 202461 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/Plugin.h', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm']" exit_code: 1
Source/WebKit2/WebProcess/Plugins/PluginView.cpp:788:  Declaration has space between type name and * in NSObject *PluginView  [whitespace/declaration] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tim Horton 2013-05-21 13:03:14 PDT 
Comment on attachment 202461 [details]
patch

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

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:110
> +- (id)initWithPDFPlugin:(WebKit::PDFPlugin *)plugin;

Wrong side *.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:120
> +- (id)initWithPDFPlugin:(WebKit::PDFPlugin *)plugin

Ditto.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:397
> +    m_accessibilityObject = adoptNS([[WKPDFPluginAccessibilityObject alloc] initWithPDFPlugin:this]);

This could be above.
Comment 4 Tim Horton 2013-05-21 13:03:58 PDT 
(In reply to comment #2)
> Attachment 202461 [details] did not pass style-queue:
> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:788:  Declaration has space between type name and * in NSObject *PluginView  [whitespace/declaration] [3]
> Total errors found: 1 in 13 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

You're a false positive :\
Comment 5 EFL EWS Bot 2013-05-21 13:06:59 PDT 
Comment on attachment 202461 [details]
patch

Attachment 202461 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/292228
Comment 6 chris fleizach 2013-05-21 13:16:23 PDT 
Comment on attachment 202461 [details]
patch

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

overall looks good. just a few minor nits.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:121
> +- (CPReadingModel*)readingModel;

space before the *

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:217
> +    return [NSArray array];

we should probably expose the NSAccessibilityShowMenuAction
since I believe you can right click on the PDF

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h:148
> +    virtual NSObject *accessibilityObject() const OVERRIDE { return 0; }

is this needed?
Comment 7 Early Warning System Bot 2013-05-21 15:25:08 PDT 
Comment on attachment 202461 [details]
patch

Attachment 202461 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/524273
Comment 8 Tim Horton 2013-05-21 17:07:27 PDT 
(In reply to comment #6)
> (From update of attachment 202461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202461&action=review
> 
> overall looks good. just a few minor nits.
> 
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:121
> > +- (CPReadingModel*)readingModel;
> 
> space before the *

Yep.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:217
> > +    return [NSArray array];
> 
> we should probably expose the NSAccessibilityShowMenuAction
> since I believe you can right click on the PDF

Good idea!

> > Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h:148
> > +    virtual NSObject *accessibilityObject() const OVERRIDE { return 0; }
> 
> is this needed?

Perhaps not!
Comment 9 Tim Horton 2013-05-24 11:40:31 PDT 
Created attachment 202841 [details]
patch
Comment 10 Tim Horton 2013-05-24 11:42:33 PDT 
Created attachment 202842 [details]
patch
Comment 11 WebKit Commit Bot 2013-05-24 11:43:45 PDT 
Attachment 202842 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/Plugin.h', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm']" exit_code: 1
Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:96:  The parameter name "point" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/PluginView.cpp:523:  Declaration has space between type name and * in NSObject *PluginView  [whitespace/declaration] [3]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Tim Horton 2013-05-24 11:44:22 PDT 
Created attachment 202843 [details]
patch
Comment 13 WebKit Commit Bot 2013-05-24 11:46:37 PDT 
Attachment 202843 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/Plugin.h', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm']" exit_code: 1
Source/WebKit2/WebProcess/Plugins/PluginView.cpp:523:  Declaration has space between type name and * in NSObject *PluginView  [whitespace/declaration] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Sam Weinig 2013-05-25 12:25:41 PDT 
Comment on attachment 202843 [details]
patch

Can we test this?

r=me
Comment 15 Tim Horton 2013-05-25 12:27:00 PDT 
(In reply to comment #14)
> (From update of attachment 202843 [details])
> Can we test this?
> 
> r=me

Not yet!

Thanks!
Comment 16 Tim Horton 2013-05-27 12:52:16 PDT 
http://trac.webkit.org/changeset/150771