Fix Flash fullscreen focus regressions, and add interactive_ui_tests.

Fix Flash fullscreen focus regressions, and add interactive_ui_tests.

This revisits focus handling logic relating to Flash fullscreen in
WebContentsImpl.  Before this change, calls to WebContentsImpl::Focus()
would directly call RenderWidgetHostView::Focus(), but this prevented
WebContentsView and its delegate from "shadowing" these focus changes in
the browser UI.  In other words, on Windows/Linux/CrOS, the focus state
for widgets in the ui::views toolkit were not necessarily synchronized
with the render widget's focus state (see bug for details).

90% of this change adds regression testing for everything described
above.  The tests run a simulated fullscreened Flash widget, and then
invoke user keyboard and mouse actions on the browser UI to confirm both
the Flash widget and ui::views are in agreement and respond correctly to
focus changes; also, including focus store/restore when switching tabs.

BUG=403641
TEST=Repro steps in bug 403641; and also that focus remains with the omnibox or tab contents when switching back and forth to other tabs.  New automated interactive_ui_tests.

Committed: https://crrev.com/9e14e49082f175ceccaa93a3329a8816b905ae34
Cr-Commit-Position: refs/heads/master@{#301269}

[miu, 2014-10-22 01:34:03]

miu@chromium.org changed reviewers:
+ raymes@chromium.org, sky@chromium.org

[miu, 2014-10-22 01:56:50]

miu@chromium.org changed reviewers:
+ yzshen@chromium.org

[miu, 2014-10-22 04:01:34]

Please review.

raymes: ppapi/*

sky: content/browser/*

yzshen1:
chrome/browser/ui/fullscreen/flash_fullscreen_interactive_browsertest.cc

[sky, 2014-10-22 15:10:58]

LGTM - but please get an owner for the mac changes. I'm not familiar with that
code.

[yzshen1, 2014-10-22 16:54:12]

chrome/browser/ui/fullscreen/flash_fullscreen_interactive_browsertest.cc LGTM

(But I don't know the actual logic well.)

[miu, 2014-10-22 19:33:29]

On 2014/10/22 16:54:12, yzshen1 wrote:
> chrome/browser/ui/fullscreen/flash_fullscreen_interactive_browsertest.cc LGTM
> 
> (But I don't know the actual logic well.)

yzshen: Since I've been the main designer/implementer of Fullscreen-Within-Tab,
and also had to do a lot of browser-side replumbing work for Flash fullscreen,
I'd be happy to be added to the OWNERS files in the relevant locations.  If this
SGTY, I'll whip up another change.

[miu, 2014-10-22 19:33:45]

miu@chromium.org changed reviewers:
+ avi@chromium.org

[miu, 2014-10-22 19:34:54]

avi: Could you take a look at the Mac changes?  (Just this file:
content/browser/web_contents/web_contents_view_mac.mm)

[yzshen1, 2014-10-22 19:37:48]

Sounds good. Thanks!
And I don't think I should stay as an OWNER for this area anymore.

You might want to send the review to an owner of the parent folder.

[raymes, 2014-10-24 00:51:08]

lgtm on the ppapi/* changes with a couple of nits.

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
File ppapi/tests/test_flash_fullscreen_for_browser_ui.cc (right):

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
ppapi/tests/test_flash_fullscreen_for_browser_ui.cc:22: // browser UI testing
needs requires it to remain and to be interactive.
nit: "UI testing requires it to remain"

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
ppapi/tests/test_flash_fullscreen_for_browser_ui.cc:73: return false;
nit: use {} for multiline if

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
File ppapi/tests/test_flash_fullscreen_for_browser_ui.h (right):

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
ppapi/tests/test_flash_fullscreen_for_browser_ui.h:19: // the PPAPI's.  Instead,
this is a simulated Flash plugin, used to place the
nit: PPAPI's->Pepper APIs

[miu, 2014-10-24 21:20:09]

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
File ppapi/tests/test_flash_fullscreen_for_browser_ui.cc (right):

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
ppapi/tests/test_flash_fullscreen_for_browser_ui.cc:22: // browser UI testing
needs requires it to remain and to be interactive.
On 2014/10/24 00:51:08, raymes wrote:
> nit: "UI testing requires it to remain"

Done.

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
ppapi/tests/test_flash_fullscreen_for_browser_ui.cc:73: return false;
On 2014/10/24 00:51:08, raymes wrote:
> nit: use {} for multiline if

Done.

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
File ppapi/tests/test_flash_fullscreen_for_browser_ui.h (right):

https://codereview.chromium.org/670653005/diff/1/ppapi/tests/test_flash_fulls...
ppapi/tests/test_flash_fullscreen_for_browser_ui.h:19: // the PPAPI's.  Instead,
this is a simulated Flash plugin, used to place the
On 2014/10/24 00:51:08, raymes wrote:
> nit: PPAPI's->Pepper APIs

Done.

[Avi (use Gerrit), 2014-10-24 21:28:52]

https://codereview.chromium.org/670653005/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/670653005/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_mac.mm:186: const gfx::NativeView
native_view = rwhv ? rwhv->GetNativeView() : nil;
This chunk of code is repeated thrice. Can we pull it out into a helper
function?

[miu, 2014-10-24 21:42:41]

https://codereview.chromium.org/670653005/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/670653005/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_mac.mm:186: const gfx::NativeView
native_view = rwhv ? rwhv->GetNativeView() : nil;
On 2014/10/24 21:28:52, Avi wrote:
> This chunk of code is repeated thrice. Can we pull it out into a helper
> function?

Done.

[Avi (use Gerrit), 2014-10-24 23:04:05]

https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_mac.mm:171: NSWindow* const
window = [native_view window];
Why add |const| here?

https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_mac.mm:186: const gfx::NativeView
native_view = GetNativeViewForFocus();
Again, why const here and below? It's very unusual to use const everywhere when
dealing with ObjC objects.

[miu, 2014-10-25 00:04:33]

https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_mac.mm:171: NSWindow* const
window = [native_view window];
On 2014/10/24 23:04:05, Avi wrote:
> Why add |const| here?

It's a declaration that the pointer will not be changed for the remainder of the
scope.  More details in other comment response...

https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_mac.mm:186: const gfx::NativeView
native_view = GetNativeViewForFocus();
On 2014/10/24 23:04:05, Avi wrote:
> Again, why const here and below? It's very unusual to use const everywhere
when
> dealing with ObjC objects.

Generally speaking, using const helps the compiler protect from unintended bugs
(e.g., logic that assumes a value does not change).  The Google C++ style guide
speaks to this:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const

Also, an interesting perspective on the topic:
http://gamesfromwithin.com/the-const-nazi

Note that in this code change it's the pointers that are const, not the objects
being pointed at.  Because they're all in methods with with few LOC, I'm okay
with removing them if they annoy you.  I'll admit I'm being very pedantic in
this particular code change, but it's [IMHO] a good habit that I've formed.

[Avi (use Gerrit), 2014-10-25 00:58:18]

On 2014/10/25 00:04:33, miu wrote:
>
https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_view_mac.mm (right):
> 
>
https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
> content/browser/web_contents/web_contents_view_mac.mm:171: NSWindow* const
> window = [native_view window];
> On 2014/10/24 23:04:05, Avi wrote:
> > Why add |const| here?
> 
> It's a declaration that the pointer will not be changed for the remainder of
the
> scope.  More details in other comment response...
> 
>
https://codereview.chromium.org/670653005/diff/60001/content/browser/web_cont...
> content/browser/web_contents/web_contents_view_mac.mm:186: const
gfx::NativeView
> native_view = GetNativeViewForFocus();
> On 2014/10/24 23:04:05, Avi wrote:
> > Again, why const here and below? It's very unusual to use const everywhere
> when
> > dealing with ObjC objects.
> 
> Generally speaking, using const helps the compiler protect from unintended
bugs
> (e.g., logic that assumes a value does not change).  The Google C++ style
guide
> speaks to this:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const
> 
> Also, an interesting perspective on the topic:
> http://gamesfromwithin.com/the-const-nazi
> 
> Note that in this code change it's the pointers that are const, not the
objects
> being pointed at.  Because they're all in methods with with few LOC, I'm okay
> with removing them if they annoy you.  I'll admit I'm being very pedantic in
> this particular code change, but it's [IMHO] a good habit that I've formed.

I don't have a problem doing this in C++ code. But it's extremely unusual and
non-idiomatic for code touching ObjC objects. In addition, if you add "const" to
the wrong place and modify the ObjC object itself, things stop working and fail
to compile. It's a great idiom and habit to use for C++, but please don't bring
it near ObjC objects.

LGTM if you remove the extra consts.

[miu, 2014-10-25 01:39:20]

Patchset #6 (id:100001) has been deleted

[miu, 2014-10-25 01:39:47]

On 2014/10/25 00:58:18, Avi wrote:
> LGTM if you remove the extra consts.

Fair enough.  Done.

[miu, 2014-10-25 01:40:04]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-10-25 01:42:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/670653005/80001

[commit-bot: I haz the power, 2014-10-25 02:39:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-25 02:39:53]

Patchset 5 (id:??) landed as
https://crrev.com/9e14e49082f175ceccaa93a3329a8816b905ae34
Cr-Commit-Position: refs/heads/master@{#301269}