Fix: Certain redirections remove sites from the history...

Fix: Certain redirections remove sites from the history

Currently, PageTransition::CHAIN_END flag is removed from a History database
entry for a redirect source, even when the redirect is user initiated.

This change prevents the flag removal for user-initiated redirects.

TEST=Open http://www.google.com/ig and click on tabs multiple times. Without
this change, only the last tab clicked appears in the History page (CTRL+H).
With this change, all the tabs should appear.

TESTED=gcl try, manually

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

[Yuzo, 2009-06-25 10:29:36]

Hi, Brett,

Can you review this?

Yuzo

[Yuzo, 2009-06-29 04:50:56]

On 2009/06/25 10:29:36, yuzo wrote:
> Hi, Brett,
> 
> Can you review this?
> 
> Yuzo

Ping?

[brettw, 2009-06-29 23:52:54]

Sorry I lost this patch. We should be sure to get a unit test for this new
behavior. Can you please write one?

[brettw, 2009-06-29 23:57:25]

Let me try to direct you in a good direction:

Probably the best place for this is in history_backend_unittest.cc (the other
option is history_unittest.cc but it includes threading which may make it more
complicated).

You can use different URLs for each step, then use GetRowForURL +
GetVisitsForURL to check the transition type assigned. Be sure to test the
non-user-initiated case as well, and you can delete the TODO in history backend
around where you made your changes that says it should be tested.

Thanks!

[Yuzo, 2009-06-30 08:13:04]

Brett,

Thank you for the detailed comment.

I've added a test to history_backend_unittest.cc and
removed the TODO comment from history_backend.cc.

Can you take another look?

Yuzo

On 2009/06/29 23:57:25, brettw wrote:
> Let me try to direct you in a good direction:
> 
> Probably the best place for this is in history_backend_unittest.cc (the other
> option is history_unittest.cc but it includes threading which may make it more
> complicated).
> 
> You can use different URLs for each step, then use GetRowForURL +
> GetVisitsForURL to check the transition type assigned. Be sure to test the
> non-user-initiated case as well, and you can delete the TODO in history
backend
> around where you made your changes that says it should be tested.
> 
> Thanks!

[brettw, 2009-06-30 17:34:33]

LGTM with a few minor comments.

http://codereview.chromium.org/147145/diff/3031/2021
File chrome/browser/history/history_backend_unittest.cc (right):

http://codereview.chromium.org/147145/diff/3031/2021#newcode77
Line 77: void AddClientRedirectChain(const GURL& url1, const GURL& url2,
The function arguments should always line up, so align the wrapped ones to the (
on the previous line.

Can you also add a comment for this function describing what it does?

http://codereview.chromium.org/147145/diff/3031/2012
File chrome/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/147145/diff/3031/2012#newcode406
Line 406: void RendererDidNavigateToNewPage(
Can you add another block to this comment saying something like "The functions
taking |did_replace_entry| will fill into the given variable whether the last
entry has been replaced or not. See LoadCommittedDetails.did_replace_entry."

[Yuzo, 2009-07-01 01:56:33]

Brett,

Thank you very much for your review.
I'll submit this after confirming gcl try still succeeds.

Yuzo

http://codereview.chromium.org/147145/diff/3031/2021
File chrome/browser/history/history_backend_unittest.cc (right):

http://codereview.chromium.org/147145/diff/3031/2021#newcode77
Line 77: void AddClientRedirectChain(const GURL& url1, const GURL& url2,
On 2009/06/30 17:34:33, brettw wrote:
> The function arguments should always line up, so align the wrapped ones to the
(
> on the previous line.
> 
> Can you also add a comment for this function describing what it does?

Fixed the alignment and added the comment.

http://codereview.chromium.org/147145/diff/3031/2012
File chrome/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/147145/diff/3031/2012#newcode406
Line 406: void RendererDidNavigateToNewPage(
On 2009/06/30 17:34:33, brettw wrote:
> Can you add another block to this comment saying something like "The functions
> taking |did_replace_entry| will fill into the given variable whether the last
> entry has been replaced or not. See LoadCommittedDetails.did_replace_entry."

Done.