Forward MIME types to BrowserPlugin when a viewer is specified.

content/browser/loader/buffered_resource_handler.cc

 // Copyright (c) 2012 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 "content/browser/loader/buffered_resource_handler.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
 #include "content/browser/download/download_resource_handler.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/loader/certificate_resource_handler.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
+#include "content/browser/loader/stream_resource_handler.h"
 #include "content/browser/plugin_service_impl.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_save_info.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/browser/resource_context.h"
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/webplugininfo.h"
 #include "net/base/io_buffer.h"
@@ skipping 286 matching lines @@
   }
 
   if (!info->allow_download())
     return true;
 
   bool must_download = MustDownload();
   if (!must_download) {
     if (net::IsSupportedMimeType(mime_type))
       return true;
 
-    scoped_ptr<ResourceHandler> handler(
+    scoped_ptr<StreamResourceHandler> handler(
         host_->MaybeInterceptAsStream(request(), response_.get()));
-    if (handler)
-      return UseAlternateNextHandler(handler.Pass());
+    if (handler) {
+      if (handler->HasPayload()) {

[mmenke, 2014/05/12 17:12:13]

I'm really not following any of this code.

+        handler->SetNextHandler(next_handler_.Pass());
+
+        next_handler_ = handler.Pass();
+        next_handler_->SetController(this);
+
+        int request_id =
+            ResourceRequestInfo::ForRequest(request())->GetRequestID();
+        return CopyReadBufferToNextHandler(request_id);

[mmenke, 2014/05/13 15:14:32]

I think it's less than ideal to split the responsibility to handle the
particulars of this case between the BufferedResourceHandler and the
StreamResourceHandler like this - I don't think both classes should know about
the behaviors in this case.

Given that we want to use "UseAlternateNextHander" in the non-payload case, my
suggestion is to do this:

// Feel free to come up with a better name.
std::string payload_to_send_old_handler;

scoped_ptr<StreamResourceHandler> handler(request(), response_.get(),
&payload_to_send_old_handler);

if (handler) {
  UseAlternateNextHandler(handler.PassAs<ResourceHandler>(),
payload_to_send_old_handler);
}

Then just maintain the old behavior unless the payload is non-empty.

I'm also not sure about the non-200 response case, where we cancel the request
in UseAlternateNextHandler - do we want this behavior or not, in the new case?


An alternate approach would be to move all the logic into the
StreamResourceHandler, but I think you'd still need the check if you should call
UseAlternateNextHandler or not, so I prefer the other approach (Also, that means
only the BufferedResourceHandler is responsible for sending information to the
original next handler, in all cases, which I think is a little cleaner).

[Zachary Kuznia, 2014/05/20 22:24:39]

If I understand correctly, you're saying that I should move the code in
StreamResourceHandler() into UseAlternateNextHandler(), and use that codepath
instead of cancelling the request if a payload is passed in via
payload_to_send_old_handler?

[mmenke, 2014/05/21 15:17:49]

That's my suggestion.  I don't think the new behavior should be split between
the BufferedResourceHandler and the StreamResourceHandler.  I'm certainly open
to doing that in other ways, but that seems the simplest option to me.

+      } else {
+        UseAlternateNextHandler(handler.PassAs<ResourceHandler>());
+      }
+    }
 
 #if defined(ENABLE_PLUGINS)
     bool stale;
     bool has_plugin = HasSupportingPlugin(&stale);
     if (stale) {
       // Refresh the plugins asynchronously.
       PluginServiceImpl::GetInstance()->GetPlugins(
           base::Bind(&BufferedResourceHandler::OnPluginsLoaded,
                      weak_ptr_factory_.GetWeakPtr()));
       request()->LogBlockedBy("BufferedResourceHandler");
@@ skipping 139 matching lines @@
   request()->LogUnblocked();
   bool defer = false;
   if (!ProcessResponse(&defer)) {
     controller()->Cancel();
   } else if (!defer) {
     controller()->Resume();
   }
 }
 
 }  // namespace content