Bug 105710 – PDFPlugin: Find in page

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

We need to hook up find-in-page to PDFPlugin!

<rdar://problem/12555331>

[Tim Horton]

Created attachment 180651 [details]
patch

[WebKit Review Bot]

Attachment 180651 [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:102:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:102:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:103:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/PluginView.h:97:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/PluginView.h:97:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/Plugins/PluginView.h:98:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Plugin.h:258:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Plugin.h:258:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/Plugins/Plugin.h:260:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:962:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:217:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/Plugins/PluginProxy.h:131:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h:177:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/Plugins/PluginView.cpp:739:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 14 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 180652 [details]
style

[Alexey Proskuryakov]

Comment on attachment 180652 [details]
style

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

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:218
> +    virtual unsigned countFindMatches(const String&, WebCore::FindOptions, unsigned);
> +    virtual bool findString(const String&, WebCore::FindOptions, unsigned);

OVERRIDE

Also, the "unsigned" arguments could use a name.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:103
> +    virtual unsigned countFindMatches(const String& target, WebCore::FindOptions, unsigned) OVERRIDE;
> +    virtual bool findString(const String& target, WebCore::FindOptions, unsigned) OVERRIDE;

The unsigned arguments could use a name.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:851
> +unsigned PDFPlugin::countFindMatches(const String& target, WebCore::FindOptions options, unsigned limit)

There is a "using namespace WebCore" in this file. Is this prefix necessary to disambiguate?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:856
> +    int nsOptions = options & FindOptionsCaseInsensitive ? NSCaseInsensitiveSearch : 0;

Braces around the condition would make this more readable.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:903
> +bool PDFPlugin::findString(const String& target, WebCore::FindOptions options, unsigned maxMatchCount)

There is a "using namespace WebCore" in this file. Is this prefix necessary to disambiguate?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:919
> +    if (target == emptyString()) {

target.isEmpty()?

Should this case reset m_lastFoundString?

> Source/WebKit2/WebProcess/Plugins/Plugin.h:260
> +    virtual unsigned countFindMatches(const String& target, WebCore::FindOptions, unsigned) = 0;
> +
> +    virtual bool findString(const String& target, WebCore::FindOptions, unsigned) = 0;

The unsigned argument could use a name (and I'm not sure if "limit" is descriptive).

> Source/WebKit2/WebProcess/Plugins/PluginView.h:98
> +    virtual unsigned countFindMatches(const String& target, WebCore::FindOptions, unsigned);
> +    virtual bool findString(const String& target, WebCore::FindOptions, unsigned);

These functions don't seem to be overrides, or ever overridden. Should they actually be virtual?

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:76
> +    PluginView* pluginView = static_cast<PluginView*>(pluginDocument->pluginWidget());

Why is this typecast safe?

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:188
> +    bool found = false;

Is this initialization necessary to shut up a particular dumb compiler? It doesn't help readability.

[Build Bot]

Comment on attachment 180652 [details]
style

Attachment 180652 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15492396

New failing tests:
svg/as-image/img-relative-height.html

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 180652 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180652&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:851
> > +unsigned PDFPlugin::countFindMatches(const String& target, WebCore::FindOptions options, unsigned limit)
> 
> There is a "using namespace WebCore" in this file. Is this prefix necessary to disambiguate?

I believe it is. I'm also slightly concerned about the fact that we have WebKit::FindOptions and WebCore::FindOptions with overlapping unrelated values:

In WebCore:

enum FindOptionFlag {
    CaseInsensitive = 1 << 0,
    AtWordStarts = 1 << 1,
    // When combined with AtWordStarts, accepts a match in the middle of a word if the match begins with
    // an uppercase letter followed by a lowercase or non-letter. Accepts several other intra-word matches.
    TreatMedialCapitalAsWordStart = 1 << 2,
    Backwards = 1 << 3,
    WrapAround = 1 << 4,
    StartInSelection = 1 << 5
};

typedef unsigned FindOptions;

In WebKit2:

enum FindOptions {
    FindOptionsCaseInsensitive = 1 << 0,
    FindOptionsAtWordStarts = 1 << 1,
    FindOptionsTreatMedialCapitalAsWordStart = 1 << 2,
    FindOptionsBackwards = 1 << 3,
    FindOptionsWrapAround = 1 << 4,
    FindOptionsShowOverlay = 1 << 5,
    FindOptionsShowFindIndicator = 1 << 6,
    FindOptionsShowHighlight = 1 << 7
};

(Note that StartInSelection and ShowOverlay have the same value. A bit scary/unfortunate, probably not problematic.)

[Tim Horton]

http://trac.webkit.org/changeset/138461

[Alexey Proskuryakov]

> I believe it is. I'm also slightly concerned about the fact that we have WebKit::FindOptions and WebCore::FindOptions with overlapping unrelated values:

I think that that renaming one of these would be a better solution than relying on namespaces.

[Tim Horton]

(In reply to comment #8)
> > I believe it is. I'm also slightly concerned about the fact that we have WebKit::FindOptions and WebCore::FindOptions with overlapping unrelated values:
> 
> I think that that renaming one of these would be a better solution than relying on namespaces.

I filed https://bugs.webkit.org/show_bug.cgi?id=105745 about that.