Further refine the pinned tab navigation algorithm.

Further refine the pinned tab navigation algorithm.

BUG=29281
TEST=included

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

[Evan Stade, 2010-06-11 23:26:59]

This doesn't actually work as is. Clicking on a link in a pinned tab will always
navigate within that pinned tab, even cross-domain. That is because the browser
doesn't handle the navigation. To fix this, we need to adjust
RenderView::decidePolicyForNavigation() so that it lets the browser handle the
navigation if the tab is pinned (as it does in many other cases). To do this we
can either

- create a new sync IPC where the render view asks the browser whether it is
pinned (not sure how bad this is for performance).
- inform the render view every time its tab's pinned state changes.

I presume the latter is better, even though it breaks the tabstrip abstraction,
sort of.

[Peter Kasting, 2010-06-11 23:47:30]

LGTM

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

http://codereview.chromium.org/2747011/diff/1/2#newcode3579
chrome/browser/browser.cc:3579: transition != PageTransition::LINK) {
Nit: No {}; you could combine all three of the above statements without (IMO)
loss of readability

http://codereview.chromium.org/2747011/diff/1/4
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/2747011/diff/1/4#newcode889
chrome/browser/browser_browsertest.cc:889: // TODO(ben): this test was never
enabled. It has bit-rotted since being added.
Do we really want to move this code rather than delete it?

[Evan Stade, 2010-06-12 00:18:56]

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

http://codereview.chromium.org/2747011/diff/1/2#newcode3579
chrome/browser/browser.cc:3579: transition != PageTransition::LINK) {
On 2010/06/11 23:47:31, Peter Kasting wrote:
> Nit: No {}; you could combine all three of the above statements without (IMO)
> loss of readability

Done.

http://codereview.chromium.org/2747011/diff/1/4
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/2747011/diff/1/4#newcode889
chrome/browser/browser_browsertest.cc:889: // TODO(ben): this test was never
enabled. It has bit-rotted since being added.
On 2010/06/11 23:47:31, Peter Kasting wrote:
> Do we really want to move this code rather than delete it?

hopefully someone will enable it, and it will help us in our coverage goals.

[sky, 2010-06-14 16:42:01]

LGTM

http://codereview.chromium.org/2747011/diff/8001/9001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/2747011/diff/8001/9001#newcode3589
chrome/browser/browser.cc:3589: if (url.host() == referrer.host() &&
scheme_matches)
How about a comment as to why we do this.

http://codereview.chromium.org/2747011/diff/8001/9002
File chrome/browser/browser.h (right):

http://codereview.chromium.org/2747011/diff/8001/9002#newcode920
chrome/browser/browser.h:920: // |current_tab| is pinned.
there is no current_tab parameter.

http://codereview.chromium.org/2747011/diff/8001/9004
File chrome/browser/browser_unittest.cc (right):

http://codereview.chromium.org/2747011/diff/8001/9004#newcode100
chrome/browser/browser_unittest.cc:100:
kNavigationScenarios[i].original_disposition));
Add:
<< i
so that if it fails you know where.

[Evan Stade, 2010-06-14 18:44:19]

http://codereview.chromium.org/2747011/diff/8001/9001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/2747011/diff/8001/9001#newcode3589
chrome/browser/browser.cc:3589: if (url.host() == referrer.host() &&
scheme_matches)
On 2010/06/14 16:42:01, sky wrote:
> How about a comment as to why we do this.

Done.

http://codereview.chromium.org/2747011/diff/8001/9002
File chrome/browser/browser.h (right):

http://codereview.chromium.org/2747011/diff/8001/9002#newcode920
chrome/browser/browser.h:920: // |current_tab| is pinned.
On 2010/06/14 16:42:01, sky wrote:
> there is no current_tab parameter.

Done.

http://codereview.chromium.org/2747011/diff/8001/9004
File chrome/browser/browser_unittest.cc (right):

http://codereview.chromium.org/2747011/diff/8001/9004#newcode100
chrome/browser/browser_unittest.cc:100:
kNavigationScenarios[i].original_disposition));
On 2010/06/14 16:42:01, sky wrote:
> Add:
> << i
> so that if it fails you know where.

Done.