Remove VisitedLink dependency on rest of chrome

Remove VisitedLink dependency on rest of chrome

This is in preparation of componentizing VisitedLink implementation and
be shared with Android WebView.

* Added VisitedLinkDelegate which HistoryService implements.
* Removed VisitedLinkMasterFactory and have HistoryService directly own
  VisitedLinkMaster.
* Introduce URLIterator interface for DeleteURLs.

Last committed in https://src.chromium.org/viewvc/chrome?view=rev&revision=175186

Reverted in https://src.chromium.org/viewvc/chrome?view=rev&revision=175217
due to memory leaks.

Before this patch, VisitedLink was a ProfileKeyedService and some using
HistoryService did not create VisitedLink at all. After this patch,
VisitedLink is created and owned directly by HistoryService, so they are
now created for some tests depending on which HistoryService constructor
is used.

First version was leaking URLEnumerator in HistoryService::RebuildTable
which was doing manual ref counting (AddRef/Release) calls. Some tests do
not start the background thread at all, so URLEnumerator::OnComplete is
never called to delete itself.

Fixed by properly ref-counting URLEnumerator with scoped_refptr in this
path.

BUG=168716
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175906

[boliu, 2012-12-18 07:53:34]

FYI for now. First bullet point in mail.

[Jói, 2012-12-18 10:03:10]

Cool, one question for now.

https://codereview.chromium.org/11573060/diff/1/chrome/browser/visitedlink/vi...
File chrome/browser/visitedlink/visitedlink_event_listener.cc (right):

https://codereview.chromium.org/11573060/diff/1/chrome/browser/visitedlink/vi...
chrome/browser/visitedlink/visitedlink_event_listener.cc:188: case
content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: {
I think you are no longer signed up for this; did you mean to also move it to
history.cc?

[boliu, 2012-12-18 16:42:54]

I only compiled this and did not run/test it. And clearly from the massive
trybot failures, there is probably an error on start up.

Also as Joi suggested in mail, maybe a delegate interface is better/safer.

https://codereview.chromium.org/11573060/diff/1/chrome/browser/visitedlink/vi...
File chrome/browser/visitedlink/visitedlink_event_listener.cc (right):

https://codereview.chromium.org/11573060/diff/1/chrome/browser/visitedlink/vi...
chrome/browser/visitedlink/visitedlink_event_listener.cc:188: case
content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: {
On 2012/12/18 10:03:10, Jói wrote:
> I think you are no longer signed up for this; did you mean to also move it to
> history.cc?

Registered for this in RenderProcessCreated, with a specific source of that
particular RPH.

[boliu, 2012-12-19 01:53:58]

Expanding scope of this. Still not for commit yet and only uploaded for early
review.

I decided to have HistoryService directly own VisitedLinkMaster despite the
warning in the DEPs file, partly because this is an easier refactor and partly
because this ownership model is much simpler than using singleton factories. It
looks like VisitedLink will be componentized first, at which point those
includes won't seem so bad. No idea about impact on start up time or other
potential issues though, desktop linux chrome seems to still work fine.

unit_tests needs a lot more work (it doesn't compile in Patch Set 2). Looks like
it's much more deeply integrated with Profile and HistoryService (which kind of
defeats the purpose of 'unit' tests). Probably will end up splitting them up,
some going into history service which now owns VisitedLink, some using the new
delegate.

Please ignore the general lack of documentation for now.

[Jói, 2012-12-19 14:07:20]

Looks great, I think this is very much in the right direction.

I agree ignoring the DEPS warning and making the VisitedLinkMaster looks like a
good way to go.  The DEPS warning is likely a hold-over from when we were trying
to componentize HistoryService before componentizing its dependencies, and while
it's good to try to reduce the number of new "bad" dependencies that creep in,
this one seems fine since VisitedLinkMaster will become a component before
History.

Cheers,
Jói

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
chrome/browser/history/history.h:655: virtual bool
IsEquivalentContext(content::BrowserContext* context1,
nit: I know I suggested this name, but now that I see it, it's more like a
binary operator so perhaps should call it AreEquivalentContexts?

[sky, 2012-12-19 16:26:33]

sky->brettw

[joth, 2012-12-20 01:39:29]

Liking the look of this too. some passing comments - let us know when you want
another look.

Thanks

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
File chrome/browser/history/history.cc (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
chrome/browser/history/history.cc:917: visitedlink_master_->DeleteAllURLs();
nit: include { } when there's an else part.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_delegate.h:30: virtual void
RebuildTable(URLEnumerator* enumerator) = 0;
virtual d'tor, DISALLOW_COPY etc.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_master.cc:245: if (browser_context_ &&
browser_context_->IsOffTheRecord()) {
note BrowserContext has a TODO to remove IsOffTheRecord(). I don't think you can
do much about that here though.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_master.cc:948: if (!url.is_empty()) {
if (url.is_valid()) ?

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_master.h:339: VisitedLinkDelegate*
delegate_;
// weak.

[tfarina, 2012-12-20 01:43:33]

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_delegate.h:8: #include <vector>
you don't use this include in this header file.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_delegate.h:26: protected:
add a blank line above.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_delegate.h:30: virtual void
RebuildTable(URLEnumerator* enumerator) = 0;
On 2012/12/20 01:39:29, joth wrote:
> virtual d'tor, DISALLOW_COPY etc.
you don't need this subtle pattern of DISALLOW_COPY in an abstract class (aka
interface). Just let the subclass do this. But yeah, a protected virtual
destructor is missing though.

[boliu, 2012-12-29 01:48:12]

Ready for review!

chrome_frame_tests are still failing, but other than that, everything is ready.

Happy holidays everyone :)

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
File chrome/browser/history/history.cc (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
chrome/browser/history/history.cc:917: visitedlink_master_->DeleteAllURLs();
On 2012/12/20 01:39:29, joth wrote:
> nit: include { } when there's an else part.

Done.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/history/his...
chrome/browser/history/history.h:655: virtual bool
IsEquivalentContext(content::BrowserContext* context1,
On 2012/12/19 14:07:20, Jói wrote:
> nit: I know I suggested this name, but now that I see it, it's more like a
> binary operator so perhaps should call it AreEquivalentContexts?

Done.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_delegate.h:8: #include <vector>
On 2012/12/20 01:43:34, tfarina wrote:
> you don't use this include in this header file.

Done.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_delegate.h:26: protected:
On 2012/12/20 01:43:34, tfarina wrote:
> add a blank line above.

Done.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_delegate.h:30: virtual void
RebuildTable(URLEnumerator* enumerator) = 0;
On 2012/12/20 01:43:34, tfarina wrote:
> On 2012/12/20 01:39:29, joth wrote:
> > virtual d'tor, DISALLOW_COPY etc.
> you don't need this subtle pattern of DISALLOW_COPY in an abstract class (aka
> interface). Just let the subclass do this. But yeah, a protected virtual
> destructor is missing though.

Done.

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_master.cc:245: if (browser_context_ &&
browser_context_->IsOffTheRecord()) {
On 2012/12/20 01:39:29, joth wrote:
> note BrowserContext has a TODO to remove IsOffTheRecord(). I don't think you
can
> do much about that here though.

Added a TODO

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/7001/chrome/browser/visitedlink...
chrome/browser/visitedlink/visitedlink_master.h:339: VisitedLinkDelegate*
delegate_;
On 2012/12/20 01:39:29, joth wrote:
> // weak.

Added lengthier comment.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_delegate.h:22: class URLEnumerator {
style question: HistoryService inherits VisitedLinkDelegate. This URLEnumerator
class are sometimes referred to as HistoryService::URLEnumerator (in old code)
and VisitedLinkDelegate::URLEnumerator (new code in this patch).

Is there any convention that should be followed here?

[joth, 2012-12-29 18:32:06]

Generally looks good to me, thanks Bo!

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_delegate.h:22: class URLEnumerator {
On 2012/12/29 01:48:12, boliu wrote:
> style question: HistoryService inherits VisitedLinkDelegate. This
URLEnumerator
> class are sometimes referred to as HistoryService::URLEnumerator (in old code)
> and VisitedLinkDelegate::URLEnumerator (new code in this patch).
> 
> Is there any convention that should be followed here?

I don't know about any specific convention, but I find the latter more obvious
as it immediately tells me where to look to find the inner class definition.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_delegate.h:28: // indicates all URLs has
been returned successfully.
nit: has -> have

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_event_listener.cc (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_event_listener.cc:116:
content::NotificationService::AllBrowserContextsAndSources());
slightly confusing that we register for all browser contexts here, but then
filter them out with the delegate AreEquivContext() calls. It's because we want
root profile comparison (treating incognito and main as same) whereas this would
just do pointer comparison?

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:105: virtual const GURL&
NextURL() = 0;
suggest returning GURL by value rather than const ref (esp on abstract methods)
-- avoids weird lifetime issues in the implementation.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:115: void
DeleteURLs(URLIterator* iterator);
the abstract iterator is OK but a little confusing to keep in mind along side
the similar delegate URLEnumerator class. presume they can't be combined?

[boliu, 2012-12-30 02:08:15]

Looks like chrome_frame_tests are just flaky/broken (very different results on
the same patch set 9), so going to hope that they get fixed when this lands.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_delegate.h:22: class URLEnumerator {
On 2012/12/29 18:32:06, joth wrote:
> On 2012/12/29 01:48:12, boliu wrote:
> > style question: HistoryService inherits VisitedLinkDelegate. This
> URLEnumerator
> > class are sometimes referred to as HistoryService::URLEnumerator (in old
code)
> > and VisitedLinkDelegate::URLEnumerator (new code in this patch).
> > 
> > Is there any convention that should be followed here?
> 
> I don't know about any specific convention, but I find the latter more obvious
> as it immediately tells me where to look to find the inner class definition.

Changed to VisitedLinkDelegate::URLEnumerator everywhere. Only one that might be
controversial is HistoryBackend::IterateURLs (which should probably be re-named
in a CL afterwards)

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_delegate.h:28: // indicates all URLs has
been returned successfully.
On 2012/12/29 18:32:06, joth wrote:
> nit: has -> have

Done.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_event_listener.cc (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_event_listener.cc:116:
content::NotificationService::AllBrowserContextsAndSources());
On 2012/12/29 18:32:06, joth wrote:
> slightly confusing that we register for all browser contexts here, but then
> filter them out with the delegate AreEquivContext() calls. It's because we
want
> root profile comparison (treating incognito and main as same) whereas this
would
> just do pointer comparison?

That would be my guess. I don't want to change existing behavior in this
"refactor" CL, but probably worth looking at afterwards.

Also probably better to refactor this logic out of VisitedLink and add a method
like VisitedLinkMaster::OnRendererProcessCreated to add renderers explicitly.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:105: virtual const GURL&
NextURL() = 0;
On 2012/12/29 18:32:06, joth wrote:
> suggest returning GURL by value rather than const ref (esp on abstract
methods)
> -- avoids weird lifetime issues in the implementation. 

Done. But client code is still using references.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:115: void
DeleteURLs(URLIterator* iterator);
On 2012/12/29 18:32:06, joth wrote:
> the abstract iterator is OK but a little confusing to keep in mind along side
> the similar delegate URLEnumerator class. presume they can't be combined?

I am going to argue no. Only after this CL did I learn the difference between
Iterator and Enumerator. Basically Iterator is created by the delegate and
passed into VisitedLink; Enumerator is created by VisitedLink and passed into
the delegate. (They also do slightly different things). So I think it'll be more
confusing if they are combined into the same interface (unless it lives
somewhere else that both delegate and VisitedLink depends on, like base)

[joth, 2012-12-30 21:51:07]

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:105: virtual const GURL&
NextURL() = 0;
On 2012/12/30 02:08:15, boliu wrote:
> On 2012/12/29 18:32:06, joth wrote:
> > suggest returning GURL by value rather than const ref (esp on abstract
> methods)
> > -- avoids weird lifetime issues in the implementation. 
> 
> Done. But client code is still using references.

OK... I think there's special magic in the language to keep the temporary return
value alive as long as the reference when you do this.  but my own preference
would be to just copy construct an instance. (RVO means there's probably no
difference in performance)

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:115: void
DeleteURLs(URLIterator* iterator);
On 2012/12/30 02:08:15, boliu wrote:
> On 2012/12/29 18:32:06, joth wrote:
> > the abstract iterator is OK but a little confusing to keep in mind along
side
> > the similar delegate URLEnumerator class. presume they can't be combined?
> 
> I am going to argue no. Only after this CL did I learn the difference between
> Iterator and Enumerator. Basically Iterator is created by the delegate and
> passed into VisitedLink; Enumerator is created by VisitedLink and passed into
> the delegate. (They also do slightly different things). So I think it'll be
more
> confusing if they are combined into the same interface (unless it lives
> somewhere else that both delegate and VisitedLink depends on, like base)

Right - URLEnumerator is actually visitor pattern. So it comes down to whether
you want push (visitor) or pull (iterator) semantics on your algorithm.
Looking at HistoryBackend::IteratorURLs, that would be hard to change to "pull",
so yeah looks like we need to keep both concepts.

(somewhat bugs me that  AddURLs takes a vector but DeleteURLs  takes an abstract
iterator, guess if needed we just change AddURLs to use the abstract iterator
too)

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_delegate.h:38: virtual void
RebuildTable(URLEnumerator* enumerator) = 0;
Just realized, the History implementation of this delegates it onto
HistoryBackend to do at some point in future. (On same or another thread? Can't
easily see)
so we need to document that |enumerator| remains valid to be called after this
method returned. (And only on this thread? or on any thread?) And until when?
Until the next call to this method or the underlying VisitedLinkMaster is
destroyed? (guesses)

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:105: virtual const GURL
NextURL() = 0;
nit: remove const, it doesn't achieve anything.

[Jói, 2012-12-31 17:41:57]

LGTM once joth@'s comments are resolved to his satisfaction.
On Dec 30, 2012 9:51 PM, <joth@chromium.org> wrote:

>
> https://codereview.chromium.**org/11573060/diff/23001/**
>
chrome/browser/visitedlink/**visitedlink_master.h<https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlink/visitedlink_master.h>
> File chrome/browser/visitedlink/**visitedlink_master.h (right):
>
> https://codereview.chromium.**org/11573060/diff/23001/**
>
chrome/browser/visitedlink/**visitedlink_master.h#**newcode105<https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlink/visitedlink_master.h#newcode105>
> chrome/browser/visitedlink/**visitedlink_master.h:105: virtual const GURL&
> NextURL() = 0;
> On 2012/12/30 02:08:15, boliu wrote:
>
>> On 2012/12/29 18:32:06, joth wrote:
>> > suggest returning GURL by value rather than const ref (esp on
>>
> abstract
>
>> methods)
>> > -- avoids weird lifetime issues in the implementation.
>>
>
>  Done. But client code is still using references.
>>
>
> OK... I think there's special magic in the language to keep the
> temporary return value alive as long as the reference when you do this.
> but my own preference would be to just copy construct an instance. (RVO
> means there's probably no difference in performance)
>
> https://codereview.chromium.**org/11573060/diff/23001/**
>
chrome/browser/visitedlink/**visitedlink_master.h#**newcode115<https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlink/visitedlink_master.h#newcode115>
> chrome/browser/visitedlink/**visitedlink_master.h:115: void
> DeleteURLs(URLIterator* iterator);
> On 2012/12/30 02:08:15, boliu wrote:
>
>> On 2012/12/29 18:32:06, joth wrote:
>> > the abstract iterator is OK but a little confusing to keep in mind
>>
> along side
>
>> > the similar delegate URLEnumerator class. presume they can't be
>>
> combined?
>
>  I am going to argue no. Only after this CL did I learn the difference
>>
> between
>
>> Iterator and Enumerator. Basically Iterator is created by the delegate
>>
> and
>
>> passed into VisitedLink; Enumerator is created by VisitedLink and
>>
> passed into
>
>> the delegate. (They also do slightly different things). So I think
>>
> it'll be more
>
>> confusing if they are combined into the same interface (unless it
>>
> lives
>
>> somewhere else that both delegate and VisitedLink depends on, like
>>
> base)
>
> Right - URLEnumerator is actually visitor pattern. So it comes down to
> whether you want push (visitor) or pull (iterator) semantics on your
> algorithm.
> Looking at HistoryBackend::IteratorURLs, that would be hard to change to
> "pull", so yeah looks like we need to keep both concepts.
>
> (somewhat bugs me that  AddURLs takes a vector but DeleteURLs  takes an
> abstract iterator, guess if needed we just change AddURLs to use the
> abstract iterator too)
>
> https://codereview.chromium.**org/11573060/diff/32001/**
>
chrome/browser/visitedlink/**visitedlink_delegate.h<https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlink/visitedlink_delegate.h>
> File chrome/browser/visitedlink/**visitedlink_delegate.h (right):
>
> https://codereview.chromium.**org/11573060/diff/32001/**
>
chrome/browser/visitedlink/**visitedlink_delegate.h#**newcode38<https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlink/visitedlink_delegate.h#newcode38>
> chrome/browser/visitedlink/**visitedlink_delegate.h:38: virtual void
> RebuildTable(URLEnumerator* enumerator) = 0;
> Just realized, the History implementation of this delegates it onto
> HistoryBackend to do at some point in future. (On same or another
> thread? Can't easily see)
> so we need to document that |enumerator| remains valid to be called
> after this method returned. (And only on this thread? or on any thread?)
> And until when? Until the next call to this method or the underlying
> VisitedLinkMaster is destroyed? (guesses)
>
> https://codereview.chromium.**org/11573060/diff/32001/**
>
chrome/browser/visitedlink/**visitedlink_master.h<https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlink/visitedlink_master.h>
> File chrome/browser/visitedlink/**visitedlink_master.h (right):
>
> https://codereview.chromium.**org/11573060/diff/32001/**
>
chrome/browser/visitedlink/**visitedlink_master.h#**newcode105<https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlink/visitedlink_master.h#newcode105>
> chrome/browser/visitedlink/**visitedlink_master.h:105: virtual const GURL
> NextURL() = 0;
> nit: remove const, it doesn't achieve anything.
>
>
https://codereview.chromium.**org/11573060/<https://codereview.chromium.org/1...
>

[boliu, 2013-01-02 17:09:36]

+erikwright for DEPS

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:105: virtual const GURL&
NextURL() = 0;
On 2012/12/30 21:51:07, joth wrote:
> On 2012/12/30 02:08:15, boliu wrote:
> > On 2012/12/29 18:32:06, joth wrote:
> > > suggest returning GURL by value rather than const ref (esp on abstract
> > methods)
> > > -- avoids weird lifetime issues in the implementation. 
> > 
> > Done. But client code is still using references.
> 
> OK... I think there's special magic in the language to keep the temporary
return
> value alive as long as the reference when you do this.  but my own preference
> would be to just copy construct an instance. (RVO means there's probably no
> difference in performance) 

Done.

https://codereview.chromium.org/11573060/diff/23001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:115: void
DeleteURLs(URLIterator* iterator);
On 2012/12/30 21:51:07, joth wrote:
> On 2012/12/30 02:08:15, boliu wrote:
> > On 2012/12/29 18:32:06, joth wrote:
> > > the abstract iterator is OK but a little confusing to keep in mind along
> side
> > > the similar delegate URLEnumerator class. presume they can't be combined?
> > 
> > I am going to argue no. Only after this CL did I learn the difference
between
> > Iterator and Enumerator. Basically Iterator is created by the delegate and
> > passed into VisitedLink; Enumerator is created by VisitedLink and passed
into
> > the delegate. (They also do slightly different things). So I think it'll be
> more
> > confusing if they are combined into the same interface (unless it lives
> > somewhere else that both delegate and VisitedLink depends on, like base)
> 
> Right - URLEnumerator is actually visitor pattern. So it comes down to whether
> you want push (visitor) or pull (iterator) semantics on your algorithm.
> Looking at HistoryBackend::IteratorURLs, that would be hard to change to
"pull",
> so yeah looks like we need to keep both concepts.
> 
> (somewhat bugs me that  AddURLs takes a vector but DeleteURLs  takes an
abstract
> iterator, guess if needed we just change AddURLs to use the abstract iterator
> too)

One more thing to do *after* refactoring and componentization.

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_delegate.h (right):

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_delegate.h:38: virtual void
RebuildTable(URLEnumerator* enumerator) = 0;
On 2012/12/30 21:51:07, joth wrote:
> Just realized, the History implementation of this delegates it onto
> HistoryBackend to do at some point in future. (On same or another thread?
Can't
> easily see)
> so we need to document that |enumerator| remains valid to be called after this
> method returned. (And only on this thread? or on any thread?) And until when?
> Until the next call to this method or the underlying VisitedLinkMaster is
> destroyed? (guesses)

Yes, history calls enumerators on the background thread. Added documentation
that methods can be called on any thread, but the delegate is responsible for
synchronization.

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://codereview.chromium.org/11573060/diff/32001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.h:105: virtual const GURL
NextURL() = 0;
On 2012/12/30 21:51:07, joth wrote:
> nit: remove const, it doesn't achieve anything.

Done.

[erikwright (departed), 2013-01-02 17:29:31]

DEPS LGTM.

[brettw, 2013-01-02 20:23:40]

lgtm

https://codereview.chromium.org/11573060/diff/44001/chrome/browser/history/hi...
File chrome/browser/history/history.cc (right):

https://codereview.chromium.org/11573060/diff/44001/chrome/browser/history/hi...
chrome/browser/history/history.cc:122: virtual GURL NextURL() {
Since you're doing a const iterator, does it make sense to change this interface
to return a "const GURL&" to avoid the copy?

Can you also add OVERRIDE for the overridden functions?

https://codereview.chromium.org/11573060/diff/44001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

https://codereview.chromium.org/11573060/diff/44001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_master.cc:367: const GURL
url(urls->NextURL());
If you change the interface as suggested above, and you keep the & here to avoid
the copy?

[joth, 2013-01-02 21:28:21]

https://codereview.chromium.org/11573060/diff/44001/chrome/browser/history/hi...
File chrome/browser/history/history.cc (right):

https://codereview.chromium.org/11573060/diff/44001/chrome/browser/history/hi...
chrome/browser/history/history.cc:122: virtual GURL NextURL() {
On 2013/01/02 20:23:40, brettw wrote:
> Since you're doing a const iterator, does it make sense to change this
interface
> to return a "const GURL&" to avoid the copy?
> 
> Can you also add OVERRIDE for the overridden functions?

Ah this was my fault, due to my suggestion back up thread... we're planning to
make another subclass of VisitedLinkMaster::URLIterator and that one may not
have the url sat around as a GURL in order to return a const-ref to. (It will be
pulling the urls from a java-side, so may only hold it in a temporary?).

Another option would be to document that the reference returned is only valid up
until the next call to NextURL() (so then an implementation can more easily
cache it as a member). That seems a bit subtle though.

[boliu, 2013-01-02 21:36:10]

https://chromiumcodereview.appspot.com/11573060/diff/44001/chrome/browser/his...
File chrome/browser/history/history.cc (right):

https://chromiumcodereview.appspot.com/11573060/diff/44001/chrome/browser/his...
chrome/browser/history/history.cc:122: virtual GURL NextURL() {
On 2013/01/02 21:28:21, joth wrote:
> On 2013/01/02 20:23:40, brettw wrote:
> > Since you're doing a const iterator, does it make sense to change this
> interface
> > to return a "const GURL&" to avoid the copy?
> > 
> > Can you also add OVERRIDE for the overridden functions?
> 
> Ah this was my fault, due to my suggestion back up thread... we're planning to
> make another subclass of VisitedLinkMaster::URLIterator and that one may not
> have the url sat around as a GURL in order to return a const-ref to. (It will
be
> pulling the urls from a java-side, so may only hold it in a temporary?).
> 
> Another option would be to document that the reference returned is only valid
up
> until the next call to NextURL() (so then an implementation can more easily
> cache it as a member). That seems a bit subtle though.

OVERRIDE added.

https://chromiumcodereview.appspot.com/11573060/diff/44001/chrome/browser/vis...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

https://chromiumcodereview.appspot.com/11573060/diff/44001/chrome/browser/vis...
chrome/browser/visitedlink/visitedlink_master.cc:367: const GURL
url(urls->NextURL());
On 2013/01/02 20:23:40, brettw wrote:
> If you change the interface as suggested above, and you keep the & here to
avoid
> the copy?

Joth replied before I did but after I uploaded a new patch. I changed it to
GURL& here, but did not update the Iterator interface.

Is there a style you prefer?

[boliu, 2013-01-04 07:27:47]

Decided to avoid copying and added a comment to Iterator about lifetime of GURL.
It's not so bad because Iterator is not meant to be used by delegates; the only
use of Iterator is in this patch which already satisfies condition.

[commit-bot: I haz the power, 2013-01-04 19:18:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11573060/56001

[commit-bot: I haz the power, 2013-01-04 20:27:17]

Change committed as 175186

[boliu, 2013-01-04 22:37:26]

Re-opening this since it has memory leaks and I reverted it here:
https://codereview.chromium.org/11784006/

[boliu, 2013-01-05 18:31:33]

brettw@ PTAL if my scoped_refptr usage is correct

Some tests were leaking URLEnumerator/TableBuilder when RebuildTable posts a
task to the background thread but the task never runs. URLEnumerator used to
deleted in OnComplete which calls Release and this never happens if the task is
not run. Fixed by having HistoryService::RebuildTable and
HistoryBackend::IterateURLs take scoped_refptr instead of raw pointers.

Before this patch, tests would skip creating VisitedLink service so that code is
never exercised. Now depending on the constructor the tests use, VisitedLink is
created by HistoryService directly.

[boliu, 2013-01-07 23:07:52]

ping!

[tfarina, 2013-01-08 00:16:43]

https://codereview.chromium.org/11573060/diff/78001/chrome/browser/visitedlin...
File chrome/browser/visitedlink/visitedlink_unittest.cc (right):

https://codereview.chromium.org/11573060/diff/78001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_unittest.cc:52: //
========================== TestVisitedLinkDelegate ==========================
nit: what is up with this kind of comment? Can you just remove it? It's very
uncommon to have this.

https://codereview.chromium.org/11573060/diff/78001/chrome/browser/visitedlin...
chrome/browser/visitedlink/visitedlink_unittest.cc:84: //
======================== TestVisitedLinkDelegate End ========================
nit: heh, never saw this before ;), could you remove?

[tfarina, 2013-01-08 00:17:31]

Could you also fill the BUG= with the corresponding number?

Ahh, just saw this was committed...

[boliu, 2013-01-08 00:29:03]

On 2013/01/08 00:17:31, tfarina wrote:
> Could you also fill the BUG= with the corresponding number?
> 
> Ahh, just saw this was committed...

Committed and reverted and now has more changes.

Could you double check I used scoped_refptr correctly (first time using it as
function params) See comment #23.

Removed the weird comments. The feature bug for android webview is in internal
bug tracker, but will file a bug for componentizing visitedlinks and add the bug
number.

[boliu, 2013-01-09 19:57:35]

ping!

Could someone double check my scoped_refptr usage follows style?

I searched through chromium-dev and got the general impression that const
scoped_refptr<>& is way to go? See the updated the description for where this
was added.

[Jói, 2013-01-09 20:15:18]

> Could someone double check my scoped_refptr usage follows style?

I think const scoped_refptr<>& is pretty standard.

Cheers,
Jói


On Wed, Jan 9, 2013 at 11:57 AM,  <boliu@chromium.org> wrote:
> ping!
>
> Could someone double check my scoped_refptr usage follows style?
>
> I searched through chromium-dev and got the general impression that const
> scoped_refptr<>& is way to go? See the updated the description for where
> this
> was added.
>
> https://codereview.chromium.org/11573060/

[commit-bot: I haz the power, 2013-01-09 20:47:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11573060/90001

[commit-bot: I haz the power, 2013-01-09 22:37:23]

Change committed as 175906