Stack preview back at top after an active contents switch.

Stack preview back at top after an active contents switch.


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

[MAD, 2012-11-29 14:32:24]

There are a bunch of things that AddChildView does that ReorderChildView doesn't
do. I have not investigated which ones makes a difference in our case, it might
be the fact that the layout manager is not notified for reorder, or the fact
that InitFocusSiblings is also called (which should be fixing related issue
162112)...

I could research further if you want...

[sreeram, 2012-11-29 17:57:50]

lgtm

[MAD, 2012-11-29 20:24:38]

Thanks Sreeram... 

sky@, care to OWNER review this small change?

Thanks!

BYE
MAD...

[sky, 2012-11-29 20:57:54]

https://codereview.chromium.org/11428083/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/11428083/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:59:
RemoveChildView(preview_);
Will this result in sending a bunch of messages to the preview_? Such as perhaps
its been hidden then removed?

[MAD, 2012-11-29 21:21:30]

On 2012/11/29 20:57:54, sky wrote:
>
https://codereview.chromium.org/11428083/diff/1/chrome/browser/ui/views/frame...
> File chrome/browser/ui/views/frame/contents_container.cc (right):
> 
>
https://codereview.chromium.org/11428083/diff/1/chrome/browser/ui/views/frame...
> chrome/browser/ui/views/frame/contents_container.cc:59:
> RemoveChildView(preview_);
> Will this result in sending a bunch of messages to the preview_? Such as
perhaps
> its been hidden then removed?

I'll try to confirm it in the debugger, but from an execution point of view, if
I click on the identify widget and it pops something up (e.g., context menu to
switch account) it stays popped up as I switch to another new tab (which is a
weird behavior that is already there without my change on Chrome OS), so I think
there are no interruption of display...

But I can dig deeper to make sure...

[MAD, 2012-11-29 21:35:11]

The only think I see happening at the web view level is the hierarchy change,
and when is_add, it calls attach we content, but since the content didn't
change, it's a noop:

void WebView::AttachWebContents() {
  // Prevents attachment if the WebView isn't already in a Widget, or it's
  // already attached.
  if (!GetWidget() || !web_contents_ ||
      wcv_holder_->native_view() == web_contents_->GetNativeView()) {
    return;
  }

And then a SetFocus...

Is there anything else that could happen that wouldn't go through the web view?
I set breakpoints on a  views::WebView::* symbols and this is all I saw when I
switch tabs.

Thanks!

[MAD, 2012-11-29 22:46:36]

On 2012/11/29 21:35:11, MAD wrote:
> The only think I see happening at the web view level is the hierarchy change,
> and when is_add, it calls attach we content, but since the content didn't
> change, it's a noop:
> 
> void WebView::AttachWebContents() {
>   // Prevents attachment if the WebView isn't already in a Widget, or it's
>   // already attached.
>   if (!GetWidget() || !web_contents_ ||
>       wcv_holder_->native_view() == web_contents_->GetNativeView()) {
>     return;
>   }
> 
> And then a SetFocus...
> 
> Is there anything else that could happen that wouldn't go through the web
view?
> I set breakpoints on a  views::WebView::* symbols and this is all I saw when I
> switch tabs.
> 
> Thanks!

OK, so I dug a little deeper and it does cause a webkitvisibilitychange to be
fired, which doesn't happen on the current crOS layering... :-/

I'll also try to break on all methods of the renderviewhost as sky suggested.

Could we still commit this anyway and file a bug for a better solution not
causing the event to fire?

Note that the current layering solution on crOS doesn't bring back the
clickability on the webview... This change should fix that... So if we look for
another solution, we will need one that also fixes the clickability issue.

I'll see now if I can find a way to restore the proper z-order without removing
the child...

[sreeram, 2012-12-01 00:24:02]

I think it's okay to send the visibilitychanged messages to the renderer. The
user *is* switching tabs, after all. It's not like the user didn't do anything
and we still sent those extra messages.

You could even think of this as a feature that lets the Instant preview detect
when the user switches tabs. Recall the case we are talking about: You have two
NTP tabs, and you switch from one to the other. Since both are NTP, we want to
keep the Instant preview portion of the NTP (with the logo) showing across the
tab switch. Perhaps the preview can use the visibilitychanged messages to reset
any NTP state (so if the user had clicked on the Google+ notification popup or
such, it could decide that it should be closed).

[MAD, 2012-12-03 14:40:41]

Thanks Sreeram. So sky@, are you OK with the fix of removing and adding the
child? Or you prefer the Windows only fix of reparenting?

And if we choose the child fix, are you OK with doing it for both Aura and
non-Aura builds? Or do you prefer to keep the layer on top fix for Aura, and
then we need to find another fix for the clickability...

I vote for the child remove/add fix for all platforms...

Note that, as I said in another comment, this is likely to go away since we are
thinking of moving the bottom contents into the google.com page, and so we won't
need to deal with layering...

Thanks!

[sky, 2012-12-03 18:08:40]

LGTM

[commit-bot: I haz the power, 2012-12-03 19:01:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/11428083/1

[commit-bot: I haz the power, 2012-12-03 21:50:15]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-03 21:53:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/11428083/1

[commit-bot: I haz the power, 2012-12-04 03:19:08]

Retried try job too often on mac_rel for step(s) browser_tests

[sky, 2012-12-04 08:24:15]

Stick with the add/remove.

On Mon, Dec 3, 2012 at 6:40 AM,  <mad@chromium.org> wrote:
> Thanks Sreeram. So sky@, are you OK with the fix of removing and adding the
> child? Or you prefer the Windows only fix of reparenting?
>
> And if we choose the child fix, are you OK with doing it for both Aura and
> non-Aura builds? Or do you prefer to keep the layer on top fix for Aura, and
> then we need to find another fix for the clickability...
>
> I vote for the child remove/add fix for all platforms...
>
> Note that, as I said in another comment, this is likely to go away since we
> are
> thinking of moving the bottom contents into the google.com page, and so we
> won't
> need to deal with layering...
>
> Thanks!
>
>
> https://codereview.chromium.org/11428083/

[MAD, 2012-12-04 08:27:22]

Our emails have crossed...

So all that is missing is your OWNER l g t m before I can send this to the
CQ.

Thanks!


On Mon, Dec 3, 2012 at 11:13 AM, Scott Violet <sky@chromium.org> wrote:

> Stick with the add/remove.
>
> On Mon, Dec 3, 2012 at 6:40 AM,  <mad@chromium.org> wrote:
> > Thanks Sreeram. So sky@, are you OK with the fix of removing and adding
> the
> > child? Or you prefer the Windows only fix of reparenting?
> >
> > And if we choose the child fix, are you OK with doing it for both Aura
> and
> > non-Aura builds? Or do you prefer to keep the layer on top fix for Aura,
> and
> > then we need to find another fix for the clickability...
> >
> > I vote for the child remove/add fix for all platforms...
> >
> > Note that, as I said in another comment, this is likely to go away since
> we
> > are
> > thinking of moving the bottom contents into the google.com page, and so
> we
> > won't
> > need to deal with layering...
> >
> > Thanks!
> >
> >
> > https://codereview.chromium.org/11428083/
>