Bug 36434 - [chromium]WebKit side of adding search support to Pepper.
Summary: [chromium]WebKit side of adding search support to Pepper.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-21 22:33 PDT by John Abd-El-Malek
Modified: 2010-03-24 14:10 PDT (History)
1 user (show)

See Also:


Attachments
Patch (24.90 KB, patch)
2010-03-21 22:49 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Patch (4.54 KB, patch)
2010-03-22 19:01 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2010-03-24 13:27 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2010-03-24 13:31 PDT, John Abd-El-Malek
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2010-03-21 22:33:42 PDT
[chromium]WebKit side of adding search support to Pepper.
Comment 1 John Abd-El-Malek 2010-03-21 22:49:13 PDT
Created attachment 51267 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2010-03-21 23:53:23 PDT
Note to fellow reviewers.  This review is blocked pending agreement on the API, which is being discussed here:  http://codereview.chromium.org/1075011
Comment 3 John Abd-El-Malek 2010-03-22 19:01:31 PDT
Created attachment 51384 [details]
Patch
Comment 4 John Abd-El-Malek 2010-03-22 19:02:27 PDT
Darin: I've updated the patch per our discussion.
Comment 5 Darin Fisher (:fishd, Google) 2010-03-23 15:05:58 PDT
Comment on attachment 51384 [details]
Patch

> 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
Comment 6 John Abd-El-Malek 2010-03-24 13:27:12 PDT
Created attachment 51537 [details]
Patch
Comment 7 John Abd-El-Malek 2010-03-24 13:31:56 PDT
Created attachment 51538 [details]
Patch
Comment 8 John Abd-El-Malek 2010-03-24 13:32:19 PDT
Darin: when you have a chance, here's the updated patch with your suggested changes.
Comment 9 Darin Fisher (:fishd, Google) 2010-03-24 13:58:15 PDT
Comment on attachment 51538 [details]
Patch

> Index: WebKit/chromium/public/WebNode.h

> +    // DEPRECATED! use toConstElement() instead

nit: "use toConst() instead"


> Index: WebKit/chromium/public/WebPluginDocument.h

> +    WebPluginDocument(const WebPluginDocument& e) : WebDocument(e) { }
> +
> +    WebPluginDocument& operator=(const WebPluginDocument& e)
> +    {
> +        WebNode::assign(e);
> +        return *this;
> +    }
> +    void assign(const WebPluginDocument& e) { WebNode::assign(e); }

nit: "e" stood for WebElement probably in the case you copied this
from.  how about using "d" for document instead?


R=me
Comment 10 John Abd-El-Malek 2010-03-24 14:10:42 PDT
Committed r56457: <http://trac.webkit.org/changeset/56457>