Cancel prerender when we discover a download starting from a page we are prer...

chrome/browser/renderer_host/download_throttling_resource_handler.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/renderer_host/download_throttling_resource_handler.h"
 
 #include "base/logging.h"
+#include "chrome/browser/cert_store.h"

[cbentzel, 2011/02/10 14:44:34]

A bunch of these includes are not needed:

cert_store
resource_request_details.h

probably others. 

[dominich, 2011/02/10 18:46:22]

Done.

 #include "chrome/browser/renderer_host/download_resource_handler.h"
+#include "chrome/browser/renderer_host/render_view_host.h"
+#include "chrome/browser/renderer_host/render_view_host_delegate.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
+#include "chrome/browser/renderer_host/resource_dispatcher_host_request_info.h"
+#include "chrome/browser/renderer_host/resource_request_details.h"
+#include "chrome/common/notification_service.h"
 #include "chrome/common/resource_response.h"
 #include "net/base/io_buffer.h"
 #include "net/base/mime_sniffer.h"
 
+namespace {
+
+void NotifyOnUI(NotificationType::Type notification_type,
+                int process_id, int view_id) {
+  RenderViewHost* rvh = RenderViewHost::FromID(process_id, view_id);

[cbentzel, 2011/02/10 14:44:34]

You should probably DCHECK that this is called on the UI thread - although some
of the things you call may be doing that as well.

[cbentzel, 2011/02/10 14:44:34]

Do you want to DCHECK that the notification_type is an expected one?

[dominich, 2011/02/10 18:46:22]

Done.

[dominich, 2011/02/10 18:46:22]

I can, but given this is local to the file and is written to be generic, I don't
think it's necessary. Especially if I use the handler as a Source.

+  if (rvh) {

[cbentzel, 2011/02/10 14:44:34]

You can also do 

if (!rvh)
  return;

to minimize indentation. 

+    RenderViewHostDelegate* rvhd = rvh->delegate();
+    NotificationService::current()->Notify(notification_type,
+        Source<RenderViewHostDelegate>(rvhd),

[cbentzel, 2011/02/10 14:44:34]

This certainly simplifies things for prerender, but feels a little weird that
the rvhd is being used a source rather than renderviewsource, or Profile, or
DownloadManger.

I don't think you need to change for now, but it seems like a mismatch.

[Randy Smith (Not in Mondays), 2011/02/10 16:57:10]

I would agree with this comment, and unless it really makes your life difficult,
I'd like to see it changed.  Temporary solutions aren't, and this would be weird
and wrong to have in the code long-term.

[dominich, 2011/02/10 18:46:22]

Done.

+        NotificationService::NoDetails());
+  }
+}
+
+}

[cbentzel, 2011/02/10 14:44:34]

Nit: add // namespace to make it clear what this block closes. 

+
 DownloadThrottlingResourceHandler::DownloadThrottlingResourceHandler(
     ResourceDispatcherHost* host,
     net::URLRequest* request,
     const GURL& url,
     int render_process_host_id,
     int render_view_id,
     int request_id,
     bool in_complete)
     : host_(host),
       request_(request),
@@ skipping 29 matching lines @@
     return download_handler_->OnRequestRedirected(
         request_id, url, response, defer);
   }
   url_ = url;
   return true;
 }
 
 bool DownloadThrottlingResourceHandler::OnResponseStarted(
     int request_id,
     ResourceResponse* response) {
+  BrowserThread::PostTask(

[cbentzel, 2011/02/10 14:44:34]

I thought this was going to move into DownloadResourceHandler as a more
centralized location? Am I wrong?

[Randy Smith (Not in Mondays), 2011/02/10 16:57:10]

We never get to DownloadResourceHandler, so he can't put it there.

+      BrowserThread::UI, FROM_HERE,
+      NewRunnableFunction(&NotifyOnUI, NotificationType::DOWNLOAD_INITIATED,
+                          render_process_host_id_, render_view_id_));
+
   if (download_handler_.get())
     return download_handler_->OnResponseStarted(request_id, response);
   response_ = response;
   return true;
 }
 
 bool DownloadThrottlingResourceHandler::OnWillStart(int request_id,
                                                     const GURL& url,
                                                     bool* defer) {
   if (download_handler_.get())
@@ skipping 103 matching lines @@
   int buf_size;
   if (download_handler_->OnWillRead(request_id_, &buffer, &buf_size,
                                     tmp_buffer_length_)) {
     CHECK(buf_size >= tmp_buffer_length_);
     memcpy(buffer->data(), tmp_buffer_->data(), tmp_buffer_length_);
     download_handler_->OnReadCompleted(request_id_, &tmp_buffer_length_);
   }
   tmp_buffer_length_ = 0;
   tmp_buffer_ = NULL;
 }