[Sync] Adding revisit logic for typed URLs.

[Sync] Adding revisit logic for typed URLs.

BUG=536841
stanisc@ - sync/revisit changes

rkaplow@ - histogram.xml changes

sky@ - DEPS changes

Committed: https://crrev.com/abe85f4ae69e22c612b14296a48d59fc169b2967
Cr-Commit-Position: refs/heads/master@{#353954}

[skym, 2015-10-13 19:18:25]

skym@chromium.org changed reviewers:
+ rkaplow@chromium.org, sky@chromium.org, stanisc@chromium.org

[rkaplow, 2015-10-13 21:02:20]

https://codereview.chromium.org/1399053004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1399053004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:45663: +    The client side execution
time to check for revisists with typed URL data.
revisits?

https://codereview.chromium.org/1399053004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:45671: +    typed on a foriegn client.
foreign

[rkaplow, 2015-10-13 21:02:32]

lgtm

lg otherwise

[skym, 2015-10-13 21:06:52]

Update to fix spelling errors.

https://codereview.chromium.org/1399053004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1399053004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:45663: +    The client side execution
time to check for revisists with typed URL data.
On 2015/10/13 21:02:20, rkaplow wrote:
> revisits?

Done.

https://codereview.chromium.org/1399053004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:45671: +    typed on a foriegn client.
On 2015/10/13 21:02:20, rkaplow wrote:
> foreign

Done.

[stanisc, 2015-10-13 21:53:33]

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
File components/sync_sessions/revisit/typed_url_page_revisit_observer.cc
(right):

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_observer.cc:26:
&task_tracker_);
Is that OK to reuse task_tracker_? What if the previous task hasn't finished?
Could it be cancelled as a result?

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
File components/sync_sessions/revisit/typed_url_page_revisit_observer.h (right):

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_observer.h:20: class
HistoryService;
Second forward declaration for HistoryService. See above.

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_observer.h:25: //
creates and owns a weak pointer to the HistoryService.
Nit: it is OK to own a weak pointer to another object on a different thread.
What matters is that dereferencing the weak pointer should be done on the same
thread where it was created.
May be replace "owns" with "uses".

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
File components/sync_sessions/revisit/typed_url_page_revisit_task.cc (right):

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_task.cc:23:
history::HistoryBackend* backend,
Should HistoryBackend* be a const pointer?

[skym, 2015-10-13 22:25:15]

Updated for and responded to Stan's comments.

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
File components/sync_sessions/revisit/typed_url_page_revisit_observer.cc
(right):

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_observer.cc:26:
&task_tracker_);
On 2015/10/13 21:53:33, stanisc wrote:
> Is that OK to reuse task_tracker_? What if the previous task hasn't finished?
> Could it be cancelled as a result?

Great question. It looks like it is okay, it seems that the
CancelableTaskTracker object is essentially a map of ids to flags, see
https://code.google.com/p/chromium/codesearch#chromium/src/base/task/cancelab...
, so having multiple outstanding tasks is pretty much its job.

You can see in this code path history logic requests an id from the tracker at
https://code.google.com/p/chromium/codesearch#chromium/src/components/history...
which is basically just ++ing an int64
https://code.google.com/p/chromium/codesearch#chromium/src/base/task/cancelab...
, so each concurrently running task will have a unique task id (assuming that we
spawn tasks on the same thread, which we do, UI thread).

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
File components/sync_sessions/revisit/typed_url_page_revisit_observer.h (right):

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_observer.h:20: class
HistoryService;
On 2015/10/13 21:53:33, stanisc wrote:
> Second forward declaration for HistoryService. See above.

Done.

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_observer.h:25: //
creates and owns a weak pointer to the HistoryService.
On 2015/10/13 21:53:33, stanisc wrote:
> Nit: it is OK to own a weak pointer to another object on a different thread.
> What matters is that dereferencing the weak pointer should be done on the same
> thread where it was created.
> May be replace "owns" with "uses".

Done.

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
File components/sync_sessions/revisit/typed_url_page_revisit_task.cc (right):

https://codereview.chromium.org/1399053004/diff/1/components/sync_sessions/re...
components/sync_sessions/revisit/typed_url_page_revisit_task.cc:23:
history::HistoryBackend* backend,
On 2015/10/13 21:53:33, stanisc wrote:
> Should HistoryBackend* be a const pointer?

We could do "history::HistoryBackend* const backend" I think, but I'm assuming
you're asking about "const history::HistoryBackend* backend", which we cannot
do. Pretty much all the methods on HistoryBackend are non-const, which seems to
be because all the backing DB implementation functions are also non-const.

[stanisc, 2015-10-13 22:32:47]

lgtm

[sky, 2015-10-14 02:39:36]

LGTM

[skym, 2015-10-14 02:40:48]

The CQ bit was checked by skym@chromium.org

[skym, 2015-10-14 02:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1399053004/#ps40001
(title: "Updated for Stan's comments.")

[commit-bot: I haz the power, 2015-10-14 02:41:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399053004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399053004/40001

[commit-bot: I haz the power, 2015-10-14 03:52:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-14 03:53:49]

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