Convert visitedlink to use mojo.

Convert visitedlink to use mojo.

BUG=577685
Committed: https://crrev.com/aa22276dd0feae03cfc4eaa11f1ddf432dc87b11
Cr-Commit-Position: refs/heads/master@{#419998}

[Sam McNally, 2016-06-07 01:46:28]

Description was changed from

==========
Converting visitedlink to use mojo.

BUG=577685
==========

to

==========
Convert visitedlink to use mojo.

BUG=577685
==========

[Sam McNally, 2016-06-07 01:47:06]

Description was changed from

==========
Convert visitedlink to use mojo.

BUG=577685
==========

to

==========
Convert visitedlink to use mojo.

BUG=577685
==========

[Sam McNally, 2016-06-08 10:32:09]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-06-08 10:32:30]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-06-08 10:32:32]

Patchset #1 (id:30001) has been deleted

[Sam McNally, 2016-06-08 10:32:34]

Patchset #1 (id:50001) has been deleted

[Sam McNally, 2016-06-08 10:37:01]

Patchset #2 (id:80001) has been deleted

[Sam McNally, 2016-06-08 10:37:36]

Patchset #1 (id:60026) has been deleted

[Sam McNally, 2016-06-08 11:05:11]

Patchset #2 (id:120001) has been deleted

[Sam McNally, 2016-06-08 11:29:26]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2016-06-08 12:08:33]

sammc@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-06-08 16:32:21]

sky@chromium.org changed reviewers:
+ brettw@chromium.org
- sky@chromium.org

[sky, 2016-06-08 16:33:09]

sky->brettw

[Sam McNally, 2016-09-13 08:23:20]

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

[commit-bot: I haz the power, 2016-09-13 08:23:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 08:44:44]

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

[commit-bot: I haz the power, 2016-09-13 08:44:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Sam McNally, 2016-09-13 09:49:31]

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

[commit-bot: I haz the power, 2016-09-13 09:49:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 11:00:48]

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

[commit-bot: I haz the power, 2016-09-13 11:00:49]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-14 01:07:21]

Patchset #3 (id:180001) has been deleted

[Sam McNally, 2016-09-14 01:42:21]

ping

[brettw, 2016-09-14 22:44:32]

Sorry. Next time please ping me sooner than 3 months! I'll look at this soon.

[brettw, 2016-09-14 23:02:13]

Didn't finish everything, will do the rest tomorrow.

https://codereview.chromium.org/2048503002/diff/200001/components/visitedlink...
File components/visitedlink/browser/visitedlink_master.cc (right):

https://codereview.chromium.org/2048503002/diff/200001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.cc:302: if
(shared_memory_.is_valid())
I don't understand this addition, can you describe what's changed to now require
this?

https://codereview.chromium.org/2048503002/diff/200001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.cc:918:
mojo::ScopedSharedBufferMapping hash_table_mapping =
I kind of preferred the old way. Unmapping and remapping the entire table are
heavyweight VM operations just to move a pointer that the old code already
handled. Are there benefits of doing it this way?

[Sam McNally, 2016-09-15 03:38:40]

Patchset #4 (id:220001) has been deleted

[Sam McNally, 2016-09-15 04:12:16]

https://codereview.chromium.org/2048503002/diff/200001/components/visitedlink...
File components/visitedlink/browser/visitedlink_master.cc (right):

https://codereview.chromium.org/2048503002/diff/200001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.cc:302: if
(shared_memory_.is_valid())
On 2016/09/14 23:02:13, brettw (ping on IM after 24h) wrote:
> I don't understand this addition, can you describe what's changed to now
require
> this?

Previously, VisitedLinkEventListener received a pointer to its
VisitedLinkMaster, which it only used to get the shared memory from the master
(to send to newly launched renderer processes).

I wanted to break this dependency so I could inject a VisitedLinkEventListener
with a mock Timer in tests. Now, the listener holds a handle to the shared
memory which it replaces on each NewTable() notification. That maintains the
existing behaviour, except for before the first NewTable() notification. To fix
this discrepancy, this pushes the initial shared memory to the listener once
it's ready.

https://codereview.chromium.org/2048503002/diff/200001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.cc:918:
mojo::ScopedSharedBufferMapping hash_table_mapping =
On 2016/09/14 23:02:13, brettw (ping on IM after 24h) wrote:
> I kind of preferred the old way. Unmapping and remapping the entire table are
> heavyweight VM operations just to move a pointer that the old code already
> handled. Are there benefits of doing it this way?

I think it makes the code a bit simpler, but adding a helper to convert from the
mapping to the hash table seems fine (and moves the casts to a single place,
which is nice).

[brettw, 2016-09-15 22:33:08]

lgtm

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/browser/visitedlink_event_listener.h (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/browser/visitedlink_event_listener.h:32: base::Timer*
coalesce_timer_override);
Above here we should document coalesce_timer_override and it's ability to be
null.

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/browser/visitedlink_event_listener.h:47:
base::OneShotTimer coalesce_timer_owner_;
Can you put a comment here on this, especially the pointer (since somebody may
wonder about ownership of this value).

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/browser/visitedlink_master.h (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.h:76:
VisitedLinkMaster(content::BrowserContext* browser_context,
Can this document what happens if the listener is null and why one might need to
supply one?

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/renderer/visitedlink_slave.cc (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/renderer/visitedlink_slave.cc:39: // map the header into
our process so we can see how long the rest is,
Check that the new comments you added (also below) follow the guide of complete
sentences (looks like the old ones are wrong (probably my bad), but at least the
new ones can be improved).

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/test/BUILD.gn (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/test/BUILD.gn:17:
"//components/visitedlink/common:interfaces",
You shouldn't need this since interfaces is already a public dep of common.

[Sam McNally, 2016-09-19 00:22:07]

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

[commit-bot: I haz the power, 2016-09-19 00:22:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-09-19 00:28:17]

sammc@chromium.org changed reviewers:
+ dcheng@chromium.org

[Sam McNally, 2016-09-19 00:28:19]

+dcheng for the mojom

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/browser/visitedlink_event_listener.h (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/browser/visitedlink_event_listener.h:32: base::Timer*
coalesce_timer_override);
On 2016/09/15 22:33:08, brettw (ping on IM after 24h) wrote:
> Above here we should document coalesce_timer_override and it's ability to be
> null.

Done.

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/browser/visitedlink_event_listener.h:47:
base::OneShotTimer coalesce_timer_owner_;
On 2016/09/15 22:33:08, brettw (ping on IM after 24h) wrote:
> Can you put a comment here on this, especially the pointer (since somebody may
> wonder about ownership of this value).

Done.

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/browser/visitedlink_master.h (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.h:76:
VisitedLinkMaster(content::BrowserContext* browser_context,
On 2016/09/15 22:33:08, brettw (ping on IM after 24h) wrote:
> Can this document what happens if the listener is null and why one might need
to
> supply one?

Done.

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/renderer/visitedlink_slave.cc (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/renderer/visitedlink_slave.cc:39: // map the header into
our process so we can see how long the rest is,
On 2016/09/15 22:33:08, brettw (ping on IM after 24h) wrote:
> Check that the new comments you added (also below) follow the guide of
complete
> sentences (looks like the old ones are wrong (probably my bad), but at least
the
> new ones can be improved).

Done.

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
File components/visitedlink/test/BUILD.gn (right):

https://codereview.chromium.org/2048503002/diff/260001/components/visitedlink...
components/visitedlink/test/BUILD.gn:17:
"//components/visitedlink/common:interfaces",
On 2016/09/15 22:33:08, brettw (ping on IM after 24h) wrote:
> You shouldn't need this since interfaces is already a public dep of common.

Done.

[commit-bot: I haz the power, 2016-09-19 01:23:07]

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

[commit-bot: I haz the power, 2016-09-19 01:23:08]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-20 22:29:27]

https://codereview.chromium.org/2048503002/diff/300001/android_webview/render...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/2048503002/diff/300001/android_webview/render...
android_webview/renderer/aw_content_renderer_client.cc:77:
base::Unretained(visited_link_slave_.get())));
I am still uncomfortable that Mojo is encouraging wide use of base::Unretained()
in situations like this. In RenderThread or RenderFrame, it's somewhat easy to
verify that lifetimes are correct. But once impls start living in things that
aren't as tightly coupled, then the picture becomes significantly less clear: is
it possible for the InterfaceRegistry to outlive |this|?

We had several use-after-frees bugs last year due to base::Unretained() use that
"should have been safe" but weren't. Is there any possibility to just default to
refcounting impls that are reused? Alternatively, even using a WeakPtr would be
better. What would be the downside?

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
File components/visitedlink/browser/visitedlink_event_listener.cc (right):

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
components/visitedlink/browser/visitedlink_event_listener.cc:164: FROM_HERE,
TimeDelta::FromMilliseconds(kCommitIntervalMs),
Not added by this CL, but let's make kCommitIntervalMs a constexpr

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
File components/visitedlink/browser/visitedlink_master.h (right):

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.h:81:
std::unique_ptr<Listener> listener = nullptr);
Nit: I think it's slightly better to just have SetListenerForTest(). That way,
we minimize the amount of test-only code in the Chrome binary. It means we
needlessly allocate an object in a test, but if we immediately replace it, that
should be fine, right?

(Similar thoughts about the timer override in VisitedLinkUpdater)

[Sam McNally, 2016-09-21 01:46:39]

Patchset #8 (id:320001) has been deleted

[Sam McNally, 2016-09-21 01:49:15]

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

[commit-bot: I haz the power, 2016-09-21 01:49:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-09-21 02:38:15]

https://codereview.chromium.org/2048503002/diff/300001/android_webview/render...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/2048503002/diff/300001/android_webview/render...
android_webview/renderer/aw_content_renderer_client.cc:77:
base::Unretained(visited_link_slave_.get())));
On 2016/09/20 22:29:27, dcheng wrote:
> I am still uncomfortable that Mojo is encouraging wide use of
base::Unretained()
> in situations like this. In RenderThread or RenderFrame, it's somewhat easy to
> verify that lifetimes are correct. But once impls start living in things that
> aren't as tightly coupled, then the picture becomes significantly less clear:
is
> it possible for the InterfaceRegistry to outlive |this|?
> 
> We had several use-after-frees bugs last year due to base::Unretained() use
that
> "should have been safe" but weren't. Is there any possibility to just default
to
> refcounting impls that are reused? Alternatively, even using a WeakPtr would
be
> better. What would be the downside?

Done.

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
File components/visitedlink/browser/visitedlink_event_listener.cc (right):

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
components/visitedlink/browser/visitedlink_event_listener.cc:164: FROM_HERE,
TimeDelta::FromMilliseconds(kCommitIntervalMs),
On 2016/09/20 22:29:27, dcheng wrote:
> Not added by this CL, but let's make kCommitIntervalMs a constexpr

Done.

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
File components/visitedlink/browser/visitedlink_master.h (right):

https://codereview.chromium.org/2048503002/diff/300001/components/visitedlink...
components/visitedlink/browser/visitedlink_master.h:81:
std::unique_ptr<Listener> listener = nullptr);
On 2016/09/20 22:29:27, dcheng wrote:
> Nit: I think it's slightly better to just have SetListenerForTest(). That way,
> we minimize the amount of test-only code in the Chrome binary. It means we
> needlessly allocate an object in a test, but if we immediately replace it,
that
> should be fine, right?
> 
> (Similar thoughts about the timer override in VisitedLinkUpdater)

Done.

[commit-bot: I haz the power, 2016-09-21 02:47:17]

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

[commit-bot: I haz the power, 2016-09-21 02:47:18]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-21 04:24:24]

mojo lgtm

[Sam McNally, 2016-09-21 05:04:25]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-09-21 05:04:26]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2048503002/#ps360001
(title: "clang format")

[commit-bot: I haz the power, 2016-09-21 05:04:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 07:31:04]

Description was changed from

==========
Convert visitedlink to use mojo.

BUG=577685
==========

to

==========
Convert visitedlink to use mojo.

BUG=577685
==========

[commit-bot: I haz the power, 2016-09-21 07:31:06]

Committed patchset #9 (id:360001)

[commit-bot: I haz the power, 2016-09-21 07:34:16]

Description was changed from

==========
Convert visitedlink to use mojo.

BUG=577685
==========

to

==========
Convert visitedlink to use mojo.

BUG=577685

Committed: https://crrev.com/aa22276dd0feae03cfc4eaa11f1ddf432dc87b11
Cr-Commit-Position: refs/heads/master@{#419998}
==========

[commit-bot: I haz the power, 2016-09-21 07:34:18]

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