Do not save and restore View focus on aura::Window focus changes.

Do not save and restore View focus on aura::Window focus changes.

My http://crrev.com/271312 was wrong; see http://crbug.com/391376#c16

This resolves the following defects:
1) 394661 - print preview focus is restored on tab switch.
2) 391376 - print preview gets focused from CrOS photo editor.
3) 398766 - saving and restoring focus on bubbles/dialogs.

This reintroduces the following defects fixed by r271312:
1) 368691 - can't re-type in omnibox after focusing print preview
2) omnibox retains focus when re-focusing print preview.
(shows selection/cursor and leaves dropdown window open)

TODO(msw): Fix 368691 again... (web dialog focus issue)

BUG=391376, 394661, 398766, 368691
TEST=Issues 391376, 394661, and 398766 are fixed; 368691 regresses (for now).
R=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286589

[msw, 2014-07-25 19:56:27]

Hey Ben, what's the right way to fix the original defects?
(we can land this and fix those defects later if desired)

[Ben Goodger (Google), 2014-07-25 20:03:00]

lgtm for the revert. can you come by at some point so we can discuss the
original issues?

[msw, 2014-07-29 23:10:24]

Hey Ben, please take another look. I found a simpler solution than the one we
discussed. The dialog WebView wasn't getting proper views focus; that can be
fixed like PS2 or PS3, or maybe you know a better method.

[Ben Goodger (Google), 2014-07-30 02:57:26]

https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2006:
FOR_EACH_OBSERVER(WebContentsObserver, observers_, WasFocused());
So you should be able to trigger this off of the FocusChangeObserver
notification that gets sent via view_->Focus(), rather than adding this new
cross platform notification that's only observed by the views frontend.

https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1252:
web_contents_->Focus();
Can you show me what a typical call stack to here looks like?

[msw, 2014-07-30 03:18:13]

https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2006:
FOR_EACH_OBSERVER(WebContentsObserver, observers_, WasFocused());
On 2014/07/30 02:57:26, Ben Goodger (Google) wrote:
> So you should be able to trigger this off of the FocusChangeObserver
> notification that gets sent via view_->Focus(), rather than adding this new
> cross platform notification that's only observed by the views frontend.

I'll look into this, but the WebView doesn't get Focus()'ed, afaict.

https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1252:
web_contents_->Focus();
On 2014/07/30 02:57:26, Ben Goodger (Google) wrote:
> Can you show me what a typical call stack to here looks like?

Sure, here are three cases I collected with my PS3 applied. It could be bad that
CTRL+P's WebView::OnFocus already calls content::WebContentsImpl::Focus, but
there doesn't seem to be any noticeable fallout in local testing.

Clicking on the NTP from the Omnibox:
  content.dll!content::WebContentsViewAura::GotFocus()
  content.dll!content::RenderViewHostImpl::GotFocus()
  content.dll!content::RenderWidgetHostViewAura::OnWindowFocused(...)
  wm.dll!wm::FocusController::SetFocusedWindow(...)
  wm.dll!wm::FocusController::FocusWindow(...)
  wm.dll!wm::FocusController::WindowFocusedFromInputEvent(...)
  wm.dll!wm::FocusController::OnMouseEvent(...)
  events.dll!ui::EventHandler::OnEvent(...)
  events.dll!ui::EventDispatcher::DispatchEvent(...)
  events.dll!ui::EventDispatcher::DispatchEventToEventHandlers(...)
  events.dll!ui::EventDispatcher::ProcessEvent(...)
  events.dll!ui::EventDispatcherDelegate::DispatchEventToTarget(...)
  events.dll!ui::EventDispatcherDelegate::DispatchEvent(...)
  events.dll!ui::EventProcessor::OnEventFromSource(...)
  events.dll!ui::EventSource::DeliverEventToProcessor(...)
  events.dll!ui::EventSource::SendEventToProcessor(...)
  views.dll!views::DesktopWindowTreeHostWin::HandleMouseEvent(...)
  views.dll!views::HWNDMessageHandler::HandleMouseEventInternal(...)
  views.dll!views::HWNDMessageHandler::HandleMouseMessage(...)
  content.dll!content::LegacyRenderWidgetHostHWND::OnMouseRange(...)
  content.dll!content::LegacyRenderWidgetHostHWND::_ProcessWindowMessage(...)
  content.dll!content::LegacyRenderWidgetHostHWND::ProcessWindowMessage(...)

CTRL+P for the print dialog:
  content.dll!content::WebContentsViewAura::GotFocus()
  content.dll!content::RenderViewHostImpl::GotFocus()
  content.dll!content::RenderWidgetHostViewAura::OnWindowFocused(...)
  wm.dll!wm::FocusController::SetFocusedWindow(...)
  wm.dll!wm::FocusController::FocusWindow(...)
  aura.dll!aura::Window::Focus()
  content.dll!content::RenderWidgetHostViewAura::Focus()
  content.dll!content::WebContentsViewAura::Focus()
  content.dll!content::WebContentsImpl::Focus()
  webview.dll!views::WebView::OnFocus()
  views.dll!views::View::Focus()
  views.dll!views::FocusManager::SetFocusedViewWithReason(...)
  views.dll!views::FocusManager::SetFocusedView(...)
  views.dll!views::View::RequestFocus()
  views.dll!views::Widget::SetInitialFocus(...)
  views.dll!views::NativeWidgetAura::SetInitialFocus(...)
  views.dll!views::NativeWidgetAura::ShowWithWindowState(...)
  views.dll!views::Widget::Show()
  chrome.dll!`anonymous
namespace'::NativeWebContentsModalDialogManagerViews::Show()
 
chrome.dll!web_modal::WebContentsModalDialogManager::ShowDialogWithManager(...)
  chrome.dll!web_modal::WebContentsModalDialogManager::ShowModalDialog(...)
  chrome.dll!web_modal::PopupManager::ShowModalDialog(...)
  chrome.dll!ShowWebModalDialogViews(...)
  chrome.dll!CreateConstrainedWebDialog(...)
 
chrome.dll!printing::PrintPreviewDialogController::CreatePrintPreviewDialog(...)

Clicking the print preview dialog from the omnibox:
  content.dll!content::WebContentsViewAura::GotFocus()
  content.dll!content::RenderViewHostImpl::GotFocus()
  content.dll!content::RenderWidgetHostViewAura::OnWindowFocused(...)
  wm.dll!wm::FocusController::SetFocusedWindow(...)
  wm.dll!wm::FocusController::FocusWindow(...)
  wm.dll!wm::FocusController::WindowFocusedFromInputEvent(...)
  wm.dll!wm::FocusController::OnMouseEvent(...)
  events.dll!ui::EventHandler::OnEvent(...)
  events.dll!ui::EventDispatcher::DispatchEvent(...)
  events.dll!ui::EventDispatcher::DispatchEventToEventHandlers(...)
  events.dll!ui::EventDispatcher::ProcessEvent(...)
  events.dll!ui::EventDispatcherDelegate::DispatchEventToTarget(...)
  events.dll!ui::EventDispatcherDelegate::DispatchEvent(...)
  events.dll!ui::EventProcessor::OnEventFromSource(...)
  events.dll!ui::EventSource::DeliverEventToProcessor(...)
  events.dll!ui::EventSource::SendEventToProcessor(...)
  views.dll!views::DesktopWindowTreeHostWin::HandleMouseEvent(...)
  views.dll!views::HWNDMessageHandler::HandleMouseEventInternal(...)
  views.dll!views::HWNDMessageHandler::HandleMouseMessage(...)
  content.dll!content::LegacyRenderWidgetHostHWND::OnMouseRange(...)
  content.dll!content::LegacyRenderWidgetHostHWND::_ProcessWindowMessage(...)
  content.dll!content::LegacyRenderWidgetHostHWND::ProcessWindowMessage(...)

[msw, 2014-07-30 14:53:04]

Since I'll need to merge to M-37, maybe I'll just do the revert in this CL?
That will regress 368691 (print preview focus), but we can address that later.
Ben, what do you think?

[msw, 2014-07-30 15:29:28]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-30 15:30:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/418093005/60001

[Ben Goodger (Google), 2014-07-30 15:44:03]

https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2006:
FOR_EACH_OBSERVER(WebContentsObserver, observers_, WasFocused());
Not the WebView... view_->Focus() sets focus to an aura::Window. It should be
possible to detect that.

https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1252:
web_contents_->Focus();
So all of these calls are in response to focus being set to the RWHVA's
aura::Window. 

So in this change what you're doing is in response to focus being set, you call
WebContents::Focus(). Looking at the implementation of that method, it seems
that that ends up setting focus to the RWHVA again (a no-op) and notifying the
WebContentsViewDelegate. I'm assuming it's this latter notification that's
meaningful to you. Surely there must be a simpler way of notifying that object
than this confusing loop-de-loop through content classes?

[msw, 2014-07-30 15:53:12]

On 2014/07/30 15:44:03, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/418093005/diff/20001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl.cc:2006:
> FOR_EACH_OBSERVER(WebContentsObserver, observers_, WasFocused());
> Not the WebView... view_->Focus() sets focus to an aura::Window. It should be
> possible to detect that.
> 
>
https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_view_aura.cc (right):
> 
>
https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...
> content/browser/web_contents/web_contents_view_aura.cc:1252:
> web_contents_->Focus();
> So all of these calls are in response to focus being set to the RWHVA's
> aura::Window. 
> 
> So in this change what you're doing is in response to focus being set, you
call
> WebContents::Focus(). Looking at the implementation of that method, it seems
> that that ends up setting focus to the RWHVA again (a no-op) and notifying the
> WebContentsViewDelegate. I'm assuming it's this latter notification that's
> meaningful to you. Surely there must be a simpler way of notifying that object
> than this confusing loop-de-loop through content classes?

I'll address your comments in the followup CL:
https://codereview.chromium.org/431573003/
This CL will simply revert r271312, since we'll need to merge that to M-37.

[commit-bot: I haz the power, 2014-07-30 17:32:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-30 17:37:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-30 17:37:23]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msw, 2014-07-30 17:41:17]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-30 17:42:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/418093005/60001

[commit-bot: I haz the power, 2014-07-30 20:22:26]

Change committed as 286589