Create a visible window with class name Chrome_RenderWidgetHostHWND which corresponds to the bounds…

Create a visible window with class name Chrome_RenderWidgetHostHWND which corresponds to the bounds of the web contents.

This is done to ensure that legacy drivers for trackpoints/trackpads and others which use track gestures for back forward
navigations work. These drivers call the WindowFromPoint API and get the process name. For Chrome they look for the old
Window class Chrome_RenderWidgetHostHWND. They use this to determine whether to send mouse wheels and special keyboard
message for back forward navigations, etc.

With Aura we don't have this window class anymore causing Chrome to not work well for a number of users.

To workaround this in the shortterm, a dummy HWND with the Chrome_RenderWidgetHostHWND class name is created. This window
is visible and handles mouse events. It is transparent to ensure that it does not need to draw. To ensure that we don't
mess wit mouse events too much we set capture to the parent window as needed. This window also serves as the parent to
windowless NPAPI plugins like Flash. Additionally the dummy accessibility window with this class name has been removed
and the functionality has been consolidated into the newly added LegacyRenderWidgetHostHWND class which provides this
functionality.

BUG=326022, 335941
R=cpu@chromium.org, scottmg@chromium.org, sky@chromium.org, cpu, scottmg
TBR=jam

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

[ananta, 2014-01-31 01:52:39]

+shrikant

[scottmg, 2014-01-31 17:28:47]

Scary! Is there any way we could do this on only when necessary? I guess a
setting/flag might not be appropriate if the user of accessibility software
can't get to the settings page.

(Also worry that after we add this there's no pressure on vendors to fix
existing software so we're stuck with this forever?)

https://codereview.chromium.org/151083002/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/151083002/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:7: #include
<atlbase.h>
Looks like you can remove all these ATL includes.

[ananta, 2014-01-31 18:25:29]

As per a discussion with cpu on Wed, we decided to put this CL up for review and
discuss in the ensuing thread :).

I agree that the change does look scary. However overall it is not so bad. The
only complexity is with the mouse events with the capture setting, etc. From
cursory testing it seems to work well. We can give it a shot in Canary and have
QA bang on it for a while before deciding to dump it.

I agree that having this change means that driver vendors have no incentive to
fix buggy software.

https://codereview.chromium.org/151083002/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/151083002/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:7: #include
<atlbase.h>
On 2014/01/31 17:28:48, scottmg wrote:
> Looks like you can remove all these ATL includes.

Done.

[scottmg, 2014-01-31 21:52:00]

I patched it in, and seems to be ok, so I guess... lgtm-ish. :)

This seems to handle mouse mostly. There's nothing extra we have to do for
keyboard media keys, etc.?

I guess we are stuck with it forever. Maybe that's not the end of the world.

https://codereview.chromium.org/151083002/diff/90001/content/browser/renderer...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/90001/content/browser/renderer...
content/browser/renderer_host/legacy_render_widget_host_win.cc:19:
Create(parent, rect, NULL, WS_CHILDWINDOW, WS_EX_TRANSPARENT);
could you change the title to "Legacy Helper" or something? (mostly for less
confusion when using spy++ :)

https://codereview.chromium.org/151083002/diff/90001/ui/views/win/hwnd_messag...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/151083002/diff/90001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:791: // DCHECK(!HasCapture());
when is not doing this necessary? wouldn't it either be on the legacy, or us?

[scottmg, 2014-01-31 22:15:24]

Oh, also touch events I guess. Do we need to do anything explicit to forward
those through the transparent window?

[dmazzoni at google, 2014-01-31 23:13:04]

Ananta, can you confirm that the transparent HWND doesn't mess up
accessible hit testing using AccessibleObjectFromPoint? Turn on web
accessibility using chrome://accessibility, then open Inspect.exe and try
to select an accessible object in a webpage using the tool that lets you
hover over an object to examine it.

We also need to QA this change with JAWS. I'm on leave so I'm not able to
patch it in and try it. After it lands, you can ask kathrelkeld@ or
jyotsnakaki@ to test Canary.

These tests should be suficient to make sure nothing regressed with JAWS:
* Test that the JAWS virtual buffer works (press up/down arrows to browse
static text in a webpage)
* Test that Ctrl+F opens the JAWS Find dialog and not the Chrome Find dialog

Thanks,
- Dominic

On Fri, Jan 31, 2014 at 2:15 PM, <scottmg@chromium.org> wrote:

> Oh, also touch events I guess. Do we need to do anything explicit to
> forward
> those through the transparent window?
>
> https://codereview.chromium.org/151083002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ananta, 2014-01-31 23:56:25]

Keyboard events go directly to the parent window, very likely because the child
is transparent. Need to test what happens with ASUS.

We post touch events directly to the parent. That works well.

https://codereview.chromium.org/151083002/diff/90001/content/browser/renderer...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/90001/content/browser/renderer...
content/browser/renderer_host/legacy_render_widget_host_win.cc:19:
Create(parent, rect, NULL, WS_CHILDWINDOW, WS_EX_TRANSPARENT);
On 2014/01/31 21:52:01, scottmg wrote:
> could you change the title to "Legacy Helper" or something? (mostly for less
> confusion when using spy++ :)

Changed to Chrome Legacy Helper

https://codereview.chromium.org/151083002/diff/90001/ui/views/win/hwnd_messag...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/151083002/diff/90001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:791: // DCHECK(!HasCapture());
On 2014/01/31 21:52:01, scottmg wrote:
> when is not doing this necessary? wouldn't it either be on the legacy, or us?

The dummy Chrome_RenderWidgetHostHWND class sets capture to the parent.
When the parent receives a lbuttondown, it sets capture again which causes this
DCHECK to fire.

[ananta, 2014-01-31 23:58:33]

On 2014/01/31 23:13:04, dmazzoni at google wrote:
> Ananta, can you confirm that the transparent HWND doesn't mess up
> accessible hit testing using AccessibleObjectFromPoint? Turn on web
> accessibility using chrome://accessibility, then open Inspect.exe and try
> to select an accessible object in a webpage using the tool that lets you
> hover over an object to examine it.
> 
> We also need to QA this change with JAWS. I'm on leave so I'm not able to
> patch it in and try it. After it lands, you can ask kathrelkeld@ or
> jyotsnakaki@ to test Canary.
> 
> These tests should be suficient to make sure nothing regressed with JAWS:
> * Test that the JAWS virtual buffer works (press up/down arrows to browse
> static text in a webpage)
> * Test that Ctrl+F opens the JAWS Find dialog and not the Chrome Find dialog
> 
> Thanks,
> - Dominic
> 
> On Fri, Jan 31, 2014 at 2:15 PM, <mailto:scottmg@chromium.org> wrote:
> 
> > Oh, also touch events I guess. Do we need to do anything explicit to
> > forward
> > those through the transparent window?
> >
> > https://codereview.chromium.org/151083002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hi Dominic

Thanks for taking a peek at this. I will run the tests you mentioned. On a
related note the accessibility unittests
pass. Regarding the concern with transparent windows, that style is ignored on
child windows for input events like mouse events. We receive mouse events. 

Thanks
Ananta

[ananta, 2014-02-03 21:17:33]

+sky

[ananta, 2014-02-04 01:58:29]

Update the patchset to not subclass the parent and capture mouse events. We now
send input messages via SendMessage to
the parent window.

[cpu_(ooo_6.6-7.5), 2014-02-04 22:35:58]

https://codereview.chromium.org/151083002/diff/1610001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/151083002/diff/1610001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_win.cc:39:
parent_hwnd_(accessible_hwnd->parent()),
I don't think parent_hwnd_ is used except in the previous code for creation so
we can remove it completely from this class.

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:19:
Create(parent, rect, L"Chrome Legacy Helper",
"Chrome Legacy Window" ?

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:79: // While this
works, this has the sideeffect of converting input messages into
side effect

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:91: return 0;
return ::SendMessage(parent_, message, w_param, l_param);

same in all the others?

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:9: #include
<atlapp.h>
do wee need atlapp? and atlcom ?

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:21: // Some screen
readers expect every tab / every unique web content container
Please update this comment to reflect the other reason / duties of this class.
Including the plugin issue.

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:44: //window class
to enable certain features. It'd be great to remove this.
remove comment 43/44 since we'll have a better explanation on the top.

[ananta, 2014-02-04 23:30:45]

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:19:
Create(parent, rect, L"Chrome Legacy Helper",
On 2014/02/04 22:35:58, cpu wrote:
> "Chrome Legacy Window" ?

Done.

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:79: // While this
works, this has the sideeffect of converting input messages into
On 2014/02/04 22:35:58, cpu wrote:
> side effect

Done.

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:91: return 0;
On 2014/02/04 22:35:58, cpu wrote:
> return ::SendMessage(parent_, message, w_param, l_param);
> 
> same in all the others?

Done.

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:9: #include
<atlapp.h>
On 2014/02/04 22:35:58, cpu wrote:
> do wee need atlapp? and atlcom ?

Changed to only include atlwin.h

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:21: // Some screen
readers expect every tab / every unique web content container
On 2014/02/04 22:35:58, cpu wrote:
> Please update this comment to reflect the other reason / duties of this class.
> Including the plugin issue.

Done.

https://codereview.chromium.org/151083002/diff/1610001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:44: //window class
to enable certain features. It'd be great to remove this.
On 2014/02/04 22:35:58, cpu wrote:
> remove comment 43/44 since we'll have a better explanation on the top.

Done.

[ananta, 2014-02-04 23:31:57]

https://codereview.chromium.org/151083002/diff/1610001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/151083002/diff/1610001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_win.cc:39:
parent_hwnd_(accessible_hwnd->parent()),
On 2014/02/04 22:35:58, cpu wrote:
> I don't think parent_hwnd_ is used except in the previous code for creation so
> we can remove it completely from this class.

Leaving this as is for now. This is used by accessibility unittests. Will look
into this in a subsequent CL.

[cpu_(ooo_6.6-7.5), 2014-02-05 03:07:58]

lgtm

I am both scared and excited to land this.

[sky, 2014-02-05 15:22:43]

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

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:21: 
CheckWindowCreated(hwnd())

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:26: HRESULT hr =
::CreateStdAccessibleObject(
Did you intend to check the result? If not, no need to assign it.

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:30: if
(::IsWindow(hwnd()))
I don't think this if should be necessary.

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:62: if
(::IsWindow(hwnd()))
Can this be moved to the destructor instead? That way this method isn't needed.

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

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:34: //   
TabContents, etc. This causes the plugin mouse event calculations to
nit: TabContents->WebContents.

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

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:812:
content::LegacyRenderWidgetHostHWND* legacy_render_widget_host_HWND_;
What are the advantages of wiring LegacyRenderWidgetHostHWND at RWHVA instead of
WebContentsViewAura?

[ananta, 2014-02-05 16:35:50]

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

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:21: 
On 2014/02/05 15:22:44, sky wrote:
> CheckWindowCreated(hwnd())

Cannot use that as this code would also run for Windows 8 ASH where child window
creation would fail.
Added some comments to make this clear. The class would be a dummy for ASH.

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:26: HRESULT hr =
::CreateStdAccessibleObject(
On 2014/02/05 15:22:44, sky wrote:
> Did you intend to check the result? If not, no need to assign it.

Added a DCHECK for hr.

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:30: if
(::IsWindow(hwnd()))
On 2014/02/05 15:22:44, sky wrote:
> I don't think this if should be necessary.

Done.

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:62: if
(::IsWindow(hwnd()))
On 2014/02/05 15:22:44, sky wrote:
> Can this be moved to the destructor instead? That way this method isn't
needed.

Done.

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

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.h:34: //   
TabContents, etc. This causes the plugin mouse event calculations to
On 2014/02/05 15:22:44, sky wrote:
> nit: TabContents->WebContents.

Done.

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

https://codereview.chromium.org/151083002/diff/890002/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:812:
content::LegacyRenderWidgetHostHWND* legacy_render_widget_host_HWND_;
On 2014/02/05 15:22:44, sky wrote:
> What are the advantages of wiring LegacyRenderWidgetHostHWND at RWHVA instead
of
> WebContentsViewAura?

RWHVA has plumbing code to track when its aura::Window is added to the
RootWindow, which is used to reparent the container HWND as needed. This is
needed for plugins to work correctly.

[sky, 2014-02-05 18:16:47]

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:21: // The window
creation could fail if we are running on Windows 8 ASH, which
Alternative suggestion. Make the constructor private, add a factory (static)
method for creation. Factory method returns NULL if creation fails. That way
don't have to worry about hwnd() being invalid everywhere.

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3364: if
(legacy_render_widget_host_HWND_)
nit: no need for if.

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3529: if
(legacy_render_widget_host_HWND_ && GetNativeViewId()) {
Can you instead destroy in RemoveFromRootWindow and recreate here?

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:811: // This
instance is deleted when the parent window is destroyed.
Comment is wrong here, this class deletes it. In fact can this be a scoped_ptr?

[ananta, 2014-02-05 19:52:35]

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:21: // The window
creation could fail if we are running on Windows 8 ASH, which
On 2014/02/05 18:16:47, sky wrote:
> Alternative suggestion. Make the constructor private, add a factory (static)
> method for creation. Factory method returns NULL if creation fails. That way
> don't have to worry about hwnd() being invalid everywhere.

Good point. Done.

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3364: if
(legacy_render_widget_host_HWND_)
On 2014/02/05 18:16:47, sky wrote:
> nit: no need for if.
Code has been changed to call legacy_render_widget_host_HWND_.reset(NULL) as it
is a scoped_ptr

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3529: if
(legacy_render_widget_host_HWND_ && GetNativeViewId()) {
On 2014/02/05 18:16:47, sky wrote:
> Can you instead destroy in RemoveFromRootWindow and recreate here?
I don't think destroying and creating would work given that the same code
executes when a tab is reparented to a new window etc. Plugins request the HWND
once and hold on to it.

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:811: // This
instance is deleted when the parent window is destroyed.
On 2014/02/05 18:16:47, sky wrote:
> Comment is wrong here, this class deletes it. In fact can this be a
scoped_ptr?

Done.

[sky, 2014-02-05 21:56:15]

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3529: if
(legacy_render_widget_host_HWND_ && GetNativeViewId()) {
On 2014/02/05 19:52:36, ananta wrote:
> On 2014/02/05 18:16:47, sky wrote:
> > Can you instead destroy in RemoveFromRootWindow and recreate here?
> I don't think destroying and creating would work given that the same code
> executes when a tab is reparented to a new window etc. Plugins request the
HWND
> once and hold on to it.
> 

My worry is that I don't know if you are guaranteed a Remove is immediately
followed by an add. If it's not immediate than you've left the
legacy_render_widget_host_HWND_ in the wrong window, and it may end up being
destroyed by virtue of being a child. I think you need to change the parent in
both remove and add.

[sky, 2014-02-05 22:09:12]

LGTM with a comment

[ananta, 2014-02-05 22:20:25]

On 2014/02/05 21:56:15, sky wrote:
>
https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_aura.cc:3529: if
> (legacy_render_widget_host_HWND_ && GetNativeViewId()) {
> On 2014/02/05 19:52:36, ananta wrote:
> > On 2014/02/05 18:16:47, sky wrote:
> > > Can you instead destroy in RemoveFromRootWindow and recreate here?
> > I don't think destroying and creating would work given that the same code
> > executes when a tab is reparented to a new window etc. Plugins request the
> HWND
> > once and hold on to it.
> > 
> 
> My worry is that I don't know if you are guaranteed a Remove is immediately
> followed by an add. If it's not immediate than you've left the
> legacy_render_widget_host_HWND_ in the wrong window, and it may end up being
> destroyed by virtue of being a child. I think you need to change the parent in
> both remove and add.

The SetParent call is done by the RenderWidgetHostImpl::ParentChanged function.
We need to update the parent_ member
in the legacy class in both the Add and Remove from root functions. Changed
SetParentWindow to an inline set_parent_window
function.

[ananta, 2014-02-05 22:52:20]

On 2014/02/05 22:20:25, ananta wrote:
> On 2014/02/05 21:56:15, sky wrote:
> >
>
https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/151083002/diff/2400001/content/browser/render...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:3529: if
> > (legacy_render_widget_host_HWND_ && GetNativeViewId()) {
> > On 2014/02/05 19:52:36, ananta wrote:
> > > On 2014/02/05 18:16:47, sky wrote:
> > > > Can you instead destroy in RemoveFromRootWindow and recreate here?
> > > I don't think destroying and creating would work given that the same code
> > > executes when a tab is reparented to a new window etc. Plugins request the
> > HWND
> > > once and hold on to it.
> > > 
> > 
> > My worry is that I don't know if you are guaranteed a Remove is immediately
> > followed by an add. If it's not immediate than you've left the
> > legacy_render_widget_host_HWND_ in the wrong window, and it may end up being
> > destroyed by virtue of being a child. I think you need to change the parent
in
> > both remove and add.
> 
> The SetParent call is done by the RenderWidgetHostImpl::ParentChanged
function.
> We need to update the parent_ member
> in the legacy class in both the Add and Remove from root functions. Changed
> SetParentWindow to an inline set_parent_window
> function.

Removed the ParentChanged function from RenderWidgetHost and the calls from
RWHVA. Added
two functions UpdateParent and GetParent in the LegacyRenderWidgetHostHWND class
to set
and get the parent. Also removed the parent_ member and used GetParent wherever
needed.

[sky, 2014-02-05 23:56:40]

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:58: static
LegacyRenderWidgetHostHWND* Create(HWND parent);
nit: make this return a scoped_ptr.

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3565:
legacy_render_widget_host_HWND_->UpdateParent(::GetDesktopWindow());
When you do this should you disable the window or anything? Otherwise might it
get events and other stuff?

[ananta, 2014-02-06 00:42:03]

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:58: static
LegacyRenderWidgetHostHWND* Create(HWND parent);
On 2014/02/05 23:56:41, sky wrote:
> nit: make this return a scoped_ptr.

Done.

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/151083002/diff/2630003/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3565:
legacy_render_widget_host_HWND_->UpdateParent(::GetDesktopWindow());
On 2014/02/05 23:56:41, sky wrote:
> When you do this should you disable the window or anything? Otherwise might it
> get events and other stuff?

It should not because of WS_EX_TRANSPARENT. Added code to disable the window and
enable it anyways to prevent any surprises.

[sky, 2014-02-06 00:58:40]

LGTM

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:17:
scoped_ptr<LegacyRenderWidgetHostHWND> LegacyRenderWidgetHostHWND::Create(HWND
parent) {
nit: > 80

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:61:
~LegacyRenderWidgetHostHWND();
nit: destructor before Create.

[ananta, 2014-02-06 01:15:57]

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.cc:17:
scoped_ptr<LegacyRenderWidgetHostHWND> LegacyRenderWidgetHostHWND::Create(HWND
parent) {
On 2014/02/06 00:58:41, sky wrote:
> nit: > 80
Done

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
File content/browser/renderer_host/legacy_render_widget_host_win.h (right):

https://codereview.chromium.org/151083002/diff/3140001/content/browser/render...
content/browser/renderer_host/legacy_render_widget_host_win.h:61:
~LegacyRenderWidgetHostHWND();
On 2014/02/06 00:58:41, sky wrote:
> nit: destructor before Create.

Done.

[ananta, 2014-02-06 20:13:44]

Added a switch disable-legacy-window to enable QA and others to verify if the
addition of the dummy Chrome_RenderWidgetHostHWND window is causing regresions.
The disable-legacy-window switch disables the creation of this window.

Additionally I added code to reparent the legacy window to the hidden global
window on the same lines as plugin windows when we switch tabs. We reparent the
legacy window back to the browser window when the user switches back to the
original tab. We do this before other plugin windows to ensure that Zorder is
preserved.

[cpu_(ooo_6.6-7.5), 2014-02-06 21:23:04]

https://codereview.chromium.org/151083002/diff/3570003/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/151083002/diff/3570003/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:155:
::InvalidateRect(window, NULL, FALSE);
do we need this? aren't we invalidated anyhow?

[ananta, 2014-02-06 21:26:10]

https://codereview.chromium.org/151083002/diff/3570003/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/151083002/diff/3570003/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:155:
::InvalidateRect(window, NULL, FALSE);
On 2014/02/06 21:23:05, cpu wrote:
> do we need this? aren't we invalidated anyhow?
Removed. Added for debugging flash painting issues.

[cpu_(ooo_6.6-7.5), 2014-02-06 21:26:54]

lgtm

[ananta, 2014-02-06 23:56:24]

TBR'ing jam for content OWNERS stamp

[ananta, 2014-02-07 00:00:33]

Committed patchset #28 manually as r249537.

[tietjen.anthony, 2014-03-04 17:54:36]

It appears that the Chrome_RenderWidgetHostHWND (which "Visible" but
"Transparent") is in Chrome 34 Beta and 35 Canary for Windows. When using a tool
to look at the window hierarchy, there is only one of those listed as children
under the main Chrome instance. When switching tabs, it switches out the handle
of that Chrome_RenderWidgetHostHWND but still only shows one instance of it
under the main Chrome instance.

However, using a window hierarchy inspection tool on Chrome 33 Stable it reveals
multiple [Static] handles (which are not "Visible") as children of the main
Chrome instance. It seems there is one for each tab.

If I understand correctly, you made the change to use
Chrome_RenderWidgetHostHWND instead of the [Static] objects because the [Static]
objects didn't work for vendors' existing code which looks for the
Chrome_RenderWidgetHostHWND instead.

I just have a few questions:
1. Is this something that broke for vendors with the release of the Aura
versions of Chrome?
2. Did older versions of Chrome (that didn't use Aura) use
Chrome_RenderWidgetHostHWND, thus it used to work for vendors?
3. Most importantly: The description states that this is a short-term
workaround. Also, one comment indicates that it would go to QA before deciding
to drop it. Will you continue to use Chrome_RenderWidgetHostHWND in the future
releases, or is there another potential solution in the works?

Thanks in advance!

Anthony