[Sync] Add interface and backend impl for typed URL syncable service

[Sync] Add interface and backend impl for typed URL syncable service

The typed URL sync datatype is being transitioned to the syncable service API,
and this patch introduces the new interface the typed url sync code will use
to communicate with sync as well as the HistoryBackend which implements the
datatype.

This patch implements logic for syncing typed url changes originating from
the history backend to the sync server, through the syncable service API.

BUG=77819

[mgist1, 2013-03-29 08:11:02]

PTAL. This is the first patch for implementing typed urls with the syncable
service api, and should include all of the changes to HistoryBackend required
for this transition.

+atwilson: please review chrome/browser/history/typed_url*
+brettw: please review chrome/browser/history/history*

[mgist1, 2013-04-06 01:40:11]

Added some unit testing for the current typed url processing of changes from
backend. PTAL.

[Andrew T Wilson (Slow), 2013-04-09 12:48:30]

A few nits/questions, but overall it looks good. I like the new tests!

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:895: // Notify typed url sync
nit: use sentence punctuation.

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:896: if
(typed_url_syncable_service_.get()) {
nit: omit braces on single-line if statements, because that seems to be the
consistent standard in this file.

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:990: // Notify typed url sync
Nit: use sentence punctuation (here and below).

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:2889: if (type ==
chrome::NOTIFICATION_HISTORY_URLS_DELETED &&
It seems weird that we're doing deletions via BroadcastNotifications, but
adds/modifies. Why is this? Why not put all of the logic for notifying
typed_url_syncable_service in this function (or else have ExpireHistoryBackend
notify typed_url_syncable_service directly)?

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
File chrome/browser/history/history_service.cc (right):

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
chrome/browser/history/history_service.cc:366: return
history_backend_->GetTypedUrlSyncableService();
HistoryBackend and TypedUrlSyncableService are run on the DB thread. Why is it
OK to handle references here on the UI thread?

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
File chrome/browser/history/typed_url_syncable_service.cc (right):

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service.cc:288:
syncer::SyncChange::ACTION_ADD;
Is there a better way to differentiate between adds and updates that doesn't
require keeping an in-memory copy of all urls?

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service.cc:308:
typed_url->set_url(row.url().spec());
Why do we special-case ACTION_DELETE here?

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
File chrome/browser/history/typed_url_syncable_service.h (right):

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service.h:38: class
TypedUrlSyncableService : public syncer::SyncableService {
Should this be inside the history namespace?

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service.h:121: std::set<GURL>
synced_typed_urls_;
So we keep an in-memory copy of all currently synced typed URLs now? Should we
be concerned about the memory overhead?

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
File chrome/browser/history/typed_url_syncable_service_unittest.cc (right):

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service_unittest.cc:160: virtual void
SetUp() {
add OVERRIDE?

https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service_unittest.cc:268: // fill the
URL details to send to the sync service
nit: sentence punctuation for all comments.

[mgist1, 2013-04-10 01:28:38]

On 2013/04/09 12:48:30, Andrew T Wilson wrote:
> A few nits/questions, but overall it looks good. I like the new tests!
> 
>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> File chrome/browser/history/history_backend.cc (right):
> 
>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> chrome/browser/history/history_backend.cc:895: // Notify typed url sync
> nit: use sentence punctuation.
> 

Done. (This code was moved. See below.)

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> chrome/browser/history/history_backend.cc:896: if
> (typed_url_syncable_service_.get()) {
> nit: omit braces on single-line if statements, because that seems to be the
> consistent standard in this file.
> 

Done. (This code was moved. See below.)

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> chrome/browser/history/history_backend.cc:990: // Notify typed url sync
> Nit: use sentence punctuation (here and below).
> 

Done. (This code was moved. See below.)

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> chrome/browser/history/history_backend.cc:2889: if (type ==
> chrome::NOTIFICATION_HISTORY_URLS_DELETED &&
> It seems weird that we're doing deletions via BroadcastNotifications, but
> adds/modifies. Why is this? Why not put all of the logic for notifying
> typed_url_syncable_service in this function (or else have ExpireHistoryBackend
> notify typed_url_syncable_service directly)?
> 

The separation was done to avoid adding extra complexity to access
typed_url_sycable_service_ from ExpireHistoryBackend, and reducing type checking
and casting. For improved code clarity, the
modified and visited handling calls have been moved into BroadcastNotifications.

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> File chrome/browser/history/history_service.cc (right):
> 
>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> chrome/browser/history/history_service.cc:366: return
> history_backend_->GetTypedUrlSyncableService();
> HistoryBackend and TypedUrlSyncableService are run on the DB thread. Why is it
> OK to handle references here on the UI thread?
> 

My intent was to have this function only called during the
ProfileSyncComponentsFactoryImpl::GetSyncableServiceForType call that is made
during execution of the
NonUIDataTypeController::StartAssociationWithSharedChangeProcessor task that is
posted to the backend (by PostTaskOnBackendThread), in order to return the
service to the controller. Is there still threading danger in this case? Please
let me know if you have suggestions for making this safer.

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> File chrome/browser/history/typed_url_syncable_service.cc (right):
> 
>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> chrome/browser/history/typed_url_syncable_service.cc:288:
> syncer::SyncChange::ACTION_ADD;
> Is there a better way to differentiate between adds and updates that doesn't
> require keeping an in-memory copy of all urls?
> 

While it is possible, for instance, to identify individual cases of local data
changes, and make handling calls or set flags from each point in the code where
a particular change is identified, this does not completely remove the need for
an in-memory cache (see notes for typed_url_syncable_service.h:121 below).

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> chrome/browser/history/typed_url_syncable_service.cc:308:
> typed_url->set_url(row.url().spec());
> Why do we special-case ACTION_DELETE here?
> 

Deletes do not require visit information, which may not even be available from
the backend at this point, so this case is to avoid visit parsing logic when
building the specifics.

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> File chrome/browser/history/typed_url_syncable_service.h (right):
> 
>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> chrome/browser/history/typed_url_syncable_service.h:38: class
> TypedUrlSyncableService : public syncer::SyncableService {
> Should this be inside the history namespace?
> 

Done.

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> chrome/browser/history/typed_url_syncable_service.h:121: std::set<GURL>
> synced_typed_urls_;
> So we keep an in-memory copy of all currently synced typed URLs now? Should we
> be concerned about the memory overhead?
> 

When deleting urls from history backend, they may be synced as typed urls but
have a current typed_count of 0, due to visit expiry, etc. Additionally,
deleting all_history==true means that no url information is given, and only/all
the urls that the server has should be deleted. This means that knowing the set
of server urls independently from local urls is required, to send the
appropriate SyncChangeList.

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> File chrome/browser/history/typed_url_syncable_service_unittest.cc (right):
> 
>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> chrome/browser/history/typed_url_syncable_service_unittest.cc:160: virtual
void
> SetUp() {
> add OVERRIDE?
> 

Done.

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> chrome/browser/history/typed_url_syncable_service_unittest.cc:268: // fill the
> URL details to send to the sync service
> nit: sentence punctuation for all comments.

Done.

[Andrew T Wilson (Slow), 2013-04-10 08:33:50]

On 2013/04/10 01:28:38, mgist1 wrote:
> My intent was to have this function only called during the
> ProfileSyncComponentsFactoryImpl::GetSyncableServiceForType call that is made
> during execution of the
> NonUIDataTypeController::StartAssociationWithSharedChangeProcessor task that
is
> posted to the backend (by PostTaskOnBackendThread), in order to return the
> service to the controller. Is there still threading danger in this case?
Please
> let me know if you have suggestions for making this safer.

Perhaps add a comment to this effect in the header file definition for this API.

>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/ty...
> > chrome/browser/history/typed_url_syncable_service.h:121: std::set<GURL>
> > synced_typed_urls_;
> > So we keep an in-memory copy of all currently synced typed URLs now? Should
we
> > be concerned about the memory overhead?
> > 
> 
> When deleting urls from history backend, they may be synced as typed urls but
> have a current typed_count of 0, due to visit expiry, etc. Additionally,
> deleting all_history==true means that no url information is given, and
only/all
> the urls that the server has should be deleted. This means that knowing the
set
> of server urls independently from local urls is required, to send the
> appropriate SyncChangeList.

Hmmm. The old typed_url_change_processor.cc was able to walk all the typedurl
nodes in the sync DB and delete them in this case - there's no need to track the
URLs separately. Is this not possible with the new SyncableService API? Perhaps
zea@ may have a suggestion here?

[Andrew T Wilson (Slow), 2013-04-10 08:43:22]

LGTM, but strongly consider finding a way to not keep an in-memory cache of all
typed URLs. It should be possible to do this (maybe with some special-casing of
"delete all") since the old sync code never required this.

https://codereview.chromium.org/12703036/diff/47001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/12703036/diff/47001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:894: 
nit: remove extraneous blank line

https://codereview.chromium.org/12703036/diff/47001/chrome/browser/history/hi...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/12703036/diff/47001/chrome/browser/history/hi...
chrome/browser/history/history_service.h:163: history::TypedUrlSyncableService*
GetTypedUrlSyncableService() const;
nit: sentence punctuation, and also add comment around how this should be used
(since this object is generally intended to be accessed only via the DB thread)

[mgist1, 2013-04-15 23:28:19]

On 2013/04/10 08:43:22, Andrew T Wilson wrote:
> LGTM, but strongly consider finding a way to not keep an in-memory cache of
all
> typed URLs. It should be possible to do this (maybe with some special-casing
of
> "delete all") since the old sync code never required this.
> 

Added a TODO to investigate replacing caching with additional datatype api
logic.

>
https://codereview.chromium.org/12703036/diff/47001/chrome/browser/history/hi...
> chrome/browser/history/history_backend.cc:894: 
> nit: remove extraneous blank line
> 

Done.

>
https://codereview.chromium.org/12703036/diff/47001/chrome/browser/history/hi...
> chrome/browser/history/history_service.h:163:
history::TypedUrlSyncableService*
> GetTypedUrlSyncableService() const;
> nit: sentence punctuation, and also add comment around how this should be used
> (since this object is generally intended to be accessed only via the DB
thread)

Done.

[mgist1, 2013-04-20 00:17:47]

Ping brettw. PTAL

[brettw, 2013-04-20 05:14:18]

I didn't look at the actual syncable service yet, will follow-up

https://codereview.chromium.org/12703036/diff/57001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/12703036/diff/57001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:2875: case
chrome::NOTIFICATION_HISTORY_URLS_DELETED: {
I don't think it makes sense to key off of the notification inside the history
system itself on the same thread. We're trying to delete the notification
service, and using this exceedingly weak coupling inside of one logical
subsystem unnecessarily declarifies things.

Maybe one thing that would be better would be to add these functions on the
delegate interface with the necessary parameters, and have the backend create
the details and issue the notification.

https://codereview.chromium.org/12703036/diff/57001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/12703036/diff/57001/chrome/browser/history/hi...
chrome/browser/history/history_backend.h:903: // Used to sync typed urls
This comment seems redundant with the variable type. Maybe replace it with a
quick sentence about whether/when this can be null?

[mgist1, 2013-04-22 20:47:05]

https://codereview.chromium.org/12703036/diff/57001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/12703036/diff/57001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:2875: case
chrome::NOTIFICATION_HISTORY_URLS_DELETED: {
On 2013/04/20 05:14:18, brettw wrote:
> Maybe one thing that would be better would be to add these functions on the
> delegate interface with the necessary parameters, and have the backend create
> the details and issue the notification.

Just to clarify, would it be better to remove this code completely from
BroadcastNotifications and instead call the necessary handler from within the
code that calls BroadcastNotifications (the way the code was before Patch Set
7), and in addition have a wrapper function within
BroadcastNotificationDelegate, implemented by HistoryBackend, to take details
from ExpireHistoryBackend and call OnUrlsDeleted?

[brettw, 2013-04-22 23:39:28]

I think this should be removed from BroadcastNotifications, yeah.

And I wasn't excited about adding dependency from the expiration backend to your
typed thing. So that implies there's a delegate function that takes a list, and
then the history backend implementation of that calls you and issues the
notification. I think the details should be created only for the notification,
your code shouldn't take a notification details (since the're not particularly
typesafe and prone to error).

[Andrew T Wilson (Slow), 2013-04-23 07:56:16]

I defer to brett about the proper way to notify TypedUrlService. My comment
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
basically was complaining about doing some notifications via
BroadcastNotifications and some via direct calls. I'm OK with doing them all via
direct calls if that's brett's preference - I just wanted them all to be
consistent one way or the other.

[mgist1, 2013-04-23 23:20:38]

On 2013/04/23 07:56:16, Andrew T Wilson wrote:
> I defer to brett about the proper way to notify TypedUrlService. My comment
>
https://codereview.chromium.org/12703036/diff/37007/chrome/browser/history/hi...
> basically was complaining about doing some notifications via
> BroadcastNotifications and some via direct calls. I'm OK with doing them all
via
> direct calls if that's brett's preference - I just wanted them all to be
> consistent one way or the other.

I agree. As per brettw's comments, this has been refactored to remove
dependency. This current patch set also includes some small style cleanup and
has been rebased since the previous patch set, just fyi. PTAL.

atwilson please review updates to the code changes in c/b/h/typed_url*
brettw please review updates to the code in c/b/h/history* and c/b/h/expire*

Thanks

[brettw, 2013-04-30 22:15:33]

LGTM on the integration. I didn't check the details of the new code, which looks
tricky, so hopefully Andrew did a really good job :)

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:891: if
(typed_url_syncable_service_.get()) {
No {} for single-line conditionals (match surrounding code).

Same below.

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:2891:
typed_url_syncable_service_->OnUrlsDeleted(all_history,
Doesn't this fit on one line?

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/ty...
File chrome/browser/history/typed_url_syncable_service.cc (right):

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service.cc:37: }
Nit: add "  // namespace" to the end here.

[Nicolas Zea, 2013-05-06 19:22:42]

Landing this for Michelle (whose internship has ended) via
https://codereview.chromium.org/12703036/.

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:891: if
(typed_url_syncable_service_.get()) {
On 2013/04/30 22:15:34, brettw wrote:
> No {} for single-line conditionals (match surrounding code).
> 
> Same below.

Done.

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:2891:
typed_url_syncable_service_->OnUrlsDeleted(all_history,
On 2013/04/30 22:15:34, brettw wrote:
> Doesn't this fit on one line?

Done.

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/ty...
File chrome/browser/history/typed_url_syncable_service.cc (right):

https://codereview.chromium.org/12703036/diff/71001/chrome/browser/history/ty...
chrome/browser/history/typed_url_syncable_service.cc:37: }
On 2013/04/30 22:15:34, brettw wrote:
> Nit: add "  // namespace" to the end here.

Done.