First cut at creating a refactored version of tab_contents_views. This is

First cut at creating a refactored version of tab_contents_views. This is
motivated by a discussion with Ben and Scott in the process of fixing a z-order
issue in http://codereview.chromium.org/5700003/

BUG=none
TEST=manually on both touch and non-touch builds, trybots

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

[Alex Nicolaou, 2010-12-21 03:46:13]

Here is the refactored CL we'd discussed.

I will commit the original fix in isolation. This one is now with the try bots
so I may edit again if some problem is discovered - a quick manual test showed
all is well.

thanks
alex

[Alex Nicolaou, 2010-12-22 22:09:46]

Adding Brett since I'm guessing Scott&Ben are on vacation now. Thanks Brett!

alex

[brettw, 2010-12-22 23:02:06]

I'm a little hesitant to review this since I'm quite unfamiliar with the
TabContentsContainer level, and even after reading the previous discussion, I'm
not 100% sure I understand what Scott & Ben wanted. I've mostly only dealt with
TabContents and lower, how it gets in the browser, I have no idea!

If it's not going to be a huge hold-up, it would probably be better to wait for
Ben or Scott to get back. Sorry.

[Alex Nicolaou, 2010-12-23 12:47:59]

Sure, no problem. I already checked in the underlying bug fix that is what I
actually care about and I don't really understand the team's style of using
multiple files instead of #ifdef's so I could have easily gone about this
the "wrong" way. So we'll wait until Ben can check it out in January.

thanks for looking
alex

On Wed, Dec 22, 2010 at 6:02 PM, <brettw@chromium.org> wrote:

> I'm a little hesitant to review this since I'm quite unfamiliar with the
> TabContentsContainer level, and even after reading the previous discussion,
> I'm
> not 100% sure I understand what Scott & Ben wanted. I've mostly only dealt
> with
> TabContents and lower, how it gets in the browser, I have no idea!
>
> If it's not going to be a huge hold-up, it would probably be better to wait
> for
> Ben or Scott to get back. Sorry.
>
>
> http://codereview.chromium.org/6024007/
>

[sky, 2011-01-04 19:23:30]

Could you use svn cp so that we can easily see what changed?

  -Scott

[Alex Nicolaou, 2011-01-04 19:29:03]

Sure I can try to redo this, didn't mean to make incomprehensible diffs.

There is no change in functionality here, I pulled the ifdefs apart
into the different files and kept the common stuff in the common file.

I'm not using svn - is there a wiki page that describes the right
steps to get git to do what you want? Sorry to have to ask but I'm a
git novice at best :(

thanks
alex

On Tuesday, January 4, 2011,  <sky@chromium.org> wrote:
> Could you use svn cp so that we can easily see what changed?
>
>   -Scott
>
> http://codereview.chromium.org/6024007/
>

[brettw, 2011-01-04 20:06:28]

On Tue, Jan 4, 2011 at 11:28 AM, Alex Nicolaou <anicolao@chromium.org> wrote:
> Sure I can try to redo this, didn't mean to make incomprehensible diffs.
>
> There is no change in functionality here, I pulled the ifdefs apart
> into the different files and kept the common stuff in the common file.
>
> I'm not using svn - is there a wiki page that describes the right
> steps to get git to do what you want? Sorry to have to ask but I'm a
> git novice at best :(

I would probably ask on #chromium, there seems to always be people
there excited to talk about git.

Brett

[sky, 2011-01-05 16:43:03]

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container.cc:1: // For reasons
that are as yet unclear to me, all of this "common" code
Is this comment right?

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/tab_contents_container_native.cc
(left):

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container_native.cc:110: if
(native_container_) {
You need this if.

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:3582: ['touchui==1', {
Can this section be merged into the section on line 4285?

[Ben Goodger (Google), 2011-01-05 17:14:44]

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container.cc:1: // For reasons
that are as yet unclear to me, all of this "common" code
On 2011/01/05 16:43:03, sky wrote:
> Is this comment right?

I don't believe it is - the TAB_CONTENTS_DESTROYED observer is necessary for
crash prevention. Not to say it couldn't be better... I've always found it a
little odd.

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container.cc:2: // is dead
code. Left here for cleanup post-refactoring.
Also, comments like this should go below the license... otherwise I bet
presubmit will barf.

[Alex Nicolaou, 2011-01-06 09:37:17]

My computer is packed for the office move so it'll be a while before I can
actually upload a fixed patchset. We expect FDOB to be Monday.

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container.cc:1: // For reasons
that are as yet unclear to me, all of this "common" code
On 2011/01/05 16:43:03, sky wrote:
> Is this comment right?

No sorry this is construction dust from a intermediate state where my gyp file
was wrong and I was confused. I will remove it.

http://codereview.chromium.org/6024007/diff/9001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container.cc:2: // is dead
code. Left here for cleanup post-refactoring.
On 2011/01/05 17:14:44, Ben Goodger wrote:
> Also, comments like this should go below the license... otherwise I bet
> presubmit will barf.

Sorry this comment will get deleted it's wrong.

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:3582: ['touchui==1', {
On 2011/01/05 16:43:03, sky wrote:
> Can this section be merged into the section on line 4285?

Maybe but I don't think so. Can we follow up on this in a separate CL? Also
these diffs look wrong, I'll have to look at how I made my patch, only the lines
above about tab_contents_container_* are relevant.

[sky, 2011-01-06 16:01:16]

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:3582: ['touchui==1', {
On 2011/01/06 09:37:17, Alex Nicolaou wrote:
> On 2011/01/05 16:43:03, sky wrote:
> > Can this section be merged into the section on line 4285?
> 
> Maybe but I don't think so. Can we follow up on this in a separate CL? Also
> these diffs look wrong, I'll have to look at how I made my patch, only the
lines
> above about tab_contents_container_* are relevant.

Sure. Please file a bug so we don't forget it.

[Alex Nicolaou, 2011-01-12 18:51:14]

I think this is all fixed and ready to go, it is with the try bots now. PTAL

alex

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:3582: ['touchui==1', {
OK so I spent a minute to figure out why this is as it is. These
includes/excludes go with the generic views includes/excludes immediately above
here in the file, while the ones on line 4285 apply to the linux-specific
includes/excludes later in the file. I can't comment on why the pre-existing
views rules are coded this way in the gyp file, but I don't want to embark on
merging the two (large) views sections into one section, and these lines
logically belong near the rules that they override in my view. So I'd prefer to
change nothing, because moving these lines lower will obscure what they do/when
they apply, and merging the big sections is a lot more work.

Is that OK?

[sky, 2011-01-12 19:06:53]

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6024007/diff/9001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:3582: ['touchui==1', {
On 2011/01/12 18:51:14, Alex Nicolaou wrote:
> OK so I spent a minute to figure out why this is as it is. These
> includes/excludes go with the generic views includes/excludes immediately
above
> here in the file, while the ones on line 4285 apply to the linux-specific
> includes/excludes later in the file. I can't comment on why the pre-existing
> views rules are coded this way in the gyp file, but I don't want to embark on
> merging the two (large) views sections into one section, and these lines
> logically belong near the rules that they override in my view. So I'd prefer
to
> change nothing, because moving these lines lower will obscure what they
do/when
> they apply, and merging the big sections is a lot more work.
> 
> Is that OK?

Yes, that's fine.

http://codereview.chromium.org/6024007/diff/21001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_container_native.cc
(left):

http://codereview.chromium.org/6024007/diff/21001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_container_native.cc:110: if
(native_container_) {
You need this if.

[Alex Nicolaou, 2011-01-13 15:56:29]

PTA(hopefully final)L

thanks
alex

http://codereview.chromium.org/6024007/diff/21001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_container_native.cc
(left):

http://codereview.chromium.org/6024007/diff/21001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_container_native.cc:110: if
(native_container_) {
On 2011/01/12 19:06:53, sky wrote:
> You need this if.

Oh :( You're right, this pre-exists my previous change. I had added a lot of if
(native_container_) tests in the version where everything was all in one file
and I thought this was one of mine. I did a git blame on the file and verified
that this was the only test that I removed that I shouldn't have.

[sky, 2011-01-13 16:59:33]

LGTM

[loislo, 2011-02-05 11:42:22]

Looks like we have a perf regression here
http://build.chromium.org/f/chromium/perf/vista-release-single-core/morejs/re...

[sky, 2011-02-07 16:46:42]

loislo, please file a P1 against this.

  -Scott

On Sat, Feb 5, 2011 at 3:42 AM,  <loislo@chromium.org> wrote:
> Looks like we have a perf regression here
>
http://build.chromium.org/f/chromium/perf/vista-release-single-core/morejs/re...
>
>
> http://codereview.chromium.org/6024007/
>

[Alex Nicolaou, 2011-02-07 19:18:36]

I'm back in the office tomorrow, will make it a priority to take a look

alex

On Mon, Feb 7, 2011 at 11:46 AM, Scott Violet <sky@chromium.org> wrote:

> loislo, please file a P1 against this.
>
>  -Scott
>
> On Sat, Feb 5, 2011 at 3:42 AM,  <loislo@chromium.org> wrote:
> > Looks like we have a perf regression here
> >
>
http://build.chromium.org/f/chromium/perf/vista-release-single-core/morejs/re...
> >
> >
> > http://codereview.chromium.org/6024007/
> >
>