Don't create a sync node when updating an URL that wasn't typed.

Don't create a sync node when updating an URL that wasn't typed.

TypedUrlChangeProcessor is a HistoryService observer and gets notified
about changes to all addresses, not just typed ones. This means that
when HistoryService::SetPageTitle() is called for a non-typed URL,
TypedUrlChangeProcessor::OnURLsModified() will be triggered and,
consequently, TypedUrlChangeProcessor::CreateOrUpdateSyncNode().
As the name suggests, if the modified node was not previously present
(because it was filtered out by ShouldSyncVisit() in OnURLVisited()),
it would be created.
End effect was that *all* URLs that had their title updated (which means
every page that has a <title> tag) were synced, even if they were not typed.

The solution is to return early in CreateOrUpdateSyncNode() if we detect
that the URL in question has no typed visits.

(Caused by https://codereview.chromium.org/1087773002/)

BUG=

Committed: https://crrev.com/3be1b71d63be98a42c0e98673ff35fcd4fc738ab
Cr-Commit-Position: refs/heads/master@{#329370}

[mpawlowski, 2015-05-05 11:15:59]

mpawlowski@opera.com changed reviewers:
+ zea@google.com

[mpawlowski, 2015-05-05 11:17:28]

Fixing my regression :(

[mpawlowski, 2015-05-08 09:05:03]

bump?

[pval...(no longer on Chromium), 2015-05-08 17:26:22]

pvalenzuela@chromium.org changed reviewers:
+ maniscalco@chromium.org, pvalenzuela@chromium.org
- zea@google.com

[pval...(no longer on Chromium), 2015-05-08 17:26:23]

zea is out until May 19. I've added maniscalco@ for the glue code review.

from me:
lgtm for the test code

[maniscalco, 2015-05-08 19:38:00]

LGTM % one question...

https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
chrome/browser/sync/glue/typed_url_change_processor.cc:160: if
(std::find_if(visit_vector.begin(), visit_vector.end(),
Question about performance.  How big does visit_vector get and how often is
CreateOrUpdateSycnNode called?  If the vector is on the order of tens of items,
then I'm not worried.  Just want to make sure this isn't going to be super
expensive.

https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
(right):

https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:380:
IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest,
Nice test.

[mpawlowski, 2015-05-11 09:27:18]

https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
chrome/browser/sync/glue/typed_url_change_processor.cc:160: if
(std::find_if(visit_vector.begin(), visit_vector.end(),
On 2015/05/08 19:38:00, maniscalco wrote:
> Question about performance.  How big does visit_vector get and how often is
> CreateOrUpdateSycnNode called?  If the vector is on the order of tens of
items,
> then I'm not worried.  Just want to make sure this isn't going to be super
> expensive.

Good point, I've checked with an old profile with loads of history and it seems
this can get big. I've just seen a 400+ sized vector for facebook.com.

I'm not sure what's the best way to approach this now. The only way to know if
an URL should be synced is by establishing whether it's been typed. There are
two tables in the history database that govern this: 'urls' and 'visits'. Each
entry in 'urls' has a 'typed_count' (which is used widely through typed history
sync code) but it's currently a broken heuristic - if I type "facebook.com" and
it gets redirected to "https://www.facebook.com", the second address has
'typed_count' = 0 even though the corresponding visit in the 'visits' table has
type PAGE_TRANSITION_TYPED. This is why I decided to inspect the visits database
instead of urls.

Maybe I should investigate fixing 'typed_count' in the 'urls' table so that it
is incremented for URLs that are results of server-side redirections to things
typed into the address bar. Or come up with a fast method of checking the
'visits' table within HistoryBackend that doesn't require iterating through that
much data (don't know if it's possible).

[maniscalco, 2015-05-11 16:48:09]

On 2015/05/11 09:27:18, mpawlowski wrote:
>
https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
> File chrome/browser/sync/glue/typed_url_change_processor.cc (right):
> 
>
https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
> chrome/browser/sync/glue/typed_url_change_processor.cc:160: if
> (std::find_if(visit_vector.begin(), visit_vector.end(),
> On 2015/05/08 19:38:00, maniscalco wrote:
> > Question about performance.  How big does visit_vector get and how often is
> > CreateOrUpdateSycnNode called?  If the vector is on the order of tens of
> items,
> > then I'm not worried.  Just want to make sure this isn't going to be super
> > expensive.
> 
> Good point, I've checked with an old profile with loads of history and it
seems
> this can get big. I've just seen a 400+ sized vector for http://facebook.com.
> 
> I'm not sure what's the best way to approach this now. The only way to know if
> an URL should be synced is by establishing whether it's been typed. There are
> two tables in the history database that govern this: 'urls' and 'visits'. Each
> entry in 'urls' has a 'typed_count' (which is used widely through typed
history
> sync code) but it's currently a broken heuristic - if I type "facebook.com"
and
> it gets redirected to "https://www.facebook.com", the second address has
> 'typed_count' = 0 even though the corresponding visit in the 'visits' table
has
> type PAGE_TRANSITION_TYPED. This is why I decided to inspect the visits
database
> instead of urls.
> 
> Maybe I should investigate fixing 'typed_count' in the 'urls' table so that it
> is incremented for URLs that are results of server-side redirections to things
> typed into the address bar. Or come up with a fast method of checking the
> 'visits' table within HistoryBackend that doesn't require iterating through
that
> much data (don't know if it's possible).

Looks like it could be up to 1000 items:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

I don't have a sense of how expensive the find_if would be with a typical data
set.  How expensive is CreateOrUpdateSycnNode already?  It looks like we're
already doing a lot of work here (loading the data in HistoryBackend,
std::reverse'ing it, etc.).  If the impact of this CL is small relative to the
existing cost, I'm OK with it.  If not, I think it may be time to explore ways
to restructure the data to make CreateOrUpdateSycnNode faster.

[mpawlowski, 2015-05-12 07:07:52]

On 2015/05/11 16:48:09, maniscalco wrote:
> On 2015/05/11 09:27:18, mpawlowski wrote:
> >
>
https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
> > File chrome/browser/sync/glue/typed_url_change_processor.cc (right):
> > 
> >
>
https://codereview.chromium.org/1126633005/diff/1/chrome/browser/sync/glue/ty...
> > chrome/browser/sync/glue/typed_url_change_processor.cc:160: if
> > (std::find_if(visit_vector.begin(), visit_vector.end(),
> > On 2015/05/08 19:38:00, maniscalco wrote:
> > > Question about performance.  How big does visit_vector get and how often
is
> > > CreateOrUpdateSycnNode called?  If the vector is on the order of tens of
> > items,
> > > then I'm not worried.  Just want to make sure this isn't going to be super
> > > expensive.
> > 
> > Good point, I've checked with an old profile with loads of history and it
> seems
> > this can get big. I've just seen a 400+ sized vector for
http://facebook.com.
> > 
> > I'm not sure what's the best way to approach this now. The only way to know
if
> > an URL should be synced is by establishing whether it's been typed. There
are
> > two tables in the history database that govern this: 'urls' and 'visits'.
Each
> > entry in 'urls' has a 'typed_count' (which is used widely through typed
> history
> > sync code) but it's currently a broken heuristic - if I type "facebook.com"
> and
> > it gets redirected to "https://www.facebook.com", the second address has
> > 'typed_count' = 0 even though the corresponding visit in the 'visits' table
> has
> > type PAGE_TRANSITION_TYPED. This is why I decided to inspect the visits
> database
> > instead of urls.
> > 
> > Maybe I should investigate fixing 'typed_count' in the 'urls' table so that
it
> > is incremented for URLs that are results of server-side redirections to
things
> > typed into the address bar. Or come up with a fast method of checking the
> > 'visits' table within HistoryBackend that doesn't require iterating through
> that
> > much data (don't know if it's possible).
> 
> Looks like it could be up to 1000 items:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
> 
> I don't have a sense of how expensive the find_if would be with a typical data
> set.  How expensive is CreateOrUpdateSycnNode already?  It looks like we're
> already doing a lot of work here (loading the data in HistoryBackend,
> std::reverse'ing it, etc.).  If the impact of this CL is small relative to the
> existing cost, I'm OK with it.  If not, I think it may be time to explore ways
> to restructure the data to make CreateOrUpdateSycnNode faster.

Now that I think of it, the most expensive part is likely the database fetch
that happens anyway. Iterating (find_if is going to be O(N) here) over a vector
of value-held objects and comparing a bitfield will  most likely be
instantaneous compared to this. I'll go ahead and integrate.

[mpawlowski, 2015-05-12 07:09:08]

The CQ bit was checked by mpawlowski@opera.com

[commit-bot: I haz the power, 2015-05-12 07:09:31]

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

[commit-bot: I haz the power, 2015-05-12 07:57:02]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-12 07:57:58]

Patchset 1 (id:??) landed as
https://crrev.com/3be1b71d63be98a42c0e98673ff35fcd4fc738ab
Cr-Commit-Position: refs/heads/master@{#329370}

[pneubeck (no reviews), 2015-05-12 13:16:20]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1127353004/ by pneubeck@chromium.org.

The reason for reverting is: Suspected to cause failure of
MigrationSingleClientTest.AllTypesIndividually
on Linux GN:

https://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/28301.