Ensure LegacyRenderWidgetHostHWND is always created.

Ensure LegacyRenderWidgetHostHWND is always created.

Previously, LegacyRenderWidgetHostHWND was only created inside
RenderWidgetHostViewAura::InternalSetBounds, and only if the parent
HWND was known. However, under some circumstances, InternalSetBounds
only gets called once and it gets called *before* the aura Window is
attached, so the legacy win never gets created.

Fix it by also trying to create the legacy win in
RWHVA::AddedToRootWindow().

BUG=613336
Committed: https://crrev.com/a7e78f6f5d453c52313b74de10c5f680aa3d5ee3
Cr-Commit-Position: refs/heads/master@{#395649}

[dmazzoni, 2016-05-19 22:07:39]

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

[ananta, 2016-05-20 23:54:09]

Is there a way to come up with tests for the cases you found?

https://codereview.chromium.org/1996163002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1996163002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:560: if
(legacy_render_widget_host_HWND_) {
Can we unify this with UpdateLegacyWin()?

[dmazzoni, 2016-05-23 22:45:49]

Test added.

https://codereview.chromium.org/1996163002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1996163002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:560: if
(legacy_render_widget_host_HWND_) {
On 2016/05/20 23:54:09, ananta wrote:
> Can we unify this with UpdateLegacyWin()?

Done.

[ananta, 2016-05-23 22:49:54]

https://codereview.chromium.org/1996163002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1996163002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2511: if
(legacy_render_widget_host_HWND_) {
We can remove this if check

[dmazzoni, 2016-05-23 22:57:23]

https://codereview.chromium.org/1996163002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1996163002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2511: if
(legacy_render_widget_host_HWND_) {
On 2016/05/23 22:49:54, ananta wrote:
> We can remove this if check 

I don't think that's safe, since Legacy...HWND::Create() can return null -
either due to a flag to disable it, or if it can't create the window.

[ananta, 2016-05-23 22:59:33]

lgtm

[dmazzoni, 2016-05-23 23:07:07]

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

[dmazzoni, 2016-05-23 23:07:08]

Sadrul, could you give me an owners review?
+sadrul
-sky

[sadrul, 2016-05-24 16:17:39]

A couple of suggestions. Other than that, lgtm

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2502:
window_->GetBoundsInRootWindow().IsEmpty()) {
Does it need to look at empty GetBoundsInRootWindow()? Can it just check if
GetRootWindow() is null instead?

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2505: 
Should this early return if legacy_render_widget_host_HWND_ already exists?
Looks like it will need to still UpdateParent() before the early-return, but it
avoid some additional work, e.g. SetBounds() etc.?

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:394: // Create
and/or update the legacy dummy window which corresponds to
*Creates ... *updates

[dmazzoni, 2016-05-24 16:53:07]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-05-24 16:53:08]

The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1996163002/#ps60001
(title: "Feedback from sadrul")

[dmazzoni, 2016-05-24 16:53:09]

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2502:
window_->GetBoundsInRootWindow().IsEmpty()) {
On 2016/05/24 16:17:39, sadrul wrote:
> Does it need to look at empty GetBoundsInRootWindow()? Can it just check if
> GetRootWindow() is null instead?

Should be fine, it just means there's a chance we'll create the
window with empty bounds and then update it later.

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2505: 
On 2016/05/24 16:17:39, sadrul wrote:
> Should this early return if legacy_render_widget_host_HWND_ already exists?
> Looks like it will need to still UpdateParent() before the early-return, but
it
> avoid some additional work, e.g. SetBounds() etc.?

The most frequent time we call UpdateLegacyWin will be when the
size changes, so we can't skip the SetBounds call then. The only other
times we call UpdateLegacyWin are on Show() and AddedToRootWindow()
which are quite infrequent, so I don't think we need to optimize those
cases.

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1996163002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:394: // Create
and/or update the legacy dummy window which corresponds to
On 2016/05/24 16:17:39, sadrul wrote:
> *Creates ... *updates

Done.

[commit-bot: I haz the power, 2016-05-24 16:53:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996163002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996163002/60001

[commit-bot: I haz the power, 2016-05-24 18:43:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-24 18:44:22]

Description was changed from

==========
Ensure LegacyRenderWidgetHostHWND is always created.

Previously, LegacyRenderWidgetHostHWND was only created inside
RenderWidgetHostViewAura::InternalSetBounds, and only if the parent
HWND was known. However, under some circumstances, InternalSetBounds
only gets called once and it gets called *before* the aura Window is
attached, so the legacy win never gets created.

Fix it by also trying to create the legacy win in
RWHVA::AddedToRootWindow().

BUG=613336
==========

to

==========
Ensure LegacyRenderWidgetHostHWND is always created.

Previously, LegacyRenderWidgetHostHWND was only created inside
RenderWidgetHostViewAura::InternalSetBounds, and only if the parent
HWND was known. However, under some circumstances, InternalSetBounds
only gets called once and it gets called *before* the aura Window is
attached, so the legacy win never gets created.

Fix it by also trying to create the legacy win in
RWHVA::AddedToRootWindow().

BUG=613336

Committed: https://crrev.com/a7e78f6f5d453c52313b74de10c5f680aa3d5ee3
Cr-Commit-Position: refs/heads/master@{#395649}
==========

[commit-bot: I haz the power, 2016-05-24 18:44:24]

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