Implementation of ResourceDispatcher::OnDownloadProgress

chrome/common/resource_dispatcher.cc

 // Copyright (c) 2006-2008 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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "chrome/common/resource_dispatcher.h"
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
@@ skipping 262 matching lines @@
   // Make sure any deferred messages are dispatched before we dispatch more.
   if (!request_info.deferred_message_queue.empty())
     FlushDeferredMessages(request_id);
 
   DispatchMessage(message);
   return true;
 }
 
 void ResourceDispatcher::OnDownloadProgress(
     int request_id, int64 position, int64 size) {
-  // TODO(hclam): delegate this message to
-  // ResourceLoaderBridge::Peer::OnDownloadProgress and send an ACK message
-  // back to ResourceDispatcherHost.
+  PendingRequestList::iterator it = pending_requests_.find(request_id);
+  if (it == pending_requests_.end()) {
+     DLOG(WARNING) << "Got download progress for a nonexistant or "

[Erik does not do reviews, 2009/03/04 17:55:44]

should this be a DCHECK?

[Alpha Left Google, 2009/03/05 00:32:06]

I guess browser process can accidentally send resource messages to renderer with
invalid request_id sometimes, it would be better log it than crash the whole
renderer.

+         " finished requests";
+     return;
+  }
+
+  PendingRequestInfo& request_info = it->second;
+
+  RESOURCE_LOG("Dispatching download progress for " <<

[Erik does not do reviews, 2009/03/04 17:55:44]

this log macro looks different than our normal style.  I'd expect it to be
RESOURCE_LOG("msg") << extra; rather than RESOURCE_LOG("msg" << extra);  Is this
correct?

[Alpha Left Google, 2009/03/05 00:32:06]

It looks strange to me too, but seems the whole file is using this style, I
would prefer making it coherent.

+               request_info.peer->GetURLForDebugging());
+  request_info.peer->OnDownloadProgress(position, size);

[Erik does not do reviews, 2009/03/04 17:55:44]

is request_info.peer guaranteed to be non-null?  DCHECK?

[Alpha Left Google, 2009/03/05 00:32:06]

This assumption is made throughout this file too.

+
+  // Send the ACK message back.
+  IPC::Message::Sender* sender = message_sender();
+  if (sender)

[Erik does not do reviews, 2009/03/04 17:55:44]

Since the contents of the if wrap to multiple lines, use {} to make it more
clear.

[Alpha Left Google, 2009/03/05 00:32:06]

Done.

+    sender->Send(
+        new ViewHostMsg_DownloadProgress_ACK(MSG_ROUTING_NONE, request_id));
 }
 
 void ResourceDispatcher::OnUploadProgress(
     int request_id, int64 position, int64 size) {
   PendingRequestList::iterator it = pending_requests_.find(request_id);
   if (it == pending_requests_.end()) {
     // this might happen for kill()ed requests on the webkit end, so perhaps
     // it shouldn't be a warning...
     DLOG(WARNING) << "Got upload progress for a nonexistant or "
         "finished request";
@@ skipping 225 matching lines @@
     case ViewMsg_Resource_RequestComplete::ID:
       return true;
 
     default:
       break;
   }
 
   return false;
 }