Chrome side of adding search support to Pepper.

Chrome side of adding search support to Pepper.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42585

[jam, 2010-03-22 05:50:44]

btw the WebKit patch is here: https://bugs.webkit.org/show_bug.cgi?id=36434

[darin (slow to review), 2010-03-22 05:52:45]

initial round: questions about the interface

http://codereview.chromium.org/1075011/diff/1/4
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1075011/diff/1/4#newcode546
third_party/npapi/bindings/npapi_extensions.h:546: uint32 options,
options can be a bitwise union of NPFindOption values?

http://codereview.chromium.org/1075011/diff/1/4#newcode557
third_party/npapi/bindings/npapi_extensions.h:557: typedef NPError
(*NPPStopFindPtr) (
The implication of StopFind is that Find continues to do work after finding the
first result?  It would be good to document this asynchronous behavior.

Perhaps GoToFindResult should just be named FindNext?  Does GoToFindResult
return an error if there are no more results?  If so, then perhaps Find should
also just return an error if there are no results and success if there was a
match?

How does the embedder learn how many matches there are on the page?

[jam, 2010-03-22 05:59:32]

http://codereview.chromium.org/1075011/diff/1/4
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1075011/diff/1/4#newcode546
third_party/npapi/bindings/npapi_extensions.h:546: uint32 options,
On 2010/03/22 05:52:45, darin wrote:
> options can be a bitwise union of NPFindOption values?

yep that's correct.  In other places that did this, I didn't see an explicit
comment which is why I didn't add one here.

http://codereview.chromium.org/1075011/diff/1/4#newcode557
third_party/npapi/bindings/npapi_extensions.h:557: typedef NPError
(*NPPStopFindPtr) (
On 2010/03/22 05:52:45, darin wrote:
> The implication of StopFind is that Find continues to do work after finding
the
> first result?  It would be good to document this asynchronous behavior.

Not quite. I should have documented this more, I will add more comments about
how these functions should be used once we agree on the API.

I tried to come up with something as simple as possible to understand (I found
the current code in WebKit that mixes both searching and navigating hard to
understand).  I found it much easier to separate the searching from the
navigation.  The embedder calls find() repeatedly until it returns false.  This
will provide the number of results.  The plugin stores information about the
found items, so that the next/previous calls are just very cheap functions and
it doesn't have to recalculate the next position.

Makes sense?
> 
> Perhaps GoToFindResult should just be named FindNext?  Does GoToFindResult
> return an error if there are no more results?  If so, then perhaps Find should
> also just return an error if there are no results and success if there was a
> match?
> 
> How does the embedder learn how many matches there are on the page?

[jam, 2010-03-23 05:20:29]

Updated patch to reflect new API per discussion.

[darin (slow to review), 2010-03-23 21:46:54]

http://codereview.chromium.org/1075011/diff/19001/5004
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1075011/diff/19001/5004#newcode15
third_party/npapi/bindings/npapi_extensions.h:15: * The variable returns a
pointer to an NPPepperExtensions structure. */
it looks like NPPepperExtensions is actually NPExtensions in the code, but it
should probably be NPNExtensions.

http://codereview.chromium.org/1075011/diff/19001/5004#newcode20
third_party/npapi/bindings/npapi_extensions.h:20: * The variable returns a
pointer to an NPPPepperExtensions structure. */
likewise, NPPPepperExtensions is actually NPPExtensions in the code

http://codereview.chromium.org/1075011/diff/19001/5004#newcode189
third_party/npapi/bindings/npapi_extensions.h:189: int current_selection);
nit: current_selection -> currentSelection or selectedIndex?

[jam, 2010-03-24 21:00:02]

http://codereview.chromium.org/1075011/diff/19001/5004
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1075011/diff/19001/5004#newcode15
third_party/npapi/bindings/npapi_extensions.h:15: * The variable returns a
pointer to an NPPepperExtensions structure. */
On 2010/03/23 21:46:54, darin wrote:
> it looks like NPPepperExtensions is actually NPExtensions in the code, but it
> should probably be NPNExtensions.

updated the code and comment

http://codereview.chromium.org/1075011/diff/19001/5004#newcode20
third_party/npapi/bindings/npapi_extensions.h:20: * The variable returns a
pointer to an NPPPepperExtensions structure. */
On 2010/03/23 21:46:54, darin wrote:
> likewise, NPPPepperExtensions is actually NPPExtensions in the code

updated the comment

http://codereview.chromium.org/1075011/diff/19001/5004#newcode189
third_party/npapi/bindings/npapi_extensions.h:189: int current_selection);
On 2010/03/23 21:46:54, darin wrote:
> nit: current_selection -> currentSelection or selectedIndex?

this is now in a different function and i just used 'index'

[darin (slow to review), 2010-03-24 23:16:29]

LGTM

http://codereview.chromium.org/1075011/diff/36001/37007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1075011/diff/36001/37007#newcode3282
chrome/renderer/render_view.cc:3282: webkit_glue::WebPluginDelegate* delegate =
nit: how about adding a helper function to get the WebPluginDelegate?  it could
be used in OnStopFinding as well.

http://codereview.chromium.org/1075011/diff/36001/37003
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1075011/diff/36001/37003#newcode156
third_party/npapi/bindings/npapi_extensions.h:156: // TODO: this typedef is just
for the NaCl code until they switch to NPNExtensions.
It might be nice to use the term "DEPRECATED" so that things like this are easy
to search for in the future.

[jam, 2010-03-24 23:52:03]

http://codereview.chromium.org/1075011/diff/36001/37007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1075011/diff/36001/37007#newcode3282
chrome/renderer/render_view.cc:3282: webkit_glue::WebPluginDelegate* delegate =
On 2010/03/24 23:16:29, darin wrote:
> nit: how about adding a helper function to get the WebPluginDelegate?  it
could
> be used in OnStopFinding as well.

Done.

http://codereview.chromium.org/1075011/diff/36001/37003
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1075011/diff/36001/37003#newcode156
third_party/npapi/bindings/npapi_extensions.h:156: // TODO: this typedef is just
for the NaCl code until they switch to NPNExtensions.
On 2010/03/24 23:16:29, darin wrote:
> It might be nice to use the term "DEPRECATED" so that things like this are
easy
> to search for in the future.

Done.