content/browser/web_contents/web_contents_impl.cc - Issue 2451143003: <webview>: Correctly shift focus between WebContents.

Side by Side Diff: content/browser/web_contents/web_contents_impl.cc

Issue 2451143003: <webview>: Correctly shift focus between WebContents. (Closed)

Patch Set: Address wjmaclean comment: ternary op. Created 4 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "content/browser/web_contents/web_contents_impl.h"	5 #include "content/browser/web_contents/web_contents_impl.h"

6	6

7 #include <stddef.h>	7 #include <stddef.h>

8	8

9 #include <cmath>	9 #include <cmath>

10 #include <utility>	10 #include <utility>

(...skipping 4608 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
4619 GetSiteInstance());	4619 GetSiteInstance());

4620 } else {	4620 } else {

4621 RenderFrameHostImpl* source_rfhi =	4621 RenderFrameHostImpl* source_rfhi =

4622 static_cast<RenderFrameHostImpl*>(source_rfh);	4622 static_cast<RenderFrameHostImpl*>(source_rfh);

4623 source_rfhi->frame_tree_node()->render_manager()->CreateOpenerProxies(	4623 source_rfhi->frame_tree_node()->render_manager()->CreateOpenerProxies(

4624 GetSiteInstance(), nullptr);	4624 GetSiteInstance(), nullptr);

4625 }	4625 }

4626 }	4626 }

4627 }	4627 }

4628	4628

	4629 void WebContentsImpl::ChangeFocus(WebContentsImpl* old_contents) {

	4630 // Focus is moving between frame trees.

	4631 //

	4632 // Ensure that the embedding frame in each outer WebContent's frame tree is

	4633 // focused, if present.
	alexmos 2016/10/28 06:36:39 Can you please help me understand why we need the Can you please help me understand why we need the while loop? Why isn't BlurDirect on the old_contents and FocusDirect() and the SetFocusedWebContents on the new WebContents sufficient? avallee 2016/10/28 19:19:57 We want to let the embedding renderer(s) know that Show quoted text On 2016/10/28 06:36:39, alexmos wrote: > Can you please help me understand why we need the while loop? Why isn't > BlurDirect on the old_contents and FocusDirect() and the SetFocusedWebContents > on the new WebContents sufficient? We want to let the embedding renderer(s) know that focus has moved. alexmos 2016/10/31 18:57:57 Sorry, I'm still a bit fuzzy on this. So looking Show quoted text On 2016/10/28 19:19:57, avallee wrote: > On 2016/10/28 06:36:39, alexmos wrote: > > Can you please help me understand why we need the while loop? Why isn't > > BlurDirect on the old_contents and FocusDirect() and the SetFocusedWebContents > > on the new WebContents sufficient? > > We want to let the embedding renderer(s) know that focus has moved. Sorry, I'm still a bit fuzzy on this. So looking at the code, basically whenever an inner WebContents becomes focused, this focuses the corresponding frame in the outer WebContents. This seems reasonable, but I can't figure out what exactly breaks if we don't do that? Doesn't seem like it would affect event routing as long as we call SetFocusedWebContents(this). (I tried out your CL without the while loop on a couple of sample webview apps, and everything still appeared to work, hence why I'm confused.) avallee 2016/11/07 19:18:44 Now that I look at it, it seems to work. I don't r Show quoted text On 2016/10/31 18:57:57, alexmos wrote: > On 2016/10/28 19:19:57, avallee wrote: > > On 2016/10/28 06:36:39, alexmos wrote: > > > Can you please help me understand why we need the while loop? Why isn't > > > BlurDirect on the old_contents and FocusDirect() and the > SetFocusedWebContents > > > on the new WebContents sufficient? > > > > We want to let the embedding renderer(s) know that focus has moved. > > Sorry, I'm still a bit fuzzy on this. So looking at the code, basically > whenever an inner WebContents becomes focused, this focuses the corresponding > frame in the outer WebContents. This seems reasonable, but I can't figure out > what exactly breaks if we don't do that? Doesn't seem like it would affect > event routing as long as we call SetFocusedWebContents(this). (I tried out your > CL without the while loop on a couple of sample webview apps, and everything > still appeared to work, hence why I'm confused.) Now that I look at it, it seems to work. I don't remember why I had added this back in initially. I'll delete for now unless it breaks some tests.
	4634 //

	4635 // Send a page level blur to the old contents so that it displays inactive UI

	4636 // and focus this contents to activate it.
	alexmos 2016/10/28 06:36:39 These two lines of the comment seem more appropria These two lines of the comment seem more appropriate below for the code after the while loop. avallee 2016/10/28 19:19:57 Done. Show quoted text On 2016/10/28 06:36:39, alexmos wrote: > These two lines of the comment seem more appropriate below for the code after > the while loop. Done.
	4637 WebContentsImpl* web_contents = this;

	4638 while (web_contents->GetOuterWebContents()) {

	4639 FrameTreeNode* tree_node = FrameTreeNode::GloballyFindByID(

	4640 web_contents->GetOuterDelegateFrameTreeNodeId());

	4641 RenderFrameProxyHost* outer_proxy = web_contents->frame_tree_.root()

	4642 ->render_manager()

	4643 ->GetProxyToOuterDelegate();

	4644 if (!tree_node) {
	lfg 2016/10/27 20:00:35 It took me some time to understand that this can o It took me some time to understand that this can only happen for browser plugin, can you add a comment explaining, with a TODO to remove? (you can reference bug 533069). avallee 2016/10/28 19:19:57 Done. My main worry: <body><webview></webview><if Show quoted text On 2016/10/27 20:00:35, lfg wrote: > It took me some time to understand that this can only happen for browser plugin, > can you add a comment explaining, with a TODO to remove? (you can reference bug > 533069). Done. My main worry: <body><webview></webview><iframe></iframe></body>. Say focus is in the iframe in the BrowserPlugin case. I worry that we need to focus the main frame otherwise BrowserPluginEmbedder won't set focus on the BrowserPlugin. I tested and it seems to work fine though.
	4645 tree_node = web_contents->GetOuterWebContents()

	4646 ->GetMainFrame()
	lfg 2016/10/27 20:00:35 +ekaramad@ FYI. This will probably need to be fixe +ekaramad@ FYI. This will probably need to be fixed in your CL that fixes PDF inside OOPIFs. avallee 2016/11/07 19:18:44 +lfg, +ekaramad@ deleted... Show quoted text On 2016/10/27 20:00:35, lfg wrote: > +ekaramad@ FYI. This will probably need to be fixed in your CL that fixes PDF > inside OOPIFs. +lfg, +ekaramad@ deleted... EhsanK 2016/11/08 16:05:47 Thanks! It probably should. I think webcontents->G Show quoted text On 2016/10/27 20:00:35, lfg wrote: > +ekaramad@ FYI. This will probably need to be fixed in your CL that fixes PDF > inside OOPIFs. Thanks! It probably should. I think webcontents->GetBrowserPluginGuest()->GetEmbedderFrame()-GetSiteInstance() should replace the site instance below.
	4647 ->frame_tree_node();

	4648 outer_proxy = web_contents->frame_tree_.root()
	lfg 2016/10/27 20:00:35 Why do we want to call Focus on this proxy? (assum Why do we want to call Focus on this proxy? (assuming this only happens on BrowserPlugin). avallee 2016/10/28 19:19:56 Doesn't make sense. Deleted. Show quoted text On 2016/10/27 20:00:35, lfg wrote: > Why do we want to call Focus on this proxy? (assuming this only happens on > BrowserPlugin). Doesn't make sense. Deleted.
	4649 ->render_manager()

	4650 ->GetRenderFrameProxyHost(

	4651 tree_node->current_frame_host()->GetSiteInstance());

	4652 }

	4653

	4654 web_contents = web_contents->GetOuterWebContents();

	4655 if (tree_node == web_contents->frame_tree_.GetFocusedFrame())

	4656 break;

	4657 if (outer_proxy) {

	4658 web_contents->frame_tree_.SetFocusedFrame(tree_node,

	4659 outer_proxy->GetSiteInstance());

	4660 outer_proxy->SetFocusedFrame();

	4661 }

	4662 }

	4663

	4664 // Since we are changing focus, we don't want to call Focus()/Blur() which

	4665 // would indirect back into WebContentsImpl(::GetFocusedRenderWidgetHost).

	4666 if (old_contents)

	4667 old_contents->GetMainFrame()->GetRenderWidgetHost()->BlurDirect();

	4668 GetMainFrame()->GetRenderWidgetHost()->FocusDirect();

	4669 GetOutermostWebContents()->node_->SetFocusedWebContents(this);

	4670 }

	4671

4629 void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,	4672 void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,

4630 SiteInstance* source) {	4673 SiteInstance* source) {

4631 if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) {	4674 if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) {

4632 frame_tree_.SetFocusedFrame(node, source);	4675 frame_tree_.SetFocusedFrame(node, source);

4633 return;	4676 return;

4634 }	4677 }

4635

4636 // 1. Find old focused frame and unfocus it.

4637 // 2. Focus the new frame in the current FrameTree.

4638 // 3. Set current WebContents as focused.

4639 WebContentsImpl* old_focused_contents = GetFocusedWebContents();	4678 WebContentsImpl* old_focused_contents = GetFocusedWebContents();

4640 if (old_focused_contents != this) {	4679 if (old_focused_contents != this) {

4641 // Focus is moving between frame trees, unfocus the frame in the old tree.	4680 // Focus is moving between frame trees, unfocus the frame in the old tree.
	alexmos 2016/10/28 06:36:39 Comment looks stale (ChangeFocus tinkers with page Comment looks stale (ChangeFocus tinkers with page focus and doesn't unfocus any frames.) avallee 2016/10/28 19:19:57 Done. Show quoted text On 2016/10/28 06:36:39, alexmos wrote: > Comment looks stale (ChangeFocus tinkers with page focus and doesn't unfocus any > frames.) Done.
4642 old_focused_contents->frame_tree_.SetFocusedFrame(nullptr, source);	4681 ChangeFocus(old_focused_contents);
alexmos 2016/10/28 06:36:39 To sanity check my understanding, previously, old_ To sanity check my understanding, previously, old_focused_contents maintained page focus when the user clicked on a different WebContents on the page, so we had to clear the focused frame for things like making the caret go away from a focused input field. Now, you'll remove page focus from old_focused_contents instead, which will have the same effect, and this allows you to keep the focused frame intact, which is why this call isn't necessary. Correct? If yes, given that this was the only place that ever cleared a focused frame, could we also remove the RFHI::ClearFocusedFrame and all the associated plumbing? avallee 2016/10/28 19:19:56 Yes, we used to clear focus in order to have the r Show quoted text On 2016/10/28 06:36:39, alexmos wrote: > To sanity check my understanding, previously, old_focused_contents maintained > page focus when the user clicked on a different WebContents on the page, so we > had to clear the focused frame for things like making the caret go away from a > focused input field. Now, you'll remove page focus from old_focused_contents > instead, which will have the same effect, and this allows you to keep the > focused frame intact, which is why this call isn't necessary. Correct? > > If yes, given that this was the only place that ever cleared a focused frame, > could we also remove the RFHI::ClearFocusedFrame and all the associated > plumbing? Yes, we used to clear focus in order to have the renderer initiate the WebContents focus change via RPC to set _some_ frame as focused, the side effect being that the right web contents will be focused. This is also why carets and other indicators of being the focused page disappear. Instead ChangeFocusIfNecessary assumes that focus changes are browser initiated, with the SetFocusedFrame hook handling renderer initiated changes. This is indeed the only place that would lead to ClearFocusedFrame. I was hoping to make that a subsequent CL to clean up and seprate the concerns.
4643 GetOutermostWebContents()->node_->SetFocusedWebContents(this);

4644 }	4682 }

4645	4683

4646 frame_tree_.SetFocusedFrame(node, source);	4684 frame_tree_.SetFocusedFrame(node, source);

4647

4648 // TODO(avallee): Remove this once page focus is fixed.

4649 RenderWidgetHostImpl* rwh = node->current_frame_host()->GetRenderWidgetHost();

4650 if (rwh && old_focused_contents != this)

4651 rwh->Focus();

4652 }	4685 }

4653	4686

4654 bool WebContentsImpl::AddMessageToConsole(int32_t level,	4687 bool WebContentsImpl::AddMessageToConsole(int32_t level,

4655 const base::string16& message,	4688 const base::string16& message,

4656 int32_t line_no,	4689 int32_t line_no,

4657 const base::string16& source_id) {	4690 const base::string16& source_id) {

4658 if (!delegate_)	4691 if (!delegate_)

4659 return false;	4692 return false;

4660 return delegate_->AddMessageToConsole(this, level, message, line_no,	4693 return delegate_->AddMessageToConsole(this, level, message, line_no,

4661 source_id);	4694 source_id);

4662 }	4695 }

4663	4696

4664 void WebContentsImpl::OnUserInteraction(	4697 void WebContentsImpl::OnUserInteraction(

4665 RenderWidgetHostImpl* render_widget_host,	4698 RenderWidgetHostImpl* render_widget_host,

4666 const blink::WebInputEvent::Type type) {	4699 const blink::WebInputEvent::Type type) {

4667 // Ignore unless the widget is currently in the frame tree.	4700 // Ignore unless the widget is currently in the frame tree.

4668 if (!HasMatchingWidgetHost(&frame_tree_, render_widget_host))	4701 if (!HasMatchingWidgetHost(&frame_tree_, render_widget_host))

4669 return;	4702 return;

4670	4703

4671 for (auto& observer : observers_)	4704 for (auto& observer : observers_)

4672 observer.DidGetUserInteraction(type);	4705 observer.DidGetUserInteraction(type);

4673	4706

4674 ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();	4707 ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();

4675 // Exclude scroll events as user gestures for resource load dispatches.	4708 // Exclude scroll events as user gestures for resource load dispatches.

4676 // rdh is NULL in unittests.	4709 // rdh is NULL in unittests.

4677 if (rdh && type != blink::WebInputEvent::MouseWheel)	4710 if (rdh && type != blink::WebInputEvent::MouseWheel)

4678 rdh->OnUserGesture();	4711 rdh->OnUserGesture();

4679 }	4712 }

4680	4713

	4714 void WebContentsImpl::EnsureOwningContentsIsFocused(

	4715 RenderWidgetHostImpl* render_widget_host) {

	4716 if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_)

	4717 return;

	4718

	4719 RenderWidgetHostImpl* focused_widget =

	4720 GetFocusedRenderWidgetHost(render_widget_host);

	4721

	4722 if (focused_widget != render_widget_host &&

	4723 focused_widget->delegate() != render_widget_host->delegate()) {

	4724 ChangeFocus(static_cast<WebContentsImpl*>(focused_widget->delegate()));
	lfg 2016/10/27 20:00:35 I'm not sure this cast is safe, a quick search tel I'm not sure this cast is safe, a quick search tells me that InterstitialPageImpl is also a RenderWidgetHostDelegate. avallee 2016/10/28 19:19:56 Moved the check into ChangeFocusIfNeeded(); The c Show quoted text On 2016/10/27 20:00:35, lfg wrote: > I'm not sure this cast is safe, a quick search tells me that > InterstitialPageImpl is also a RenderWidgetHostDelegate. Moved the check into ChangeFocusIfNeeded(); The contents is fetched internally.
	4725 }

	4726 }

	4727

4681 void WebContentsImpl::OnIgnoredUIEvent() {	4728 void WebContentsImpl::OnIgnoredUIEvent() {

4682 // Notify observers.	4729 // Notify observers.

4683 for (auto& observer : observers_)	4730 for (auto& observer : observers_)

4684 observer.DidGetIgnoredUIEvent();	4731 observer.DidGetIgnoredUIEvent();

4685 }	4732 }

4686	4733

4687 void WebContentsImpl::RendererUnresponsive(	4734 void WebContentsImpl::RendererUnresponsive(

4688 RenderWidgetHostImpl* render_widget_host,	4735 RenderWidgetHostImpl* render_widget_host,

4689 RendererUnresponsiveType type) {	4736 RendererUnresponsiveType type) {

4690 for (auto& observer : observers_)	4737 for (auto& observer : observers_)

(...skipping 515 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
5206 dialog_manager_ = dialog_manager;	5253 dialog_manager_ = dialog_manager;

5207 }	5254 }

5208	5255

5209 void WebContentsImpl::RemoveBindingSet(const std::string& interface_name) {	5256 void WebContentsImpl::RemoveBindingSet(const std::string& interface_name) {

5210 auto it = binding_sets_.find(interface_name);	5257 auto it = binding_sets_.find(interface_name);

5211 if (it != binding_sets_.end())	5258 if (it != binding_sets_.end())

5212 binding_sets_.erase(it);	5259 binding_sets_.erase(it);

5213 }	5260 }

5214	5261

5215 } // namespace content	5262 } // namespace content

OLD	NEW

« content/browser/web_contents/web_contents_impl.h ('K') | « content/browser/web_contents/web_contents_impl.h ('k') | no next file » | no next file with comments »