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

chrome/browser/renderer_host/download_throttling_resource_handler.cc

Index: chrome/browser/renderer_host/download_throttling_resource_handler.cc
===================================================================
--- chrome/browser/renderer_host/download_throttling_resource_handler.cc	(revision 74377)
+++ chrome/browser/renderer_host/download_throttling_resource_handler.cc	(working copy)
@@ -5,12 +5,33 @@
 #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,
@@ -60,6 +81,11 @@
 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;