Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1934703002:
Fix keyboard focus for OOPIF-<webview>. (Closed)
|
Created:
4 years, 7 months ago by avallee Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org, ncarter (slow), Charlie Reis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix keyboard focus for OOPIF-<webview>.
When using --use-cross-process-frames-for-guests input events were being
routed to the embedder which should not be able to process guests' input
events.
+ WebContents tree traversal from parent to child.
+ WebContents keeps global track of focused frame, unlike frame tree
which might not know about inner/outer web contents.
+ RenderFrameHostImpl::OnFrameFocused delegates back to WebContents
instead of frame tree.
+ Send input to Webcontents that is focused.
+ Add unfocus message to renderer. When moving focus from a frame in the
webview back to the embedder, the inner contents no longer has any focus
(the frame with focus is not visible to the inner guest).
~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere.
~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order.
~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility.
~ Reworked RenderWidgetHostViewGuest to use
MaybeSendSyntheticTapGesture for Touch and Mouse event.
~ Edited SelectShowHide for direct routing.
BUG=600395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a
Cr-Commit-Position: refs/heads/master@{#408445}
Patch Set 1 #Patch Set 2 : Ready for review: Fix works with oopif webview but does not fix crbug.com/609903 #
Total comments: 33
Patch Set 3 : Address comments. Fix tab focus change. #
Total comments: 67
Patch Set 4 : Respond to alexmos@ #
Total comments: 3
Patch Set 5 : Blink ASSERT was missing.. #
Total comments: 7
Patch Set 6 : Simplify approach based on alexmos feedback. #
Total comments: 25
Patch Set 7 : Added test and addressed comments. #
Total comments: 17
Patch Set 8 : Rebase ToT #
Total comments: 2
Patch Set 9 : Fixed test flakiness and comments in PS 7,8. #
Total comments: 11
Patch Set 10 : Fixed tests, pass in all of {--use-cross-process-frames-for-guests,}{--site-per-process,} and comme… #Patch Set 11 : Fix focus issue in browserplugin with direct routing. #Patch Set 12 : Fixed WebViewTests/WebViewTest.SelectShowHide/1 browser_test #
Total comments: 6
Patch Set 13 : Add comment as suggested my wjmaclean. #Patch Set 14 : Fix alexmos nits. #
Total comments: 1
Patch Set 15 : Address dcheng@ comments #Patch Set 16 : Missed comment from PS9 #Patch Set 17 : Now with 80% less built breakage. #
Total comments: 2
Patch Set 18 : Added comment requested by dcheng. #
Total comments: 4
Patch Set 19 : Address nasko@ comments. #Messages
Total messages: 90 (39 generated)
Description was changed from ========== WIP: Fix webview focus. + WebContents tree can now traverse towards leaves. + Send input to Webcontents that is focused. - Page focus seems wrong. BUG=600395 ========== to ========== WIP: Fix webview focus. + WebContents tree can now traverse towards leaves. + Send input to Webcontents that is focused. - Page focus seems wrong. BUG=600395 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
avallee@chromium.org changed reviewers: + kenrb@chromium.org, lfg@chromium.org
avallee@chromium.org changed reviewers: + kenrb@chromium.org, lfg@chromium.org
lfg@ Care to comment before I send out more widely. kenrb@ IPC to unset focus.
lfg@ Care to comment before I send out more widely. kenrb@ IPC to unset focus.
Description was changed from ========== WIP: Fix webview focus. + WebContents tree can now traverse towards leaves. + Send input to Webcontents that is focused. - Page focus seems wrong. BUG=600395 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). BUG=600395 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Just a few comments. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:255: if (GetFocusedFrame()) { Nit: no need for {}. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:259: // FIXME: Do we need to update the AX tree here? Check with dmazzoni@ if this is necessary. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:284: if (proxy) { nit: {} https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:183: virtual void SetFocusedFrame(FrameTreeNode* node, SiteInstance* source){}; Add a comment with a description of the interface. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:419: // focus inside a <webview> when focus in the embedder goes from the webview Let's use inner WebContents instead of <webview>, since it's the generic terminology used in content/. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:353: for (auto children : inner_web_contents_tree_nodes_) Use auto&, so you don't make a copy of the pair to the stack. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4517: if (rwh) { nit: {} https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4539: if (inner_contents) { nit: {} https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4554: // maybe focus outer main frame. Not sure I follow this. Why are we focusing the root? https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4560: outer_node->current_frame_host()->GetSiteInstance(); I think this should be outer_node->parent()->current_frame_host(), since the outer_node shouldn't have a living RenderFrameHost. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4564: if (outer_proxy) { nit: {} https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* find_contents_for_frame(int frame_id) const; Use FindContentsForFrame(int). Unix-style naming is only for accessors. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:976: void SetNotWaitingForResponse() { waiting_for_response_ = false; } Add blank line, since the comment above doesn't apply here. https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2909: // TODO(avallee): else NOTREACHED() ? No need for this.
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
Could you try updating WebViewAccessibilityTest::FocusAccessibility so that it runs with --use-cross-process-frames-for-guests? It's currently set to run only without that flag, but now that you've fixed the focus issues it should hopefully pass now. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:259: // FIXME: Do we need to update the AX tree here? On 2016/05/06 21:23:33, lfg wrote: > Check with dmazzoni@ if this is necessary. Yes, it'd be good to add this line (just like is done at the bottom of this function): root()->current_frame_host()->UpdateAXTreeData();
Drive-by. https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2909: // TODO(avallee): else NOTREACHED() ? On 2016/05/06 at 21:23:33, lfg wrote: > No need for this. Is there ever a case where we'll legitimately call this when focusedFrame() != frame->toImplBase()->frame()? Seems like we should just assert.
avallee@chromium.org changed reviewers: + alexmos@chromium.org, dcheng@chromium.org, nasko@chromium.org
PTAL dcheng: Blink dmazzoni: frame_tree comments. alexmos: overall nasko: Feel free to comment. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:255: if (GetFocusedFrame()) { On 2016/05/06 21:23:33, lfg wrote: > Nit: no need for {}. Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:259: // FIXME: Do we need to update the AX tree here? On 2016/05/06 21:23:33, lfg wrote: > Check with dmazzoni@ if this is necessary. Sent email. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:259: // FIXME: Do we need to update the AX tree here? On 2016/05/09 17:07:20, dmazzoni wrote: > On 2016/05/06 21:23:33, lfg wrote: > > Check with dmazzoni@ if this is necessary. > > Yes, it'd be good to add this line (just like is done at the bottom of this > function): > > root()->current_frame_host()->UpdateAXTreeData(); This doesn't seem to make the test pass. I've filed a bug. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:284: if (proxy) { On 2016/05/06 21:23:32, lfg wrote: > nit: {} Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:183: virtual void SetFocusedFrame(FrameTreeNode* node, SiteInstance* source){}; On 2016/05/06 21:23:33, lfg wrote: > Add a comment with a description of the interface. Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:419: // focus inside a <webview> when focus in the embedder goes from the webview On 2016/05/06 21:23:33, lfg wrote: > Let's use inner WebContents instead of <webview>, since it's the generic > terminology used in content/. Sure, done. I did see mentions of <webview> further up in the file though. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:353: for (auto children : inner_web_contents_tree_nodes_) On 2016/05/06 21:23:33, lfg wrote: > Use auto&, so you don't make a copy of the pair to the stack. Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4517: if (rwh) { On 2016/05/06 21:23:33, lfg wrote: > nit: {} Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4539: if (inner_contents) { On 2016/05/06 21:23:33, lfg wrote: > nit: {} Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4554: // maybe focus outer main frame. On 2016/05/06 21:23:33, lfg wrote: > Not sure I follow this. Why are we focusing the root? Comment should answer this. It's not ideal, but it currently seems that it is not possible to embed a webview in any frame but the root. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4560: outer_node->current_frame_host()->GetSiteInstance(); On 2016/05/06 21:23:33, lfg wrote: > I think this should be outer_node->parent()->current_frame_host(), since the > outer_node shouldn't have a living RenderFrameHost. Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4564: if (outer_proxy) { On 2016/05/06 21:23:33, lfg wrote: > nit: {} Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* find_contents_for_frame(int frame_id) const; On 2016/05/06 21:23:33, lfg wrote: > Use FindContentsForFrame(int). Unix-style naming is only for accessors. Done. https://codereview.chromium.org/1934703002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:976: void SetNotWaitingForResponse() { waiting_for_response_ = false; } On 2016/05/06 21:23:33, lfg wrote: > Add blank line, since the comment above doesn't apply here. Done. https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2909: // TODO(avallee): else NOTREACHED() ? On 2016/05/06 21:23:33, lfg wrote: > No need for this. Done. https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2909: // TODO(avallee): else NOTREACHED() ? On 2016/05/09 18:25:08, dcheng wrote: > On 2016/05/06 at 21:23:33, lfg wrote: > > No need for this. > > Is there ever a case where we'll legitimately call this when focusedFrame() != > frame->toImplBase()->frame()? Seems like we should just assert. I don't think so, unless the browser and renderer get out of sync.
Description was changed from ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). BUG=600395 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. BUG=600395 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
avallee@chromium.org changed reviewers: + wjmaclean@chromium.org
Thanks, lgtm.
wjmaclean: Looking for rubber stamp on chrome/browser/apps/guest_view/web_view_browsertest.cc
On 2016/05/11 18:35:02, avallee wrote: > wjmaclean: Looking for rubber stamp on > chrome/browser/apps/guest_view/web_view_browsertest.cc LGTM for chrome/browser/apps/guest_view/web_view_browsertest.cc
https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2909: // TODO(avallee): else NOTREACHED() ? On 2016/05/11 at 18:26:11, avallee wrote: > On 2016/05/09 18:25:08, dcheng wrote: > > On 2016/05/06 at 21:23:33, lfg wrote: > > > No need for this. > > > > Is there ever a case where we'll legitimately call this when focusedFrame() != > > frame->toImplBase()->frame()? Seems like we should just assert. > > I don't think so, unless the browser and renderer get out of sync. If this can never happen, why do we need the if (...)?
On 2016/05/11 18:37:44, wjmaclean wrote: > On 2016/05/11 18:35:02, avallee wrote: > > wjmaclean: Looking for rubber stamp on > > chrome/browser/apps/guest_view/web_view_browsertest.cc > > LGTM for chrome/browser/apps/guest_view/web_view_browsertest.cc nit: In the issue description, when you say "input events were being routed ..." do you mean *all* input events? Or just keyboard events?
https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:259: // TODO(avallee): https://crbug.com/610795 This line is not sufficient to I'm okay with landing this change as-is and fixing the accessibility issue in a follow-up. As I described in a separate email, I think you probably need to call UpdateAXTreeData on the root frame of the embedder WebContents.
ipc lgtm
Thanks for working on this! Some initial comments and questions below. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:256: GetFocusedFrame()->current_frame_host()->UnsetFocusedFrame(); Do proxies need to be notified also? E.g., suppose the guest has a main frame A and a cross-process subframe B. Suppose A is focused, and then the embedder receives focus, so we get here to clear focus from A. It seems the renderer for B also needs to clear focus. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:286: if (proxy) Why is adding this check necessary? https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:293: if (current_instance && current_instance != source) Similar question here. I can't see how this can be null. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:186: virtual void SetFocusedFrame(FrameTreeNode* node, SiteInstance* source){}; nit: space before {} and no ; at the end https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2149: void RenderFrameHostImpl::UnsetFocusedFrame() { nit: naming this (and related bits) ClearFocusedFrame seems more consistent with existing focus code I've seen. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2752: // fix the bug to enable the test. nit: explicitly mention that this code is problematic for <webview>, so someone reading the code doesn't need to look up the bug to find out what the comment is about. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:420: // WebContents to another element in the embedder WebContents. This sounded a bit ambiguous to me, do you mean "This is used to clear focus inside an inner WebContents when focus shifts to a frame in the outer WebContents"? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:353: for (auto& children : inner_web_contents_tree_nodes_) nit: s/children/child/ (it's still referring to just one WebContentsTreeNode*, right?) https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1811: // return its RenderWidgetHost. This will recurse down if there are multiple levels of WebContents nesting, right? Might be worth mentioning that explicitly. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1813: (node_.get()) nit: .get() shouldn't be necessary https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4516: auto rwh = node->current_frame_host()->GetRenderWidgetHost(); nit: RenderWidgetHostImpl* instead of auto might be better for readability (otherwise it's not clear whether it's an Impl or not, for instance), and is more consistent with existing style. Same for autos in function below. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4518: rwh->Focus(); Why is this necessary? Page focus and frame focus are usually disjoint -- you can have frame focus but no page focus. It doesn't seem right that for example doing a window.focus() on a subframe would automatically give page focus as well. This also might result in an incorrect isFocused/isActive state in blink::FocusController if you alt-tab out. Is this due to WebContentsImpl::ReplicatePageFocus not working with inner/outer WebContents? Perhaps that should be fixed first? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4530: // we have one. I had trouble following this comment when I first read it. It makes sense after looking at the code, but maybe try to rephrase to make it more clear? The second paragraph is especially hard to follow. I.e., this is really doing three distinct steps: 1) Recursively clear focus in all inner WebContents. 2) Set |node| as the focused FTN in the current WebContents. 3) If there's an outer WebContents, update frame focus in its FrameTree to the FTN representing the current WebContents. Recursively do this for all WebContents ancestors. Let me make sure I understand this with an example (which may be useful in the comment as well). If an embedder page A contains a <webview> B, which embeds a subframe C, and focus shifts from A to C, then the outer FrameTree will update its focused FTN from A to the FTN corresponding to the inner FrameTree, and the inner FrameTree will update its focused FTN from null to the FTN for subframe C. Later, if we focus the embedder again, we will clear focused frame in the inner FrameTree, and set the focused frame to FTN A in the outer FrameTree. Is my example correct? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4532: auto old_focus_node = frame_tree_.GetFocusedFrame(); nit: s/old_focus_node/old_focused_node/ https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4534: if (old_focus_node != nullptr && node_.get()) { nit: .get() not necessary https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4539: inner_contents->SetFocusedFrameInternal(nullptr, source, false); I think this whole function could be simplified to make it easier to follow. Perhaps, rather than overloading it with being to recurse for two different things (recursively clearing focus in inner WC, and recursively updating focus in outer WC), maybe just make two different helpers for these two things? Then the main WC::SetFocusedFrame can be a really short and easy-to-follow function, corresponding to my three steps in the comment above. And/or, it seems you could avoid recursion and passing a bool here by just iterating through the focused inner_contents down, and through outer_contents_ up. The only thing you really need to do on each inner WC is inner_contents->frame_tree_->SetFocusedFrame(nullptr). Also, I think there's no need to check if old_focused_node is not equal to |node| when clearing inner WC focus -- FrameTree::SetFocusedFrame will already return early if the focused frame doesn't change. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4554: outer_contents->SetFocusedFrameInternal( Please follow up on this hack with BrowserPlugin experts - I don't really know how BrowserPlugin focus works and whether this makes sense. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4563: if (outer_proxy) Why would outer_proxy be null? After talking through <webview> proxy setup with lfg@, it seems to me that this proxy should always exit. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4564: outer_proxy->SetFocusedFrame(); Won't you notify the embedder's SiteInstance as part of FrameTree::SetFocusedFrame when it's called as part of outer_contents->SetFocusedFrameInternal? Why is this separate call necessary? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:814: WebContentsTreeNode(WebContentsImpl* inner); nit: explicit? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* FindContentsForFrame(int frame_id) { It might be better to mention "Inner" in the name explicitly, since this only searches for an inner WebContents, right? So, FindInnerWebContents or GetInnerWebContents? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* FindContentsForFrame(int frame_id) { This seems to be a FrameTreeNode id, so I'd rename it to frame_tree_node_id to make this clear. Also, maybe even just take a FrameTreeNode* and make the use of FTN ID an internal detail? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:829: static_cast<const WebContentsTreeNode*>(this)->FindContentsForFrame( This doesn't look like it should be inlined (I think usually only trivial getters/setters are inlined). https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:832: const WebContentsImpl* FindContentsForFrame(int frame_id) const; Are two declarations for this really necessary? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:835: // The web contents that owns this node nit: s/web contents/WebContents/ nit: period at the end. (also below) https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:836: WebContentsImpl* const inner_web_contents_; I found the naming of this confusing. It sounds similar to inner_web_contents_tree_nodes_, which is used for something very different. In addition, it means the wrong thing for the topmost WebContents, which isn't an inner WebContents for anything. Given that this is just the WebContents for this node, maybe just name it web_contents_? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:837: nit: either no blank line here, or blank lines separating all of these private fields for consistency.
alexmos: Responded to your comments. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:256: GetFocusedFrame()->current_frame_host()->UnsetFocusedFrame(); On 2016/05/12 20:28:39, alexmos wrote: > Do proxies need to be notified also? E.g., suppose the guest has a main frame A > and a cross-process subframe B. Suppose A is focused, and then the embedder > receives focus, so we get here to clear focus from A. It seems the renderer for > B also needs to clear focus. Agreed that they would need to be notified. However, my current understanding is that all frames inside a webview are local. I'm not sure if there are cases other than webview where the WebContents has a child WebContents that loses focus like this. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:259: // TODO(avallee): https://crbug.com/610795 This line is not sufficient to On 2016/05/12 02:58:56, dmazzoni wrote: > I'm okay with landing this change as-is and fixing the accessibility issue in a > follow-up. > As I described in a separate email, I think you probably need to call > UpdateAXTreeData > on the root frame of the embedder WebContents. Thanks for the follow up dmazzoni. I have an idea on getting the root updated correctly. I'll follow up on that thread with my additional questions. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:286: if (proxy) On 2016/05/12 20:28:39, alexmos wrote: > Why is adding this check necessary? I don't think this is necessary anymore, but was initially getting an instance that had no proxy and thus ended up being null. I'll check, but I think my code no longer triggers this. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:293: if (current_instance && current_instance != source) On 2016/05/12 20:28:39, alexmos wrote: > Similar question here. I can't see how this can be null. Removed. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:186: virtual void SetFocusedFrame(FrameTreeNode* node, SiteInstance* source){}; On 2016/05/12 20:28:39, alexmos wrote: > nit: space before {} and no ; at the end Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2149: void RenderFrameHostImpl::UnsetFocusedFrame() { On 2016/05/12 20:28:40, alexmos wrote: > nit: naming this (and related bits) ClearFocusedFrame seems more consistent with > existing focus code I've seen. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2752: // fix the bug to enable the test. On 2016/05/12 20:28:39, alexmos wrote: > nit: explicitly mention that this code is problematic for <webview>, so someone > reading the code doesn't need to look up the bug to find out what the comment is > about. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:420: // WebContents to another element in the embedder WebContents. On 2016/05/12 20:28:40, alexmos wrote: > This sounded a bit ambiguous to me, do you mean "This is used to clear focus > inside an inner WebContents when focus shifts to a frame in the outer > WebContents"? Pretty good. Focus could also move to a sibling webcontents. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:353: for (auto& children : inner_web_contents_tree_nodes_) On 2016/05/12 20:28:41, alexmos wrote: > nit: s/children/child/ (it's still referring to just one WebContentsTreeNode*, > right?) Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1811: // return its RenderWidgetHost. On 2016/05/12 20:28:40, alexmos wrote: > This will recurse down if there are multiple levels of WebContents nesting, > right? Might be worth mentioning that explicitly. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1813: (node_.get()) On 2016/05/12 20:28:42, alexmos wrote: > nit: .get() shouldn't be necessary Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4516: auto rwh = node->current_frame_host()->GetRenderWidgetHost(); On 2016/05/12 20:28:40, alexmos wrote: > nit: RenderWidgetHostImpl* instead of auto might be better for readability > (otherwise it's not clear whether it's an Impl or not, for instance), and is > more consistent with existing style. Same for autos in function below. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4518: rwh->Focus(); On 2016/05/12 20:28:40, alexmos wrote: > Why is this necessary? Page focus and frame focus are usually disjoint -- you > can have frame focus but no page focus. It doesn't seem right that for example > doing a window.focus() on a subframe would automatically give page focus as > well. This also might result in an incorrect isFocused/isActive state in > blink::FocusController if you alt-tab out. > > Is this due to WebContentsImpl::ReplicatePageFocus not working with inner/outer > WebContents? Perhaps that should be fixed first? Without this call, the renderer for the webview does not believe it is focused. There is no highlight if I select an input box and no blinking caret. The caret and focus border do disappear from the embedder though. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4530: // we have one. On 2016/05/12 20:28:40, alexmos wrote: > I had trouble following this comment when I first read it. It makes sense after > looking at the code, but maybe try to rephrase to make it more clear? The > second paragraph is especially hard to follow. > > I.e., this is really doing three distinct steps: > 1) Recursively clear focus in all inner WebContents. > 2) Set |node| as the focused FTN in the current WebContents. > 3) If there's an outer WebContents, update frame focus in its FrameTree to the > FTN representing the current WebContents. Recursively do this for all > WebContents ancestors. > > Let me make sure I understand this with an example (which may be useful in the > comment as well). If an embedder page A contains a <webview> B, which embeds a > subframe C, and focus shifts from A to C, then the outer FrameTree will update > its focused FTN from A to the FTN corresponding to the inner FrameTree, and the > inner FrameTree will update its focused FTN from null to the FTN for subframe C. > Later, if we focus the embedder again, we will clear focused frame in the inner > FrameTree, and set the focused frame to FTN A in the outer FrameTree. Is my > example correct? Here's an enhanced version of your example https://goo.gl/photos/GJLpaqP8DY7GJ5pD8 Initially, 3 is focused (1 implicitly by being the root web contents). If you were you click on if2, we want to focus 5 and then focus 7 (1 is focused implicitly as the root web contents, 2 is focused since frame 5 is focused). When moving back to the embedder, we would clear 7 and then focus 3. Here are two situations that I expect to happen eventually and requires the slightly more complex implementation. Document 1 <p>Content</p> <webview name="page1"><!-- focus is here --></webview> <webview name="page2"></webview> In this document, if we clicked on the <p>, the web contents that originates the event is the outer one. It would unfocus its inner contents and then focus itself. If we clicked on the sibling <webview>, its webcontents would trigger the focus change. It has no inner web contents, but needs to unfocus its sibling first. There is no recursion into inner contents, then we set focus in the outer context. The outer context would unfocus the currently focused inner context, possibly recur into an outer webcontents (not in the case of this example) and then set focus in its own frame tree. Finally, it would return and the inner contents would set focus in its own frame tree. In this chain, there is never more than one focused element at a time. Document 2 <div id=embedder> <webview></webview> <guestview name=a><guestview name=b><!--focus starts here --></guestview></guestview> </div> Same idea here. If we want to focus guestview a, we need to unfocus b, then go up to the embedder, nothing needs to be done there, then finally update the free for the middle guestview. I'm not sure if that was super clear, but that's the main idea. The simpler approach might be to start at the root web contents (or focused common ancestor to both the old and new focus web contents) and do an unfocus traversal, then from the focused contents, traverse parents setting focus until finally focusing itself. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4532: auto old_focus_node = frame_tree_.GetFocusedFrame(); On 2016/05/12 20:28:40, alexmos wrote: > nit: s/old_focus_node/old_focused_node/ Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4534: if (old_focus_node != nullptr && node_.get()) { On 2016/05/12 20:28:42, alexmos wrote: > nit: .get() not necessary Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4539: inner_contents->SetFocusedFrameInternal(nullptr, source, false); On 2016/05/12 20:28:41, alexmos wrote: > I think this whole function could be simplified to make it easier to follow. > Perhaps, rather than overloading it with being to recurse for two different > things (recursively clearing focus in inner WC, and recursively updating focus > in outer WC), maybe just make two different helpers for these two things? Then > the main WC::SetFocusedFrame can be a really short and easy-to-follow function, > corresponding to my three steps in the comment above. And/or, it seems you > could avoid recursion and passing a bool here by just iterating through the > focused inner_contents down, and through outer_contents_ up. The only thing you > really need to do on each inner WC is > inner_contents->frame_tree_->SetFocusedFrame(nullptr). > > Also, I think there's no need to check if old_focused_node is not equal to > |node| when clearing inner WC focus -- FrameTree::SetFocusedFrame will already > return early if the focused frame doesn't change. I agree that the split could work. 1. Find the common outer web contents with focus between the old and new frames. 2. unfocus the inner webcontents that are focused. (Stop recursion if the currently focused frame has no inner webcontents). Set focused frame to nullptr on each of those webcontents' frame trees. 3. For each outer webcontents to the one to be focused, set focused to its frame in the outer free. 4. Set focus on the proper node in the target webcontents' frame tree. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4554: outer_contents->SetFocusedFrameInternal( On 2016/05/12 20:28:40, alexmos wrote: > Please follow up on this hack with BrowserPlugin experts - I don't really know > how BrowserPlugin focus works and whether this makes sense. We tested it and it seems we can only embed in the main frame. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4563: if (outer_proxy) On 2016/05/12 20:28:40, alexmos wrote: > Why would outer_proxy be null? After talking through <webview> proxy setup with > lfg@, it seems to me that this proxy should always exit. Agreed, it should exist. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4564: outer_proxy->SetFocusedFrame(); On 2016/05/12 20:28:42, alexmos wrote: > Won't you notify the embedder's SiteInstance as part of > FrameTree::SetFocusedFrame when it's called as part of > outer_contents->SetFocusedFrameInternal? Why is this separate call necessary? For the webview specific case, the rfp that we want to update is owned by the root of the inner frame tree for the chrome-guest://... site instance no not findable in the parent frame tree. Also, I'm not 100% on this, but it also seems like CollectSiteInstances in the inner frame tree does not find that instance and so never notifies it. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:814: WebContentsTreeNode(WebContentsImpl* inner); On 2016/05/12 20:28:42, alexmos wrote: > nit: explicit? Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* FindContentsForFrame(int frame_id) { On 2016/05/12 20:28:43, alexmos wrote: > It might be better to mention "Inner" in the name explicitly, since this only > searches for an inner WebContents, right? So, FindInnerWebContents or > GetInnerWebContents? Did something in that line. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:829: static_cast<const WebContentsTreeNode*>(this)->FindContentsForFrame( On 2016/05/12 20:28:43, alexmos wrote: > This doesn't look like it should be inlined (I think usually only trivial > getters/setters are inlined). This is trivial, it compiles down to calling the const version. Casting has no runtime cost. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:832: const WebContentsImpl* FindContentsForFrame(int frame_id) const; On 2016/05/12 20:28:42, alexmos wrote: > Are two declarations for this really necessary? Find frame makes sense as a const function, but should not return a non-const pointer to the parent which could get me back with a non-const pointer to this. Without an explicit const_cast. This is in the spirit of Effective C++, item 3: Use const whenever possible. (This pattern is on page 23 of the third edition) https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:835: // The web contents that owns this node On 2016/05/12 20:28:43, alexmos wrote: > nit: s/web contents/WebContents/ > nit: period at the end. (also below) Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:836: WebContentsImpl* const inner_web_contents_; On 2016/05/12 20:28:43, alexmos wrote: > I found the naming of this confusing. It sounds similar to > inner_web_contents_tree_nodes_, which is used for something very different. In > addition, it means the wrong thing for the topmost WebContents, which isn't an > inner WebContents for anything. Given that this is just the WebContents for > this node, maybe just name it web_contents_? Agreed, done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:837: On 2016/05/12 20:28:42, alexmos wrote: > nit: either no blank line here, or blank lines separating all of these private > fields for consistency. Done.
alexmos: Responded to your comments. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:256: GetFocusedFrame()->current_frame_host()->UnsetFocusedFrame(); On 2016/05/12 20:28:39, alexmos wrote: > Do proxies need to be notified also? E.g., suppose the guest has a main frame A > and a cross-process subframe B. Suppose A is focused, and then the embedder > receives focus, so we get here to clear focus from A. It seems the renderer for > B also needs to clear focus. Agreed that they would need to be notified. However, my current understanding is that all frames inside a webview are local. I'm not sure if there are cases other than webview where the WebContents has a child WebContents that loses focus like this. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:186: virtual void SetFocusedFrame(FrameTreeNode* node, SiteInstance* source){}; On 2016/05/12 20:28:39, alexmos wrote: > nit: space before {} and no ; at the end Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2149: void RenderFrameHostImpl::UnsetFocusedFrame() { On 2016/05/12 20:28:40, alexmos wrote: > nit: naming this (and related bits) ClearFocusedFrame seems more consistent with > existing focus code I've seen. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2752: // fix the bug to enable the test. On 2016/05/12 20:28:39, alexmos wrote: > nit: explicitly mention that this code is problematic for <webview>, so someone > reading the code doesn't need to look up the bug to find out what the comment is > about. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:420: // WebContents to another element in the embedder WebContents. On 2016/05/12 20:28:40, alexmos wrote: > This sounded a bit ambiguous to me, do you mean "This is used to clear focus > inside an inner WebContents when focus shifts to a frame in the outer > WebContents"? Pretty good. Focus could also move to a sibling webcontents. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:353: for (auto& children : inner_web_contents_tree_nodes_) On 2016/05/12 20:28:41, alexmos wrote: > nit: s/children/child/ (it's still referring to just one WebContentsTreeNode*, > right?) Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1811: // return its RenderWidgetHost. On 2016/05/12 20:28:40, alexmos wrote: > This will recurse down if there are multiple levels of WebContents nesting, > right? Might be worth mentioning that explicitly. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1813: (node_.get()) On 2016/05/12 20:28:42, alexmos wrote: > nit: .get() shouldn't be necessary Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4516: auto rwh = node->current_frame_host()->GetRenderWidgetHost(); On 2016/05/12 20:28:40, alexmos wrote: > nit: RenderWidgetHostImpl* instead of auto might be better for readability > (otherwise it's not clear whether it's an Impl or not, for instance), and is > more consistent with existing style. Same for autos in function below. Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4518: rwh->Focus(); On 2016/05/12 20:28:40, alexmos wrote: > Why is this necessary? Page focus and frame focus are usually disjoint -- you > can have frame focus but no page focus. It doesn't seem right that for example > doing a window.focus() on a subframe would automatically give page focus as > well. This also might result in an incorrect isFocused/isActive state in > blink::FocusController if you alt-tab out. > > Is this due to WebContentsImpl::ReplicatePageFocus not working with inner/outer > WebContents? Perhaps that should be fixed first? Without this call, the renderer for the webview does not believe it is focused. There is no highlight if I select an input box and no blinking caret. The caret and focus border do disappear from the embedder though. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4530: // we have one. On 2016/05/12 20:28:40, alexmos wrote: > I had trouble following this comment when I first read it. It makes sense after > looking at the code, but maybe try to rephrase to make it more clear? The > second paragraph is especially hard to follow. > > I.e., this is really doing three distinct steps: > 1) Recursively clear focus in all inner WebContents. > 2) Set |node| as the focused FTN in the current WebContents. > 3) If there's an outer WebContents, update frame focus in its FrameTree to the > FTN representing the current WebContents. Recursively do this for all > WebContents ancestors. > > Let me make sure I understand this with an example (which may be useful in the > comment as well). If an embedder page A contains a <webview> B, which embeds a > subframe C, and focus shifts from A to C, then the outer FrameTree will update > its focused FTN from A to the FTN corresponding to the inner FrameTree, and the > inner FrameTree will update its focused FTN from null to the FTN for subframe C. > Later, if we focus the embedder again, we will clear focused frame in the inner > FrameTree, and set the focused frame to FTN A in the outer FrameTree. Is my > example correct? Here's an enhanced version of your example https://goo.gl/photos/GJLpaqP8DY7GJ5pD8 Initially, 3 is focused (1 implicitly by being the root web contents). If you were you click on if2, we want to focus 5 and then focus 7 (1 is focused implicitly as the root web contents, 2 is focused since frame 5 is focused). When moving back to the embedder, we would clear 7 and then focus 3. Here are two situations that I expect to happen eventually and requires the slightly more complex implementation. Document 1 <p>Content</p> <webview name="page1"><!-- focus is here --></webview> <webview name="page2"></webview> In this document, if we clicked on the <p>, the web contents that originates the event is the outer one. It would unfocus its inner contents and then focus itself. If we clicked on the sibling <webview>, its webcontents would trigger the focus change. It has no inner web contents, but needs to unfocus its sibling first. There is no recursion into inner contents, then we set focus in the outer context. The outer context would unfocus the currently focused inner context, possibly recur into an outer webcontents (not in the case of this example) and then set focus in its own frame tree. Finally, it would return and the inner contents would set focus in its own frame tree. In this chain, there is never more than one focused element at a time. Document 2 <div id=embedder> <webview></webview> <guestview name=a><guestview name=b><!--focus starts here --></guestview></guestview> </div> Same idea here. If we want to focus guestview a, we need to unfocus b, then go up to the embedder, nothing needs to be done there, then finally update the free for the middle guestview. I'm not sure if that was super clear, but that's the main idea. The simpler approach might be to start at the root web contents (or focused common ancestor to both the old and new focus web contents) and do an unfocus traversal, then from the focused contents, traverse parents setting focus until finally focusing itself. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4532: auto old_focus_node = frame_tree_.GetFocusedFrame(); On 2016/05/12 20:28:40, alexmos wrote: > nit: s/old_focus_node/old_focused_node/ Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4534: if (old_focus_node != nullptr && node_.get()) { On 2016/05/12 20:28:42, alexmos wrote: > nit: .get() not necessary Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4539: inner_contents->SetFocusedFrameInternal(nullptr, source, false); On 2016/05/12 20:28:41, alexmos wrote: > I think this whole function could be simplified to make it easier to follow. > Perhaps, rather than overloading it with being to recurse for two different > things (recursively clearing focus in inner WC, and recursively updating focus > in outer WC), maybe just make two different helpers for these two things? Then > the main WC::SetFocusedFrame can be a really short and easy-to-follow function, > corresponding to my three steps in the comment above. And/or, it seems you > could avoid recursion and passing a bool here by just iterating through the > focused inner_contents down, and through outer_contents_ up. The only thing you > really need to do on each inner WC is > inner_contents->frame_tree_->SetFocusedFrame(nullptr). > > Also, I think there's no need to check if old_focused_node is not equal to > |node| when clearing inner WC focus -- FrameTree::SetFocusedFrame will already > return early if the focused frame doesn't change. I agree that the split could work. 1. Find the common outer web contents with focus between the old and new frames. 2. unfocus the inner webcontents that are focused. (Stop recursion if the currently focused frame has no inner webcontents). Set focused frame to nullptr on each of those webcontents' frame trees. 3. For each outer webcontents to the one to be focused, set focused to its frame in the outer free. 4. Set focus on the proper node in the target webcontents' frame tree. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4554: outer_contents->SetFocusedFrameInternal( On 2016/05/12 20:28:40, alexmos wrote: > Please follow up on this hack with BrowserPlugin experts - I don't really know > how BrowserPlugin focus works and whether this makes sense. We tested it and it seems we can only embed in the main frame. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4563: if (outer_proxy) On 2016/05/12 20:28:40, alexmos wrote: > Why would outer_proxy be null? After talking through <webview> proxy setup with > lfg@, it seems to me that this proxy should always exit. Agreed, it should exist. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4564: outer_proxy->SetFocusedFrame(); On 2016/05/12 20:28:42, alexmos wrote: > Won't you notify the embedder's SiteInstance as part of > FrameTree::SetFocusedFrame when it's called as part of > outer_contents->SetFocusedFrameInternal? Why is this separate call necessary? For the webview specific case, the rfp that we want to update is owned by the root of the inner frame tree for the chrome-guest://... site instance no not findable in the parent frame tree. Also, I'm not 100% on this, but it also seems like CollectSiteInstances in the inner frame tree does not find that instance and so never notifies it. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:814: WebContentsTreeNode(WebContentsImpl* inner); On 2016/05/12 20:28:42, alexmos wrote: > nit: explicit? Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* FindContentsForFrame(int frame_id) { On 2016/05/12 20:28:43, alexmos wrote: > It might be better to mention "Inner" in the name explicitly, since this only > searches for an inner WebContents, right? So, FindInnerWebContents or > GetInnerWebContents? Did something in that line. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:829: static_cast<const WebContentsTreeNode*>(this)->FindContentsForFrame( On 2016/05/12 20:28:43, alexmos wrote: > This doesn't look like it should be inlined (I think usually only trivial > getters/setters are inlined). This is trivial, it compiles down to calling the const version. Casting has no runtime cost. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:832: const WebContentsImpl* FindContentsForFrame(int frame_id) const; On 2016/05/12 20:28:42, alexmos wrote: > Are two declarations for this really necessary? Find frame makes sense as a const function, but should not return a non-const pointer to the parent which could get me back with a non-const pointer to this. Without an explicit const_cast. This is in the spirit of Effective C++, item 3: Use const whenever possible. (This pattern is on page 23 of the third edition) https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:835: // The web contents that owns this node On 2016/05/12 20:28:43, alexmos wrote: > nit: s/web contents/WebContents/ > nit: period at the end. (also below) Done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:836: WebContentsImpl* const inner_web_contents_; On 2016/05/12 20:28:43, alexmos wrote: > I found the naming of this confusing. It sounds similar to > inner_web_contents_tree_nodes_, which is used for something very different. In > addition, it means the wrong thing for the topmost WebContents, which isn't an > inner WebContents for anything. Given that this is just the WebContents for > this node, maybe just name it web_contents_? Agreed, done. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:837: On 2016/05/12 20:28:42, alexmos wrote: > nit: either no blank line here, or blank lines separating all of these private > fields for consistency. Done.
https://codereview.chromium.org/1934703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2906: if (page()->focusController().focusedFrame() == frame->toImplBase()->frame()) { Based on earlier comments, I thought we agreed that this would never be called when this isn't true. Can we just ASSERT() this instead of making it an if()?
https://codereview.chromium.org/1934703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2906: if (page()->focusController().focusedFrame() == frame->toImplBase()->frame()) { On 2016/05/16 20:28:58, dcheng wrote: > Based on earlier comments, I thought we agreed that this would never be called > when this isn't true. Can we just ASSERT() this instead of making it an if()? Sorry, hadn't hit save there. Done.
https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:256: GetFocusedFrame()->current_frame_host()->UnsetFocusedFrame(); On 2016/05/16 20:26:43, avallee wrote: > On 2016/05/12 20:28:39, alexmos wrote: > > Do proxies need to be notified also? E.g., suppose the guest has a main frame > A > > and a cross-process subframe B. Suppose A is focused, and then the embedder > > receives focus, so we get here to clear focus from A. It seems the renderer > for > > B also needs to clear focus. > > Agreed that they would need to be notified. > > However, my current understanding is that all frames inside a webview are local. > I'm not sure if there are cases other than webview where the WebContents has a > child WebContents that loses focus like this. I brought this up on site isolation chat, and indeed, today there's no support for OOPIFs inside a <webview>. However, that's going to be fixed in the future, and it's better not to rely on this assumption when we can. Ideally, it'd be nice to make this future-proof, but I guess notifying proxies would require adding all the ClearFocusedFrame plumbing for proxies? Can we at least add a detailed TODO to fix this once <webview> supports OOPIFs, along with a bug reference? https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:420: // WebContents to another element in the embedder WebContents. On 2016/05/16 20:26:43, avallee wrote: > On 2016/05/12 20:28:40, alexmos wrote: > > This sounded a bit ambiguous to me, do you mean "This is used to clear focus > > inside an inner WebContents when focus shifts to a frame in the outer > > WebContents"? > > Pretty good. > > Focus could also move to a sibling webcontents. Acknowledged. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4518: rwh->Focus(); On 2016/05/16 20:26:43, avallee wrote: > On 2016/05/12 20:28:40, alexmos wrote: > > Why is this necessary? Page focus and frame focus are usually disjoint -- you > > can have frame focus but no page focus. It doesn't seem right that for > example > > doing a window.focus() on a subframe would automatically give page focus as > > well. This also might result in an incorrect isFocused/isActive state in > > blink::FocusController if you alt-tab out. > > > > Is this due to WebContentsImpl::ReplicatePageFocus not working with > inner/outer > > WebContents? Perhaps that should be fixed first? > > Without this call, the renderer for the webview does not believe it is focused. > There is no highlight if I select an input box and no blinking caret. The caret > and focus border do disappear from the embedder though. Yup, that very likely indicates a problem with page focus. I ran into the same issue when fixing keyboard input in OOPIFs (see https://crbug.com/530663, r355833 and https://goo.gl/B9S5ou). I don't think having this call here is correct -- it's a bandaid over the real problem, and it might break things (see my earlier comment). FWIW, I've tried out the current CL with https://chrome.google.com/webstore/detail/browser-sample/edggnmnajhcbhlnpjnog..., and noticed that if you focus the Google search <input> field, and then click on a different window (or just alt-tab out), that keeps the Google keyboard focus blinking in the inactive webview window, which is wrong since the webview window lost page focus. I think we should just fix WebContentsImpl::ReplicatePageFocus for inner/outer contents and whatever else is necessary for page focus instead of this call. (Fine to do it in a separate CL before/after.) https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4530: // we have one. On 2016/05/16 20:26:43, avallee wrote: > On 2016/05/12 20:28:40, alexmos wrote: > > I had trouble following this comment when I first read it. It makes sense > after > > looking at the code, but maybe try to rephrase to make it more clear? The > > second paragraph is especially hard to follow. > > > > I.e., this is really doing three distinct steps: > > 1) Recursively clear focus in all inner WebContents. > > 2) Set |node| as the focused FTN in the current WebContents. > > 3) If there's an outer WebContents, update frame focus in its FrameTree to the > > FTN representing the current WebContents. Recursively do this for all > > WebContents ancestors. > > > > Let me make sure I understand this with an example (which may be useful in the > > comment as well). If an embedder page A contains a <webview> B, which embeds a > > subframe C, and focus shifts from A to C, then the outer FrameTree will update > > its focused FTN from A to the FTN corresponding to the inner FrameTree, and > the > > inner FrameTree will update its focused FTN from null to the FTN for subframe > C. > > Later, if we focus the embedder again, we will clear focused frame in the > inner > > FrameTree, and set the focused frame to FTN A in the outer FrameTree. Is my > > example correct? > > Here's an enhanced version of your example > https://goo.gl/photos/GJLpaqP8DY7GJ5pD8 > > Initially, 3 is focused (1 implicitly by being the root web contents). If you > were you click on if2, we want to focus 5 and then focus 7 (1 is focused > implicitly as the root web contents, 2 is focused since frame 5 is focused). > > When moving back to the embedder, we would clear 7 and then focus 3. > > Here are two situations that I expect to happen eventually and requires the > slightly more complex implementation. > > Document 1 > <p>Content</p> > <webview name="page1"><!-- focus is here --></webview> > <webview name="page2"></webview> > > In this document, if we clicked on the <p>, the web contents that originates the > event is the outer one. It would unfocus its inner contents and then focus > itself. > > If we clicked on the sibling <webview>, its webcontents would trigger the focus > change. It has no inner web contents, but needs to unfocus its sibling first. > There is no recursion into inner contents, then we set focus in the outer > context. The outer context would unfocus the currently focused inner context, > possibly recur into an outer webcontents (not in the case of this example) and > then set focus in its own frame tree. Finally, it would return and the inner > contents would set focus in its own frame tree. > > In this chain, there is never more than one focused element at a time. > > > Document 2 > <div id=embedder> > <webview></webview> > <guestview name=a><guestview name=b><!--focus starts here > --></guestview></guestview> > </div> > > Same idea here. If we want to focus guestview a, we need to unfocus b, then go > up to the embedder, nothing needs to be done there, then finally update the free > for the middle guestview. > > I'm not sure if that was super clear, but that's the main idea. > > The simpler approach might be to start at the root web contents (or focused > common ancestor to both the old and new focus web contents) and do an unfocus > traversal, then from the focused contents, traverse parents setting focus until > finally focusing itself. Thanks for the detailed examples, this makes sense. I didn't consider the sibling cases, so it helps to walk through that example. Is it important that the walk through outer contents happens before setting focus in the current FrameTree? It doesn't seem to matter, but wanted to verify. Here's a different idea: instead of maintaining focused frames across the whole WebContents ancestor chain, what if we only maintained one focused FrameTreeNode across all inner/outer WebContents for a given page? I.e., in your enhanced example, when you click on if2, just focus 7, and clear focus from 3 (leaving the FrameTree at the root without a focused frame). Would anything break if you didn't actually have 5 focused? This would make things very simple: just unfocus stuff in the old WebContentsTreeNode, and focus stuff in the new one (though it probably would need to keep track of the currently focused WebContentsTreeNode somewhere). https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4539: inner_contents->SetFocusedFrameInternal(nullptr, source, false); On 2016/05/16 20:26:43, avallee wrote: > On 2016/05/12 20:28:41, alexmos wrote: > > I think this whole function could be simplified to make it easier to follow. > > Perhaps, rather than overloading it with being to recurse for two different > > things (recursively clearing focus in inner WC, and recursively updating focus > > in outer WC), maybe just make two different helpers for these two things? > Then > > the main WC::SetFocusedFrame can be a really short and easy-to-follow > function, > > corresponding to my three steps in the comment above. And/or, it seems you > > could avoid recursion and passing a bool here by just iterating through the > > focused inner_contents down, and through outer_contents_ up. The only thing > you > > really need to do on each inner WC is > > inner_contents->frame_tree_->SetFocusedFrame(nullptr). > > > > Also, I think there's no need to check if old_focused_node is not equal to > > |node| when clearing inner WC focus -- FrameTree::SetFocusedFrame will already > > return early if the focused frame doesn't change. > > I agree that the split could work. > 1. Find the common outer web contents with focus between the old and new frames. > 2. unfocus the inner webcontents that are focused. (Stop recursion if the > currently focused frame has no inner webcontents). Set focused frame to nullptr > on each of those webcontents' frame trees. > 3. For each outer webcontents to the one to be focused, set focused to its frame > in the outer free. > 4. Set focus on the proper node in the target webcontents' frame tree. This makes sense, though I'm not sure if the code will be less complex. Might be worth trying, though I'm curious about your take on my proposal above first (about not involving ancestors at all). If that doesn't work, it might be ok to just break apart the recursive inner focus clearing from the current function into a helper? I think this will let you remove the can_visit_outer_contents and make the rest easier to follow. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4554: outer_contents->SetFocusedFrameInternal( On 2016/05/16 20:26:44, avallee wrote: > On 2016/05/12 20:28:40, alexmos wrote: > > Please follow up on this hack with BrowserPlugin experts - I don't really know > > how BrowserPlugin focus works and whether this makes sense. > > We tested it and it seems we can only embed in the main frame. Acknowledged. Can you remove the "HACK" from the comment then? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4563: if (outer_proxy) On 2016/05/16 20:26:44, avallee wrote: > On 2016/05/12 20:28:40, alexmos wrote: > > Why would outer_proxy be null? After talking through <webview> proxy setup > with > > lfg@, it seems to me that this proxy should always exit. > > Agreed, it should exist. So then, can you remove this if? https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4564: outer_proxy->SetFocusedFrame(); On 2016/05/16 20:26:43, avallee wrote: > On 2016/05/12 20:28:42, alexmos wrote: > > Won't you notify the embedder's SiteInstance as part of > > FrameTree::SetFocusedFrame when it's called as part of > > outer_contents->SetFocusedFrameInternal? Why is this separate call necessary? > > For the webview specific case, the rfp that we want to update is owned by the > root of the inner frame tree for the chrome-guest://... site instance no not > findable in the parent frame tree. Also, I'm not 100% on this, but it also seems > like CollectSiteInstances in the inner frame tree does not find that instance > and so never notifies it. Sorry, I'm confused about this. Consider the sample webview app in the crbug. The embedder's FrameTree will have two FTNs, with the following SiteInstances for current_frame_host(): FTN_1 [chrome-extension://extid] | FTN_2 [chrome-extension://extid] FTN_1 is for the main page, FTN_2 is for the <webview> (it's sitting at about:blank, its RFH is unused (its RenderFrame isn't live), and its SiteInstance matches its parent). The inner contents will have a FTN_3, with SiteInstance [chrome-guest://extid]. That FTN will also have a proxy RFP_1 for [chrome-extension://extid] (e.g., this allows the embedder to postMessage the webview). IIUC, there are no proxies created initially in the outer FrameTree FTNs, but one may be created on-demand (RFP_2) for [chrome-guest://extid] in FTN_1 if embedder postMessages the guest, so that the guest can reply to the message. Now, the code as written will find the proxy RFP_1 and set the corresponding remote frame as focused in the chrome-extension:// renderer. But you said "for the chrome-guest://... site instance", and there's no such proxy in FTN_3. Did you mean the chrome-extension:// RFP_1? Now, in my original question, I was surprised by this, since I thought the chrome-extension:// renderer should update focus when processing its own FrameTree. But I think I know why: when you recurse into outer WebContents and set focus to FTN_2, it tries to send a message to its current_frame_host(), but that isn't live, so the SetFocusedFrame message isn't sent and chrome-extensions:// renderer isn't notified. This is why you need to go through RFP_1. Correct? Additionally, I agree with you that RFP_1 won't be found by CollectSiteInstances in inner FrameTree::SetFocusedFrame, since chrome-extension:// won't correspond to any of the RFHs in the inner tree. OK, the outer_proxy makes sense then. But it's really subtle and needs a thorough comment for why this isn't handled in any of the FrameTree::SetFocusedFrame calls. :) https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:832: const WebContentsImpl* FindContentsForFrame(int frame_id) const; On 2016/05/16 20:26:44, avallee wrote: > On 2016/05/12 20:28:42, alexmos wrote: > > Are two declarations for this really necessary? > > Find frame makes sense as a const function, but should not return a non-const > pointer to the parent which could get me back with a non-const pointer to this. > Without an explicit const_cast. > > This is in the spirit of Effective C++, item 3: Use const whenever possible. > (This pattern is on page 23 of the third edition) I wasn't sure myself, so I ran this by nick@ and I'm passing on his response. There is a convention regarding const in the content API: "don't add the const identifier to interfaces. For interfaces implemented by the embedder, we can't make assumptions about what the embedder needs to implement it. For interfaces implemented by content, the implementation details doesn't have to be exposed." (from https://www.chromium.org/developers/content-module/content-api) So, WebContents isn't supposed to be const-correct, so a function that returns a const WebContentsImpl isn't very meaningful. Unfortunately, lots of WebContents methods are const, which disagrees with the theory and ideally would be cleaned up -- but nick@ apparently doesn't hate it enough to go rip out const. :) https://codereview.chromium.org/1934703002/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:254: if (node == nullptr) { nit: !node https://codereview.chromium.org/1934703002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:254: if (node == nullptr) { Can you add a comment above this, explaining that we might call this with a null node when clearing focus for an inner WebContents? https://codereview.chromium.org/1934703002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:261: root()->current_frame_host()->UpdateAXTreeData(); Should all three statements in the outer if only happen if (!node && GetFocusedFrame())? Seems like you'd call UpdateAXTreeData redundantly if you try to clear focus from a FrameTree that already had it cleared. https://codereview.chromium.org/1934703002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4559: outer_node->parent()->current_frame_host()->GetSiteInstance(); I don't think you need parent() here -- seems like the outer_node should be in the same SiteInstance as its parent.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
alexmos: Simplified the approach by tracking which webcontents has focus in the webcontents tree and only touching at most 2 frame trees. dcheng: It is possible to call unfocus on a different frame than the focused one. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4518: rwh->Focus(); On 2016/05/19 00:08:09, alexmos wrote: > On 2016/05/16 20:26:43, avallee wrote: > > On 2016/05/12 20:28:40, alexmos wrote: > > > Why is this necessary? Page focus and frame focus are usually disjoint -- > you > > > can have frame focus but no page focus. It doesn't seem right that for > > example > > > doing a window.focus() on a subframe would automatically give page focus as > > > well. This also might result in an incorrect isFocused/isActive state in > > > blink::FocusController if you alt-tab out. > > > > > > Is this due to WebContentsImpl::ReplicatePageFocus not working with > > inner/outer > > > WebContents? Perhaps that should be fixed first? > > > > Without this call, the renderer for the webview does not believe it is > focused. > > There is no highlight if I select an input box and no blinking caret. The > caret > > and focus border do disappear from the embedder though. > > Yup, that very likely indicates a problem with page focus. I ran into the same > issue when fixing keyboard input in OOPIFs (see https://crbug.com/530663, > r355833 and https://goo.gl/B9S5ou). > > I don't think having this call here is correct -- it's a bandaid over the real > problem, and it might break things (see my earlier comment). FWIW, I've tried > out the current CL with > https://chrome.google.com/webstore/detail/browser-sample/edggnmnajhcbhlnpjnog..., > and noticed that if you focus the Google search <input> field, and then click on > a different window (or just alt-tab out), that keeps the Google keyboard focus > blinking in the inactive webview window, which is wrong since the webview window > lost page focus. > > I think we should just fix WebContentsImpl::ReplicatePageFocus for inner/outer > contents and whatever else is necessary for page focus instead of this call. > (Fine to do it in a separate CL before/after.) Added note in code. Will fix in follow-up cl. My first attempt to fix ReplicatePageFocus did not help here. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4530: // we have one. On 2016/05/19 00:08:09, alexmos wrote: > On 2016/05/16 20:26:43, avallee wrote: > > On 2016/05/12 20:28:40, alexmos wrote: > > > I had trouble following this comment when I first read it. It makes sense > > after > > > looking at the code, but maybe try to rephrase to make it more clear? The > > > second paragraph is especially hard to follow. > > > > > > I.e., this is really doing three distinct steps: > > > 1) Recursively clear focus in all inner WebContents. > > > 2) Set |node| as the focused FTN in the current WebContents. > > > 3) If there's an outer WebContents, update frame focus in its FrameTree to > the > > > FTN representing the current WebContents. Recursively do this for all > > > WebContents ancestors. > > > > > > Let me make sure I understand this with an example (which may be useful in > the > > > comment as well). If an embedder page A contains a <webview> B, which embeds > a > > > subframe C, and focus shifts from A to C, then the outer FrameTree will > update > > > its focused FTN from A to the FTN corresponding to the inner FrameTree, and > > the > > > inner FrameTree will update its focused FTN from null to the FTN for > subframe > > C. > > > Later, if we focus the embedder again, we will clear focused frame in the > > inner > > > FrameTree, and set the focused frame to FTN A in the outer FrameTree. Is my > > > example correct? > > > > Here's an enhanced version of your example > > https://goo.gl/photos/GJLpaqP8DY7GJ5pD8 > > > > Initially, 3 is focused (1 implicitly by being the root web contents). If you > > were you click on if2, we want to focus 5 and then focus 7 (1 is focused > > implicitly as the root web contents, 2 is focused since frame 5 is focused). > > > > When moving back to the embedder, we would clear 7 and then focus 3. > > > > Here are two situations that I expect to happen eventually and requires the > > slightly more complex implementation. > > > > Document 1 > > <p>Content</p> > > <webview name="page1"><!-- focus is here --></webview> > > <webview name="page2"></webview> > > > > In this document, if we clicked on the <p>, the web contents that originates > the > > event is the outer one. It would unfocus its inner contents and then focus > > itself. > > > > If we clicked on the sibling <webview>, its webcontents would trigger the > focus > > change. It has no inner web contents, but needs to unfocus its sibling first. > > There is no recursion into inner contents, then we set focus in the outer > > context. The outer context would unfocus the currently focused inner context, > > possibly recur into an outer webcontents (not in the case of this example) and > > then set focus in its own frame tree. Finally, it would return and the inner > > contents would set focus in its own frame tree. > > > > In this chain, there is never more than one focused element at a time. > > > > > > Document 2 > > <div id=embedder> > > <webview></webview> > > <guestview name=a><guestview name=b><!--focus starts here > > --></guestview></guestview> > > </div> > > > > Same idea here. If we want to focus guestview a, we need to unfocus b, then go > > up to the embedder, nothing needs to be done there, then finally update the > free > > for the middle guestview. > > > > I'm not sure if that was super clear, but that's the main idea. > > > > The simpler approach might be to start at the root web contents (or focused > > common ancestor to both the old and new focus web contents) and do an unfocus > > traversal, then from the focused contents, traverse parents setting focus > until > > finally focusing itself. > > Thanks for the detailed examples, this makes sense. I didn't consider the > sibling cases, so it helps to walk through that example. Is it important that > the walk through outer contents happens before setting focus in the current > FrameTree? It doesn't seem to matter, but wanted to verify. > > Here's a different idea: instead of maintaining focused frames across the whole > WebContents ancestor chain, what if we only maintained one focused FrameTreeNode > across all inner/outer WebContents for a given page? I.e., in your enhanced > example, when you click on if2, just focus 7, and clear focus from 3 (leaving > the FrameTree at the root without a focused frame). Would anything break if you > didn't actually have 5 focused? This would make things very simple: just > unfocus stuff in the old WebContentsTreeNode, and focus stuff in the new one > (though it probably would need to keep track of the currently focused > WebContentsTreeNode somewhere). Your approach I think would work nicely. It's simple and I think all we need to keep track of is which web contents has the currently focused tree. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4554: outer_contents->SetFocusedFrameInternal( On 2016/05/19 00:08:09, alexmos wrote: > On 2016/05/16 20:26:44, avallee wrote: > > On 2016/05/12 20:28:40, alexmos wrote: > > > Please follow up on this hack with BrowserPlugin experts - I don't really > know > > > how BrowserPlugin focus works and whether this makes sense. > > > > We tested it and it seems we can only embed in the main frame. > > Acknowledged. Can you remove the "HACK" from the comment then? Deleted this whole function. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4563: if (outer_proxy) On 2016/05/19 00:08:09, alexmos wrote: > On 2016/05/16 20:26:44, avallee wrote: > > On 2016/05/12 20:28:40, alexmos wrote: > > > Why would outer_proxy be null? After talking through <webview> proxy setup > > with > > > lfg@, it seems to me that this proxy should always exit. > > > > Agreed, it should exist. > > So then, can you remove this if? Deleted https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4564: outer_proxy->SetFocusedFrame(); On 2016/05/19 00:08:09, alexmos wrote: > On 2016/05/16 20:26:43, avallee wrote: > > On 2016/05/12 20:28:42, alexmos wrote: > > > Won't you notify the embedder's SiteInstance as part of > > > FrameTree::SetFocusedFrame when it's called as part of > > > outer_contents->SetFocusedFrameInternal? Why is this separate call > necessary? > > > > For the webview specific case, the rfp that we want to update is owned by the > > root of the inner frame tree for the chrome-guest://... site instance no not > > findable in the parent frame tree. Also, I'm not 100% on this, but it also > seems > > like CollectSiteInstances in the inner frame tree does not find that instance > > and so never notifies it. > > Sorry, I'm confused about this. Consider the sample webview app in the crbug. > The embedder's FrameTree will have two FTNs, with the following SiteInstances > for current_frame_host(): > FTN_1 [chrome-extension://extid] > | > FTN_2 [chrome-extension://extid] > > FTN_1 is for the main page, FTN_2 is for the <webview> (it's sitting at > about:blank, its RFH is unused (its RenderFrame isn't live), and its > SiteInstance matches its parent). > > The inner contents will have a FTN_3, with SiteInstance [chrome-guest://extid]. > That FTN will also have a proxy RFP_1 for [chrome-extension://extid] (e.g., > this allows the embedder to postMessage the webview). IIUC, there are no > proxies created initially in the outer FrameTree FTNs, but one may be created > on-demand (RFP_2) for [chrome-guest://extid] in FTN_1 if embedder postMessages > the guest, so that the guest can reply to the message. > > Now, the code as written will find the proxy RFP_1 and set the corresponding > remote frame as focused in the chrome-extension:// renderer. But you said "for > the chrome-guest://... site instance", and there's no such proxy in FTN_3. Did > you mean the chrome-extension:// RFP_1? > > Now, in my original question, I was surprised by this, since I thought the > chrome-extension:// renderer should update focus when processing its own > FrameTree. But I think I know why: when you recurse into outer WebContents and > set focus to FTN_2, it tries to send a message to its current_frame_host(), but > that isn't live, so the SetFocusedFrame message isn't sent and > chrome-extensions:// renderer isn't notified. This is why you need to go > through RFP_1. Correct? > > Additionally, I agree with you that RFP_1 won't be found by CollectSiteInstances > in inner FrameTree::SetFocusedFrame, since chrome-extension:// won't correspond > to any of the RFHs in the inner tree. > > OK, the outer_proxy makes sense then. But it's really subtle and needs a > thorough comment for why this isn't handled in any of the > FrameTree::SetFocusedFrame calls. :) I think that this is no longer necessary with the much simplified version of focus with no recursion. I must check that the CL for page focus makes this correct. https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:832: const WebContentsImpl* FindContentsForFrame(int frame_id) const; On 2016/05/19 00:08:09, alexmos wrote: > On 2016/05/16 20:26:44, avallee wrote: > > On 2016/05/12 20:28:42, alexmos wrote: > > > Are two declarations for this really necessary? > > > > Find frame makes sense as a const function, but should not return a non-const > > pointer to the parent which could get me back with a non-const pointer to > this. > > Without an explicit const_cast. > > > > This is in the spirit of Effective C++, item 3: Use const whenever possible. > > (This pattern is on page 23 of the third edition) > > I wasn't sure myself, so I ran this by nick@ and I'm passing on his response. > There is a convention regarding const in the content API: > "don't add the const identifier to interfaces. For interfaces implemented by the > embedder, we can't make assumptions about what the embedder needs to implement > it. For interfaces implemented by content, the implementation details doesn't > have to be exposed." > (from https://www.chromium.org/developers/content-module/content-api) > > So, WebContents isn't supposed to be const-correct, so a function that returns a > const WebContentsImpl isn't very meaningful. Unfortunately, lots of WebContents > methods are const, which disagrees with the theory and ideally would be cleaned > up -- but nick@ apparently doesn't hate it enough to go rip out const. :) Ack. So even though we implement WebContents, and apparently an embedder wouldn't be expected to implement this, the interface is still not supposed to have const methods? That's fine then. Const correctness is a nice thing for reasoning about things unfortunately. https://codereview.chromium.org/1934703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2906: if (page()->focusController().focusedFrame() == frame->toImplBase()->frame()) { On 2016/05/17 15:13:18, avallee wrote: > On 2016/05/16 20:28:58, dcheng wrote: > > Based on earlier comments, I thought we agreed that this would never be called > > when this isn't true. Can we just ASSERT() this instead of making it an if()? > > Sorry, hadn't hit save there. Done. So after test it is very much possible for these to be different. I'm not sure if it makes sens to move this into the page and always unfocus or keep it with the if statement saying: "If this frame is focused, it should lose focus now". https://codereview.chromium.org/1934703002/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1934703002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:254: if (node == nullptr) { On 2016/05/19 00:08:09, alexmos wrote: > nit: !node Done. https://codereview.chromium.org/1934703002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:261: root()->current_frame_host()->UpdateAXTreeData(); On 2016/05/19 at 00:08:09, alexmos wrote: > Should all three statements in the outer if only happen if (!node && GetFocusedFrame())? Seems like you'd call UpdateAXTreeData redundantly if you try to clear focus from a FrameTree that already had it cleared. If already nullptr, then: if (node == GetFocusedFrame()) above would early-out. https://codereview.chromium.org/1934703002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4559: outer_node->parent()->current_frame_host()->GetSiteInstance(); On 2016/05/19 at 00:08:09, alexmos wrote: > I don't think you need parent() here -- seems like the outer_node should be in the same SiteInstance as its parent. lfg suggested the change as the outer_node does not have a live frame_host. (It's swapped out) Edit: It's all gone now. https://codereview.chromium.org/1934703002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1934703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2906: if (page()->focusController().focusedFrame() == frame->toImplBase()->frame()) { Had to bring this back. See reply to comment on previous patchset. Does it make sense to unfocus for the whole page via a single frame? I've hit the assert in several situations while testing.
On 2016/05/24 at 20:07:08, avallee wrote: > alexmos: Simplified the approach by tracking which webcontents has focus in the webcontents tree and only touching at most 2 frame trees. > > dcheng: It is possible to call unfocus on a different frame than the focused one. > when/how?
On 2016/05/24 20:41:54, dcheng wrote: > On 2016/05/24 at 20:07:08, avallee wrote: > > alexmos: Simplified the approach by tracking which webcontents has focus in > the webcontents tree and only touching at most 2 frame trees. > > > > dcheng: It is possible to call unfocus on a different frame than the focused > one. > > > > when/how? So the assert is triggered when the focus change is started by someone remote to the frame being focused. For example, the embedder calling webview.focus() or tabbing from the embedder into the guest will assert false in the embedding renderer. This happens since the FocusController::focusedFrame() returns nullptr if the focused frame is remote. So when trying to clear focus in the embedder, it already thinks it doesn't have focus and m_focusedFrame == the frame we are trying to clear. What do you think of: WebFrame* focusedFrame = page()->focusController().focusedFrame(); ASSERT(!focusedFrame || focusedFrame == frame->toImplBase()->frame()); page()->...
Great, thanks for simplifying the approach, I think this looks much cleaner! Some more nits and cleanup below, but I think this is mostly ready; apart from still needing a test and the page focus issue. I left some comments on how I think we can deal with page focus, and also a question for lfg@ about storing tree-level stuff for WebContentsTreeNodes. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:369: outer_web_contents_->node_->SetFocusedWebContents(outer_web_contents_); nit: maybe add a comment that we can only get here if outer_web_contents_ is the the root of the WebContents tree? (something similar to what you already have in GetFocusedWebContents) https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4533: // 3. Set current contents as focus a couple of nits (just to be more precise with class names): 1. Find old focused frame and unfocus it. 2. Focus new frame in the current FrameTree. 3. Set current WebContents as focused. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4535: // Find root contents. Find focused contents. nit: this comment is probably unnecessary (root contents is an implementation detail of GetFocusedWebContents). https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4543: // Notify proxy? what does this refer to? https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4549: rwh->Focus(); We don't want this to break default modes in Chrome. If we have to fix this in a followup, let's at least restrict this to only be done when --use-cross-process-frames-for-guests is on, and only if old_focused_contents doesn't match |this|. This is the only case when it matters, right? https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4549: rwh->Focus(); I poked at this a bit more to refresh my memory on how page focus works following our chat. I think there are two main things to take care of: 1. When the outermost WC changes page focus, you need to do the same for all inner WC. Since ReplicatePageFocus only does it for proxies and not the main frame's SiteInstance, you can just use RWH::Focus instead, which does both. So, something like this in WC::ReplicatePageFocus: for (const auto& inner : node_->inner_web_contents_tree_nodes_) { WebContentsImpl *inner_contents = inner.second->web_contents_; RenderWidgetHostImpl* rwh = inner_contents->frame_tree_.root()->current_frame_host()->GetRenderWidgetHost(); if (is_focused) rwh->Focus(); else rwh->Blur(); } 2. When a new inner WC gets created, it probably needs to inherit page focus from its outer WC. So if the outer WC was focused, the new inner WC needs to be focused also. We had to do something similar to pass page focus to new OOP subframe renderers: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... In this case, you probably want to put some sort of check in AttachToOuterWebContentsFrame, something along the lines of if (outer_web_contents->GetRenderWidgetHostView()->HasFocus()) GetMainFrame()->GetRenderWidgetHost()->Focus(); https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4929: WebContentsImpl* WebContentsImpl::GetOutermostWebContents() { nit: prefer to match up the order of all new functions in web_contents_impl.h and here. (e.g., GetFocusedWebContents can go above this to match header order.) https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* get_focused_web_contents() { nit: just focused_web_contents(), mirroring naming for outer_web_contents() (and it probably fits on one line) https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:835: WebContentsImpl* web_contents_; This looks unused now. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:842: ChildrenMap inner_web_contents_tree_nodes_; Is the transformation to use a map here still necessary? (It doesn't seem like this CL needs it anymore, though looks like it enables a more efficient cleanup in the destructor. I'm ok keeping it if that's something desirable for <webview> in general, since you've already done the work, though it might be cleaner in a separate CL.) https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:845: WebContentsImpl* focused_web_contents_; It's unfortunate that we have to keep this around for each WebContentsTreeNode, but I don't see a good/easy alternative either. Lucas, does this seem ok? Maybe we should introduce something like a WebContentsFrameTree that could hold the root WebContentsTreeNode and where we could also put something like this? https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:985: // Returns the focused WebContents. nit: expand a bit on what this is for. I.e., if there are inner/outer WebContents (for <webview>, <guestview>, ...), return the WebContents that contains the currently focused frame. Otherwise just returns a pointer to itself.
Patchset #7 (id:160001) has been deleted
PTAL. Added test and addressed alexmos comments. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:369: outer_web_contents_->node_->SetFocusedWebContents(outer_web_contents_); On 2016/05/26 00:32:25, alexmos wrote: > nit: maybe add a comment that we can only get here if outer_web_contents_ is the > the root of the WebContents tree? (something similar to what you already have > in GetFocusedWebContents) Done. Does it make sense to assert that we don't have children when we connect to an outer web contents? I think the assert above would trip anyways since we connect an set focus in the outer contents which would then fail the assert if trying to connect up to an even more outer contents. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4533: // 3. Set current contents as focus On 2016/05/26 00:32:25, alexmos wrote: > a couple of nits (just to be more precise with class names): > 1. Find old focused frame and unfocus it. > 2. Focus new frame in the current FrameTree. > 3. Set current WebContents as focused. Done. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4535: // Find root contents. Find focused contents. On 2016/05/26 00:32:25, alexmos wrote: > nit: this comment is probably unnecessary (root contents is an implementation > detail of GetFocusedWebContents). Done. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4543: // Notify proxy? On 2016/05/26 00:32:25, alexmos wrote: > what does this refer to? Mental implementation detail. Should have been removed. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4549: rwh->Focus(); On 2016/05/26 00:32:25, alexmos wrote: > I poked at this a bit more to refresh my memory on how page focus works > following our chat. > > I think there are two main things to take care of: > > 1. When the outermost WC changes page focus, you need to do the same for all > inner WC. Since ReplicatePageFocus only does it for proxies and not the main > frame's SiteInstance, you can just use RWH::Focus instead, which does both. So, > something like this in WC::ReplicatePageFocus: > > for (const auto& inner : node_->inner_web_contents_tree_nodes_) { > WebContentsImpl *inner_contents = inner.second->web_contents_; > RenderWidgetHostImpl* rwh = > inner_contents->frame_tree_.root()->current_frame_host()->GetRenderWidgetHost(); > if (is_focused) > rwh->Focus(); > else > rwh->Blur(); > } > > 2. When a new inner WC gets created, it probably needs to inherit page focus > from its outer WC. So if the outer WC was focused, the new inner WC needs to be > focused also. We had to do something similar to pass page focus to new OOP > subframe renderers: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > In this case, you probably want to put some sort of check in > AttachToOuterWebContentsFrame, something along the lines of > if (outer_web_contents->GetRenderWidgetHostView()->HasFocus()) > GetMainFrame()->GetRenderWidgetHost()->Focus(); > I've restricted for now. My trial implementation of WC::ReplicatePageFocus was pretty much the same but had issues with Focus() calling right back into WC::ReplicatePageFocus. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4929: WebContentsImpl* WebContentsImpl::GetOutermostWebContents() { On 2016/05/26 00:32:25, alexmos wrote: > nit: prefer to match up the order of all new functions in web_contents_impl.h > and here. (e.g., GetFocusedWebContents can go above this to match header > order.) Done. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:827: WebContentsImpl* get_focused_web_contents() { On 2016/05/26 00:32:25, alexmos wrote: > nit: just focused_web_contents(), mirroring naming for outer_web_contents() (and > it probably fits on one line) Done. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:835: WebContentsImpl* web_contents_; On 2016/05/26 00:32:25, alexmos wrote: > This looks unused now. Removed. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:842: ChildrenMap inner_web_contents_tree_nodes_; On 2016/05/26 00:32:26, alexmos wrote: > Is the transformation to use a map here still necessary? (It doesn't seem like > this CL needs it anymore, though looks like it enables a more efficient cleanup > in the destructor. I'm ok keeping it if that's something desirable for > <webview> in general, since you've already done the work, though it might be > cleaner in a separate CL.) I think it will be necessary for page focus setting. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:985: // Returns the focused WebContents. On 2016/05/26 00:32:26, alexmos wrote: > nit: expand a bit on what this is for. I.e., if there are inner/outer > WebContents (for <webview>, <guestview>, ...), return the WebContents that > contains the currently focused frame. Otherwise just returns a pointer to > itself. Done.
Thanks! One new question about updating focus when destroying WebContentsTreeNodes, but other than that this is ready to go from my side. A couple more nits below. The new test looks good, but I don't really know how <webview> tests work, so it'd be good if someone more familiar with them takes a look at it (Lucas or James?) https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:369: outer_web_contents_->node_->SetFocusedWebContents(outer_web_contents_); On 2016/06/07 15:37:43, avallee wrote: > On 2016/05/26 00:32:25, alexmos wrote: > > nit: maybe add a comment that we can only get here if outer_web_contents_ is > the > > the root of the WebContents tree? (something similar to what you already have > > in GetFocusedWebContents) > > Done. > > Does it make sense to assert that we don't have children when we connect to an > outer web contents? I think the assert above would trip anyways since we connect > an set focus in the outer contents which would then fail the assert if trying to > connect up to an even more outer contents. Yeah, it seems that the DCHECK should be sufficient. https://codereview.chromium.org/1934703002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4549: rwh->Focus(); On 2016/06/07 15:37:43, avallee wrote: > On 2016/05/26 00:32:25, alexmos wrote: > > I poked at this a bit more to refresh my memory on how page focus works > > following our chat. > > > > I think there are two main things to take care of: > > > > 1. When the outermost WC changes page focus, you need to do the same for all > > inner WC. Since ReplicatePageFocus only does it for proxies and not the main > > frame's SiteInstance, you can just use RWH::Focus instead, which does both. > So, > > something like this in WC::ReplicatePageFocus: > > > > for (const auto& inner : node_->inner_web_contents_tree_nodes_) { > > WebContentsImpl *inner_contents = inner.second->web_contents_; > > RenderWidgetHostImpl* rwh = > > > inner_contents->frame_tree_.root()->current_frame_host()->GetRenderWidgetHost(); > > if (is_focused) > > rwh->Focus(); > > else > > rwh->Blur(); > > } > > > > 2. When a new inner WC gets created, it probably needs to inherit page focus > > from its outer WC. So if the outer WC was focused, the new inner WC needs to > be > > focused also. We had to do something similar to pass page focus to new OOP > > subframe renderers: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > > In this case, you probably want to put some sort of check in > > AttachToOuterWebContentsFrame, something along the lines of > > if (outer_web_contents->GetRenderWidgetHostView()->HasFocus()) > > GetMainFrame()->GetRenderWidgetHost()->Focus(); > > > > I've restricted for now. > > My trial implementation of WC::ReplicatePageFocus was pretty much the same but > had issues with Focus() calling right back into WC::ReplicatePageFocus. OK, let's deal with that in a followup CL. It should be ok that Focus() actually calls ReplicatePageFocus again, since that'll now happen on inner WCs, i.e. this is what you can use to recurse down into all inner contents. https://codereview.chromium.org/1934703002/diff/180001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/1934703002/diff/180001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1360: gfx::Point(40, 40)); This expected to land on the <input> field created in the guest by createInput, right? (Might be good to note that in a comment.) Also, are we guaranteed to wait for the <input> to be created and visible before this is dispatched, and that the coordinates won't change for some weird platform? (These have caused flakiness issues for similar tests in the past.) https://codereview.chromium.org/1934703002/diff/180001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1364: EXPECT_NE(embedder_web_contents()->GetFocusedFrame(), embedder_web_contents()->GetFocusedFrame() should be null at this point, no? Can we just expect that? https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js (right): https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:397: no blank line https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:398: embedder.testFocus_(function(webview){ nit: space before { https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:417: if (data[0] == 'response-inputValue', data[1] == expected) { should that comma be &&? https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:337: WebContentsImpl::WebContentsTreeNode::~WebContentsTreeNode() { What will happen if the currently focused WebContents gets destroyed, for example if a focused <webview> WC gets detached? Should we update focused_web_contents_ to the embedder or to null in that case? https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:364: // This will reached when connecting into a new WebContents tree. We s/will reached/will only be reached/ https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:365: // implicitly set the root of this tree as being focused since this is The part after "since" was difficult to parse ("called in the process of" made me think of which process this is called in, i.e. browser process). I think you can just remove that part and leave the rest. Something like "This will only be reached when creating a new WebContents tree. Initialize the root of this tree and set it as focused." https://codereview.chromium.org/1934703002/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4166: if (!node_) { nit: { not necessary
alexmos, lfg: PTAL. https://codereview.chromium.org/1934703002/diff/180001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/1934703002/diff/180001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1360: gfx::Point(40, 40)); On 2016/06/07 22:03:43, alexmos wrote: > This expected to land on the <input> field created in the guest by createInput, > right? (Might be good to note that in a comment.) > > Also, are we guaranteed to wait for the <input> to be created and visible before > this is dispatched, and that the coordinates won't change for some weird > platform? (These have caused flakiness issues for similar tests in the past.) 1. Comment added. 2. Looking at it, I think you're right, this won't be synchronized. Added a round trip in the javascript to release the test to this step once the input is known to be in the DOM tree of the webview. https://codereview.chromium.org/1934703002/diff/180001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1364: EXPECT_NE(embedder_web_contents()->GetFocusedFrame(), On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > embedder_web_contents()->GetFocusedFrame() should be null at this point, no? > Can we just expect that? Done. https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js (right): https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:397: On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > no blank line Done. https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:398: embedder.testFocus_(function(webview){ On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > nit: space before { Done. https://codereview.chromium.org/1934703002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:417: if (data[0] == 'response-inputValue', data[1] == expected) { On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > should that comma be &&? Should have been a nested if. https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:337: WebContentsImpl::WebContentsTreeNode::~WebContentsTreeNode() { On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > What will happen if the currently focused WebContents gets destroyed, for > example if a focused <webview> WC gets detached? Should we update > focused_web_contents_ to the embedder or to null in that case? It feels like there should be no element with focus when detached. I think we should match what happens when we form a tree in the first place, which is to set the top of the tree as focused. I'm not sure where WebContents get destroyed but there does not seem to be a detach method. The only way to get to this destructor seems to be when WebContentsImpl is destroyed. https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:364: // This will reached when connecting into a new WebContents tree. We On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > s/will reached/will only be reached/ Done. https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:365: // implicitly set the root of this tree as being focused since this is On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > The part after "since" was difficult to parse ("called in the process of" made > me think of which process this is called in, i.e. browser process). I think you > can just remove that part and leave the rest. Something like "This will only be > reached when creating a new WebContents tree. Initialize the root of this tree > and set it as focused." Done. https://codereview.chromium.org/1934703002/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4166: if (!node_) { On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > nit: { not necessary Done.
Preemptively sending out a couple more nits for the latest PS for when you're back. I think this is basically ready to go once we deal with the new test failing with --site-per-process. Depending on the cause, it might be ok to land this with the test disabled for --site-per-process and work on it in a followup, especially if it passes with --isolate-extensions, since we do want to land this before branch cut next week (this is a blocking bug for --isolate-extensions). https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:337: WebContentsImpl::WebContentsTreeNode::~WebContentsTreeNode() { On 2016/06/15 13:32:10, avallee wrote: > On 2016/06/07 22:03:43, alexmos (slow until 6-20) wrote: > > What will happen if the currently focused WebContents gets destroyed, for > > example if a focused <webview> WC gets detached? Should we update > > focused_web_contents_ to the embedder or to null in that case? > > It feels like there should be no element with focus when detached. I think we > should match what happens when we form a tree in the first place, which is to > set the top of the tree as focused. > > I'm not sure where WebContents get destroyed but there does not seem to be a > detach method. The only way to get to this destructor seems to be when > WebContentsImpl is destroyed. Acknowledged. https://codereview.chromium.org/1934703002/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1368: if (GetParam()) { nit: { not necessary https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:431: if(data[1] == expected) { nit: space after if https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js:15: window.console.log('replying with data" '+ JSON.stringify(data)); Was this for debugging? (Or, if you wanted to keep it, there's the LOG helper right above.) https://codereview.chromium.org/1934703002/diff/220001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/1934703002/diff/220001/content/public/test/br... content/public/test/browser_test_utils.cc:1299: struct FrameFocusedObserver::FrameTreeNodeObserverImpl Any reason why this is a struct and not a class?
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by avallee@chromium.org
Description was changed from ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. BUG=600395 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. BUG=600395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. BUG=600395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. ~ Reworked RenderWidgetHostViewGuest to use MaybeSendSyntheticTapGesture for Touch and Mouse event. ~ Edited SelectShowHide for direct routing. BUG=600395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
PTAL alexmos: I think everything is green and submittable now. I got all the tests that fail. wjmaclean: Peek at my reworking of the synthetic tap gesture being sent. https://codereview.chromium.org/1934703002/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1368: if (GetParam()) { On 2016/06/24 01:38:49, alexmos wrote: > nit: { not necessary Done. https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:431: if(data[1] == expected) { On 2016/06/24 01:38:49, alexmos wrote: > nit: space after if Done. https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js:15: window.console.log('replying with data" '+ JSON.stringify(data)); On 2016/06/24 01:38:50, alexmos wrote: > Was this for debugging? (Or, if you wanted to keep it, there's the LOG helper > right above.) It was, delorted.
PTAL alexmos: I think everything is green and submittable now. I got all the tests that fail. wjmaclean: Peek at my reworking of the synthetic tap gesture being sent. https://codereview.chromium.org/1934703002/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1368: if (GetParam()) { On 2016/06/24 01:38:49, alexmos wrote: > nit: { not necessary Done. https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js:431: if(data[1] == expected) { On 2016/06/24 01:38:49, alexmos wrote: > nit: space after if Done. https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js:15: window.console.log('replying with data" '+ JSON.stringify(data)); On 2016/06/24 01:38:50, alexmos wrote: > Was this for debugging? (Or, if you wanted to keep it, there's the LOG helper > right above.) It was, delorted.
lgtm re sending of focusing SyntheticGestureTap https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (left): https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:182: // the plugin and thus onwards to the guest. Is it worth keeping some part of this comment around so readers know why MaybeSendSyntheticTapGesture() exists? Possibly should be a one-liner at each call site?
https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (left): https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:182: // the plugin and thus onwards to the guest. On 2016/07/25 19:54:31, wjmaclean wrote: > Is it worth keeping some part of this comment around so readers know why > MaybeSendSyntheticTapGesture() exists? Possibly should be a one-liner at each > call site? Done.
LGTM with nits. Thanks for pulling this through! It's a bit sad that we have to use the synthetic gesture for the BrowserPlugin case, but it's probably the best we can do atm, especially given the precedent for it. Hopefully we'll be able to rip out BrowserPlugin soon enough. :) https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js:15: window.console.log('replying with data" '+ JSON.stringify(data)); On 2016/07/25 19:02:05, avallee wrote: > On 2016/06/24 01:38:50, alexmos wrote: > > Was this for debugging? (Or, if you wanted to keep it, there's the LOG helper > > right above.) > > It was, delorted. This is still present in PS12. https://codereview.chromium.org/1934703002/diff/220001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/1934703002/diff/220001/content/public/test/br... content/public/test/browser_test_utils.cc:1299: struct FrameFocusedObserver::FrameTreeNodeObserverImpl On 2016/06/24 01:38:50, alexmos (OOO until 7-26) wrote: > Any reason why this is a struct and not a class? Still have this question. :) https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:174: RenderWidgetHostImpl* embedder = static_cast<RenderWidgetHostImpl*>( Is this cast necessary? Calling GetView()->HasFocus() and Focus() seems to be possible directly on a RenderWidgetHost. https://codereview.chromium.org/1934703002/diff/280001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4218: outermost->node_->SetFocusedWebContents(outermost); So with BrowserPlugin, we'll now have |node_| defined for outermost WC even without --use-cpffg, for tracking the focused WebContents? I was curious if any existing code inferred --use-cpffg by checking for a non-null node_, but it doesn't looks like it, so this seems ok.
Looks like this is almost ready to land - please check with me after it does and we can make sure accessibility is working with OOPIF webview, too.
alexmos: All done. dmazzoni: My approach has changed a bit. In my next cl where I fix the actual page-level focus, I plan to remove the unsetting of focused frame and accessibility should work as before and I should be able to enable the tests marked todo: enable in this CL. https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1934703002/diff/280001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:174: RenderWidgetHostImpl* embedder = static_cast<RenderWidgetHostImpl*>( On 2016/07/26 21:08:30, alexmos wrote: > Is this cast necessary? Calling GetView()->HasFocus() and Focus() seems to be > possible directly on a RenderWidgetHost. Done. https://codereview.chromium.org/1934703002/diff/280001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1934703002/diff/280001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4218: outermost->node_->SetFocusedWebContents(outermost); On 2016/07/26 21:08:30, alexmos wrote: > So with BrowserPlugin, we'll now have |node_| defined for outermost WC even > without --use-cpffg, for tracking the focused WebContents? I was curious if any > existing code inferred --use-cpffg by checking for a non-null node_, but it > doesn't looks like it, so this seems ok. That's correct.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, lfg@chromium.org, wjmaclean@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/1934703002/#ps320001 (title: "Fix alexmos nits.")
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
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...)
https://codereview.chromium.org/1934703002/diff/320001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1934703002/diff/320001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:204: virtual void unfocusDocumentView(WebFrame*) = 0; Please add a comment and describe what the argument is supposed to be. I think I would personally prefer that we just remove the argument or find another way to express it... it's unused except for the DCHECK. In this particular instance, I think the embedder can just express the DCHECK directly by using WebView::focusedFrame?
On 2016/07/27 05:13:19, dcheng wrote: > https://codereview.chromium.org/1934703002/diff/320001/third_party/WebKit/pub... > File third_party/WebKit/public/web/WebView.h (right): > > https://codereview.chromium.org/1934703002/diff/320001/third_party/WebKit/pub... > third_party/WebKit/public/web/WebView.h:204: virtual void > unfocusDocumentView(WebFrame*) = 0; > Please add a comment and describe what the argument is supposed to be. I think I > would personally prefer that we just remove the argument or find another way to > express it... it's unused except for the DCHECK. > > In this particular instance, I think the embedder can just express the DCHECK > directly by using WebView::focusedFrame? I think the DCHECK would be difficult to enforce in the embedder. I think it might not be necessary after all. My plan going forward is to remove the current hack that defocuses the current frame and I think I can make things work correctly with SetActive. This will also remove the call to RenderWidgetHostImpl::Focus() from WebContentsImpl::SetFocusedFrame().
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...
PTAL: alexmos, sorry I missed the comments back on PS9. They've been addressed. dcheng: should be all done for you as well. https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js (right): https://codereview.chromium.org/1934703002/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js:15: window.console.log('replying with data" '+ JSON.stringify(data)); On 2016/07/26 21:08:30, alexmos wrote: > On 2016/07/25 19:02:05, avallee wrote: > > On 2016/06/24 01:38:50, alexmos wrote: > > > Was this for debugging? (Or, if you wanted to keep it, there's the LOG > helper > > > right above.) > > > > It was, delorted. > > This is still present in PS12. Done. https://codereview.chromium.org/1934703002/diff/220001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/1934703002/diff/220001/content/public/test/br... content/public/test/browser_test_utils.cc:1299: struct FrameFocusedObserver::FrameTreeNodeObserverImpl On 2016/07/26 21:08:30, alexmos wrote: > On 2016/06/24 01:38:50, alexmos (OOO until 7-26) wrote: > > Any reason why this is a struct and not a class? > > Still have this question. :) Good point, it's not a POD and should be a class. It's an implementation detail of FrameFocusedObserver.
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_...)
PS13 -> PS16 LGTM, though you'll need to fix the compile error in FrameFocusedObserver.
On 2016/07/28 00:49:00, alexmos wrote: > PS13 -> PS16 LGTM, though you'll need to fix the compile error in > FrameFocusedObserver. Teaches me to upload and dry-run right before going home.
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...
LGTM with nits, thanks! https://codereview.chromium.org/1934703002/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1934703002/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:204: virtual void unfocusDocumentView() = 0; Nit: let's add a comment for this, even if it's fairly obvious what it should do. Perhaps the comment could describe why this is a useful API to add.
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/1934703002/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1934703002/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:204: virtual void unfocusDocumentView() = 0; On 2016/07/28 02:14:18, dcheng (OOO Aug 1 - Aug 11) wrote: > Nit: let's add a comment for this, even if it's fairly obvious what it should > do. Perhaps the comment could describe why this is a useful API to add. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avallee@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
nick or creis: Can you PTAL. LGTM needed for content/. Thanks.
Based on everyone's reviews, LGTM with a few nits. https://codereview.chromium.org/1934703002/diff/400001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1934703002/diff/400001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:189: // possibly changing focus in distinct but related FrameTrees (Guest, Embedder nit: inner/outer WebContents https://codereview.chromium.org/1934703002/diff/400001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1934703002/diff/400001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:842: void SetFocusedWebContents(WebContentsImpl* web_contents); nit: No need for empty line since these two are the setter and getter. https://codereview.chromium.org/1934703002/diff/400001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:853: WebContentsImpl* focused_web_contents_; Can this be arbitrarily deep? It is worth clarifying in the comment. https://codereview.chromium.org/1934703002/diff/400001/content/browser/web_co... File content/browser/web_contents/web_contents_view_child_frame.cc (right): https://codereview.chromium.org/1934703002/diff/400001/content/browser/web_co... content/browser/web_contents/web_contents_view_child_frame.cc:145: // embedder nit: "in outer WebContents"? Also, end the comment with a period.
creis@chromium.org changed reviewers: - creis@chromium.org, nick@chromium.org
Thanks Nasko! I'm moving Nick and myself to CC since you handled the content/ review.
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org, kenrb@chromium.org, lfg@chromium.org, alexmos@chromium.org, dcheng@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1934703002/#ps420001 (title: "Address nasko@ 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 ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. ~ Reworked RenderWidgetHostViewGuest to use MaybeSendSyntheticTapGesture for Touch and Mouse event. ~ Edited SelectShowHide for direct routing. BUG=600395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. ~ Reworked RenderWidgetHostViewGuest to use MaybeSendSyntheticTapGesture for Touch and Mouse event. ~ Edited SelectShowHide for direct routing. BUG=600395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #19 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. ~ Reworked RenderWidgetHostViewGuest to use MaybeSendSyntheticTapGesture for Touch and Mouse event. ~ Edited SelectShowHide for direct routing. BUG=600395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix keyboard focus for OOPIF-<webview>. When using --use-cross-process-frames-for-guests input events were being routed to the embedder which should not be able to process guests' input events. + WebContents tree traversal from parent to child. + WebContents keeps global track of focused frame, unlike frame tree which might not know about inner/outer web contents. + RenderFrameHostImpl::OnFrameFocused delegates back to WebContents instead of frame tree. + Send input to Webcontents that is focused. + Add unfocus message to renderer. When moving focus from a frame in the webview back to the embedder, the inner contents no longer has any focus (the frame with focus is not visible to the inner guest). ~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere. ~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order. ~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility. ~ Reworked RenderWidgetHostViewGuest to use MaybeSendSyntheticTapGesture for Touch and Mouse event. ~ Edited SelectShowHide for direct routing. BUG=600395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a Cr-Commit-Position: refs/heads/master@{#408445} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a Cr-Commit-Position: refs/heads/master@{#408445} |
