[webkit-reviews] review granted: [Bug 105710] PDFPlugin: Find in page : [Attachment 180652] style

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 105710: PDFPlugin: Find in page
https://bugs.webkit.org/show_bug.cgi?id=105710

Attachment 180652: style
https://bugs.webkit.org/attachment.cgi?id=180652&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.