Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715213009:
Re-enable FocusBeforeNavigation test. (Closed)
DescriptionRe-enable FocusBeforeNavigation test.
+ Correctly focus the inner WebContents when the attachment frame is
focused.
+ Enable Enable WebViewFocusInteractiveTest.Focus_FocusBeforeNavigation.
+ Prevent BrowserPluginGuest from unfocusing the guest in oopif mode.
BUG=672947
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2715213009
Cr-Commit-Position: refs/heads/master@{#467296}
Committed: https://chromium.googlesource.com/chromium/src/+/7529b2aa9b7de2c64e424082b2ee09cad44e27c4
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebase #
Messages
Total messages: 21 (13 generated)
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avallee@chromium.org changed reviewers: + alexmos@chromium.org, wjmaclean@chromium.org
Looks good, just one question about the early return in RenderViewReady. https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:662: if (GuestMode::IsCrossProcessFrameGuest(GetWebContents())) I'll defer to James or Lucas here, but in the CL description you say this intends to prevent BrowserPluginGuest from unfocusing the guest in oopif mode, but this function seems to do some other things, like set the hung renderer delay and send the BrowserPluginMsg_GuestReady message. Are those still needed in oopif mode? https://codereview.chromium.org/2715213009/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2715213009/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1480: SetFocusedFrame(frame_tree_.root(), nullptr); I'm curious if this makes the code that returns the main frame if there's no focused frame in GetFocusedFrameIncludingInnerWebContents (that I asked about in https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con...) unnecessary? This should guarantee that you focus the main frame on attach, though I'm not sure about the other case you mentioned where the focused frame can become null.
lgtm for test, BrowserPluginGuest https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:662: if (GuestMode::IsCrossProcessFrameGuest(GetWebContents())) On 2017/03/04 01:12:16, alexmos wrote: > I'll defer to James or Lucas here, but in the CL description you say this > intends to prevent BrowserPluginGuest from unfocusing the guest in oopif mode, > but this function seems to do some other things, like set the hung renderer > delay and send the BrowserPluginMsg_GuestReady message. Are those still needed > in oopif mode? I'm not sure I follow: in WebView+OOPIF mode aren't we now early-outing and avoiding doing all those things?
https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:662: if (GuestMode::IsCrossProcessFrameGuest(GetWebContents())) On 2017/03/14 16:13:37, wjmaclean (ooo Mar 15-26) wrote: > On 2017/03/04 01:12:16, alexmos wrote: > > I'll defer to James or Lucas here, but in the CL description you say this > > intends to prevent BrowserPluginGuest from unfocusing the guest in oopif mode, > > but this function seems to do some other things, like set the hung renderer > > delay and send the BrowserPluginMsg_GuestReady message. Are those still > needed > > in oopif mode? > > I'm not sure I follow: in WebView+OOPIF mode aren't we now early-outing and > avoiding doing all those things? Right, so my question is, if we are in webview+OOPIF mode, should we only skip the "Send(new InputMsg_SetFocus(routing_id(), focused_))", which is presumably the "unfocusing" part, but still do the other bits, like set the hung renderer delay and the GuestReady message? Or is none of this function needed in webview+OOPIF mode?
https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:662: if (GuestMode::IsCrossProcessFrameGuest(GetWebContents())) On 2017/03/15 00:16:07, alexmos wrote: > On 2017/03/14 16:13:37, wjmaclean (ooo Mar 15-26) wrote: > > On 2017/03/04 01:12:16, alexmos wrote: > > > I'll defer to James or Lucas here, but in the CL description you say this > > > intends to prevent BrowserPluginGuest from unfocusing the guest in oopif > mode, > > > but this function seems to do some other things, like set the hung renderer > > > delay and send the BrowserPluginMsg_GuestReady message. Are those still > > needed > > > in oopif mode? > > > > I'm not sure I follow: in WebView+OOPIF mode aren't we now early-outing and > > avoiding doing all those things? > > Right, so my question is, if we are in webview+OOPIF mode, should we only skip > the "Send(new InputMsg_SetFocus(routing_id(), focused_))", which is presumably > the "unfocusing" part, but still do the other bits, like set the hung renderer > delay and the GuestReady message? Or is none of this function needed in > webview+OOPIF mode? We definitely need to skip the SetFocus message since webview+OOPIF does not update BrowserPluginGuest::focused_ correctly and plumbing that through looks decently complicated for no apparent benefit. I also think that SendMessageToEmbedder does nothing in oopif and on the other side only clears the guest_crashed_ parameter. I can see that the hung_renderer_delay might still make sense. It's been a while now, but when i spoke with lfg his feeling was that nothing here mattered and that this would get deleted with BrowserPlugin. https://codereview.chromium.org/2715213009/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2715213009/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1480: SetFocusedFrame(frame_tree_.root(), nullptr); On 2017/03/04 01:12:16, alexmos wrote: > I'm curious if this makes the code that returns the main frame if there's no > focused frame in GetFocusedFrameIncludingInnerWebContents (that I asked about in > https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con...) > unnecessary? This should guarantee that you focus the main frame on attach, > though I'm not sure about the other case you mentioned where the focused frame > can become null. This will end up calling focus on the widget which will cause the renderer side to focus the main frame when there is no focused frame.
LGTM https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2715213009/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:662: if (GuestMode::IsCrossProcessFrameGuest(GetWebContents())) On 2017/03/15 02:55:20, avallee wrote: > On 2017/03/15 00:16:07, alexmos wrote: > > On 2017/03/14 16:13:37, wjmaclean (ooo Mar 15-26) wrote: > > > On 2017/03/04 01:12:16, alexmos wrote: > > > > I'll defer to James or Lucas here, but in the CL description you say this > > > > intends to prevent BrowserPluginGuest from unfocusing the guest in oopif > > mode, > > > > but this function seems to do some other things, like set the hung > renderer > > > > delay and send the BrowserPluginMsg_GuestReady message. Are those still > > > needed > > > > in oopif mode? > > > > > > I'm not sure I follow: in WebView+OOPIF mode aren't we now early-outing and > > > avoiding doing all those things? > > > > Right, so my question is, if we are in webview+OOPIF mode, should we only skip > > the "Send(new InputMsg_SetFocus(routing_id(), focused_))", which is presumably > > the "unfocusing" part, but still do the other bits, like set the hung renderer > > delay and the GuestReady message? Or is none of this function needed in > > webview+OOPIF mode? > > We definitely need to skip the SetFocus message since webview+OOPIF does not > update BrowserPluginGuest::focused_ correctly and plumbing that through looks > decently complicated for no apparent benefit. > > I also think that SendMessageToEmbedder does nothing in oopif and on the other > side only clears the guest_crashed_ parameter. > > I can see that the hung_renderer_delay might still make sense. > > It's been a while now, but when i spoke with lfg his feeling was that nothing > here mattered and that this would get deleted with BrowserPlugin. Ack. The hung_renderer_delay also seems like it's just being set to the default value that isn't really changed anywhere else.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2715213009/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493206393518350,
"parent_rev": "5642039423c57e59ee6651f14c34f1b73c41d08c", "commit_rev":
"7529b2aa9b7de2c64e424082b2ee09cad44e27c4"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable FocusBeforeNavigation test. + Correctly focus the inner WebContents when the attachment frame is focused. + Enable Enable WebViewFocusInteractiveTest.Focus_FocusBeforeNavigation. + Prevent BrowserPluginGuest from unfocusing the guest in oopif mode. BUG=672947 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Re-enable FocusBeforeNavigation test. + Correctly focus the inner WebContents when the attachment frame is focused. + Enable Enable WebViewFocusInteractiveTest.Focus_FocusBeforeNavigation. + Prevent BrowserPluginGuest from unfocusing the guest in oopif mode. BUG=672947 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2715213009 Cr-Commit-Position: refs/heads/master@{#467296} Committed: https://chromium.googlesource.com/chromium/src/+/7529b2aa9b7de2c64e424082b2ee... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7529b2aa9b7de2c64e424082b2ee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
