Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6459005:
Cancel prerender when we discover a download starting from a page we are prer... (Closed)
|
Created:
9 years, 10 months ago by dominich Modified:
9 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionCancel prerender when we discover a download starting from a page we are prerendering.
BUG=71214
TEST=Search for carbon poker download and wait. The first result, which has a <link rel="prefetch"> will not cause a prerender as the download that the next page starts will cancel it.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75168
Patch Set 1 #Patch Set 2 : Adding browser test #
Total comments: 13
Patch Set 3 : '' #Patch Set 4 : Now with conflicts resolved #
Total comments: 21
Patch Set 5 : Changed source of notification and now notify on all entry points for download initiation. #
Total comments: 12
Patch Set 6 : Moved notification to ctor and added notification for manual downloads #
Total comments: 5
Patch Set 7 : Moved notification to download_util to be more central #
Total comments: 4
Patch Set 8 : Moved second notification to RDH::BeginDownload #Patch Set 9 : Post resolve #
Total comments: 4
Patch Set 10 : '' #Messages
Total messages: 39 (0 generated)
Randy - the download is triggered via an iframe with a src set to a non-html link. Do you expect this to bring up the download shelf? I'll take a look at the code a bit later. On Tue, Feb 8, 2011 at 6:58 PM, <dominich@chromium.org> wrote: > Reviewers: cbentzel, rdsmith, > > Description: > Cancel prerender when we discover a download starting from a page we are > prerendering. > > Contributed by: dominich@chromium.org > > BUG=71214 > TEST=Search for carbon poker download and wait. The first result, which has > a > <link rel="prefetch"> will not cause a prerender as the download that the > next > page starts will cancel it. > > > > Please review this at http://codereview.chromium.org/6459005/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src/ > > Affected files: > M chrome/browser/prerender/prerender_browsertest.cc > M chrome/browser/prerender/prerender_contents.h > M chrome/browser/prerender/prerender_contents.cc > M chrome/browser/renderer_host/download_throttling_resource_handler.h > M chrome/browser/renderer_host/download_throttling_resource_handler.cc > M chrome/common/notification_type.h > A chrome/test/data/prerender/prerender_download.html > > > Index: chrome/browser/prerender/prerender_browsertest.cc > =================================================================== > --- chrome/browser/prerender/prerender_browsertest.cc (revision 74194) > +++ chrome/browser/prerender/prerender_browsertest.cc (working copy) > @@ -242,3 +242,10 @@ > PrerenderContents::FINAL_STATUS_USED, 2); > NavigateToDestURL(); > } > + > +// Prerenders a page that contains an automatic download. This should not > +// prerender successfully > +IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderDownload) { > + PrerenderTestURL("prerender_download.html", > + PrerenderContents::FINAL_STATUS_DOWNLOAD, 1); > +} > Index: chrome/browser/prerender/prerender_contents.cc > =================================================================== > --- chrome/browser/prerender/prerender_contents.cc (revision 74194) > +++ chrome/browser/prerender/prerender_contents.cc (working copy) > @@ -11,6 +11,7 @@ > #include "chrome/browser/prerender/prerender_manager.h" > #include "chrome/browser/profiles/profile.h" > #include "chrome/browser/renderer_host/render_view_host.h" > +#include "chrome/browser/renderer_host/resource_request_details.h" > #include "chrome/browser/renderer_host/site_instance.h" > #include "chrome/browser/renderer_preferences_util.h" > #include "chrome/browser/ui/login/login_prompt.h" > @@ -82,6 +83,10 @@ > registrar_.Add(this, NotificationType::AUTH_CANCELLED, > NotificationService::AllSources()); > > + // Register all responses to see if we should cancel. > + registrar_.Add(this, NotificationType::DOWNLOAD_STARTED, > + NotificationService::AllSources()); > + > DCHECK(load_start_time_.is_null()); > load_start_time_ = base::TimeTicks::Now(); > > @@ -206,6 +211,18 @@ > break; > } > > + case NotificationType::DOWNLOAD_STARTED: { > + // If the download is started from a RenderViewHost that we are > + // delegating, kill the prerender. This will also stop the download > and > + // navigating to the page will restart it as expected. > + RenderViewHostDelegate* rvhd = > + Source<RenderViewHostDelegate>(source).ptr(); > + if (rvhd == this) { > + Destroy(FINAL_STATUS_DOWNLOAD); > + } > + break; > + } > + > default: > NOTREACHED() << "Unexpected notification sent."; > break; > Index: chrome/browser/prerender/prerender_contents.h > =================================================================== > --- chrome/browser/prerender/prerender_contents.h (revision 74194) > +++ chrome/browser/prerender/prerender_contents.h (working copy) > @@ -50,6 +50,7 @@ > FINAL_STATUS_APP_TERMINATING, > FINAL_STATUS_JAVASCRIPT_ALERT, > FINAL_STATUS_AUTH_NEEDED, > + FINAL_STATUS_DOWNLOAD, > FINAL_STATUS_MAX, > }; > > Index: chrome/browser/renderer_host/download_throttling_resource_handler.cc > =================================================================== > --- chrome/browser/renderer_host/download_throttling_resource_handler.cc > (revision 74194) > +++ chrome/browser/renderer_host/download_throttling_resource_handler.cc > (working copy) > @@ -5,8 +5,14 @@ > #include > "chrome/browser/renderer_host/download_throttling_resource_handler.h" > > #include "base/logging.h" > +#include "chrome/browser/cert_store.h" > #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" > @@ -36,6 +42,34 @@ > DownloadThrottlingResourceHandler::~DownloadThrottlingResourceHandler() { > } > > +ResourceRequestDetails* > + DownloadThrottlingResourceHandler::GetResourceRequestDetails() { > + int certID = 0; > + if (request_->ssl_info().cert) { > + ResourceDispatcherHostRequestInfo* info = > + ResourceDispatcherHost::InfoForRequest(request_); > + > + certID = > CertStore::GetInstance()->StoreCert(request_->ssl_info().cert, > + info->child_id()); > + } > + > + return new ResourceRequestDetails(request_, certID); > +} > + > +template<NotificationType::Type T> > +void DownloadThrottlingResourceHandler::NotifyOnUI( > + int process_id, int view_id, > + ResourceRequestDetails* details) { > + RenderViewHost* rvh = RenderViewHost::FromID(process_id, view_id); > + if (rvh) { > + RenderViewHostDelegate* rvhd = rvh->delegate(); > + NotificationService::current()->Notify(T, > + Source<RenderViewHostDelegate>(rvhd), > + Details<ResourceRequestDetails>(details)); > + } > + delete details; > +} > + > bool DownloadThrottlingResourceHandler::OnUploadProgress(int request_id, > uint64 position, > uint64 size) { > @@ -60,6 +94,14 @@ > bool DownloadThrottlingResourceHandler::OnResponseStarted( > int request_id, > ResourceResponse* response) { > + BrowserThread::PostTask( > + BrowserThread::UI, FROM_HERE, > + NewRunnableFunction( > + &DownloadThrottlingResourceHandler::NotifyOnUI > + <NotificationType::DOWNLOAD_STARTED>, > + render_process_host_id_, render_view_id_, > + GetResourceRequestDetails())); > + > if (download_handler_.get()) > return download_handler_->OnResponseStarted(request_id, response); > response_ = response; > @@ -141,6 +183,14 @@ > } > > void DownloadThrottlingResourceHandler::CancelDownload() { > + BrowserThread::PostTask( > + BrowserThread::UI, FROM_HERE, > + NewRunnableFunction( > + &DownloadThrottlingResourceHandler::NotifyOnUI > + <NotificationType::DOWNLOAD_CANCELLED>, > + render_process_host_id_, render_view_id_, > + GetResourceRequestDetails())); > + > host_->CancelRequest(render_process_host_id_, request_id_, false); > } > > Index: chrome/browser/renderer_host/download_throttling_resource_handler.h > =================================================================== > --- chrome/browser/renderer_host/download_throttling_resource_handler.h > (revision 74194) > +++ chrome/browser/renderer_host/download_throttling_resource_handler.h > (working copy) > @@ -10,10 +10,12 @@ > > #include "chrome/browser/download/download_request_limiter.h" > #include "chrome/browser/renderer_host/resource_handler.h" > +#include "chrome/common/notification_type.h" > #include "googleurl/src/gurl.h" > > class DownloadResourceHandler; > class ResourceDispatcherHost; > +class ResourceRequestDetails; > > namespace net { > class URLRequest; > @@ -65,6 +67,12 @@ > > void CopyTmpBufferToDownloadHandler(); > > + ResourceRequestDetails* GetResourceRequestDetails(); > + > + template<NotificationType::Type T> > + static void NotifyOnUI(int process_id, int view_id, > + ResourceRequestDetails* details); > + > ResourceDispatcherHost* host_; > net::URLRequest* request_; > GURL url_; > Index: chrome/common/notification_type.h > =================================================================== > --- chrome/common/notification_type.h (revision 74194) > +++ chrome/common/notification_type.h (working copy) > @@ -1229,7 +1229,16 @@ > // The source is the corresponding RenderViewHost. There are not > details. > AUTOFILL_DID_FILL_FORM_DATA, > > + // Download Notifications > -------------------------------------------------- > > + // Sent when a download starts. The source is the > RenderViewHostDelegate. > + // The details are a ResourceRequestDetails. > + DOWNLOAD_STARTED, > + > + // Sent when a download is canceled. The source is the > + // RenderViewHostDelegate. The details are a ResourceRequestDetails. > + DOWNLOAD_CANCELLED, > + > // Misc > -------------------------------------------------------------------- > > #if defined(OS_CHROMEOS) > Index: chrome/test/data/prerender/prerender_download.html > =================================================================== > --- chrome/test/data/prerender/prerender_download.html (revision 0) > +++ chrome/test/data/prerender/prerender_download.html (revision 0) > @@ -0,0 +1,7 @@ > +<html> > + <head><title>Prerender download test</title></head> > + <body> > + <iframe name="download" src="../download-test1.lib" width="0" > height="0" > + frameborder="0"></iframe> > + </body> > +</html> > > Property changes on: chrome/test/data/prerender/prerender_download.html > ___________________________________________________________________ > Added: svn:eol-style > + LF > > > >
The big question for me is whether this is the earliest possible place to trigger the notification - Randy would be a better judge. http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:221: Destroy(FINAL_STATUS_DOWNLOAD); Nit: I'd like an explicit return here, since Destroy is calling delete this. It's OK for now, but if any logic went below the switch statement it might be problematic. http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.cc:52: certID = CertStore::GetInstance()->StoreCert(request_->ssl_info().cert, I have no idea what's going on here, but it seems like storing SSL certs for a download notification is probably not correct. Also, none of the consumers currently use any of the details - just the source. Could you use a generic details? http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.cc:190: <NotificationType::DOWNLOAD_CANCELLED>, There are no consumers of DOWNLOAD_CANCELLED right now - should you skip adding it in this CL? http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... File chrome/browser/renderer_host/download_throttling_resource_handler.h (right): http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.h:72: template<NotificationType::Type T> Move this into .cc file and put in anonymous namespace. It's only being used by internal methods. It also doesn't look like there are substantial gains templating it on NotificationType::Type - why not pass it in as an argument?
http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:87: registrar_.Add(this, NotificationType::DOWNLOAD_STARTED, Instead of registering with AllSources, you could register with Source<RenderViewHostDelegate>(this). That way the filtering is done in the NotificationService rather than here.
So, I'm not convinced DownloadThrottlingResourceHandler is the right place to put the notification. There's other entry points to the download system. For example, an IPC of ViewHostMsg_DownloadUrl will cause a DownloadResourceHandler to be created, but not a throttling resource handler. I'm guessing that the Notification should be done in the DownloadManager or some other location than a resource handler.
[Not yet my full review, just tossing in a couple of comments. ] On 2011/02/09 13:08:50, cbentzel wrote: > So, I'm not convinced DownloadThrottlingResourceHandler is the right place to > put the notification. > > There's other entry points to the download system. For example, an IPC of > ViewHostMsg_DownloadUrl will cause a DownloadResourceHandler to be created, but > not a throttling resource handler. > > I'm guessing that the Notification should be done in the DownloadManager or some > other location than a resource handler. I'd suggest DownloadResourceHandler. I believe that everything that ends up in DownloadManager goes through DownloadResourceHandle, and I've got a background CL that's really going to mess with the areas in which you'd want to put the notification in the DownloadManager. If you really want to put it in DownloadManager, let me know and I'll give you guidance as to where. Also, a question I don't know the answer to: Is there any way for a pre-renderer to invoke a SavePackage operation? SavePackage is what's used for "Save Page As ..."; it's separate from the download code for (bad) historical reasons, and I don't know the code very well. My guess is that there isn't such a way, but it might be worth scanning.
I'm going to wait for my primary review on the resolution to Chris' points (it looks like that may change the download code significantly) but one high-level question: When you kill a pre-render because of downloads, you come back and cancel the download, right? Where is the code that cancels the download?
On 2011/02/09 10:47:08, cbentzel wrote: > Randy - the download is triggered via an iframe with a src set to a non-html > link. Do you expect this to bring up the download shelf? I would, but my knowledge of those aspects of the UI is not yet certain, so if you want to be sure you should test it in a non-prerenderer context.
On 2011/02/09 12:11:13, cbentzel wrote: > The big question for me is whether this is the earliest possible place to > trigger the notification - Randy would be a better judge. For what it's worth, my answer to that question is "yes". The DownloadThrottlingResourceHandler may cancel the download, so you can't be sure that the download is actually going to happen until just about the point in the code where the notification was put. That doesn't mean it's the best place; for the reasons you called out, there may be downloads that don't go through this section of code. So I'd be inclined to put it in the DownloadResourceHandler; that doesn't lose you very much time at all.
On Wed, Feb 9, 2011 at 9:01 AM, <rdsmith@chromium.org> wrote: > On 2011/02/09 10:47:08, cbentzel wrote: > >> Randy - the download is triggered via an iframe with a src set to a >> non-html >> link. Do you expect this to bring up the download shelf? >> > > I would, but my knowledge of those aspects of the UI is not yet certain, so > if > you want to be sure you should test it in a non-prerenderer context. This does work in non-prerender context [Dominic pointed to a site which did it, and I reproed]. My question was whether this _should_ work.
On 2011/02/09 13:08:50, cbentzel wrote: > So, I'm not convinced DownloadThrottlingResourceHandler is the right place to > put the notification. > > There's other entry points to the download system. For example, an IPC of > ViewHostMsg_DownloadUrl will cause a DownloadResourceHandler to be created, but > not a throttling resource handler. > > I'm guessing that the Notification should be done in the DownloadManager or some > other location than a resource handler. My experience is that in the prerender case it never gets to the DownloadManager. This is the only place I could find to catch it. I'm more than happy to find somewhere else though.
On 2011/02/09 13:52:17, rdsmith wrote: > I'm going to wait for my primary review on the resolution to Chris' points (it > looks like that may change the download code significantly) but one high-level > question: When you kill a pre-render because of downloads, you come back and > cancel the download, right? Where is the code that cancels the download? I don't explicitly cancel the download but it doesn't show in the history and no files are created. As we destroy the background tab being used for the prerender I expected the download to be cancelled. If this isn't true, I'll work in a cancellation after changing the code as per Chris's comments.
On 2011/02/09 14:03:37, rdsmith wrote: > On 2011/02/09 12:11:13, cbentzel wrote: > > The big question for me is whether this is the earliest possible place to > > trigger the notification - Randy would be a better judge. > > For what it's worth, my answer to that question is "yes". The > DownloadThrottlingResourceHandler may cancel the download, so you can't be sure > that the download is actually going to happen until just about the point in the > code where the notification was put. That doesn't mean it's the best place; for > the reasons you called out, there may be downloads that don't go through this > section of code. So I'd be inclined to put it in the DownloadResourceHandler; > that doesn't lose you very much time at all. As I mentioned above, I didn't find that the download made it to the DownloadResourceHandler in the prerender case. It seems that it is paused and never resumed. I'll take another look as I may have missed something, but I'm fairly confident this is the right, if not the best, place to put it.
On 2011/02/09 14:54:39, cbentzel wrote: > On Wed, Feb 9, 2011 at 9:01 AM, <mailto:rdsmith@chromium.org> wrote: > > > On 2011/02/09 10:47:08, cbentzel wrote: > > > >> Randy - the download is triggered via an iframe with a src set to a > >> non-html > >> link. Do you expect this to bring up the download shelf? > >> > > > > I would, but my knowledge of those aspects of the UI is not yet certain, so > > if > > you want to be sure you should test it in a non-prerenderer context. > > > This does work in non-prerender context [Dominic pointed to a site which did > it, and I reproed]. > > My question was whether this _should_ work. From a quick search, it seems this is given as the best technique to start downloads automatically from a page. If there are other ways of building the page to start a download that might go through a different code path, let me know and I'll test them.
On 2011/02/09 14:54:39, cbentzel wrote: > On Wed, Feb 9, 2011 at 9:01 AM, <mailto:rdsmith@chromium.org> wrote: > > > On 2011/02/09 10:47:08, cbentzel wrote: > > > >> Randy - the download is triggered via an iframe with a src set to a > >> non-html > >> link. Do you expect this to bring up the download shelf? > >> > > > > I would, but my knowledge of those aspects of the UI is not yet certain, so > > if > > you want to be sure you should test it in a non-prerenderer context. > > > This does work in non-prerender context [Dominic pointed to a site which did > it, and I reproed]. > > My question was whether this _should_ work. Ah, sorry. Yes, it should.
On 2011/02/09 16:25:57, dominic wrote: > On 2011/02/09 13:08:50, cbentzel wrote: > > So, I'm not convinced DownloadThrottlingResourceHandler is the right place to > > put the notification. > > > > There's other entry points to the download system. For example, an IPC of > > ViewHostMsg_DownloadUrl will cause a DownloadResourceHandler to be created, > but > > not a throttling resource handler. > > > > I'm guessing that the Notification should be done in the DownloadManager or > some > > other location than a resource handler. > > My experience is that in the prerender case it never gets to the > DownloadManager. This is the only place I could find to catch it. I'm more than > happy to find somewhere else though. That's ... interesting. Where does it block; can you tell? (or if you give me a repro, I can find it). It *should* get to the download manager ....
On 2011/02/09 16:27:47, dominic wrote: > On 2011/02/09 13:52:17, rdsmith wrote: > > I'm going to wait for my primary review on the resolution to Chris' points (it > > looks like that may change the download code significantly) but one high-level > > question: When you kill a pre-render because of downloads, you come back and > > cancel the download, right? Where is the code that cancels the download? > > I don't explicitly cancel the download but it doesn't show in the history and no > files are created. As we destroy the background tab being used for the prerender > I expected the download to be cancelled. If this isn't true, I'll work in a > cancellation after changing the code as per Chris's comments. I don't believe it's true--certainly I can start a download from a tab and kill the tab without killing the download. The downloads are separate from tabs (and I think even separate from the browser windows, even though the download shelf is per-browser window). This may be related to the fact that you didn't see a download get to the DownloadManager; if it doesn't get to the DownloadManager, it ain't showing up in the file system or history.
> As I mentioned above, I didn't find that the download made it to the > DownloadResourceHandler in the prerender case. It seems that it is paused and > never resumed. If it never gets to the DownloadResourceHandler, I believe that means that the DownloadRequestLimitter is blocking it. I'd be interested in whether you get a DownloadThrottlingResourceHandler::CancelDownload() or if you just never get ContinueDownload() called. But either way, that means that no download will occur. Which may mean you don't need to notify at all. Would be nice to know what's going on in the DownloadRequestLimiter in response to this invocation. (The relevant code chain starts from the DownloadThrottlingResourceHandler constructor; it calls into the DownloadRequestLimiter, and is expecting a callback of some sort; either cancel or continue--see download_request_limiter.cc). > > I'll take another look as I may have missed something, but I'm fairly confident > this is the right, if not the best, place to put it.
On Wed, Feb 9, 2011 at 8:53 AM, <rdsmith@chromium.org> wrote: > As I mentioned above, I didn't find that the download made it to the >> DownloadResourceHandler in the prerender case. It seems that it is paused >> and >> never resumed. >> > > If it never gets to the DownloadResourceHandler, I believe that means that > the > DownloadRequestLimitter is blocking it. I'd be interested in whether you > get a > DownloadThrottlingResourceHandler::CancelDownload() or if you just never > get > ContinueDownload() called. But either way, that means that no download > will > occur. Which may mean you don't need to notify at all. Would be nice to > know > what's going on in the DownloadRequestLimiter in response to this > invocation. > (The relevant code chain starts from the DownloadThrottlingResourceHandler > constructor; it calls into the DownloadRequestLimiter, and is expecting a > callback of some sort; either cancel or continue--see > download_request_limiter.cc). > > From memory, it never gets the continue but I need to double check. I still need to notify as I need the prerender to be cancelled. If I don't do this, while there are no obvious negative effects, on navigation to the page the automatic download doesn't start as it should (as it is paused). > > > > I'll take another look as I may have missed something, but I'm fairly >> > confident > >> this is the right, if not the best, place to put it. >> > > > > http://codereview.chromium.org/6459005/ > -- Dominic Hamon Software Engineer | Google
On 2011/02/09 16:53:53, rdsmith wrote: > > As I mentioned above, I didn't find that the download made it to the > > DownloadResourceHandler in the prerender case. It seems that it is paused and > > never resumed. > > If it never gets to the DownloadResourceHandler, I believe that means that the > DownloadRequestLimitter is blocking it. I'd be interested in whether you get a > DownloadThrottlingResourceHandler::CancelDownload() or if you just never get > ContinueDownload() called. But either way, that means that no download will > occur. Which may mean you don't need to notify at all. Would be nice to know > what's going on in the DownloadRequestLimiter in response to this invocation. > (The relevant code chain starts from the DownloadThrottlingResourceHandler > constructor; it calls into the DownloadRequestLimiter, and is expecting a > callback of some sort; either cancel or continue--see > download_request_limiter.cc). The DownloadRequestLimiter is cancelling the download immediately as the prerendered render view has no TabContents. > > > > > > I'll take another look as I may have missed something, but I'm fairly > confident > > this is the right, if not the best, place to put it.
On 2011/02/09 17:58:16, dominic wrote: > The DownloadRequestLimiter is cancelling the download immediately as the > prerendered render view has no TabContents. From my perspective this is awesome behavior; let's keep it! :-} :-| Thanks for checking this out. Chris is right, though, in that there are other paths into this code that go straight to the DownloadResourceHandler, and hence trigger the download. So you'll probably need to instrument multiple paths. I'm also a bit uncomfortable about doing a general "DownloadStarted" notification on a path on which the download actually hasn't been started (from my perspective, if we never get to the DownloadResourceHandler, the download hasn't been started). Maybe call it "DOWNLOAD_INITIATED" with a comment at notification type definition that this doesn't actually mean that the download's going to be started? A month or two ago I did a reverse call graph of all the paths into DownloadResourceHandler; I'll send that under separate cover (I don't trust the formatting to stand up to cut&paste in a web form).
I was hoping that there was a single entry point for kicking off a download where the notification would happen, partly to help future-proof this in case there are other triggers for downloads over what currently exists. I agree that the code Dominic wrote handles the most likely case, however.
Added more browser test variations for downloading of files based on existing download browser tests. http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:87: registrar_.Add(this, NotificationType::DOWNLOAD_STARTED, On 2011/02/09 12:19:15, cbentzel wrote: > Instead of registering with AllSources, you could register with > Source<RenderViewHostDelegate>(this). That way the filtering is done in the > NotificationService rather than here. Done. http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:221: Destroy(FINAL_STATUS_DOWNLOAD); On 2011/02/09 12:11:14, cbentzel wrote: > Nit: I'd like an explicit return here, since Destroy is calling delete this. > > It's OK for now, but if any logic went below the switch statement it might be > problematic. I'll add the same to the AUTH_* cases above. http://codereview.chromium.org/6459005/diff/7/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:221: Destroy(FINAL_STATUS_DOWNLOAD); On 2011/02/09 12:11:14, cbentzel wrote: > Nit: I'd like an explicit return here, since Destroy is calling delete this. > > It's OK for now, but if any logic went below the switch statement it might be > problematic. Done. http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.cc:52: certID = CertStore::GetInstance()->StoreCert(request_->ssl_info().cert, On 2011/02/09 12:11:14, cbentzel wrote: > I have no idea what's going on here, but it seems like storing SSL certs for a > download notification is probably not correct. > > Also, none of the consumers currently use any of the details - just the source. > Could you use a generic details? Done. http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.cc:190: <NotificationType::DOWNLOAD_CANCELLED>, On 2011/02/09 12:11:14, cbentzel wrote: > There are no consumers of DOWNLOAD_CANCELLED right now - should you skip adding > it in this CL? I can. I thought that it would be good to have both in for completeness, but I'll strip this out. http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.cc:190: <NotificationType::DOWNLOAD_CANCELLED>, On 2011/02/09 12:11:14, cbentzel wrote: > There are no consumers of DOWNLOAD_CANCELLED right now - should you skip adding > it in this CL? Done. http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... File chrome/browser/renderer_host/download_throttling_resource_handler.h (right): http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.h:72: template<NotificationType::Type T> On 2011/02/09 12:11:14, cbentzel wrote: > Move this into .cc file and put in anonymous namespace. It's only being used by > internal methods. > > It also doesn't look like there are substantial gains templating it on > NotificationType::Type - why not pass it in as an argument? I was following the pattern established in the ResourceDispatcherHost to be consistent, but yes I'd rather this was a parameter too. http://codereview.chromium.org/6459005/diff/7/chrome/browser/renderer_host/do... chrome/browser/renderer_host/download_throttling_resource_handler.h:72: template<NotificationType::Type T> On 2011/02/09 12:11:14, cbentzel wrote: > Move this into .cc file and put in anonymous namespace. It's only being used by > internal methods. > > It also doesn't look like there are substantial gains templating it on > NotificationType::Type - why not pass it in as an argument? Done.
http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:8: #include "chrome/browser/cert_store.h" A bunch of these includes are not needed: cert_store resource_request_details.h probably others. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:24: RenderViewHost* rvh = RenderViewHost::FromID(process_id, view_id); 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. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:24: RenderViewHost* rvh = RenderViewHost::FromID(process_id, view_id); Do you want to DCHECK that the notification_type is an expected one? http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:25: if (rvh) { You can also do if (!rvh) return; to minimize indentation. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:28: Source<RenderViewHostDelegate>(rvhd), 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. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:33: } Nit: add // namespace to make it clear what this block closes. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:84: BrowserThread::PostTask( I thought this was going to move into DownloadResourceHandler as a more centralized location? Am I wrong? http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.h (right): http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.h:13: #include "chrome/common/notification_type.h" Include not needed. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.h:18: class ResourceRequestDetails; This isn't needed now that you are passing NoDetails. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.h:70: ResourceRequestDetails* GetResourceRequestDetails(); Also not needed since passing NoDetails.
I apologize for requesting the extra work, but I'd like to see this put on all of the paths that may initiate a download. Calling it "DOWNLOAD_INITIATED" will tempt people to hook it, and then they'll be surprised when a download does an end run around the notification. (This is also why I don't want the RenderViewHostDelegate as the source.) It's not ideal that we're by hand instrumenting several different locations, but I don't see a better way to go given that we want to hook initiation and can't count on making it to DownloadResourceHandler. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/11003/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:220: // delegating, kill the prerender. This will also stop the download and Can you give me the context for how killing the prerender stops the download? I thought that the download never got started because the limiter blocked it. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:28: Source<RenderViewHostDelegate>(rvhd), On 2011/02/10 14:44:34, cbentzel wrote: > 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. 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. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:84: BrowserThread::PostTask( On 2011/02/10 14:44:34, cbentzel wrote: > I thought this was going to move into DownloadResourceHandler as a more > centralized location? Am I wrong? We never get to DownloadResourceHandler, so he can't put it there.
http://codereview.chromium.org/6459005/diff/11003/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/11003/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:220: // delegating, kill the prerender. This will also stop the download and On 2011/02/10 16:57:10, rdsmith wrote: > Can you give me the context for how killing the prerender stops the download? I > thought that the download never got started because the limiter blocked it. You're right - changed the comment to reflect that. When the prerender is cancelled, the RenderViewHost is shutdown and deleted and unless I've missed something this cancels all requests made from that RenderViewHost. Done. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:8: #include "chrome/browser/cert_store.h" On 2011/02/10 14:44:34, cbentzel wrote: > A bunch of these includes are not needed: > > cert_store > resource_request_details.h > > probably others. Done. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:24: RenderViewHost* rvh = RenderViewHost::FromID(process_id, view_id); On 2011/02/10 14:44:34, cbentzel wrote: > 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. Done. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:24: RenderViewHost* rvh = RenderViewHost::FromID(process_id, view_id); On 2011/02/10 14:44:34, cbentzel wrote: > Do you want to DCHECK that the notification_type is an expected one? 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. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:28: Source<RenderViewHostDelegate>(rvhd), On 2011/02/10 16:57:10, rdsmith wrote: > On 2011/02/10 14:44:34, cbentzel wrote: > > 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. > > 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. Done. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.h (right): http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.h:13: #include "chrome/common/notification_type.h" On 2011/02/10 14:44:34, cbentzel wrote: > Include not needed. Done. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.h:18: class ResourceRequestDetails; On 2011/02/10 14:44:34, cbentzel wrote: > This isn't needed now that you are passing NoDetails. Done. http://codereview.chromium.org/6459005/diff/11003/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.h:70: ResourceRequestDetails* GetResourceRequestDetails(); On 2011/02/10 14:44:34, cbentzel wrote: > Also not needed since passing NoDetails. Done.
Prerender stuff looks fine, but the downloads change still looks wrong to me. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/14001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:224: if (rvh->delegate() == this) { May not hurt to be defensive and check that rvh is non-NULL. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:227: return; move return next to Destroy and make this break, to be similar to Observe. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:145: BrowserThread::UI, FROM_HERE, I really don't think this is what you want - it will send out a notification each time some bytes are read from the stream.
Annotated some changes but i'm not uploading a new patch set yet. I went through the entry points from RDH into the Downloads and made sure I'm adding the notification to those places that are called when a download starts. I'd like some feedback before I upload again. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/14001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:224: if (rvh->delegate() == this) { On 2011/02/10 19:32:18, cbentzel wrote: > May not hurt to be defensive and check that rvh is non-NULL. Done. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:227: return; On 2011/02/10 19:32:18, cbentzel wrote: > move return next to Destroy and make this break, to be similar to Observe. Done. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:108: NewRunnableFunction(&NotifyOnUI, NotificationType::DOWNLOAD_INITIATED, Moving this to OnWillStart for the same reasons as outlined below. This is called once but only once we've started. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:145: BrowserThread::UI, FROM_HERE, On 2011/02/10 19:32:18, cbentzel wrote: > I really don't think this is what you want - it will send out a notification > each time some bytes are read from the stream. You're right - I was going from rdsmith's diagram of entry points but it looks like this one can't be hit without one of the other entry points being hit first. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:145: BrowserThread::UI, FROM_HERE, On 2011/02/10 19:32:18, cbentzel wrote: > I really don't think this is what you want - it will send out a notification > each time some bytes are read from the stream. Done. http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:159: NewRunnableFunction(&NotifyOnUI, NotificationType::DOWNLOAD_INITIATED, This suffers from the same issue as above. Even though it is an entry point from RDH, it is only called when a download request cannot be initiated or has completed. Neither match the concept of an initiated download so I'm removing this too.
I just read through and reviewed the download code. I would like to see the other entry points for downloads be instrumented as well. That isn't necessary for the pre-render work, but if you put a notification in the system that claims to be triggered when a download is initiated, it should be triggered whenever a download is initiated, yes? From the reverse callgraph I posted, I think that just means you need to instrument ResourceDispatcherHost::BeginDownload. Any objection to that? http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:86: render_process_host_id_, render_view_id_)); Why do separate notifications for request redirected and response started? It's a single download. My inclination would be to put the notification in the constructor; any reason not to do that? http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:145: BrowserThread::UI, FROM_HERE, On 2011/02/10 20:02:17, dominic wrote: > You're right - I was going from rdsmith's diagram of entry points but it looks > like this one can't be hit without one of the other entry points being hit > first. Sorry; that diagram shows control flow within the download system. For all the reasons we've talked about, that's probably not relevant to you; the limiter keeps the download from actually starting in the common case. I think you want the notification just from the constructor.
On Fri, Feb 11, 2011 at 10:44 AM, <rdsmith@chromium.org> wrote: > I just read through and reviewed the download code. > > I would like to see the other entry points for downloads be instrumented as > well. That isn't necessary for the pre-render work, but if you put a > notification in the system that claims to be triggered when a download is > initiated, it should be triggered whenever a download is initiated, yes? > From > the reverse callgraph I posted, I think that just means you need to > instrument > ResourceDispatcherHost::BeginDownload. Any objection to that? > > I've checked out the call-graph for the automatic and manual download cases, and this is what I found: From the context menu: DownloadManager::DownloadUrl . download_util::DownloadUrl . . ResourceDispatcherHost::BeginDownload . . . DownloadResourceHandler::DownloadResourceHandler Automatic downloads: ResourceDispatcherHost::CompleteRead . SafeBrowsingResourceHandler::OnReadCompleted . . BufferedResourceHandler::OnReadCompleted . . . BufferedResourceHandler::CompleteResponseStarted . . . . DownloadThrottlingResourceHandler:: DownloadThrottlingResourceHandler So I agree that notifying in both the Download*ResourceHandler constructors is the way to go. I'll clean everything up and upload for review.
http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/14001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:86: render_process_host_id_, render_view_id_)); On 2011/02/11 18:44:15, rdsmith wrote: > Why do separate notifications for request redirected and response started? It's > a single download. > > My inclination would be to put the notification in the constructor; any reason > not to do that? Done.
http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_resource_handler.cc:68: render_process_host_id, render_view_id_)); I'd rather not notify from the DownloadResourceHandler constructor, as that'll mean in the case where a download comes off of the usual path (i.e. through the DownloadThrottlingResourceHandler) you'll get two notifications that the same download is starting. I don't like the fragility of putting the notification in ResourceDispatcherHost::BeginDownload, but it has the advantage of covering all the (current :-{) pathways through which a download can be initiated while still giving you the notification you're looking for when the initiation doesn't actually go through to creating a DownloadResourceHandler. http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:28: } // namespace Is dislike duplicating this code; is there anywhere more appropriate to put it? I'll admit I'm not coming up with anything :-{. Maybe make it specific to DOWNLOAD_INITIATED and make it a static function on DownloadResourceHandler? (I'd also accept leaving it here--I'm not coming up with great options on this one.)
http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_resource_handler.cc:68: render_process_host_id, render_view_id_)); On 2011/02/13 03:08:36, rdsmith wrote: > I'd rather not notify from the DownloadResourceHandler constructor, as that'll > mean in the case where a download comes off of the usual path (i.e. through the > DownloadThrottlingResourceHandler) you'll get two notifications that the same > download is starting. I don't like the fragility of putting the notification in > ResourceDispatcherHost::BeginDownload, but it has the advantage of covering all > the (current :-{) pathways through which a download can be initiated while still > giving you the notification you're looking for when the initiation doesn't > actually go through to creating a DownloadResourceHandler. I was considering download_util::DownloadUrl as an alternative to RDH::BeginDownload as it may be a little more future-proof. What do you think? http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_throttling_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_throttling_resource_handler.cc:28: } // namespace On 2011/02/13 03:08:36, rdsmith wrote: > Is dislike duplicating this code; is there anywhere more appropriate to put it? > I'll admit I'm not coming up with anything :-{. Maybe make it specific to > DOWNLOAD_INITIATED and make it a static function on DownloadResourceHandler? > > (I'd also accept leaving it here--I'm not coming up with great options on this > one.) I do want to keep it generic as I can see this being extended to include download cancelation notifications in the future, but I agree it isn't great. Maybe download_util makes sense for this too.
I think we're almost there. Just a few comments. http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/download_resource_handler.cc (right): http://codereview.chromium.org/6459005/diff/22001/chrome/browser/renderer_hos... chrome/browser/renderer_host/download_resource_handler.cc:68: render_process_host_id, render_view_id_)); On 2011/02/14 16:07:12, dominic wrote: > On 2011/02/13 03:08:36, rdsmith wrote: > > I'd rather not notify from the DownloadResourceHandler constructor, as that'll > > mean in the case where a download comes off of the usual path (i.e. through > the > > DownloadThrottlingResourceHandler) you'll get two notifications that the same > > download is starting. I don't like the fragility of putting the notification > in > > ResourceDispatcherHost::BeginDownload, but it has the advantage of covering > all > > the (current :-{) pathways through which a download can be initiated while > still > > giving you the notification you're looking for when the initiation doesn't > > actually go through to creating a DownloadResourceHandler. > > I was considering download_util::DownloadUrl as an alternative to > RDH::BeginDownload as it may be a little more future-proof. What do you think? Neither one's particularly future-proof, I'm afraid :-{. I have (vague) plans to get rid of download_util based on a recent rant of Brett's on chromium-dev, and no current plans to get rid of RDH::BeginDownload, but I don't consider either of them part of a particularly clean architecture. It's sort of like the breakfast cereal adds: "Standing next to ^W ^W Part of this complete architecture" :-} :-J. Having said that, I'd vote in favor of RDH just because there's a path that initiales a download that doesn't get notified if we use download_util: ResourceMessageFilter::OnDownloadUrl -> ResourceDispatcherHost::BeginDownload-> DownloadResourceHandler(). That happens when the renderer processes sends us a message requesting a download; I'm still not sure under what circumstances that happens, but if the path's in the code, I'd like to catch it. http://codereview.chromium.org/6459005/diff/28001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6459005/diff/28001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:748: render_process_host_id, render_view_id)); See notes elsewhere; this either needs to be moved to ResourceDispatcherHost or we need another notification point in ResourceMessageFilter::OnDownloadUrl, and I'd vote for choice #1. http://codereview.chromium.org/6459005/diff/28001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:767: NotificationService::NoDetails()); I'm happy with this location and structure.
http://codereview.chromium.org/6459005/diff/28001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6459005/diff/28001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:748: render_process_host_id, render_view_id)); On 2011/02/14 19:20:51, rdsmith wrote: > See notes elsewhere; this either needs to be moved to ResourceDispatcherHost or > we need another notification point in ResourceMessageFilter::OnDownloadUrl, and > I'd vote for choice #1. Done. http://codereview.chromium.org/6459005/diff/28001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:767: NotificationService::NoDetails()); On 2011/02/14 19:20:51, rdsmith wrote: > I'm happy with this location and structure. Done.
LGTM, with two nits. Please make sure Randy is OK with the download+RDH change. http://codereview.chromium.org/6459005/diff/43005/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/43005/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:222: DCHECK(details == NotificationService::NoDetails()); Nit: DCHECK_EQ http://codereview.chromium.org/6459005/diff/43005/chrome/common/notification_... File chrome/common/notification_type.h (right): http://codereview.chromium.org/6459005/diff/43005/chrome/common/notification_... chrome/common/notification_type.h:1227: // prematurely. The source is the RenderViewHostDelegate. There are no Comment is wrong - the source is RenderViewHost. You may also want to mention that it will not be none.
Fixed the nits. Will wait to upload in case rdsmith has any more comments. http://codereview.chromium.org/6459005/diff/43005/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6459005/diff/43005/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:222: DCHECK(details == NotificationService::NoDetails()); On 2011/02/15 20:51:23, cbentzel wrote: > Nit: DCHECK_EQ Done. http://codereview.chromium.org/6459005/diff/43005/chrome/common/notification_... File chrome/common/notification_type.h (right): http://codereview.chromium.org/6459005/diff/43005/chrome/common/notification_... chrome/common/notification_type.h:1227: // prematurely. The source is the RenderViewHostDelegate. There are no On 2011/02/15 20:51:23, cbentzel wrote: > Comment is wrong - the source is RenderViewHost. You may also want to mention > that it will not be none. Done.
LGTM (only download and resource_dispatcher_host.* portions reviewed).
LGTM On Tue, Feb 15, 2011 at 5:33 PM, <rdsmith@chromium.org> wrote: > LGTM (only download and resource_dispatcher_host.* portions reviewed). > > > > http://codereview.chromium.org/6459005/ > |
