When we switch tabs in chrome, the tab being switched away from gets hidden/shown/hidden.

When we switch tabs in chrome, the tab being switched away from gets hidden/shown/hidden.

This occurs in the NativeViewHostAura::NativeViewDetaching code path where we first remove the
clipping window which is the intermediate parent of the web contents view. The clipping window
is hidden which causes the RWHVA::Hide function to get called which initiates the hiding sequence.
Then the web contents view is reparented to the main view which is still visible. Now the RWHVA::Show
function is called which initiates the show sequence. Eventually the main view is hidden, which then
initiates the hide sequence.

Addressed this with the following changes.
1. WebView::AttachWebContents and WebView::DetachWebContents
   now show and hide the webcontents native view. The
   WebContents is shown and hidden as before in
   WebContentsNativeViewAura::OnWindowVisibilityChanged.

2. Removed the WebContentsNativeViewAura::OnWindowParentChanged function.
   This function was present to show and hide the webcontents if the window was visible.
   This should not be needed with the change in #1 above.

3. Added a new file webview_unittest.cc. This contains the unittest WebViewUnitTest.TestWebViewAttachDetachWebContents
   This is run as part of unit_tests.exe.

BUG=412989
R=sky

Committed: https://crrev.com/99941773a742f62892fc9aad1a1ebfb7cc967164
Cr-Commit-Position: refs/heads/master@{#294962}

[sky, 2014-09-10 22:35:39]

How about some test coverage?

[ananta, 2014-09-11 02:27:47]

On 2014/09/10 22:35:39, sky wrote:
> How about some test coverage?

Added a views unittest DetachHidesNativeViewBeforeReparenting

PTAL

[sky, 2014-09-11 14:55:05]

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura_unittest.cc (right):

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura_unittest.cc:89: virtual void
OnWindowParentChanged(aura::Window* window,
Can't you at events_, since presumably OnWindowVisibilityChanged is being
called, right?

[ananta, 2014-09-11 15:01:56]

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura_unittest.cc (right):

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura_unittest.cc:89: virtual void
OnWindowParentChanged(aura::Window* window,
On 2014/09/11 14:55:05, sky wrote:
> Can't you at events_, since presumably OnWindowVisibilityChanged is being
> called, right?

OnWindowVisibilityChanged is only called in Chrome. The WCVA class handles
parent changed and makes the child visible again.

[sky, 2014-09-11 15:29:20]

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura_unittest.cc (right):

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura_unittest.cc:89: virtual void
OnWindowParentChanged(aura::Window* window,
On 2014/09/11 15:01:56, ananta wrote:
> On 2014/09/11 14:55:05, sky wrote:
> > Can't you at events_, since presumably OnWindowVisibilityChanged is being
> > called, right?
> 
> OnWindowVisibilityChanged is only called in Chrome. The WCVA class handles
> parent changed and makes the child visible again.
> 

OnWindowVisibilityChanged is from aura::WindowObserver. It should be invoked any
time the visibility changes, right?

[ananta, 2014-09-11 15:48:49]

On 2014/09/11 15:29:20, sky wrote:
>
https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
> File ui/views/controls/native/native_view_host_aura_unittest.cc (right):
> 
>
https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
> ui/views/controls/native/native_view_host_aura_unittest.cc:89: virtual void
> OnWindowParentChanged(aura::Window* window,
> On 2014/09/11 15:01:56, ananta wrote:
> > On 2014/09/11 14:55:05, sky wrote:
> > > Can't you at events_, since presumably OnWindowVisibilityChanged is being
> > > called, right?
> > 
> > OnWindowVisibilityChanged is only called in Chrome. The WCVA class handles
> > parent changed and makes the child visible again.
> > 
> 
> OnWindowVisibilityChanged is from aura::WindowObserver. It should be invoked
any
> time the visibility changes, right?

Yes. Rephrasing. WCVA has code to listen for parent change and make the window
visible. In the test that does not happen. We just check that after detach the
parent is hidden

[sky, 2014-09-11 16:03:19]

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:101:
host_->native_view()->Hide();
Why do we need to hide at all here?

[ananta, 2014-09-11 16:21:07]

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:101:
host_->native_view()->Hide();
On 2014/09/11 16:03:19, sky wrote:
> Why do we need to hide at all here?

If we don't hide it the WCVA code which listens for parent changed, makes the
window visible if the parent is visible.

[sky, 2014-09-11 16:43:36]

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:101:
host_->native_view()->Hide();
On 2014/09/11 16:21:07, ananta wrote:
> On 2014/09/11 16:03:19, sky wrote:
> > Why do we need to hide at all here?
> 
> If we don't hide it the WCVA code which listens for parent changed, makes the
> window visible if the parent is visible.
> 

Here's what the code for windows did eons ago:

void NativeViewHostWin::NativeViewDetaching(bool destroyed) {
  if (!destroyed) {
    if (installed_clip_)
      UninstallClip();
    Widget::ReparentNativeView(host_->native_view(), ui::GetHiddenWindow());
  }
  installed_clip_ = false;
}

It didn't muck with visibility at all.

[ananta, 2014-09-11 18:42:34]

On 2014/09/11 16:43:36, sky wrote:
>
https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
> File ui/views/controls/native/native_view_host_aura.cc (right):
> 
>
https://codereview.chromium.org/564553002/diff/40001/ui/views/controls/native...
> ui/views/controls/native/native_view_host_aura.cc:101:
> host_->native_view()->Hide();
> On 2014/09/11 16:21:07, ananta wrote:
> > On 2014/09/11 16:03:19, sky wrote:
> > > Why do we need to hide at all here?
> > 
> > If we don't hide it the WCVA code which listens for parent changed, makes
the
> > window visible if the parent is visible.
> > 
> 
> Here's what the code for windows did eons ago:
> 
> void NativeViewHostWin::NativeViewDetaching(bool destroyed) {
>   if (!destroyed) {
>     if (installed_clip_)
>       UninstallClip();
>     Widget::ReparentNativeView(host_->native_view(), ui::GetHiddenWindow());
>   }
>   installed_clip_ = false;
> }
> 
> It didn't muck with visibility at all.

I moved the RemoveClippingWindow call to the end of the
NativeViewHostAura::NativeViewDetaching function.
The test still works.

[sky, 2014-09-11 19:43:41]

LGTM

[ananta, 2014-09-11 21:40:38]

The CQ bit was checked by ananta@chromium.org

[ananta, 2014-09-11 22:08:35]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2014-09-11 23:12:44]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-11 23:22:39]

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

[commit-bot: I haz the power, 2014-09-12 01:28:18]

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

[commit-bot: I haz the power, 2014-09-12 01:28:19]

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

[ananta, 2014-09-12 02:22:24]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-12 02:23:35]

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

[commit-bot: I haz the power, 2014-09-12 03:43:45]

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

[commit-bot: I haz the power, 2014-09-12 03:43:45]

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

[ananta, 2014-09-12 06:07:34]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-12 06:07:55]

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

[commit-bot: I haz the power, 2014-09-12 07:29:48]

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

[commit-bot: I haz the power, 2014-09-12 07:29:49]

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

[ananta, 2014-09-12 12:46:28]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-12 12:46:47]

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

[commit-bot: I haz the power, 2014-09-12 14:07:28]

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

[commit-bot: I haz the power, 2014-09-12 14:07:29]

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

[sky, 2014-09-12 21:27:06]

https://codereview.chromium.org/564553002/diff/100001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/564553002/diff/100001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:1151: if (root_window) {
This makes me nervous. See big comment above this. Under what conditions are we
now getting here with no parent? Who was the window previously parented to?

[ananta, 2014-09-14 01:30:44]

https://codereview.chromium.org/564553002/diff/100001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/564553002/diff/100001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:1151: if (root_window) {
On 2014/09/12 21:27:05, sky wrote:
> This makes me nervous. See big comment above this. Under what conditions are
we
> now getting here with no parent? Who was the window previously parented to?

As per discussion, added code to show hide the WebContents view in
AttachWebContents/DetachWebContents. The webcontents instance is show/hidden by
the window visible observer in WebContentsViewAura as before. 

Added a new file webview unittest.cc which run as part of unit_tests.exe. This
file contains one test which verifies attach/detach webcontents and visibility.

[sky, 2014-09-15 15:38:35]

Did you try running WebContentsViewAuraTest.HideContentOnParenHide and make sure
it doesn't break? The interesting case is to make sure the renderer is told the
page is hidden when minimizing a window on chromeos. See 310365 for specifics.

[ananta, 2014-09-15 18:40:03]

On 2014/09/15 15:38:35, sky wrote:
> Did you try running WebContentsViewAuraTest.HideContentOnParenHide and make
sure
> it doesn't break? The interesting case is to make sure the renderer is told
the
> page is hidden when minimizing a window on chromeos. See 310365 for specifics.

It ran correctly on the trybots. I tried the use case in the bug, i.e.
minimizing the window with the url in the bug.
It correctly hides the webcontents via the visibility changed notification. I
also ran the test you mentioned locally.
It passes.

[sky, 2014-09-15 20:40:39]

https://codereview.chromium.org/564553002/diff/240001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/564553002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:307: if (!is_embedding_fullscreen_widget_)
Why is this code any different when there is a fullscreen widget or not? By that
I mean shouldn't you always to Show() here (and hide below)?

[sky, 2014-09-15 20:40:51]

Also, if no show/hide is the right thing, document why.

[ananta, 2014-09-15 21:21:09]

https://codereview.chromium.org/564553002/diff/240001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/564553002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:307: if (!is_embedding_fullscreen_widget_)
On 2014/09/15 20:40:39, sky wrote:
> Why is this code any different when there is a fullscreen widget or not? By
that
> I mean shouldn't you always to Show() here (and hide below)?

Updated the comment to clarify why we should not be calling Show and Hide here.

[sky, 2014-09-15 22:26:07]

LGTM

[ananta, 2014-09-15 22:52:56]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-15 22:55:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/564553002/260001

[commit-bot: I haz the power, 2014-09-16 01:31:23]

Committed patchset #14 (id:260001) as a8cc46587b78359f024b3a1e2efbad95afbcb63d

[commit-bot: I haz the power, 2014-09-16 01:33:03]

Patchset 14 (id:??) landed as
https://crrev.com/99941773a742f62892fc9aad1a1ebfb7cc967164
Cr-Commit-Position: refs/heads/master@{#294962}

[nhiroki, 2014-09-16 06:23:46]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/577533003/ by nhiroki@chromium.org.

The reason for reverting is: This seems to cause memory leaks in
WebViewUnitTest.TestWebViewAttachDetachWebContents on Linux(ASan/LSan) and
ChromiumOS(ASan/LSan)

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....