Make the RenderViewHostImpl update its visibility after a swap.

Make the RenderViewHostImpl update its visibility after a swap.

BUG=286259

[zturner, 2013-09-16 22:55:01]

I'm including creis@ on the reviewers list even though he's not an owner,
because sky@ suggested he might have some familiarity with this codepath.

Although the solution employed here is to insert an additional call to
render_view_host_->WasShown(), myself and jamesr@ both wonder if it might be
more appropriate to have the view update its host's visibility (or vice versa).

[Charlie Reis, 2013-09-16 23:28:44]

On 2013/09/16 22:55:01, zturner wrote:
> I'm including creis@ on the reviewers list even though he's not an owner,
> because sky@ suggested he might have some familiarity with this codepath.

Side note: I am an OWNER for this file.  See content/OWNERS.

> Although the solution employed here is to insert an additional call to
> render_view_host_->WasShown(), myself and jamesr@ both wonder if it might be
> more appropriate to have the view update its host's visibility (or vice
versa).

Calling both WasShown and Show seems really confusing to me, but then again, I
don't understand any of the view/show/hide logic very well.  I would prefer to
have someone more familiar with the view logic look at it.  I would have said
jamesr, but maybe darin/piman/rjkroege?

[jamesr, 2013-09-16 23:34:11]

I put what I think is happening in the bug:

https://code.google.com/p/chromium/issues/detail?id=286259#c9

creis@ - can you take a look at that and see what you think?  Does the scenario
I describe make sense and, if so, who should be notifying who about state
changes?  I think the bug here is confusion of responsibility between
RenderViewHostManager, RenderWidgetHostImpl and RenderWidgetHostView* but am not
entirely sure which one's wrong

[zturner, 2013-09-16 23:37:43]

On 2013/09/16 23:28:44, creis wrote:
> On 2013/09/16 22:55:01, zturner wrote:
> > I'm including creis@ on the reviewers list even though he's not an owner,
> > because sky@ suggested he might have some familiarity with this codepath.
> 
> Side note: I am an OWNER for this file.  See content/OWNERS.
> 
> > Although the solution employed here is to insert an additional call to
> > render_view_host_->WasShown(), myself and jamesr@ both wonder if it might be
> > more appropriate to have the view update its host's visibility (or vice
> versa).
> 
> Calling both WasShown and Show seems really confusing to me, but then again, I
> don't understand any of the view/show/hide logic very well.  I would prefer to
> have someone more familiar with the view logic look at it.  I would have said
> jamesr, but maybe darin/piman/rjkroege?

Yea jamesr and I diagnosed this together, and it's confusing to both of us as
well, but we're not sure if there's a better alternative.  There's a similar
pattern a few lines down in the file that calls host->GetView()->Hide() and
host->WasSwappedOut() on the swapped out view.  

render_view_host_->WasShown() doesn't seem to imply
render_view_host_->GetView()->Show() to, but whether or not the reverse should
be true is up for debate, I suppose.

I'll Cc the other people you suggested in case anyone has any input.

[zturner, 2013-09-16 23:39:13]

+darin, piman, rjkroege in case anyone has some input here.

[rjkroege, 2013-09-17 19:58:18]

On 2013/09/16 23:39:13, zturner wrote:
> +darin, piman, rjkroege in case anyone has some input here.

This looks a odd/scary to me (I'd expect it to add an additional repaint of the
view?) but someone more familiar with the view logic needs to have a look.

[sadrul, 2013-09-17 20:07:39]

On 2013/09/17 19:58:18, rjkroege wrote:
> On 2013/09/16 23:39:13, zturner wrote:
> > +darin, piman, rjkroege in case anyone has some input here.
> 
> This looks a odd/scary to me (I'd expect it to add an additional repaint of
the
> view?) but someone more familiar with the view logic needs to have a look.

Drive-by: one of the inconsistencies I notice with the aura and win/mac
implementation is that RWHV::Show() implementation calls into RWHV::WasShown().
So perhaps the aura/gtk implementations need to be fixed?

[Fady Samuel, 2013-09-17 20:14:43]

https://codereview.chromium.org/23866013/diff/5001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (left):

https://codereview.chromium.org/23866013/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:752:
render_view_host_->GetView()->Show();
Drive-by thought: It seems that on most platforms (other than Aura), calling
Show calls WasShown(). Maybe the fix here is to have
RenderWidgetHostViewAura::Show call RenderWidgetHostViewAura::WasShown as is the
pattern on other platforms?

[zturner, 2013-09-18 17:18:25]

Removed the change from RenderViewHostManager::CommitPending() since it could
trigger a second paint, and made the individual RVHI's show their associated
views.

This brings all the RVHI's into consistency with one another.

[jamesr, 2013-09-18 18:06:53]

https://codereview.chromium.org/23866013/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/23866013/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:359:
WasShown();
render_widget_host_view_win.cc has this comment:

  // |render_widget_host_| may be NULL if the WebContentsImpl is in the process
  // of closing.

and a null-check before it dereferences render_widget_host_. 
RWHVAndroid::WasShown() doesn't do a null check.  one of these is wrong, do you
know which?  RWHVMac doesn't do a null check

https://codereview.chromium.org/23866013/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:362: void
RenderWidgetHostViewAndroid::Hide() {
RWHVWin and RWHVMac calls WasHidden() too, you probably should

[zturner, 2013-09-18 20:08:48]

On 2013/09/18 18:06:53, jamesr wrote:
>
https://codereview.chromium.org/23866013/diff/14001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/23866013/diff/14001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_android.cc:359:
> WasShown();
> render_widget_host_view_win.cc has this comment:
> 
>   // |render_widget_host_| may be NULL if the WebContentsImpl is in the
process
>   // of closing.
> 
> and a null-check before it dereferences render_widget_host_. 
> RWHVAndroid::WasShown() doesn't do a null check.  one of these is wrong, do
you
> know which?  RWHVMac doesn't do a null check
> 
>
https://codereview.chromium.org/23866013/diff/14001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_android.cc:362: void
> RenderWidgetHostViewAndroid::Hide() {
> RWHVWin and RWHVMac calls WasHidden() too, you probably should

is calling WasHidden() from Hide the right thing to do?  I agree that making the
view visible should make the view host visible, but the converse (hiding the
view causing the view host to be hidden) doesn't seem correct.

Thoughts?

[jamesr, 2013-09-18 20:56:05]

I'm not entirely sure what is more correct in the overall design (and I doubt
that anyone does), but I think it's more important to get consistency first so
that if we do have bugs we at least have the same bugs on every platform and can
fix them all.  To me, showing and hiding should parallel each other as I'm not
sure when one would not imply the other.

[zturner, 2013-09-18 21:46:55]

On 2013/09/18 20:56:05, jamesr wrote:
> I'm not entirely sure what is more correct in the overall design (and I doubt
> that anyone does), but I think it's more important to get consistency first so
> that if we do have bugs we at least have the same bugs on every platform and
can
> fix them all.  To me, showing and hiding should parallel each other as I'm not
> sure when one would not imply the other.

Fair enough.  It's unfortunate that every current and future implementation of
RWHW has to remember to do this, perhaps at some point we can figure out if
there's a reasonable way to factor this code to a higher level common to all
RWHV's.  I'll upload another patch shortly.

[Fady Samuel, 2013-09-18 21:59:37]

On 2013/09/18 21:46:55, zturner wrote:
> On 2013/09/18 20:56:05, jamesr wrote:
> > I'm not entirely sure what is more correct in the overall design (and I
doubt
> > that anyone does), but I think it's more important to get consistency first
so
> > that if we do have bugs we at least have the same bugs on every platform and
> can
> > fix them all.  To me, showing and hiding should parallel each other as I'm
not
> > sure when one would not imply the other.
> 
> Fair enough.  It's unfortunate that every current and future implementation of
> RWHW has to remember to do this, perhaps at some point we can figure out if
> there's a reasonable way to factor this code to a higher level common to all
> RWHV's.  I'll upload another patch shortly.

The <webview> team and out-of-process iframe team has been chatting about this
offline for a while now, on and off. As far as I can tell, the issue here is
there no contract for how platform-dependent code can call into
platform-independent code. There needs to be a design pattern that establishes a
contract between RenderWidgetHost and RenderWidgetHostViewPlatform*. Perhaps
some kind of wrapper RenderWidgetHost <=> [two-way contract]
RenderWidgetHostViewWrapper =>  [one way contract]
RenderWidgetHostViewPlatform*.

Some of the problem stems from the fact that certain platforms have
platform-specific operations. I feel that those platform-specific operations
also need a wrapper to avoid inadvertent abuse and to maintain the contract that
platform-DEPENDENT code never calls directly into platform independent code.

I will avoid discussing design further in a code review, but hopefully we can
eventually agree upon a way to structure the code that works for all platforms
and for OOPIFs/<webview>.

[zturner, 2013-09-19 00:20:02]

Somehow Rietveld got confused and none of the diffs work now.  Tried
re-uploading but it doesn't fix the problem, it looks like the base files are
messed up.

Created a new issue here:

https://codereview.chromium.org/24101003