Modifying prefetch to account for multi-profile.

Modifying prefetch to account for multi-profile.

Items of note:
- predictor_api is gone. Most functions in predictor_api.cc have moved into the chrome_browser_net::Predictor class or the Profile class.
- The predictor state is cleaned up in the Profile dtor.
- Predictor is owned by the ProfileIOData of the profile.
- InitialObserver class is gone since each profile keeps their own info, the non-OTR don't care if anyone is OTR.
- Predictor is created by the profile and then passed to the ProfileIOData. Then its initialization is finished on the IOThread.
- ConnectInterceptor now subclasses off of UrlRequestJobFactory::Interceptor.
- Updated Profile to create a SimpleShutdownPredictor with limited functionality when in unittests.

BUG=89937, 90114
TEST=passes existing

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97446

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100555

[rpetterson, 2011-07-21 06:12:10]

I'm having an issue with the predictor being deleted upon AddRef/Release when
the RunnableMethods are created, but otherwise it's ready. I imagine I've just
done something silly with the initialization somehow. But I wanted to get this
CL out to you asap since I'd like to try to get it in by Monday (if possible :).

[willchan no longer on Chromium, 2011-07-21 12:10:53]

Getting this in by Monday is ambitious :P

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:598: Profile* profile) {
Profile* is not allowed to be used on the IO thread. There is a potential race
condition with shutdown here. Profile is a UI thread only object.

This code shoudl move into Predictor initialization and should be removed from
IOThread.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:615: if (profile->GetPredictor()) {
This should die. Once you've made the Predictor be per-Profile, and you don't
have it for incognito, there's no danger of the Predictor getting contaminated
with OTR data. Just kill this off.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor.h:56: // the IO thread. Failure to do so will
result in a DCHECK at runtime.
This comment is wrong. It is used from both the UI and IO threads. Here's my
recommendation for lifetime management and thread organization stuff. Have
ProfileImpl construct the Predictor object (this is different from the current
world where Predictor is constructed on the IO thread, so be a bit careful with
some of the member variables that expect to be initialized and destroyed on the
same thread, such as the ScopedRunnableMethodFactory). Use the io_data_.Init()
call on
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...
to pass ownership of the Predictor object to the IO thread. Have ProfileImpl
keep a cached weak pointer to the Predictor object (this is why we created it on
the UI thread, so we can cache a non-owning pointer for use on the UI thread).
Note that this is safe because the ProfileImpl is destroyed on the UI thread
before the ProfileImplIOData is destroyed on the IO thread.

Note that ProfileIOData so far is always lazily initialized for startup reasons
and what not. You will want to eagerly initialize the Predictor object on the
ProfileImplIOData. Therefore, in
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...,
you should not use LazyParams. You should post a task to the IO thread to pass
ownership of the Predictor object to the IO thread. You should probably add a
InitOnIOThread() method to Predictor that gets invoked when this happens. This
will be analogous to the existing InitNetworkPredictorOnIOThread() that exists
in the IOThread class. Here you should initialize the member variables that
expect to be initialize on the IO thread, such as the
ScopedRunnableMethodFactory (it'll probably have to be wrapped in scoped_ptr).

Then please organize your methods here. Create one section for methods called on
the UI thread. Create another section for methods called on the IO thread. That
way it's clear to users what the thread expectations of the API are.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
File chrome/browser/net/predictor_api.cc (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor_api.cc:197:
PredictorInit::PredictorInit(PrefService* user_prefs,
PredictorInit() is doing 2 different things. It's initializing configuration
parameters that are tweakable via FieldTrials. It's also initializing the first
resources to prefetch for this predictor. These are two separate things and
should be broken up. See
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/browse...
for an example of how to do FieldTrial setup in browser_main.cc. That's what we
should be doing here. Initialize some global variables while Chrome is still
single threaded. Those global variables become const afterward for the lifetime
of the browser process.

Then the other part where we initialize the prefetch list, that should be part
of the initialization of Predictor.

Basically, I think we don't really need this PredictorInit stuff anymore. Most
of this should move out to BrowserMain. The small part that affects the
Predictor should move out to the Predictor object.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:569: virtual chrome_browser_net::Predictor*
GetPredictor() {
Please call this NetworkPredictor(). Predictor is too generic of a
non-namespaced name.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:573: virtual void
SetPredictor(chrome_browser_net::Predictor* predictor) {}
This shouldn't be needed either.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:578: void PredictorOnTheRecord(bool enable);
As noted elsewhere, you shouldn't need this.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:105: chrome_browser_net::Predictor*
GetPredictor();
This should move to the ProfileImplIOData class since OffTheRecordProfileIOData
does not use this.

[Miranda Callahan, 2011-07-21 14:38:04]

Thanks for this huge CL! A couple nits in the Profile code, and then I started
writing some comments about threading issues, but I see that willchan has
already written extensively about this problem.  I'll leave off reviewing until
you've addressed his comments...

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
File chrome/browser/net/predictor_api.cc (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor_api.cc:121: break;  // Ignore ordinary windows.
indent "break;

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor_api.cc:199: Profile* profile,
Not sure if this is valid after you make the changes suggested by willchan, but
if you're adding Profile* to the param list here and in InitNetworkPredictor,
you should remove user_prefs and just get them from the profile passed in when
needed.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:226: if (predictor &&
predictor->predictor_enabled()) {
Isn't there be a race condition here if you allow setting of the predictor state
on the UI thread, and accessing that state directly on the IO thread? It seems
like there should be state for the predictor on the IO thread that is set by a
task sent to that thread from the UI thread.

[rpetterson, 2011-08-10 02:24:46]

I've addressed all the comments thus far. PTAL.

Also, I'm running into an issue where it's deref'ing my predictor upon creating
a NewRunnableMethod (it does the first time, not at random times). I'm not super
familiar with how NewRunnableMethod works to create its object, so please let me
know if you see anything obviously wrong in my initialization of the Predictor.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:598: Profile* profile) {
On 2011/07/21 12:10:53, willchan wrote:
> Profile* is not allowed to be used on the IO thread. There is a potential race
> condition with shutdown here. Profile is a UI thread only object.
> 
> This code shoudl move into Predictor initialization and should be removed from
> IOThread.

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:615: if (profile->GetPredictor()) {
On 2011/07/21 12:10:53, willchan wrote:
> This should die. Once you've made the Predictor be per-Profile, and you don't
> have it for incognito, there's no danger of the Predictor getting contaminated
> with OTR data. Just kill this off.

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor.h:56: // the IO thread. Failure to do so will
result in a DCHECK at runtime.
On 2011/07/21 12:10:53, willchan wrote:
> This comment is wrong. It is used from both the UI and IO threads. Here's my
> recommendation for lifetime management and thread organization stuff. Have
> ProfileImpl construct the Predictor object (this is different from the current
> world where Predictor is constructed on the IO thread, so be a bit careful
with
> some of the member variables that expect to be initialized and destroyed on
the
> same thread, such as the ScopedRunnableMethodFactory). Use the io_data_.Init()
> call on
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...
> to pass ownership of the Predictor object to the IO thread. Have ProfileImpl
> keep a cached weak pointer to the Predictor object (this is why we created it
on
> the UI thread, so we can cache a non-owning pointer for use on the UI thread).
> Note that this is safe because the ProfileImpl is destroyed on the UI thread
> before the ProfileImplIOData is destroyed on the IO thread.
> 
> Note that ProfileIOData so far is always lazily initialized for startup
reasons
> and what not. You will want to eagerly initialize the Predictor object on the
> ProfileImplIOData. Therefore, in
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...,
> you should not use LazyParams. You should post a task to the IO thread to pass
> ownership of the Predictor object to the IO thread. You should probably add a
> InitOnIOThread() method to Predictor that gets invoked when this happens. This
> will be analogous to the existing InitNetworkPredictorOnIOThread() that exists
> in the IOThread class. Here you should initialize the member variables that
> expect to be initialize on the IO thread, such as the
> ScopedRunnableMethodFactory (it'll probably have to be wrapped in scoped_ptr).
> 
> Then please organize your methods here. Create one section for methods called
on
> the UI thread. Create another section for methods called on the IO thread.
That
> way it's clear to users what the thread expectations of the API are.

I think I've covered everything in this comment. Looking forward to your
comments on it.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
File chrome/browser/net/predictor_api.cc (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor_api.cc:121: break;  // Ignore ordinary windows.
On 2011/07/21 14:38:04, Miranda Callahan wrote:
> indent "break;

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor_api.cc:197:
PredictorInit::PredictorInit(PrefService* user_prefs,
On 2011/07/21 12:10:53, willchan wrote:
> PredictorInit() is doing 2 different things. It's initializing configuration
> parameters that are tweakable via FieldTrials. It's also initializing the
first
> resources to prefetch for this predictor. These are two separate things and
> should be broken up. See
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/browse...
> for an example of how to do FieldTrial setup in browser_main.cc. That's what
we
> should be doing here. Initialize some global variables while Chrome is still
> single threaded. Those global variables become const afterward for the
lifetime
> of the browser process.
> 
> Then the other part where we initialize the prefetch list, that should be part
> of the initialization of Predictor.
> 
> Basically, I think we don't really need this PredictorInit stuff anymore. Most
> of this should move out to BrowserMain. The small part that affects the
> Predictor should move out to the Predictor object.

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/net/predictor...
chrome/browser/net/predictor_api.cc:199: Profile* profile,
On 2011/07/21 14:38:04, Miranda Callahan wrote:
> Not sure if this is valid after you make the changes suggested by willchan,
but
> if you're adding Profile* to the param list here and in InitNetworkPredictor,
> you should remove user_prefs and just get them from the profile passed in when
> needed.

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:226: if (predictor &&
predictor->predictor_enabled()) {
On 2011/07/21 14:38:04, Miranda Callahan wrote:
> Isn't there be a race condition here if you allow setting of the predictor
state
> on the UI thread, and accessing that state directly on the IO thread? It seems
> like there should be state for the predictor on the IO thread that is set by a
> task sent to that thread from the UI thread.

I've fixed EnablePredictor to post a task to enable instead of doing it
directly.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:569: virtual chrome_browser_net::Predictor*
GetPredictor() {
On 2011/07/21 12:10:53, willchan wrote:
> Please call this NetworkPredictor(). Predictor is too generic of a
> non-namespaced name.

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:573: virtual void
SetPredictor(chrome_browser_net::Predictor* predictor) {}
On 2011/07/21 12:10:53, willchan wrote:
> This shouldn't be needed either.

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:578: void PredictorOnTheRecord(bool enable);
On 2011/07/21 12:10:53, willchan wrote:
> As noted elsewhere, you shouldn't need this.

Done.

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7467012/diff/1065/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:105: chrome_browser_net::Predictor*
GetPredictor();
On 2011/07/21 12:10:53, willchan wrote:
> This should move to the ProfileImplIOData class since
OffTheRecordProfileIOData
> does not use this.

Done.

[rpetterson, 2011-08-10 03:43:38]

Please ignore the previous question about the deref'ing. I realized the error
and have uploaded a new version with that fixed.

Thanks.

[willchan no longer on Chromium, 2011-08-10 07:04:08]

I need to go through this with a fine toothed comb, but generally speaking this
changelist is much more like what I had envisioned. Nicely done! I need to go to
bed, but I've sent some initial comments and will follow up with more tomorrow.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:737: NewRunnableMethod(this,
&Predictor::ResolveList, urls, motivation));
As I mentioned elsewhere, instead of NewRunnableMethod(), you should use
base::Bind(&Predictor::ResolveList, base::Unretained(this), urls, motivation);

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:60: class Predictor : public
base::RefCountedThreadSafe<Predictor> {
Please remove refcounting. Predictor will be destroyed when ProfileIOData gets
destroyed, so we shouldn't refcount it. I suspect you kept it around because you
use NewRunnableMethod(). Don't bother. Use base::Bind() and use
base::Unretained(predictor) to instruct base::Bind() that it doesn't need to be
refcounted. See the example at
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/callback.h&type=....

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:29: #include "chrome/browser/io_thread.h"
Is this include needed still?

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:323: predictor_->AddRef();
I don't think the AddRef() is needed since I advised elsewhere for you to get
rid of the use of base::RefCountedThreadSafe.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:660: predictor_->Shutdown();
According to the comments in predictor.h, this is an IO thread only method. I
think it has a DCHECK that it's on the IO thread. ProfileImpl gets deleted on
the UI thread. So something's off here.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.h:58:
chrome_browser_net::Predictor* GetNetworkPredictor();
Where is this used? Is it needed? If it's not, I suspect you get also get rid of
ProfileIOData::GetNetworkPredictor().

[Miranda Callahan, 2011-08-10 07:58:39]

I'm turning this review entirely over to willchan, as this is his area of
expertise -- thanks for the hard work on this extensive change!


On 2011/08/10 07:04:08, willchan wrote:
> I need to go through this with a fine toothed comb, but generally speaking
this
> changelist is much more like what I had envisioned. Nicely done! I need to go
to
> bed, but I've sent some initial comments and will follow up with more
tomorrow.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> File chrome/browser/net/predictor.cc (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> chrome/browser/net/predictor.cc:737: NewRunnableMethod(this,
> &Predictor::ResolveList, urls, motivation));
> As I mentioned elsewhere, instead of NewRunnableMethod(), you should use
> base::Bind(&Predictor::ResolveList, base::Unretained(this), urls, motivation);
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
> File chrome/browser/net/predictor.h (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> chrome/browser/net/predictor.h:60: class Predictor : public
> base::RefCountedThreadSafe<Predictor> {
> Please remove refcounting. Predictor will be destroyed when ProfileIOData gets
> destroyed, so we shouldn't refcount it. I suspect you kept it around because
you
> use NewRunnableMethod(). Don't bother. Use base::Bind() and use
> base::Unretained(predictor) to instruct base::Bind() that it doesn't need to
be
> refcounted. See the example at
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/callback.h&type=....
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile.cc (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile.cc:29: #include "chrome/browser/io_thread.h"
> Is this include needed still?
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_impl.cc:323: predictor_->AddRef();
> I don't think the AddRef() is needed since I advised elsewhere for you to get
> rid of the use of base::RefCountedThreadSafe.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_impl.cc:660: predictor_->Shutdown();
> According to the comments in predictor.h, this is an IO thread only method. I
> think it has a DCHECK that it's on the IO thread. ProfileImpl gets deleted on
> the UI thread. So something's off here.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_impl_io_data.h (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_impl_io_data.h:58:
> chrome_browser_net::Predictor* GetNetworkPredictor();
> Where is this used? Is it needed? If it's not, I suspect you get also get rid
of
> ProfileIOData::GetNetworkPredictor().

[willchan no longer on Chromium, 2011-08-10 16:10:34]

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:39: #include
"chrome/browser/net/predictor.h"
Is this needed at all? I don't see predictor stuff at a glance, but maybe it's
needed.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_shut...
File chrome/browser/browser_shutdown.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_shut...
chrome/browser/browser_shutdown.cc:42: #include "net/predictor.h"
I don't think you need this anymore.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:368:
io_thread->ClearNetworkingHistory(profile_->GetNetworkPredictor());
Please delete IOThread::ClearNetworkingHistory(). Move the
IOThread::ClearHostCache() code directly here. Then call
profile_->GetNetworkPredictor()->DiscardXXX(); here.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:384: void IOThread::InitNetworkPredictor(
Please kill off IOThread::InitNetworkPredictor(). It's only caller is Predictor,
but IOThread::InitNetworkPredictorOnIOThread() just calls back into
Predictor::FinalizeInitializationOnIOThread(). There's no reason to bounce these
calls through IOThread, just have Predictor do the PostTask internally.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
After your changes, there does not appear to be any reason for IOThread to need
to know anything about Predictor. Please remove all references here.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:17: #include "chrome/browser/profiles/profile.h"
I don't think this needs to be here.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:196: // Note: we user explicit pointers rather than
smart pointers to be more
This comment is no longer true, just delete it.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.cc (left):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:22:
net::URLRequest::Deprecated::RegisterRequestInterceptor(this);
there's a friend list in net/url_request/url_request.h, please remove the line
for this use:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/url_request/url_r....

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:36: if (predictor_)
|predictor_| should never be NULL, right? It was possible before due to
incognito, but now the incognito profile never creates a ConnectInterceptor
object. So we shouldn't need this NULL check. Please DCHECK(predictor); in the
constructor.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
File chrome/browser/net/net_pref_observer.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:8: #include
"chrome/browser/browser_process.h"
What's this for?

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:13: #include
"chrome/browser/profiles/profile_manager.h"
What's this for?

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:37: predictor_(predictor) {
Is NetPrefObserver only used in ProfileImpl which always has a Predictor? Can we
DCHECK(predictor) and not bother with NULL checks throughout the file?

[rpetterson, 2011-08-12 03:12:36]

Okay, hit all the comments. PTAL.

I'm still having an issue with the ProfileManagerTest. I've created a mock
predictor (you can see if you look at the file) but I don't know how to inject
it into where we create the profile. I've run through my list of ideas and none
of them seem to work. Ideas? Suggestions? Is there a typical way of doing this?

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:39: #include
"chrome/browser/net/predictor.h"
On 2011/08/10 16:10:35, willchan wrote:
> Is this needed at all? I don't see predictor stuff at a glance, but maybe it's
> needed.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_shut...
File chrome/browser/browser_shutdown.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_shut...
chrome/browser/browser_shutdown.cc:42: #include "net/predictor.h"
On 2011/08/10 16:10:35, willchan wrote:
> I don't think you need this anymore.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:368:
io_thread->ClearNetworkingHistory(profile_->GetNetworkPredictor());
On 2011/08/10 16:10:35, willchan wrote:
> Please delete IOThread::ClearNetworkingHistory(). Move the
> IOThread::ClearHostCache() code directly here. Then call
> profile_->GetNetworkPredictor()->DiscardXXX(); here.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:384: void IOThread::InitNetworkPredictor(
On 2011/08/10 16:10:35, willchan wrote:
> Please kill off IOThread::InitNetworkPredictor(). It's only caller is
Predictor,
> but IOThread::InitNetworkPredictorOnIOThread() just calls back into
> Predictor::FinalizeInitializationOnIOThread(). There's no reason to bounce
these
> calls through IOThread, just have Predictor do the PostTask internally.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2011/08/10 16:10:35, willchan wrote:
> After your changes, there does not appear to be any reason for IOThread to
need
> to know anything about Predictor. Please remove all references here.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:17: #include "chrome/browser/profiles/profile.h"
On 2011/08/10 16:10:35, willchan wrote:
> I don't think this needs to be here.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:196: // Note: we user explicit pointers rather than
smart pointers to be more
On 2011/08/10 16:10:35, willchan wrote:
> This comment is no longer true, just delete it.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.cc (left):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:22:
net::URLRequest::Deprecated::RegisterRequestInterceptor(this);
On 2011/08/10 16:10:35, willchan wrote:
> there's a friend list in net/url_request/url_request.h, please remove the line
> for this use:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/url_request/url_r....

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:36: if (predictor_)
On 2011/08/10 16:10:35, willchan wrote:
> |predictor_| should never be NULL, right? It was possible before due to
> incognito, but now the incognito profile never creates a ConnectInterceptor
> object. So we shouldn't need this NULL check. Please DCHECK(predictor); in the
> constructor.

Yes, you're right since we only create the connect_interceptor in the
profile_impl_io_data.

DCHECK in constructor and removed this null check and others.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
File chrome/browser/net/net_pref_observer.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:8: #include
"chrome/browser/browser_process.h"
On 2011/08/10 16:10:35, willchan wrote:
> What's this for?

No longer necessary. Removed.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:13: #include
"chrome/browser/profiles/profile_manager.h"
On 2011/08/10 16:10:35, willchan wrote:
> What's this for?

No longer necessary. Removed.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:37: predictor_(predictor) {
On 2011/08/10 16:10:35, willchan wrote:
> Is NetPrefObserver only used in ProfileImpl which always has a Predictor? Can
we
> DCHECK(predictor) and not bother with NULL checks throughout the file?

NetPrefObserver::RegisterPrefs is called from browser_prefs but that doesn't
touch the predictor, so I've updated with DCHECK.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:737: NewRunnableMethod(this,
&Predictor::ResolveList, urls, motivation));
On 2011/08/10 07:04:08, willchan wrote:
> As I mentioned elsewhere, instead of NewRunnableMethod(), you should use
> base::Bind(&Predictor::ResolveList, base::Unretained(this), urls, motivation);

Done. Elsewhere as well.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:60: class Predictor : public
base::RefCountedThreadSafe<Predictor> {
On 2011/08/10 07:04:08, willchan wrote:
> Please remove refcounting. Predictor will be destroyed when ProfileIOData gets
> destroyed, so we shouldn't refcount it. I suspect you kept it around because
you
> use NewRunnableMethod(). Don't bother. Use base::Bind() and use
> base::Unretained(predictor) to instruct base::Bind() that it doesn't need to
be
> refcounted. See the example at
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/callback.h&type=....

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:29: #include "chrome/browser/io_thread.h"
On 2011/08/10 07:04:08, willchan wrote:
> Is this include needed still?

Nope. Removed.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:323: predictor_->AddRef();
On 2011/08/10 07:04:08, willchan wrote:
> I don't think the AddRef() is needed since I advised elsewhere for you to get
> rid of the use of base::RefCountedThreadSafe.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:660: predictor_->Shutdown();
On 2011/08/10 07:04:08, willchan wrote:
> According to the comments in predictor.h, this is an IO thread only method. I
> think it has a DCHECK that it's on the IO thread. ProfileImpl gets deleted on
> the UI thread. So something's off here.

Thanks for catching this. Since the profile doesn't even own the predictor any
more, I've moved it's shutdown to the ProfileIOData dtor which should provide
the same effect of saving the information before calling the predictor's dtor.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.h:58:
chrome_browser_net::Predictor* GetNetworkPredictor();


On 2011/08/10 07:04:08, willchan wrote:
> Where is this used? Is it needed? If it's not, I suspect you get also get rid
of
> ProfileIOData::GetNetworkPredictor().

You're right. Now that ProfileImpl has a weak pointer to the profile it's not
necessary.

[willchan no longer on Chromium, 2011-08-12 21:17:29]

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:60: request_scheme_host);
indent is off

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:70: request_scheme_host);
indent is off

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.h (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.h:9: #include
"net/url_request/url_request_job_factory.h"
This header shouldn't be first, it should be sorted alphabetically.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.h:77: Predictor* predictor_;
I'm a bit of a const nazi, can you make this Predictor* const predictor_?

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:220: // Given that the underlying Chromium
resolver defaults to a total maximum of
constants go above methods in class declaration order:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:244: friend class
base::RefCountedThreadSafe<Predictor>;
Remove this

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:195:
ProfileImplIOData::~ProfileImplIOData() {
The rest of the file uses {}, can you keep it like {} to be consistent?

[willchan no longer on Chromium, 2011-08-12 21:19:21]

On 2011/08/12 03:12:36, rpetterson wrote:
> Okay, hit all the comments. PTAL.
> 
> I'm still having an issue with the ProfileManagerTest. I've created a mock
> predictor (you can see if you look at the file) but I don't know how to inject
> it into where we create the profile. I've run through my list of ideas and
none
> of them seem to work. Ideas? Suggestions? Is there a typical way of doing
this?


I think ProfileManagerTest is testing more than it should. There's no need for
it to do a test of ProfileImpl initialization and destruction. It should just be
focusing on ProfileManager and use mock profile objects. It doesn't support
dependency, but I think it should be refactored to support this. I defer to
Miranda here.

[willchan no longer on Chromium, 2011-08-12 21:51:46]

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:856: if
(profile_->GetNetworkPredictor())
Nit: please use braces for multiline if statements.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_browsertest.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_browsertest.cc:15: #include
"chrome/browser/net/predictor.h"
Can this be deleted now?

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:20: #include "chrome/browser/browser_process.h"
Please eliminate the need for this include by removing g_browser_process.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:27: #include "chrome/browser/profiles/profile.h"
Is this header needed?

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:83: static int g_max_queueing_delay_ms;
Please initialize this

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:146: initial_observer_.reset(new
InitialObserver());
Can this be initialized in the initializer list?

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:148: const CommandLine* command_line =
CommandLine::ForCurrentProcess();
As I mentioned earlier, I prefer explicitly passing in dependencies like this.
Please take a bool parameter in the constructor to control whether or not this
is disabled.

Btw, which is supposed to have precedence? Command line flag or preference?
Perhaps you can ask jar@? I suspect a command line flag has precedence.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:155: Predictor::~Predictor() {
Please DCHECK BrowserThread::IO

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:161: void
Predictor::InitNetworkPredictor(PrefService* user_prefs) {
Please take PrefService* local_state and IOThread* as parameters. I prefer to
push global variables as close to BrowserMain() as possible and have the objects
get passed in as explicit dependencies. This makes dependency injection for
testing much easier, since that way you don't have to mock g_browser_process in
tests.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:287: void
Predictor::PredictFrameSubresources(const GURL& url) {
Please DCHECK BrowserThread::UI

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:356: void Predictor::set_max_queueing_delay(int
max_queueing_delay_ms) {
Please DCHECK BrowserThread::UI

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:360: void
Predictor::set_max_parallel_resolves(size_t max_parallel_resolves) {
Please DCHECK BrowserThread::UI

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:365: void
Predictor::RegisterUserPrefs(PrefService* user_prefs) {
Please DCHECK BrowserThread::UI

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:59: // the IO thread. Failure to do so will
result in a DCHECK at runtime.
You should update this comment. You should have something like this:

Predictor is constructed during Profile construction, but is destroyed on the IO
thread when ProfileIOData goes away. All of its core state and functionality
happens on the IO thread. The only UI methods are initialization / shutdown
related, or convenience methods that internally forward calls to the IO thread.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
File chrome/browser/net/predictor_unittest.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor_unittest.cc:97: void TearDown() {
Just delete this

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor_unittest.cc:115: scoped_ptr<Predictor>
testing_master(
Just do Predictor testing_master;
Same elsewhere.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.h:19: explicit
ChromeRenderViewHostObserver(
Doesn't need to be explicit anymore.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:671: if (profile->GetNetworkPredictor())
How about && instead of nesting another if?

[rpetterson, 2011-08-13 00:55:17]

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:856: if
(profile_->GetNetworkPredictor())
On 2011/08/12 21:51:47, willchan wrote:
> Nit: please use braces for multiline if statements.

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_browsertest.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_browsertest.cc:15: #include
"chrome/browser/net/predictor.h"
On 2011/08/12 21:51:47, willchan wrote:
> Can this be deleted now?

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:60: request_scheme_host);
On 2011/08/12 21:17:29, willchan wrote:
> indent is off

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.cc:70: request_scheme_host);
On 2011/08/12 21:17:29, willchan wrote:
> indent is off

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.h (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.h:9: #include
"net/url_request/url_request_job_factory.h"
On 2011/08/12 21:17:29, willchan wrote:
> This header shouldn't be first, it should be sorted alphabetically.

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.h:77: Predictor* predictor_;
On 2011/08/12 21:17:29, willchan wrote:
> I'm a bit of a const nazi, can you make this Predictor* const predictor_?

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:20: #include "chrome/browser/browser_process.h"
On 2011/08/12 21:51:47, willchan wrote:
> Please eliminate the need for this include by removing g_browser_process.

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:27: #include "chrome/browser/profiles/profile.h"
On 2011/08/12 21:51:47, willchan wrote:
> Is this header needed?

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:83: static int g_max_queueing_delay_ms;
On 2011/08/12 21:51:47, willchan wrote:
> Please initialize this

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:146: initial_observer_.reset(new
InitialObserver());
On 2011/08/12 21:51:47, willchan wrote:
> Can this be initialized in the initializer list?

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:148: const CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2011/08/12 21:51:47, willchan wrote:
> As I mentioned earlier, I prefer explicitly passing in dependencies like this.
> Please take a bool parameter in the constructor to control whether or not this
> is disabled.
> 
> Btw, which is supposed to have precedence? Command line flag or preference?
> Perhaps you can ask jar@? I suspect a command line flag has precedence.

Checked with jar -- command line takes precedence. Now passed in as parameter.
Although I don't think we ever check user prefs for preconnect, just for
predictor in general.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:155: Predictor::~Predictor() {
On 2011/08/12 21:51:47, willchan wrote:
> Please DCHECK BrowserThread::IO

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:161: void
Predictor::InitNetworkPredictor(PrefService* user_prefs) {
On 2011/08/12 21:51:47, willchan wrote:
> Please take PrefService* local_state and IOThread* as parameters. I prefer to
> push global variables as close to BrowserMain() as possible and have the
objects
> get passed in as explicit dependencies. This makes dependency injection for
> testing much easier, since that way you don't have to mock g_browser_process
in
> tests.

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:287: void
Predictor::PredictFrameSubresources(const GURL& url) {
On 2011/08/12 21:51:47, willchan wrote:
> Please DCHECK BrowserThread::UI

This is not always called from the UI thread. For example, ConnectInterceptor it
is called from the IO thread. I've moved it to that section of the header file
and updated the function to handle being called from different threads in the
same way similar functions do.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:356: void Predictor::set_max_queueing_delay(int
max_queueing_delay_ms) {
On 2011/08/12 21:51:47, willchan wrote:
> Please DCHECK BrowserThread::UI

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:360: void
Predictor::set_max_parallel_resolves(size_t max_parallel_resolves) {
On 2011/08/12 21:51:47, willchan wrote:
> Please DCHECK BrowserThread::UI

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:365: void
Predictor::RegisterUserPrefs(PrefService* user_prefs) {
On 2011/08/12 21:51:47, willchan wrote:
> Please DCHECK BrowserThread::UI

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:59: // the IO thread. Failure to do so will
result in a DCHECK at runtime.
On 2011/08/12 21:51:47, willchan wrote:
> You should update this comment. You should have something like this:
> 
> Predictor is constructed during Profile construction, but is destroyed on the
IO
> thread when ProfileIOData goes away. All of its core state and functionality
> happens on the IO thread. The only UI methods are initialization / shutdown
> related, or convenience methods that internally forward calls to the IO
thread.

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:220: // Given that the underlying Chromium
resolver defaults to a total maximum of
On 2011/08/12 21:17:29, willchan wrote:
> constants go above methods in class declaration order:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:244: friend class
base::RefCountedThreadSafe<Predictor>;
On 2011/08/12 21:17:29, willchan wrote:
> Remove this

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
File chrome/browser/net/predictor_unittest.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor_unittest.cc:97: void TearDown() {
On 2011/08/12 21:51:47, willchan wrote:
> Just delete this

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/net/predicto...
chrome/browser/net/predictor_unittest.cc:115: scoped_ptr<Predictor>
testing_master(
On 2011/08/12 21:51:47, willchan wrote:
> Just do Predictor testing_master;
> Same elsewhere.

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:195:
ProfileImplIOData::~ProfileImplIOData() {
On 2011/08/12 21:17:29, willchan wrote:
> The rest of the file uses {}, can you keep it like {} to be consistent?

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.h:19: explicit
ChromeRenderViewHostObserver(
On 2011/08/12 21:51:47, willchan wrote:
> Doesn't need to be explicit anymore.

Done.

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/7467012/diff/41004/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:671: if (profile->GetNetworkPredictor())
On 2011/08/12 21:51:47, willchan wrote:
> How about && instead of nesting another if?

Done.

[willchan no longer on Chromium, 2011-08-16 00:16:51]

I think we're pretty close now! There's just the issue of the
profile_manager_unittest.cc.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:716:
source_->profile()->PredictorGetHtmlInfo(&data);
I believe this is a bug. profile() should only be accessible on the UI thread.
Perhaps what you need to do is make GetHtmlInfo() a member function of
Predictor. In StartOnUIThread(), acquire a pointer to Predictor and post it to
the IO thread, get the data there, and then post back to the UI thread.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:376: delete *it;
Just use STLDeleteElements(&pending_lookups_);, it's in base/stl_util-inl.h.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:409: DCHECK(assignees.size() <=
max_concurrent_dns_lookups_);
I know you didn't write this, but while you're here: DCHECK_LE instead.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:680: // ScopedrunnableMethodFactory instances
need to be created and destroyed
nit: ScopedRunnableMethodFactory (capitalize the 'R')

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:128: virtual void Shutdown();
Are all these virtuals necessary still? They're used for the mock Predictor
right? Are you actually using that? I think we need a mock Profile instead, that
can be injected into ProfileManager.

[jar (doing other things), 2011-08-16 01:19:09]

The flow and trust we're putting in the Unretained bind probably needs to be
documented.

I'm a bit wary about when the pre-resolution will be done on startup.  We may
want to histogram when it is run, relative to process start, and if it is too
late, we could simplify a lot by abandoning that feature (or figuring out how to
make it happen sooner).

IMO, this is a sizeable landing.  It would be good to get a lot of bake time on
canaries before the next M-stone pulls it to Dev... so I appreciate the rushing
you're doing to get this landed well before the next Dev fork.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (left):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1861: chrome_browser_net::PredictorInit
dns_prefetch(
This historically has a side effect of starting to pre-resolve the domains which
will probably be used at startup.  Since startup takes a few hundred
milliseconds, it is nice to overlay this DNS stuff (which can take a few hundred
milliseconds!).
Please be sure your new initialization as as aggressively early in the startup
as possible.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (left):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:602: predictor_ = new chrome_browser_net::Predictor(
Code like this is what caused us to have such a method.  This is where
predictor_ was constructed.  Now the method takes a constructed predictor as an
argument... so the name has to change... or else we should let this method do
its work  to constract and return, or save, or post the new predictor.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:567:
predictor->FinalizeInitializationOnIOThread(startup_urls, referral_list,
This method name should be reflected in the name of the method on line 561... or
perhaps we should not even bother to pass through the IOThread class, as it does
not (currently) appear that the class is benefiting (recording values or
pointers as they pass through?) from this indirection.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:114: chrome_browser_net::Predictor* predictor);
This method name is now confusing.  It appears that you pass in a predictor...
but the method suggests we initialize it.  IN the past, it really constructed
the essences of it (all statics).  Now presumably it will better populate the
|predictor| it is given.  That sure sounds like it should be a Predictor method.
 Alternatively, should this be a request for a construction of a Predictor? 

...or does the method name and comment need to change?

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.h:56: bool WasRecentlySeen(const GURL&
url) const;
Surprising to see const on a method that says it impacts the instance.  My guess
is that if it can be const, it is an artifact of the implementation, and should
not be exposed.

Was there some context where we really needed const (ref) instances, and you
wanted to allow that, ended up with the mutable declaration on line 69?

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:145: initial_observer_.reset(new
InitialObserver());
Other code (for finalizing initialization) suggest that this constructor is not
run on the IO thread (and that is why we need to "wait" to finish initializing).
 Is it safe to construct an InitialObserver here then?  ... or should that wait
for the IO thread finalization of init??

If this is always run on the UI thread, it would be good to DCHECK it to make
the code clearer.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:60: class Predictor : public
base::RefCountedThreadSafe<Predictor> {
WillChan: I'm trying to be sure I understand the pattern you proposed:
Are you counting on the fact that a posted message to the IO thread will be
dropped when the IO thread goes away... and after the IO thread goes away,
then(?) ProfileIOData will be destroyed... so Predictor will live "long enough"
to service any posts?

On 2011/08/10 07:04:08, willchan wrote:
> Please remove refcounting. Predictor will be destroyed when ProfileIOData gets
> destroyed, so we shouldn't refcount it. I suspect you kept it around because
you
> use NewRunnableMethod(). Don't bother. Use base::Bind() and use
> base::Unretained(predictor) to instruct base::Bind() that it doesn't need to
be
> refcounted. See the example at
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/callback.h&type=....

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:89: void PredictFrameSubresources(const GURL&
url);
I'm surprised this happens on the UI thread.  I'd expect it to happen when an
observer on the IO thread sees a navigation.

Is this instead the entry point used when a user enters an URL in the Omnibox,
and we don't want to wait for the renderer to ack it and get going??

I suspect the posting of the task meant we *could* call it on the UI thread (and
perhaps PreconnectUrlAndSubresources() uses it?)... but I think it is also
called on the IO thread.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:236: // avoidance will kick in and all
speculations in the queue will be discarded.
nit: indent comments.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:447: bool preconnect_enabled_;
I don't think we really support this feature any more (pre-resolution, without
pre-connection support)... and perhaps you could kill it and simplify some code.

[willchan no longer on Chromium, 2011-08-16 01:42:57]

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (left):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1861: chrome_browser_net::PredictorInit
dns_prefetch(
On 2011/08/16 01:19:09, jar wrote:
> This historically has a side effect of starting to pre-resolve the domains
which
> will probably be used at startup.  Since startup takes a few hundred
> milliseconds, it is nice to overlay this DNS stuff (which can take a few
hundred
> milliseconds!).
> Please be sure your new initialization as as aggressively early in the startup
> as possible.

I think that the new code will actually invoke this even earlier! That's because
everything is moved into the Profile initialization, which is initializing the
Predictor object. CreateProfile() is called on line 1815, which is before this,
so this sounds like we'll have the nice side effect of beginning DNS prefetch
even earlier on in initialization.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:567:
predictor->FinalizeInitializationOnIOThread(startup_urls, referral_list,
On 2011/08/16 01:19:09, jar wrote:
> This method name should be reflected in the name of the method on line 561...
or
> perhaps we should not even bother to pass through the IOThread class, as it
does
> not (currently) appear that the class is benefiting (recording values or
> pointers as they pass through?) from this indirection.

I think I may have commented on this before (I hope I did, hehe). Yes, this
indirection is unnecessary. We should remove the call to IOThread and just have
the predictor post a task to BrowserThread::IO directly.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:60: class Predictor : public
base::RefCountedThreadSafe<Predictor> {
On 2011/08/16 01:19:09, jar wrote:
> WillChan: I'm trying to be sure I understand the pattern you proposed:
> Are you counting on the fact that a posted message to the IO thread will be
> dropped when the IO thread goes away... and after the IO thread goes away,
> then(?) ProfileIOData will be destroyed... so Predictor will live "long
enough"
> to service any posts?

Not quite. Shutdown is confusing, but here's how it works. Predictor will get
destroyed when ProfileIOData goes away. The event order is:
* On the UI (main) thread, MessageLoop::Run() returns. We destroy
g_browser_process, which leads to ProfileManager getting destroyed and
destroying all Profile instances.
* Destroying the Profile will cause the member variable
ProfileImplIOData::Handle to get destroyed, which will post a DeleteTask to the
IO thread to delete the ProfileIOData. Predictor gets destroyed when
ProfileIOData does.

Since ProfileIOData strictly gets destroyed *after* the Profile does, it should
be impossible for any tasks to get posted to the Predictor on the IO thread
after it has been destroyed, since those tasks should only be generated by
objects associated with the Profile. If there are still global references to
Predictor, then it's a bug. If an object that is tied to a Profile does not get
destroyed when Profile goes away (translation: it is leaked), it's a bug.

Let me know if that lifetime management makes sense. Chrome shutdown is
confusing so I suspect this won't immediately make sense to folks, but it's the
paradigm we've been switching to and has been working so far.

> 
> On 2011/08/10 07:04:08, willchan wrote:
> > Please remove refcounting. Predictor will be destroyed when ProfileIOData
gets
> > destroyed, so we shouldn't refcount it. I suspect you kept it around because
> you
> > use NewRunnableMethod(). Don't bother. Use base::Bind() and use
> > base::Unretained(predictor) to instruct base::Bind() that it doesn't need to
> be
> > refcounted. See the example at
> >
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/callback.h&type=....
>

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:89: void PredictFrameSubresources(const GURL&
url);
On 2011/08/16 01:19:09, jar wrote:
> I'm surprised this happens on the UI thread.  I'd expect it to happen when an
> observer on the IO thread sees a navigation.

It seems to be a translation of the existing code, which seems to get called on
the UI thread and post to the IO thread. If the translation is incorrect, let's
definitely fix it. I defer to your expertise here Jim.

> 
> Is this instead the entry point used when a user enters an URL in the Omnibox,
> and we don't want to wait for the renderer to ack it and get going??
> 
> I suspect the posting of the task meant we *could* call it on the UI thread
(and
> perhaps PreconnectUrlAndSubresources() uses it?)... but I think it is also
> called on the IO thread.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:447: bool preconnect_enabled_;
On 2011/08/16 01:19:09, jar wrote:
> I don't think we really support this feature any more (pre-resolution, without
> pre-connection support)... and perhaps you could kill it and simplify some
code.

I would need to reread the code, but I think this member variable's meaning has
changed, and might just indicate whether or not network prediction is
enabled/disabled (due to user preferences in chrome://settings, or to enterprise
policy settings). Maybe the comment should be updated and the variable renamed
to |enabled_|.

Please verify my statement, I'm not 100% sure.

[rpetterson, 2011-08-16 03:52:10]

@jar: I think @willchan responded to many of your concerns. I've also added my
thoughts and responded to the others (below). Also, could you be more specific
about what you mean about documenting the flow and trust we're putting in
Unretained bind?

@willchan: I've hit all of your most recent comments. I would like to discuss
with @mirandac before making a final decision on the ProfileManagerTest.

@mirandac: waiting to hear back thoughts on ProfileManagerTest.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:567:
predictor->FinalizeInitializationOnIOThread(startup_urls, referral_list,
On 2011/08/16 01:42:57, willchan wrote:
> On 2011/08/16 01:19:09, jar wrote:
> > This method name should be reflected in the name of the method on line
561...
> or
> > perhaps we should not even bother to pass through the IOThread class, as it
> does
> > not (currently) appear that the class is benefiting (recording values or
> > pointers as they pass through?) from this indirection.
> 
> I think I may have commented on this before (I hope I did, hehe). Yes, this
> indirection is unnecessary. We should remove the call to IOThread and just
have
> the predictor post a task to BrowserThread::IO directly.

Yes, this has already been pulled out.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
File chrome/browser/net/connect_interceptor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
chrome/browser/net/connect_interceptor.h:56: bool WasRecentlySeen(const GURL&
url) const;
On 2011/08/16 01:19:09, jar wrote:
> Surprising to see const on a method that says it impacts the instance.  My
guess
> is that if it can be const, it is an artifact of the implementation, and
should
> not be exposed.
> 
> Was there some context where we really needed const (ref) instances, and you
> wanted to allow that, ended up with the mutable declaration on line 69?

Yes, the new pattern is to sublass off of the UrlRequestFactory::Interceptor and
all of its methods are const. This seems to be the only case where we are
subclassing from it and run into this situation. The alternative was to alter
UrlRequestFactory::Interceptor which would would be a much more invasive change
so I opted to go with the mutable option you noted.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:145: initial_observer_.reset(new
InitialObserver());
On 2011/08/16 01:19:09, jar wrote:
> Other code (for finalizing initialization) suggest that this constructor is
not
> run on the IO thread (and that is why we need to "wait" to finish
initializing).
>  Is it safe to construct an InitialObserver here then?  ... or should that
wait
> for the IO thread finalization of init??
> 
> If this is always run on the UI thread, it would be good to DCHECK it to make
> the code clearer.

I've added a DCHECK that the Predictor constructor runs on the UI thread.

I've also bumped the creation of the InitialObserver to the FinalizeOnIOThread
method. You're correct that we should do that on the IOThread to prevent calling
a method (all of which are on the IOThread) on it before it's been initialized.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:89: void PredictFrameSubresources(const GURL&
url);
On 2011/08/16 01:42:57, willchan wrote:
> On 2011/08/16 01:19:09, jar wrote:
> > I'm surprised this happens on the UI thread.  I'd expect it to happen when
an
> > observer on the IO thread sees a navigation.
> 
> It seems to be a translation of the existing code, which seems to get called
on
> the UI thread and post to the IO thread. If the translation is incorrect,
let's
> definitely fix it. I defer to your expertise here Jim.
> 
> > 
> > Is this instead the entry point used when a user enters an URL in the
Omnibox,
> > and we don't want to wait for the renderer to ack it and get going??
> > 
> > I suspect the posting of the task meant we *could* call it on the UI thread
> (and
> > perhaps PreconnectUrlAndSubresources() uses it?)... but I think it is also
> > called on the IO thread.
> 

This function is called in two places that I know of:
- on the IO thread from ConnectInterceptor::MaybeIntercept(..)
- on the UI thread from Predictor::PreconnectUrlAndSubresources(..) (which is
called from ChromeRenderViewHostObserver::Navigate)

Please let me know if either of these are in error.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:236: // avoidance will kick in and all
speculations in the queue will be discarded.
On 2011/08/16 01:19:09, jar wrote:
> nit: indent comments.

Done.

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:447: bool preconnect_enabled_;
On 2011/08/16 01:42:57, willchan wrote:
> On 2011/08/16 01:19:09, jar wrote:
> > I don't think we really support this feature any more (pre-resolution,
without
> > pre-connection support)... and perhaps you could kill it and simplify some
> code.
> 
> I would need to reread the code, but I think this member variable's meaning
has
> changed, and might just indicate whether or not network prediction is
> enabled/disabled (due to user preferences in chrome://settings, or to
enterprise
> policy settings). Maybe the comment should be updated and the variable renamed
> to |enabled_|.
> 
> Please verify my statement, I'm not 100% sure.

Right now the code definitely looks for the preconnect_enabled flag. This can
apparently still be set via the command line so well be taking away that control
as well if we kill it. Let me know.

Will, the variable you're referring to is predictor_enabled_ which is used for
user preferences about doing network prediction at all.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:716:
source_->profile()->PredictorGetHtmlInfo(&data);
On 2011/08/16 00:16:51, willchan wrote:
> I believe this is a bug. profile() should only be accessible on the UI thread.
> Perhaps what you need to do is make GetHtmlInfo() a member function of
> Predictor. In StartOnUIThread(), acquire a pointer to Predictor and post it to
> the IO thread, get the data there, and then post back to the UI thread.

Done.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
File chrome/browser/net/predictor.cc (right):

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:376: delete *it;
On 2011/08/16 00:16:51, willchan wrote:
> Just use STLDeleteElements(&pending_lookups_);, it's in base/stl_util-inl.h.

Done.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:409: DCHECK(assignees.size() <=
max_concurrent_dns_lookups_);
On 2011/08/16 00:16:51, willchan wrote:
> I know you didn't write this, but while you're here: DCHECK_LE instead.

Done.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.cc:680: // ScopedrunnableMethodFactory instances
need to be created and destroyed
On 2011/08/16 00:16:51, willchan wrote:
> nit: ScopedRunnableMethodFactory (capitalize the 'R')

Done.

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:128: virtual void Shutdown();
On 2011/08/16 00:16:51, willchan wrote:
> Are all these virtuals necessary still? They're used for the mock Predictor
> right? Are you actually using that? I think we need a mock Profile instead,
that
> can be injected into ProfileManager.

Waiting on mirandac for the final work on the ProfileManagerTest. If we inject
the Profile, I'll remove these virtuals.

[Miranda Callahan, 2011-08-16 08:23:37]

On 2011/08/16 03:52:10, rpetterson wrote:
> @jar: I think @willchan responded to many of your concerns. I've also added my
> thoughts and responded to the others (below). Also, could you be more specific
> about what you mean about documenting the flow and trust we're putting in
> Unretained bind?
> 
> @willchan: I've hit all of your most recent comments. I would like to discuss
> with @mirandac before making a final decision on the ProfileManagerTest.
> 
> @mirandac: waiting to hear back thoughts on ProfileManagerTest.
> 
> http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/io_thread.cc...
> chrome/browser/io_thread.cc:567:
> predictor->FinalizeInitializationOnIOThread(startup_urls, referral_list,
> On 2011/08/16 01:42:57, willchan wrote:
> > On 2011/08/16 01:19:09, jar wrote:
> > > This method name should be reflected in the name of the method on line
> 561...
> > or
> > > perhaps we should not even bother to pass through the IOThread class, as
it
> > does
> > > not (currently) appear that the class is benefiting (recording values or
> > > pointers as they pass through?) from this indirection.
> > 
> > I think I may have commented on this before (I hope I did, hehe). Yes, this
> > indirection is unnecessary. We should remove the call to IOThread and just
> have
> > the predictor post a task to BrowserThread::IO directly.
> 
> Yes, this has already been pulled out.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
> File chrome/browser/net/connect_interceptor.h (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/connect_...
> chrome/browser/net/connect_interceptor.h:56: bool WasRecentlySeen(const GURL&
> url) const;
> On 2011/08/16 01:19:09, jar wrote:
> > Surprising to see const on a method that says it impacts the instance.  My
> guess
> > is that if it can be const, it is an artifact of the implementation, and
> should
> > not be exposed.
> > 
> > Was there some context where we really needed const (ref) instances, and you
> > wanted to allow that, ended up with the mutable declaration on line 69?
> 
> Yes, the new pattern is to sublass off of the UrlRequestFactory::Interceptor
and
> all of its methods are const. This seems to be the only case where we are
> subclassing from it and run into this situation. The alternative was to alter
> UrlRequestFactory::Interceptor which would would be a much more invasive
change
> so I opted to go with the mutable option you noted.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> File chrome/browser/net/predictor.cc (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> chrome/browser/net/predictor.cc:145: initial_observer_.reset(new
> InitialObserver());
> On 2011/08/16 01:19:09, jar wrote:
> > Other code (for finalizing initialization) suggest that this constructor is
> not
> > run on the IO thread (and that is why we need to "wait" to finish
> initializing).
> >  Is it safe to construct an InitialObserver here then?  ... or should that
> wait
> > for the IO thread finalization of init??
> > 
> > If this is always run on the UI thread, it would be good to DCHECK it to
make
> > the code clearer.
> 
> I've added a DCHECK that the Predictor constructor runs on the UI thread.
> 
> I've also bumped the creation of the InitialObserver to the FinalizeOnIOThread
> method. You're correct that we should do that on the IOThread to prevent
calling
> a method (all of which are on the IOThread) on it before it's been
initialized.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
> File chrome/browser/net/predictor.h (right):
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> chrome/browser/net/predictor.h:89: void PredictFrameSubresources(const GURL&
> url);
> On 2011/08/16 01:42:57, willchan wrote:
> > On 2011/08/16 01:19:09, jar wrote:
> > > I'm surprised this happens on the UI thread.  I'd expect it to happen when
> an
> > > observer on the IO thread sees a navigation.
> > 
> > It seems to be a translation of the existing code, which seems to get called
> on
> > the UI thread and post to the IO thread. If the translation is incorrect,
> let's
> > definitely fix it. I defer to your expertise here Jim.
> > 
> > > 
> > > Is this instead the entry point used when a user enters an URL in the
> Omnibox,
> > > and we don't want to wait for the renderer to ack it and get going??
> > > 
> > > I suspect the posting of the task meant we *could* call it on the UI
thread
> > (and
> > > perhaps PreconnectUrlAndSubresources() uses it?)... but I think it is also
> > > called on the IO thread.
> > 
> 
> This function is called in two places that I know of:
> - on the IO thread from ConnectInterceptor::MaybeIntercept(..)
> - on the UI thread from Predictor::PreconnectUrlAndSubresources(..) (which is
> called from ChromeRenderViewHostObserver::Navigate)
> 
> Please let me know if either of these are in error.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> chrome/browser/net/predictor.h:236: // avoidance will kick in and all
> speculations in the queue will be discarded.
> On 2011/08/16 01:19:09, jar wrote:
> > nit: indent comments.
> 
> Done.
> 
>
http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
> chrome/browser/net/predictor.h:447: bool preconnect_enabled_;
> On 2011/08/16 01:42:57, willchan wrote:
> > On 2011/08/16 01:19:09, jar wrote:
> > > I don't think we really support this feature any more (pre-resolution,
> without
> > > pre-connection support)... and perhaps you could kill it and simplify some
> > code.
> > 
> > I would need to reread the code, but I think this member variable's meaning
> has
> > changed, and might just indicate whether or not network prediction is
> > enabled/disabled (due to user preferences in chrome://settings, or to
> enterprise
> > policy settings). Maybe the comment should be updated and the variable
renamed
> > to |enabled_|.
> > 
> > Please verify my statement, I'm not 100% sure.
> 
> Right now the code definitely looks for the preconnect_enabled flag. This can
> apparently still be set via the command line so well be taking away that
control
> as well if we kill it. Let me know.
> 
> Will, the variable you're referring to is predictor_enabled_ which is used for
> user preferences about doing network prediction at all.
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/browser_abou...
> File chrome/browser/browser_about_handler.cc (right):
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/browser_abou...
> chrome/browser/browser_about_handler.cc:716:
> source_->profile()->PredictorGetHtmlInfo(&data);
> On 2011/08/16 00:16:51, willchan wrote:
> > I believe this is a bug. profile() should only be accessible on the UI
thread.
> > Perhaps what you need to do is make GetHtmlInfo() a member function of
> > Predictor. In StartOnUIThread(), acquire a pointer to Predictor and post it
to
> > the IO thread, get the data there, and then post back to the UI thread.
> 
> Done.
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
> File chrome/browser/net/predictor.cc (right):
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
> chrome/browser/net/predictor.cc:376: delete *it;
> On 2011/08/16 00:16:51, willchan wrote:
> > Just use STLDeleteElements(&pending_lookups_);, it's in base/stl_util-inl.h.
> 
> Done.
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
> chrome/browser/net/predictor.cc:409: DCHECK(assignees.size() <=
> max_concurrent_dns_lookups_);
> On 2011/08/16 00:16:51, willchan wrote:
> > I know you didn't write this, but while you're here: DCHECK_LE instead.
> 
> Done.
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
> chrome/browser/net/predictor.cc:680: // ScopedrunnableMethodFactory instances
> need to be created and destroyed
> On 2011/08/16 00:16:51, willchan wrote:
> > nit: ScopedRunnableMethodFactory (capitalize the 'R')
> 
> Done.
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predictor.h
> File chrome/browser/net/predictor.h (right):
> 
>
http://codereview.chromium.org/7467012/diff/43052/chrome/browser/net/predicto...
> chrome/browser/net/predictor.h:128: virtual void Shutdown();
> On 2011/08/16 00:16:51, willchan wrote:
> > Are all these virtuals necessary still? They're used for the mock Predictor
> > right? Are you actually using that? I think we need a mock Profile instead,
> that
> > can be injected into ProfileManager.
> 
> Waiting on mirandac for the final work on the ProfileManagerTest. If we inject
> the Profile, I'll remove these virtuals.

Agree with willchan about injecting a mock Profile - please shoot me an email if
you have any questions about how best to do this!

[jar (doing other things), 2011-08-16 18:45:22]

comments directed to Will about the change from using method factories to using
base::Unretained()

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:60: class Predictor : public
base::RefCountedThreadSafe<Predictor> {
I'm vaguely convinced that the shutdown order is (currently) as you specified.  

I think depending on such complex global ordering to ensure safety is a bad
pattern. I much prefer the readability of revocable method factories that allow
a very narrow (local) read of the code to assure correctness.  Heaven help us if
someone tries to copy this pattern to another context, without full
understanding of its extensive justifications!

I REALLY like the known global ordering, and the ability to depend on it to
write rational code that should shutdown deterministically.  I like the fact
that you're talking about this ordering.  

However... I don't like depending on global assertions from many distinct sites,
as there is no way to a) assert correctness locally; b) list all the
dependendencies on the ordering (in case we ever wanted to change it!).  With
intricate global ordering dependencies, ordering can't be changed, and is
fragile, with many distant hidden dependencies.

Bottom line: I don't think we should be advocating for this pattern.  I'm sure
there will be performance critical places where we'll have to... but it is IMO a
bad generic pattern.

Am I misunderstanding something??

On 2011/08/16 01:42:57, willchan wrote:
> On 2011/08/16 01:19:09, jar wrote:
> > WillChan: I'm trying to be sure I understand the pattern you proposed:
> > Are you counting on the fact that a posted message to the IO thread will be
> > dropped when the IO thread goes away... and after the IO thread goes away,
> > then(?) ProfileIOData will be destroyed... so Predictor will live "long
> enough"
> > to service any posts?
> 
> Not quite. Shutdown is confusing, but here's how it works. Predictor will get
> destroyed when ProfileIOData goes away. The event order is:
> * On the UI (main) thread, MessageLoop::Run() returns. We destroy
> g_browser_process, which leads to ProfileManager getting destroyed and
> destroying all Profile instances.
> * Destroying the Profile will cause the member variable
> ProfileImplIOData::Handle to get destroyed, which will post a DeleteTask to
the
> IO thread to delete the ProfileIOData. Predictor gets destroyed when
> ProfileIOData does.
> 
> Since ProfileIOData strictly gets destroyed *after* the Profile does, it
should
> be impossible for any tasks to get posted to the Predictor on the IO thread
> after it has been destroyed, since those tasks should only be generated by
> objects associated with the Profile. If there are still global references to
> Predictor, then it's a bug. If an object that is tied to a Profile does not
get
> destroyed when Profile goes away (translation: it is leaked), it's a bug.
> 
> Let me know if that lifetime management makes sense. Chrome shutdown is
> confusing so I suspect this won't immediately make sense to folks, but it's
the
> paradigm we've been switching to and has been working so far.
> 
> > 
> > On 2011/08/10 07:04:08, willchan wrote:
> > > Please remove refcounting. Predictor will be destroyed when ProfileIOData
> gets
> > > destroyed, so we shouldn't refcount it. I suspect you kept it around
because
> > you
> > > use NewRunnableMethod(). Don't bother. Use base::Bind() and use
> > > base::Unretained(predictor) to instruct base::Bind() that it doesn't need
to
> > be
> > > refcounted. See the example at
> > >
> >
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/callback.h&type=....
> > 
>

[willchan no longer on Chromium, 2011-08-16 19:02:49]

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

http://codereview.chromium.org/7467012/diff/32011/chrome/browser/net/predicto...
chrome/browser/net/predictor.h:60: class Predictor : public
base::RefCountedThreadSafe<Predictor> {
On 2011/08/16 18:45:22, jar wrote:
> I'm vaguely convinced that the shutdown order is (currently) as you specified.
 
> 
> I think depending on such complex global ordering to ensure safety is a bad
> pattern. I much prefer the readability of revocable method factories that
allow
> a very narrow (local) read of the code to assure correctness.  Heaven help us
if
> someone tries to copy this pattern to another context, without full
> understanding of its extensive justifications!

Agreed!

> 
> I REALLY like the known global ordering, and the ability to depend on it to
> write rational code that should shutdown deterministically.  I like the fact
> that you're talking about this ordering.  
> 
> However... I don't like depending on global assertions from many distinct
sites,
> as there is no way to a) assert correctness locally; b) list all the
> dependendencies on the ordering (in case we ever wanted to change it!).  With
> intricate global ordering dependencies, ordering can't be changed, and is
> fragile, with many distant hidden dependencies.
> 
> Bottom line: I don't think we should be advocating for this pattern.  I'm sure
> there will be performance critical places where we'll have to... but it is IMO
a
> bad generic pattern.
> 
> Am I misunderstanding something??

Note that we aren't *able* to use revocable method factories.
ScopedRunnableMethodFactory uses WeakPtr which depends on NonThreadSafe
implementation details. You'll note in predictor.cc, we use base::Unretained()
in any places where we are posting across threads, since we can't use
ScopedRunnableMethodFactory there.

The previous code used base::RefCountedThreadSafe. The problem with refcounting
is then the destruction order is not well defined. You note that it's great to
have a known global ordering and have deterministic shutdown. I agree with that.
Otherwise then during shutdown, *ALL* objects have to be refcounted and it
usually leads to reference cycles. Since it'll be possible in the future for
profiles to be deleted during normal runtime (due to multiprofile), we can't
afford to refcount, therefore we need to have defined global ordering.

We've tried to limit the scope of having to understand the global ordering to
within the .cc file and it's documented in the header file.

> 
> On 2011/08/16 01:42:57, willchan wrote:
> > On 2011/08/16 01:19:09, jar wrote:
> > > WillChan: I'm trying to be sure I understand the pattern you proposed:
> > > Are you counting on the fact that a posted message to the IO thread will
be
> > > dropped when the IO thread goes away... and after the IO thread goes away,
> > > then(?) ProfileIOData will be destroyed... so Predictor will live "long
> > enough"
> > > to service any posts?
> > 
> > Not quite. Shutdown is confusing, but here's how it works. Predictor will
get
> > destroyed when ProfileIOData goes away. The event order is:
> > * On the UI (main) thread, MessageLoop::Run() returns. We destroy
> > g_browser_process, which leads to ProfileManager getting destroyed and
> > destroying all Profile instances.
> > * Destroying the Profile will cause the member variable
> > ProfileImplIOData::Handle to get destroyed, which will post a DeleteTask to
> the
> > IO thread to delete the ProfileIOData. Predictor gets destroyed when
> > ProfileIOData does.
> > 
> > Since ProfileIOData strictly gets destroyed *after* the Profile does, it
> should
> > be impossible for any tasks to get posted to the Predictor on the IO thread
> > after it has been destroyed, since those tasks should only be generated by
> > objects associated with the Profile. If there are still global references to
> > Predictor, then it's a bug. If an object that is tied to a Profile does not
> get
> > destroyed when Profile goes away (translation: it is leaked), it's a bug.
> > 
> > Let me know if that lifetime management makes sense. Chrome shutdown is
> > confusing so I suspect this won't immediately make sense to folks, but it's
> the
> > paradigm we've been switching to and has been working so far.
> > 
> > > 
> > > On 2011/08/10 07:04:08, willchan wrote:
> > > > Please remove refcounting. Predictor will be destroyed when
ProfileIOData
> > gets
> > > > destroyed, so we shouldn't refcount it. I suspect you kept it around
> because
> > > you
> > > > use NewRunnableMethod(). Don't bother. Use base::Bind() and use
> > > > base::Unretained(predictor) to instruct base::Bind() that it doesn't
need
> to
> > > be
> > > > refcounted. See the example at
> > > >
> > >
> >
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/callback.h&type=....
> > > 
> > 
>

[willchan no longer on Chromium, 2011-08-16 22:14:48]

LGTM, but wait for jar to sign off on predictor stuff, and mirandac to sign off
on ProfileManagerTest. I suspect/hope they don't have many comments left.

http://codereview.chromium.org/7467012/diff/53001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7467012/diff/53001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:561: }
Add a line after this.

[rpetterson, 2011-08-17 05:43:38]

Should be passing all tests. Got around the ProfileManagerTest situation by
creating a SimpleShutdownPredictor that gets created when we're running
unittests as discussed offline with @mirandac. I put forth some serious effort
today trying to get a ProfileFactory model in there, but ran into issues getting
the TestingProfile to behave with respect to notifications and observers. I've
saved the changes in another CL though so we can tackle them after this one is
in. 

@jar, I verified that clearing history results in the correct behavior.

Just waiting on @mirandac's okay for the profile pieces and @jar's okay on the
rest of the predictor stuff.

http://codereview.chromium.org/7467012/diff/53001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7467012/diff/53001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:561: }
On 2011/08/16 22:14:48, willchan wrote:
> Add a line after this.

Done.

[Miranda Callahan, 2011-08-17 08:45:36]

On 2011/08/17 05:43:38, rpetterson wrote:
> Should be passing all tests. Got around the ProfileManagerTest situation by
> creating a SimpleShutdownPredictor that gets created when we're running
> unittests as discussed offline with @mirandac. I put forth some serious effort
> today trying to get a ProfileFactory model in there, but ran into issues
getting
> the TestingProfile to behave with respect to notifications and observers. I've
> saved the changes in another CL though so we can tackle them after this one is
> in. 
> 
> @jar, I verified that clearing history results in the correct behavior.
> 
> Just waiting on @mirandac's okay for the profile pieces and @jar's okay on the
> rest of the predictor stuff.
> 
>
http://codereview.chromium.org/7467012/diff/53001/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile.h (right):
> 
>
http://codereview.chromium.org/7467012/diff/53001/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile.h:561: }
> On 2011/08/16 22:14:48, willchan wrote:
> > Add a line after this.
> 
> Done.

LGTM with nit!

[Miranda Callahan, 2011-08-17 08:45:56]

http://codereview.chromium.org/7467012/diff/58004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7467012/diff/58004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:323:
g_browser_process->profile_manager() == NULL);
nit: can you add a comment here like "If no ProfileManager is attached to the
browser process, we are in a unit test."? Otherwise it may be a mysterious
comparison.

[rpetterson, 2011-08-17 16:28:24]

FYI, I pulled the delete of predictor_api.{h,cc} out of this CL and will submit
it as a second CL after this one lands. It was causing patch conflict errors.

http://codereview.chromium.org/7467012/diff/58004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7467012/diff/58004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:323:
g_browser_process->profile_manager() == NULL);
On 2011/08/17 08:45:56, Miranda Callahan wrote:
> nit: can you add a comment here like "If no ProfileManager is attached to the
> browser process, we are in a unit test."? Otherwise it may be a mysterious
> comparison.

Done.

[jar (doing other things), 2011-08-17 18:37:56]

LGTM

[commit-bot: I haz the power, 2011-08-19 09:36:20]

Change committed as 97446

[commit-bot: I haz the power, 2011-09-08 20:03:28]

Can't apply patch for file chrome/browser/browser_main.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/browser_main.cc
Hunk #1 succeeded at 50 (offset -3 lines).
Hunk #2 FAILED at 701.
Hunk #3 succeeded at 974 (offset -31 lines).
Hunk #4 succeeded at 1127 (offset -31 lines).
Hunk #5 FAILED at 1678.
2 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/browser_main.cc.rej

[commit-bot: I haz the power, 2011-09-09 03:40:10]

Can't apply patch for file chrome/browser/browser_main.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/browser_main.cc
Hunk #1 FAILED at 50.
Hunk #2 FAILED at 974.
Hunk #3 FAILED at 1045.
Hunk #4 FAILED at 1549.
4 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/browser_main.cc.rej

[commit-bot: I haz the power, 2011-09-09 15:52:20]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win, revision is 100408, job name
was 7467012-105007 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-09-09 20:16:15]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win, revision is 100471, job name
was 7467012-105007 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-09-10 01:30:49]

Change committed as 100555

[TVL, 2011-09-13 19:07:50]

http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.h:550: virtual chrome_browser_net::Predictor*
GetNetworkPredictor();
why is this not a pure virtual?  And why is an implementation provide in
profile.cc, profile.cc should only have static and non virtual methods.

[willchan no longer on Chromium, 2011-09-13 19:12:46]

http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.h:550: virtual chrome_browser_net::Predictor*
GetNetworkPredictor();
On 2011/09/13 19:07:51, TVL wrote:
> why is this not a pure virtual?  And why is an implementation provide in
> profile.cc, profile.cc should only have static and non virtual methods.

Woops, I missed that in the review. That's absolutely correct.

[rpetterson, 2011-09-13 19:39:10]

http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.h:550: virtual chrome_browser_net::Predictor*
GetNetworkPredictor();
On 2011/09/13 19:12:47, willchan wrote:
> On 2011/09/13 19:07:51, TVL wrote:
> > why is this not a pure virtual?  And why is an implementation provide in
> > profile.cc, profile.cc should only have static and non virtual methods.
> 
> Woops, I missed that in the review. That's absolutely correct.

This restriction wasn't clear to me. @TVL, I'll send you a CL with the fix
shortly.

[willchan no longer on Chromium, 2011-09-13 19:52:05]

On Tue, Sep 13, 2011 at 12:39 PM,  <rlp@chromium.org> wrote:
>
>
http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile.h (right):
>
>
http://codereview.chromium.org/7467012/diff/105007/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile.h:550: virtual
> chrome_browser_net::Predictor* GetNetworkPredictor();
> On 2011/09/13 19:12:47, willchan wrote:
>>
>> On 2011/09/13 19:07:51, TVL wrote:
>> > why is this not a pure virtual?  And why is an implementation
>
> provide in
>>
>> > profile.cc, profile.cc should only have static and non virtual
>
> methods.
>
>> Woops, I missed that in the review. That's absolutely correct.
>
> This restriction wasn't clear to me. @TVL, I'll send you a CL with the
> fix shortly.

Don't worry, not your fault. My fault for not catching as a reviewer.
Thanks to TVL for pointing this out. Please add me as a OWNERS
reviewer.

>
> http://codereview.chromium.org/7467012/
>