Fix views::WebView focus for Print Preview and other web dialogs.

Fix views::WebView focus for Print Preview and other web dialogs.

Focus the dialog's WebView when its WebContents is focused.
Use a FocusChangeObserver to assist with this task.

BrowserView does this for its WebViews by overriding:
  WebContentsDelegate::WebContentsFocused

TODO: Cleanup the WebView/WebContents/etc. focus pattern...
(See Ben's WebView::OnWebContentsFocused TODO)

BUG=368691
TEST=Focusing between the omnibox and print preview dialogs works correctly (the omnibox loses focus, closes any dropdowns, doesn't show cursor/selection).
R=ben@chromium.org

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

[msw, 2014-07-31 16:42:43]

Hey Ben, please take a look, thanks!

I addressed your earlier comments from:
https://codereview.chromium.org/418093005/#msg9

You suggested using a WebContentsViewDelegate notification:
https://codereview.chromium.org/418093005/diff/40001/content/browser/web_cont...


Relatedly, BrowserView focuses its WebViews by overriding
WebContentsDelegate::WebContentsFocused, but it could be awkward to add this
views::WebView behavior tweak to the cross-platform
PrintPreviewWebContentDelegate, its WebDialogWebContentsDelegate base class,
and/or other WebContentsDelegates potentially used for web dialogs.

I haven't looked closely at how to add such behavior to the
WebContentsViewDelegate instances used here, but I can take a look if you think
that's a better approach.

[Ben Goodger (Google), 2014-07-31 18:20:15]

This is probably fine for this instance.

If you wanted to make it more generic I think you might like to bind the
lifetime of your observer to that of the window its observing.

https://codereview.chromium.org/431573003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

https://codereview.chromium.org/431573003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:100:
gfx::NativeView native_view_;
may as well just use aura::Window* here

[Ben Goodger (Google), 2014-07-31 18:20:20]

lgtm

[msw, 2014-07-31 18:35:14]

I'll leave generalization for follow-up; landing!

https://codereview.chromium.org/431573003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

https://codereview.chromium.org/431573003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:100:
gfx::NativeView native_view_;
On 2014/07/31 18:20:15, Ben Goodger (Google) wrote:
> may as well just use aura::Window* here

Done.

[msw, 2014-07-31 18:35:23]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-31 18:36:07]

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

[commit-bot: I haz the power, 2014-07-31 22:38:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-31 22:48:51]

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

[commit-bot: I haz the power, 2014-07-31 22:48:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2014-08-01 00:15:08]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-08-01 00:16:51]

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

[commit-bot: I haz the power, 2014-08-01 14:41:05]

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_...)

[miu, 2014-08-01 17:58:27]

msw: Have you tested that this change doesn't break fullscreen focus behavior
(both Flash and HTML5 video)?

[msw, 2014-08-01 18:17:49]

On 2014/08/01 17:58:27, miu wrote:
> msw: Have you tested that this change doesn't break fullscreen focus behavior
> (both Flash and HTML5 video)?

How could it? Those aren't implemented as ConstrainedWebDialogDelegateViewViews.
CreateConstrainedWebDialog only has three non-test call sites in:
print_preview_dialog_controller.cc, profile_signin_confirmation_dialog.cc, and
certificate_viewer_webui.cc:
https://code.google.com/p/chromium/codesearch#search/&q=CreateConstrainedWebD...

Feel free to patch this in and test it yourself, my machines are unplugged until
Wed for the great migration.

[miu, 2014-08-01 18:47:22]

On 2014/08/01 18:17:49, msw wrote:
> On 2014/08/01 17:58:27, miu wrote:
> > msw: Have you tested that this change doesn't break fullscreen focus
behavior
> > (both Flash and HTML5 video)?
> 
> How could it? Those aren't implemented as
ConstrainedWebDialogDelegateViewViews.
> CreateConstrainedWebDialog only has three non-test call sites in:
> print_preview_dialog_controller.cc, profile_signin_confirmation_dialog.cc, and
> certificate_viewer_webui.cc:
>
https://code.google.com/p/chromium/codesearch#search/&q=CreateConstrainedWebD...
> 
> Feel free to patch this in and test it yourself, my machines are unplugged
until
> Wed for the great migration.

Oh, my mistake.  I had this confused with ChromeWebContentsViewDelegate for some
reason.  This lgtm.

[commit-bot: I haz the power, 2014-08-01 20:39:20]

Change committed as 287082