Set plugin focus by implementing HTMLPlugInElement::SetFocused.

Set plugin focus by implementing HTMLPlugInElement::SetFocused.

Set plugin focus in HTMLPlugInElement::SetFocused rather than an
explicit check and call in Document::SetFocusedElement.
This causes plugins to have DidFocusChange executed twice
(true/false), if the DOM plugin element changes focus in
onblur or onfocus, but the plugin does remain in a consistent
state as to whether it has focus or not.

BUG=637460
Review-Url: https://codereview.chromium.org/2810593002
Cr-Commit-Position: refs/heads/master@{#463917}
Committed: https://chromium.googlesource.com/chromium/src/+/e7aa842423521bcc232e2f0c3070196038f92f25

[joelhockey, 2017-04-10 02:23:13]

Description was changed from

==========
Set plugin focus by implementing HTMLPlugInElement::SetFocused.

Set plugin focus in HTMLPlugInElement::SetFocused rather than an
explicit check and call in Document::SetFocusedElement.
This causes plugins to have DidFocusChange executed twice
(false/true), if the DOM plugin element changes focus in
onblur or onfocus, but the plugin does remain in a consistent
state as to whether it has focus or not.

BUG=637460
==========

to

==========
Set plugin focus by implementing HTMLPlugInElement::SetFocused.

Set plugin focus in HTMLPlugInElement::SetFocused rather than an
explicit check and call in Document::SetFocusedElement.
This causes plugins to have DidFocusChange executed twice
(true/false), if the DOM plugin element changes focus in
onblur or onfocus, but the plugin does remain in a consistent
state as to whether it has focus or not.

BUG=637460
==========

[joelhockey, 2017-04-10 02:29:42]

joelhockey@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[joelhockey, 2017-04-10 02:29:42]

It has occurred to me that HTMLPlugInElement can also set the plugin focus by
hooking into Node::SetFocused.

I didn't pick the option before since this version of SetFocus does not include
the WebFocusType.  However, I see now that we never end up passing that into the
plugin in any case.

This change does mean that we can end up setting focus on the plugin and then
removing it when we otherwise wouldn't when the DOM element changes focus during
onblur/onfocus, but it does always keep the plugin in a consistent state.

dcheng, if you don't like this change, or just prefer to keep the code as-is
that is fine by me.

[haraken, 2017-04-10 03:52:21]

(I'll defer this review to dcheng@.)

[joelhockey, 2017-04-10 07:43:11]

The CQ bit was checked by joelhockey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 07:43:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 09:05:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 09:05:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joelhockey, 2017-04-10 11:27:10]

The CQ bit was checked by joelhockey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 11:27:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 13:03:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 13:03:14]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-11 05:36:55]

LGTM

(Followup is OK for the comment)

https://codereview.chromium.org/2810593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/2810593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:162:
plugin_->SetFocused(focused, kWebFocusTypeNone);
Can we remove the second argument?

[dcheng, 2017-04-11 05:37:19]

To elaborate a bit, I feel this is OK because there are other SetFocused()
things that appear to run script--so it seems like we must handle this case
correctly already.

[joelhockey, 2017-04-11 07:08:23]

I've tried removing WebFocusType from the various plugin related focus methods:
components/plugins/renderer/webview_plugin::UpdateFocus
content/renderer/browser_plugin/browser_plugin::UpdateFocus
content/renderer/pepper/pepper_webplugin_impl::UpdateFocus
content/shell/test_runner/test_plugin::UpdateFocus
public/web/WebPlugin::UpdateFocus

It makes no difference to PepperWebPlugin since it never passes on the
WebFocusType
https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_webplugin...
But it looks like other code might rely on this?


Another possible change is for Node::SetFocused to also take WebFocusType. 
There are not many overrides I would need to change.  I'll do another CL with
that solution right now.

https://codereview.chromium.org/2810593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/2810593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:162:
plugin_->SetFocused(focused, kWebFocusTypeNone);
On 2017/04/11 at 05:36:55, dcheng wrote:
> Can we remove the second argument?

Done, also removed WebFocusType from the trail of focus methods used by plugins.
 I'm not sure this is a good idea though.

[dcheng, 2017-04-11 07:29:33]

Ah... I think we need to plumb through the focus type for now then =/

There's always one thing...

[joelhockey, 2017-04-11 08:52:07]

The CQ bit was checked by joelhockey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 08:52:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joelhockey, 2017-04-11 08:55:14]

The CQ bit was checked by joelhockey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 08:55:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joelhockey, 2017-04-11 08:56:49]

On 2017/04/11 at 07:29:33, dcheng wrote:
> Ah... I think we need to plumb through the focus type for now then =/
> 
> There's always one thing...

This time with Node::SetFocused to include WebFocusType.  This fix went quite
smoothly.  The only trick was FocusController where I had to explicitly use
kWebFocusTypePage, but this was an easy choice since this method also calls blur
and focus events and hard-codes these as kWebFocusTypePage.

[commit-bot: I haz the power, 2017-04-11 10:21:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 10:21:02]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-11 23:40:52]

LGTM

[joelhockey, 2017-04-12 03:18:07]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-04-12 03:18:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 03:23:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491967087124510,
"parent_rev": "185a6fa4442ecee538c9f7539c3bc7f52bb0ea36", "commit_rev":
"e7aa842423521bcc232e2f0c3070196038f92f25"}

[commit-bot: I haz the power, 2017-04-12 03:24:37]

Description was changed from

==========
Set plugin focus by implementing HTMLPlugInElement::SetFocused.

Set plugin focus in HTMLPlugInElement::SetFocused rather than an
explicit check and call in Document::SetFocusedElement.
This causes plugins to have DidFocusChange executed twice
(true/false), if the DOM plugin element changes focus in
onblur or onfocus, but the plugin does remain in a consistent
state as to whether it has focus or not.

BUG=637460
==========

to

==========
Set plugin focus by implementing HTMLPlugInElement::SetFocused.

Set plugin focus in HTMLPlugInElement::SetFocused rather than an
explicit check and call in Document::SetFocusedElement.
This causes plugins to have DidFocusChange executed twice
(true/false), if the DOM plugin element changes focus in
onblur or onfocus, but the plugin does remain in a consistent
state as to whether it has focus or not.

BUG=637460

Review-Url: https://codereview.chromium.org/2810593002
Cr-Commit-Position: refs/heads/master@{#463917}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7aa842423521bcc232e2f0c3070...
==========

[commit-bot: I haz the power, 2017-04-12 03:24:38]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e7aa842423521bcc232e2f0c3070...