Sync typed urls with typed_cont() == 0

Sync typed urls with typed_cont() == 0

When the user types "google.com" into the address bar, the server will
redirect him to "www.google.com" (or a regional equivalent). The redirected
URL will have PAGE_TRANSITION_TYPED but typed_count() == 0. We still want
to Sync such URL, otherwise the receiving client will only see "google.com"
with PAGE_TRANSITION_CHAIN_START and without the matching _CHAIN_END.
Additionally, the history view won't show that "google.com" URL because
VisitDatabase::GetVisibleVisitsInRange filters out results that don't
match PAGE_TRANSITION_CHAIN_END.

BUG=

Committed: https://crrev.com/7f5588cf33aa5e6a793382387058935a579421b8
Cr-Commit-Position: refs/heads/master@{#325990}

[mpawlowski, 2015-04-14 08:25:30]

mpawlowski@opera.com changed reviewers:
+ tim@chromium.org, zea@chromium.org

[Nicolas Zea, 2015-04-14 14:42:55]

The CQ bit was checked by zea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-14 14:43:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087773002/1

[commit-bot: I haz the power, 2015-04-14 14:52:46]

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

[commit-bot: I haz the power, 2015-04-14 14:52:47]

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...)

[Nicolas Zea, 2015-04-14 17:47:38]

What's to prevent Sync from syncing all urls here, even those not part of a
typed navigation chain? I wonder if perhaps we should be fixing
GetVisibleVisitsInRange to handle the case where a chain isn't ended?

[mpawlowski, 2015-04-15 07:03:33]

On 2015/04/14 17:47:38, Nicolas Zea wrote:
> What's to prevent Sync from syncing all urls here, even those not part of a
> typed navigation chain? I wonder if perhaps we should be fixing
> GetVisibleVisitsInRange to handle the case where a chain isn't ended?

See TypedUrlChangeProcessor::ShouldSyncVisit. Right above my change there's a
ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_TYPED) check.
When an address is typed into the address bar and the server redirects, a new
PAGE_TRANSITION_CHAIN_END address is added to history but it retains
PAGE_TRANSITION_TYPED (as it is a result of a TYPED navigation), only it's
typed_count() is zero (as it was never *actually* typed). PageTransition is a
bitmask, so that new address is marked as PAGE_TRANSITION_TYPED |
PAGE_TRANSITION_CHAIN_END | PAGE_TRANSITION_SERVER_REDIRECT.

Modifying the GetVisibleVisitsInRange logic to also return entries that aren't
PAGE_TRANSITION_CHAIN_END results in the history webui showing both google.com
and www.google.com, thus every typed address that was redirected is now
effectively duplicated. I also have a feeling that having incomplete redirection
chains in the history model is iffy in general.

[Nicolas Zea, 2015-04-16 15:52:08]

I see, makes sense. I think the only thing I'd like to see then is some tests
for this. Adding an integration test in two_client_typed_urls_sync_test might be
the right place (see some of the examples that exist there).

[mpawlowski, 2015-04-17 09:35:14]

On 2015/04/16 15:52:08, Nicolas Zea wrote:
> I see, makes sense. I think the only thing I'd like to see then is some tests
> for this. Adding an integration test in two_client_typed_urls_sync_test might
be
> the right place (see some of the examples that exist there).

Done. Had to write a new helper function because the existing
GetTypedUrlsFromClient uses the same typed_url() > 0 logic to find typed urls.
The 'urls' table only holds typed_count, not transition, so there's no way of
fixing that heuristic without having it return LINK entries as well. We have to
consult the 'visits' database to get the right results instead.

[Nicolas Zea, 2015-04-20 19:46:03]

LGTM, thanks for adding the test!

https://codereview.chromium.org/1087773002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
(right):

https://codereview.chromium.org/1087773002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:380:
IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, SyncTypedRedirects) {
nit: add a comment describing the purpose of the test?

[Nicolas Zea, 2015-04-21 00:15:10]

The CQ bit was checked by zea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-21 00:16:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087773002/20001

[commit-bot: I haz the power, 2015-04-21 01:45:48]

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

[commit-bot: I haz the power, 2015-04-21 01:45:49]

Dry run: This issue passed the CQ dry run.

[mpawlowski, 2015-04-21 07:00:51]

The CQ bit was checked by mpawlowski@opera.com

[commit-bot: I haz the power, 2015-04-21 07:01:01]

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

[commit-bot: I haz the power, 2015-04-21 07:02:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-21 07:03:35]

Patchset 2 (id:??) landed as
https://crrev.com/7f5588cf33aa5e6a793382387058935a579421b8
Cr-Commit-Position: refs/heads/master@{#325990}