[webkit-reviews] review denied: [Bug 36434] [chromium]WebKit side of adding search support to Pepper. : [Attachment 51384] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied John Abd-El-Malek
<jam at chromium.org>'s request for review:
Bug 36434: [chromium]WebKit side of adding search support to Pepper.
https://bugs.webkit.org/show_bug.cgi?id=36434

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/public/WebDocument.h
...
>      // Returns the frame the document belongs to or 0 if the document is
frameless.
>      WEBKIT_API WebFrame* frame() const;
>      WEBKIT_API bool isHTMLDocument() const;
> +    WEBKIT_API bool isPluginDocument(WebPluginContainer**);

I think it is a little awkward for this function to have the side effect of
returning the contained plugin.

We could instead follow the inheritance approach.  Define WebPluginDocument:

class WebPluginDocument : public WebDocument {
public:
    /* boiler plate */
    WEBKIT_API WebPlugin* plugin();
};

Then, WebDocument would just have the simple method:

    WEBKIT_API bool isPluginDocument() const;

Folks would then use WebPluginDocument like so.

    WebDocument doc = ...;
    if (doc.isPluginDocument()) {
	WebPlugin* plugin = doc.to<WebPluginDocument>.plugin();
	...
    }

Unfortunately, WebNode (which WebDocument extends) doesn't have a to<T>
method.  It just has a toElement<T> method, which should really just be
renamed to<T> so that it can be used with derived types that are not
elements.  For now, we could just add a copy of toElement<T> named to<T>,
and mark toElement<T> as deprecated.  You can leave it to me to clean up
the deprecated usage.

-Darin