Set plugin focus via element dispatch

Update Document handling of Widget/FrameViewBase focus.

Out of all FrameViewBase types, only plugin implemented the setFocus method, so Document checks if element is HTMLPlugInElement and calls plugin::setFocus directly.

Added tests to ensure that if JS onfocus/onblur code changes element focus, the plugin stays in a consistent state of whether it thinks it is in focus.

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

[joelhockey, 2017-04-05 07:28:20]

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

[commit-bot: I haz the power, 2017-04-05 07:28: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-05 07:33:33]

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

[commit-bot: I haz the power, 2017-04-05 07:33:48]

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-05 07:36:04]

Description was changed from

==========
[TEST] set plugin focus via element dispatch.

BUG=
==========

to

==========
Set plugin focus via element dispatch.

Document has some ugly Widget/FrameViewBase code to call setFocused.  The
setFocused call is only implemented by plugin out of the possible FrameViewBase
types FrameView, PluginView and Scrollbar.

This code can be removed and HTMLPlugInElement can implement dispatchBlurEvent
and dispatchFocus event to send the setFocused calls to plugin.

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

[joelhockey, 2017-04-05 07:37:22]

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

[joelhockey, 2017-04-05 07:37:22]

This change doesn't break anything in the fast layout tests, but I'm waiting to
see that nothing else breaks.

I think this is a big improvement to simplify the code in Document so that it
has no relationship at all to FrameViewBase.

[dcheng, 2017-04-05 07:48:09]

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4148: view()->setFocused(true,
params.type);
I think we can delete setFocused() off FrameViewBase as well, right?

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

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
plugin->setFocused(false, type);
One potential subtlety that I'm not sure if we need to watch out for: I wonder
if it's possible for this to synchronously dispatch an event in the plugin,
which will run script, which will change the focus.

Elsewhere in setFocusedElement(), we bail out if the focus is changed in the
process of dispatching handlers: do we need that early bail out logic here? Is
it even possible to implement at this level?

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.h:49: virtual void
dispatchBlurEvent(Element*,
Nit: mark this as override rather than virtual (ditto below)

We have a clang plugin to enforce these checks, but it's not yet enabled in
Blink (though hopefully will be soon)

[commit-bot: I haz the power, 2017-04-05 09:03:29]

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

[commit-bot: I haz the power, 2017-04-05 09:03:29]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-05 11:59:00]

This is a great CL!

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4141: updateStyleAndLayout();

Is it okay to remove updateStyleAndLayout()?

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

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
plugin->setFocused(false, type);
On 2017/04/05 07:48:08, dcheng wrote:
> One potential subtlety that I'm not sure if we need to watch out for: I wonder
> if it's possible for this to synchronously dispatch an event in the plugin,
> which will run script, which will change the focus.
> 
> Elsewhere in setFocusedElement(), we bail out if the focus is changed in the
> process of dispatching handlers: do we need that early bail out logic here? Is
> it even possible to implement at this level?

Would it be crazy to call Element::dispatchBlurEvent and then setFocused?

[dcheng, 2017-04-05 21:14:44]

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

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
plugin->setFocused(false, type);
On 2017/04/05 11:59:00, haraken wrote:
> On 2017/04/05 07:48:08, dcheng wrote:
> > One potential subtlety that I'm not sure if we need to watch out for: I
wonder
> > if it's possible for this to synchronously dispatch an event in the plugin,
> > which will run script, which will change the focus.
> > 
> > Elsewhere in setFocusedElement(), we bail out if the focus is changed in the
> > process of dispatching handlers: do we need that early bail out logic here?
Is
> > it even possible to implement at this level?
> 
> Would it be crazy to call Element::dispatchBlurEvent and then setFocused?
> 

Either way, I think we may need to do a focus check to make sure focus state
didn't change, no matter which order we dispatch these things in.

[joelhockey, 2017-04-05 23:59:11]

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4141: updateStyleAndLayout();
On 2017/04/05 at 11:59:00, haraken wrote:
> Is it okay to remove updateStyleAndLayout()?

I don't really know, but I can say that all the tests still pass.  I can't think
how moving focus in or out of the plugin requires a style update.  This code is
from 2006 and I suspect it isn't needed, but no-one has been game to touch it?

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4148: view()->setFocused(true,
params.type);
On 2017/04/05 at 07:48:08, dcheng wrote:
> I think we can delete setFocused() off FrameViewBase as well, right?

Yes, done

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

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
plugin->setFocused(false, type);
On 2017/04/05 at 21:14:44, dcheng wrote:
> On 2017/04/05 11:59:00, haraken wrote:
> > On 2017/04/05 07:48:08, dcheng wrote:
> > > One potential subtlety that I'm not sure if we need to watch out for: I
wonder
> > > if it's possible for this to synchronously dispatch an event in the
plugin,
> > > which will run script, which will change the focus.
> > > 
> > > Elsewhere in setFocusedElement(), we bail out if the focus is changed in
the
> > > process of dispatching handlers: do we need that early bail out logic
here? Is
> > > it even possible to implement at this level?
> > 
> > Would it be crazy to call Element::dispatchBlurEvent and then setFocused?
> > 
> 
> Either way, I think we may need to do a focus check to make sure focus state
didn't change, no matter which order we dispatch these things in.

I think this change will fix any potential bugs relating to the scenario that
you described.  Previously, the setFocus on plugins was done after the dispatch
events, and there were no checks about whether calling the plugin caused a
change of focus.  Now, the handling of plugins is the same as other elements. 
So as you say, if calling blur on the plugin causes the plugin to change the
focus, then the existing code in Document::setFocusedElement method catches it. 
See check that m_focusedElement gets set by dispatchBlurEvent.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...

Likewise there is a check for (m_focusedElement != newFocusedElement) after the
call to dispatchFocusEvent that now comes into play for plugins.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.h:49: virtual void
dispatchBlurEvent(Element*,
On 2017/04/05 at 07:48:08, dcheng wrote:
> Nit: mark this as override rather than virtual (ditto below)
> 
> We have a clang plugin to enforce these checks, but it's not yet enabled in
Blink (though hopefully will be soon)

Thanks

[joelhockey, 2017-04-06 00:01:18]

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

[commit-bot: I haz the power, 2017-04-06 00:01:53]

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-06 01:39:16]

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

[commit-bot: I haz the power, 2017-04-06 01:39:17]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-06 02:39:52]

On 2017/04/05 23:59:11, joelhockey wrote:
>
https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (left):
> 
>
https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:4141: updateStyleAndLayout();
> On 2017/04/05 at 11:59:00, haraken wrote:
> > Is it okay to remove updateStyleAndLayout()?
> 
> I don't really know, but I can say that all the tests still pass.  I can't
think
> how moving focus in or out of the plugin requires a style update.  This code
is
> from 2006 and I suspect it isn't needed, but no-one has been game to touch it?
> 
>
https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:4148: view()->setFocused(true,
> params.type);
> On 2017/04/05 at 07:48:08, dcheng wrote:
> > I think we can delete setFocused() off FrameViewBase as well, right?
> 
> Yes, done
> 
>
https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right):
> 
>
https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
> plugin->setFocused(false, type);
> On 2017/04/05 at 21:14:44, dcheng wrote:
> > On 2017/04/05 11:59:00, haraken wrote:
> > > On 2017/04/05 07:48:08, dcheng wrote:
> > > > One potential subtlety that I'm not sure if we need to watch out for: I
> wonder
> > > > if it's possible for this to synchronously dispatch an event in the
> plugin,
> > > > which will run script, which will change the focus.
> > > > 
> > > > Elsewhere in setFocusedElement(), we bail out if the focus is changed in
> the
> > > > process of dispatching handlers: do we need that early bail out logic
> here? Is
> > > > it even possible to implement at this level?
> > > 
> > > Would it be crazy to call Element::dispatchBlurEvent and then setFocused?
> > > 
> > 
> > Either way, I think we may need to do a focus check to make sure focus state
> didn't change, no matter which order we dispatch these things in.
> 
> I think this change will fix any potential bugs relating to the scenario that
> you described.  Previously, the setFocus on plugins was done after the
dispatch
> events, and there were no checks about whether calling the plugin caused a
> change of focus.  Now, the handling of plugins is the same as other elements. 
> So as you say, if calling blur on the plugin causes the plugin to change the
> focus, then the existing code in Document::setFocusedElement method catches
it. 
> See check that m_focusedElement gets set by dispatchBlurEvent.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
> 
> Likewise there is a check for (m_focusedElement != newFocusedElement) after
the
> call to dispatchFocusEvent that now comes into play for plugins.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
> 
>
https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right):
> 
>
https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLPlugInElement.h:49: virtual void
> dispatchBlurEvent(Element*,
> On 2017/04/05 at 07:48:08, dcheng wrote:
> > Nit: mark this as override rather than virtual (ditto below)
> > 
> > We have a clang plugin to enforce these checks, but it's not yet enabled in
> Blink (though hopefully will be soon)
> 
> Thanks

All of your analysis sounds reasonable to me. LGTM.

[dcheng, 2017-04-06 03:01:33]

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

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
plugin->setFocused(false, type);
On 2017/04/05 23:59:11, joelhockey wrote:
> On 2017/04/05 at 21:14:44, dcheng wrote:
> > On 2017/04/05 11:59:00, haraken wrote:
> > > On 2017/04/05 07:48:08, dcheng wrote:
> > > > One potential subtlety that I'm not sure if we need to watch out for: I
> wonder
> > > > if it's possible for this to synchronously dispatch an event in the
> plugin,
> > > > which will run script, which will change the focus.
> > > > 
> > > > Elsewhere in setFocusedElement(), we bail out if the focus is changed in
> the
> > > > process of dispatching handlers: do we need that early bail out logic
> here? Is
> > > > it even possible to implement at this level?
> > > 
> > > Would it be crazy to call Element::dispatchBlurEvent and then setFocused?
> > > 
> > 
> > Either way, I think we may need to do a focus check to make sure focus state
> didn't change, no matter which order we dispatch these things in.
> 
> I think this change will fix any potential bugs relating to the scenario that
> you described.  Previously, the setFocus on plugins was done after the
dispatch
> events, and there were no checks about whether calling the plugin caused a
> change of focus.  Now, the handling of plugins is the same as other elements. 
> So as you say, if calling blur on the plugin causes the plugin to change the
> focus, then the existing code in Document::setFocusedElement method catches
it. 
> See check that m_focusedElement gets set by dispatchBlurEvent.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
> 
> Likewise there is a check for (m_focusedElement != newFocusedElement) after
the
> call to dispatchFocusEvent that now comes into play for plugins.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...

I guess my concern mostly regards the short-circuiting behavior: we didn't do
this for plugins previously, but it happened at the very end, after we
dispatched all the other JS events.

Now plugin event dispatch happens before the focus/blur event dispatched by the
subclass, and we don't check before calling through to the base class. I think
we probably should be?

[joelhockey, 2017-04-06 06:11:00]

I've had some chat discussion with dcheng and agreement was to change order of
calling dom onblur/onfocus events first, and then calling plugin.

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

https://codereview.chromium.org/2797943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
plugin->setFocused(false, type);
On 2017/04/05 at 21:14:44, dcheng wrote:
> On 2017/04/05 11:59:00, haraken wrote:
> > On 2017/04/05 07:48:08, dcheng wrote:
> > > One potential subtlety that I'm not sure if we need to watch out for: I
wonder
> > > if it's possible for this to synchronously dispatch an event in the
plugin,
> > > which will run script, which will change the focus.
> > > 
> > > Elsewhere in setFocusedElement(), we bail out if the focus is changed in
the
> > > process of dispatching handlers: do we need that early bail out logic
here? Is
> > > it even possible to implement at this level?
> > 
> > Would it be crazy to call Element::dispatchBlurEvent and then setFocused?
> > 
> 
> Either way, I think we may need to do a focus check to make sure focus state
didn't change, no matter which order we dispatch these things in.

I have updated to call the dom onblur/onfocus first, and then call the plugin.

[joelhockey, 2017-04-06 06:11:07]

The CQ bit was checked by joelhockey@chromium.org

[joelhockey, 2017-04-06 06:11:07]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2797943002/#ps60001
(title: "Dispatch to DOM onblur/onfocus first, then call plugin")

[commit-bot: I haz the power, 2017-04-06 06:17:24]

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

[joelhockey, 2017-04-06 06:19:09]

On 2017/04/05 at 11:59:00, haraken wrote:
> This is a great CL!

Thanks for the encouragement Kentaro.  This work isn't exactly going fast, but
I'm enjoying it as I start to understand more.

[haraken, 2017-04-06 06:20:27]

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

https://codereview.chromium.org/2797943002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
Element::dispatchBlurEvent(newFocusedElement, type, sourceCapabilities);

Can we add a comment on why we want to call Element::dispatchBlurEvent first?

[joelhockey, 2017-04-06 06:28:39]

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

https://codereview.chromium.org/2797943002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
Element::dispatchBlurEvent(newFocusedElement, type, sourceCapabilities);
On 2017/04/06 at 06:20:26, haraken wrote:
> Can we add a comment on why we want to call Element::dispatchBlurEvent first?

Done.  I hope my explanation is reasonable.  Feel free to suggest something
else.

[dcheng, 2017-04-06 06:28:50]

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

https://codereview.chromium.org/2797943002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
Element::dispatchBlurEvent(newFocusedElement, type, sourceCapabilities);
Thinking about this more, either ordering makes me nervous.

1) If we do the plugin first, and the plugin changes focus, we may
double-dispatch this blur event.

2) However, if we dispatch the DOM event first and the DOM event refocuses this
plugin, we will re-enter Document::setFocusedElement. I think this means we can
end up with plugin focus state in an inconsistent state: setFocused() will be
false on the plugin, since this runs after the re-entrant focus change.

In my mind, the safer alternative is to just have Document::setFocusedElement()
call this directly on the plugin (by checking if the focused element is a plugin
element and downcasting if so). What do you think of just doing this and not
using the override? This also lets us keep the original ordering of events.

[joelhockey, 2017-04-06 06:29:41]

The CQ bit was checked by joelhockey@chromium.org

[joelhockey, 2017-04-06 06:29:42]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2797943002/#ps80001
(title: "Add comment for why call dom first, then plugin")

[joelhockey, 2017-04-06 06:30:37]

The CQ bit was unchecked by joelhockey@chromium.org

[joelhockey, 2017-04-07 06:56:05]

The code is now casting element to HTMLPlugInElement and calling
plugin()->setFocused which matches previous behaviour.

I have added tests to check for when js attached to a plugin element changes
focus during blur or focus events.  These ensure that the plugin focus state
matches the dom state.

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

https://codereview.chromium.org/2797943002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:104:
Element::dispatchBlurEvent(newFocusedElement, type, sourceCapabilities);
On 2017/04/06 at 06:28:49, dcheng wrote:
> Thinking about this more, either ordering makes me nervous.
> 
> 1) If we do the plugin first, and the plugin changes focus, we may
double-dispatch this blur event.
> 
> 2) However, if we dispatch the DOM event first and the DOM event refocuses
this plugin, we will re-enter Document::setFocusedElement. I think this means we
can end up with plugin focus state in an inconsistent state: setFocused() will
be false on the plugin, since this runs after the re-entrant focus change.
> 
> In my mind, the safer alternative is to just have
Document::setFocusedElement() call this directly on the plugin (by checking if
the focused element is a plugin element and downcasting if so). What do you
think of just doing this and not using the override? This also lets us keep the
original ordering of events.

You are right that it was possible for plugin to end up in an inconsistent
state.

[joelhockey, 2017-04-07 06:57:55]

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

[commit-bot: I haz the power, 2017-04-07 06:58:05]

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-07 07:00:33]

Description was changed from

==========
Set plugin focus via element dispatch.

Document has some ugly Widget/FrameViewBase code to call setFocused.  The
setFocused call is only implemented by plugin out of the possible FrameViewBase
types FrameView, PluginView and Scrollbar.

This code can be removed and HTMLPlugInElement can implement dispatchBlurEvent
and dispatchFocus event to send the setFocused calls to plugin.

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

to

==========
Update Document handling of Widget/FrameViewBase focus.

Out of all FrameViewBase types, only plugin implemented the setFocus method, so
Document checks if element is HTMLPlugInElement and calls plugin::setFocus
directly.

Added tests to ensure that if JS onfocus/onblur code changes element focus, the
plugin stays in a consistent state of whether it thinks it is in focus.

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

[dcheng, 2017-04-07 07:07:50]

LGTM. Thanks for all the investigations and cleaning this up.

[joelhockey, 2017-04-07 07:31:33]

The CQ bit was checked by joelhockey@chromium.org

[joelhockey, 2017-04-07 07:31:34]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2797943002/#ps100001
(title: "Set plugin focus after dispatching DOM events and making sure focus has
not changed.")

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

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-07 08:39:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491550293001240,
"parent_rev": "da9c68bb0ee3cb79b68a493ad080aa43262865e5", "commit_rev":
"7a66ab96e524e57f6717edb13f375d50f9cd89e8"}

[commit-bot: I haz the power, 2017-04-07 08:39:53]

Description was changed from

==========
Update Document handling of Widget/FrameViewBase focus.

Out of all FrameViewBase types, only plugin implemented the setFocus method, so
Document checks if element is HTMLPlugInElement and calls plugin::setFocus
directly.

Added tests to ensure that if JS onfocus/onblur code changes element focus, the
plugin stays in a consistent state of whether it thinks it is in focus.

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

to

==========
Update Document handling of Widget/FrameViewBase focus.

Out of all FrameViewBase types, only plugin implemented the setFocus method, so
Document checks if element is HTMLPlugInElement and calls plugin::setFocus
directly.

Added tests to ensure that if JS onfocus/onblur code changes element focus, the
plugin stays in a consistent state of whether it thinks it is in focus.

BUG=637460

Review-Url: https://codereview.chromium.org/2797943002
Cr-Commit-Position: refs/heads/master@{#462810}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a66ab96e524e57f6717edb13f37...
==========

[commit-bot: I haz the power, 2017-04-07 08:39:54]

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