Create only a single LegacyRenderWidgetHostHWND per WebContentsViewAura.

Create only a single LegacyRenderWidgetHostHWND per WebContentsViewAura.

BUG=393665
Committed: https://crrev.com/4f8a4bca1a98b8ce3ed61aca14b4e1edc7a361a8
Cr-Commit-Position: refs/heads/master@{#301810}

[ananta, 2014-07-14 18:51:08]

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/legacy_render_widget_host_win.cc:118:
delegate_->OnLegacyHwndDestroyed(this);
Please validate whether all shutdown scenarios are addressed correctly. 

For e.g. Destroying the main window frame which some browser tests do. Windows
will
destroy the child implicitly. The other case is the delayed close which occurs
when 
closing the browser window.

NPAPI plugins like flash need to be tested as well. Specifically windowless
flash

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/legacy_render_widget_host_win.h:44: // A delegate
interface for LegacyRenderWidgetHostHWND to query its owner.
The comment here needs to be reworded?. query its owner is not the functionality
exposed by this interface?

[ananta, 2014-07-14 18:53:02]

On 2014/07/14 18:51:08, ananta wrote:
>
https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):
> 
>
https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/legacy_render_widget_host_win.cc:118:
> delegate_->OnLegacyHwndDestroyed(this);
> Please validate whether all shutdown scenarios are addressed correctly. 
> 
> For e.g. Destroying the main window frame which some browser tests do. Windows
> will
> destroy the child implicitly. The other case is the delayed close which occurs
> when 
> closing the browser window.
> 
> NPAPI plugins like flash need to be tested as well. Specifically windowless
> flash
> 
>
https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/legacy_render_widget_host_win.h (right):
> 
>
https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/legacy_render_widget_host_win.h:44: // A
delegate
> interface for LegacyRenderWidgetHostHWND to query its owner.
> The comment here needs to be reworded?. query its owner is not the
functionality
> exposed by this interface?

For NPAPI flash a good bug to test would be this one
https://code.google.com/p/chromium/issues/detail?id=301548

[dmazzoni, 2014-07-23 15:39:42]

I haven't forgotten about this change.

See this change first - https://codereview.chromium.org/415633002/ - it
simplifies things a bit first so that BrowserAccessibilityManagerWin doesn't
actually need direct access to the LegacyRenderWidgetHostHWND, it just requests
it from its delegate.

[dmazzoni, 2014-07-29 23:03:05]

Ready for another look.

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/legacy_render_widget_host_win.cc:118:
delegate_->OnLegacyHwndDestroyed(this);
On 2014/07/14 18:51:08, ananta wrote:
> Please validate whether all shutdown scenarios are addressed correctly. 
> 
> For e.g. Destroying the main window frame which some browser tests do. Windows
> will
> destroy the child implicitly. The other case is the delayed close which occurs
> when 
> closing the browser window.

OK, the "Simplify access to LegacyRenderWidgetHostHWND" change
(https://codereview.chromium.org/415633002/) made this much cleaner and simpler.
Nobody keeps a copy of the HWND around anymore, they just query
LegacyRenderWidgetHostHWND directly every time. And now
LegacyRenderWidgetHostHWND doesn't keep the BrowserAccessibilityManager, it just
queries the IAccessible* it should use from its delegate.

Lifetime issues should be totally automatic now. WebContentsViewAura owns
LegacyRenderWidgetHostHWND and will thus outlive any other object that requests
the LegacyRenderWidgetHostHWND.

> NPAPI plugins like flash need to be tested as well. Specifically windowless
> flash

I tested 301548 (Video controllers are not working on justin.TV with NPAPI
flash) and it worked fine.

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/legacy_render_widget_host_win.h:44: // A delegate
interface for LegacyRenderWidgetHostHWND to query its owner.
On 2014/07/14 18:51:08, ananta wrote:
> The comment here needs to be reworded?. query its owner is not the
functionality
> exposed by this interface?

Done.

[ananta, 2014-08-04 19:28:34]

Looks good. Please test this on Windows ASH as well.

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

https://codereview.chromium.org/387353004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1121: parent_hwnd =
::GetDesktopWindow();
When would context or GetHost be NULL?

[dmazzoni, 2014-09-10 23:01:31]

PTAL.

I didn't have a chance to look at this for a while. I tested it on Windows 8 /
ash and fixed a couple of issues, now it works fine in both modes.

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

https://codereview.chromium.org/387353004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1121: parent_hwnd =
::GetDesktopWindow();
On 2014/08/04 19:28:34, ananta wrote:
> When would context or GetHost be NULL?

I'm not sure. Note that context is NULL-checked earlier in this function.

[ananta, 2014-09-23 20:14:06]

Please test windowless NPAPI flash to see if continues to work correctly with
this change. Other cases to test would be dragging a tab/multiple tabs and
detaching into a separate window and verifying that the legacy window is created
for all
tabs. Detaching and reattaching to the same window would also
be a good test case.

On a related note, in the old non AURA code path the RenderWidgetHostViewWin
HWND would get created along with
the RenderWidgetHostViewWin  instance for every page load on the tab. Do you
recollect seeing similar problems with accessibility tools which necessiated
this change?. If yes then maybe we could live with the current behavior?

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
content/browser/renderer_host/legacy_render_widget_host_win.h:24: class
LegacyRenderWidgetHostHWND;
Is this needed?

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:574: // The
LegacyRenderWidgetHostHWND class provides a dummy HWND which is used
Can you also add this comment in the WCVA class where it declares the
LegacyRenderWidgetHostHWND member?

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:579: bool
legacy_window_destroyed_;
Please remove this member.

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

https://codereview.chromium.org/387353004/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:678:
view_->legacy_hwnd_->UpdateParent(::GetDesktopWindow());
I know that this is a copy of the existing code in
RenderWidgetHostViewAura::RemovingFromRootWindow. However it may be better to
reparent to the hidden window instead. This is to avoid bugs like unwanted
buttons showing up on the taskbar because the legacy_hwnd becomes a top level
window.

https://codereview.chromium.org/387353004/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1133: parent_hwnd =
::GetDesktopWindow();
It would be good to know when context is going to be NULL when this function is
called. Would be better to avoid creating the legacy hwnd if we don't have a
context.

[dmazzoni, 2014-09-24 22:46:28]

On 2014/09/23 20:14:06, ananta wrote:
> Please test windowless NPAPI flash to see if continues to work correctly with
> this change. Other cases to test would be dragging a tab/multiple tabs and
> detaching into a separate window and verifying that the legacy window is
created
> for all
> tabs. Detaching and reattaching to the same window would also
> be a good test case.

I tested all of these cases, I couldn't find any problems.

> On a related note, in the old non AURA code path the RenderWidgetHostViewWin
> HWND would get created along with
> the RenderWidgetHostViewWin  instance for every page load on the tab. Do you
> recollect seeing similar problems with accessibility tools which necessiated
> this change?. If yes then maybe we could live with the current behavior?

Creating a separate HWND for each navigation was exactly what was causing
problems. This new change gives us one consistent and stable HWND for as long as
the users stays in that tab.

I also think this simplifies the ownership and lifecycle issues.
WebContentsViewAura has a really simple lifecycle compared to RWHVA.

[dmazzoni, 2014-09-24 22:57:52]

Let's land this and then ask QA to pay special attention to the types of things
that caused issues in the past - windowless Flash, moving tabs between windows,
etc.

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
content/browser/renderer_host/legacy_render_widget_host_win.h:24: class
LegacyRenderWidgetHostHWND;
On 2014/09/23 20:14:05, ananta wrote:
> Is this needed?

Good catch, not needed anymore. Done.

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:574: // The
LegacyRenderWidgetHostHWND class provides a dummy HWND which is used
On 2014/09/23 20:14:05, ananta wrote:
> Can you also add this comment in the WCVA class where it declares the
> LegacyRenderWidgetHostHWND member?

Done.

https://codereview.chromium.org/387353004/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:579: bool
legacy_window_destroyed_;
On 2014/09/23 20:14:06, ananta wrote:
> Please remove this member.

Done.

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

https://codereview.chromium.org/387353004/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:678:
view_->legacy_hwnd_->UpdateParent(::GetDesktopWindow());
On 2014/09/23 20:14:06, ananta wrote:
> I know that this is a copy of the existing code in
> RenderWidgetHostViewAura::RemovingFromRootWindow. However it may be better to
> reparent to the hidden window instead. This is to avoid bugs like unwanted
> buttons showing up on the taskbar because the legacy_hwnd becomes a top level
> window.

Makes sense, done.

https://codereview.chromium.org/387353004/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1133: parent_hwnd =
::GetDesktopWindow();
On 2014/09/23 20:14:06, ananta wrote:
> It would be good to know when context is going to be NULL when this function
is
> called. Would be better to avoid creating the legacy hwnd if we don't have a
> context.

The comment above says that sometimes object hierarchies are build before
they're attached to a widget. I think it'd be better for accessibility if we had
created the HWND here. We already have code to reparent it in
OnWindowAddedToRootWindow.

[dmazzoni, 2014-10-08 06:18:13]

dmazzoni@chromium.org changed reviewers:
+ sky@chromium.org

[dmazzoni, 2014-10-08 06:18:14]

+sky for OWNERS review

[sky, 2014-10-08 14:43:00]

Ananta knows this code and should review it. He still hasn't approved.

[ananta, 2014-10-08 18:30:13]

https://codereview.chromium.org/387353004/diff/160001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/387353004/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1134: parent_hwnd =
::GetDesktopWindow();
I am still not convinced about creating the legacy window parented to the
desktop in the absence of a context. 

It would be better to not create the legacy window in this case.

[dmazzoni, 2014-10-10 21:09:50]

https://codereview.chromium.org/387353004/diff/160001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/387353004/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1134: parent_hwnd =
::GetDesktopWindow();
On 2014/10/08 18:30:13, ananta wrote:
> I am still not convinced about creating the legacy window parented to the
> desktop in the absence of a context. 
> 
> It would be better to not create the legacy window in this case.

Done.

[ananta, 2014-10-17 23:58:00]

Looking good. Only nits.

https://codereview.chromium.org/387353004/diff/180001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/180001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:52: // that
delegates its accessibility implementation to the
and delegates?

https://codereview.chromium.org/387353004/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/387353004/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1177:
view->SetLegacyRenderWidgetHostHWND(legacy_hwnd_.get());
if (legacy_hwnd_) here and below?

[dmazzoni, 2014-10-21 20:42:00]

https://codereview.chromium.org/387353004/diff/180001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/180001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:52: // that
delegates its accessibility implementation to the
On 2014/10/17 23:58:00, ananta wrote:
> and delegates?

Done.

https://codereview.chromium.org/387353004/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/387353004/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1177:
view->SetLegacyRenderWidgetHostHWND(legacy_hwnd_.get());
On 2014/10/17 23:58:00, ananta wrote:
> if (legacy_hwnd_) here and below?

Good idea. I guess it was harmless before because passing NULL had no effect,
but this is better.

[dmazzoni, 2014-10-24 23:08:52]

Ping?

[ananta, 2014-10-24 23:12:59]

lgtm

[sky, 2014-10-27 15:21:33]

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (left):

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:89: void
LegacyRenderWidgetHostHWND::OnFinalMessage(HWND hwnd) {
If you're going to allow this class to exist after the HWND has been destroyed
do you need a bunch of IsWindow(hwnd()) checks?

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:150:
DCHECK(delegate_);
Move to constructor.

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:43: class
CONTENT_EXPORT LegacyRenderWidgetHostHWNDDelegate {
nit: move into it's own file.

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:59: virtual
~LegacyRenderWidgetHostHWND();
Move implementation to match new position.

https://codereview.chromium.org/387353004/diff/320001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/387353004/diff/320001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1701: return NULL;
nullptr

[dmazzoni, 2014-10-27 20:32:32]

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (left):

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:89: void
LegacyRenderWidgetHostHWND::OnFinalMessage(HWND hwnd) {
On 2014/10/27 15:21:33, sky wrote:
> If you're going to allow this class to exist after the HWND has been destroyed
> do you need a bunch of IsWindow(hwnd()) checks?

Good idea, added.

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:150:
DCHECK(delegate_);
On 2014/10/27 15:21:33, sky wrote:
> Move to constructor.

Done.

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:43: class
CONTENT_EXPORT LegacyRenderWidgetHostHWNDDelegate {
On 2014/10/27 15:21:33, sky wrote:
> nit: move into it's own file.

Done.

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:59: virtual
~LegacyRenderWidgetHostHWND();
On 2014/10/27 15:21:33, sky wrote:
> Move implementation to match new position.

Done. I moved it immediately after Create in both files now, since statics are
supposed to come before the constructor and destructor.

https://codereview.chromium.org/387353004/diff/320001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/387353004/diff/320001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1701: return NULL;
On 2014/10/27 15:21:33, sky wrote:
> nullptr

Done.

[sky, 2014-10-27 22:04:54]

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/387353004/diff/320001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:59: virtual
~LegacyRenderWidgetHostHWND();
On 2014/10/27 20:32:32, dmazzoni wrote:
> On 2014/10/27 15:21:33, sky wrote:
> > Move implementation to match new position.
> 
> Done. I moved it immediately after Create in both files now, since statics are
> supposed to come before the constructor and destructor.

That's not entirely true. From the style guide for declaration ordering:
Typedefs and Enums
Constants (static const data members)
Constructors
Destructor
Methods, including static methods
Data Members (except static const data members)

Create should be after the destructor.

[dmazzoni, 2014-10-27 22:29:54]

On 2014/10/27 22:04:54, sky wrote:
> That's not entirely true. From the style guide for declaration ordering:
> Typedefs and Enums
> Constants (static const data members)
> Constructors
> Destructor
> Methods, including static methods
> Data Members (except static const data members)
> 
> Create should be after the destructor.

Fixed.

[sky, 2014-10-27 23:02:07]

LGTM

[dmazzoni, 2014-10-29 08:06:52]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-10-29 08:07:18]

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

[commit-bot: I haz the power, 2014-10-29 08:45:08]

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

[commit-bot: I haz the power, 2014-10-29 08:45:10]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/80107)

[dmazzoni, 2014-10-29 09:00:08]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-10-29 09:00:47]

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

[commit-bot: I haz the power, 2014-10-29 09:57:17]

Committed patchset #16 (id:420001)

[commit-bot: I haz the power, 2014-10-29 09:58:05]

Patchset 16 (id:??) landed as
https://crrev.com/4f8a4bca1a98b8ce3ed61aca14b4e1edc7a361a8
Cr-Commit-Position: refs/heads/master@{#301810}