Better location for setting the size of tab contents. This should

Better location for setting the size of tab contents.  This should
catch all code paths.  I noticed that middle clicking a bookmark
wasn't working, so I found a single place to do this.

This code is always run when a tab is added and it sizes the
contents when it's loading in the background.

BUG=619

[Nate Chapin, 2009-08-13 18:56:54]

http://codereview.chromium.org/165473/diff/1/2
File chrome/browser/browser.cc (left):

http://codereview.chromium.org/165473/diff/1/2#oldcode1515
Line 1515:
contents->view()->SizeContents(old_contents->view()->GetContainerSize());
My impression (and this could be entirely wrong) was that, in this code path, it
was crucial for SizeContents() to be called before
contents->controller().LoadURL().  Is that still the case with this patch? 
Could you make me feel better and confirm that this code path still functions
correctly? :-)

[tony, 2009-08-13 19:31:52]

On 2009/08/13 18:56:54, Nate Chapin wrote:
> http://codereview.chromium.org/165473/diff/1/2
> File chrome/browser/browser.cc (left):
> 
> http://codereview.chromium.org/165473/diff/1/2#oldcode1515
> Line 1515:
> contents->view()->SizeContents(old_contents->view()->GetContainerSize());
> My impression (and this could be entirely wrong) was that, in this code path,
it
> was crucial for SizeContents() to be called before
> contents->controller().LoadURL().  Is that still the case with this patch? 
> Could you make me feel better and confirm that this code path still functions
> correctly? :-)

Do you have a test case where this code path is triggered?  I can't seem to find
one that uses a background tab.

[Nate Chapin, 2009-08-13 19:39:23]

This path is triggered by ctrl + clicking an anchor link on a page without
any special click event handling (I use wikipedia, since they have an
abundance of anchor links).

On Thu, Aug 13, 2009 at 12:31 PM, <tony@chromium.org> wrote:

> On 2009/08/13 18:56:54, Nate Chapin wrote:
>
>> http://codereview.chromium.org/165473/diff/1/2
>> File chrome/browser/browser.cc (left):
>>
>
>  http://codereview.chromium.org/165473/diff/1/2#oldcode1515
>> Line 1515:
>>
>
> contents->view()->SizeContents(old_contents->view()->GetContainerSize());
>
>> My impression (and this could be entirely wrong) was that, in this
>>
> code path, it
>
>> was crucial for SizeContents() to be called before
>> contents->controller().LoadURL().  Is that still the case with this
>>
> patch?
>
>> Could you make me feel better and confirm that this code path still
>>
> functions
>
>> correctly? :-)
>>
>
> Do you have a test case where this code path is triggered?  I can't seem
> to find one that uses a background tab.
>
>
> http://codereview.chromium.org/165473
>

[tony, 2009-08-13 20:21:51]

Ctrl+clicking an anchor link scrolls to the right place for me with this patch. 
I tested on Linux and Windows.

On 2009/08/13 19:39:23, Nate Chapin wrote:
> This path is triggered by ctrl + clicking an anchor link on a page without
> any special click event handling (I use wikipedia, since they have an
> abundance of anchor links).
> 
> On Thu, Aug 13, 2009 at 12:31 PM, <tony@chromium.org> wrote:
> 
> > On 2009/08/13 18:56:54, Nate Chapin wrote:
> >
> >> http://codereview.chromium.org/165473/diff/1/2
> >> File chrome/browser/browser.cc (left):
> >>
> >
> >  http://codereview.chromium.org/165473/diff/1/2#oldcode1515
> >> Line 1515:
> >>
> >
> > contents->view()->SizeContents(old_contents->view()->GetContainerSize());
> >
> >> My impression (and this could be entirely wrong) was that, in this
> >>
> > code path, it
> >
> >> was crucial for SizeContents() to be called before
> >> contents->controller().LoadURL().  Is that still the case with this
> >>
> > patch?
> >
> >> Could you make me feel better and confirm that this code path still
> >>
> > functions
> >
> >> correctly? :-)
> >>
> >
> > Do you have a test case where this code path is triggered?  I can't seem
> > to find one that uses a background tab.
> >
> >
> > http://codereview.chromium.org/165473
> >
>

[Nate Chapin, 2009-08-13 20:30:27]

LGTM

I just did some experimenting and confirmed this myself.  Sorry for being
paranoid.

On 2009/08/13 20:21:51, tony wrote:
> Ctrl+clicking an anchor link scrolls to the right place for me with this
patch. 
> I tested on Linux and Windows.
> 
> On 2009/08/13 19:39:23, Nate Chapin wrote:
> > This path is triggered by ctrl + clicking an anchor link on a page without
> > any special click event handling (I use wikipedia, since they have an
> > abundance of anchor links).
> > 
> > On Thu, Aug 13, 2009 at 12:31 PM, <tony@chromium.org> wrote:
> > 
> > > On 2009/08/13 18:56:54, Nate Chapin wrote:
> > >
> > >> http://codereview.chromium.org/165473/diff/1/2
> > >> File chrome/browser/browser.cc (left):
> > >>
> > >
> > >  http://codereview.chromium.org/165473/diff/1/2#oldcode1515
> > >> Line 1515:
> > >>
> > >
> > > contents->view()->SizeContents(old_contents->view()->GetContainerSize());
> > >
> > >> My impression (and this could be entirely wrong) was that, in this
> > >>
> > > code path, it
> > >
> > >> was crucial for SizeContents() to be called before
> > >> contents->controller().LoadURL().  Is that still the case with this
> > >>
> > > patch?
> > >
> > >> Could you make me feel better and confirm that this code path still
> > >>
> > > functions
> > >
> > >> correctly? :-)
> > >>
> > >
> > > Do you have a test case where this code path is triggered?  I can't seem
> > > to find one that uses a background tab.
> > >
> > >
> > > http://codereview.chromium.org/165473
> > >
> >