Add PrerenderLocalPredictor, which will eventually perform

Add PrerenderLocalPredictor, which will eventually perform
local prerender predictions.  At this point, the class itself
is just a dummy, and the focus for now is on the interface with
the history service.  I added everything I need added in history,
and PrerenderLocalPredictor performs the basic interface functionality
it needs with the History.  In a later CL, I will actually add the
prerender semantics.
R=brettw@chromium.org, cbentzel@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133548

[cbentzel, 2012-04-12 21:16:35]

I'm going to have to spend some time looking at the history API to comment
reasonably on this CL. Just a few small generic programming questions.

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
File chrome/browser/history/history.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
chrome/browser/history/history.cc:905: if (history_backend_)
Why don't you need to do this check in the Add case?

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
File chrome/browser/history/history.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
chrome/browser/history/history.h:601: // thread in which the observer would like
to be notified.
Would it be easier to just add a callback here instead of an observer? Callbacks
handle lifetime issues pretty nicely. The main downside is that it's hard to
remove.

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.cc:88:
base::TimeDelta::FromMilliseconds(kInitDelayMs),
This kInitDelayMs seems prone to flake. Is there a better way to do this?

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.h:29: explicit
PrerenderLocalPredictor(PrerenderManager* prerender_manager);
Doesn't seem like this needs to know about the PrerenderManager - could just
pass in a Profile.

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.h:33: void
OnAddVisit(history::BriefVisitInfo info);
virtual prefix and OVERRIDE suffix.

[tburkard, 2012-04-12 21:32:37]

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
File chrome/browser/history/history.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
chrome/browser/history/history.cc:905: if (history_backend_)
In the add case, I am loading the database backend if necessary, so it will be
present as a result of loading it.
On 2012/04/12 21:16:36, cbentzel wrote:
> Why don't you need to do this check in the Add case?

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
File chrome/browser/history/history.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/history...
chrome/browser/history/history.h:601: // thread in which the observer would like
to be notified.
I thought this observer was easier, due to the different threads involved and
the fact that either side could  go away first.
On 2012/04/12 21:16:36, cbentzel wrote:
> Would it be easier to just add a callback here instead of an observer?
Callbacks
> handle lifetime issues pretty nicely. The main downside is that it's hard to
> remove.

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.cc:88:
base::TimeDelta::FromMilliseconds(kInitDelayMs),
Brett requested to wait some time until after startup to avoid using CPU during
that time. I can't find a good condition to wait on, which is why I thought a
time limit makes the most sense?

On 2012/04/12 21:16:36, cbentzel wrote:
> This kInitDelayMs seems prone to flake. Is there a better way to do this?

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.h:29: explicit
PrerenderLocalPredictor(PrerenderManager* prerender_manager);
Later it will -- to order prerenders.
On 2012/04/12 21:16:36, cbentzel wrote:
> Doesn't seem like this needs to know about the PrerenderManager - could just
> pass in a Profile.

https://chromiumcodereview.appspot.com/10067018/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.h:33: void
OnAddVisit(history::BriefVisitInfo info);
On 2012/04/12 21:16:36, cbentzel wrote:
> virtual prefix and OVERRIDE suffix. 

Done.

[cbentzel, 2012-04-17 18:29:27]

Overall this seems OK.

I do wonder if this could register itself as a WebContentsObserver and implement
DidNavigateMainFrame for the live changes. That would minimize changes to the
history system.

[cbentzel, 2012-04-17 19:07:02]

On 2012/04/17 18:29:27, cbentzel wrote:
> Overall this seems OK.
> 
> I do wonder if this could register itself as a WebContentsObserver and
implement
> DidNavigateMainFrame for the live changes. That would minimize changes to the
> history system.

I chatted with Timo.

The reason he chose this approach is that we'll be maintaining an in-memory map
of something like 

url_id -> [(url_id, count), (url_id, count), ....]

We are using the new history hooks both to update this map as well as to trigger
prerenders. This means that we only need to do URL map lookups at the time we
decide to trigger a prerender.

If we used DidNavigateMainFrame, we would need to do an inverse lookup on the
URL map on each navigation.

[cbentzel, 2012-04-17 19:35:32]

On 2012/04/17 19:07:02, cbentzel wrote:
> On 2012/04/17 18:29:27, cbentzel wrote:
> > Overall this seems OK.
> > 
> > I do wonder if this could register itself as a WebContentsObserver and
> implement
> > DidNavigateMainFrame for the live changes. That would minimize changes to
the
> > history system.
> 
> I chatted with Timo.
> 
> The reason he chose this approach is that we'll be maintaining an in-memory
map
> of something like 
> 
> url_id -> [(url_id, count), (url_id, count), ....]
> 
> We are using the new history hooks both to update this map as well as to
trigger
> prerenders. This means that we only need to do URL map lookups at the time we
> decide to trigger a prerender.
> 
> If we used DidNavigateMainFrame, we would need to do an inverse lookup on the
> URL map on each navigation.

Actually, looking at this some more I wonder if we could do the
DidNavigateMainFrame for live changes and HistoryService::QueryHistory to do the
same thing.

The main downside to this approach is that the initial history search may do a
lot of URL map lookups. The other issue is that you may want to maintain your
own URL->id table (and vice versa) and this adds complexity plus extra memory
overhead.

Sorry to keep throwing out other ideas, just want to make sure the design is
right before we get to the details.

[cbentzel, 2012-04-17 20:28:16]

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/history/hist...
chrome/browser/history/history.h:600: // Adds or removes observers for the
VisitDatabase.  Should be run in the
This is in the testing section, which is incorrect.

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/history/visi...
File chrome/browser/history/visit_database.h (right):

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/history/visi...
chrome/browser/history/visit_database.h:29: class VisitDatabaseObserver {
Should this handle DeleteVisit and UpdateVisit as well?

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/history/visi...
chrome/browser/history/visit_database.h:32: virtual void
OnAddVisit(BriefVisitInfo info) = 0;
Why doesn't this take a const VisitRow& as an argument?

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_local_predictor.cc:90:
&PrerenderLocalPredictor::Init);
Potential for a use-after-free here if the Predictor is deleted before the task
is handled. This could be a weak pointer supporting class.

http://codereview.chromium.org/10067018/diff/6002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_local_predictor.cc:121: new
GetURLForURLIDTask(this, info.url_id),
Why do you need to do this? I thought the whole purpose of adding the observer
to the VisitDB was to minimize URL table lookups?

[tburkard, 2012-04-17 20:36:26]

Yes.  The other reason is that since this is not really a history feature,
we would like to keep this outside of history.  Furthermore, we want to
only keep around URLIDs so as to now blow up memory usage.

Thanks.

Timo

On Tue, Apr 17, 2012 at 12:35 PM, <cbentzel@chromium.org> wrote:

> On 2012/04/17 19:07:02, cbentzel wrote:
>
>> On 2012/04/17 18:29:27, cbentzel wrote:
>> > Overall this seems OK.
>> >
>> > I do wonder if this could register itself as a WebContentsObserver and
>> implement
>> > DidNavigateMainFrame for the live changes. That would minimize changes
>> to
>>
> the
>
>> > history system.
>>
>
>  I chatted with Timo.
>>
>
>  The reason he chose this approach is that we'll be maintaining an
>> in-memory
>>
> map
>
>> of something like
>>
>
>  url_id -> [(url_id, count), (url_id, count), ....]
>>
>
>  We are using the new history hooks both to update this map as well as to
>>
> trigger
>
>> prerenders. This means that we only need to do URL map lookups at the
>> time we
>> decide to trigger a prerender.
>>
>
>  If we used DidNavigateMainFrame, we would need to do an inverse lookup on
>> the
>> URL map on each navigation.
>>
>
> Actually, looking at this some more I wonder if we could do the
> DidNavigateMainFrame for live changes and HistoryService::QueryHistory to
> do the
> same thing.
>
> The main downside to this approach is that the initial history search may
> do a
> lot of URL map lookups. The other issue is that you may want to maintain
> your
> own URL->id table (and vice versa) and this adds complexity plus extra
> memory
> overhead.
>
> Sorry to keep throwing out other ideas, just want to make sure the design
> is
> right before we get to the details.
>
>
>
http://codereview.chromium.**org/10067018/<http://codereview.chromium.org/100...
>

[tburkard, 2012-04-17 20:51:36]

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/hist...
File chrome/browser/history/history.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/hist...
chrome/browser/history/history.h:600: // Adds or removes observers for the
VisitDatabase.  Should be run in the
On 2012/04/17 20:28:16, cbentzel wrote:
> This is in the testing section, which is incorrect.

Done.

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/hist...
File chrome/browser/history/visit_database.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/hist...
chrome/browser/history/visit_database.h:29: class VisitDatabaseObserver {
I'm not concerned about these for now, the majority of operations is adding
visits.  We may add that later.

On 2012/04/17 20:28:16, cbentzel wrote:
> Should this handle DeleteVisit and UpdateVisit as well?

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/hist...
chrome/browser/history/visit_database.h:32: virtual void
OnAddVisit(BriefVisitInfo info) = 0;
Either would work.  I figured I'd use BriefVisitInfo since that was the
information I needed, so at the place where I'm using it, everything is
BriefVisitInfo's.

On 2012/04/17 20:28:16, cbentzel wrote:
> Why doesn't this take a const VisitRow& as an argument?

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/prer...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/prer...
chrome/browser/prerender/prerender_local_predictor.cc:90:
&PrerenderLocalPredictor::Init);
Shouldn't, because timer_ takes care of this (when destructed), I thought?
On 2012/04/17 20:28:16, cbentzel wrote:
> Potential for a use-after-free here if the Predictor is deleted before the
task
> is handled. This could be a weak pointer supporting class.

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/prer...
chrome/browser/prerender/prerender_local_predictor.cc:121: new
GetURLForURLIDTask(this, info.url_id),
I will eventually need to lookup URLs when I want to actually prerender.

As for why I'm doing it here, it was mostly to demonstrate the APIs I'm using. 
This is not really doing anything useful yet.  Once I make the second CL that is
actually using this info and initiating prerenders, this line will go away, and
the URL lookup will only occur at places where we actually need the URL.
On 2012/04/17 20:28:16, cbentzel wrote:
> Why do you need to do this? I thought the whole purpose of adding the observer
> to the VisitDB was to minimize URL table lookups?

[cbentzel, 2012-04-17 20:57:51]

OK, at this point I'm pretty satisfied with the overall approach. I want Brett
to review the history stuff now though.

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/hist...
File chrome/browser/history/visit_database.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/hist...
chrome/browser/history/visit_database.h:32: virtual void
OnAddVisit(BriefVisitInfo info) = 0;
On 2012/04/17 20:51:36, tburkard wrote:
> Either would work.  I figured I'd use BriefVisitInfo since that was the
> information I needed, so at the place where I'm using it, everything is
> BriefVisitInfo's.
> 
> On 2012/04/17 20:28:16, cbentzel wrote:
> > Why doesn't this take a const VisitRow& as an argument?
> 

Brett should make the call here - and OWNERS requires it :)

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/prer...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/prer...
chrome/browser/prerender/prerender_local_predictor.cc:90:
&PrerenderLocalPredictor::Init);
On 2012/04/17 20:51:36, tburkard wrote:
> Shouldn't, because timer_ takes care of this (when destructed), I thought?
> On 2012/04/17 20:28:16, cbentzel wrote:
> > Potential for a use-after-free here if the Predictor is deleted before the
> task
> > is handled. This could be a weak pointer supporting class.
> 

Yeah, you are right.

https://chromiumcodereview.appspot.com/10067018/diff/6002/chrome/browser/prer...
chrome/browser/prerender/prerender_local_predictor.cc:121: new
GetURLForURLIDTask(this, info.url_id),
On 2012/04/17 20:51:36, tburkard wrote:
> I will eventually need to lookup URLs when I want to actually prerender.
> 
> As for why I'm doing it here, it was mostly to demonstrate the APIs I'm using.

> This is not really doing anything useful yet.  Once I make the second CL that
is
> actually using this info and initiating prerenders, this line will go away,
and
> the URL lookup will only occur at places where we actually need the URL.
> On 2012/04/17 20:28:16, cbentzel wrote:
> > Why do you need to do this? I thought the whole purpose of adding the
observer
> > to the VisitDB was to minimize URL table lookups? 
> 

OK. I'd just remove it for now.

[brettw, 2012-04-19 19:17:51]

https://chromiumcodereview.appspot.com/10067018/diff/10002/chrome/browser/his...
File chrome/browser/history/history_backend.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/10002/chrome/browser/his...
chrome/browser/history/history_backend.cc:2496:
db_->AddVisitDatabaseObserver(observer);
This isn't safe to do from any thread but the DB thread, since the db_ member is
changed from the backend on the history thread. You'll need to post to the
history thread to add the observer.

Maybe we need a new add method in ObserverListThreadsafe that takes a thread for
the observer rather than just using current.

[tburkard, 2012-04-20 19:38:21]

https://chromiumcodereview.appspot.com/10067018/diff/10002/chrome/browser/his...
File chrome/browser/history/history_backend.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/10002/chrome/browser/his...
chrome/browser/history/history_backend.cc:2496:
db_->AddVisitDatabaseObserver(observer);
Per our conversation, pulled the ObserverListThreadsafe into HistoryService.

On 2012/04/19 19:17:51, brettw wrote:
> This isn't safe to do from any thread but the DB thread, since the db_ member
is
> changed from the backend on the history thread. You'll need to post to the
> history thread to add the observer.
> 
> Maybe we need a new add method in ObserverListThreadsafe that takes a thread
for
> the observer rather than just using current.

[brettw, 2012-04-20 22:21:50]

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history.h:914: visit_database_observers_;
Indent this line 4 spaces.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history_backend.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history_backend.h:595: void
NotifyVisitObservers(VisitRow* visit);
Can't you pass this by const ref?

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history_types.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history_types.h:765: virtual void
OnAddVisit(BriefVisitInfo info) = 0;
I'd probably change all the places where you pass this to be by const ref
(including in history.h). Although this case is pretty borderline (looks like 20
bytes) I think people are more used to that and if we add onto the struct we
won't have to change everything.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/visit_database.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/visit_database.cc:566: sql::Statement
statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
You're only calling this once, right? Then I'd use "UniqueStatement" since the
cached statement will keep some stuff around forever. Might be good to add a
comment that we don't expect this to be used repeatedly.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:73: };
This line should be "}  // namespace"

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:117:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
You're jumping through a lot of hoops here. Say you visit a URL with 2 redirects
(not that uncommon, say an ad click). This is a lot of messages floating around.

It seems like maybe your observer should have the URL inside it. You can add the
observe at a higher level in historyBackend::AddPageVisit which has all the info
you need without doing all these extra thread hops and DB lookups.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:132: void
PrerenderLocalPredictor::OnLookupURL(history::URLID url_id, GURL url) {
GURL -> const GURL&

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.h:61: }
Should have "  // namespace prerender" at the end and a blank line after.

[tburkard, 2012-04-20 22:56:44]

Thanks for your comments, Brett.

About passing BriefVisitInfo as const ref&, I'm not sure if that work due to the
fact that the callback happens after the stack object is already gone?

See my comment below, and let me know!

Thanks.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history.h:914: visit_database_observers_;
On 2012/04/20 22:21:50, brettw wrote:
> Indent this line 4 spaces.

Done.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history_backend.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history_backend.h:595: void
NotifyVisitObservers(VisitRow* visit);
On 2012/04/20 22:21:50, brettw wrote:
> Can't you pass this by const ref?

Done.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history_types.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history_types.h:765: virtual void
OnAddVisit(BriefVisitInfo info) = 0;
Wouldn't that be problematic, since the BriefVisitInfo is on the stack when it
is generated, and wouldnt be around long enough for the callback on the other
thread of the observer?

On 2012/04/20 22:21:50, brettw wrote:
> I'd probably change all the places where you pass this to be by const ref
> (including in history.h). Although this case is pretty borderline (looks like
20
> bytes) I think people are more used to that and if we add onto the struct we
> won't have to change everything.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/visit_database.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/visit_database.cc:566: sql::Statement
statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
On 2012/04/20 22:21:50, brettw wrote:
> You're only calling this once, right? Then I'd use "UniqueStatement" since the
> cached statement will keep some stuff around forever. Might be good to add a
> comment that we don't expect this to be used repeatedly.

Done.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:73: };
On 2012/04/20 22:21:50, brettw wrote:
> This line should be "}  // namespace"

Done.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:117:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
This is just "dummy code" at this point to demonstrate the API.  Once I commit
this change, I will immediately modify this class to actually do smart
predictions.

In that scenario, AddVisit callbacks will not trigger automatic lookups, but
only at times when we decide to do a prerender, do we need to lookup the URLID
that we would like to prerender.  So in reality, we expect this to be perhaps 1
lookup for every 5 (real, without redirects) navigations.
On 2012/04/20 22:21:50, brettw wrote:
> You're jumping through a lot of hoops here. Say you visit a URL with 2
redirects
> (not that uncommon, say an ad click). This is a lot of messages floating
around.
> 
> It seems like maybe your observer should have the URL inside it. You can add
the
> observe at a higher level in historyBackend::AddPageVisit which has all the
info
> you need without doing all these extra thread hops and DB lookups.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:132: void
PrerenderLocalPredictor::OnLookupURL(history::URLID url_id, GURL url) {
On 2012/04/20 22:21:50, brettw wrote:
> GURL -> const GURL&

Done.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.h:61: }
On 2012/04/20 22:21:50, brettw wrote:
> Should have "  // namespace prerender" at the end and a blank line after.

Done.

[brettw, 2012-04-23 17:52:54]

LGTM with pass by ref change. I'd still consider in the future whether you want
to just pass the GURL with the notification and move the notification source to
be the HistoryBackend.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history_types.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history_types.h:765: virtual void
OnAddVisit(BriefVisitInfo info) = 0;
PostTask knows to copy pass-by-ref args to other threads. So pass by ref should
be OK as far as I know.

[tburkard, 2012-04-23 18:24:11]

Thanks for the LGTM.  I just fixed the const ref, and I will now run trybots and
then commit.

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
File chrome/browser/history/history_types.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/his...
chrome/browser/history/history_types.h:765: virtual void
OnAddVisit(BriefVisitInfo info) = 0;
On 2012/04/23 17:52:54, brettw wrote:
> PostTask knows to copy pass-by-ref args to other threads. So pass by ref
should
> be OK as far as I know.

Done.

[cbentzel, 2012-04-23 20:07:29]

On Mon, Apr 23, 2012 at 2:24 PM, <tburkard@chromium.org> wrote:

> Thanks for the LGTM.  I just fixed the const ref, and I will now run
> trybots and
> then commit.


I'd like to take a second pass over the prerender changes. Will do right
now.


>
>
>
> https://chromiumcodereview.**appspot.com/10067018/diff/**
>
18001/chrome/browser/history/**history_types.h<https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/history/history_types.h>
> File chrome/browser/history/**history_types.h (right):
>
> https://chromiumcodereview.**appspot.com/10067018/diff/**
>
18001/chrome/browser/history/**history_types.h#newcode765<https://chromiumcodereview.appspot.com/10067018/diff/18001/chrome/browser/history/history_types.h#newcode765>
> chrome/browser/history/**history_types.h:765: virtual void
> OnAddVisit(BriefVisitInfo info) = 0;
> On 2012/04/23 17:52:54, brettw wrote:
>
>> PostTask knows to copy pass-by-ref args to other threads. So pass by
>>
> ref should
>
>> be OK as far as I know.
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/10067018/<https://chromiumcodereview...
>

[cbentzel, 2012-04-23 20:29:46]

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:82:
PrerenderLocalPredictor::PrerenderLocalPredictor(
Nit: ordering of implementations should match ordering of declarations in the
class.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:94: void
PrerenderLocalPredictor::OnGetInitialVisitHistory(
Instead of a raw pointer, this should take a scoped_ptr argument and use the
.Pass() method in GetVisitHistoryTask::DoneRunOnMainThread. See the chromium-dev
thread about it if you haven't seen this before.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:109:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Maybe move this DCHECK to the top of the function.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:112:
&history_db_consumer_);
Is it possible that this is run after the PrerenderLocalPredictor is destroyed?
Or does the history_db_consumer_ prevent that from being a problem?

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:127: HistoryService*
history = GetHistoryIfExists();
Is it possible for the history not to exist now, even though it did during
::Init()? I'm worried that this could somehow remain an observer.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.h:29: explicit
PrerenderLocalPredictor(PrerenderManager* prerender_manager);
Perhaps it's implied due to raw pointer - but you should state the lifetime
expectations of PrerenderManager relative to PrerenderLocalPredictor.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.h:49: static const int
kInitDelayMs = 5 * 1000;
Nit: I like moving these into the anonymous namespace in the .cc file so I can
tweak them without causing a number of rebuilds. Also thins out the header a
bit.

[tburkard, 2012-04-23 20:54:55]

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:82:
PrerenderLocalPredictor::PrerenderLocalPredictor(
On 2012/04/23 20:29:47, cbentzel wrote:
> Nit: ordering of implementations should match ordering of declarations in the
> class.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:94: void
PrerenderLocalPredictor::OnGetInitialVisitHistory(
On 2012/04/23 20:29:47, cbentzel wrote:
> Instead of a raw pointer, this should take a scoped_ptr argument and use the
> .Pass() method in GetVisitHistoryTask::DoneRunOnMainThread. See the
chromium-dev
> thread about it if you haven't seen this before.

Done.  Can you re-review that the way I did it is correct? (since I'm doing this
for the first time).

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:109:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/04/23 20:29:47, cbentzel wrote:
> Maybe move this DCHECK to the top of the function. 

Done.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:112:
&history_db_consumer_);
The consumer takes care of that case.

On 2012/04/23 20:29:47, cbentzel wrote:
> Is it possible that this is run after the PrerenderLocalPredictor is
destroyed?
> Or does the history_db_consumer_ prevent that from being a problem?

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.cc:127: HistoryService*
history = GetHistoryIfExists();
If the history no longer exists, it must have been destroyed, at which point the
observer list was destroyed, too.

On 2012/04/23 20:29:47, cbentzel wrote:
> Is it possible for the history not to exist now, even though it did during
> ::Init()? I'm worried that this could somehow remain an observer.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.h:29: explicit
PrerenderLocalPredictor(PrerenderManager* prerender_manager);
On 2012/04/23 20:29:47, cbentzel wrote:
> Perhaps it's implied due to raw pointer - but you should state the lifetime
> expectations of PrerenderManager relative to PrerenderLocalPredictor.

Done.

https://chromiumcodereview.appspot.com/10067018/diff/31003/chrome/browser/pre...
chrome/browser/prerender/prerender_local_predictor.h:49: static const int
kInitDelayMs = 5 * 1000;
On 2012/04/23 20:29:47, cbentzel wrote:
> Nit: I like moving these into the anonymous namespace in the .cc file so I can
> tweak them without causing a number of rebuilds. Also thins out the header a
> bit.

Done.

[cbentzel, 2012-04-23 21:15:54]

LGTM