Fix pinned tab link navigations.

Fix pinned tab link navigations.

This ties up that loose end I mentioned in http://codereview.chromium.org/2747011 (I had put this off because I expected it would require slightly more effort).

BUG=29281
TEST=manual

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

[sky, 2010-06-23 17:44:27]

http://codereview.chromium.org/2844017/diff/1/2
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/2844017/diff/1/2#newcode412
chrome/browser/tabs/tab_strip_model.cc:412:
contents->render_view_host()->SyncRendererPrefs();
Do you need to null check the render_view_host? Other places seem to.

[Peter Kasting, 2010-06-23 18:21:39]

I'm not really qualified to review the critical bit of this.

http://codereview.chromium.org/2844017/diff/1/2
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/2844017/diff/1/2#newcode381
chrome/browser/tabs/tab_strip_model.cc:381: CHECK(ContainsIndex(index));
Do we really want all these DCHECKs converted to CHECKs?  I don't think that's
necessitated by converting at() to []... a DCHECK to make the function
preconditions clear seems sufficient to me.

http://codereview.chromium.org/2844017/diff/1/3
File chrome/common/renderer_preferences.h (right):

http://codereview.chromium.org/2844017/diff/1/3#newcode1
chrome/common/renderer_preferences.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
Nit: 2010

[Evan Stade, 2010-06-23 20:18:48]

http://codereview.chromium.org/2844017/diff/1/2
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/2844017/diff/1/2#newcode381
chrome/browser/tabs/tab_strip_model.cc:381: CHECK(ContainsIndex(index));
On 2010/06/23 18:21:40, Peter Kasting wrote:
> Do we really want all these DCHECKs converted to CHECKs?  I don't think that's
> necessitated by converting at() to []... a DCHECK to make the function
> preconditions clear seems sufficient to me.

yea. The DCHECK->CHECK is separate from the at() to []. I did the latter to make
the file consistent. I did the former because if the [D]CHECK fails, the program
will crash later. Better to CHECK than crash via null dereference.

http://codereview.chromium.org/2844017/diff/1/2#newcode412
chrome/browser/tabs/tab_strip_model.cc:412:
contents->render_view_host()->SyncRendererPrefs();
On 2010/06/23 17:44:27, sky wrote:
> Do you need to null check the render_view_host? Other places seem to.

The closest analogy to this code is void
Browser::ConvertContentsToApplication(), which does

  tab_contents->GetMutableRendererPrefs()->can_accept_load_drops = false;
  tab_contents->render_view_host()->SyncRendererPrefs();

and it doesn't make that check.

[sky, 2010-06-23 20:35:19]

On Wed, Jun 23, 2010 at 1:18 PM,  <estade@chromium.org> wrote:
>
> http://codereview.chromium.org/2844017/diff/1/2
> File chrome/browser/tabs/tab_strip_model.cc (right):
>
> http://codereview.chromium.org/2844017/diff/1/2#newcode381
> chrome/browser/tabs/tab_strip_model.cc:381: CHECK(ContainsIndex(index));
> On 2010/06/23 18:21:40, Peter Kasting wrote:
>>
>> Do we really want all these DCHECKs converted to CHECKs?  I don't
>
> think that's
>>
>> necessitated by converting at() to []... a DCHECK to make the function
>> preconditions clear seems sufficient to me.
>
> yea. The DCHECK->CHECK is separate from the at() to []. I did the latter
> to make the file consistent. I did the former because if the [D]CHECK
> fails, the program will crash later. Better to CHECK than crash via null
> dereference.
>
> http://codereview.chromium.org/2844017/diff/1/2#newcode412
> chrome/browser/tabs/tab_strip_model.cc:412:
> contents->render_view_host()->SyncRendererPrefs();
> On 2010/06/23 17:44:27, sky wrote:
>>
>> Do you need to null check the render_view_host? Other places seem to.
>
> The closest analogy to this code is void
> Browser::ConvertContentsToApplication(), which does
>
>  tab_contents->GetMutableRendererPrefs()->can_accept_load_drops =
> false;
>  tab_contents->render_view_host()->SyncRendererPrefs();
>
> and it doesn't make that check.

OTOH if you search for checks of render_view_host(), there are plenty.

  -Scott

[sky, 2010-06-23 20:35:59]

On Wed, Jun 23, 2010 at 1:35 PM, Scott Violet <sky@chromium.org> wrote:
> On Wed, Jun 23, 2010 at 1:18 PM,  <estade@chromium.org> wrote:
>>
>> http://codereview.chromium.org/2844017/diff/1/2
>> File chrome/browser/tabs/tab_strip_model.cc (right):
>>
>> http://codereview.chromium.org/2844017/diff/1/2#newcode381
>> chrome/browser/tabs/tab_strip_model.cc:381: CHECK(ContainsIndex(index));
>> On 2010/06/23 18:21:40, Peter Kasting wrote:
>>>
>>> Do we really want all these DCHECKs converted to CHECKs?  I don't
>>
>> think that's
>>>
>>> necessitated by converting at() to []... a DCHECK to make the function
>>> preconditions clear seems sufficient to me.
>>
>> yea. The DCHECK->CHECK is separate from the at() to []. I did the latter
>> to make the file consistent. I did the former because if the [D]CHECK
>> fails, the program will crash later. Better to CHECK than crash via null
>> dereference.
>>
>> http://codereview.chromium.org/2844017/diff/1/2#newcode412
>> chrome/browser/tabs/tab_strip_model.cc:412:
>> contents->render_view_host()->SyncRendererPrefs();
>> On 2010/06/23 17:44:27, sky wrote:
>>>
>>> Do you need to null check the render_view_host? Other places seem to.
>>
>> The closest analogy to this code is void
>> Browser::ConvertContentsToApplication(), which does
>>
>>  tab_contents->GetMutableRendererPrefs()->can_accept_load_drops =
>> false;
>>  tab_contents->render_view_host()->SyncRendererPrefs();
>>
>> and it doesn't make that check.
>
> OTOH if you search for checks of render_view_host(), there are plenty.

If you think it's ok to not null-check, please add a comment as to why
it won't be null here.

  -Scott

[Evan Stade, 2010-06-23 20:43:24]

I don't know or have a convincing argument for why render_view_host() can't be
NULL. Check added.

[sky, 2010-06-23 20:45:24]

LGTM

[Peter Kasting, 2010-06-23 20:47:37]

http://codereview.chromium.org/2844017/diff/1/2
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/2844017/diff/1/2#newcode381
chrome/browser/tabs/tab_strip_model.cc:381: CHECK(ContainsIndex(index));
On 2010/06/23 20:18:48, Evan Stade wrote:
> On 2010/06/23 18:21:40, Peter Kasting wrote:
> > Do we really want all these DCHECKs converted to CHECKs?
> 
> I did the former because if the [D]CHECK fails, the program
> will crash later. Better to CHECK than crash via null dereference.

Hmm.  I don't really agree, especially since NULL deref crashes are fairly easy
to trace back.  As the last part of our coding style doc alludes to (
http://dev.chromium.org/developers/coding-style ), we normally use DCHECK to
indicate "this is how this function must be used" and only rely on CHECK when
trying to track down a bug or to avoid exploitable holes.  I really think the
previous DCHECKs would be better here.

[Evan Stade, 2010-06-23 21:00:15]

http://codereview.chromium.org/2844017/diff/1/2
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/2844017/diff/1/2#newcode381
chrome/browser/tabs/tab_strip_model.cc:381: CHECK(ContainsIndex(index));
On 2010/06/23 20:47:37, Peter Kasting wrote:
> On 2010/06/23 20:18:48, Evan Stade wrote:
> > On 2010/06/23 18:21:40, Peter Kasting wrote:
> > > Do we really want all these DCHECKs converted to CHECKs?
> > 
> > I did the former because if the [D]CHECK fails, the program
> > will crash later. Better to CHECK than crash via null dereference.
> 
> Hmm.  I don't really agree, especially since NULL deref crashes are fairly
easy
> to trace back.  As the last part of our coding style doc alludes to (
> http://dev.chromium.org/developers/coding-style ), we normally use DCHECK to
> indicate "this is how this function must be used" and only rely on CHECK when
> trying to track down a bug or to avoid exploitable holes.  I really think the
> previous DCHECKs would be better here.

CHECKs are the easiest type of crash to understand because the code is
self-documenting. I disagree with the link you posted, in particular,

"In some cases the consequences of a failed DCHECK may result in a crash. This
is not necessarily a terrible thing. The crash will be logged, and that will
result in visibility to the fact that the DCHECK fails in the wild."

DCHECKs don't execute in the wild, so aside from manual inspection of the code,
there's no visibility of the DCHECK's failure in crash reports. If we are going
to crash anyway, it's more transparent in the crash report to fail fast. But
clearly this has nothing to do with the patch at hand, and I'd rather land it
than argue about this.

[Peter Kasting, 2010-06-23 21:04:16]

On 2010/06/23 21:00:15, Evan Stade wrote:
> I disagree with the link you posted, in particular,
> 
> "In some cases the consequences of a failed DCHECK may result in a crash. This
> is not necessarily a terrible thing. The crash will be logged, and that will
> result in visibility to the fact that the DCHECK fails in the wild."
> 
> DCHECKs don't execute in the wild, so aside from manual inspection of the
code,
> there's no visibility of the DCHECK's failure in crash reports.

The "manual inspection of the code" part was supposed to be implied by that
text.

The whole section was written quite a while ago by Darin after some discussion
on the team mailing list, so if you think it's wrong, you should probably raise
it on chromium-dev.  It is better to have a policy everyone knows and follows,
so if our current policy isn't optimal we should change it.