Don't treat back/forwards as typed transitions in history.

Don't treat back/forwards as typed transitions in history.

BUG=393441
TEST=See bug comment 23 for repro steps

Committed: https://crrev.com/33237351a162fe71ee8daab3d12fe04f4006d72b
Cr-Commit-Position: refs/heads/master@{#322644}

[Charlie Reis, 2015-03-24 16:11:09]

creis@chromium.org changed reviewers:
+ pkasting@chromium.org

[Charlie Reis, 2015-03-24 16:11:10]

This appears to solve the typed count issue from comments 22-23 on the bug.

[Mark P, 2015-03-24 18:03:00]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-03-24 18:03:01]

https://codereview.chromium.org/1011943003/diff/1/components/history/core/bro...
File components/history/core/browser/expire_history_backend.cc (right):

https://codereview.chromium.org/1011943003/diff/1/components/history/core/bro...
components/history/core/browser/expire_history_backend.cc:389: //
TODO(pkasting): http://b/1148304 We shouldn't be marking so many URLs as
Can you clean up this comment too?

https://codereview.chromium.org/1011943003/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1011943003/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:728: // TODO(pkasting):
http://b/1148304 We shouldn't be marking so many URLs as
Can you clean up this comment too?

[Charlie Reis, 2015-03-24 19:13:19]

https://codereview.chromium.org/1011943003/diff/1/components/history/core/bro...
File components/history/core/browser/expire_history_backend.cc (right):

https://codereview.chromium.org/1011943003/diff/1/components/history/core/bro...
components/history/core/browser/expire_history_backend.cc:389: //
TODO(pkasting): http://b/1148304 We shouldn't be marking so many URLs as
On 2015/03/24 18:03:01, Mark P wrote:
> Can you clean up this comment too?

Sure, I guess these are stale given that the bug is closed.

[Peter Kasting, 2015-03-24 21:02:50]

https://codereview.chromium.org/1011943003/diff/20001/components/history/core...
File components/history/core/browser/expire_history_backend.cc (right):

https://codereview.chromium.org/1011943003/diff/20001/components/history/core...
components/history/core/browser/expire_history_backend.cc:394:
ui::PageTransitionIsNewNavigation(visits[i].transition) &&
This IsNewNavigation check should actually happen for KEYWORD_GENERATED as well
(in other words, it needs to be hoisted higher in this boolean expression).

(Both files.)

[Charlie Reis, 2015-03-24 21:54:41]

Good catch, thanks.

https://codereview.chromium.org/1011943003/diff/20001/components/history/core...
File components/history/core/browser/expire_history_backend.cc (right):

https://codereview.chromium.org/1011943003/diff/20001/components/history/core...
components/history/core/browser/expire_history_backend.cc:394:
ui::PageTransitionIsNewNavigation(visits[i].transition) &&
On 2015/03/24 21:02:50, Peter Kasting wrote:
> This IsNewNavigation check should actually happen for KEYWORD_GENERATED as
well
> (in other words, it needs to be hoisted higher in this boolean expression).
> 
> (Both files.)

Done.

[Peter Kasting, 2015-03-24 22:10:00]

LGTM

https://codereview.chromium.org/1011943003/diff/40001/chrome/browser/history/...
File chrome/browser/history/history_backend_unittest.cc (right):

https://codereview.chromium.org/1011943003/diff/40001/chrome/browser/history/...
chrome/browser/history/history_backend_unittest.cc:873: ASSERT_EQ(1,
row.typed_count());
Can you extend this test somewhere to verify that navigating back to this
doesn't bump the typed_count to 2, or else extend your new test to test the
KEYWORD_GENERATED type?

https://codereview.chromium.org/1011943003/diff/40001/chrome/browser/history/...
chrome/browser/history/history_backend_unittest.cc:1214:
backend_->AddPageVisit(url, base::Time::Now(), 0,
I would explicitly check that typed and visit counts are both 1 here.

You may also want to add another test that typed redirects don't get an
incremented typed_count when visited directly or via back/forward.

[Mark P, 2015-03-25 21:39:21]

mpearson@chromium.org changed reviewers:
- mpearson@chromium.org

[Charlie Reis, 2015-03-27 17:03:14]

Thanks.  Added the tests.  I'll commit if the try jobs look good.

https://codereview.chromium.org/1011943003/diff/40001/chrome/browser/history/...
File chrome/browser/history/history_backend_unittest.cc (right):

https://codereview.chromium.org/1011943003/diff/40001/chrome/browser/history/...
chrome/browser/history/history_backend_unittest.cc:873: ASSERT_EQ(1,
row.typed_count());
On 2015/03/24 22:09:59, Peter Kasting wrote:
> Can you extend this test somewhere to verify that navigating back to this
> doesn't bump the typed_count to 2, or else extend your new test to test the
> KEYWORD_GENERATED type?

Done.

https://codereview.chromium.org/1011943003/diff/40001/chrome/browser/history/...
chrome/browser/history/history_backend_unittest.cc:1214:
backend_->AddPageVisit(url, base::Time::Now(), 0,
On 2015/03/24 22:09:59, Peter Kasting wrote:
> I would explicitly check that typed and visit counts are both 1 here.
> 
> You may also want to add another test that typed redirects don't get an
> incremented typed_count when visited directly or via back/forward.

Done.

[Charlie Reis, 2015-03-27 18:01:25]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-03-27 18:01:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1011943003/#ps60001
(title: "Add tests")

[commit-bot: I haz the power, 2015-03-27 18:01:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011943003/60001

[commit-bot: I haz the power, 2015-03-27 18:12:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-27 18:12:21]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Reis, 2015-03-27 18:39:33]

creis@chromium.org changed reviewers:
+ sky@chromium.org

[Charlie Reis, 2015-03-27 18:39:34]

[+@sky]

Scott, can you review for owner's approval?

[sky, 2015-03-27 21:14:25]

LGTM

[Charlie Reis, 2015-03-27 21:21:02]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-03-27 21:21:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011943003/60001

[commit-bot: I haz the power, 2015-03-27 21:25:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-27 21:26:56]

Patchset 4 (id:??) landed as
https://crrev.com/33237351a162fe71ee8daab3d12fe04f4006d72b
Cr-Commit-Position: refs/heads/master@{#322644}