Fix issue that IME can't be enabled when set Plugin mode to "Click to play".

Fix issue that IME can't be enabled when set Plugin mode to "Click to play".

In normal mode, the plugin is created at the moment the page is loaded. Then when the first time the plugin is clicked. It will set the flag "has_webkit_focus_" true in real plugin instance. Then, send out the focus change notification in PepperPluginInstanceImpl::SetWebKitFocus, which will finally affect the IME stuff.

However, at "Click to play" mode, WebViewPlugin is created at first, which will be replaced by real plugin. When the first time we click, the real plugin is created, loaded. But no focus is set in real plugin, which cause the IME stuff can't be enabled. 

In solution, we transfer the |focused_| in WebViewPlugin to new plugin after it loaded.

BUG=336740
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251303

[Ruiyi Luo, 2014-01-22 09:29:03]

I don't know who else is available for this review. 
Thanks a lot.

[Bernhard Bauer, 2014-01-22 10:15:09]

https://codereview.chromium.org/144803003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/144803003/diff/1/AUTHORS#newcode259
AUTHORS:259: Ruiyi Luo <luoruiyi2008@gmail.com>
You also need to sign the Contributor License Agreement at
https://developers.google.com/open-source/cla/individual.

https://codereview.chromium.org/144803003/diff/1/components/plugins/renderer/...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/144803003/diff/1/components/plugins/renderer/...
components/plugins/renderer/plugin_placeholder.cc:215:
plugin->updateFocus(true);
Hmm... I'm somewhat worried that this will unconditionally set the focus on the
plugin. Could we keep a |focused_| flag in WebViewPlugin instead, then transfer
that flag to the new plugin when we load it?

[Ruiyi Luo, 2014-01-24 13:18:22]

https://codereview.chromium.org/144803003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/144803003/diff/1/AUTHORS#newcode259
AUTHORS:259: Ruiyi Luo <luoruiyi2008@gmail.com>
On 2014/01/22 10:15:09, Bernhard Bauer wrote:
> You also need to sign the Contributor License Agreement at
> https://developers.google.com/open-source/cla/individual.

Done.

https://codereview.chromium.org/144803003/diff/1/components/plugins/renderer/...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/144803003/diff/1/components/plugins/renderer/...
components/plugins/renderer/plugin_placeholder.cc:215:
plugin->updateFocus(true);
Hello Bernhard, 

Thanks. I will do more debug here. 

1. At "Click to play" mode, the first time when we click the Plugin container(No
plugin loaded), it will set everything as the plugin has been loaded, except the
WebPluginContainerImpl::setFocus(true), where no plugin instance, and no
inherited function is available, thus noting was set there. If this loadPlugin()
will not only called only by situation "Click to play", I think this solution is
acceptable, at least, we put focus into the plugin container. And we know, for
"Run automatically", the plugin will be created elsewhere when the page is
created. 

2.  After the plugin is loaded, however, every time we clicked, the focused
element and the oldFocusedElement will be the same. 
FocusController.cpp line:626. Or else, we could reset the focused element in the
document to NULL after the plugin is loaded.
 
3. WebViewPlugin seems not involved here. 

Any more suggestion?

[Bernhard Bauer, 2014-01-24 13:37:52]

https://codereview.chromium.org/144803003/diff/1/components/plugins/renderer/...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/144803003/diff/1/components/plugins/renderer/...
components/plugins/renderer/plugin_placeholder.cc:215:
plugin->updateFocus(true);
On 2014/01/24 13:18:22, Ruiyi.Luo wrote:
> Hello Bernhard, 
> 
> Thanks. I will do more debug here. 
> 
> 1. At "Click to play" mode, the first time when we click the Plugin
container(No
> plugin loaded), it will set everything as the plugin has been loaded, except
the
> WebPluginContainerImpl::setFocus(true), where no plugin instance, and no
> inherited function is available, thus noting was set there.

Well, WebPluginContainerImpl will call WebPlugin::updateFocus(). While we are
showing a placeholder, the plugin will be a WebViewPlugin, where updateFocus()
is implemented with an empty body.

What I'm suggesting is that we implement WebViewPlugin::updateFocus() to store
the focused flag, and then transfer that flag to the real plugin when we load
it. If the WebViewPlugin gets focus, this should have the exact same effect as
the current patch.

> If this loadPlugin()
> will not only called only by situation "Click to play", I think this solution
is
> acceptable, at least, we put focus into the plugin container. And we know, for
> "Run automatically", the plugin will be created elsewhere when the page is
> created. 

I'm not exactly sure I understand what you are saying here, but there are other
ways that blocked plugins can be run: via the context menu, and via the blocked
plugin icon in the omnibox. I don't think the context menu would give focus to
the plugin (only a left click does), and the blocked icon bubble is outside of
the content area anyway.

> 2.  After the plugin is loaded, however, every time we clicked, the focused
> element and the oldFocusedElement will be the same. 
> FocusController.cpp line:626. 

Right, this is correct, because from the point of view of the web page, nothing
should change if we load the real plugin. 

> Or else, we could reset the focused element in the
> document to NULL after the plugin is loaded.
>  
> 3. WebViewPlugin seems not involved here. 

WebViewPlugin is the implementation of WebPlugin that we use initially to draw
the placeholder.

> Any more suggestion?

[Ruiyi Luo, 2014-01-25 05:00:25]

Great Bernhard, Your answers are exactly what I want.
I have update the patch with your suggestion. Please review it, thanks a lot.

[Bernhard Bauer, 2014-01-25 14:36:37]

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:87: // We need to transfer the
|focused_| to new plugin after it loaded.
Move this down. After ReplayReceivedData() seems to be a good point again.

Hm, now that I think about it, it might make sense to merge this with
ReplayReceivedData() into a single method on WebViewPlugin that transfers the
state to the new plugin. If you want to do that, that would be nice, but it's
fine if you leave that particular yak-shaving to me :)

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
components/plugins/renderer/webview_plugin.cc:154: void
WebViewPlugin::updateFocus(bool focused){
Nit: space before {

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
components/plugins/renderer/webview_plugin.cc:158: bool
WebViewPlugin::hasFocus(){
Here too.

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
File components/plugins/renderer/webview_plugin.h (right):

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
components/plugins/renderer/webview_plugin.h:89: virtual bool hasFocus();
This method doesn't override a method from WebPlugin, so it doesn't need to be
virtual, and should be named in the chromium style (HasFocus()) and moved
outside of the block of WebPlugin methods. After ReplayReceivedData seems like a
good place for it.

[Ruiyi Luo, 2014-01-26 09:34:18]

I have updated it with merging it into ReplayReceivedData(), also removing the
HasFocus(). 
This is my first time involved into Chromium. Thank you for your patience and
kindly support.

-Ruiyi

[Ruiyi Luo, 2014-01-26 09:40:28]

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:87: // We need to transfer the
|focused_| to new plugin after it loaded.
Agree with you. 
Merging it with ReplayReceivedData() is more reasonable and cleaner.

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
components/plugins/renderer/webview_plugin.cc:154: void
WebViewPlugin::updateFocus(bool focused){
On 2014/01/25 14:36:37, Bernhard Bauer wrote:
> Nit: space before {

Done.

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
File components/plugins/renderer/webview_plugin.h (right):

https://codereview.chromium.org/144803003/diff/90001/components/plugins/rende...
components/plugins/renderer/webview_plugin.h:89: virtual bool hasFocus();
Remove this function as no need after merging.

[Bernhard Bauer, 2014-01-26 18:38:42]

LGTM, thanks for putting up with my nits!

[Ruiyi Luo, 2014-01-27 07:25:47]

Hi Bernhard, it still need you commit it for me as I am not a committer. Thanks
a lot.

[Bernhard Bauer, 2014-01-27 09:04:46]

On 2014/01/27 07:25:47, Ruiyi.Luo wrote:
> Hi Bernhard, it still need you commit it for me as I am not a committer.
Thanks
> a lot.

You can commit yourself via the commit queue; just check the "Commit" button and
then have some patience :)

Also, description nit:
* Usually the BUG= line goes after the free text.
* The "contributed by" line is unnecessary if you use the commit queue, as it
will automatically attribute the patch to you.

[Ruiyi Luo, 2014-02-14 03:16:47]

The CQ bit was checked by luoruiyi2008@gmail.com

[commit-bot: I haz the power, 2014-02-14 03:18:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/luoruiyi2008@gmail.com/144803003/180001

[commit-bot: I haz the power, 2014-02-14 14:09:48]

Change committed as 251303