Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
898443003:
Use RWH instead of RWHV to know whether a frame is focus. (Closed)
|
Created:
5 years, 10 months ago by mlamouri (slow - plz ping) Modified:
5 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse RWH instead of RWHV to know whether a frame is focus.
Using RWH is a pure reflection of what the renderer process knows while
RWHV is linked to the native implementations.
This is fixing the LayoutTest failures created by:
https://codereview.chromium.org/871013003
It might also be the cause of the test flakyness. Another CL will
attempt to re-enable them (can't reproduce locally).
BUG=450634
Committed: https://crrev.com/2ff3b31d405dbcf301972f3dc5833c40674042b8
Cr-Commit-Position: refs/heads/master@{#314590}
Patch Set 1 #
Total comments: 7
Patch Set 2 : add comments #
Total comments: 1
Patch Set 3 : comments and TODO #
Total comments: 20
Patch Set 4 : review comments #
Messages
Total messages: 28 (7 generated)
mlamouri@chromium.org changed reviewers: + creis@chromium.org, kenrb@chromium.org
lgtm
mlamouri@chromium.org changed reviewers: + avi@chromium.org
Avi or Charlie, does one of you feel like rubber stamping that? :)
lgtm stampity stamp
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/898443003/1
Please wait. Comments below. https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_impl.cc:1598: GetView()->GetRenderWidgetHost())->is_focused() && This is pretty awkward. Ken, this is the kind of thing I was fearing due to RFHI::GetRenderWidgetHost() returning nullptr in some cases. Can we just fix that method and use it here? https://codereview.chromium.org/898443003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/898443003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.h:847: bool is_focused_; It's getting hard to track why we need this here. Please put a comment saying why it's need in both RWHI and RWHV, and when (if ever) they differ.
The CQ bit was unchecked by creis@chromium.org
Elaborating a bit, now that it's out of the CQ: I'm finding it quite difficult to tell where focus information is supposed to live, after we went back and forth on https://codereview.chromium.org/860393004/. It's important to give future developers the right context to know which one to use. Also, Ken and I had been disagreeing about the GetRenderWidgetHost behavior, and it looks like we should make it behave well for cases like this. Sorry for the extra requests.
https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_impl.cc:1598: GetView()->GetRenderWidgetHost())->is_focused() && On 2015/02/02 at 18:03:55, Charlie Reis wrote: > This is pretty awkward. Ken, this is the kind of thing I was fearing due to RFHI::GetRenderWidgetHost() returning nullptr in some cases. Can we just fix that method and use it here? Yes, GetRenderWidgetHost() doesn't behave like I would expect and Ken, for the other patches said that doing this was fine. I would be okay in changing that call but it's already being used in other places. I would gladly add a TODO and point to a bug if we consider changing that. https://codereview.chromium.org/898443003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/898443003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.h:847: bool is_focused_; On 2015/02/02 at 18:03:55, Charlie Reis wrote: > It's getting hard to track why we need this here. Please put a comment saying why it's need in both RWHI and RWHV, and when (if ever) they differ. Basically, RWH focus isn't linked to the system while RWHV is. When a frame gets focused, it will inform the RWH (ViewHostMsg_{Focus,Blur}) so if we want a 1:1 relationship between what the renderer process think and what the browser process knows, it's better to use that information. Regarding situations where it differs, it happened in LayoutTests where a frame was expected to be focused but wasn't according to the browser process.
I've added comments explaining what |is_focused_| is for. creis@, PTAL.
https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_impl.cc:1598: GetView()->GetRenderWidgetHost())->is_focused() && On 2015/02/02 18:03:55, Charlie Reis wrote: > This is pretty awkward. Ken, this is the kind of thing I was fearing due to > RFHI::GetRenderWidgetHost() returning nullptr in some cases. Can we just fix > that method and use it here? I agree it is awkward. I am okay with it because I think it should be an exceptional case. I am somewhat suspicious of anything that needs to get an ancestral RenderWidgetHost from the RenderFrameHost. I believe you probably want the RenderWidgetHostView in that case, which is why he checked focus state on the view in the earlier patch. I talked to Mounir about this and he says it fails tests, but he isn't exactly clear on the circumstances in which the RenderWidgetHost and RenderWidgetHostView are reporting different values for is_focused()/HasFocus(), but since the RWH seems to behave more reliably, this change was needed.
Sorry, given Ken's comment I can't approve this yet. Let's understand why the RWH and RWHV cases differ more specifically, so that we end up with APIs that we can reason about. https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_impl.cc:1598: GetView()->GetRenderWidgetHost())->is_focused() && On 2015/02/02 18:21:15, Mounir Lamouri wrote: > On 2015/02/02 at 18:03:55, Charlie Reis wrote: > > This is pretty awkward. Ken, this is the kind of thing I was fearing due to > RFHI::GetRenderWidgetHost() returning nullptr in some cases. Can we just fix > that method and use it here? > > Yes, GetRenderWidgetHost() doesn't behave like I would expect and Ken, for the > other patches said that doing this was fine. I would be okay in changing that > call but it's already being used in other places. I would gladly add a TODO and > point to a bug if we consider changing that. Ok, please file a bug and add a TODO. I think this is very important for the RenderFrameHostImpl API, but it doesn't have to get fixed in this CL. https://codereview.chromium.org/898443003/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_impl.cc:1598: GetView()->GetRenderWidgetHost())->is_focused() && On 2015/02/02 18:28:17, kenrb wrote: > On 2015/02/02 18:03:55, Charlie Reis wrote: > > This is pretty awkward. Ken, this is the kind of thing I was fearing due to > > RFHI::GetRenderWidgetHost() returning nullptr in some cases. Can we just fix > > that method and use it here? > > I agree it is awkward. I am okay with it because I think it should be an > exceptional case. > > I am somewhat suspicious of anything that needs to get an ancestral > RenderWidgetHost from the RenderFrameHost. I believe you probably want the > RenderWidgetHostView in that case, which is why he checked focus state on the > view in the earlier patch. I talked to Mounir about this and he says it fails > tests, but he isn't exactly clear on the circumstances in which the > RenderWidgetHost and RenderWidgetHostView are reporting different values for > is_focused()/HasFocus(), but since the RWH seems to behave more reliably, this > change was needed. This is exactly what's bothering me about this CL. We don't seem to understand why these cases differ, and why this method is needed. I'm inclined to not approve this until then. https://codereview.chromium.org/898443003/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/898443003/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:849: // in that regard. Please be more specific here: when is different from RWHV?
For what I can see, the reason why the values differ is because RWHVMac doesn't return |true| to HasFocus() when we expect it to. The reason is because makeFirstResponder [1] isn't guaranteed to succeed. The other implementations are also using system specific checks. Given that we are using Aura on Desktop outside of Mac, I'm not surprised that Mac is the only outlier here. I think we have two solutions here: - we want a IsFocused() method that really reflect what the content knows (in which case, it's not aware of what the system is doing and is just calling .focus() and assume it's now focused) or; - we want IsFocused() to reflect something else and then we disable the test on Mac because in some situations, a focused frame for the renderer isn't really focused. [1] https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica...:
kenrb@chromium.org changed reviewers: + piman@chromium.org
piman@: Can you take a look at what is happening here? We are working on tracking tab-level focus via RenderWidgetHosts but RenderWidgetHostView::HasFocus() doesn't seem adequate for reasons described above.
As far as I can tell (but I don't know much about focus), the content state should match the RWHV state (modulo IPC), the RWHV tells the RWH about focus when it receives the event from Cocoa/Aura after a focus change - that is whether RWHV::Focus did it or not. That part seems to be true on both Mac and Aura. On mac-only there is some extra logic, namely RWHV::SetActive, that seems to force Blur on the RWH even though the NSView is (still) the first responder (i.e. RWHV::HasFocus() will return true). I suppose that's the cause of the discrepancy. I don't know if it's by design, or if SetActive(false) should also blur the NSView and restore consistency. In particular currently I think if a Focus() would happen after a SetActive(false), the RWH would be focused, whereas if Focus() is followed by SetActive(false) the RWH would be blurred. So the "focus" and "active" states don't seem orthogonal, and possibly the cause of bugs.
On 2015/02/03 at 19:11:07, piman wrote: > As far as I can tell (but I don't know much about focus), the content state should match the RWHV state (modulo IPC), the RWHV tells the RWH about focus when it receives the event from Cocoa/Aura after a focus change - that is whether RWHV::Focus did it or not. That part seems to be true on both Mac and Aura. > On mac-only there is some extra logic, namely RWHV::SetActive, that seems to force Blur on the RWH even though the NSView is (still) the first responder (i.e. RWHV::HasFocus() will return true). I suppose that's the cause of the discrepancy. I don't know if it's by design, or if SetActive(false) should also blur the NSView and restore consistency. In particular currently I think if a Focus() would happen after a SetActive(false), the RWH would be focused, whereas if Focus() is followed by SetActive(false) the RWH would be blurred. So the "focus" and "active" states don't seem orthogonal, and possibly the cause of bugs. What is pointed here is slightly different from my problem. The failing LayoutTests has a not focused frame that should be focused. RFH::IsFocused() is trying to reflect whether the frame is focused according to the renderer, the same way RFH::GetVisibilityState() does. Given that RenderFrame focus state comes from RWH, it seems fair to use that value (it was the initial one and switched to RWHV because of RWH and RFH relationship breakage just before I land the original CL). The problem this CL is trying to fix is that RFH::IsFocused() is _not_ reflecting document.hasFocus() thus can't be used to know if a frame is considered as focused as far as Blink is concerned.
I've updated the comment, created a TODO and opened a bug for GetRenderWidgetHost() returning nullptr sometimes. creis@, PTAL?
Ok, I won't block this further since it's preventing future work, though I think it's unfortunate that we're tracking so many different notions of focus. LGTM with the comment changes below. https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:281: // TODO(mlamouri,kenrb): call directly GetRenderWidgetHost() when it stops nit: call GetRenderWidgetHost directly https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:282: // returning nullptr in some cases, https://crbug.com/455245 nit: Period after "cases," then "See ..." Also period after link. https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1606: bool RenderFrameHostImpl::IsFocused() { The comment for this method in the .h file is now wrong. Please change "its associated RenderWidgetHostView has to be focused" to "its associated RenderWidgetHost has to think it is focused." https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1607: // TODO(mlamouri,kenrb): call directly GetRenderWidgetHost() when it stops Same nits as above. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:231: // Indicates whether the RenderWidgetHost is focused. nit: thinks it is focused. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:232: // This is different from RenderWidgetHostView in the sense that it reflects nit: RenderWidgetHostView::HasFocus() https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:233: // what the renderer process knows - it saves the state that is sent/received. nit: colon rather than dash https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:235: // it is possible in some edge cases that a view was requested to be focused nit: requested to be focused but it failed, thus HasFocus() returns false. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:852: // Indicates whether this frame is focused. This is trying to match what the nit: thinks it is focused. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:854: // in that regard. nit: in that regard -> in that the focus request may fail, causing HasFocus to return false when is_focused_ is true.
New patchsets have been uploaded after l-g-t-m from creis@chromium.org
review comments applied. Thank you for your understanding Charlie! :) https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:281: // TODO(mlamouri,kenrb): call directly GetRenderWidgetHost() when it stops On 2015/02/04 at 17:32:42, Charlie Reis wrote: > nit: call GetRenderWidgetHost directly Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:282: // returning nullptr in some cases, https://crbug.com/455245 On 2015/02/04 at 17:32:42, Charlie Reis wrote: > nit: Period after "cases," then "See ..." Also period after link. Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1606: bool RenderFrameHostImpl::IsFocused() { On 2015/02/04 at 17:32:42, Charlie Reis wrote: > The comment for this method in the .h file is now wrong. Please change "its associated RenderWidgetHostView has to be focused" to "its associated RenderWidgetHost has to think it is focused." Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1607: // TODO(mlamouri,kenrb): call directly GetRenderWidgetHost() when it stops On 2015/02/04 at 17:32:43, Charlie Reis wrote: > Same nits as above. Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:231: // Indicates whether the RenderWidgetHost is focused. On 2015/02/04 at 17:32:43, Charlie Reis wrote: > nit: thinks it is focused. Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:232: // This is different from RenderWidgetHostView in the sense that it reflects On 2015/02/04 at 17:32:43, Charlie Reis wrote: > nit: RenderWidgetHostView::HasFocus() Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:233: // what the renderer process knows - it saves the state that is sent/received. On 2015/02/04 at 17:32:43, Charlie Reis wrote: > nit: colon rather than dash Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:235: // it is possible in some edge cases that a view was requested to be focused On 2015/02/04 at 17:32:43, Charlie Reis wrote: > nit: requested to be focused but it failed, thus HasFocus() returns false. Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:852: // Indicates whether this frame is focused. This is trying to match what the On 2015/02/04 at 17:32:43, Charlie Reis wrote: > nit: thinks it is focused. Done. https://codereview.chromium.org/898443003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:854: // in that regard. On 2015/02/04 at 17:32:43, Charlie Reis wrote: > nit: in that regard -> in that the focus request may fail, causing HasFocus to return false when is_focused_ is true. Done.
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/898443003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2ff3b31d405dbcf301972f3dc5833c40674042b8 Cr-Commit-Position: refs/heads/master@{#314590} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
