Do not fallback to FocusedOrMainFrame() in FocusedLocalFrameInWidget()

Do not fallback to FocusedOrMainFrame() in FocusedLocalFrameInWidget()

For tasks such as editing and IME we use FocusedLocalFrameInWidget() which is
supposed to return the focused frame sharing the same local root as that of
the frame widget. This in WebViewImpl is slightly different as the method
could return the MainFrameImpl as the fallback value.

However, the types of tasks which should rely on this method are those that
are to be (eventually) handled by a WebFrameWidget and therefore, falling
back to MainFrameImpl does not seem like the right thing to do.

This CL will move the logic in FocusedLocalFrameInWidget() from both
WebViewImpl and WebFrameWidgetImpl into WebFrameWidgetBase and leaves a shallow
implementation in WebViewImpl that simply calls into the frame widget of the
MainFrameImpl. That assures in calling FocusedLocalFrameInWidget we never hit
FocusedOrMainFrame().

BUG=726763, 629721

[EhsanK, 2017-05-30 16:57:46]

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

[commit-bot: I haz the power, 2017-05-30 16:58:14]

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-05-30 18:17:37]

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

[commit-bot: I haz the power, 2017-05-30 18:17:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2017-05-30 20:27:31]

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

[commit-bot: I haz the power, 2017-05-30 20:28:02]

Dry run: CQ is trying da patch.

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

[EhsanK, 2017-05-30 20:31:48]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2017-05-30 20:33:34]

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

[commit-bot: I haz the power, 2017-05-30 20:34:17]

Dry run: CQ is trying da patch.

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

[EhsanK, 2017-05-30 21:55:53]

ekaramad@chromium.org changed reviewers:
+ dcheng@chromium.org

[EhsanK, 2017-05-30 21:55:54]

Daniel, PTAL.

Note:
I started making the move but during dry-run another CL
(https://codereview.chromium.org/2807433003/) landed a change to add
FocusedLocalFrameInWidget() const inside WebFrameWidgetBase which now makes too
many of it (there is on in WebFrameWidgetImpl and one in WebViewImpl).

After this I can dedupe some more IME code and hopefully remove the WebViewImpl
version of it if the callers are few.

[commit-bot: I haz the power, 2017-05-30 22:21:02]

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

[commit-bot: I haz the power, 2017-05-30 22:21:03]

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

[EhsanK, 2017-05-30 22:37:50]

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

[commit-bot: I haz the power, 2017-05-30 22:38:33]

Dry run: CQ is trying da patch.

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

[EhsanK, 2017-05-30 22:39:22]

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:2327:
web_view->SetInitialFocus(false);
Without it line 2332 returns nullptr after the change. We used to fallback to
main frame before but now that we don't, we need main frame to be focused here.

[EhsanK, 2017-05-30 22:39:23]

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:2327:
web_view->SetInitialFocus(false);
Without it line 2332 returns nullptr after the change. We used to fallback to
main frame before but now that we don't, we need main frame to be focused here.

[commit-bot: I haz the power, 2017-05-31 01:49:11]

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

[commit-bot: I haz the power, 2017-05-31 01:49:11]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-01 04:20:48]

dcheng@chromium.org changed reviewers:
+ alexmos@chromium.org

[dcheng, 2017-06-01 04:20:49]

I have a more general question as well, so adding alexmos@

In general, what behavior do we want if we make an IPC to the renderer that acts
on the focused frame, but the focused frame has already changed?

I see several approaches:
1 Drop IPC handling if the focused frame has changed. AFAIK, we don't really do
this.
2. Try to target a focused frame in the same widget. I think this is what some
of the IME code does.
3. Try to find any frame: I think most code does this, by virtue of using
FocusedOrMainFrame.

For 2 and 3, the issue is the fallback won't work if the fallback options are
remote frames, so there's inconsistent behavior. However, if we do 1, it would
be weird, because it means we would drop keyboard events if focus changed, which
would be weird...

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h (right):

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:108: // This method
returns the focused frame belonging to this WebWidget, that
NIt: colon instead of , before 'that'

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:110: // to this
widget. It will return nullptr if no frame is focused or, the
Nit: no comma after or

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:112: LocalFrame*
FocusedLocalFrameInWidget() const;
Let's remove the "InWidget" part since this is a method on WebFrameWidget?

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:113: LocalFrame*
FocusedLocalFrameAvailableForIme() const;
Also, let's add some comments that document the behavior differences between
these two.

(It might be slightly preferred to encode what this method does, rather than how
it's used)

[EhsanK, 2017-06-01 13:47:45]

Thanks! PTAL.

Some basic thoughts:
1- Hard to even do when the IPC is handled by a RenderWidget/RenderView; unless
we add a flag to the IPC to explicitly ask it to be performed on a RenderFrame?
In that case shouldn't the IPC go to RenderFrame?

2- Yes, except that we drop the IPC if there is no focused frame inside widget.
My understanding at the time was that this approach is closes to how IME used to
work in the main frame except for the fallback to the main frame part.

3- But don't we already know that the frame which was supposed to consume the
IPC was in the RenderWidget/LocalRoot which received it? So why handle it for
another frame in another local root?

I think if we don't want to drop the events such as keyboard we should just send
them to the frame they were supposed to go to. In that case it might've helped
if input would go to RenderFrame instead of RenderWidget.

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h (right):

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:108: // This method
returns the focused frame belonging to this WebWidget, that
On 2017/06/01 04:20:49, dcheng wrote:
> NIt: colon instead of , before 'that'

Done.

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:110: // to this
widget. It will return nullptr if no frame is focused or, the
On 2017/06/01 04:20:49, dcheng wrote:
> Nit: no comma after or

Done.

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:112: LocalFrame*
FocusedLocalFrameInWidget() const;
On 2017/06/01 04:20:49, dcheng wrote:
> Let's remove the "InWidget" part since this is a method on WebFrameWidget?

Acknowledged.

https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:113: LocalFrame*
FocusedLocalFrameAvailableForIme() const;
On 2017/06/01 04:20:49, dcheng wrote:
> Also, let's add some comments that document the behavior differences between
> these two.
> 
> (It might be slightly preferred to encode what this method does, rather than
how
> it's used)

Agreed. However, I took a second look at usage of this method and apparently
there was only one. I think after I moved IME logic to
WebInputMethodControllerImpl this method became kind of stale. I have removed it
now and explicitly check for |ime_accept_events_| where it used to be called.
Thanks!

[EhsanK, 2017-06-14 17:36:36]

On 2017/06/01 13:47:45, EhsanK wrote:
> Thanks! PTAL.
> 
> Some basic thoughts:
> 1- Hard to even do when the IPC is handled by a RenderWidget/RenderView;
unless
> we add a flag to the IPC to explicitly ask it to be performed on a
RenderFrame?
> In that case shouldn't the IPC go to RenderFrame?
> 
> 2- Yes, except that we drop the IPC if there is no focused frame inside
widget.
> My understanding at the time was that this approach is closes to how IME used
to
> work in the main frame except for the fallback to the main frame part.
> 
> 3- But don't we already know that the frame which was supposed to consume the
> IPC was in the RenderWidget/LocalRoot which received it? So why handle it for
> another frame in another local root?
> 
> I think if we don't want to drop the events such as keyboard we should just
send
> them to the frame they were supposed to go to. In that case it might've helped
> if input would go to RenderFrame instead of RenderWidget.
> 
>
https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h (right):
> 
>
https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:108: // This method
> returns the focused frame belonging to this WebWidget, that
> On 2017/06/01 04:20:49, dcheng wrote:
> > NIt: colon instead of , before 'that'
> 
> Done.
> 
>
https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:110: // to this
> widget. It will return nullptr if no frame is focused or, the
> On 2017/06/01 04:20:49, dcheng wrote:
> > Nit: no comma after or
> 
> Done.
> 
>
https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:112: LocalFrame*
> FocusedLocalFrameInWidget() const;
> On 2017/06/01 04:20:49, dcheng wrote:
> > Let's remove the "InWidget" part since this is a method on WebFrameWidget?
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2910233002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h:113: LocalFrame*
> FocusedLocalFrameAvailableForIme() const;
> On 2017/06/01 04:20:49, dcheng wrote:
> > Also, let's add some comments that document the behavior differences between
> > these two.
> > 
> > (It might be slightly preferred to encode what this method does, rather than
> how
> > it's used)
> 
> Agreed. However, I took a second look at usage of this method and apparently
> there was only one. I think after I moved IME logic to
> WebInputMethodControllerImpl this method became kind of stale. I have removed
it
> now and explicitly check for |ime_accept_events_| where it used to be called.
> Thanks!

A friendly ping on this CL in case everyone already migrated to gerrit and
forgot about this :).

[alexmos, 2017-06-15 02:53:06]

Sorry, this definitely slipped through the cracks, so thanks for the ping!

I'm fine with this CL, as overall it seems to make IME focus saner and less
confusing.  Some thoughts around Daniel's questions:

> In general, what behavior do we want if we make an IPC to the renderer that
acts
> on the focused frame, but the focused frame has already changed?
> 
> I see several approaches:
> 1 Drop IPC handling if the focused frame has changed. AFAIK, we don't really
do
> this.
> 2. Try to target a focused frame in the same widget. I think this is what some
> of the IME code does.
> 3. Try to find any frame: I think most code does this, by virtue of using
> FocusedOrMainFrame.
> 
> For 2 and 3, the issue is the fallback won't work if the fallback options are
> remote frames, so there's inconsistent behavior. However, if we do 1, it would
> be weird, because it means we would drop keyboard events if focus changed,
which
> would be weird...

I'm curious which races you guys are thinking about here, since the desired
behavior might be different depending on the race.  For things like a user
pressing a key just as the page changes frame focus via window.focus() or
detaching the focused frame, dropping keyboard events might not be too weird, as
it might actually prevent you from typing into something that you probably
didn't intend to type into anyway.  In such cases, I actually wonder how
terrible is it to deliver the input event to the *old* focused frame (if it
still exists)?

But a more common case seems to be the user clicks on something, and before the
browser process gets notified of a focused frame change via
FrameMsg_SetFocusedFrame, the user types something intended for the future
focused frame, but the RenderWidgetHostInputEventRouter dispatches it to the
past focused frame.  That seems more suitable for options 2 and 3 (the user
probably prefers for the event to be delivered to the future frame).  Ideally
we'd know on the browser process side how an input event will change frame focus
before forwarding it to the renderer, but that's probably really hard.

If we didn't want to drop anything, even in remote focused frame cases, we could
use SendPageMessage and then drop the IPCs in all renderers except the one with
the currently focused frame -- terribly inefficient, but we could only do it if
we somehow detect that we might be in a focused frame race (could we do that
with InputEventAcks and being clever about which kinds of events might change
focused frames?). :)

For Ehsan's question that it's kind of hard to tell if an IPC for RV or RW is
really targeting a focused frame, presumably we'll need to send over the browser
process's value of focused_frame in these IPCs, which we then can compare to the
renderer value?

https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2320: ->ime_accept_events_ = true;
Ugh, really hope we can clean this up soon.  I agree with you that it'd be nice
to not dispatch IME events without page focus in the first place, but that's
probably not for this CL.  Is there a use case for page focus in WebFrameWidgets
other than IME?  If so, we should just consider calling
WebFrameWidgetImpl::SetFocus.

[EhsanK, 2017-06-15 15:17:12]

Thank you Alex for the very detailed explanation! PTAL.

On 2017/06/15 02:53:06, alexmos wrote:
> Sorry, this definitely slipped through the cracks, so thanks for the ping!
> 
> I'm fine with this CL, as overall it seems to make IME focus saner and less
> confusing.  Some thoughts around Daniel's questions:
> 
> > In general, what behavior do we want if we make an IPC to the renderer that
> acts
> > on the focused frame, but the focused frame has already changed?
> > 
> > I see several approaches:
> > 1 Drop IPC handling if the focused frame has changed. AFAIK, we don't really
> do
> > this.
> > 2. Try to target a focused frame in the same widget. I think this is what
some
> > of the IME code does.
> > 3. Try to find any frame: I think most code does this, by virtue of using
> > FocusedOrMainFrame.
> > 
> > For 2 and 3, the issue is the fallback won't work if the fallback options
are
> > remote frames, so there's inconsistent behavior. However, if we do 1, it
would
> > be weird, because it means we would drop keyboard events if focus changed,
> which
> > would be weird...
> 
> I'm curious which races you guys are thinking about here, since the desired
> behavior might be different depending on the race.  For things like a user
> pressing a key just as the page changes frame focus via window.focus() or
> detaching the focused frame, dropping keyboard events might not be too weird,
as
> it might actually prevent you from typing into something that you probably
> didn't intend to type into anyway.
Agreed. I was under the impression that this is the right thing to do.
> In such cases, I actually wonder how
> terrible is it to deliver the input event to the *old* focused frame (if it
> still exists)?
I think dropping the input is better than sending it to the wrong frame. At
least
the user would not have to go back and delete the unwanted characters :).

> But a more common case seems to be the user clicks on something, and before
the
> browser process gets notified of a focused frame change via
> FrameMsg_SetFocusedFrame, the user types something intended for the future
> focused frame, but the RenderWidgetHostInputEventRouter dispatches it to the
> past focused frame.  That seems more suitable for options 2 and 3 (the user
> probably prefers for the event to be delivered to the future frame).  Ideally
> we'd know on the browser process side how an input event will change frame
focus
> before forwarding it to the renderer, but that's probably really hard.
This seems quite reproducible (for instance in janky renderers). I still think
it
is safer to drop input when there is a mismatch between the target defined by
the
browser and the receiving end. Currently, all we can determine is the target
RenderWidget
so we can find mismatches at the local root level.

> If we didn't want to drop anything, even in remote focused frame cases, we
could
> use SendPageMessage and then drop the IPCs in all renderers except the one
with
> the currently focused frame -- terribly inefficient, but we could only do it
if
> we somehow detect that we might be in a focused frame race (could we do that
> with InputEventAcks and being clever about which kinds of events might change
> focused frames?). :)
> 
> For Ehsan's question that it's kind of hard to tell if an IPC for RV or RW is
> really targeting a focused frame, presumably we'll need to send over the
browser
> process's value of focused_frame in these IPCs, which we then can compare to
the
> renderer value?
My suggestion was mostly about sending the IPC to the actual RenderFrameImpl as
opposed
to RenderWidget or RenderView. Alternatively send along the routing ID of the
frame?

>
https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:2320: ->ime_accept_events_ =
true;
> Ugh, really hope we can clean this up soon.  I agree with you that it'd be
nice
> to not dispatch IME events without page focus in the first place, but that's
> probably not for this CL.  Is there a use case for page focus in
WebFrameWidgets
> other than IME?  If so, we should just consider calling
> WebFrameWidgetImpl::SetFocus.

[EhsanK, 2017-06-16 19:25:14]

Sorry...forgot to send this comment.

https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2320: ->ime_accept_events_ = true;
On 2017/06/15 02:53:06, alexmos wrote:
> Ugh, really hope we can clean this up soon.  I agree with you that it'd be
nice
> to not dispatch IME events without page focus in the first place, but that's
> probably not for this CL.
Agreed. I have been meaning to get started on this soon. I will try to put up a
fix soon after this CL.
> Is there a use case for page focus in WebFrameWidgets
> other than IME?  If so, we should just consider calling
> WebFrameWidgetImpl::SetFocus.

From looking at WebFrameWidgetImpl::SetFocus(enabled)

1- if (enabled): there is some reference to FocusController().SetActive() which
is perhaps incorrect since this should be done by WebViewImpl instead. Other
than that there seems to be some frame selection upates on the focused frame.

2- if(!enabled): this is the case where we commit an ongoing IME composition.
Again dealing with the focused frame.

From 1 & 2 I actually do not see a compelling reason to keep
WebFrameWidget::SetFocus and perhaps it could live inside WebView.h instead.
However, it will be quite nice to expose WebView in RenderWidget so that when
necessary (i.e., handling IME IPCs), RenderWidget can drop IPC if WebView is not
focused.

I will try to investigate this soon.

[alexmos, 2017-06-17 01:06:43]

On 2017/06/15 15:17:12, EhsanK wrote:
> Thank you Alex for the very detailed explanation! PTAL.
> 
> On 2017/06/15 02:53:06, alexmos wrote:
> > Sorry, this definitely slipped through the cracks, so thanks for the ping!
> > 
> > I'm fine with this CL, as overall it seems to make IME focus saner and less
> > confusing.  Some thoughts around Daniel's questions:
> > 
> > > In general, what behavior do we want if we make an IPC to the renderer
that
> > acts
> > > on the focused frame, but the focused frame has already changed?
> > > 
> > > I see several approaches:
> > > 1 Drop IPC handling if the focused frame has changed. AFAIK, we don't
really
> > do
> > > this.
> > > 2. Try to target a focused frame in the same widget. I think this is what
> some
> > > of the IME code does.
> > > 3. Try to find any frame: I think most code does this, by virtue of using
> > > FocusedOrMainFrame.
> > > 
> > > For 2 and 3, the issue is the fallback won't work if the fallback options
> are
> > > remote frames, so there's inconsistent behavior. However, if we do 1, it
> would
> > > be weird, because it means we would drop keyboard events if focus changed,
> > which
> > > would be weird...
> > 
> > I'm curious which races you guys are thinking about here, since the desired
> > behavior might be different depending on the race.  For things like a user
> > pressing a key just as the page changes frame focus via window.focus() or
> > detaching the focused frame, dropping keyboard events might not be too
weird,
> as
> > it might actually prevent you from typing into something that you probably
> > didn't intend to type into anyway.
> Agreed. I was under the impression that this is the right thing to do.
> > In such cases, I actually wonder how
> > terrible is it to deliver the input event to the *old* focused frame (if it
> > still exists)?
> I think dropping the input is better than sending it to the wrong frame. At
> least
> the user would not have to go back and delete the unwanted characters :).

That's a fair point. :)

> 
> > But a more common case seems to be the user clicks on something, and before
> the
> > browser process gets notified of a focused frame change via
> > FrameMsg_SetFocusedFrame, the user types something intended for the future
> > focused frame, but the RenderWidgetHostInputEventRouter dispatches it to the
> > past focused frame.  That seems more suitable for options 2 and 3 (the user
> > probably prefers for the event to be delivered to the future frame). 
Ideally
> > we'd know on the browser process side how an input event will change frame
> focus
> > before forwarding it to the renderer, but that's probably really hard.
> This seems quite reproducible (for instance in janky renderers). I still think
> it
> is safer to drop input when there is a mismatch between the target defined by
> the
> browser and the receiving end. Currently, all we can determine is the target
> RenderWidget
> so we can find mismatches at the local root level.

Daniel and I talked about this a bit today, and this race might not be as bad as
I thought due to existing input synchronization that we already do - but I don't
really know how it works, so I'll let Daniel comment on that (he was planning to
take another look at this CL soon).  

I agree that dropping input on mismatched focused frame seems like the safest
option, and that falling back to something else is risky and might be
inconsistent with OOPIFs.  Things are already pretty bad today: I think
sometimes we fall back to main frame, but only when it's local, sometimes to
another frame within the local root, sometimes we don't fall back at all? 
Perhaps we should file a bug to ensure consistent behavior for focus races and
continue this discussion there. 

> 
> > If we didn't want to drop anything, even in remote focused frame cases, we
> could
> > use SendPageMessage and then drop the IPCs in all renderers except the one
> with
> > the currently focused frame -- terribly inefficient, but we could only do it
> if
> > we somehow detect that we might be in a focused frame race (could we do that
> > with InputEventAcks and being clever about which kinds of events might
change
> > focused frames?). :)
> > 
> > For Ehsan's question that it's kind of hard to tell if an IPC for RV or RW
is
> > really targeting a focused frame, presumably we'll need to send over the
> browser
> > process's value of focused_frame in these IPCs, which we then can compare to
> the
> > renderer value?
> My suggestion was mostly about sending the IPC to the actual RenderFrameImpl
as
> opposed
> to RenderWidget or RenderView. Alternatively send along the routing ID of the
> frame?

Yes, I was thinking about sending the routing ID of the focused frame in my
suggestion.  Sending it to RenderFrame directly seems a bit weirder, as widgets
are supposed to deal with input events, though maybe that would work too
depending on the IPC.

https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2320: ->ime_accept_events_ = true;
On 2017/06/16 19:25:14, EhsanK wrote:
> On 2017/06/15 02:53:06, alexmos wrote:
> > Ugh, really hope we can clean this up soon.  I agree with you that it'd be
> nice
> > to not dispatch IME events without page focus in the first place, but that's
> > probably not for this CL.
> Agreed. I have been meaning to get started on this soon. I will try to put up
a
> fix soon after this CL.
> > Is there a use case for page focus in WebFrameWidgets
> > other than IME?  If so, we should just consider calling
> > WebFrameWidgetImpl::SetFocus.
> 
> From looking at WebFrameWidgetImpl::SetFocus(enabled)
> 
> 1- if (enabled): there is some reference to FocusController().SetActive()
which
> is perhaps incorrect since this should be done by WebViewImpl instead. Other
> than that there seems to be some frame selection upates on the focused frame.
> 
> 2- if(!enabled): this is the case where we commit an ongoing IME composition.
> Again dealing with the focused frame.
> 
> From 1 & 2 I actually do not see a compelling reason to keep
> WebFrameWidget::SetFocus and perhaps it could live inside WebView.h instead.
> However, it will be quite nice to expose WebView in RenderWidget so that when
> necessary (i.e., handling IME IPCs), RenderWidget can drop IPC if WebView is
not
> focused.
> 
> I will try to investigate this soon.

Sounds good.  The other concern I remember with this in the past was  that this
is really about page-level focus, so it made more sense to do it once per-view
rather than multiple times for each widget.  I'll defer to Daniel about exposing
WebView.

[EhsanK, 2017-08-02 15:29:55]

On 2017/06/17 01:06:43, alexmos wrote:
> On 2017/06/15 15:17:12, EhsanK wrote:
> > Thank you Alex for the very detailed explanation! PTAL.
> > 
> > On 2017/06/15 02:53:06, alexmos wrote:
> > > Sorry, this definitely slipped through the cracks, so thanks for the ping!
> > > 
> > > I'm fine with this CL, as overall it seems to make IME focus saner and
less
> > > confusing.  Some thoughts around Daniel's questions:
> > > 
> > > > In general, what behavior do we want if we make an IPC to the renderer
> that
> > > acts
> > > > on the focused frame, but the focused frame has already changed?
> > > > 
> > > > I see several approaches:
> > > > 1 Drop IPC handling if the focused frame has changed. AFAIK, we don't
> really
> > > do
> > > > this.
> > > > 2. Try to target a focused frame in the same widget. I think this is
what
> > some
> > > > of the IME code does.
> > > > 3. Try to find any frame: I think most code does this, by virtue of
using
> > > > FocusedOrMainFrame.
> > > > 
> > > > For 2 and 3, the issue is the fallback won't work if the fallback
options
> > are
> > > > remote frames, so there's inconsistent behavior. However, if we do 1, it
> > would
> > > > be weird, because it means we would drop keyboard events if focus
changed,
> > > which
> > > > would be weird...
> > > 
> > > I'm curious which races you guys are thinking about here, since the
desired
> > > behavior might be different depending on the race.  For things like a user
> > > pressing a key just as the page changes frame focus via window.focus() or
> > > detaching the focused frame, dropping keyboard events might not be too
> weird,
> > as
> > > it might actually prevent you from typing into something that you probably
> > > didn't intend to type into anyway.
> > Agreed. I was under the impression that this is the right thing to do.
> > > In such cases, I actually wonder how
> > > terrible is it to deliver the input event to the *old* focused frame (if
it
> > > still exists)?
> > I think dropping the input is better than sending it to the wrong frame. At
> > least
> > the user would not have to go back and delete the unwanted characters :).
> 
> That's a fair point. :)
> 
> > 
> > > But a more common case seems to be the user clicks on something, and
before
> > the
> > > browser process gets notified of a focused frame change via
> > > FrameMsg_SetFocusedFrame, the user types something intended for the future
> > > focused frame, but the RenderWidgetHostInputEventRouter dispatches it to
the
> > > past focused frame.  That seems more suitable for options 2 and 3 (the
user
> > > probably prefers for the event to be delivered to the future frame). 
> Ideally
> > > we'd know on the browser process side how an input event will change frame
> > focus
> > > before forwarding it to the renderer, but that's probably really hard.
> > This seems quite reproducible (for instance in janky renderers). I still
think
> > it
> > is safer to drop input when there is a mismatch between the target defined
by
> > the
> > browser and the receiving end. Currently, all we can determine is the target
> > RenderWidget
> > so we can find mismatches at the local root level.
> 
> Daniel and I talked about this a bit today, and this race might not be as bad
as
> I thought due to existing input synchronization that we already do - but I
don't
> really know how it works, so I'll let Daniel comment on that (he was planning
to
> take another look at this CL soon).  
> 
> I agree that dropping input on mismatched focused frame seems like the safest
> option, and that falling back to something else is risky and might be
> inconsistent with OOPIFs.  Things are already pretty bad today: I think
> sometimes we fall back to main frame, but only when it's local, sometimes to
> another frame within the local root, sometimes we don't fall back at all? 
> Perhaps we should file a bug to ensure consistent behavior for focus races and
> continue this discussion there. 
> 
> > 
> > > If we didn't want to drop anything, even in remote focused frame cases, we
> > could
> > > use SendPageMessage and then drop the IPCs in all renderers except the one
> > with
> > > the currently focused frame -- terribly inefficient, but we could only do
it
> > if
> > > we somehow detect that we might be in a focused frame race (could we do
that
> > > with InputEventAcks and being clever about which kinds of events might
> change
> > > focused frames?). :)
> > > 
> > > For Ehsan's question that it's kind of hard to tell if an IPC for RV or RW
> is
> > > really targeting a focused frame, presumably we'll need to send over the
> > browser
> > > process's value of focused_frame in these IPCs, which we then can compare
to
> > the
> > > renderer value?
> > My suggestion was mostly about sending the IPC to the actual RenderFrameImpl
> as
> > opposed
> > to RenderWidget or RenderView. Alternatively send along the routing ID of
the
> > frame?
> 
> Yes, I was thinking about sending the routing ID of the focused frame in my
> suggestion.  Sending it to RenderFrame directly seems a bit weirder, as
widgets
> are supposed to deal with input events, though maybe that would work too
> depending on the IPC.
> 
>
https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2910233002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:2320: ->ime_accept_events_ =
true;
> On 2017/06/16 19:25:14, EhsanK wrote:
> > On 2017/06/15 02:53:06, alexmos wrote:
> > > Ugh, really hope we can clean this up soon.  I agree with you that it'd be
> > nice
> > > to not dispatch IME events without page focus in the first place, but
that's
> > > probably not for this CL.
> > Agreed. I have been meaning to get started on this soon. I will try to put
up
> a
> > fix soon after this CL.
> > > Is there a use case for page focus in WebFrameWidgets
> > > other than IME?  If so, we should just consider calling
> > > WebFrameWidgetImpl::SetFocus.
> > 
> > From looking at WebFrameWidgetImpl::SetFocus(enabled)
> > 
> > 1- if (enabled): there is some reference to FocusController().SetActive()
> which
> > is perhaps incorrect since this should be done by WebViewImpl instead. Other
> > than that there seems to be some frame selection upates on the focused
frame.
> > 
> > 2- if(!enabled): this is the case where we commit an ongoing IME
composition.
> > Again dealing with the focused frame.
> > 
> > From 1 & 2 I actually do not see a compelling reason to keep
> > WebFrameWidget::SetFocus and perhaps it could live inside WebView.h instead.
> > However, it will be quite nice to expose WebView in RenderWidget so that
when
> > necessary (i.e., handling IME IPCs), RenderWidget can drop IPC if WebView is
> not
> > focused.
> > 
> > I will try to investigate this soon.
> 
> Sounds good.  The other concern I remember with this in the past was  that
this
> is really about page-level focus, so it made more sense to do it once per-view
> rather than multiple times for each widget.  I'll defer to Daniel about
exposing
> WebView.

This CL is quite outdated. I will redo/rebase thi and kick of reviews on gerrit.
Thanks!