Hide the Legacy HWND when the RWHVA is destroyed.

Hide the Legacy HWND when the RWHVA is destroyed.

BUG=437701
Committed: https://crrev.com/5a1a4d9160774ff982d4ecb0abb8d9c9e00b0ba1
Cr-Commit-Position: refs/heads/master@{#307167}

[dmazzoni, 2014-12-05 18:08:30]

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

[ananta, 2014-12-05 22:20:19]

Is it possible to control this from RWHVA?
Invoke on the Legacy HWND class to hide the window when the RWHVA is destroyed?

[dmazzoni, 2014-12-05 22:35:48]

On 2014/12/05 22:20:19, ananta wrote:
> Is it possible to control this from RWHVA?
> Invoke on the Legacy HWND class to hide the window when the RWHVA is
destroyed?

The problem I see with that approach is that there's sometimes more than one
RWHVA per WebContents, like during a page load. It'd have to check whether it's
the active one.

Also, since WCVA owns the Legacy HWND, I think it's better to keep all of the
logic about whether it's shown or hidden in one place. As a future refactoring I
was thinking of having RWHVA not even know about the Legacy HWND class and just
call delegate functions to get the HWND or IAccessible* from the WCV when
needed. That seems much cleaner and foolproof.

Another idea: WCVA could be a WebContentsObserver, then it could respond to
RenderProcessGone directly and we don't need to add a new interface to
WebContentsView.h - would you like that better?

[ananta, 2014-12-05 22:44:41]

On 2014/12/05 22:35:48, dmazzoni wrote:
> On 2014/12/05 22:20:19, ananta wrote:
> > Is it possible to control this from RWHVA?
> > Invoke on the Legacy HWND class to hide the window when the RWHVA is
> destroyed?
> 
> The problem I see with that approach is that there's sometimes more than one
> RWHVA per WebContents, like during a page load. It'd have to check whether
it's
> the active one.
> 
> Also, since WCVA owns the Legacy HWND, I think it's better to keep all of the
> logic about whether it's shown or hidden in one place. As a future refactoring
I
> was thinking of having RWHVA not even know about the Legacy HWND class and
just
> call delegate functions to get the HWND or IAccessible* from the WCV when
> needed. That seems much cleaner and foolproof.
> 
> Another idea: WCVA could be a WebContentsObserver, then it could respond to
> RenderProcessGone directly and we don't need to add a new interface to
> WebContentsView.h - would you like that better?

The multiple RWHVA thing. I am guessing you are talking about pages with
iframes, etc. I am not sure
if all of them have an associated legacy window.

[dmazzoni, 2014-12-05 22:48:30]

On Fri, Dec 5, 2014 at 2:44 PM, <ananta@chromium.org> wrote:

> The multiple RWHVA thing. I am guessing you are talking about pages with
> iframes, etc. I am not sure
> if all of them have an associated legacy window.


Out-of-process iframes will have a RWHVGuest, so that isn't an issue.

What I'm talking about is when you navigate from one page to another,
sometimes there's a period of time when the RWHVA from the old page and the
new page both exist.

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

[ananta, 2014-12-05 22:57:08]

On 2014/12/05 22:48:30, dmazzoni wrote:
> On Fri, Dec 5, 2014 at 2:44 PM, <mailto:ananta@chromium.org> wrote:
> 
> > The multiple RWHVA thing. I am guessing you are talking about pages with
> > iframes, etc. I am not sure
> > if all of them have an associated legacy window.
> 
> 
> Out-of-process iframes will have a RWHVGuest, so that isn't an issue.
> 
> What I'm talking about is when you navigate from one page to another,
> sometimes there's a period of time when the RWHVA from the old page and the
> new page both exist.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok. I like the WebContentsObserver approach which would ensure that this change
stays with WCVA

[dmazzoni, 2014-12-05 23:28:01]

On 2014/12/05 22:57:08, ananta wrote:
> Ok. I like the WebContentsObserver approach which would ensure that this
change
> stays with WCVA

Done, take a look.

I like it, this ended up being a smaller change.

[ananta, 2014-12-05 23:38:20]

lgtm

[dmazzoni, 2014-12-06 00:15:25]

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

[dmazzoni, 2014-12-06 00:15:25]

+avi for owners review

[Avi (use Gerrit), 2014-12-06 03:53:06]

lgtm

OK

[dmazzoni, 2014-12-06 07:23:38]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-12-06 07:24:20]

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

[commit-bot: I haz the power, 2014-12-06 08:27:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-06 08:28:18]

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