Make RenderView call the plugin through WebKit::WebPlugin (instead of casting...

Chrome side of making RenderView not special case PluginDocument and making the WebPlugin API more like WebView.

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

[jam, 2010-07-08 00:59:00]

Updated per the new WebKit API

[Nico, 2010-07-08 03:51:39]

Looks good as far as I can tell, but I'm not too familiar with the plumbing in
renderer.

http://codereview.chromium.org/2827047/diff/17001/18002
File chrome/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/2827047/diff/17001/18002#newcode36
chrome/renderer/pepper_plugin_delegate_impl.h:36: virtual void
NumberOfFindResultsChanged(int identifier,
nit: ...DidChange sounds less like an accessor

http://codereview.chromium.org/2827047/diff/17001/18013
File webkit/glue/plugins/webplugin_delegate.h (right):

http://codereview.chromium.org/2827047/diff/17001/18013#newcode160
webkit/glue/plugins/webplugin_delegate.h:160: // Gets the selected UTF8 text, if
any.
"UTF8" is probably out of date. s/text/text or markup/

http://codereview.chromium.org/2827047/diff/17001/18015
File webkit/glue/plugins/webplugin_impl.h (right):

http://codereview.chromium.org/2827047/diff/17001/18015#newcode104
webkit/glue/plugins/webplugin_impl.h:104: virtual void selectFindResult(bool
forward);
nit: enums are much better than bools for parameters.

Compare calling site

  plugin->selectFindResult(false);

to

  plugin->selectFindResult(FIND_BACKWARD);

[jam, 2010-07-08 04:11:43]

thanks.

On Wed, Jul 7, 2010 at 8:51 PM, <thakis@chromium.org> wrote:

> Looks good as far as I can tell, but I'm not too familiar with the plumbing
> in
> renderer.
>
>
> http://codereview.chromium.org/2827047/diff/17001/18002
> File chrome/renderer/pepper_plugin_delegate_impl.h (right):
>
> http://codereview.chromium.org/2827047/diff/17001/18002#newcode36
> chrome/renderer/pepper_plugin_delegate_impl.h:36: virtual void
> NumberOfFindResultsChanged(int identifier,
> nit: ...DidChange sounds less like an accessor
>

in this case, and others below, I'm keeping the same names as before (this
change moves the code around a bit).  i prefer to keep it the same as in
other places.


>
> http://codereview.chromium.org/2827047/diff/17001/18013
> File webkit/glue/plugins/webplugin_delegate.h (right):
>
> http://codereview.chromium.org/2827047/diff/17001/18013#newcode160
> webkit/glue/plugins/webplugin_delegate.h:160: // Gets the selected UTF8
> text, if any.
> "UTF8" is probably out of date. s/text/text or markup/
>

took out


>
> http://codereview.chromium.org/2827047/diff/17001/18015
> File webkit/glue/plugins/webplugin_impl.h (right):
>
> http://codereview.chromium.org/2827047/diff/17001/18015#newcode104
> webkit/glue/plugins/webplugin_impl.h:104: virtual void
> selectFindResult(bool forward);
> nit: enums are much better than bools for parameters.
>
> Compare calling site
>
>  plugin->selectFindResult(false);
>
> to
>
>  plugin->selectFindResult(FIND_BACKWARD);


agreed looks nicer, but as this already landed in webkit, and was just
moving code around, i prefer to keep as is

>
>
> http://codereview.chromium.org/2827047/show
>

[viettrungluu, 2010-07-08 04:33:15]

Looks okay to me too, though I don't know enough to comment on the "making the
WebPlugin API more like WebView" part off-hand.

[darin (slow to review), 2010-07-08 06:29:13]

Looks great overall.

http://codereview.chromium.org/2827047/diff/17001/18002
File chrome/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/2827047/diff/17001/18002#newcode36
chrome/renderer/pepper_plugin_delegate_impl.h:36: virtual void
NumberOfFindResultsChanged(int identifier,
On 2010/07/08 03:51:39, Nico wrote:
> nit: ...DidChange sounds less like an accessor

or even better: DidChangeNumberOfFindResults, DidChangeSelectedFindResult