Changed typed url sync to no longer modify typed/visit_count.

Changed typed url sync to no longer modify typed/visit_count.

The sync code now relies on the underlying history code to keep typed/visit_count
up-to-date. This requires us to sync across the transition type for each visit.

Since we want to set the transition type appropriately for every visit, this CL
also changes AddPagesWithDetails() to no longer automatically create a visit with
the last_visit timestamp if the VisitSource is SYNCED. For convenience, it still
creates a visit for the other sources (browser history import).

BUG=84258
TEST=run sync integration/unit tests, run typed url test cases.

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

[Andrew T Wilson (Slow), 2011-06-09 18:07:12]

Brett, can you review the changes to history (basically, just changed
AddVisits() to let us specify transitions along with the visits).

Nicolas, if you could review the sync portions I'd appreciate it.

[Nicolas Zea, 2011-06-09 19:50:39]

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_change_processor.cc:239:
TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(typed_url, &new_url);
It's kind of confusing that TypedUrlSpecificsToURLRow doesn't actually set all
the fields of the URLRow, and you in fact have to initialize it before hand. I
think I liked the previous method, where it returned a URLRow better. Or perhaps
rename to something else?

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_change_processor.cc:298: }
Comment here about explaining that we never delete?

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
File chrome/browser/sync/glue/typed_url_model_associator.cc (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:185:
typed_url.visit_transitions(c)) <= PageTransition::LAST_CORE);
DCHECK_LT

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:383:
DCHECK(node.visits_size() == node.visit_transitions_size());
DCHECK_EQ

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:429: // added on either
side (left = sync node, right = history db entry).
left/right naming not used anymore.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:548:
DCHECK(typed_url.visits_size() > 0);
DCHECK_GT

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:549:
DCHECK(typed_url.visit_transitions_size() == typed_url.visits_size());
DCHECK_EQ

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
File chrome/browser/sync/glue/typed_url_model_associator.h (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.h:153: // Initializes the
passed |url_row| based on the values in |specifics|.
It no longer fully initializes it right (see previous comment)

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:230:
DCHECK(typed_url.visits_size() == typed_url.visit_transitions_size());
DCHECK_EQ

[Andrew T Wilson (Slow), 2011-06-09 20:21:10]

Scott, looks like Brett's on vacation - do you mind taking a look at the changes
to chrome/browser/history?

[sky, 2011-06-09 20:49:00]

http://codereview.chromium.org/7104088/diff/1/chrome/browser/history/history_...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_backend.h:285: const std::vector<std::pair<
How about some typedefs for these in history_types.

[Andrew T Wilson (Slow), 2011-06-09 22:11:36]

Please take another look.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/history/history_...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_backend.h:285: const std::vector<std::pair<
On 2011/06/09 20:49:01, sky wrote:
> How about some typedefs for these in history_types.

Added VisitInfo.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_change_processor.cc:239:
TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(typed_url, &new_url);
On 2011/06/09 19:50:39, nzea wrote:
> It's kind of confusing that TypedUrlSpecificsToURLRow doesn't actually set all
> the fields of the URLRow, and you in fact have to initialize it before hand. I
> think I liked the previous method, where it returned a URLRow better. Or
perhaps
> rename to something else?

The problem is the old way was fragile - if there are fields in the URLRow that
aren't part of the sync data (like |typed_count| now), we would have no way of
correctly initializing them in the case that we are doing an update (we would
just clobber them with new default values).

I'll change the name to something more descriptive like
UpdateURLRowFromTypedUrlSpecifics().

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_change_processor.cc:298: }
On 2011/06/09 19:50:39, nzea wrote:
> Comment here about explaining that we never delete?

We handle the delete at line 207.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
File chrome/browser/sync/glue/typed_url_model_associator.cc (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:185:
typed_url.visit_transitions(c)) <= PageTransition::LAST_CORE);
On 2011/06/09 19:50:39, nzea wrote:
> DCHECK_LT

Done.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:383:
DCHECK(node.visits_size() == node.visit_transitions_size());
On 2011/06/09 19:50:39, nzea wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:429: // added on either
side (left = sync node, right = history db entry).
On 2011/06/09 19:50:39, nzea wrote:
> left/right naming not used anymore.

Done.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:548:
DCHECK(typed_url.visits_size() > 0);
On 2011/06/09 19:50:39, nzea wrote:
> DCHECK_GT

Done.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.cc:549:
DCHECK(typed_url.visit_transitions_size() == typed_url.visits_size());
On 2011/06/09 19:50:39, nzea wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
File chrome/browser/sync/glue/typed_url_model_associator.h (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/glue/typed_...
chrome/browser/sync/glue/typed_url_model_associator.h:153: // Initializes the
passed |url_row| based on the values in |specifics|.
On 2011/06/09 19:50:39, nzea wrote:
> It no longer fully initializes it right (see previous comment)

Done.

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

http://codereview.chromium.org/7104088/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:230:
DCHECK(typed_url.visits_size() == typed_url.visit_transitions_size());
On 2011/06/09 19:50:39, nzea wrote:
> DCHECK_EQ

Done.

[sky, 2011-06-09 22:31:41]

LGTM

[Nicolas Zea, 2011-06-09 22:37:54]

LGTM