Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10416002:
Seculative resource prefetching for URLs CL. (Closed)
|
Created:
8 years, 7 months ago by Shishir Modified:
8 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, arv (Not doing code reviews), darin-cc_chromium.org, igrigorik+cr_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSpeculative resource prefetching for URLs CL.
The learning model and database persistance for speculative resource
prefetching based on URLs. The host based learning will come in another CL.
Will add tests once the design is approved.
BUG=
TEST=unit_test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144019
Patch Set 1 #
Total comments: 152
Patch Set 2 : Addressing dominich's and willchan's comments. #
Total comments: 40
Patch Set 3 : Moving to WeakPtrs and RVHD. #
Total comments: 55
Patch Set 4 : Addressing Dominich's comments. #
Total comments: 34
Patch Set 5 : Addressing Will's comments and adding a unittest. #
Total comments: 40
Patch Set 6 : Addressing comments, adding test. #
Total comments: 6
Patch Set 7 : Addressing Domnich's comments. #
Total comments: 4
Patch Set 8 : Fixing enum name. #
Total comments: 11
Patch Set 9 : Addressing willchan's comments. #Patch Set 10 : Rebasing and windows test bug fixes. #Patch Set 11 : Resolving conflicts. #Patch Set 12 : Resolving conflicts. #Messages
Total messages: 33 (0 generated)
Could you please review this CL? willchan: for chrome/browser/net/... dominich, tonyg: All of it.
This is disabled by default with no field trial, correct? So you'll add tests first, then check this in, then add a field trial to enable it? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_interceptor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_interceptor.h:12: #include "net/url_request/url_request_job_factory.h" nit: this include should be first as it defines the base class. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.cc:30: if (render_process_id_ != rhs.render_process_id_) { nit: braces unnecessary throughout. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.cc:40: return render_process_id_ == rhs.render_process_id_ && use IsSameRenderer here. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:19: struct NavigationID { This should be a class (it has constructors, operators, and methods) and should be in its own header. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:21: NavigationID(const NavigationID& other); Try explicit here - I don't know if STL is using direct initialization or copy initialization off the top of my head, but if you can get away with adding explicit it might help enforce passing by reference. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:22: NavigationID(const content::WebContents& web_contents); This should be explicit as you call it directly. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:28: int render_process_id_; Do these all need to be public? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:38: // If a navigation hasn't seen a load complete event in this much time, it is How much work has there been to tune these numbers? Do you expect them to require experimentation? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:53: // Dont store subresources whose Urls are longer than this. nit: Don't https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:62: ResourceType::Type GetResourceTypeFromMimeType(std::string mime_type, const std::string& mime_type to save the copy. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:94: bool ResourcePrefetchPredictor::URLRequestSummary::InitFromURLRequest( TODO: check if this return value is used. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:100: LOG(ERROR) << "No ResourceRequestInfo in request"; should this be a CHECK/DCHECK? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:139: history_service->InMemoryDatabase(); You're not using the result of this call - this might cause a compile error on some platform. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:154: bool ResourcePrefetchPredictor::IsEnabled() { These methods are not ordered as in the header - please rearrange them. Also, I like the practice of labeling static methods with a // static comment. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:175: // TODO(shishir): The following would be much more efficient if we used Can you use insertion sort to sort them as they're added instead? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:200: switch (request_info->GetResourceType()) { could be: return request_info->GetResourceType() == ResourceType::MAIN_FRAME && IsHandledMainPage(request); https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:215: switch (request_info->GetResourceType()) { could be: return request_info->GetResourceType() == ResourceType::MAIN_FRAME ? IsHandledMainPage(response) : IsHandledSubresource(response); https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:232: switch (request_info->GetResourceType()) { As above https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:241: if (request->original_url().scheme() != chrome::kHttpScheme) there's too many negatives here. How about: return request->original_url().scheme() == chrome::kHttpScheme; https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:252: // Check the scheme of the orign. We only do http. nit: origin. Also, consider if these comments are actually useful. I think the code is obvious enough that the comments don't add anything. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:276: if (!is_cacheable) return is_cacheable; https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:282: bool ResourcePrefetchPredictor::IsCacheable(net::URLRequest* response) { const net::URLRequest* response? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:283: // If this was serverd from cache, we are good. nit: served https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:301: switch (request.resource_type_) { I find 'if' much more readable in the case of a single condition. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:314: switch (response.resource_type_) { if/else is more readable https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:328: case ResourceType::MAIN_FRAME: if rather than switch. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:343: // TODO(shishir): Maybe fix it. Enter a bug for this and reference it here. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:358: inflight_navigations_[request.navigation_id_]; Please use insert here with a default constructed std::vector<UrlRequestSummary>() to make it clear what's going on. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:367: // TODO(shishir): The prefreshing will be stopped here. Can you add this as part of this CL? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:376: inflight_navigations_.erase(response.navigation_id_); can you add a comment explaining why we're not stopping prefreshing here? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:399: summary.resource_type_ = ResourceType::LAST_TYPE; // Dont have type here. You could add it - WebContentsImpl::OnDidLoadResourceFromMemoryCache has the type. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:411: it != inflight_navigations_.end();) { You can increment |it| in the for loop as map::erase doesn't invalidate the iterator. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:431: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK instead of CHECK. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:476: if (history_service && history_service->InMemoryDatabase()) { I think you can DCHECK on history_service, or even assume that it's available, given when this method is called. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:477: history::URLDatabase* url_db = history_service->InMemoryDatabase(); Then you can store this in the local var outside the conditional and save a call to InMemoryDatabase. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:481: it != url_table_cache_.end();) { Increment |it| in the for loop. map::erase doesn't invalidate iterators. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:536: if (inflight_navigations_.find(navigation_id) == inflight_navigations_.end()) So make it a DCHECK :) https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:544: if (url_table_cache_.find(main_frame_url) != url_table_cache_.end() || Is it worth putting the cache_ check inside ShouldTrackUrl? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:555: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Can any of this be done as a PostTask to avoid blocking the UI loop? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:562: for (int i = 0; i < static_cast<int>(new_resources.size()); ++i) { cache the loop end variable outside the loop to avoid recalculation every iteration. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:577: for (int i = 0; i < static_cast<int>(new_resources.size()); ++i) { cache the loop end var in a local variable. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:583: for (int i = 0; i < static_cast<int>(old_resources.size()); ++i) { cache the loop end var in a local variable. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:590: for (int i = 0; i < static_cast<int>(old_resources.size()); ++i) { cache loop end var. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:593: old_row.number_of_misses_++; prefer pre-increment. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:597: new_resources[new_index[old_row.resource_url_]]; you could remove it from new_resources here to make the following loop smaller. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:599: // Update the resource type if its missing. nit: it's https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:601: old_row.resource_type_ = new_row.resource_type_; what if the resource_type has changed? unlikely, but an edge case to consider. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:607: old_row.number_of_hits_++; prefer pre-increment. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:613: for (int i = 0; i < static_cast<int>(new_resources.size()); ++i) { cache loop var. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:615: if (old_index.find(summary.resource_url_) != old_index.end()) This would be a DCHECK if you remove from new_resources above. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:634: for (int i = rows.size() - 1; i >= 0; --i) { use iterator loop here. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:692: for (int i = 0; i < static_cast<int>(actual.size()); ++i) { use iterator loop. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:702: prefetch_missed++; prefer pre-increment https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:704: prefetch_cached++; prefer pre-increment https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:706: prefetch_network++; prefer pre-increment https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:42: // - All functions apart form constructor/destructor needs to be called on nit: form-> from, needs -> need https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:42: // - All functions apart form constructor/destructor needs to be called on this isn't accurate - the static methods are not thread-dependent. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:53: URLRequestSummary(const URLRequestSummary& other); Try explicit here - I don't know if STL is using direct initialization or copy initialization off the top of my head, but if you can get away with adding explicit it might help enforce passing by reference. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:57: NavigationID navigation_id_; is there any need for these members to be public? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_factory.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_factory.cc:30: // TODO(shishir): When HistoryService is PKSFized, uncomment this. Is there a bug to track this? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:59: average_position_(0), warning: initializing a double with an int. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:75: void ResourcePrefetchPredictorTables::UrlTableRow::UpdateScore() { Comment explaining the heuristic here. Also, might this heuristic change or might there be alternative field trials? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:87: default: What other resource types should be handled? Maybe you can NOTREACHED in the default case. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:123: if (!DB()->DoesTableExist(kResourcePredictorUrlTableName)) add braces as the inner if has them. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:130: void ResourcePrefetchPredictorTables::LogDatabaseStats() { Consider tracking the raw file size. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:157: url_row.UpdateScore(); why don't you store the score? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:171: sql::Statement delete_statement(DB()->GetUniqueStatement( would it be more optimal to SELECT and then UPDATE/INSERT as necessary? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:12: #include "chrome/browser/predictors/predictor_table_base.h" This include should be first as it provides the base class. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:26: UrlTableRow(const UrlTableRow& other); check if you can mark this explicit - STL may not use direct-initialization but if it does, you can help discourage pass-by-value. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:35: double average_position_; Can this be a float? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:38: double score_; Can this be a float? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:41: // Sorts the UrlTableRows by score. ascending or descending? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/ui/webu... File chrome/browser/ui/webui/predictors/predictors_handler.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/ui/webu... chrome/browser/ui/webui/predictors/predictors_handler.cc:101: for (int i = 0; i < static_cast<int>(it->second.rows_.size()); ++i) { use iterator loop
https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_interceptor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_interceptor.h:21: : public net::URLRequestJobFactory::Interceptor { No, do not use this interface. You're not intercepting the URLRequest job creation in order to replace it with another URLRequestJob. You just want callbacks at different stages of URLRequest processing so you can record the data. You should use the NetworkDelegate for this. Do not inherit from this interface. Instead, write your class to have the appropriate public non-virtual member functions and add an instance of this class as a member variable in ChromeNetworkDelegate. In ChromeNetworkDelegate, at the appropriate points, forward the calls to this class here. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_interceptor.h:42: scoped_refptr<predictors::ResourcePrefetchPredictor> predictor_; Does ResourcePrefetchInterceptor own predictors::ResourcePrefetchPredictor? I assume so, since it's holding a reference to ResourcePrefetchPredictor. This confuses me, because ResourcePrefetchPredictor looks like it's owned by the Profile. Do you have a ownership reference cycle? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:45: public RefcountedProfileKeyedService { You need way more documentation. How is this class supposed to be used across threads? What's the threading model? What data is accessed on which thread? I see you've marked it as refcounted...why? Who owns this class? Who is supposed to acquire a refcount?
https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_interceptor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_interceptor.h:12: #include "net/url_request/url_request_job_factory.h" On 2012/05/21 16:16:53, dominich wrote: > nit: this include should be first as it defines the base class. No longer required. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_interceptor.h:21: : public net::URLRequestJobFactory::Interceptor { On 2012/05/21 20:20:18, willchan wrote: > No, do not use this interface. You're not intercepting the URLRequest job > creation in order to replace it with another URLRequestJob. You just want > callbacks at different stages of URLRequest processing so you can record the > data. You should use the NetworkDelegate for this. Do not inherit from this > interface. Instead, write your class to have the appropriate public non-virtual > member functions and add an instance of this class as a member variable in > ChromeNetworkDelegate. In ChromeNetworkDelegate, at the appropriate points, > forward the calls to this class here. Done, https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/net/res... chrome/browser/net/resource_prefetch_interceptor.h:42: scoped_refptr<predictors::ResourcePrefetchPredictor> predictor_; On 2012/05/21 20:20:18, willchan wrote: > Does ResourcePrefetchInterceptor own predictors::ResourcePrefetchPredictor? I > assume so, since it's holding a reference to ResourcePrefetchPredictor. This > confuses me, because ResourcePrefetchPredictor looks like it's owned by the > Profile. Do you have a ownership reference cycle? ResourcePrefetchPredictor is refcounted as it needs to be accessed both in the UI and in the IO threads. Most of its functions need to be called in the UI thread, but I need to have a pointer to it in the IO thread from where I post tasks to it. I don't think there is a cycle here as the predictor itself does not hold a refcounted reference to anything. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.cc:30: if (render_process_id_ != rhs.render_process_id_) { On 2012/05/21 16:16:53, dominich wrote: > nit: braces unnecessary throughout. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.cc:40: return render_process_id_ == rhs.render_process_id_ && On 2012/05/21 16:16:53, dominich wrote: > use IsSameRenderer here. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:19: struct NavigationID { On 2012/05/21 16:16:53, dominich wrote: > This should be a class (it has constructors, operators, and methods) and should > be in its own header. I don't think it is significant enough to warrant being a class. The members also need to be public like a struct. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:21: NavigationID(const NavigationID& other); On 2012/05/21 16:16:53, dominich wrote: > Try explicit here - I don't know if STL is using direct initialization or copy > initialization off the top of my head, but if you can get away with adding > explicit it might help enforce passing by reference. STL doenst like it. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:22: NavigationID(const content::WebContents& web_contents); On 2012/05/21 16:16:53, dominich wrote: > This should be explicit as you call it directly. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:28: int render_process_id_; On 2012/05/21 16:16:53, dominich wrote: > Do these all need to be public? Unless we want to expose each member through functions, yes. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:38: // If a navigation hasn't seen a load complete event in this much time, it is On 2012/05/21 16:16:53, dominich wrote: > How much work has there been to tune these numbers? Do you expect them to > require experimentation? Some of these numbers do not need experimentation like kMaxNavigationLifetimeSeconds. Other we might need to change based on the histograms. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:53: // Dont store subresources whose Urls are longer than this. On 2012/05/21 16:16:53, dominich wrote: > nit: Don't Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:62: ResourceType::Type GetResourceTypeFromMimeType(std::string mime_type, On 2012/05/21 16:16:53, dominich wrote: > const std::string& mime_type to save the copy. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:94: bool ResourcePrefetchPredictor::URLRequestSummary::InitFromURLRequest( On 2012/05/21 16:16:53, dominich wrote: > TODO: check if this return value is used. It is used in the interceptor(now the network delegate). If we can't get the renderer (it may not be a renderer request) we dont bother recording it. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:100: LOG(ERROR) << "No ResourceRequestInfo in request"; On 2012/05/21 16:16:53, dominich wrote: > should this be a CHECK/DCHECK? No, as explained before. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:139: history_service->InMemoryDatabase(); On 2012/05/21 16:16:53, dominich wrote: > You're not using the result of this call - this might cause a compile error on > some platform. Should not. There are examples of this in the codebase. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:154: bool ResourcePrefetchPredictor::IsEnabled() { On 2012/05/21 16:16:53, dominich wrote: > These methods are not ordered as in the header - please rearrange them. Also, I > like the practice of labeling static methods with a // static comment. Added //static. I will reorder the function before checking in just to make sure that the diff are not lost. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:175: // TODO(shishir): The following would be much more efficient if we used On 2012/05/21 16:16:53, dominich wrote: > Can you use insertion sort to sort them as they're added instead? Will that be more efficient? They would still have the copying problem. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:200: switch (request_info->GetResourceType()) { On 2012/05/21 16:16:53, dominich wrote: > could be: > > return request_info->GetResourceType() == ResourceType::MAIN_FRAME && > IsHandledMainPage(request); > Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:215: switch (request_info->GetResourceType()) { On 2012/05/21 16:16:53, dominich wrote: > could be: > > return request_info->GetResourceType() == ResourceType::MAIN_FRAME ? > IsHandledMainPage(response) : > IsHandledSubresource(response); Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:232: switch (request_info->GetResourceType()) { On 2012/05/21 16:16:53, dominich wrote: > As above Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:241: if (request->original_url().scheme() != chrome::kHttpScheme) On 2012/05/21 16:16:53, dominich wrote: > there's too many negatives here. How about: > > return request->original_url().scheme() == chrome::kHttpScheme; Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:252: // Check the scheme of the orign. We only do http. On 2012/05/21 16:16:53, dominich wrote: > nit: origin. Also, consider if these comments are actually useful. I think the > code is obvious enough that the comments don't add anything. Removed trivial comments. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:276: if (!is_cacheable) On 2012/05/21 16:16:53, dominich wrote: > return is_cacheable; Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:282: bool ResourcePrefetchPredictor::IsCacheable(net::URLRequest* response) { On 2012/05/21 16:16:53, dominich wrote: > const net::URLRequest* response? Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:283: // If this was serverd from cache, we are good. On 2012/05/21 16:16:53, dominich wrote: > nit: served Removed comment. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:301: switch (request.resource_type_) { On 2012/05/21 16:16:53, dominich wrote: > I find 'if' much more readable in the case of a single condition. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:314: switch (response.resource_type_) { On 2012/05/21 16:16:53, dominich wrote: > if/else is more readable Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:328: case ResourceType::MAIN_FRAME: On 2012/05/21 16:16:53, dominich wrote: > if rather than switch. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:343: // TODO(shishir): Maybe fix it. On 2012/05/21 16:16:53, dominich wrote: > Enter a bug for this and reference it here. This should not be an issue now that we dont use an interceptor. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:358: inflight_navigations_[request.navigation_id_]; On 2012/05/21 16:16:53, dominich wrote: > Please use insert here with a default constructed > std::vector<UrlRequestSummary>() to make it clear what's going on. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:367: // TODO(shishir): The prefreshing will be stopped here. On 2012/05/21 16:16:53, dominich wrote: > Can you add this as part of this CL? The entire prefreshing is missing from this CL. Tony suggested we just measure accuracy first. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:376: inflight_navigations_.erase(response.navigation_id_); On 2012/05/21 16:16:53, dominich wrote: > can you add a comment explaining why we're not stopping prefreshing here? Because we are not actually doing any prefreshing in this CL :) We are only learning. I will add the comment when we actually do the prefreshing. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:399: summary.resource_type_ = ResourceType::LAST_TYPE; // Dont have type here. On 2012/05/21 16:16:53, dominich wrote: > You could add it - WebContentsImpl::OnDidLoadResourceFromMemoryCache has the > type. The resource_type is not very accurate. I am adding a CL to get the mime_type. Adding a todo here to use it once available. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:411: it != inflight_navigations_.end();) { On 2012/05/21 16:16:53, dominich wrote: > You can increment |it| in the for loop as map::erase doesn't invalidate the > iterator. That actually shouldn't work because 'it' itself should become invalid after the erase. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:431: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/05/21 16:16:53, dominich wrote: > DCHECK instead of CHECK. I have all the thread checks as CHECKS and DCHECKs. I see the same in some other files. Any reason this should be a DCHECK? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:476: if (history_service && history_service->InMemoryDatabase()) { On 2012/05/21 16:16:53, dominich wrote: > I think you can DCHECK on history_service, or even assume that it's available, > given when this method is called. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:477: history::URLDatabase* url_db = history_service->InMemoryDatabase(); On 2012/05/21 16:16:53, dominich wrote: > Then you can store this in the local var outside the conditional and save a call > to InMemoryDatabase. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:481: it != url_table_cache_.end();) { On 2012/05/21 16:16:53, dominich wrote: > Increment |it| in the for loop. map::erase doesn't invalidate iterators. As above the after the erase call, the 'it' itself becomes invalid so I have to ++ it in the erase call or create a copy. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:536: if (inflight_navigations_.find(navigation_id) == inflight_navigations_.end()) On 2012/05/21 16:16:53, dominich wrote: > So make it a DCHECK :) Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:544: if (url_table_cache_.find(main_frame_url) != url_table_cache_.end() || On 2012/05/21 16:16:53, dominich wrote: > Is it worth putting the cache_ check inside ShouldTrackUrl? Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:555: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/05/21 16:16:53, dominich wrote: > Can any of this be done as a PostTask to avoid blocking the UI loop? We could potentially post this to the DB thread let it do the math and report back the results. Do you think it is worth it? This happens once per page load. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:562: for (int i = 0; i < static_cast<int>(new_resources.size()); ++i) { On 2012/05/21 16:16:53, dominich wrote: > cache the loop end variable outside the loop to avoid recalculation every > iteration. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:577: for (int i = 0; i < static_cast<int>(new_resources.size()); ++i) { On 2012/05/21 16:16:53, dominich wrote: > cache the loop end var in a local variable. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:583: for (int i = 0; i < static_cast<int>(old_resources.size()); ++i) { On 2012/05/21 16:16:53, dominich wrote: > cache the loop end var in a local variable. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:590: for (int i = 0; i < static_cast<int>(old_resources.size()); ++i) { On 2012/05/21 16:16:53, dominich wrote: > cache loop end var. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:593: old_row.number_of_misses_++; On 2012/05/21 16:16:53, dominich wrote: > prefer pre-increment. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:597: new_resources[new_index[old_row.resource_url_]]; On 2012/05/21 16:16:53, dominich wrote: > you could remove it from new_resources here to make the following loop smaller. Wont removing the struct from the vector be more inefficient than just iterating over it once? https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:599: // Update the resource type if its missing. On 2012/05/21 16:16:53, dominich wrote: > nit: it's Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:601: old_row.resource_type_ = new_row.resource_type_; On 2012/05/21 16:16:53, dominich wrote: > what if the resource_type has changed? unlikely, but an edge case to consider. Fixed. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:607: old_row.number_of_hits_++; On 2012/05/21 16:16:53, dominich wrote: > prefer pre-increment. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:613: for (int i = 0; i < static_cast<int>(new_resources.size()); ++i) { On 2012/05/21 16:16:53, dominich wrote: > cache loop var. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:615: if (old_index.find(summary.resource_url_) != old_index.end()) On 2012/05/21 16:16:53, dominich wrote: > This would be a DCHECK if you remove from new_resources above. Pending reply on the above comemnt. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:634: for (int i = rows.size() - 1; i >= 0; --i) { On 2012/05/21 16:16:53, dominich wrote: > use iterator loop here. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:692: for (int i = 0; i < static_cast<int>(actual.size()); ++i) { On 2012/05/21 16:16:53, dominich wrote: > use iterator loop. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:702: prefetch_missed++; On 2012/05/21 16:16:53, dominich wrote: > prefer pre-increment Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:704: prefetch_cached++; On 2012/05/21 16:16:53, dominich wrote: > prefer pre-increment Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:706: prefetch_network++; On 2012/05/21 16:16:53, dominich wrote: > prefer pre-increment Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:42: // - All functions apart form constructor/destructor needs to be called on On 2012/05/21 16:16:53, dominich wrote: > nit: form-> from, needs -> need removed comment. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:42: // - All functions apart form constructor/destructor needs to be called on On 2012/05/21 16:16:53, dominich wrote: > this isn't accurate - the static methods are not thread-dependent. removed comment. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:45: public RefcountedProfileKeyedService { On 2012/05/21 20:20:18, willchan wrote: > You need way more documentation. How is this class supposed to be used across > threads? What's the threading model? What data is accessed on which thread? I > see you've marked it as refcounted...why? Who owns this class? Who is supposed > to acquire a refcount? Added documentation. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:53: URLRequestSummary(const URLRequestSummary& other); On 2012/05/21 16:16:53, dominich wrote: > Try explicit here - I don't know if STL is using direct initialization or copy > initialization off the top of my head, but if you can get away with adding > explicit it might help enforce passing by reference. Doesnt work with STL. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:57: NavigationID navigation_id_; On 2012/05/21 16:16:53, dominich wrote: > is there any need for these members to be public? Not sure I understand. Are you suggesting making the ResourcePrefetchPredictor a friend of this class or adding accessors; https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_factory.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_factory.cc:30: // TODO(shishir): When HistoryService is PKSFized, uncomment this. On 2012/05/21 16:16:53, dominich wrote: > Is there a bug to track this? Added. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:59: average_position_(0), On 2012/05/21 16:16:53, dominich wrote: > warning: initializing a double with an int. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:75: void ResourcePrefetchPredictorTables::UrlTableRow::UpdateScore() { On 2012/05/21 16:16:53, dominich wrote: > Comment explaining the heuristic here. Also, might this heuristic change or > might there be alternative field trials? Added comment. We might have additional field trials, but we want to start with this simple heuristic. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:87: default: On 2012/05/21 16:16:53, dominich wrote: > What other resource types should be handled? Maybe you can NOTREACHED in the > default case. Sometimes we do not know the type of resource, eg if a resource is loaded by webkit inmemory cache or obtained through a 304. We still want to prefetch it, since its cacheable. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:123: if (!DB()->DoesTableExist(kResourcePredictorUrlTableName)) On 2012/05/21 16:16:53, dominich wrote: > add braces as the inner if has them. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:130: void ResourcePrefetchPredictorTables::LogDatabaseStats() { On 2012/05/21 16:16:53, dominich wrote: > Consider tracking the raw file size. That happens in the PredictorDatabaseInternal. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:157: url_row.UpdateScore(); On 2012/05/21 16:16:53, dominich wrote: > why don't you store the score? Not storing it right now because we want to run experiments and keep changing it. May store it in the future if its not a simple function of the other fields. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:171: sql::Statement delete_statement(DB()->GetUniqueStatement( On 2012/05/21 16:16:53, dominich wrote: > would it be more optimal to SELECT and then UPDATE/INSERT as necessary? May not be, since we will end up updating all the rows for the URL. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:12: #include "chrome/browser/predictors/predictor_table_base.h" On 2012/05/21 16:16:53, dominich wrote: > This include should be first as it provides the base class. Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:26: UrlTableRow(const UrlTableRow& other); On 2012/05/21 16:16:53, dominich wrote: > check if you can mark this explicit - STL may not use direct-initialization but > if it does, you can help discourage pass-by-value. Doesnt work with STL. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:35: double average_position_; On 2012/05/21 16:16:53, dominich wrote: > Can this be a float? Since sql/statement doesnt expose float , keeping it double to be consistent. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:38: double score_; On 2012/05/21 16:16:53, dominich wrote: > Can this be a float? Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:41: // Sorts the UrlTableRows by score. On 2012/05/21 16:16:53, dominich wrote: > ascending or descending? Done. https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/ui/webu... File chrome/browser/ui/webui/predictors/predictors_handler.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/1/chrome/browser/ui/webu... chrome/browser/ui/webui/predictors/predictors_handler.cc:101: for (int i = 0; i < static_cast<int>(it->second.rows_.size()); ++i) { On 2012/05/21 16:16:53, dominich wrote: > use iterator loop Done.
generally looks good, but i'm confused about the ownership of the Observer vs Predictor. I think, from William's comments, that he is too. Perhaps this needs some more thought about exactly where ownership of each part is established. http://codereview.chromium.org/10416002/diff/2002/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/10416002/diff/2002/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:184: if (resource_prefetch_predictor_observer_.get()) Is there a way to create a ProfileKeyedService on IO? If so, having the RPPObserver be created that way would clarify ownership. http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_common.cc (right): http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_common.cc:26: main_frame_url_(web_contents.GetURL()) { not initializing creation_time here? http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:81: : was_cached_(false) { initialize resource_type_ to something sensible? http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:160: switches::kEnableSpeculativeResourcePrefetching); to confirm - this is currently only enabled by explicit command line. There's no field trial to enable it for a small %age of users? http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:177: // Score and sort the database caches. comment is inaccurate - scoring is updated in GetAll. This just sorts them. http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:543: UrlTableRowVector& old_resources = url_table_cache_[main_frame_url].rows_; i'm really worried about how slow this will be. Putting it on the DB thread is not really a solution, but I urge you to keep an eye on about:profiler when this code is enabled and monitor how long it blocks the UI thread. I believe you can PostTask to the message loop on this thread and it will run at some point later, which might be reasonable. http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:685: UMA_HISTOGRAM_PERCENTAGE( it might be more flexible to report the total counts and calculate the %age offline. http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.h (right): http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.h:51: // * ResourcePrefetchPredictorObserver - Listens for URL requests, responses and who owns this? http://codereview.chromium.org/10416002/diff/2002/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.h:56: // * ResourcePrefetchPredictor - Learns about resource requirements per URL in document who owns this http://codereview.chromium.org/10416002/diff/2002/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/10416002/diff/2002/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:178: scoped_refptr<predictors::ResourcePrefetchPredictor> comment applies to |profile| below. So move this. http://codereview.chromium.org/10416002/diff/2002/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:178: scoped_refptr<predictors::ResourcePrefetchPredictor> this is the source of my confusion: in Profile IO data you have a ref to the UI-based Predictor. This is then passed to the NetworkDelegate which creates an IO-based Observer which communicates with the UI-based predictor. Why not have the Observer created and owned by the IO data and have it get the reference to the Predictor as necessary?
https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... chrome/browser/net/resource_prefetch_predictor_observer.cc:54: void(predictors::ResourcePrefetchPredictor::*recordFunction) recordFunction is not Chromium/Google naming style. Looks like WebKit :) Can you fix? https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... chrome/browser/net/resource_prefetch_predictor_observer.cc:61: BrowserThread::PostTask(BrowserThread::UI, How is this safe if the Profile goes away? I know we only delete it right now when the Profile is stopped, but what about when we fix http://code.google.com/p/chromium/issues/detail?id=88586? https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:99: const content::ResourceRequestInfo* info = OIC, this is all only for RDH requests? Then maybe you should hook in via the ChromeResourceDispatcherHostDelegate instead of ChromeNetworkDelegate. ChromeNetworkDelegate hooks *all* network requests for a profile, whereas ChromeResourceDispatcherHostDelegate only hooks the ones issued by ResourceDispatcherHost, which sounds like what you want. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:145: BrowserThread::PostTaskAndReply( How about PostTaskAndReplyWithResult()? http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/bro... Also, have you considered delaying this? I'm worried about disk I/O contention on startup. It seems like this is unimportant, so it'd be OK to delay its startup a bit. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:197: bool ResourcePrefetchPredictor::ShouldRecordRequest(net::URLRequest* request) { Can we use a const net::URLRequest*? Ditto for these other guys. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:232: bool ResourcePrefetchPredictor::IsHandledMainPage(net::URLRequest* request) { const net::URLRequest&? https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:267: bool ResourcePrefetchPredictor::IsCacheable(const net::URLRequest* response) { const net::URLRequest&? https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.h:65: virtual ~ResourcePrefetchPredictor(); If this is refcounted...should this be private? https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor_tables.h:23: class ResourcePrefetchPredictorTables : public PredictorTableBase { Why is PredictorTableBase inherited? The Google style guide recommends composition over implementation inheritance. Then for the virtuals, just pass in a base::Callback to the constructor or something instead. If this is non-trivial extra work for you, don't let this block you. But in general, I like to encourage composition over implementation inheritance.
Hi Will, Dominich, I have moved the code to use WeakPtrs instead of being refcounted, though I am sure it will make my future CLs messier. I have also moved the code to use the RenderViewHostDelegate instead of ChromeNetworkDelegate. Please take a look. Thanks Shishir https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... File chrome/browser/net/chrome_network_delegate.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... chrome/browser/net/chrome_network_delegate.cc:184: if (resource_prefetch_predictor_observer_.get()) On 2012/05/24 16:07:53, dominich wrote: > Is there a way to create a ProfileKeyedService on IO? If so, having the > RPPObserver be created that way would clarify ownership. The ProfileKeyedServices are all created together on the UI thread when the prefs are loaded and there doesnt seem to be a way of creating one separately on IO. Please look at the new ownership. Much clearer now. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... chrome/browser/net/resource_prefetch_predictor_observer.cc:54: void(predictors::ResourcePrefetchPredictor::*recordFunction) On 2012/05/24 22:28:52, willchan wrote: > recordFunction is not Chromium/Google naming style. Looks like WebKit :) Can you > fix? Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/... chrome/browser/net/resource_prefetch_predictor_observer.cc:61: BrowserThread::PostTask(BrowserThread::UI, On 2012/05/24 22:28:52, willchan wrote: > How is this safe if the Profile goes away? I know we only delete it right now > when the Profile is stopped, but what about when we fix > http://code.google.com/p/chromium/issues/detail?id=88586? Wont matter now since this is a weak ptr. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_common.cc:26: main_frame_url_(web_contents.GetURL()) { On 2012/05/24 16:07:53, dominich wrote: > not initializing creation_time here? There is no creation time in web_contents. It comes from net::UrlRequest. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:81: : was_cached_(false) { On 2012/05/24 16:07:53, dominich wrote: > initialize resource_type_ to something sensible? Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:99: const content::ResourceRequestInfo* info = On 2012/05/24 22:28:52, willchan wrote: > OIC, this is all only for RDH requests? Then maybe you should hook in via the > ChromeResourceDispatcherHostDelegate instead of ChromeNetworkDelegate. > ChromeNetworkDelegate hooks *all* network requests for a profile, whereas > ChromeResourceDispatcherHostDelegate only hooks the ones issued by > ResourceDispatcherHost, which sounds like what you want. Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:145: BrowserThread::PostTaskAndReply( On 2012/05/24 22:28:52, willchan wrote: > How about PostTaskAndReplyWithResult()? > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/bro... > > Also, have you considered delaying this? I'm worried about disk I/O contention > on startup. It seems like this is unimportant, so it'd be OK to delay its > startup a bit. Delayed to first navigation seen. The PostTaskAndReply seems cleaner since I just want to fill in the vector in place. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:160: switches::kEnableSpeculativeResourcePrefetching); On 2012/05/24 16:07:53, dominich wrote: > to confirm - this is currently only enabled by explicit command line. There's no > field trial to enable it for a small %age of users? No field trial at the moment. Just the command line flag. Will add the field trial once this CL goes in. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:177: // Score and sort the database caches. On 2012/05/24 16:07:53, dominich wrote: > comment is inaccurate - scoring is updated in GetAll. This just sorts them. Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:197: bool ResourcePrefetchPredictor::ShouldRecordRequest(net::URLRequest* request) { On 2012/05/24 22:28:52, willchan wrote: > Can we use a const net::URLRequest*? Ditto for these other guys. Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:232: bool ResourcePrefetchPredictor::IsHandledMainPage(net::URLRequest* request) { On 2012/05/24 22:28:52, willchan wrote: > const net::URLRequest&? Same comment as above. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:267: bool ResourcePrefetchPredictor::IsCacheable(const net::URLRequest* response) { On 2012/05/24 22:28:52, willchan wrote: > const net::URLRequest&? Same as above. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:543: UrlTableRowVector& old_resources = url_table_cache_[main_frame_url].rows_; On 2012/05/24 16:07:53, dominich wrote: > i'm really worried about how slow this will be. Putting it on the DB thread is > not really a solution, but I urge you to keep an eye on about:profiler when this > code is enabled and monitor how long it blocks the UI thread. > > I believe you can PostTask to the message loop on this thread and it will run at > some point later, which might be reasonable. Will look at the profiler in more detail over this. Will posting it back in the same loop help? Wont that take still block the UI thread although at a later stage? https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.cc:685: UMA_HISTOGRAM_PERCENTAGE( On 2012/05/24 16:07:53, dominich wrote: > it might be more flexible to report the total counts and calculate the %age > offline. Will we be able to do that? We will only get aggregate percentages, but not percentage brackets. Isnt this more useful? https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.h:51: // * ResourcePrefetchPredictorObserver - Listens for URL requests, responses and On 2012/05/24 16:07:53, dominich wrote: > who owns this? Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.h:56: // * ResourcePrefetchPredictor - Learns about resource requirements per URL in On 2012/05/24 16:07:53, dominich wrote: > document who owns this Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor.h:65: virtual ~ResourcePrefetchPredictor(); On 2012/05/24 22:28:52, willchan wrote: > If this is refcounted...should this be private? No longer refcounted. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/pred... chrome/browser/predictors/resource_prefetch_predictor_tables.h:23: class ResourcePrefetchPredictorTables : public PredictorTableBase { On 2012/05/24 22:28:52, willchan wrote: > Why is PredictorTableBase inherited? The Google style guide recommends > composition over implementation inheritance. > > Then for the virtuals, just pass in a base::Callback to the constructor or > something instead. > > If this is non-trivial extra work for you, don't let this block you. But in > general, I like to encourage composition over implementation inheritance. I added a todo for this. I would like to do that in another CL as this will touch other files outside the scope of this CL. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/prof... File chrome/browser/profiles/profile_io_data.h (right): https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/prof... chrome/browser/profiles/profile_io_data.h:178: scoped_refptr<predictors::ResourcePrefetchPredictor> On 2012/05/24 16:07:53, dominich wrote: > comment applies to |profile| below. So move this. Done. https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/prof... chrome/browser/profiles/profile_io_data.h:178: scoped_refptr<predictors::ResourcePrefetchPredictor> On 2012/05/24 16:07:53, dominich wrote: > this is the source of my confusion: in Profile IO data you have a ref to the > UI-based Predictor. This is then passed to the NetworkDelegate which creates an > IO-based Observer which communicates with the UI-based predictor. > > Why not have the Observer created and owned by the IO data and have it get the > reference to the Predictor as necessary? Done.
On Tue, May 29, 2012 at 6:07 PM, <shishir@chromium.org> wrote: > Hi Will, Dominich, > > I have moved the code to use WeakPtrs instead of being refcounted, though > I am > sure it will make my future CLs messier. I have also moved the code to use > the > RenderViewHostDelegate instead of ChromeNetworkDelegate. Please take a > look. > https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr... has where I discuss refcounted vs WeakPtrs in shutdown. If you prefer refcounted, that's OK. Just be aware of the downsides with the ambiguity of ownership. I'll review the updated CL tomorrow. > > Thanks > Shishir > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/net/**chrome_network_delegate.cc<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/chrome_network_delegate.cc> > File chrome/browser/net/chrome_**network_delegate.cc (right): > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/net/**chrome_network_delegate.cc#**newcode184<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/chrome_network_delegate.cc#newcode184> > chrome/browser/net/chrome_**network_delegate.cc:184: if > (resource_prefetch_predictor_**observer_.get()) > > On 2012/05/24 16:07:53, dominich wrote: > >> Is there a way to create a ProfileKeyedService on IO? If so, having >> > the > >> RPPObserver be created that way would clarify ownership. >> > > The ProfileKeyedServices are all created together on the UI thread when > the prefs are loaded and there doesnt seem to be a way of creating one > separately on IO. > > Please look at the new ownership. Much clearer now. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/net/**resource_prefetch_predictor_**observer.cc<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/resource_prefetch_predictor_observer.cc> > File chrome/browser/net/resource_**prefetch_predictor_observer.cc (right): > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/net/**resource_prefetch_predictor_** > observer.cc#newcode54<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/resource_prefetch_predictor_observer.cc#newcode54> > chrome/browser/net/resource_**prefetch_predictor_observer.**cc:54: > void(predictors::**ResourcePrefetchPredictor::***recordFunction) > On 2012/05/24 22:28:52, willchan wrote: > >> recordFunction is not Chromium/Google naming style. Looks like WebKit >> > :) Can you > >> fix? >> > > Done. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/net/**resource_prefetch_predictor_** > observer.cc#newcode61<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/net/resource_prefetch_predictor_observer.cc#newcode61> > chrome/browser/net/resource_**prefetch_predictor_observer.**cc:61: > BrowserThread::PostTask(**BrowserThread::UI, > On 2012/05/24 22:28:52, willchan wrote: > >> How is this safe if the Profile goes away? I know we only delete it >> > right now > >> when the Profile is stopped, but what about when we fix >> http://code.google.com/p/**chromium/issues/detail?id=**88586<http://code.goog... >> ? >> > > Wont matter now since this is a weak ptr. > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**common.cc<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_common.cc> > File chrome/browser/predictors/**resource_prefetch_common.cc (right): > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**common.cc#newcode26<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_common.cc#newcode26> > chrome/browser/predictors/**resource_prefetch_common.cc:**26: > main_frame_url_(web_contents.**GetURL()) { > On 2012/05/24 16:07:53, dominich wrote: > >> not initializing creation_time here? >> > > There is no creation time in web_contents. It comes from > net::UrlRequest. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**predictor.cc<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc> > File chrome/browser/predictors/**resource_prefetch_predictor.cc (right): > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode81<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode81> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:81: : > was_cached_(false) { > > On 2012/05/24 16:07:53, dominich wrote: > >> initialize resource_type_ to something sensible? >> > > Done. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode99<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode99> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:99: const > content::ResourceRequestInfo* info = > On 2012/05/24 22:28:52, willchan wrote: > >> OIC, this is all only for RDH requests? Then maybe you should hook in >> > via the > >> ChromeResourceDispatcherHostDe**legate instead of ChromeNetworkDelegate. >> ChromeNetworkDelegate hooks *all* network requests for a profile, >> > whereas > >> ChromeResourceDispatcherHostDe**legate only hooks the ones issued by >> ResourceDispatcherHost, which sounds like what you want. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode145<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode145> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:145: > BrowserThread::**PostTaskAndReply( > On 2012/05/24 22:28:52, willchan wrote: > >> How about PostTaskAndReplyWithResult()? >> > > http://code.google.com/**searchframe#OAMlx_jo-ck/src/** > content/public/browser/**browser_thread.h&exact_**package=chromium&l=139<http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/browser_thread.h&exact_package=chromium&l=139> > > Also, have you considered delaying this? I'm worried about disk I/O >> > contention > >> on startup. It seems like this is unimportant, so it'd be OK to delay >> > its > >> startup a bit. >> > > Delayed to first navigation seen. The PostTaskAndReply seems cleaner > since I just want to fill in the vector in place. > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode160<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode160> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:160: > switches::**kEnableSpeculativeResourcePref**etching); > > On 2012/05/24 16:07:53, dominich wrote: > >> to confirm - this is currently only enabled by explicit command line. >> > There's no > >> field trial to enable it for a small %age of users? >> > > No field trial at the moment. Just the command line flag. Will add the > field trial once this CL goes in. > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode177<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode177> > > chrome/browser/predictors/**resource_prefetch_predictor.**cc:177: // Score > and sort the database caches. > On 2012/05/24 16:07:53, dominich wrote: > >> comment is inaccurate - scoring is updated in GetAll. This just sorts >> > them. > > Done. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode197<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode197> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:197: bool > ResourcePrefetchPredictor::**ShouldRecordRequest(net::**URLRequest* > request) > { > On 2012/05/24 22:28:52, willchan wrote: > >> Can we use a const net::URLRequest*? Ditto for these other guys. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode232<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode232> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:232: bool > ResourcePrefetchPredictor::**IsHandledMainPage(net::**URLRequest* > request) { > On 2012/05/24 22:28:52, willchan wrote: > >> const net::URLRequest&? >> > > Same comment as above. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode267<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode267> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:267: bool > ResourcePrefetchPredictor::**IsCacheable(const net::URLRequest* response) > { > On 2012/05/24 22:28:52, willchan wrote: > >> const net::URLRequest&? >> > > Same as above. > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode543<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode543> > > chrome/browser/predictors/**resource_prefetch_predictor.**cc:543: > UrlTableRowVector& old_resources = > url_table_cache_[main_frame_**url].rows_; > On 2012/05/24 16:07:53, dominich wrote: > >> i'm really worried about how slow this will be. Putting it on the DB >> > thread is > >> not really a solution, but I urge you to keep an eye on about:profiler >> > when this > >> code is enabled and monitor how long it blocks the UI thread. >> > > I believe you can PostTask to the message loop on this thread and it >> > will run at > >> some point later, which might be reasonable. >> > > Will look at the profiler in more detail over this. Will posting it back > in the same loop help? Wont that take still block the UI thread although > at a later stage? > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor.cc#newcode685<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode685> > chrome/browser/predictors/**resource_prefetch_predictor.**cc:685: > UMA_HISTOGRAM_PERCENTAGE( > > On 2012/05/24 16:07:53, dominich wrote: > >> it might be more flexible to report the total counts and calculate the >> > %age > >> offline. >> > > Will we be able to do that? We will only get aggregate percentages, but > not percentage brackets. Isnt this more useful? > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**predictor.h<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.h> > File chrome/browser/predictors/**resource_prefetch_predictor.h (right): > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**predictor.h#newcode51<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.h#newcode51> > > chrome/browser/predictors/**resource_prefetch_predictor.h:**51: // * > ResourcePrefetchPredictorObser**ver - Listens for URL requests, responses > and > On 2012/05/24 16:07:53, dominich wrote: > >> who owns this? >> > > Done. > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**predictor.h#newcode56<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.h#newcode56> > > chrome/browser/predictors/**resource_prefetch_predictor.h:**56: // * > ResourcePrefetchPredictor - Learns about resource requirements per URL > in > On 2012/05/24 16:07:53, dominich wrote: > >> document who owns this >> > > Done. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**predictor.h#newcode65<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor.h#newcode65> > chrome/browser/predictors/**resource_prefetch_predictor.h:**65: virtual > ~ResourcePrefetchPredictor(); > On 2012/05/24 22:28:52, willchan wrote: > >> If this is refcounted...should this be private? >> > > No longer refcounted. > > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_**predictor_tables.h<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor_tables.h> > File chrome/browser/predictors/**resource_prefetch_predictor_**tables.h > (right): > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/**predictors/resource_prefetch_** > predictor_tables.h#newcode23<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/predictors/resource_prefetch_predictor_tables.h#newcode23> > chrome/browser/predictors/**resource_prefetch_predictor_**tables.h:23: > class > ResourcePrefetchPredictorTable**s : public PredictorTableBase { > On 2012/05/24 22:28:52, willchan wrote: > >> Why is PredictorTableBase inherited? The Google style guide recommends >> composition over implementation inheritance. >> > > Then for the virtuals, just pass in a base::Callback to the >> > constructor or > >> something instead. >> > > If this is non-trivial extra work for you, don't let this block you. >> > But in > >> general, I like to encourage composition over implementation >> > inheritance. > > I added a todo for this. I would like to do that in another CL as this > will touch other files outside the scope of this CL. > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/profiles/**profile_io_data.h<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/profiles/profile_io_data.h> > File chrome/browser/profiles/**profile_io_data.h (right): > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/profiles/**profile_io_data.h#newcode178<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/profiles/profile_io_data.h#newcode178> > > chrome/browser/profiles/**profile_io_data.h:178: > scoped_refptr<predictors::**ResourcePrefetchPredictor> > On 2012/05/24 16:07:53, dominich wrote: > >> comment applies to |profile| below. So move this. >> > > Done. > > https://chromiumcodereview.**appspot.com/10416002/diff/** > 2002/chrome/browser/profiles/**profile_io_data.h#newcode178<https://chromiumcodereview.appspot.com/10416002/diff/2002/chrome/browser/profiles/profile_io_data.h#newcode178> > > chrome/browser/profiles/**profile_io_data.h:178: > scoped_refptr<predictors::**ResourcePrefetchPredictor> > On 2012/05/24 16:07:53, dominich wrote: > >> this is the source of my confusion: in Profile IO data you have a ref >> > to the > >> UI-based Predictor. This is then passed to the NetworkDelegate which >> > creates an > >> IO-based Observer which communicates with the UI-based predictor. >> > > Why not have the Observer created and owned by the IO data and have it >> > get the > >> reference to the Predictor as necessary? >> > > Done. > > https://chromiumcodereview.**appspot.com/10416002/<https://chromiumcodereview... >
Getting close I think. The weak pointer is much clearer than the refcounting, but we'll see how it works out with future CLs. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:33: ResourcePrefetchPredictor::URLRequestSummary summary; I think it's Chromium-style to create and initialize structs like this but it makes me really nervous. There's no way to catch uninitialized data with this style initialization. I would prefer a constructor that takes each of the below as parameters, though I understand that it would make URLRequestSummary non-POD (at least until C++11...) and you might need the default constructor for STL use anyway. This shouldn't block the CL, I just wanted to rant a bit. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:54: ResourcePrefetchPredictor::URLRequestSummary summary; similar to the above - I prefer a constructor that takes a request and a boolean, though you'd then need an is_valid() method. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.cc:29: bool NavigationID::operator<(const NavigationID& rhs) const { you should check for -1 for process/view, or add an is_valid method that you can check. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.cc:38: bool NavigationID::operator==(const NavigationID& rhs) const { check for -1 process/view https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.cc:42: bool NavigationID::IsSameRenderer(const NavigationID& other) const { check for -1 process/view https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_common.h (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.h:20: NavigationID(); would you consider a constructor that takes the process, view, and URL to ensure complete construction? https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:80: ResourcePrefetchPredictor::URLRequestSummary::URLRequestSummary() initialize resource_type_ and mime_type_ to something sensible https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:181: url_table_cache_[it->main_frame_url_].rows_.push_back(*it); initialize last_visit_ to something useful? https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:290: LazilyInitialize(); You could record the request in a pending list to process when initialization is complete, if you don't want to miss these initial requests. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:292: } else if (initialization_state_ != INITIALIZED) { paranoid version: else if (initialization_state_ == INITIALIZING) return; DCHECK_EQ(INITALIZED, initialized_state_); https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:324: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); paranoid version: DCHECK_EQ(INITALIZED, initialized_state_); https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:361: inflight_navigations_.end()) it may be worth counting these in a histogram to determine if you're marking navigations as abandoned too aggressively. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:370: if (inflight_navigations_.find(navigation_id) == inflight_navigations_.end()) see above re counting https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:509: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.DidNavigationComplete", should this be counted after the check for inflight? Maybe you need three states: complete, complete_abandoned, incomplete. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:512: if (inflight_navigations_.find(navigation_id) == inflight_navigations_.end()) see above re counting https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:690: UMA_HISTOGRAM_PERCENTAGE( It's up to you to decide what stats are important. I found it more useful to have the raw counts to analyze rather than precomputed percentages. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:43: // - The class is a profile keyed service owned by the pofile. nit: profile https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:52: // ProfileIOData for the profile. note for willchan: Do you think we need a ProfileIOKeyedService for profile keyed services that exist on the IO thread? https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:61: // NOTE: The acutal prefetching is not yet implemented. Can you make this a TODO(shishir)? https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:126: if (!DB()->DoesTableExist(kResourcePredictorUrlTableName)) { nit: if (!DB()->DoesTableExist(...) && !DB()->Execute(...)) ResetDB(); https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:129: return; nit: unnecessary return https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:154: url_row_buffer->clear(); you could avoid the call to clear() and CHECK that it's empty instead - make that part of the API. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:185: for (int i = 0; i < static_cast<int>(row_buffer.size()); ++i) { use iterator loop https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:214: for (int i = 0; i < static_cast<int>(urls.size()); ++i) { iterator loop https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pro... chrome/browser/profiles/profile_io_data.cc:206: if (predictors::ResourcePrefetchPredictorFactory::GetForProfile(profile)) { nit: if (predictors::ResourcePrefetchPredictor* predictor = predictors::ResourcePrefetchPredictorFactory::GetForProfile(profile)) { resource_prefetch_predictor_observer_.reset( new chrome_browser_net::ResourcePrefetchPredictorObserver(predictor)); } https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/res... File chrome/browser/resources/predictors/predictors.css (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/res... chrome/browser/resources/predictors/predictors.css:63: nit: you have one extra blank line https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/ui/... File chrome/browser/ui/webui/predictors/predictors_handler.h (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/ui/... chrome/browser/ui/webui/predictors/predictors_handler.h:10: #include "base/memory/ref_counted.h" do you still need this include? https://chromiumcodereview.appspot.com/10416002/diff/17001/content/browser/re... File content/browser/renderer_host/async_resource_handler.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/content/browser/re... content/browser/renderer_host/async_resource_handler.cc:136: rdh_->delegate()->OnResponseStarted(request, resource_context, response, nit: rdh_->delegate()->OnResponseStarted( request, filter_->resource_context(), response, filter_); no need for a local var.
https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:33: ResourcePrefetchPredictor::URLRequestSummary summary; On 2012/05/30 15:35:01, dominich wrote: > I think it's Chromium-style to create and initialize structs like this but it > makes me really nervous. There's no way to catch uninitialized data with this > style initialization. I would prefer a constructor that takes each of the below > as parameters, though I understand that it would make URLRequestSummary non-POD > (at least until C++11...) and you might need the default constructor for STL use > anyway. > > This shouldn't block the CL, I just wanted to rant a bit. Added a todo to NavigationId to use a constructor with arguments that will lead to this also being rethought. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:54: ResourcePrefetchPredictor::URLRequestSummary summary; On 2012/05/30 15:35:01, dominich wrote: > similar to the above - I prefer a constructor that takes a request and a > boolean, though you'd then need an is_valid() method. As Above. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.cc:29: bool NavigationID::operator<(const NavigationID& rhs) const { On 2012/05/30 15:35:01, dominich wrote: > you should check for -1 for process/view, or add an is_valid method that you can > check. Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.cc:38: bool NavigationID::operator==(const NavigationID& rhs) const { On 2012/05/30 15:35:01, dominich wrote: > check for -1 process/view Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.cc:42: bool NavigationID::IsSameRenderer(const NavigationID& other) const { On 2012/05/30 15:35:01, dominich wrote: > check for -1 process/view Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_common.h (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.h:20: NavigationID(); On 2012/05/30 15:35:01, dominich wrote: > would you consider a constructor that takes the process, view, and URL to ensure > complete construction? This would lead to the UrlRequestSummary also having to take all those as input, in effect changing the InitMethod. I will add a todo here to take a look again at this. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:80: ResourcePrefetchPredictor::URLRequestSummary::URLRequestSummary() On 2012/05/30 15:35:01, dominich wrote: > initialize resource_type_ and mime_type_ to something sensible Seemed to have lost this change. Fixed. Mime_type will be empty as default. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:181: url_table_cache_[it->main_frame_url_].rows_.push_back(*it); On 2012/05/30 15:35:01, dominich wrote: > initialize last_visit_ to something useful? By default it will initialize to 0, and we will do an immediate filtering in the OnHistoryAndCacheLoaded. So is there a need to do anything else here? https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:290: LazilyInitialize(); On 2012/05/30 15:35:01, dominich wrote: > You could record the request in a pending list to process when initialization is > complete, if you don't want to miss these initial requests. That would add the initialization check all over the code, making it harder to understand. I think missing a request or two might be acceptable for now. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:292: } else if (initialization_state_ != INITIALIZED) { On 2012/05/30 15:35:01, dominich wrote: > paranoid version: > > else if (initialization_state_ == INITIALIZING) > return; > > DCHECK_EQ(INITALIZED, initialized_state_); Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:324: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/05/30 15:35:01, dominich wrote: > paranoid version: > > DCHECK_EQ(INITALIZED, initialized_state_); Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:361: inflight_navigations_.end()) On 2012/05/30 15:35:01, dominich wrote: > it may be worth counting these in a histogram to determine if you're marking > navigations as abandoned too aggressively. Added enum https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:370: if (inflight_navigations_.find(navigation_id) == inflight_navigations_.end()) On 2012/05/30 15:35:01, dominich wrote: > see above re counting same comment as above. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:509: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.DidNavigationComplete", On 2012/05/30 15:35:01, dominich wrote: > should this be counted after the check for inflight? Maybe you need three > states: complete, complete_abandoned, incomplete. Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:512: if (inflight_navigations_.find(navigation_id) == inflight_navigations_.end()) On 2012/05/30 15:35:01, dominich wrote: > see above re counting Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:690: UMA_HISTOGRAM_PERCENTAGE( On 2012/05/30 15:35:01, dominich wrote: > It's up to you to decide what stats are important. I found it more useful to > have the raw counts to analyze rather than precomputed percentages. Since both the numerator and denominator vary per request, having all stats on a single base makes sense. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:43: // - The class is a profile keyed service owned by the pofile. On 2012/05/30 15:35:01, dominich wrote: > nit: profile Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:61: // NOTE: The acutal prefetching is not yet implemented. On 2012/05/30 15:35:01, dominich wrote: > Can you make this a TODO(shishir)? Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:126: if (!DB()->DoesTableExist(kResourcePredictorUrlTableName)) { On 2012/05/30 15:35:01, dominich wrote: > nit: > > if (!DB()->DoesTableExist(...) && !DB()->Execute(...)) > ResetDB(); Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:129: return; On 2012/05/30 15:35:01, dominich wrote: > nit: unnecessary return Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:154: url_row_buffer->clear(); On 2012/05/30 15:35:01, dominich wrote: > you could avoid the call to clear() and CHECK that it's empty instead - make > that part of the API. Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:185: for (int i = 0; i < static_cast<int>(row_buffer.size()); ++i) { On 2012/05/30 15:35:01, dominich wrote: > use iterator loop Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:214: for (int i = 0; i < static_cast<int>(urls.size()); ++i) { On 2012/05/30 15:35:01, dominich wrote: > iterator loop Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/pro... chrome/browser/profiles/profile_io_data.cc:206: if (predictors::ResourcePrefetchPredictorFactory::GetForProfile(profile)) { On 2012/05/30 15:35:01, dominich wrote: > nit: > > if (predictors::ResourcePrefetchPredictor* predictor = > predictors::ResourcePrefetchPredictorFactory::GetForProfile(profile)) { > resource_prefetch_predictor_observer_.reset( > new chrome_browser_net::ResourcePrefetchPredictorObserver(predictor)); > } Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/res... File chrome/browser/resources/predictors/predictors.css (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/res... chrome/browser/resources/predictors/predictors.css:63: On 2012/05/30 15:35:01, dominich wrote: > nit: you have one extra blank line Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/ui/... File chrome/browser/ui/webui/predictors/predictors_handler.h (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/chrome/browser/ui/... chrome/browser/ui/webui/predictors/predictors_handler.h:10: #include "base/memory/ref_counted.h" On 2012/05/30 15:35:01, dominich wrote: > do you still need this include? Done. https://chromiumcodereview.appspot.com/10416002/diff/17001/content/browser/re... File content/browser/renderer_host/async_resource_handler.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/17001/content/browser/re... content/browser/renderer_host/async_resource_handler.cc:136: rdh_->delegate()->OnResponseStarted(request, resource_context, response, On 2012/05/30 15:35:01, dominich wrote: > nit: > rdh_->delegate()->OnResponseStarted( > request, filter_->resource_context(), response, filter_); > > no need for a local var. Its used in a couple of places so the var might be ok.
I don't see anywhere that ties into BrowsingDataRemover. Are you planning on hooking into that somewhere? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:17: : predictor_(predictor->AsWeakPtr()) { This needs to happen on the UI thread, right? Can you add the appropriate assertion here? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:20: ResourcePrefetchPredictorObserver::~ResourcePrefetchPredictorObserver() { This needs to happen on the IO thread, right? If so, can you add the appropriate assertion here? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:28: CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); Can we use DCHECK instead? I'd prefer to avoid the binary bloat. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.h:18: class ResourcePrefetchPredictorObserver { Can you add a class comment explaining what you're observing and why and where this class is expected to live? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.h:24: // Parts of the NetworkDelegate that we want to observe. Not NetworkDelegate https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.h:35: DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorObserver); include basictypes.h for this macro definition https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_common.h (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.h:34: int render_process_id_; If this is a struct, don't use trailing underscores. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:146: ResourcePrefetchPredictor::~ResourcePrefetchPredictor() { This is deleted on the UI thread, right? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:153: switches::kEnableSpeculativeResourcePrefetching); Is this supposed to dynamically checked? Do you want it dynamically configurable via about:flags at some point? Or would you rather initialize it only on startup? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:456: void ResourcePrefetchPredictor::OnHistoryAndCacheLoaded() { In locations like this where you populate internal member variables from persistent data stores (databases in this case), can you add a layer of indirection? Use a PersistenceDelegate or something here. The reasons are two-fold: * ease of testing with fake delegates * removes a dependency on chrome specifics, allowing for potential reuse in other codebases that want to do resource prefetching but don't want to be tied to desktop Chrome's persistence data stores. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:75: bool InitFromURLRequest(net::URLRequest* request, bool is_response); Why is this defined here? It seems to only be used in the observer. How about defining a local function in the observer's .cc file that handles this initialization. I say this because structs aren't supposed to have behavior according to the Google style guide. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:77: NavigationID navigation_id_; Remove the trailing underscores for structs. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:93: // UI thread. Everything's on the IO thread, right? Other than the statics. I'd remove this comment so people don't go looking for IO thread methods. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:99: friend class ::PredictorsHandler; Hm...do you really need this? AFAICT, this is only for UrlTableCacheMap. Why don't you add a const member function that returns a const reference to that guy? I guess it's a bit annoying to expose some structs and typedefs, but whatever. I think it's cleaner than friend declarations. Up to you guys though, since it's style preference and your guys' code. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:141: enum InitializationState { types go in the beginning of the access section, per style guide https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:147: Profile* profile_; Profile* const? Should never change, right? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:149: scoped_refptr<ResourcePrefetchPredictorTables> tables_; Doesn't this class own the ResourcePrefetchPredictorTables? I think this situation is pretty similar to ResourcePrefetchPredictorObserver. I think what you want to do is allocate the object here and use PostTaskAndReply() (you're already doing this) with a WeakPtr to |this|, and then in the destructor of ResourcePrefetchPredictor, post the deletion of the ResourcePrefetchPredictorTables object to the DB thread. That will provide more well-defined ownership semantics.
Addressed Will's comments, and also added a unittest. Will add the other unittest soon. Please take a look. Adding jam for: content/public/browser/resource_dispatcher_host_delegate.h content/public/browser/resource_dispatcher_host_delegate.cc content/browser/renderer_host/async_resource_handler.cc content/browser/renderer_host/sync_resource_handler.cc chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:17: : predictor_(predictor->AsWeakPtr()) { On 2012/05/31 02:08:58, willchan wrote: > This needs to happen on the UI thread, right? Can you add the appropriate > assertion here? Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:20: ResourcePrefetchPredictorObserver::~ResourcePrefetchPredictorObserver() { On 2012/05/31 02:08:58, willchan wrote: > This needs to happen on the IO thread, right? If so, can you add the appropriate > assertion here? Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:28: CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2012/05/31 02:08:58, willchan wrote: > Can we use DCHECK instead? I'd prefer to avoid the binary bloat. Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.h:18: class ResourcePrefetchPredictorObserver { On 2012/05/31 02:08:58, willchan wrote: > Can you add a class comment explaining what you're observing and why and where > this class is expected to live? Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.h:24: // Parts of the NetworkDelegate that we want to observe. On 2012/05/31 02:08:58, willchan wrote: > Not NetworkDelegate Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.h:35: DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorObserver); On 2012/05/31 02:08:58, willchan wrote: > include basictypes.h for this macro definition Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_common.h (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_common.h:34: int render_process_id_; On 2012/05/31 02:08:58, willchan wrote: > If this is a struct, don't use trailing underscores. Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:146: ResourcePrefetchPredictor::~ResourcePrefetchPredictor() { On 2012/05/31 02:08:58, willchan wrote: > This is deleted on the UI thread, right? Yes. Added assertion. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:153: switches::kEnableSpeculativeResourcePrefetching); On 2012/05/31 02:08:58, willchan wrote: > Is this supposed to dynamically checked? Do you want it dynamically configurable > via about:flags at some point? Or would you rather initialize it only on > startup? It can presently be set by about:flags. But i thought that still requires it to restart? https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:456: void ResourcePrefetchPredictor::OnHistoryAndCacheLoaded() { On 2012/05/31 02:08:58, willchan wrote: > In locations like this where you populate internal member variables from > persistent data stores (databases in this case), can you add a layer of > indirection? Use a PersistenceDelegate or something here. The reasons are > two-fold: > * ease of testing with fake delegates > * removes a dependency on chrome specifics, allowing for potential reuse in > other codebases that want to do resource prefetching but don't want to be tied > to desktop Chrome's persistence data stores. I think that it would be unnecessary at this stage because we are not yet sure whether this code will even remain in chrome. It will only remain if we see gains. At that stage it might make sense to create a persistence interface. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:75: bool InitFromURLRequest(net::URLRequest* request, bool is_response); On 2012/05/31 02:08:58, willchan wrote: > Why is this defined here? It seems to only be used in the observer. How about > defining a local function in the observer's .cc file that handles this > initialization. I say this because structs aren't supposed to have behavior > according to the Google style guide. Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:77: NavigationID navigation_id_; On 2012/05/31 02:08:58, willchan wrote: > Remove the trailing underscores for structs. Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:93: // UI thread. On 2012/05/31 02:08:58, willchan wrote: > Everything's on the IO thread, right? Other than the statics. I'd remove this > comment so people don't go looking for IO thread methods. Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:99: friend class ::PredictorsHandler; On 2012/05/31 02:08:58, willchan wrote: > Hm...do you really need this? AFAICT, this is only for UrlTableCacheMap. Why > don't you add a const member function that returns a const reference to that > guy? I guess it's a bit annoying to expose some structs and typedefs, but > whatever. I think it's cleaner than friend declarations. Up to you guys though, > since it's style preference and your guys' code. In the following CLs, we will be exposing much more data in the chrome:// page and it makes more sense to have this as a friend rather than to expose each underlying object. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:141: enum InitializationState { On 2012/05/31 02:08:58, willchan wrote: > types go in the beginning of the access section, per style guide Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:147: Profile* profile_; On 2012/05/31 02:08:58, willchan wrote: > Profile* const? Should never change, right? Done. https://chromiumcodereview.appspot.com/10416002/diff/25001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.h:149: scoped_refptr<ResourcePrefetchPredictorTables> tables_; On 2012/05/31 02:08:58, willchan wrote: > Doesn't this class own the ResourcePrefetchPredictorTables? I think this > situation is pretty similar to ResourcePrefetchPredictorObserver. I think what > you want to do is allocate the object here and use PostTaskAndReply() (you're > already doing this) with a WeakPtr to |this|, and then in the destructor of > ResourcePrefetchPredictor, post the deletion of the > ResourcePrefetchPredictorTables object to the DB thread. That will provide more > well-defined ownership semantics. I would like to defer this to another CL since this touches files not in this CL. Adding a todo in predictor_database.h for this. Will send out a CL for it once this one goes in.
lgtm for my files
I'm mostly fine with this, but I didn't review the implementation in detail. I'm deferring to dominich/tonyg for that. http://codereview.chromium.org/10416002/diff/26001/chrome/browser/net/resourc... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://codereview.chromium.org/10416002/diff/26001/chrome/browser/net/resourc... chrome/browser/net/resource_prefetch_predictor_observer.cc:18: bool SummarizeResponse(net::URLRequest* response, It's weird how you name the URLRequest a "response". It's a request. I recommend you rename strings appropriately. http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:486: bool ResourcePrefetchPredictor::ShouldTrackUrl(const GURL& url) { Does this member function need to be non-const? It's not clear from the function name what side effects it could have, if any at all.
http://codereview.chromium.org/10416002/diff/26001/chrome/browser/net/resourc... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): http://codereview.chromium.org/10416002/diff/26001/chrome/browser/net/resourc... chrome/browser/net/resource_prefetch_predictor_observer.cc:62: CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); Constructed on UI but destroyed on IO? http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:201: return request->original_url().scheme() == chrome::kHttpScheme; consider https main page. you can always compare the main page scheme to the resource scheme before requesting a resource to avoid loading http from https. http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:207: if (response->first_party_for_cookies().scheme() != chrome::kHttpScheme) consider https resources as long as original url is https. http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:225: if (response->original_url().spec().length() > kMaxSubresourceUrlLengthBytes) consider tracking these cancellation reasons in a histogram http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:285: if (response.resource_type == ResourceType::MAIN_FRAME) this can be: response.resource_type == ResourceType::MAIN_FRAME ? OnMainFrameResponse(response) : OnSubresourceResponse(response); which may or may not be clearer. http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:341: inflight_navigations_.end()) nit: braces http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:428: if (urls_deleted_details->all_history) could be: urls_deleted_details->all_history ? DeleteAllUrls() : DeleteUrls(urls_deleted_details->rows; http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:657: const NavigationID& navigation_id) { this shouldn't have side-effects: const http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor_factory.h (right): http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_factory.h:20: nit: remove blank line http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc (right): http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:20: ResourcePrefetchPredictorTables::UrlTableRow CreateUrlTableRow( put this in an anonymous namespace http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:45: virtual void SetUp(); OVERRIDE http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:46: virtual void TearDown(); OVERRIDE http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:55: scoped_ptr<PredictorDatabase> db_; can this just be an instance of a PredictorDatabase instead of a scoped ptr? http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:97: void ResourcePrefetchPredictorTablesTest::SetUp() { you could do this in the constructor http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:107: test_url_rows_.clear(); no need to call clear http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:108: tables_->DeleteAllRows(); you could do this in the destructor, if at all. http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:109: db_.reset(NULL); should be no need to call this reset(NULL) http://codereview.chromium.org/10416002/diff/26001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:215: void ResourcePrefetchPredictorTablesTest::InitializeSampleUrlData() { clear test_url_rows_ here
Added the last test, please take a look https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:18: bool SummarizeResponse(net::URLRequest* response, On 2012/06/05 01:34:46, willchan wrote: > It's weird how you name the URLRequest a "response". It's a request. I recommend > you rename strings appropriately. Done. There are some subtle differences when the UrlRequest is actually a response (like mime_type being present). I thought it would be clearer with this. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:62: CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2012/06/06 17:09:09, dominich wrote: > Constructed on UI but destroyed on IO? Yes. Constructed during the profile_io_data construction which happens on IO. Then it is owned by the ResourceDispatcherHostDelegate which lives on the IO thread. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:201: return request->original_url().scheme() == chrome::kHttpScheme; On 2012/06/06 17:09:09, dominich wrote: > consider https main page. you can always compare the main page scheme to the > resource scheme before requesting a resource to avoid loading http from https. Adding a TODO in the .h file. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:207: if (response->first_party_for_cookies().scheme() != chrome::kHttpScheme) On 2012/06/06 17:09:09, dominich wrote: > consider https resources as long as original url is https. Added comment in.h file. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:225: if (response->original_url().spec().length() > kMaxSubresourceUrlLengthBytes) On 2012/06/06 17:09:09, dominich wrote: > consider tracking these cancellation reasons in a histogram Done. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:285: if (response.resource_type == ResourceType::MAIN_FRAME) On 2012/06/06 17:09:09, dominich wrote: > this can be: > response.resource_type == ResourceType::MAIN_FRAME ? > OnMainFrameResponse(response) : > OnSubresourceResponse(response); > > which may or may not be clearer. It looks a little more readable to me :) https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:341: inflight_navigations_.end()) On 2012/06/06 17:09:09, dominich wrote: > nit: braces Done. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:428: if (urls_deleted_details->all_history) On 2012/06/06 17:09:09, dominich wrote: > could be: > > urls_deleted_details->all_history ? > DeleteAllUrls() : > DeleteUrls(urls_deleted_details->rows; Again looks better to me as is. Are we trying to optimize code length here? https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:486: bool ResourcePrefetchPredictor::ShouldTrackUrl(const GURL& url) { On 2012/06/05 01:34:46, willchan wrote: > Does this member function need to be non-const? It's not clear from the function > name what side effects it could have, if any at all. Unfortunately profile_->GetHistoryService is non const, making this function non const. I can pass the history service as an argument if you prefer that. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor.cc:657: const NavigationID& navigation_id) { On 2012/06/06 17:09:09, dominich wrote: > this shouldn't have side-effects: const Done. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor_factory.h (right): https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_factory.h:20: On 2012/06/06 17:09:09, dominich wrote: > nit: remove blank line Done. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:20: ResourcePrefetchPredictorTables::UrlTableRow CreateUrlTableRow( On 2012/06/06 17:09:09, dominich wrote: > put this in an anonymous namespace No longer required. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:45: virtual void SetUp(); On 2012/06/06 17:09:09, dominich wrote: > OVERRIDE Done. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:46: virtual void TearDown(); On 2012/06/06 17:09:09, dominich wrote: > OVERRIDE Done. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:55: scoped_ptr<PredictorDatabase> db_; On 2012/06/06 17:09:09, dominich wrote: > can this just be an instance of a PredictorDatabase instead of a scoped ptr? I want to reset it for the ResourcePrefetchPredictorTablesReopenTest https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:97: void ResourcePrefetchPredictorTablesTest::SetUp() { On 2012/06/06 17:09:09, dominich wrote: > you could do this in the constructor Done https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:107: test_url_rows_.clear(); On 2012/06/06 17:09:09, dominich wrote: > no need to call clear Removed. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:108: tables_->DeleteAllRows(); On 2012/06/06 17:09:09, dominich wrote: > you could do this in the destructor, if at all. Removed. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:109: db_.reset(NULL); On 2012/06/06 17:09:09, dominich wrote: > should be no need to call this reset(NULL) Done. https://chromiumcodereview.appspot.com/10416002/diff/26001/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:215: void ResourcePrefetchPredictorTablesTest::InitializeSampleUrlData() { On 2012/06/06 17:09:09, dominich wrote: > clear test_url_rows_ here Done.
Ping.
http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:200: if (response->first_party_for_cookies().scheme() != chrome::kHttpScheme) I meant something like the prerender::FinalStatus. Ie, an enum with: UNSUPPORTED_SCHEME UNSUPPORTED_MIME_TYPE UNSUPPORTED_METHOD URL_TOO_LONG http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:541: resources_seen.end()) nit: braces http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.h (right): http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.h:105: explicit ResourcePrefetchPredictor(const Config& config, Profile* profile); nit: no need for explicit now
http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:200: if (response->first_party_for_cookies().scheme() != chrome::kHttpScheme) On 2012/06/13 18:57:23, dominich wrote: > I meant something like the prerender::FinalStatus. Ie, an enum with: > > UNSUPPORTED_SCHEME > UNSUPPORTED_MIME_TYPE > UNSUPPORTED_METHOD > URL_TOO_LONG Done. http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:541: resources_seen.end()) On 2012/06/13 18:57:23, dominich wrote: > nit: braces Done. http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.h (right): http://codereview.chromium.org/10416002/diff/38001/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.h:105: explicit ResourcePrefetchPredictor(const Config& config, Profile* profile); On 2012/06/13 18:57:23, dominich wrote: > nit: no need for explicit now Done.
LGTM modulo fix below. http://codereview.chromium.org/10416002/diff/37008/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): http://codereview.chromium.org/10416002/diff/37008/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:62: RESOURCE_STATUS_COUNT = 64, COUNT should be MAX http://codereview.chromium.org/10416002/diff/37008/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:241: NAVIGATION_STATUS_COUNT); RESOURCE_STATUS_MAX
Hi Will, Could you LGTM this. http://codereview.chromium.org/10416002/diff/37008/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): http://codereview.chromium.org/10416002/diff/37008/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:62: RESOURCE_STATUS_COUNT = 64, On 2012/06/13 21:48:04, dominich wrote: > COUNT should be MAX Done. http://codereview.chromium.org/10416002/diff/37008/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor.cc:241: NAVIGATION_STATUS_COUNT); On 2012/06/13 21:48:04, dominich wrote: > RESOURCE_STATUS_MAX Done.
https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:51: } // namespace you're missing a horizontal whitespace between '}' and "//". https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... File chrome/browser/predictors/predictor_database.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... chrome/browser/predictors/predictor_database.cc:90: resource_prefetch_tables_->cancelled_.Set(); Isn't |cancelled_| an internal member variable? What's going on here? And |cancelled_| looks like a protected member variable, which is disallowed by the Google C++ style guide. https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.h:66: friend class PredictorDatabaseInternal; I'm surprised at this use of a friend declaration. Can you explain why this is a clean design? https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pro... chrome/browser/profiles/profile_io_data.cc:207: resource_prefetch_predictor_observer_.reset( Ditto here on the braces. https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pro... chrome/browser/profiles/profile_io_data.cc:563: resource_prefetch_predictor_observer_.reset( Check the Chromium style guide, you should be using braces around the conditional statement if it exceeds one line.
https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/net... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/net... chrome/browser/net/resource_prefetch_predictor_observer.cc:51: } // namespace On 2012/06/18 20:15:12, willchan wrote: > you're missing a horizontal whitespace between '}' and "//". Done. https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... File chrome/browser/predictors/predictor_database.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... chrome/browser/predictors/predictor_database.cc:90: resource_prefetch_tables_->cancelled_.Set(); On 2012/06/18 20:15:12, willchan wrote: > Isn't |cancelled_| an internal member variable? What's going on here? And > |cancelled_| looks like a protected member variable, which is disallowed by the > Google C++ style guide. Added an SetCancelled method for this. I had this in the initial refactored code and was asked to remove it since the variable was accessible here. https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pre... chrome/browser/predictors/resource_prefetch_predictor_tables.h:66: friend class PredictorDatabaseInternal; On 2012/06/18 20:15:12, willchan wrote: > I'm surprised at this use of a friend declaration. Can you explain why this is a > clean design? This is to give the PredictorDatabaseInternal access to constructor and initialization functions that the rest of the callers should not call. https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pro... chrome/browser/profiles/profile_io_data.cc:207: resource_prefetch_predictor_observer_.reset( On 2012/06/18 20:15:12, willchan wrote: > Ditto here on the braces. Done. https://chromiumcodereview.appspot.com/10416002/diff/39049/chrome/browser/pro... chrome/browser/profiles/profile_io_data.cc:563: resource_prefetch_predictor_observer_.reset( On 2012/06/18 20:15:12, willchan wrote: > Check the Chromium style guide, you should be using braces around the > conditional statement if it exceeds one line. Done.
chrome/browser/net/ and chrome/browser/profiles LGTM http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/... chrome/browser/predictors/resource_prefetch_predictor_tables.h:66: friend class PredictorDatabaseInternal; On 2012/06/18 20:38:33, Shishir wrote: > On 2012/06/18 20:15:12, willchan wrote: > > I'm surprised at this use of a friend declaration. Can you explain why this is > a > > clean design? > > This is to give the PredictorDatabaseInternal access to constructor and > initialization functions that the rest of the callers should not call. Why the constructor? What harm will happen from someone calling the constructor? LogDatabaseStats() is public in PredictorTableBase. What's the point of hiding it? I see...the primary consumer is the FooPredictor classes you have. This friendship pattern you are relying on is creating a tight coupling between PredictorDatabase and all its database tables. If that's what you want, it's fine. The friendship is definitely allowing you to establish an interface boundary between FooPredictor and the FooPredictorTable. What would be cleaner, albeit more code, is to get rid of this tight coupling and replace it with a FooPredictorTable interface and a FooPredictorTableImpl that did the initialization and what not (composing the PredictorTableBase rather than using implementation inheritance). But the FooPredictor would only interact with the FooPredictorTable interface, not the FooPredictorTableImpl, so you could do dependency injection and what not. Also, it's unfortunate that FooPredictor has to be aware that FooPredictorTable is implemented via the DB thread. Really, the use of the DB thread should be a detail internal to the FooPredictorTableImpl, and the interface should take a callback. The real impl would do a PostTaskAndReply() to the DB thread, but the client wouldn't have to do this. And then for a test implementation, you could inject a mock that didn't do the PostTaskAndReply. I recommend these steps for cleaner design, but will not require them since this isn't my code to maintain. I defer to y'all.
On 2012/06/19 00:38:26, willchan wrote: > chrome/browser/net/ and chrome/browser/profiles LGTM > > http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/... > File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): > > http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/... > chrome/browser/predictors/resource_prefetch_predictor_tables.h:66: friend class > PredictorDatabaseInternal; > On 2012/06/18 20:38:33, Shishir wrote: > > On 2012/06/18 20:15:12, willchan wrote: > > > I'm surprised at this use of a friend declaration. Can you explain why this > is > > a > > > clean design? > > > > This is to give the PredictorDatabaseInternal access to constructor and > > initialization functions that the rest of the callers should not call. > > Why the constructor? What harm will happen from someone calling the constructor? > > LogDatabaseStats() is public in PredictorTableBase. What's the point of hiding > it? This is an oversight I will fix. > > I see...the primary consumer is the FooPredictor classes you have. This > friendship pattern you are relying on is creating a tight coupling between > PredictorDatabase and all its database tables. If that's what you want, it's > fine. The friendship is definitely allowing you to establish an interface > boundary between FooPredictor and the FooPredictorTable. > > What would be cleaner, albeit more code, is to get rid of this tight coupling > and replace it with a FooPredictorTable interface and a FooPredictorTableImpl > that did the initialization and what not (composing the PredictorTableBase > rather than using implementation inheritance). But the FooPredictor would only > interact with the FooPredictorTable interface, not the FooPredictorTableImpl, so > you could do dependency injection and what not. Also, it's unfortunate that > FooPredictor has to be aware that FooPredictorTable is implemented via the DB > thread. Really, the use of the DB thread should be a detail internal to the > FooPredictorTableImpl, and the interface should take a callback. The real impl > would do a PostTaskAndReply() to the DB thread, but the client wouldn't have to > do this. And then for a test implementation, you could inject a mock that didn't > do the PostTaskAndReply. > > I recommend these steps for cleaner design, but will not require them since this > isn't my code to maintain. I defer to y'all. The design you propose looks good, along with the composition change. But I would like to not do that in this stage, because I might be deleting this code in a couple of months if we don't think the latency gains are significant enough. Given this experimental nature, lets stick with what we have.
On 2012/06/20 17:42:50, Shishir wrote: > On 2012/06/19 00:38:26, willchan wrote: > > chrome/browser/net/ and chrome/browser/profiles LGTM > > > > > http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/... > > File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): > > > > > http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/... > > chrome/browser/predictors/resource_prefetch_predictor_tables.h:66: friend > class > > PredictorDatabaseInternal; > > On 2012/06/18 20:38:33, Shishir wrote: > > > On 2012/06/18 20:15:12, willchan wrote: > > > > I'm surprised at this use of a friend declaration. Can you explain why > this > > is > > > a > > > > clean design? > > > > > > This is to give the PredictorDatabaseInternal access to constructor and > > > initialization functions that the rest of the callers should not call. > > > > Why the constructor? What harm will happen from someone calling the > constructor? > > > > LogDatabaseStats() is public in PredictorTableBase. What's the point of hiding > > it? > > This is an oversight I will fix. > > > > > I see...the primary consumer is the FooPredictor classes you have. This > > friendship pattern you are relying on is creating a tight coupling between > > PredictorDatabase and all its database tables. If that's what you want, it's > > fine. The friendship is definitely allowing you to establish an interface > > boundary between FooPredictor and the FooPredictorTable. > > > > What would be cleaner, albeit more code, is to get rid of this tight coupling > > and replace it with a FooPredictorTable interface and a FooPredictorTableImpl > > that did the initialization and what not (composing the PredictorTableBase > > rather than using implementation inheritance). But the FooPredictor would only > > interact with the FooPredictorTable interface, not the FooPredictorTableImpl, > so > > you could do dependency injection and what not. Also, it's unfortunate that > > FooPredictor has to be aware that FooPredictorTable is implemented via the DB > > thread. Really, the use of the DB thread should be a detail internal to the > > FooPredictorTableImpl, and the interface should take a callback. The real impl > > would do a PostTaskAndReply() to the DB thread, but the client wouldn't have > to > > do this. And then for a test implementation, you could inject a mock that > didn't > > do the PostTaskAndReply. > > > > I recommend these steps for cleaner design, but will not require them since > this > > isn't my code to maintain. I defer to y'all. > > The design you propose looks good, along with the composition change. But I > would like to not do that in this stage, because I might be deleting this code > in a couple of months if we don't think the latency gains are significant > enough. Given this experimental nature, lets stick with what we have. Can you create a crbug.com with this and assign it to yourself, so you don't forget :)
On Wed, Jun 20, 2012 at 10:42 AM, <shishir@chromium.org> wrote: > On 2012/06/19 00:38:26, willchan wrote: > >> chrome/browser/net/ and chrome/browser/profiles LGTM >> > > > http://codereview.chromium.**org/10416002/diff/39049/** > chrome/browser/predictors/**resource_prefetch_predictor_**tables.h<http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/resource_prefetch_predictor_tables.h> > >> File chrome/browser/predictors/**resource_prefetch_predictor_**tables.h >> (right): >> > > > http://codereview.chromium.**org/10416002/diff/39049/** > chrome/browser/predictors/**resource_prefetch_predictor_** > tables.h#newcode66<http://codereview.chromium.org/10416002/diff/39049/chrome/browser/predictors/resource_prefetch_predictor_tables.h#newcode66> > >> chrome/browser/predictors/**resource_prefetch_predictor_**tables.h:66: >> friend >> > class > >> PredictorDatabaseInternal; >> On 2012/06/18 20:38:33, Shishir wrote: >> > On 2012/06/18 20:15:12, willchan wrote: >> > > I'm surprised at this use of a friend declaration. Can you explain why >> > this > >> is >> > a >> > > clean design? >> > >> > This is to give the PredictorDatabaseInternal access to constructor and >> > initialization functions that the rest of the callers should not call. >> > > Why the constructor? What harm will happen from someone calling the >> > constructor? > > LogDatabaseStats() is public in PredictorTableBase. What's the point of >> hiding >> it? >> > > This is an oversight I will fix. > > > > I see...the primary consumer is the FooPredictor classes you have. This >> friendship pattern you are relying on is creating a tight coupling between >> PredictorDatabase and all its database tables. If that's what you want, >> it's >> fine. The friendship is definitely allowing you to establish an interface >> boundary between FooPredictor and the FooPredictorTable. >> > > What would be cleaner, albeit more code, is to get rid of this tight >> coupling >> and replace it with a FooPredictorTable interface and a >> FooPredictorTableImpl >> that did the initialization and what not (composing the PredictorTableBase >> rather than using implementation inheritance). But the FooPredictor would >> only >> interact with the FooPredictorTable interface, not the >> FooPredictorTableImpl, >> > so > >> you could do dependency injection and what not. Also, it's unfortunate >> that >> FooPredictor has to be aware that FooPredictorTable is implemented via >> the DB >> thread. Really, the use of the DB thread should be a detail internal to >> the >> FooPredictorTableImpl, and the interface should take a callback. The real >> impl >> would do a PostTaskAndReply() to the DB thread, but the client wouldn't >> have >> > to > >> do this. And then for a test implementation, you could inject a mock that >> > didn't > >> do the PostTaskAndReply. >> > > I recommend these steps for cleaner design, but will not require them >> since >> > this > >> isn't my code to maintain. I defer to y'all. >> > > The design you propose looks good, along with the composition change. But I > would like to not do that in this stage, because I might be deleting this > code > in a couple of months if we don't think the latency gains are significant > enough. Given this experimental nature, lets stick with what we have. > OK, SGTM > > > > http://codereview.chromium.**org/10416002/<http://codereview.chromium.org/104... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10416002/80002
Failed to apply patch for content/browser/renderer_host/async_resource_handler.cc: While running patch -p1 --forward --force; patching file content/browser/renderer_host/async_resource_handler.cc Hunk #1 FAILED at 141. Hunk #2 FAILED at 160. 2 out of 2 hunks FAILED -- saving rejects to file content/browser/renderer_host/async_resource_handler.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10416002/88001
Failed to apply patch for content/browser/renderer_host/async_resource_handler.cc: While running patch -p1 --forward --force; patching file content/browser/renderer_host/async_resource_handler.cc Hunk #1 FAILED at 107. Hunk #2 FAILED at 127. 2 out of 2 hunks FAILED -- saving rejects to file content/browser/renderer_host/async_resource_handler.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10416002/94002
Change committed as 144019 |
