Adds speculative prefetching of resources.

Adds speculative prefetching of resources.

The learning component was already checked in.

BUG=None
TEST=To be done.


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

[Shishir, 2012-07-22 07:06:15]

Please take a look. If the structure looks ok, I will add the tests.

[dominich, 2012-07-23 16:04:41]

The structure looks ok, though I have concerns regarding ownership of objects
and use of linked_ptr. I don't think fixes to those will require any
rearchitecture but if you feel you need to, do so.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_common.h (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_common.h:51: int
max_navigation_lifetime_seconds;  // Default 60
i don't think you should put the defaults here. Anyone interested can look at
the .cc file, and this just makes it more likely to get out of date comments.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_common.h:52: // Size of LRU caches
for the Url data.
nit: URL

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_common.h:54: // The number of times,
we should have seen a visit to this Url in history
nit: URL

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_common.h:60: // The number of
consecutive misses after we stop tracking a resource Url.
nit: URL

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_common.h:70: float
min_resource_hits_to_trigger_prefetch;  // Default 4
why is this float?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_common.h:76: int
max_prefetches_inflight_per_host_per_navigation;  // Default 3
inconsistency: Both are max prefetches in-flight, but one is size_t and one is
int.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:335: if
(prefetch_manager_.get() &&
simpler:

if (!prefetch_manager_.get())
  return;

const UrlTableCacheMap::const_iterator iter =
url_table_cache_.find(main_frame_url);
if (iter == url_table_cache_.end())
  return;
const UrlTableCacheValue& value = *iter;
  ...

This saves the double lookup.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:345: if (confidence <
config_.min_resource_confidence_to_trigger_prefetch ||
warning: comparing double to float. Use double throughout config if you're going
to use double here. Or just use float here - accuracy is not exactly paramount.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:346: it->number_of_hits
< config_.min_resource_hits_to_trigger_prefetch) {
maybe histogram this failure case

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:352:
requests->push_back(make_linked_ptr(req));
why is this a linked_ptr?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:507:
DCHECK(results_map_.find(navigation_id) == results_map_.end());
again here you're doing a double lookup (in DEBUG). Use insert instead of [] to
save an implicit find.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:510:
results_map_[navigation_id].reset(requests.release());
ah, so you're using linked_ptr to have the vector deleted on erase? i'd prefer
an explicit call to delete than the overhead of the linked_ptr.

Similarly, you can use ScopedVector to have the vector contents deleted on
vector destruction.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:811:
inflight_navigations_.find(navigation_id)->second;
you're doing this find twice - do it once and use the iterator here and in the
DCHECK above.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:813:
results_map_.find(navigation_id)->second.get();
you're doing this find twice. do it once and cache the iterator.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:824: if
(actual_resources[req->resource_url]) {
You just did the find to get the iterator - you should use that instead of [] to
save another lookup.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:835: int
p_cache_a_cache = 0, p_cache_a_network = 0, p_cache_a_notused = 0,
what is p_? don't use abbreviations.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:840:
ResourcePrefetcher::Request* req = it->get();
You can combine this loop with the one above that is setting usage_status.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:71: public
ResourcePrefetcherManager::Delegate,
do you really need the delegate pattern here? Consider having an explicit
dependency between the Manager and the Predictor.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:147: typedef
std::map<NavigationID, linked_ptr<ResourcePrefetcher::RequestVector> >
red flag: linked_ptr is unlikely to be what you want. Why isn't this just a
vector pointer? Can a single RequestVector be the value for multiple
NavigationID keys? If so, this is the wrong data structure to use.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:198:
ResourcePrefetchPredictorConfig config_;
const?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:201:
scoped_refptr<ResourcePrefetcherManager> prefetch_manager_;
Why is this a refptr? I'm surprised there can be more than one instance. Maybe
it should be ProfileKeyed?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:14: static const size_t
kResourceBufferSizeBytes = 50000;
note: histogram the sizes to ensure this is reasonable.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:41:
request_vector_(requests.Pass()) {
DCHECK(request_vector_.get());

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:44: for (int i = 0; i <
static_cast<int>(request_vector_->size()); ++i) {
std::copy(request_vector_->begin(), request_vector->end(),
           std::back_inserter(request_queue_));

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:80:
config_.max_prefetches_inflight_per_navigation) &&
consider histogram for this case

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:82:
std::list<Request*>::iterator it = request_queue_.begin();
can you comment this loop? it's a little tricky and explaining that you're
searching for the first host that has an empty slot would clear it up.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:86: host_inflight_counts_[host]
<
remove double lookup.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:87:
config_.max_prefetches_inflight_per_host_per_navigation)
consider histogram for this case.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:95: SendRequest(request);
SendRequest(*it);
request_queue_.erase(it);

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:106:
delegate_->ResourcePrefetcherFinished(this, request_vector_.Pass());
hmm, now i'm confused. This doesn't take ownership of the request_vector_? Then
why is it a scoped_ptr? It could be a raw pointer and ownership can be clearer.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:132:
CHECK_GT(host_inflight_counts_[host], 0);
please use the iterator throughout this method.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:137:
inflight_requests_[request]->prefetch_status = status;
you set the status and then remove it from the map. How does someone then read
that status? This seems pointless.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:140: delete request;
here you could use a linked_ptr to get this behaviour on erase, but you don't.
I'm very confused about your use of linked_ptr.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:150: status =
request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);
I don't know enough about URLRequest - does this take ownership of the buffer,
and will it delete it correctly based on status? If we go through this loop more
than once, does it keep the old buffer around or does the buffer get replaced?

It seems to me, from reading this, that this is going to read the resource in
50k chunks until end/failure but it's throwing away the read bytes. What am I
missing?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:170: FinishRequest(request,
Request::PREFETCH_STATUS_CANCELLED);
It's worth enumerating these cancelation reasons.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.h:58: typedef
std::vector<linked_ptr<Request> > RequestVector;
This being public makes it more difficult, but I think using a ScopedVector here
would be better. It would mean the Requests would stick around until the whole
RequestVector is deleted, but they aren't long lived, right?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.h:76: ResourcePrefetcher(Delegate*
delegate,
I think this should be an explicit dependency rather than a Delegate.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.h:76: ResourcePrefetcher(Delegate*
delegate,
comment that this takes ownership of requests.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.cc:43:
prefetchers_.clear();
you're relying on linked_ptr to delete these? Is there a ScopedMap instead that
you can use?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.cc:57:
prefetchers_[main_frame_url] = make_linked_ptr(prefetcher);
double lookup.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.cc:67: ResourcePrefetcher*
prefetcher = prefetchers_[main_frame_url].get();
double lookup.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.cc:89:
prefetchers_.erase(main_frame_url);
i think an explicit delete here is better than linked_ptr overhead.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.cc:98: if (delegate_)
you CHECK(delegate_) in the ctor. this is unnecessary.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher_manager.h (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.h:29: public
base::RefCountedThreadSafe<ResourcePrefetcherManager> {
If this is created and owned by the resource prefetch predictor, which is
profile keyed, why does this need to be refcounted?

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.h:34: // manager.
nit: extra word.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher_manager.h:35: class Delegate {
I think this is such a tightly bound system that using Delegates is pointless
and reduces readability.

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

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_field_trial.cc:285: // Override any field
trial groups if the user has set a command line flag.
what field trial? You don't check a field trial.

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/1/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:1216: 
nit: remove blank line.

[Shishir, 2012-08-01 22:35:24]

Removed all delegates and linked ptrs :) 

Please take a look.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetch_common.h (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_common.h:51: int
max_navigation_lifetime_seconds;  // Default 60
On 2012/07/23 16:04:41, dominich wrote:
> i don't think you should put the defaults here. Anyone interested can look at
> the .cc file, and this just makes it more likely to get out of date comments.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_common.h:52: // Size of LRU caches
for the Url data.
On 2012/07/23 16:04:41, dominich wrote:
> nit: URL

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_common.h:54: // The number of times,
we should have seen a visit to this Url in history
On 2012/07/23 16:04:41, dominich wrote:
> nit: URL

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_common.h:60: // The number of
consecutive misses after we stop tracking a resource Url.
On 2012/07/23 16:04:41, dominich wrote:
> nit: URL

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_common.h:70: float
min_resource_hits_to_trigger_prefetch;  // Default 4
On 2012/07/23 16:04:41, dominich wrote:
> why is this float?

Changed to int

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_common.h:76: int
max_prefetches_inflight_per_host_per_navigation;  // Default 3
On 2012/07/23 16:04:41, dominich wrote:
> inconsistency: Both are max prefetches in-flight, but one is size_t and one is
> int.

Its just the way they are used. One was used in comparing vector size, not the
other. Changing all size_t to int for consistency.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:335: if
(prefetch_manager_.get() &&
On 2012/07/23 16:04:41, dominich wrote:
> simpler:
> 
> if (!prefetch_manager_.get())
>   return;
> 
> const UrlTableCacheMap::const_iterator iter =
> url_table_cache_.find(main_frame_url);
> if (iter == url_table_cache_.end())
>   return;
> const UrlTableCacheValue& value = *iter;
>   ...
> 
> This saves the double lookup.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:345: if (confidence <
config_.min_resource_confidence_to_trigger_prefetch ||
On 2012/07/23 16:04:41, dominich wrote:
> warning: comparing double to float. Use double throughout config if you're
going
> to use double here. Or just use float here - accuracy is not exactly
paramount.

Everything is float now.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:346: it->number_of_hits
< config_.min_resource_hits_to_trigger_prefetch) {
On 2012/07/23 16:04:41, dominich wrote:
> maybe histogram this failure case

We indirectly measure this.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:352:
requests->push_back(make_linked_ptr(req));
On 2012/07/23 16:04:41, dominich wrote:
> why is this a linked_ptr?

Changed to scoped vector.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:507:
DCHECK(results_map_.find(navigation_id) == results_map_.end());
On 2012/07/23 16:04:41, dominich wrote:
> again here you're doing a double lookup (in DEBUG). Use insert instead of []
to
> save an implicit find.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:510:
results_map_[navigation_id].reset(requests.release());
On 2012/07/23 16:04:41, dominich wrote:
> ah, so you're using linked_ptr to have the vector deleted on erase? i'd prefer
> an explicit call to delete than the overhead of the linked_ptr.
> 
> Similarly, you can use ScopedVector to have the vector contents deleted on
> vector destruction.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:811:
inflight_navigations_.find(navigation_id)->second;
On 2012/07/23 16:04:41, dominich wrote:
> you're doing this find twice - do it once and use the iterator here and in the
> DCHECK above.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:813:
results_map_.find(navigation_id)->second.get();
On 2012/07/23 16:04:41, dominich wrote:
> you're doing this find twice. do it once and cache the iterator.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:824: if
(actual_resources[req->resource_url]) {
On 2012/07/23 16:04:41, dominich wrote:
> You just did the find to get the iterator - you should use that instead of []
to
> save another lookup.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:835: int
p_cache_a_cache = 0, p_cache_a_network = 0, p_cache_a_notused = 0,
On 2012/07/23 16:04:41, dominich wrote:
> what is p_? don't use abbreviations.

p_ is for predicted and a_ is actual. Added comment, else the names are far too
long.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:840:
ResourcePrefetcher::Request* req = it->get();
On 2012/07/23 16:04:41, dominich wrote:
> You can combine this loop with the one above that is setting usage_status.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.h:71: public
ResourcePrefetcherManager::Delegate,
On 2012/07/23 16:04:41, dominich wrote:
> do you really need the delegate pattern here? Consider having an explicit
> dependency between the Manager and the Predictor.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.h:147: typedef
std::map<NavigationID, linked_ptr<ResourcePrefetcher::RequestVector> >
On 2012/07/23 16:04:41, dominich wrote:
> red flag: linked_ptr is unlikely to be what you want. Why isn't this just a
> vector pointer? Can a single RequestVector be the value for multiple
> NavigationID keys? If so, this is the wrong data structure to use.

Changed to plain pointers.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.h:198:
ResourcePrefetchPredictorConfig config_;
On 2012/07/23 16:04:41, dominich wrote:
> const?

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.h:201:
scoped_refptr<ResourcePrefetcherManager> prefetch_manager_;
On 2012/07/23 16:04:41, dominich wrote:
> Why is this a refptr? I'm surprised there can be more than one instance. Maybe
> it should be ProfileKeyed?

Needs to be refcounted because it needs to be accessed both on UI and IO
threads. 
It can be profile keyed, but I dont see the point since it is only used by this
class right now which is profile keyed.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetcher.cc (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:14: static const size_t
kResourceBufferSizeBytes = 50000;
On 2012/07/23 16:04:41, dominich wrote:
> note: histogram the sizes to ensure this is reasonable.

This is not the total size read, but the buffer size. Histogramming this would
be equivalent of histogramming size of resources.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:41:
request_vector_(requests.Pass()) {
On 2012/07/23 16:04:41, dominich wrote:
> DCHECK(request_vector_.get());

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:44: for (int i = 0; i <
static_cast<int>(request_vector_->size()); ++i) {
On 2012/07/23 16:04:41, dominich wrote:
> std::copy(request_vector_->begin(), request_vector->end(),
>            std::back_inserter(request_queue_));

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:80:
config_.max_prefetches_inflight_per_navigation) &&
On 2012/07/23 16:04:41, dominich wrote:
> consider histogram for this case

I will send out a separate CL with a lot more histograms added.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:82:
std::list<Request*>::iterator it = request_queue_.begin();
On 2012/07/23 16:04:41, dominich wrote:
> can you comment this loop? it's a little tricky and explaining that you're
> searching for the first host that has an empty slot would clear it up.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:86: host_inflight_counts_[host]
<
On 2012/07/23 16:04:41, dominich wrote:
> remove double lookup.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:87:
config_.max_prefetches_inflight_per_host_per_navigation)
On 2012/07/23 16:04:41, dominich wrote:
> consider histogram for this case.

Will send another CL with a lot of histograms :)

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:95: SendRequest(request);
On 2012/07/23 16:04:41, dominich wrote:
> SendRequest(*it);
> request_queue_.erase(it);

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:106:
delegate_->ResourcePrefetcherFinished(this, request_vector_.Pass());
On 2012/07/23 16:04:41, dominich wrote:
> hmm, now i'm confused. This doesn't take ownership of the request_vector_?
Then
> why is it a scoped_ptr? It could be a raw pointer and ownership can be
clearer.

Made the interfaces a little cleaner.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:132:
CHECK_GT(host_inflight_counts_[host], 0);
On 2012/07/23 16:04:41, dominich wrote:
> please use the iterator throughout this method.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:137:
inflight_requests_[request]->prefetch_status = status;
On 2012/07/23 16:04:41, dominich wrote:
> you set the status and then remove it from the map. How does someone then read
> that status? This seems pointless.

I remove it from the inflight map. It is still present in the other maps so the
field is still valid.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:140: delete request;
On 2012/07/23 16:04:41, dominich wrote:
> here you could use a linked_ptr to get this behaviour on erase, but you don't.
> I'm very confused about your use of linked_ptr.

Well no more linked ptrs.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:150: status =
request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);
On 2012/07/23 16:04:41, dominich wrote:
> I don't know enough about URLRequest - does this take ownership of the buffer,
> and will it delete it correctly based on status? If we go through this loop
more
> than once, does it keep the old buffer around or does the buffer get replaced?
> 
> It seems to me, from reading this, that this is going to read the resource in
> 50k chunks until end/failure but it's throwing away the read bytes. What am I
> missing?


The buffer is a refcounted, so it will get destroyed when the request is done
reading it. Fixed the declaration of the buffer.

As for the reading of the data, yes we read the data and throw away the buffer.
If we don't read the entire buffer, the resource is not fully downloaded and
inserted into the cache.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.cc:170: FinishRequest(request,
Request::PREFETCH_STATUS_CANCELLED);
On 2012/07/23 16:04:41, dominich wrote:
> It's worth enumerating these cancelation reasons.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetcher.h (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.h:58: typedef
std::vector<linked_ptr<Request> > RequestVector;
On 2012/07/23 16:04:41, dominich wrote:
> This being public makes it more difficult, but I think using a ScopedVector
here
> would be better. It would mean the Requests would stick around until the whole
> RequestVector is deleted, but they aren't long lived, right?

Changed to ScopedVector.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.h:76: ResourcePrefetcher(Delegate*
delegate,
On 2012/07/23 16:04:41, dominich wrote:
> comment that this takes ownership of requests.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher.h:76: ResourcePrefetcher(Delegate*
delegate,
On 2012/07/23 16:04:41, dominich wrote:
> I think this should be an explicit dependency rather than a Delegate.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.cc:43:
prefetchers_.clear();
On 2012/07/23 16:04:41, dominich wrote:
> you're relying on linked_ptr to delete these? Is there a ScopedMap instead
that
> you can use?

Nope, no ScopedMap in codebase. Using a ElementDeleter.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.cc:57:
prefetchers_[main_frame_url] = make_linked_ptr(prefetcher);
On 2012/07/23 16:04:41, dominich wrote:
> double lookup.

I can't combine them unless I create the prefetcher in advance and then delete
it later. Makes more sense to leave as is.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.cc:67: ResourcePrefetcher*
prefetcher = prefetchers_[main_frame_url].get();
On 2012/07/23 16:04:41, dominich wrote:
> double lookup.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.cc:89:
prefetchers_.erase(main_frame_url);
On 2012/07/23 16:04:41, dominich wrote:
> i think an explicit delete here is better than linked_ptr overhead.

Done. Although I should point out we are looking at one linked_ptr per
navigation.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.cc:98: if (delegate_)
On 2012/07/23 16:04:41, dominich wrote:
> you CHECK(delegate_) in the ctor. this is unnecessary.

Its required because the predictor_ is set to null in the ShutdownOnUIThread
call.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetcher_manager.h (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.h:29: public
base::RefCountedThreadSafe<ResourcePrefetcherManager> {
On 2012/07/23 16:04:41, dominich wrote:
> If this is created and owned by the resource prefetch predictor, which is
> profile keyed, why does this need to be refcounted?

Because this needs to be de-referenced on two different threads. The UI thread
to interact with the predictor and the IO thread to interact with the
prefetchers.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.h:34: // manager.
On 2012/07/23 16:04:41, dominich wrote:
> nit: extra word.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetcher_manager.h:35: class Delegate {
On 2012/07/23 16:04:41, dominich wrote:
> I think this is such a tightly bound system that using Delegates is pointless
> and reduces readability.

Removed.

http://codereview.chromium.org/10817004/diff/1/chrome/browser/prerender/prere...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10817004/diff/1/chrome/browser/prerender/prere...
chrome/browser/prerender/prerender_field_trial.cc:285: // Override any field
trial groups if the user has set a command line flag.
On 2012/07/23 16:04:41, dominich wrote:
> what field trial? You don't check a field trial.

Done.

http://codereview.chromium.org/10817004/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10817004/diff/1/chrome/common/chrome_switches....
chrome/common/chrome_switches.cc:1216: 
On 2012/07/23 16:04:41, dominich wrote:
> nit: remove blank line.

Done.

[dominich, 2012-08-02 15:00:50]

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.cc:361:
base::Passed(requests.Pass())));
I believe you can use &requests instead of calling Pass(). Would remove one set
of parentheses.

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.cc:624: if
(static_cast<int>(url_table_cache_.size()) >= config_.max_urls_to_track)
if it makes sense for this to be size_t that's fine.

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.cc:804:
NavigationMap::iterator nav_it = inflight_navigations_.find(navigation_id);
no need to do this before checking have_prefetch_results. Ie, move down to 814.

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher.cc (right):

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher.cc:86:
config_.max_prefetches_inflight_per_navigation) &&
nit - one too many spaces indented here, i think.

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher.h (right):

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher.h:71: // 'manager' has to outlive
the ResourcePrefetcher. The ResourcePrefetcher
nit: chromium style uses || around parameters referenced in comments.

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.cc:19:
ResourcePrefetchPredictor* predcitor,
nit: predictor

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.cc:53: if
(prefetcher_map_.find(main_frame_url) != prefetcher_map_.end())
Avoiding the double lookup:

PrefetcherMap::iterator prefetcher_it = prefetcher_map_.find(main_frame_url);
if (it != prefetcher_map_.end())
  return;

ResourcePrefetcher* prefetcher = new ResourcePrefetcher(
    this, config_, navigation_id, requests.Pass());
prefetcher_map_.insert(std::make_pair(main_frame_url, prefetcher));
prefetcher->Start();
    main_frame_url,

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.cc:79: // 'predictor_' can
only be accessed from the UI thread.
nit: variable refs in comments use || in chromium style

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.cc:85:
base::Passed(requests_ptr.Pass())));
i believe &requests_ptr might be less verbose

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.cc:100: // Notify the
'predictor_'.
nit: variable name in comment uses ||

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_manager.h (right):

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.h:32: // The 'predictor'
should be live till ShutdownOnIOThread is called.
nit: variable name ||

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.h:71: // UI Thread.
'predictor_'  needs to be called on the UI thread.
nit: variable name ||

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.h:80: PrefetcherMap
prefetcher_map_;  // Owns the pointers.
nit: owns what pointers?

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10817004/diff/10001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:308: if (switch_value ==
switches::kSpeculativeResourcePrefetchingEnabled)
this could be 

return switch_value == switches::kSpeculativeResourcePrefetchingEnabled;

[Shishir, 2012-08-02 22:06:53]

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetch_predictor.cc:361:
base::Passed(requests.Pass())));
On 2012/08/02 15:00:50, dominich wrote:
> I believe you can use &requests instead of calling Pass(). Would remove one
set
> of parentheses.

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetch_predictor.cc:624: if
(static_cast<int>(url_table_cache_.size()) >= config_.max_urls_to_track)
On 2012/08/02 15:00:50, dominich wrote:
> if it makes sense for this to be size_t that's fine.

Lets leave it at int to be consistent through out.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetch_predictor.cc:804:
NavigationMap::iterator nav_it = inflight_navigations_.find(navigation_id);
On 2012/08/02 15:00:50, dominich wrote:
> no need to do this before checking have_prefetch_results. Ie, move down to
814.

This is a correctness DCHECK, so I would prefer to have it at the start of the
function.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher.cc:86:
config_.max_prefetches_inflight_per_navigation) &&
On 2012/08/02 15:00:50, dominich wrote:
> nit - one too many spaces indented here, i think.

Looks right. 4 spaces from 's'

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher.h:71: // 'manager' has to outlive
the ResourcePrefetcher. The ResourcePrefetcher
On 2012/08/02 15:00:50, dominich wrote:
> nit: chromium style uses || around parameters referenced in comments.

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.cc:19:
ResourcePrefetchPredictor* predcitor,
On 2012/08/02 15:00:50, dominich wrote:
> nit: predictor

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.cc:53: if
(prefetcher_map_.find(main_frame_url) != prefetcher_map_.end())
On 2012/08/02 15:00:50, dominich wrote:
> Avoiding the double lookup:
> 
> PrefetcherMap::iterator prefetcher_it = prefetcher_map_.find(main_frame_url);
> if (it != prefetcher_map_.end())
>   return;
> 
> ResourcePrefetcher* prefetcher = new ResourcePrefetcher(
>     this, config_, navigation_id, requests.Pass());
> prefetcher_map_.insert(std::make_pair(main_frame_url, prefetcher));
> prefetcher->Start();
>     main_frame_url,
>         

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.cc:79: // 'predictor_' can
only be accessed from the UI thread.
On 2012/08/02 15:00:50, dominich wrote:
> nit: variable refs in comments use || in chromium style

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.cc:85:
base::Passed(requests_ptr.Pass())));
On 2012/08/02 15:00:50, dominich wrote:
> i believe &requests_ptr might be less verbose

Ah syntactic sugar.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.cc:100: // Notify the
'predictor_'.
On 2012/08/02 15:00:50, dominich wrote:
> nit: variable name in comment uses ||

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
File chrome/browser/predictors/resource_prefetcher_manager.h (right):

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.h:32: // The 'predictor'
should be live till ShutdownOnIOThread is called.
On 2012/08/02 15:00:50, dominich wrote:
> nit: variable name ||

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.h:71: // UI Thread.
'predictor_'  needs to be called on the UI thread.
On 2012/08/02 15:00:50, dominich wrote:
> nit: variable name ||

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_manager.h:80: PrefetcherMap
prefetcher_map_;  // Owns the pointers.
On 2012/08/02 15:00:50, dominich wrote:
> nit: owns what pointers?

Done.

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/prerender/prerender_field_trial.cc:308: if (switch_value ==
switches::kSpeculativeResourcePrefetchingEnabled)
On 2012/08/02 15:00:50, dominich wrote:
> this could be 
> 
> return switch_value == switches::kSpeculativeResourcePrefetchingEnabled;

Done.

[dominich, 2012-08-02 22:10:15]

LGTM

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/10001/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher.cc:86:
config_.max_prefetches_inflight_per_navigation) &&
On 2012/08/02 22:06:54, Shishir wrote:
> On 2012/08/02 15:00:50, dominich wrote:
> > nit - one too many spaces indented here, i think.
> 
> Looks right. 4 spaces from 's'

But the start of the line, for this, is the ( before the s, no?

[Shishir, 2012-09-10 23:14:15]

Added unittest and also reverted the design between ResourcePrefetcher and its
manager to a Delegate to aid testing.

Adding ben for:
chrome/chrome_browser.gypi
chrome/chrome_tests.gypi
chrome/common/chrome_switches.h
chrome/common/chrome_switches.cc

PTAL

[Ben Goodger (Google), 2012-09-11 15:23:26]

lgtm

[dominich, 2012-09-11 17:40:11]

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.cc:134:
prefetch_manager_->ShutdownOnUIThread();
As this is a ProfileKeyedService this might belong in an overridden Shutdown()
method.

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.h:113:
scoped_ptr<ResourcePrefetcher::RequestVector> requests);
why is this a scoped_ptr? I'd expect this to be a const
ResourcePrefetcher::RequestVector&.

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.cc:44:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
is this necessary? It looks like you're posting a task on Shutdown with the
express intent to check that PostTask does what it's supposed to do.

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_unittest.cc:149:
TEST_F(ResourcePrefetcherTest, TestPrefetcherFinishes) {
there's quite a bit of repeated code here. In the past, I've defined a struct
for each element I want to test with an expectation. Eg:

struct TestElement {
  enum ResponseType {
    RESPONSETYPE_OK,
    RESPONSETYPE_SSLCERT,
    ...
  } response_type;
  const char[] url;
  PrefetchStatus status;
};

TestElement test_elements[] = {
    {
      RESPONSETYPE_OK,
      "http://www.google.com/resource1.html"
      Request::PREFETCH_STATUS_FROM_CACHE
    },
    {
      ...
    },
};

and then in this method you can loop over the elements to start the requests,
set the response type, and check the prefetch status.

[Shishir, 2012-09-11 18:18:08]

PTAL

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.cc:134:
prefetch_manager_->ShutdownOnUIThread();
On 2012/09/11 17:40:11, dominich wrote:
> As this is a ProfileKeyedService this might belong in an overridden Shutdown()
> method.

Done.

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.h:113:
scoped_ptr<ResourcePrefetcher::RequestVector> requests);
On 2012/09/11 17:40:11, dominich wrote:
> why is this a scoped_ptr? I'd expect this to be a const
> ResourcePrefetcher::RequestVector&.

The predictor takes ownership of the requests vector and then operates on it.
This can be made a raw pointer if the semantics are clearer that way.

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_manager.cc:44:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2012/09/11 17:40:11, dominich wrote:
> is this necessary? It looks like you're posting a task on Shutdown with the
> express intent to check that PostTask does what it's supposed to do.

This is not right. I think I overlooked this case when addressing some comments.
Fixed. Now it will explicitly delete the pointers.

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_unittest.cc:149:
TEST_F(ResourcePrefetcherTest, TestPrefetcherFinishes) {
On 2012/09/11 17:40:11, dominich wrote:
> there's quite a bit of repeated code here. In the past, I've defined a struct
> for each element I want to test with an expectation. Eg:
> 
> struct TestElement {
>   enum ResponseType {
>     RESPONSETYPE_OK,
>     RESPONSETYPE_SSLCERT,
>     ...
>   } response_type;
>   const char[] url;
>   PrefetchStatus status;
> };
> 
> TestElement test_elements[] = {
>     {
>       RESPONSETYPE_OK,
>       "http://www.google.com/resource1.html"
>       Request::PREFETCH_STATUS_FROM_CACHE
>     },
>     {
>       ...
>     },
> };
> 
> and then in this method you can loop over the elements to start the requests,
> set the response type, and check the prefetch status.
>   

I could do that, but I the order of calls to AddStartUrlRequestExpectation and
OnResponse matter and are not linear. This way it seems to me to be clearer what
is happening rather than going to have to check up indices. Thats harder to
debug.

[dominich, 2012-09-11 18:40:43]

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_unittest.cc:149:
TEST_F(ResourcePrefetcherTest, TestPrefetcherFinishes) {
On 2012/09/11 18:18:08, Shishir wrote:
> On 2012/09/11 17:40:11, dominich wrote:
> > there's quite a bit of repeated code here. In the past, I've defined a
struct
> > for each element I want to test with an expectation. Eg:
> > 
> > struct TestElement {
> >   enum ResponseType {
> >     RESPONSETYPE_OK,
> >     RESPONSETYPE_SSLCERT,
> >     ...
> >   } response_type;
> >   const char[] url;
> >   PrefetchStatus status;
> > };
> > 
> > TestElement test_elements[] = {
> >     {
> >       RESPONSETYPE_OK,
> >       "http://www.google.com/resource1.html"
> >       Request::PREFETCH_STATUS_FROM_CACHE
> >     },
> >     {
> >       ...
> >     },
> > };
> > 
> > and then in this method you can loop over the elements to start the
requests,
> > set the response type, and check the prefetch status.
> >   
> 
> I could do that, but I the order of calls to AddStartUrlRequestExpectation and
> OnResponse matter and are not linear. This way it seems to me to be clearer
what
> is happening rather than going to have to check up indices. Thats harder to
> debug.

Ah, I see. I could see a way to build this as a map with the URL as the key and
the two enums as values, but it might be less clear what's going on.

If you can think of a way to minimize the code in this test, it would be great.
It feels like it's testing much more than it should. Maybe each aspect could be
tested in isolation?

http://codereview.chromium.org/10817004/diff/42002/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

http://codereview.chromium.org/10817004/diff/42002/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.h:110: // for a
navigation. Takes ownership of |requests|.
I don't think the scoped_ptr necessarily suggests this. I'd prefer a raw
pointer, but I'll bow to the established wisdom of the Chromium code-base if
this is the Done Thing.

[Shishir, 2012-09-11 19:36:31]

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

http://codereview.chromium.org/10817004/diff/33001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher_unittest.cc:149:
TEST_F(ResourcePrefetcherTest, TestPrefetcherFinishes) {
On 2012/09/11 18:40:43, dominich wrote:
> On 2012/09/11 18:18:08, Shishir wrote:
> > On 2012/09/11 17:40:11, dominich wrote:
> > > there's quite a bit of repeated code here. In the past, I've defined a
> struct
> > > for each element I want to test with an expectation. Eg:
> > > 
> > > struct TestElement {
> > >   enum ResponseType {
> > >     RESPONSETYPE_OK,
> > >     RESPONSETYPE_SSLCERT,
> > >     ...
> > >   } response_type;
> > >   const char[] url;
> > >   PrefetchStatus status;
> > > };
> > > 
> > > TestElement test_elements[] = {
> > >     {
> > >       RESPONSETYPE_OK,
> > >       "http://www.google.com/resource1.html"
> > >       Request::PREFETCH_STATUS_FROM_CACHE
> > >     },
> > >     {
> > >       ...
> > >     },
> > > };
> > > 
> > > and then in this method you can loop over the elements to start the
> requests,
> > > set the response type, and check the prefetch status.
> > >   
> > 
> > I could do that, but I the order of calls to AddStartUrlRequestExpectation
and
> > OnResponse matter and are not linear. This way it seems to me to be clearer
> what
> > is happening rather than going to have to check up indices. Thats harder to
> > debug.
> 
> Ah, I see. I could see a way to build this as a map with the URL as the key
and
> the two enums as values, but it might be less clear what's going on.
> 
> If you can think of a way to minimize the code in this test, it would be
great.
> It feels like it's testing much more than it should. Maybe each aspect could
be
> tested in isolation?

The code looks repetitive because I have to check the internal state everytime.
I could write wrapper functions to make it look cleaner, but I think that might
be harder to read. I can remove a couple of urls but that would not really
simplify the code.

http://codereview.chromium.org/10817004/diff/42002/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

http://codereview.chromium.org/10817004/diff/42002/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetch_predictor.h:110: // for a
navigation. Takes ownership of |requests|.
On 2012/09/11 18:40:43, dominich wrote:
> I don't think the scoped_ptr necessarily suggests this. I'd prefer a raw
> pointer, but I'll bow to the established wisdom of the Chromium code-base if
> this is the Done Thing.

Raw pointer it is. In this case the method is not called via a PostTask so using
a raw pointer works. Using the scoped pointer for the PostTask is the convention
I think.

[dominich, 2012-09-11 19:41:22]

LGTM

[commit-bot: I haz the power, 2012-09-13 19:07:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10817004/30006

[Shishir, 2012-09-13 21:06:06]

Unchecking the commit bot because there seems to be a related crasher in canary
I should fix first.

[commit-bot: I haz the power, 2012-09-14 22:04:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10817004/30006

[commit-bot: I haz the power, 2012-09-14 23:29:11]

Change committed as 156926

[Lei Zhang, 2012-09-15 01:45:11]

https://chromiumcodereview.appspot.com/10817004/diff/30006/chrome/browser/pre...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10817004/diff/30006/chrome/browser/pre...
chrome/browser/predictors/resource_prefetcher_unittest.cc:62:
TestURLRequestContextGetter* request_context_getter_;
This probably should have been a scoped_ptr.