Remove undisplayed tabs from the views hierarchy because event delivery doesn't respect z-order.

Remove undisplayed tabs from the views hierarchy because event delivery doesn't respect z-order.


BUG=none
TEST=manually; open two tabs, switch between them and touch items

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69802

[Alex Nicolaou, 2010-12-10 05:49:14]

Fix event delivery for multiple instances of TabContentsViewViews.

[sky, 2010-12-10 16:37:21]

http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
File chrome/browser/ui/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
chrome/browser/ui/views/tab_contents/tab_contents_container.cc:43:
RemoveChildView(v);
All these ifdefs are making this class unreadable and hard to follow. Remove all
the methods in tab_contents_container.cc that are ifdefd, putting the
implementations in tab_contents_container_touch.cc and
tab_contents_container_views.cc (yes, name sucks).

[Ben Goodger (Google), 2010-12-10 17:58:34]

fyi sky and I discussed this and agreed that while awkward it was better
than all the ifdefs.

-Ben

On Fri, Dec 10, 2010 at 8:37 AM, <sky@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
> File chrome/browser/ui/views/tab_contents/tab_contents_container.cc
> (right):
>
>
>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
> chrome/browser/ui/views/tab_contents/tab_contents_container.cc:43:
> RemoveChildView(v);
> All these ifdefs are making this class unreadable and hard to follow.
> Remove all the methods in tab_contents_container.cc that are ifdefd,
> putting the implementations in tab_contents_container_touch.cc and
> tab_contents_container_views.cc (yes, name sucks).
>
>
> http://codereview.chromium.org/5700003/
>

[Alex Nicolaou, 2010-12-10 17:58:34]

I really don't want to duplicate the entire class for the sake of 10
lines. Can we come up with some lighter-weight way? A common base
class?

alex

On Friday, December 10, 2010,  <sky@chromium.org> wrote:
>
>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
> File chrome/browser/ui/views/tab_contents/tab_contents_container.cc
> (right):
>
>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
> chrome/browser/ui/views/tab_contents/tab_contents_container.cc:43:
> RemoveChildView(v);
> All these ifdefs are making this class unreadable and hard to follow.
> Remove all the methods in tab_contents_container.cc that are ifdefd,
> putting the implementations in tab_contents_container_touch.cc and
> tab_contents_container_views.cc (yes, name sucks).
>
> http://codereview.chromium.org/5700003/
>

[Ben Goodger (Google), 2010-12-10 17:59:09]

You don't need to duplicate the class... just move methods that are highly
specialized to touch into foo_touch.cc

-Ben

On Fri, Dec 10, 2010 at 9:58 AM, Alex Nicolaou <anicolao@chromium.org>wrote:

> I really don't want to duplicate the entire class for the sake of 10
> lines. Can we come up with some lighter-weight way? A common base
> class?
>
> alex
>
> On Friday, December 10, 2010,  <sky@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
> > File chrome/browser/ui/views/tab_contents/tab_contents_container.cc
> > (right):
> >
> >
>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
> > chrome/browser/ui/views/tab_contents/tab_contents_container.cc:43:
> > RemoveChildView(v);
> > All these ifdefs are making this class unreadable and hard to follow.
> > Remove all the methods in tab_contents_container.cc that are ifdefd,
> > putting the implementations in tab_contents_container_touch.cc and
> > tab_contents_container_views.cc (yes, name sucks).
> >
> > http://codereview.chromium.org/5700003/
> >
>

[Alex Nicolaou, 2010-12-10 18:03:10]

OK. Can I commit this two-line fix and follow up with a refactor CL?
(If so, I promise to get this refactor CL out for review today.) The
bug fixed here makes it very difficult to test for other members of my
team.

alex

On Friday, December 10, 2010, Ben Goodger (Google) <ben@chromium.org> wrote:
> You don't need to duplicate the class... just move methods that are highly
specialized to touch into foo_touch.cc
> -Ben
>
> On Fri, Dec 10, 2010 at 9:58 AM, Alex Nicolaou <anicolao@chromium.org> wrote:
> I really don't want to duplicate the entire class for the sake of 10
> lines. Can we come up with some lighter-weight way? A common base
> class?
>
> alex
>
> On Friday, December 10, 2010,  <sky@chromium.org> wrote:
>>
>>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
>> File chrome/browser/ui/views/tab_contents/tab_contents_container.cc
>> (right):
>>
>>
http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
>> chrome/browser/ui/views/tab_contents/tab_contents_container.cc:43:
>> RemoveChildView(v);
>> All these ifdefs are making this class unreadable and hard to follow.
>> Remove all the methods in tab_contents_container.cc that are ifdefd,
>> putting the implementations in tab_contents_container_touch.cc and
>> tab_contents_container_views.cc (yes, name sucks).
>>
>> http://codereview.chromium.org/5700003/
>>
>
>
>

[sky, 2010-12-10 18:20:07]

As long you cleanup later on, LGTM

http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
File chrome/browser/ui/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/5700003/diff/1/chrome/browser/ui/views/tab_con...
chrome/browser/ui/views/tab_contents/tab_contents_container.cc:42: views::View
*v = static_cast<TabContentsViewViews*>(tab_contents_->view());
nit: View* v.