Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451143003:
<webview>: Correctly shift focus between WebContents. (Closed)
Description<webview>: Correctly shift focus between WebContents.
http://crrev.com/408445 Fixes some cases of keyboard input but required
a renderer initiated frame focus change in order to do so.
This change assumes that the browser is the primary driver of
WebContents level focus changes. The leftover FrameHost*::OnFrameFocused
is left so that an embedder can script aWebview.focus() to give focus.
BUG=660044
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/9993fca0084ba71c628918b384ac701fe6514e44
Cr-Commit-Position: refs/heads/master@{#432781}
Patch Set 1 #Patch Set 2 : Focus change working. #Patch Set 3 : Focus change working. #Patch Set 4 : Ready for review #
Total comments: 4
Patch Set 5 : Address wjmaclean comment: ternary op. #
Total comments: 38
Patch Set 6 : Address comments from alexmos and lfg. #
Total comments: 22
Patch Set 7 : Delete parent notification loop as noted by alexmos@. #Patch Set 8 : Didn't rename function in cpp #Patch Set 9 : forgot rebase #
Total comments: 5
Patch Set 10 : Add test for window stealing focus and returning. #Patch Set 11 : Add fix for MacOS hang. #
Total comments: 20
Patch Set 12 : Fix creis comments. #
Total comments: 9
Patch Set 13 : Fix final comments from creis and alexmos. #
Total comments: 10
Patch Set 14 : Fix creis comments. #Messages
Total messages: 97 (59 generated)
Description was changed from ========== Fix webview focus to switch in all cases. This patch also ensures that page level focus is updated for all web contents on the page. BUG= ========== to ========== Fix webview focus to switch in all cases. This patch also ensures that page level focus is updated for all web contents on the page. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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.
Description was changed from ========== Fix webview focus to switch in all cases. This patch also ensures that page level focus is updated for all web contents on the page. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== <webview>: Correctly shift focus between WebContents. http://crrev.com/408445 Fixes some cases of keyboard input but required a renderer initiated frame focus change in order to do so. This change assumes that the browser is the primary driver of WebContents level focus changes. The leftover FrameHost*::OnFrameFocused is left so that an embedder can script aWebview.focus() to give focus. BUG=660044 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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...
avallee@chromium.org changed reviewers: + alexmos@chromium.org, lfg@chromium.org, nasko@chromium.org, wjmaclean@chromium.org
lfg/alexmos: Please comment overall. wjmaclean@chromium.org: Please review changes in nasko@chromium.org: FYI
Generally lftm (looks fine to me), but I'll defer overall to people with greater frame-tree knowledge than mine. https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:697: focused_widget = delegate_->GetFocusedRenderWidgetHost(this); Minor suggestion: RenderWidgetHostImpl* focused_widget = delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr;
https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:697: focused_widget = delegate_->GetFocusedRenderWidgetHost(this); On 2016/10/27 18:41:01, wjmaclean wrote: > Minor suggestion: > > RenderWidgetHostImpl* focused_widget = > delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; Done. How do we feel about auto*?
On 2016/10/27 19:00:57, avallee wrote: > https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.cc:697: focused_widget = > delegate_->GetFocusedRenderWidgetHost(this); > On 2016/10/27 18:41:01, wjmaclean wrote: > > Minor suggestion: > > > > RenderWidgetHostImpl* focused_widget = > > delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; > > Done. How do we feel about auto*? Unless the type is completely obvious from what's on the right, generally better to stay explicit. Here it's not obvious between RenderWidgetHost vs RenderWidgetHostImpl, so I (personally) wouldn't use auto.
Just a couple of questions. https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1364: content::FrameFocusedObserver focus_observer( Is this still needed? https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4644: if (!tree_node) { It took me some time to understand that this can only happen for browser plugin, can you add a comment explaining, with a TODO to remove? (you can reference bug 533069). https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4646: ->GetMainFrame() +ekaramad@ FYI. This will probably need to be fixed in your CL that fixes PDF inside OOPIFs. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4648: outer_proxy = web_contents->frame_tree_.root() Why do we want to call Focus on this proxy? (assuming this only happens on BrowserPlugin). https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4724: ChangeFocus(static_cast<WebContentsImpl*>(focused_widget->delegate())); I'm not sure this cast is safe, a quick search tells me that InterstitialPageImpl is also a RenderWidgetHostDelegate.
Thanks, some initial questions and comments below. https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (left): https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1381: EXPECT_EQ(embedder_web_contents()->GetFocusedFrame(), nullptr); Why isn't this needed anymore? https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:696: delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; I think in the case where GetFocusedRenderWidgetHost returns null, we're supposed to drop the event to avoid giving it to the wrong frame (see comments inside that function). Can you check how this works if the guest crashes while having page focus, and then you get into here? https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:699: focused_widget->FocusDirect(); I'm trying to understand the implication of this change for regular OOPIF cases. Say we have a page with frame A embedding frame B, and B is an OOPIF. If we focus frame A and then alt-tab back and forth, this will behave as before: send InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy in process B (thanks to ReplicatePageFocus). So both renderers get the page focus messages. If we now focus frame B and then alt-tab back and forth, this seems to behave differently: GetFocusedRenderWidgetHost will return B's RWH, hence we will send the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) is false for that RWH, we don't replicate the page focus message to A. That will also cause the is_focused_ bit to become inconsistent for both main frame and subframe's RWHs, though I'm not sure what that entails. That doesn't seem consistent/desirable. Is the different behavior intentional? Was this meant to only apply to the RWH for embedders/guests, or to subframes as well? https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:714: void RenderWidgetHostImpl::FocusDirect() { I find these names a bit hard to interpret. Since this is just setting page focus, wdyt about calling it SetPageFocus()/ClearPageFocus()? (You could even go one step further and merge the two nearly identical functions and take a bool instead) https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4642: old_focused_contents->frame_tree_.SetFocusedFrame(nullptr, source); To sanity check my understanding, previously, old_focused_contents maintained page focus when the user clicked on a different WebContents on the page, so we had to clear the focused frame for things like making the caret go away from a focused input field. Now, you'll remove page focus from old_focused_contents instead, which will have the same effect, and this allows you to keep the focused frame intact, which is why this call isn't necessary. Correct? If yes, given that this was the only place that ever cleared a focused frame, could we also remove the RFHI::ClearFocusedFrame and all the associated plumbing? https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4633: // focused, if present. Can you please help me understand why we need the while loop? Why isn't BlurDirect on the old_contents and FocusDirect() and the SetFocusedWebContents on the new WebContents sufficient? https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4636: // and focus this contents to activate it. These two lines of the comment seem more appropriate below for the code after the while loop. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4680: // Focus is moving between frame trees, unfocus the frame in the old tree. Comment looks stale (ChangeFocus tinkers with page focus and doesn't unfocus any frames.) https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:1023: // RenderWidget and direct keyboard input to it. I'd also mention what happens to |old_contents|.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
PTAL. https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (left): https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1381: EXPECT_EQ(embedder_web_contents()->GetFocusedFrame(), nullptr); On 2016/10/28 06:36:39, alexmos wrote: > Why isn't this needed anymore? This is no longer needed as the click itself is what causes the focus change to happen. This happens synchronously before dispatching the event to the renderer such that once we see the input as focused, we're guaranteed that input will end up there. https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2451143003/diff/80001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1364: content::FrameFocusedObserver focus_observer( On 2016/10/27 20:00:35, lfg wrote: > Is this still needed? Gone. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:696: delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; On 2016/10/28 06:36:39, alexmos wrote: > I think in the case where GetFocusedRenderWidgetHost returns null, we're > supposed to drop the event to avoid giving it to the wrong frame (see comments > inside that function). Can you check how this works if the guest crashes while > having page focus, and then you get into here? The main place where Focus() is called is from the RenderWidgetHostView which only knows about the top level RenderWidget. If an internal RenderWidget dies, I think it's fine to refocus the outermost RenderWidget when we refocus the window. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:699: focused_widget->FocusDirect(); On 2016/10/28 06:36:39, alexmos wrote: > I'm trying to understand the implication of this change for regular OOPIF cases. > Say we have a page with frame A embedding frame B, and B is an OOPIF. If we > focus frame A and then alt-tab back and forth, this will behave as before: send > InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy in > process B (thanks to ReplicatePageFocus). So both renderers get the page focus > messages. > > If we now focus frame B and then alt-tab back and forth, this seems to behave > differently: GetFocusedRenderWidgetHost will return B's RWH, hence we will send > the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) is > false for that RWH, we don't replicate the page focus message to A. > > That will also cause the is_focused_ bit to become inconsistent for both main > frame and subframe's RWHs, though I'm not sure what that entails. > > That doesn't seem consistent/desirable. Is the different behavior intentional? > Was this meant to only apply to the RWH for embedders/guests, or to subframes as > well? I believe that GetFocusedWebContents()->GetMainFrame() is the right thing to do here. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:714: void RenderWidgetHostImpl::FocusDirect() { On 2016/10/28 06:36:39, alexmos wrote: > I find these names a bit hard to interpret. Since this is just setting page > focus, wdyt about calling it SetPageFocus()/ClearPageFocus()? (You could even > go one step further and merge the two nearly identical functions and take a bool > instead) Done. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4642: old_focused_contents->frame_tree_.SetFocusedFrame(nullptr, source); On 2016/10/28 06:36:39, alexmos wrote: > To sanity check my understanding, previously, old_focused_contents maintained > page focus when the user clicked on a different WebContents on the page, so we > had to clear the focused frame for things like making the caret go away from a > focused input field. Now, you'll remove page focus from old_focused_contents > instead, which will have the same effect, and this allows you to keep the > focused frame intact, which is why this call isn't necessary. Correct? > > If yes, given that this was the only place that ever cleared a focused frame, > could we also remove the RFHI::ClearFocusedFrame and all the associated > plumbing? Yes, we used to clear focus in order to have the renderer initiate the WebContents focus change via RPC to set _some_ frame as focused, the side effect being that the right web contents will be focused. This is also why carets and other indicators of being the focused page disappear. Instead ChangeFocusIfNecessary assumes that focus changes are browser initiated, with the SetFocusedFrame hook handling renderer initiated changes. This is indeed the only place that would lead to ClearFocusedFrame. I was hoping to make that a subsequent CL to clean up and seprate the concerns. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4633: // focused, if present. On 2016/10/28 06:36:39, alexmos wrote: > Can you please help me understand why we need the while loop? Why isn't > BlurDirect on the old_contents and FocusDirect() and the SetFocusedWebContents > on the new WebContents sufficient? We want to let the embedding renderer(s) know that focus has moved. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4636: // and focus this contents to activate it. On 2016/10/28 06:36:39, alexmos wrote: > These two lines of the comment seem more appropriate below for the code after > the while loop. Done. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4644: if (!tree_node) { On 2016/10/27 20:00:35, lfg wrote: > It took me some time to understand that this can only happen for browser plugin, > can you add a comment explaining, with a TODO to remove? (you can reference bug > 533069). Done. My main worry: <body><webview></webview><iframe></iframe></body>. Say focus is in the iframe in the BrowserPlugin case. I worry that we need to focus the main frame otherwise BrowserPluginEmbedder won't set focus on the BrowserPlugin. I tested and it seems to work fine though. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4648: outer_proxy = web_contents->frame_tree_.root() On 2016/10/27 20:00:35, lfg wrote: > Why do we want to call Focus on this proxy? (assuming this only happens on > BrowserPlugin). Doesn't make sense. Deleted. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4680: // Focus is moving between frame trees, unfocus the frame in the old tree. On 2016/10/28 06:36:39, alexmos wrote: > Comment looks stale (ChangeFocus tinkers with page focus and doesn't unfocus any > frames.) Done. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4724: ChangeFocus(static_cast<WebContentsImpl*>(focused_widget->delegate())); On 2016/10/27 20:00:35, lfg wrote: > I'm not sure this cast is safe, a quick search tells me that > InterstitialPageImpl is also a RenderWidgetHostDelegate. Moved the check into ChangeFocusIfNeeded(); The contents is fetched internally. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:1023: // RenderWidget and direct keyboard input to it. On 2016/10/28 06:36:39, alexmos wrote: > I'd also mention what happens to |old_contents|. Done.
PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
Thanks, a few more comments. Also, I think it's worth adding some tests for this. One case could be to do something similar to what you had in WebViewInteractiveTest.KeyboardFocus, but change the active window in the middle of typing, then come back to the window with the <webview>, continue typing, and see if the text still ends up in the correct place. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:696: delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; On 2016/10/28 19:19:56, avallee wrote: > On 2016/10/28 06:36:39, alexmos wrote: > > I think in the case where GetFocusedRenderWidgetHost returns null, we're > > supposed to drop the event to avoid giving it to the wrong frame (see comments > > inside that function). Can you check how this works if the guest crashes > while > > having page focus, and then you get into here? > > The main place where Focus() is called is from the RenderWidgetHostView which > only knows about the top level RenderWidget. If an internal RenderWidget dies, I > think it's fine to refocus the outermost RenderWidget when we refocus the > window. It might not always be safe: a malicious embedder could load some sensitive site in a <webview>, then crash it just as you start typing sensitive data inside the <webview>. The embedder might then start getting the rest of the data if we focus it, until user realizes that the keystrokes are going to the wrong place? See https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content... for a discussion when this came up for OOPIFs. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:699: focused_widget->FocusDirect(); On 2016/10/28 19:19:56, avallee wrote: > On 2016/10/28 06:36:39, alexmos wrote: > > I'm trying to understand the implication of this change for regular OOPIF > cases. > > Say we have a page with frame A embedding frame B, and B is an OOPIF. If we > > focus frame A and then alt-tab back and forth, this will behave as before: > send > > InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy in > > process B (thanks to ReplicatePageFocus). So both renderers get the page > focus > > messages. > > > > If we now focus frame B and then alt-tab back and forth, this seems to behave > > differently: GetFocusedRenderWidgetHost will return B's RWH, hence we will > send > > the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) is > > false for that RWH, we don't replicate the page focus message to A. > > > > That will also cause the is_focused_ bit to become inconsistent for both main > > frame and subframe's RWHs, though I'm not sure what that entails. > > > > That doesn't seem consistent/desirable. Is the different behavior > intentional? > > Was this meant to only apply to the RWH for embedders/guests, or to subframes > as > > well? > > I believe that GetFocusedWebContents()->GetMainFrame() is the right thing to do > here. Yes, that looks better. However, are we sure that this can't ever be called on a non-main-frame widget? At first glance in codesearch, most callsites get here via smth like GetRenderViewHost()->GetWidget()->Focus(), which implies main frame widget, but I didn't check all the paths. I'm especially wondering if this is ever called for fullscreen RWHs (especially Flash fullscreen) and/or popup menu widgets (i.e., <select> dropdowns). Do you mind double-checking those cases? See https://crbug.com/593522 for some examples of Flash fullscreen. I'm curious if we need to keep the old behavior of just sending InputMsg_SetFocus to the RWH itself for those cases. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4633: // focused, if present. On 2016/10/28 19:19:57, avallee wrote: > On 2016/10/28 06:36:39, alexmos wrote: > > Can you please help me understand why we need the while loop? Why isn't > > BlurDirect on the old_contents and FocusDirect() and the SetFocusedWebContents > > on the new WebContents sufficient? > > We want to let the embedding renderer(s) know that focus has moved. Sorry, I'm still a bit fuzzy on this. So looking at the code, basically whenever an inner WebContents becomes focused, this focuses the corresponding *frame* in the outer WebContents. This seems reasonable, but I can't figure out what exactly breaks if we don't do that? Doesn't seem like it would affect event routing as long as we call SetFocusedWebContents(this). (I tried out your CL without the while loop on a couple of sample webview apps, and everything still appeared to work, hence why I'm confused.) https://codereview.chromium.org/2451143003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:253: // Support for focus tracking on multi-WebContents cases. Users other than Let's explain more carefully what this does. I.e., sends the page focus update IPC to all renderers involved in rendering the current page. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4657: web_contents = web_contents->GetOuterWebContents(); nit: seems like you could move this up to just before the "if (!tree_node)" and use it there. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4665: outer_proxy->SetFocusedFrame(); Does this need to be done if the tree_node is already the focused node in web_contents->frame_tree_? https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) Can you please explain this check? So if this is a BrowserPlugin guest we return early, but if it's either a --use-cpffg guest or not a guest at all, we proceed? Or maybe you meant !browser_plugin_guest_ in the condition? Perhaps add a comment? https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1024: // RenderWidget and direct keyboard input to it. The previously focused Is it a bit ambiguous what "this content's RenderWidget" refers to. It's really the WebContents's *main frame* RenderWidget, right? But with OOPIFs, it's also going to set page focus in subframe renderers via RenderFrameProxy::SetPageFocus(), right? https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1026: void ChangeFocusIfNecessary(); Perhaps something like SetAsFocusedWebContentsIfNecessary() would better convey what this is doing? I.e., that it sets this WebContents as focused, and doesn't change some other notion of focus.
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...
Addressed comments. Ehsan, mind to comment about (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4633: // focused, if present. On 2016/10/31 18:57:57, alexmos wrote: > On 2016/10/28 19:19:57, avallee wrote: > > On 2016/10/28 06:36:39, alexmos wrote: > > > Can you please help me understand why we need the while loop? Why isn't > > > BlurDirect on the old_contents and FocusDirect() and the > SetFocusedWebContents > > > on the new WebContents sufficient? > > > > We want to let the embedding renderer(s) know that focus has moved. > > Sorry, I'm still a bit fuzzy on this. So looking at the code, basically > whenever an inner WebContents becomes focused, this focuses the corresponding > *frame* in the outer WebContents. This seems reasonable, but I can't figure out > what exactly breaks if we don't do that? Doesn't seem like it would affect > event routing as long as we call SetFocusedWebContents(this). (I tried out your > CL without the while loop on a couple of sample webview apps, and everything > still appeared to work, hence why I'm confused.) Now that I look at it, it seems to work. I don't remember why I had added this back in initially. I'll delete for now unless it breaks some tests. https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4646: ->GetMainFrame() On 2016/10/27 20:00:35, lfg wrote: > +ekaramad@ FYI. This will probably need to be fixed in your CL that fixes PDF > inside OOPIFs. +lfg, +ekaramad@ deleted... https://codereview.chromium.org/2451143003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:253: // Support for focus tracking on multi-WebContents cases. Users other than On 2016/10/31 18:57:57, alexmos wrote: > Let's explain more carefully what this does. I.e., sends the page focus update > IPC to all renderers involved in rendering the current page. Done. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4657: web_contents = web_contents->GetOuterWebContents(); On 2016/10/31 18:57:57, alexmos wrote: > nit: seems like you could move this up to just before the "if (!tree_node)" and > use it there. Loop deleted. I think this will not be necessary. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4665: outer_proxy->SetFocusedFrame(); On 2016/10/31 18:57:57, alexmos wrote: > Does this need to be done if the tree_node is already the focused node in > web_contents->frame_tree_? I'm not sure, but deleted for now, it seems notifying parents is inconsequential. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/10/31 18:57:57, alexmos wrote: > Can you please explain this check? So if this is a BrowserPlugin guest we > return early, but if it's either a --use-cpffg guest or not a guest at all, we > proceed? Or maybe you meant !browser_plugin_guest_ in the condition? Perhaps > add a comment? This fixes an issue with TextInputState. This is the same condition that is at the top of WebContentsImpl::SetFocusedFrame. +ekaramad@ Added the original condition, I'm still not sure I wrap my head around it. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1024: // RenderWidget and direct keyboard input to it. The previously focused On 2016/10/31 18:57:57, alexmos wrote: > Is it a bit ambiguous what "this content's RenderWidget" refers to. It's really > the WebContents's *main frame* RenderWidget, right? But with OOPIFs, it's also > going to set page focus in subframe renderers via > RenderFrameProxy::SetPageFocus(), right? Yes, focusing the Widget will call back into WebContentsImpl::ReplicatePageFocus. This indeed eventually calls RenderFrameProxy::SetPageFocus(). https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1026: void ChangeFocusIfNecessary(); On 2016/10/31 18:57:57, alexmos wrote: > Perhaps something like SetAsFocusedWebContentsIfNecessary() would better convey > what this is doing? I.e., that it sets this WebContents as focused, and doesn't > change some other notion of focus. Good idea. Done.
The CQ bit was unchecked by avallee@chromium.org
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 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.
ekaramad@chromium.org changed reviewers: + ekaramad@chromium.org
https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4646: ->GetMainFrame() On 2016/10/27 20:00:35, lfg wrote: > +ekaramad@ FYI. This will probably need to be fixed in your CL that fixes PDF > inside OOPIFs. Thanks! It probably should. I think webcontents->GetBrowserPluginGuest()->GetEmbedderFrame()-GetSiteInstance() should replace the site instance below. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/07 19:18:44, avallee wrote: > On 2016/10/31 18:57:57, alexmos wrote: > > Can you please explain this check? So if this is a BrowserPlugin guest we > > return early, but if it's either a --use-cpffg guest or not a guest at all, we > > proceed? Or maybe you meant !browser_plugin_guest_ in the condition? Perhaps > > add a comment? > > This fixes an issue with TextInputState. This is the same condition that is at > the top of WebContentsImpl::SetFocusedFrame. +ekaramad@ Added the original > condition, I'm still not sure I wrap my head around it. This is actually for making sure PDF runs on BrowserPlugin even when other guests use OOPIF. The initial method would change the focused WebContents but we did not want to do it if the WebContents was a guest based on BrowserPlugin. It was superseded with a line of code like if (!UseCrossProcessFramesForGuests()) return; which would not quite cut it if had the flag to run guests on OOPIF but the WebContentsImpl was for a MimeHandlerViewGuest, which is the only guest as of now intentionally forced to used BrowserPlugin. In short, we early return here is 'this' WebContents is for a guest which is NOT using OOPIF. As to why we would early return I think the reason was that focus was supposed to go through embedder->guest since it was input related. Are we changing that logic here, i.e., BrowserPlugin WebContents' exposed to OOPIF?
Thanks, this is getting there. A couple more comments, with a question for Ehsan, and let's try to add a test for this that we discussed offline. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:699: focused_widget->FocusDirect(); On 2016/10/31 18:57:57, alexmos wrote: > On 2016/10/28 19:19:56, avallee wrote: > > On 2016/10/28 06:36:39, alexmos wrote: > > > I'm trying to understand the implication of this change for regular OOPIF > > cases. > > > Say we have a page with frame A embedding frame B, and B is an OOPIF. If > we > > > focus frame A and then alt-tab back and forth, this will behave as before: > > send > > > InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy in > > > process B (thanks to ReplicatePageFocus). So both renderers get the page > > focus > > > messages. > > > > > > If we now focus frame B and then alt-tab back and forth, this seems to > behave > > > differently: GetFocusedRenderWidgetHost will return B's RWH, hence we will > > send > > > the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) is > > > false for that RWH, we don't replicate the page focus message to A. > > > > > > That will also cause the is_focused_ bit to become inconsistent for both > main > > > frame and subframe's RWHs, though I'm not sure what that entails. > > > > > > That doesn't seem consistent/desirable. Is the different behavior > > intentional? > > > Was this meant to only apply to the RWH for embedders/guests, or to > subframes > > as > > > well? > > > > I believe that GetFocusedWebContents()->GetMainFrame() is the right thing to > do > > here. > > Yes, that looks better. However, are we sure that this can't ever be called on > a non-main-frame widget? At first glance in codesearch, most callsites get here > via smth like GetRenderViewHost()->GetWidget()->Focus(), which implies main > frame widget, but I didn't check all the paths. I'm especially wondering if > this is ever called for fullscreen RWHs (especially Flash fullscreen) and/or > popup menu widgets (i.e., <select> dropdowns). Do you mind double-checking > those cases? See https://crbug.com/593522 for some examples of Flash > fullscreen. I'm curious if we need to keep the old behavior of just sending > InputMsg_SetFocus to the RWH itself for those cases. Still curious about this. Might be worth trying putting a DCHECK here and in Blur(), to see if we ever have a focused_widget != this, yet they point to the same delegates. I'm curious if this breaks any tests. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/08 16:05:47, EhsanK wrote: > On 2016/11/07 19:18:44, avallee wrote: > > On 2016/10/31 18:57:57, alexmos wrote: > > > Can you please explain this check? So if this is a BrowserPlugin guest we > > > return early, but if it's either a --use-cpffg guest or not a guest at all, > we > > > proceed? Or maybe you meant !browser_plugin_guest_ in the condition? > Perhaps > > > add a comment? > > > > This fixes an issue with TextInputState. This is the same condition that is at > > the top of WebContentsImpl::SetFocusedFrame. +ekaramad@ Added the original > > condition, I'm still not sure I wrap my head around it. > > This is actually for making sure PDF runs on BrowserPlugin even when other > guests use OOPIF. The initial method would change the focused WebContents but we > did not want to do it if the WebContents was a guest based on BrowserPlugin. > > It was superseded with a line of code like > if (!UseCrossProcessFramesForGuests()) > return; > > which would not quite cut it if had the flag to run guests on OOPIF but the > WebContentsImpl was for a MimeHandlerViewGuest, which is the only guest as of > now intentionally forced to used BrowserPlugin. > > In short, we early return here is 'this' WebContents is for a guest which is NOT > using OOPIF. > > As to why we would early return I think the reason was that focus was supposed > to go through embedder->guest since it was input related. Are we changing that > logic here, i.e., BrowserPlugin WebContents' exposed to OOPIF? I think the intent of the early return in WebContentsImpl::SetFocusedFrame originally (see https://codereview.chromium.org/1934703002/) was to fall back to single-WebContents behavior if --use-cpffg is off. This included both BrowserPlugin guests and the cases of ordinary WebContents without any guests. I'm not sure if I fully understand the new check in WebContentsImpl::SetFocusedFrame: it will early-return only for BrowserPlugin guests, but it seems like it will do the multi-WebContents work below even when there are no guests, which I don't think we intended. Ehsan, does this reasoning make sense, or am I off somewhere? I don't understand why just !GuestMode::IsCrossProcessFrameGuest(this) isn't sufficient both in WebContentsImpl::SetFocusedFrame and here, as IsCrossProcessFrameGuest seems to already return false for PDF guests even with --use-cpffg, IIUC. It also returns false when there's no guest (right?), which is another case where we want to exit early. https://codereview.chromium.org/2451143003/diff/160001/content/browser/frame_... File content/browser/frame_host/frame_tree.cc (left): https://codereview.chromium.org/2451143003/diff/160001/content/browser/frame_... content/browser/frame_host/frame_tree.cc:276: root()->current_frame_host()->UpdateAXTreeData(); Does this need to be called somewhere else, now that this path is gone? (In any case, sounds like it's a separate issue for a future followup.)
https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/09 02:47:54, alexmos wrote: > On 2016/11/08 16:05:47, EhsanK wrote: > > On 2016/11/07 19:18:44, avallee wrote: > > > On 2016/10/31 18:57:57, alexmos wrote: > > > > Can you please explain this check? So if this is a BrowserPlugin guest we > > > > return early, but if it's either a --use-cpffg guest or not a guest at > all, > > we > > > > proceed? Or maybe you meant !browser_plugin_guest_ in the condition? > > Perhaps > > > > add a comment? > > > > > > This fixes an issue with TextInputState. This is the same condition that is > at > > > the top of WebContentsImpl::SetFocusedFrame. +ekaramad@ Added the original > > > condition, I'm still not sure I wrap my head around it. > > > > This is actually for making sure PDF runs on BrowserPlugin even when other > > guests use OOPIF. The initial method would change the focused WebContents but > we > > did not want to do it if the WebContents was a guest based on BrowserPlugin. > > > > It was superseded with a line of code like > > if (!UseCrossProcessFramesForGuests()) > > return; > > > > which would not quite cut it if had the flag to run guests on OOPIF but the > > WebContentsImpl was for a MimeHandlerViewGuest, which is the only guest as of > > now intentionally forced to used BrowserPlugin. > > > > In short, we early return here is 'this' WebContents is for a guest which is > NOT > > using OOPIF. > > > > As to why we would early return I think the reason was that focus was supposed > > to go through embedder->guest since it was input related. Are we changing that > > logic here, i.e., BrowserPlugin WebContents' exposed to OOPIF? > > I think the intent of the early return in WebContentsImpl::SetFocusedFrame > originally (see https://codereview.chromium.org/1934703002/) was to fall back to > single-WebContents behavior if --use-cpffg is off. This included both > BrowserPlugin guests and the cases of ordinary WebContents without any guests. > > I'm not sure if I fully understand the new check in > WebContentsImpl::SetFocusedFrame: it will early-return only for BrowserPlugin > guests, but it seems like it will do the multi-WebContents work below even when > there are no guests, which I don't think we intended. Ehsan, does this > reasoning make sense, or am I off somewhere? I don't understand why just > !GuestMode::IsCrossProcessFrameGuest(this) isn't sufficient both in > WebContentsImpl::SetFocusedFrame and here, as IsCrossProcessFrameGuest seems to > already return false for PDF guests even with --use-cpffg, IIUC. It also > returns false when there's no guest (right?), which is another case where we > want to exit early. alexmos@: If I understand your question, we are asking about the check in the WebContentsImpl::SetFocusedFrame at ToT now (without this change), right? If that is the case, here is my justification: We still need to go through the multi-WebContents checks. The example to show this is a <webview> with <input> inside an app window which itself has an <input>. If we early return on a non-guest WebContents, then the 'WebContents focus' will not be tracked properly. Imagine I clicked the <webview> input and then clicked the <input> in app. This leads us to a call to WebContentsImpl::SetFocusedFrame where |this| ptr is the outer WebContents. But the |old_web_contents|, aka GetFocusedWebContents(), is still the inner WebContents. By early returning we are losing the change to reset the focused WebContents to embedder. The result would be that the frame tree of the outer WebContents receives focus, but the GetFocusedWebContents() will return the inner WebContents and hence, GetFocusedRenderWidgetHost() will fail. So after clicking on app's <input>, keyboard input will go to <webview>. I tested this on ToT and it fails as expected.
https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/09 16:58:02, EhsanK wrote: > On 2016/11/09 02:47:54, alexmos wrote: > > On 2016/11/08 16:05:47, EhsanK wrote: > > > On 2016/11/07 19:18:44, avallee wrote: > > > > On 2016/10/31 18:57:57, alexmos wrote: > > > > > Can you please explain this check? So if this is a BrowserPlugin guest > we > > > > > return early, but if it's either a --use-cpffg guest or not a guest at > > all, > > > we > > > > > proceed? Or maybe you meant !browser_plugin_guest_ in the condition? > > > Perhaps > > > > > add a comment? > > > > > > > > This fixes an issue with TextInputState. This is the same condition that > is > > at > > > > the top of WebContentsImpl::SetFocusedFrame. +ekaramad@ Added the original > > > > condition, I'm still not sure I wrap my head around it. > > > > > > This is actually for making sure PDF runs on BrowserPlugin even when other > > > guests use OOPIF. The initial method would change the focused WebContents > but > > we > > > did not want to do it if the WebContents was a guest based on BrowserPlugin. > > > > > > It was superseded with a line of code like > > > if (!UseCrossProcessFramesForGuests()) > > > return; > > > > > > which would not quite cut it if had the flag to run guests on OOPIF but the > > > WebContentsImpl was for a MimeHandlerViewGuest, which is the only guest as > of > > > now intentionally forced to used BrowserPlugin. > > > > > > In short, we early return here is 'this' WebContents is for a guest which is > > NOT > > > using OOPIF. > > > > > > As to why we would early return I think the reason was that focus was > supposed > > > to go through embedder->guest since it was input related. Are we changing > that > > > logic here, i.e., BrowserPlugin WebContents' exposed to OOPIF? > > > > I think the intent of the early return in WebContentsImpl::SetFocusedFrame > > originally (see https://codereview.chromium.org/1934703002/) was to fall back > to > > single-WebContents behavior if --use-cpffg is off. This included both > > BrowserPlugin guests and the cases of ordinary WebContents without any guests. > > > > > I'm not sure if I fully understand the new check in > > WebContentsImpl::SetFocusedFrame: it will early-return only for BrowserPlugin > > guests, but it seems like it will do the multi-WebContents work below even > when > > there are no guests, which I don't think we intended. Ehsan, does this > > reasoning make sense, or am I off somewhere? I don't understand why just > > !GuestMode::IsCrossProcessFrameGuest(this) isn't sufficient both in > > WebContentsImpl::SetFocusedFrame and here, as IsCrossProcessFrameGuest seems > to > > already return false for PDF guests even with --use-cpffg, IIUC. It also > > returns false when there's no guest (right?), which is another case where we > > want to exit early. > > alexmos@: If I understand your question, we are asking about the check in the > WebContentsImpl::SetFocusedFrame at ToT now (without this change), right? Right. > If that is the case, here is my justification: I'm assuming the example is without --use-cpffg? > > We still need to go through the multi-WebContents checks. The example to show > this is a <webview> with <input> inside an app window which itself has an > <input>. If we early return on a non-guest WebContents, then the 'WebContents > focus' will not be tracked properly. > > Imagine I clicked the <webview> input and then clicked the <input> in app. This > leads us to a call to WebContentsImpl::SetFocusedFrame where |this| ptr is the > outer WebContents. But the |old_web_contents|, aka GetFocusedWebContents(), is > still the inner WebContents. What would change it to the inner WebContents in the first place? Seems like you'd early return in WCI::SetFocusedFrame when clicking on the <input> in the <webview>, and WCI::SetFocusedFrame is the only thing that can change it (to the inner WC). > By early returning we are losing the change to > reset the focused WebContents to embedder. > > The result would be that the frame tree of the outer WebContents receives focus, > but the GetFocusedWebContents() will return the inner WebContents and hence, > GetFocusedRenderWidgetHost() will fail. So after clicking on app's <input>, > keyboard input will go to <webview>. > > I tested this on ToT and it fails as expected. > Was your example broken (when done without --use-cpffg) before you fixed that check in WebContentsImpl::SetFocusedFrame?
https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) > I'm assuming the example is without --use-cpffg? No, actually this is using --use-cpffg. BTW, the new way to enable ucpffg is to add --enable-features=GuestViewCrossProcessFrames. I also had --site-per-process but I believe it might be irrelevant. > What would change it to the inner WebContents in the first place? Seems like > you'd early return in WCI::SetFocusedFrame when clicking on the <input> in the > <webview>, and WCI::SetFocusedFrame is the only thing that can change > it (to the > inner WC). Since the <webview> is using cross process frames, we will set it as focused when we first click on the <input> inside the <webview>. The problem is by early returning, we do not set the outer WebContents as focused when we click the <input> in the app. > Was your example broken (when done without --use-cpffg) before you > fixed that > check in WebContentsImpl::SetFocusedFrame? Without --ucpfg the app works fine. I think the reason is that no one ever changes the focused WebContents in that case.
https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/09 19:17:15, EhsanK wrote: > > I'm assuming the example is without --use-cpffg? > No, actually this is using --use-cpffg. BTW, the new way to enable ucpffg > is to add --enable-features=GuestViewCrossProcessFrames. I also had > --site-per-process but I believe it might be irrelevant. > > > What would change it to the inner WebContents in the first place? Seems like > > you'd early return in WCI::SetFocusedFrame when clicking on the <input> in the > > <webview>, and WCI::SetFocusedFrame is the only thing that can change > it (to > the > > inner WC). > Since the <webview> is using cross process frames, we will set it as focused > when we first click on the <input> inside the <webview>. The problem is by early > returning, we do not set the outer WebContents as focused when we click the > <input> in the app. > > > Was your example broken (when done without --use-cpffg) before you > fixed > that > > check in WebContentsImpl::SetFocusedFrame? > Without --ucpfg the app works fine. I think the reason is that no one ever > changes the focused WebContents in that case. Thanks for clarifying. What worried me is that if --use-cpffg is *off*, the old check in WebContentsImpl::SetFocusedFrame always returned early, but the new check only returns early for BrowserPlugin guests. It's not a big deal - things are still correct, but we end up doing a bit of extra work for the common case of WebContents without any embedders/guests when we don't need to. It seems the early return condition could instead be if (!GuestViewCrossProcessFrames_is_on_for_everything || this_wc_is_a_pdf_guest) (where this_wc_is_a_pdf_guest is the current condition) which should work with your example, the pdf case, and still return early when the GuestViewCrossProcessFrames trial is off. But I think it's fine to leave it as-is and just have the code behave the same way whether or not the trial is on.
https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/10 18:35:44, alexmos wrote: > On 2016/11/09 19:17:15, EhsanK wrote: > > > I'm assuming the example is without --use-cpffg? > > No, actually this is using --use-cpffg. BTW, the new way to enable ucpffg > > is to add --enable-features=GuestViewCrossProcessFrames. I also had > > --site-per-process but I believe it might be irrelevant. > > > > > What would change it to the inner WebContents in the first place? Seems > like > > > you'd early return in WCI::SetFocusedFrame when clicking on the <input> in > the > > > <webview>, and WCI::SetFocusedFrame is the only thing that can change > it > (to > > the > > > inner WC). > > Since the <webview> is using cross process frames, we will set it as focused > > when we first click on the <input> inside the <webview>. The problem is by > early > > returning, we do not set the outer WebContents as focused when we click the > > <input> in the app. > > > > > Was your example broken (when done without --use-cpffg) before you > fixed > > that > > > check in WebContentsImpl::SetFocusedFrame? > > Without --ucpfg the app works fine. I think the reason is that no one ever > > changes the focused WebContents in that case. > > Thanks for clarifying. What worried me is that if --use-cpffg is *off*, the old > check in WebContentsImpl::SetFocusedFrame always returned early, but the new > check only returns early for BrowserPlugin guests. It's not a big deal - things > are still correct, but we end up doing a bit of extra work for the common case > of WebContents without any embedders/guests when we don't need to. > > It seems the early return condition could instead be > if (!GuestViewCrossProcessFrames_is_on_for_everything || > this_wc_is_a_pdf_guest) > > (where this_wc_is_a_pdf_guest is the current condition) > which should work with your example, the pdf case, and still return early when > the GuestViewCrossProcessFrames trial is off. But I think it's fine to leave it > as-is and just have the code behave the same way whether or not the trial is on. You are right and comparing with the old code we are doing some extra work here. I actually knew this when I added the change but the reason was that after discussing with creis@ we decided to only expose one method for determining --ucpffg is on, and that method would take a WC, FWIW in the case the flag is off for all WCs, we always have (old_web_contents == this). Therefore, all we will do is to make two comparisons and also get the value of RWH from node, which I believe will not be too costly.
https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/10 18:35:44, alexmos wrote: > On 2016/11/09 19:17:15, EhsanK wrote: > > > I'm assuming the example is without --use-cpffg? > > No, actually this is using --use-cpffg. BTW, the new way to enable ucpffg > > is to add --enable-features=GuestViewCrossProcessFrames. I also had > > --site-per-process but I believe it might be irrelevant. > > > > > What would change it to the inner WebContents in the first place? Seems > like > > > you'd early return in WCI::SetFocusedFrame when clicking on the <input> in > the > > > <webview>, and WCI::SetFocusedFrame is the only thing that can change > it > (to > > the > > > inner WC). > > Since the <webview> is using cross process frames, we will set it as focused > > when we first click on the <input> inside the <webview>. The problem is by > early > > returning, we do not set the outer WebContents as focused when we click the > > <input> in the app. > > > > > Was your example broken (when done without --use-cpffg) before you > fixed > > that > > > check in WebContentsImpl::SetFocusedFrame? > > Without --ucpfg the app works fine. I think the reason is that no one ever > > changes the focused WebContents in that case. > > Thanks for clarifying. What worried me is that if --use-cpffg is *off*, the old > check in WebContentsImpl::SetFocusedFrame always returned early, but the new > check only returns early for BrowserPlugin guests. It's not a big deal - things > are still correct, but we end up doing a bit of extra work for the common case > of WebContents without any embedders/guests when we don't need to. > > It seems the early return condition could instead be > if (!GuestViewCrossProcessFrames_is_on_for_everything || > this_wc_is_a_pdf_guest) > > (where this_wc_is_a_pdf_guest is the current condition) > which should work with your example, the pdf case, and still return early when > the GuestViewCrossProcessFrames trial is off. But I think it's fine to leave it > as-is and just have the code behave the same way whether or not the trial is on. Coming back to this CL, I think this check is fine here given the discussion. Also, avallee@: can you check if the PDF case that Ehsan mentioned works properly, whether or not --use-cpffg is enabled? Specifically, typing into a form on a PDF, then typing in the embedder? https://codereview.chromium.org/2451143003/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:487: outermost->node_->SetFocusedWebContents(outermost); What happens if an inner contents with page focus is deleted? Does this need something similar to set page focus on the |outermost| contents?
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...
I believe I addressed all comments. I think that there are 3 patchsets included and it's getting confusing. Destructor might need to call outermoset->SetAsFocused() if this is focused or any child. PDF seems to work like everywhere else. The Pdf plugin pp::Instance::DidChangeFocus(bool) Addressed the ~WebContentsImpl to change focused to the outermost webcontents if not the outermost contents. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:696: delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; On 2016/10/31 18:57:57, alexmos wrote: > On 2016/10/28 19:19:56, avallee wrote: > > On 2016/10/28 06:36:39, alexmos wrote: > > > I think in the case where GetFocusedRenderWidgetHost returns null, we're > > > supposed to drop the event to avoid giving it to the wrong frame (see > comments > > > inside that function). Can you check how this works if the guest crashes > > while > > > having page focus, and then you get into here? > > > > The main place where Focus() is called is from the RenderWidgetHostView which > > only knows about the top level RenderWidget. If an internal RenderWidget dies, > I > > think it's fine to refocus the outermost RenderWidget when we refocus the > > window. > > It might not always be safe: a malicious embedder could load some sensitive > site in a <webview>, then crash it just as you start typing sensitive data > inside the <webview>. The embedder might then start getting the rest of the > data if we focus it, until user realizes that the keystrokes are going to the > wrong place? See > https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content... > for a discussion when this came up for OOPIFs. Would crashing the renderer lead a focus change? Currently, the embedder can just call element.focus() and steal from the webview anyways. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:699: focused_widget->FocusDirect(); On 2016/11/09 02:47:54, alexmos wrote: > On 2016/10/31 18:57:57, alexmos wrote: > > On 2016/10/28 19:19:56, avallee wrote: > > > On 2016/10/28 06:36:39, alexmos wrote: > > > > I'm trying to understand the implication of this change for regular OOPIF > > > cases. > > > > Say we have a page with frame A embedding frame B, and B is an OOPIF. If > > we > > > > focus frame A and then alt-tab back and forth, this will behave as before: > > > send > > > > InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy > in > > > > process B (thanks to ReplicatePageFocus). So both renderers get the page > > > focus > > > > messages. > > > > > > > > If we now focus frame B and then alt-tab back and forth, this seems to > > behave > > > > differently: GetFocusedRenderWidgetHost will return B's RWH, hence we will > > > send > > > > the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) > is > > > > false for that RWH, we don't replicate the page focus message to A. > > > > > > > > That will also cause the is_focused_ bit to become inconsistent for both > > main > > > > frame and subframe's RWHs, though I'm not sure what that entails. > > > > > > > > That doesn't seem consistent/desirable. Is the different behavior > > > intentional? > > > > Was this meant to only apply to the RWH for embedders/guests, or to > > subframes > > > as > > > > well? > > > > > > I believe that GetFocusedWebContents()->GetMainFrame() is the right thing to > > do > > > here. > > > > Yes, that looks better. However, are we sure that this can't ever be called > on > > a non-main-frame widget? At first glance in codesearch, most callsites get > here > > via smth like GetRenderViewHost()->GetWidget()->Focus(), which implies main > > frame widget, but I didn't check all the paths. I'm especially wondering if > > this is ever called for fullscreen RWHs (especially Flash fullscreen) and/or > > popup menu widgets (i.e., <select> dropdowns). Do you mind double-checking > > those cases? See https://crbug.com/593522 for some examples of Flash > > fullscreen. I'm curious if we need to keep the old behavior of just sending > > InputMsg_SetFocus to the RWH itself for those cases. > > Still curious about this. Might be worth trying putting a DCHECK here and in > Blur(), to see if we ever have a focused_widget != this, yet they point to the > same delegates. I'm curious if this breaks any tests. This DCHECK would fail if a webview is focused. Once the webview is supposed to get keyboard input, if we blur or focus the embedder's window we would come here, this == embedder, focused == webview. As for flash and select, I messed around in a webview and they seem to work. Losing window focus closes the full screen flash and select widget. Coming back leave the flash or select element as the focused element on the page. https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/10 19:16:55, alexmos wrote: > On 2016/11/10 18:35:44, alexmos wrote: > > On 2016/11/09 19:17:15, EhsanK wrote: > > > > I'm assuming the example is without --use-cpffg? > > > No, actually this is using --use-cpffg. BTW, the new way to enable ucpffg > > > is to add --enable-features=GuestViewCrossProcessFrames. I also had > > > --site-per-process but I believe it might be irrelevant. > > > > > > > What would change it to the inner WebContents in the first place? Seems > > like > > > > you'd early return in WCI::SetFocusedFrame when clicking on the <input> in > > the > > > > <webview>, and WCI::SetFocusedFrame is the only thing that can change > it > > (to > > > the > > > > inner WC). > > > Since the <webview> is using cross process frames, we will set it as focused > > > when we first click on the <input> inside the <webview>. The problem is by > > early > > > returning, we do not set the outer WebContents as focused when we click the > > > <input> in the app. > > > > > > > Was your example broken (when done without --use-cpffg) before you > > fixed > > > that > > > > check in WebContentsImpl::SetFocusedFrame? > > > Without --ucpfg the app works fine. I think the reason is that no one ever > > > changes the focused WebContents in that case. > > > > Thanks for clarifying. What worried me is that if --use-cpffg is *off*, the > old > > check in WebContentsImpl::SetFocusedFrame always returned early, but the new > > check only returns early for BrowserPlugin guests. It's not a big deal - > things > > are still correct, but we end up doing a bit of extra work for the common case > > of WebContents without any embedders/guests when we don't need to. > > > > It seems the early return condition could instead be > > if (!GuestViewCrossProcessFrames_is_on_for_everything || > > this_wc_is_a_pdf_guest) > > > > (where this_wc_is_a_pdf_guest is the current condition) > > which should work with your example, the pdf case, and still return early when > > the GuestViewCrossProcessFrames trial is off. But I think it's fine to leave > it > > as-is and just have the code behave the same way whether or not the trial is > on. > > Coming back to this CL, I think this check is fine here given the discussion. > Also, avallee@: can you check if the PDF case that Ehsan mentioned works > properly, whether or not --use-cpffg is enabled? Specifically, typing into a > form on a PDF, then typing in the embedder? I tested http://foersom.com/net/HowTo/data/OoPdfFormExample.pdf in the browser sample in each of: no flags, --site-per-process and --use-cpffg (i did not sync past lfg's flag cl yet). Input seems to work correctly, but in all cases the pdf does not blur on window blur. https://codereview.chromium.org/2451143003/diff/160001/content/browser/frame_... File content/browser/frame_host/frame_tree.cc (left): https://codereview.chromium.org/2451143003/diff/160001/content/browser/frame_... content/browser/frame_host/frame_tree.cc:276: root()->current_frame_host()->UpdateAXTreeData(); On 2016/11/09 02:47:54, alexmos wrote: > Does this need to be called somewhere else, now that this path is gone? (In any > case, sounds like it's a separate issue for a future followup.) This was for the clear frame case, this was essentially a dupe of what happens at the bottom of this function. https://codereview.chromium.org/2451143003/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:487: outermost->node_->SetFocusedWebContents(outermost); On 2016/11/10 19:16:55, alexmos wrote: > What happens if an inner contents with page focus is deleted? Does this need > something similar to set page focus on the |outermost| contents? It feels like it should focus its outer contents, not necessarily the outermost contents. I'm also unsure about deletion order. Could an outer contents be deleted before the inner contents?
I believe I addressed all comments. I think that there are 3 patchsets included and it's getting confusing. Destructor might need to call outermoset->SetAsFocused() if this is focused or any child. PDF seems to work like everywhere else. The Pdf plugin pp::Instance::DidChangeFocus(bool) Addressed the ~WebContentsImpl to change focused to the outermost webcontents if not the outermost contents.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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: - ekaramad@chromium.org, nasko@chromium.org
avallee@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
nick or creis: Can you review content/. I'd like to get this in before branch. Thanks.
LGTM for webview_interactive_browser_test
Hmm, this seems to be making the focus logic even harder to follow, at least for someone unfamiliar with it (like me). I'll defer to alexmos for whether this makes sense, but I'm concerned that the concepts involved aren't clear. The phrasing is making me concerned about changes to focus in general, which is risky to do right before branch. Hopefully I'm misunderstanding the implications and we can just clarify the changes. https://codereview.chromium.org/2451143003/diff/200001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2451143003/diff/200001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1401: // and restored on focus nit: End with period. https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:150: // focused WebContents. I'm having trouble understanding this vs GetFocusedRenderWidgetHost, since I'm not familiar with page level focus events. Why is knowing the focused widget the wrong thing? Can you elaborate in the comment? https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:157: virtual void EnsureOwningContentsIsFocused( We should try to be more direct in the name, since it's not clear what action "Ensure" is taking. Would FocusOwningWebContents be accurate? https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:743: (focused_widget ? focused_widget : this)->SetPageFocus(true); Style nit: Let's avoid using ternary operators inside an expression like this. Let's just add this before: if (!focused_widget) focused_widget = this; Same below. https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4715: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) What's this early return about? Can you clarify with a comment? https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1026: void SetAsFocusedWebContentsIfNecessary(); This seems really subtle to me, and I'm concerned about landing a major change to focus behavior just before branch. It sounds like it affects how all tabs are focused. Is it actually more specific to webviews (i.e., inner WebContents)? Is there a way to clarify if so?
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...
creis: Responded to your comments. https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:697: focused_widget = delegate_->GetFocusedRenderWidgetHost(this); On 2016/10/27 18:41:01, wjmaclean wrote: > Minor suggestion: > > RenderWidgetHostImpl* focused_widget = > delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; csreis asked for this to be reverted in https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... https://codereview.chromium.org/2451143003/diff/200001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2451143003/diff/200001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1401: // and restored on focus On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > nit: End with period. Done. https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:150: // focused WebContents. On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > I'm having trouble understanding this vs GetFocusedRenderWidgetHost, since I'm > not familiar with page level focus events. Why is knowing the focused widget > the wrong thing? Can you elaborate in the comment? See the <webview> focus explainer for extra details. https://docs.google.com/document/d/1ysPHsls5CFRhIfW19p8uYRXtVpSwKt6ZY39IX_6AnjY/ The point is that in a guest view case for example <webview> either the webview is active or the embedder is. Each is considered a different page, they don't really have insight into each other's content. ekaramad has a tabbed <webview> browser and when switching from tab to tab, the old webview should receive events to tell it that the page is no longer focused, and the new page will receive focus. This is used by RenderWidgetHostImpl::Focus/Blur to forward the event to the RenderWidgetHostImpl that is currently "active" or have page focus. The main frame widget for a single WebContents in a tree of WebContents will be focused. Knowing the focused widget is the wrong thing since this might be the widget for some oopif but in the same WebContents, the single WebContents case is already handled and works. https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:157: virtual void EnsureOwningContentsIsFocused( On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > We should try to be more direct in the name, since it's not clear what action > "Ensure" is taking. Would FocusOwningWebContents be accurate? Good idea. Done. https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:743: (focused_widget ? focused_widget : this)->SetPageFocus(true); On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > Style nit: Let's avoid using ternary operators inside an expression like this. > Let's just add this before: > if (!focused_widget) > focused_widget = this; > > Same below. wjmaclean asked for this on https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... . Reverted. https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4715: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > What's this early return about? Can you clarify with a comment? See discussion here: https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... Commend added. https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1026: void SetAsFocusedWebContentsIfNecessary(); On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > This seems really subtle to me, and I'm concerned about landing a major change > to focus behavior just before branch. > > It sounds like it affects how all tabs are focused. Is it actually more > specific to webviews (i.e., inner WebContents)? Is there a way to clarify if > so? The concept of a "Focused" WebContents is for guest views, when there are inner/outer WebContents. A single WebContents is focused and the normal routing for keyboard input will search that WebContents for the focused frame/widget. Updated comment.
Thanks for the explanations-- given that this is specific to the GuestView case, I'm less concerned. I'm happy to rubber stamp once alexmos approves. (A few followup requests below.) https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:697: focused_widget = delegate_->GetFocusedRenderWidgetHost(this); On 2016/11/16 21:18:10, avallee wrote: > On 2016/10/27 18:41:01, wjmaclean wrote: > > Minor suggestion: > > > > RenderWidgetHostImpl* focused_widget = > > delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; > > csreis asked for this to be reverted in > https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... Maybe there's a misunderstanding? I'm fine with wjmaclean's suggestion here about using a ternary operator in general. I was just asking not to use it within a parenthetical phrase that we then call a method on. https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:150: // focused WebContents. On 2016/11/16 21:18:10, avallee wrote: > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > I'm having trouble understanding this vs GetFocusedRenderWidgetHost, since I'm > > not familiar with page level focus events. Why is knowing the focused widget > > the wrong thing? Can you elaborate in the comment? > > See the <webview> focus explainer for extra details. > https://docs.google.com/document/d/1ysPHsls5CFRhIfW19p8uYRXtVpSwKt6ZY39IX_6AnjY/ > > The point is that in a guest view case for example <webview> either the webview > is active or the embedder is. Each is considered a different page, they don't > really have insight into each other's content. ekaramad has a tabbed <webview> > browser and when switching from tab to tab, the old webview should receive > events to tell it that the page is no longer focused, and the new page will > receive focus. > > > This is used by RenderWidgetHostImpl::Focus/Blur to forward the event to the > RenderWidgetHostImpl that is currently "active" or have page focus. > > The main frame widget for a single WebContents in a tree of WebContents will be > focused. > > Knowing the focused widget is the wrong thing since this might be the widget for > some oopif but in the same WebContents, the single WebContents case is already > handled and works. Ok. I think more of this needs to come across in the comment in the code, since these methods don't sound like they're specific to the inner/outer WebContents case. Maybe the idea from web_contents_impl.h of having a larger explanation for these focus methods would help here as well? https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4715: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/16 21:18:10, avallee wrote: > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > What's this early return about? Can you clarify with a comment? > > See discussion here: > https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... > > Commend added. Thanks for adding the comment. https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1026: void SetAsFocusedWebContentsIfNecessary(); On 2016/11/16 21:18:10, avallee wrote: > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > This seems really subtle to me, and I'm concerned about landing a major change > > to focus behavior just before branch. > > > > It sounds like it affects how all tabs are focused. Is it actually more > > specific to webviews (i.e., inner WebContents)? Is there a way to clarify if > > so? > > The concept of a "Focused" WebContents is for guest views, when there are > inner/outer WebContents. A single WebContents is focused and the normal routing > for keyboard input will search that WebContents for the focused frame/widget. > > Updated comment. Ok, I'm glad it's limited to that case. I think it's worth clarifying this in the code at some point, like having a section (similar to the "Navigation helpers" one below) for focus that explains how this focus concept is different from other places focus is handled.
https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:743: (focused_widget ? focused_widget : this)->SetPageFocus(true); On 2016/11/16 21:18:10, avallee wrote: > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > Style nit: Let's avoid using ternary operators inside an expression like this. > > > Let's just add this before: > > if (!focused_widget) > > focused_widget = this; > > > > Same below. > > wjmaclean asked for this on > https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... > . Reverted. It would make sense, in this case, to do RenderWidgetHostImpl* focused_widget = delegate_ ? delegate_->GetRenderWidgetHostWithPageFocus() : this; focused_widget->SetPageFocus(false); N'est-ce pas?
https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:743: (focused_widget ? focused_widget : this)->SetPageFocus(true); On 2016/11/16 21:57:09, wjmaclean wrote: > On 2016/11/16 21:18:10, avallee wrote: > > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > > Style nit: Let's avoid using ternary operators inside an expression like > this. > > > > > Let's just add this before: > > > if (!focused_widget) > > > focused_widget = this; > > > > > > Same below. > > > > wjmaclean asked for this on > > > https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... > > . Reverted. > > It would make sense, in this case, to do > > RenderWidgetHostImpl* focused_widget = > delegate_ ? delegate_->GetRenderWidgetHostWithPageFocus() : this; > focused_widget->SetPageFocus(false); > > N'est-ce pas? Only if GetRenderWidgetHostWithPageFocus() can never return null. :) Looks like at least the default impl returns null, though.
On 2016/11/16 22:00:57, Charlie Reis (slow) wrote: > https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... > content/browser/renderer_host/render_widget_host_impl.cc:743: (focused_widget ? > focused_widget : this)->SetPageFocus(true); > On 2016/11/16 21:57:09, wjmaclean wrote: > > On 2016/11/16 21:18:10, avallee wrote: > > > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > > > Style nit: Let's avoid using ternary operators inside an expression like > > this. > > > > > > > Let's just add this before: > > > > if (!focused_widget) > > > > focused_widget = this; > > > > > > > > Same below. > > > > > > wjmaclean asked for this on > > > > > > https://codereview.chromium.org/2451143003/diff/60001/content/browser/rendere... > > > . Reverted. > > > > It would make sense, in this case, to do > > > > RenderWidgetHostImpl* focused_widget = > > delegate_ ? delegate_->GetRenderWidgetHostWithPageFocus() : this; > > focused_widget->SetPageFocus(false); > > > > N'est-ce pas? > > Only if GetRenderWidgetHostWithPageFocus() can never return null. :) Looks > like at least the default impl returns null, though. Ahh, I was assuming GetRenderWidgetHostWithPageFocus() would return non-null ... I did not read the implementation though :-)
LGTM once the latest set of comments (including ones from Charlie) are addressed. Thanks for working through all this and for adding the test! https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:696: delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr; On 2016/11/16 05:38:09, avallee wrote: > On 2016/10/31 18:57:57, alexmos wrote: > > On 2016/10/28 19:19:56, avallee wrote: > > > On 2016/10/28 06:36:39, alexmos wrote: > > > > I think in the case where GetFocusedRenderWidgetHost returns null, we're > > > > supposed to drop the event to avoid giving it to the wrong frame (see > > comments > > > > inside that function). Can you check how this works if the guest crashes > > > while > > > > having page focus, and then you get into here? > > > > > > The main place where Focus() is called is from the RenderWidgetHostView > which > > > only knows about the top level RenderWidget. If an internal RenderWidget > dies, > > I > > > think it's fine to refocus the outermost RenderWidget when we refocus the > > > window. > > > > It might not always be safe: a malicious embedder could load some sensitive > > site in a <webview>, then crash it just as you start typing sensitive data > > inside the <webview>. The embedder might then start getting the rest of the > > data if we focus it, until user realizes that the keystrokes are going to the > > wrong place? See > > > https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content... > > for a discussion when this came up for OOPIFs. > > Would crashing the renderer lead a focus change? Currently, the embedder can > just call element.focus() and steal from the webview anyways. I'm not sure -- that's why I was curious if you could try it. :) But given your latter point, I think the current code is ok. https://codereview.chromium.org/2451143003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:699: focused_widget->FocusDirect(); On 2016/11/16 05:38:09, avallee wrote: > On 2016/11/09 02:47:54, alexmos wrote: > > On 2016/10/31 18:57:57, alexmos wrote: > > > On 2016/10/28 19:19:56, avallee wrote: > > > > On 2016/10/28 06:36:39, alexmos wrote: > > > > > I'm trying to understand the implication of this change for regular > OOPIF > > > > cases. > > > > > Say we have a page with frame A embedding frame B, and B is an OOPIF. > If > > > we > > > > > focus frame A and then alt-tab back and forth, this will behave as > before: > > > > send > > > > > InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy > > in > > > > > process B (thanks to ReplicatePageFocus). So both renderers get the > page > > > > focus > > > > > messages. > > > > > > > > > > If we now focus frame B and then alt-tab back and forth, this seems to > > > behave > > > > > differently: GetFocusedRenderWidgetHost will return B's RWH, hence we > will > > > > send > > > > > the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) > > is > > > > > false for that RWH, we don't replicate the page focus message to A. > > > > > > > > > > That will also cause the is_focused_ bit to become inconsistent for both > > > main > > > > > frame and subframe's RWHs, though I'm not sure what that entails. > > > > > > > > > > That doesn't seem consistent/desirable. Is the different behavior > > > > intentional? > > > > > Was this meant to only apply to the RWH for embedders/guests, or to > > > subframes > > > > as > > > > > well? > > > > > > > > I believe that GetFocusedWebContents()->GetMainFrame() is the right thing > to > > > do > > > > here. > > > > > > Yes, that looks better. However, are we sure that this can't ever be called > > on > > > a non-main-frame widget? At first glance in codesearch, most callsites get > > here > > > via smth like GetRenderViewHost()->GetWidget()->Focus(), which implies main > > > frame widget, but I didn't check all the paths. I'm especially wondering if > > > this is ever called for fullscreen RWHs (especially Flash fullscreen) and/or > > > popup menu widgets (i.e., <select> dropdowns). Do you mind double-checking > > > those cases? See https://crbug.com/593522 for some examples of Flash > > > fullscreen. I'm curious if we need to keep the old behavior of just sending > > > InputMsg_SetFocus to the RWH itself for those cases. > > > > Still curious about this. Might be worth trying putting a DCHECK here and in > > Blur(), to see if we ever have a focused_widget != this, yet they point to the > > same delegates. I'm curious if this breaks any tests. > > This DCHECK would fail if a webview is focused. Once the webview is supposed to > get keyboard input, if we blur or focus the embedder's window we would come > here, this == embedder, focused == webview. > > As for flash and select, I messed around in a webview and they seem to work. > Losing window focus closes the full screen flash and select widget. Coming back > leave the flash or select element as the focused element on the page. Sorry, the DCHECK I had in mind was assuming a single WebContents. I.e., something like DCHECK(focused_widget == this || focused_widget->delegate() != delegate_). This would check that if we can't get here for a non-main-frame widget in a single WebContents (because otherwise, we'd change behavior and send the IPC to a different widget). https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4717: if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) On 2016/11/16 05:38:09, avallee wrote: > On 2016/11/10 19:16:55, alexmos wrote: > > On 2016/11/10 18:35:44, alexmos wrote: > > > On 2016/11/09 19:17:15, EhsanK wrote: > > > > > I'm assuming the example is without --use-cpffg? > > > > No, actually this is using --use-cpffg. BTW, the new way to enable ucpffg > > > > is to add --enable-features=GuestViewCrossProcessFrames. I also had > > > > --site-per-process but I believe it might be irrelevant. > > > > > > > > > What would change it to the inner WebContents in the first place? Seems > > > like > > > > > you'd early return in WCI::SetFocusedFrame when clicking on the <input> > in > > > the > > > > > <webview>, and WCI::SetFocusedFrame is the only thing that can change > > it > > > (to > > > > the > > > > > inner WC). > > > > Since the <webview> is using cross process frames, we will set it as > focused > > > > when we first click on the <input> inside the <webview>. The problem is by > > > early > > > > returning, we do not set the outer WebContents as focused when we click > the > > > > <input> in the app. > > > > > > > > > Was your example broken (when done without --use-cpffg) before you > > > fixed > > > > that > > > > > check in WebContentsImpl::SetFocusedFrame? > > > > Without --ucpfg the app works fine. I think the reason is that no one ever > > > > changes the focused WebContents in that case. > > > > > > Thanks for clarifying. What worried me is that if --use-cpffg is *off*, the > > old > > > check in WebContentsImpl::SetFocusedFrame always returned early, but the new > > > check only returns early for BrowserPlugin guests. It's not a big deal - > > things > > > are still correct, but we end up doing a bit of extra work for the common > case > > > of WebContents without any embedders/guests when we don't need to. > > > > > > It seems the early return condition could instead be > > > if (!GuestViewCrossProcessFrames_is_on_for_everything || > > > this_wc_is_a_pdf_guest) > > > > > > (where this_wc_is_a_pdf_guest is the current condition) > > > which should work with your example, the pdf case, and still return early > when > > > the GuestViewCrossProcessFrames trial is off. But I think it's fine to > leave > > it > > > as-is and just have the code behave the same way whether or not the trial is > > on. > > > > Coming back to this CL, I think this check is fine here given the discussion. > > Also, avallee@: can you check if the PDF case that Ehsan mentioned works > > properly, whether or not --use-cpffg is enabled? Specifically, typing into a > > form on a PDF, then typing in the embedder? > > I tested http://foersom.com/net/HowTo/data/OoPdfFormExample.pdf in the browser > sample in each of: no flags, --site-per-process and --use-cpffg (i did not sync > past lfg's flag cl yet). > > Input seems to work correctly, but in all cases the pdf does not blur on window > blur. Acknowledged. https://codereview.chromium.org/2451143003/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:487: outermost->node_->SetFocusedWebContents(outermost); On 2016/11/16 05:38:09, avallee wrote: > On 2016/11/10 19:16:55, alexmos wrote: > > What happens if an inner contents with page focus is deleted? Does this need > > something similar to set page focus on the |outermost| contents? > > It feels like it should focus its outer contents, not necessarily the outermost > contents. > > I'm also unsure about deletion order. Could an outer contents be deleted before > the inner contents? I'd double-check on that with Lucas or Ehsan. I'm unclear on that myself (see comment in https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...). What you have right now seems reasonable/safe for me, though we may not need it if it's impossible for an outer contents to be deleted before the inner contents. https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:150: // focused WebContents. On 2016/11/16 21:51:19, Charlie Reis (slow) wrote: > On 2016/11/16 21:18:10, avallee wrote: > > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > > I'm having trouble understanding this vs GetFocusedRenderWidgetHost, since > I'm > > > not familiar with page level focus events. Why is knowing the focused > widget > > > the wrong thing? Can you elaborate in the comment? > > > > See the <webview> focus explainer for extra details. > > > https://docs.google.com/document/d/1ysPHsls5CFRhIfW19p8uYRXtVpSwKt6ZY39IX_6AnjY/ > > > > The point is that in a guest view case for example <webview> either the > webview > > is active or the embedder is. Each is considered a different page, they don't > > really have insight into each other's content. ekaramad has a tabbed <webview> > > browser and when switching from tab to tab, the old webview should receive > > events to tell it that the page is no longer focused, and the new page will > > receive focus. > > > > > > This is used by RenderWidgetHostImpl::Focus/Blur to forward the event to the > > RenderWidgetHostImpl that is currently "active" or have page focus. > > > > The main frame widget for a single WebContents in a tree of WebContents will > be > > focused. > > > > Knowing the focused widget is the wrong thing since this might be the widget > for > > some oopif but in the same WebContents, the single WebContents case is already > > handled and works. > > Ok. I think more of this needs to come across in the comment in the code, since > these methods don't sound like they're specific to the inner/outer WebContents > case. Maybe the idea from web_contents_impl.h of having a larger explanation > for these focus methods would help here as well? +1 to making all relevant comments emphasize that the main use case is for inner/outer WebContents. https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1400: // Ensures that input is not routed to the webview when the container is blurred nit: should this be: input *is* routed to the webview after the container loses and then gains page focus? (i.e., reverse the meaning?) https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1401: // and restored on focus nit: include bug reference. https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1445: ASSERT_TRUE(ui_test_utils::ShowAndFocusNativeWindow(GetPlatformAppWindow())); I'm curious why this only needs to be done on Mac? Seems like something reasonable for all platforms to ensure first window is focused again. If it's something Mac-specific, then please add a comment. https://codereview.chromium.org/2451143003/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:487: outermost->SetAsFocusedWebContentsIfNecessary(); This will now end up calling old_contents->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(false) where old_contents is the contents being destroyed. This SetPageFocus isn't really necessary, but I also wonder if the main frame RFH and its RWH are still around or if we've deleted them by now, in which case this would be UAF? This really only needs to do two things: outermost->node_->SetFocusedWebContents(outermost); outermost->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(true);
https://codereview.chromium.org/2451143003/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:487: outermost->SetAsFocusedWebContentsIfNecessary(); On 2016/11/16 23:29:03, alexmos wrote: > This will now end up calling > old_contents->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(false) > > where old_contents is the contents being destroyed. This SetPageFocus isn't > really necessary, but I also wonder if the main frame RFH and its RWH are still > around or if we've deleted them by now, in which case this would be UAF? > > This really only needs to do two things: > outermost->node_->SetFocusedWebContents(outermost); > outermost->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(true); To follow up, looks like the destruction will happen as ~WebContentsImpl, then ~RFH, then ~RWH. So I think your original call is fine.
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...
Comments addressed. https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1026: void SetAsFocusedWebContentsIfNecessary(); On 2016/11/16 21:51:20, Charlie Reis (slow) wrote: > On 2016/11/16 21:18:10, avallee wrote: > > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > > This seems really subtle to me, and I'm concerned about landing a major > change > > > to focus behavior just before branch. > > > > > > It sounds like it affects how all tabs are focused. Is it actually more > > > specific to webviews (i.e., inner WebContents)? Is there a way to clarify > if > > > so? > > > > The concept of a "Focused" WebContents is for guest views, when there are > > inner/outer WebContents. A single WebContents is focused and the normal > routing > > for keyboard input will search that WebContents for the focused frame/widget. > > > > Updated comment. > > Ok, I'm glad it's limited to that case. I think it's worth clarifying this in > the code at some point, like having a section (similar to the "Navigation > helpers" one below) for focus that explains how this focus concept is different > from other places focus is handled. Added comment header block. https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1400: // Ensures that input is not routed to the webview when the container is blurred On 2016/11/16 23:29:03, alexmos wrote: > nit: should this be: input *is* routed to the webview after the container loses > and then gains page focus? (i.e., reverse the meaning?) Well it tests both. It does not respond while another window is focused even though we route keypresses directly. Changed. https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1401: // and restored on focus On 2016/11/16 23:29:03, alexmos wrote: > nit: include bug reference. Done. https://codereview.chromium.org/2451143003/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1445: ASSERT_TRUE(ui_test_utils::ShowAndFocusNativeWindow(GetPlatformAppWindow())); On 2016/11/16 23:29:03, alexmos wrote: > I'm curious why this only needs to be done on Mac? Seems like something > reasonable for all platforms to ensure first window is focused again. If it's > something Mac-specific, then please add a comment. I initially had this when I was having synchronization problems. On other OSes it was not necessary after all, but on MacOS it appears that you can have open windows with none being focused (such as after a window closes itself). ifdef removed. https://codereview.chromium.org/2451143003/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2451143003/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:487: outermost->SetAsFocusedWebContentsIfNecessary(); On 2016/11/16 23:29:03, alexmos wrote: > This will now end up calling > old_contents->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(false) > > where old_contents is the contents being destroyed. This SetPageFocus isn't > really necessary, but I also wonder if the main frame RFH and its RWH are still > around or if we've deleted them by now, in which case this would be UAF? > > This really only needs to do two things: > outermost->node_->SetFocusedWebContents(outermost); > outermost->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(true); Ack, not changing. I could always add a !is_being_destroyed in SetAsFocusedWebContents().
Thanks! LGTM with nits. https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2451143003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:150: // focused WebContents. On 2016/11/16 23:29:03, alexmos wrote: > On 2016/11/16 21:51:19, Charlie Reis (slow) wrote: > > On 2016/11/16 21:18:10, avallee wrote: > > > On 2016/11/16 20:28:23, Charlie Reis (slow) wrote: > > > > I'm having trouble understanding this vs GetFocusedRenderWidgetHost, since > > I'm > > > > not familiar with page level focus events. Why is knowing the focused > > widget > > > > the wrong thing? Can you elaborate in the comment? > > > > > > See the <webview> focus explainer for extra details. > > > > > > https://docs.google.com/document/d/1ysPHsls5CFRhIfW19p8uYRXtVpSwKt6ZY39IX_6AnjY/ > > > > > > The point is that in a guest view case for example <webview> either the > > webview > > > is active or the embedder is. Each is considered a different page, they > don't > > > really have insight into each other's content. ekaramad has a tabbed > <webview> > > > browser and when switching from tab to tab, the old webview should receive > > > events to tell it that the page is no longer focused, and the new page will > > > receive focus. > > > > > > > > > This is used by RenderWidgetHostImpl::Focus/Blur to forward the event to the > > > RenderWidgetHostImpl that is currently "active" or have page focus. > > > > > > The main frame widget for a single WebContents in a tree of WebContents will > > be > > > focused. > > > > > > Knowing the focused widget is the wrong thing since this might be the widget > > for > > > some oopif but in the same WebContents, the single WebContents case is > already > > > handled and works. > > > > Ok. I think more of this needs to come across in the comment in the code, > since > > these methods don't sound like they're specific to the inner/outer WebContents > > case. Maybe the idea from web_contents_impl.h of having a larger explanation > > for these focus methods would help here as well? > > +1 to making all relevant comments emphasize that the main use case is for > inner/outer WebContents. Looks like this was overlooked. https://codereview.chromium.org/2451143003/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2451143003/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:151: virtual RenderWidgetHostImpl* GetRenderWidgetHostWithPageFocus(); I think you missed my request to group these in a block explaining that they're for inner/outer WebContents (similar to web_contents.h). https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1009: // Guest View Helpers -------------------------------------------------------- nit: GuestView isn't a concept within content, so we refer to these as Inner WebContents. https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1011: // These functions are helpers in managing a hierharchy of WebContents nit: hierarchy https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1012: // involved in rendering Guest Views. nit: rendering inner WebContents. https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1014: // When multiple WebContents are present, a single one is focused and will nit: present within a tab?
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...
https://codereview.chromium.org/2451143003/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2451143003/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:151: virtual RenderWidgetHostImpl* GetRenderWidgetHostWithPageFocus(); On 2016/11/17 00:49:48, Charlie Reis (slow) wrote: > I think you missed my request to group these in a block explaining that they're > for inner/outer WebContents (similar to web_contents.h). Sorry, missed that. Done. https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1009: // Guest View Helpers -------------------------------------------------------- On 2016/11/17 00:49:48, Charlie Reis (slow) wrote: > nit: GuestView isn't a concept within content, so we refer to these as Inner > WebContents. Done. https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1011: // These functions are helpers in managing a hierharchy of WebContents On 2016/11/17 00:49:48, Charlie Reis (slow) wrote: > nit: hierarchy Done. https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1012: // involved in rendering Guest Views. On 2016/11/17 00:49:48, Charlie Reis (slow) wrote: > nit: rendering inner WebContents. Done. https://codereview.chromium.org/2451143003/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1014: // When multiple WebContents are present, a single one is focused and will On 2016/11/17 00:49:48, Charlie Reis (slow) wrote: > nit: present within a tab? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org, wjmaclean@chromium.org, alexmos@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2451143003/#ps260001 (title: "Fix creis comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== <webview>: Correctly shift focus between WebContents. http://crrev.com/408445 Fixes some cases of keyboard input but required a renderer initiated frame focus change in order to do so. This change assumes that the browser is the primary driver of WebContents level focus changes. The leftover FrameHost*::OnFrameFocused is left so that an embedder can script aWebview.focus() to give focus. BUG=660044 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== <webview>: Correctly shift focus between WebContents. http://crrev.com/408445 Fixes some cases of keyboard input but required a renderer initiated frame focus change in order to do so. This change assumes that the browser is the primary driver of WebContents level focus changes. The leftover FrameHost*::OnFrameFocused is left so that an embedder can script aWebview.focus() to give focus. BUG=660044 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== <webview>: Correctly shift focus between WebContents. http://crrev.com/408445 Fixes some cases of keyboard input but required a renderer initiated frame focus change in order to do so. This change assumes that the browser is the primary driver of WebContents level focus changes. The leftover FrameHost*::OnFrameFocused is left so that an embedder can script aWebview.focus() to give focus. BUG=660044 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== <webview>: Correctly shift focus between WebContents. http://crrev.com/408445 Fixes some cases of keyboard input but required a renderer initiated frame focus change in order to do so. This change assumes that the browser is the primary driver of WebContents level focus changes. The leftover FrameHost*::OnFrameFocused is left so that an embedder can script aWebview.focus() to give focus. BUG=660044 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/9993fca0084ba71c628918b384ac701fe6514e44 Cr-Commit-Position: refs/heads/master@{#432781} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/9993fca0084ba71c628918b384ac701fe6514e44 Cr-Commit-Position: refs/heads/master@{#432781} |
