Ensuring interception of stream get determined by plugin path before checking mime type.

content/browser/loader/buffered_resource_handler.h

 // 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.
 
 #ifndef CONTENT_BROWSER_LOADER_BUFFERED_RESOURCE_HANDLER_H_
 #define CONTENT_BROWSER_LOADER_BUFFERED_RESOURCE_HANDLER_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 38 matching lines @@
   // ResourceController implementation:
   void Resume() override;
   void Cancel() override;
   void CancelAndIgnore() override;
   void CancelWithError(int error_code) override;
 
   bool ProcessResponse(bool* defer);
 
   bool ShouldSniffContent();
   bool DetermineMimeType();
+  bool IsHandledByPlugin(bool* defer, bool* result);

[mmenke, 2015/03/24 14:44:32]

This should be documented.  We return a boolean, and yet we also take a pointer
to a "result".  Unclear what either means.  Also not clear how we call back in
the case "defer" is set to true.

[raymes, 2015/03/25 00:26:32]

It was never clear how we call back in the case defer is set to true - the
documentation in this class isn't great :) I think that's orthogonal to this
change though.

|result| is the return value of SelectNextHandler and again there is no
documentation here, so it's unclear. But tracing the code through it looks like
it is whether the request should be cancelled.

So we can rename |result| to |cancel_request|

// Returns true if a plugin will handle the current request. |defer| 
// is set to true if plugin data is stale and needs to be refreshed 
// before the request can be handled. |cancel_request| is true if 
// the request should be cancelled and false otherwise.

[Deepak, 2015/03/25 05:43:34]

Done.

   bool SelectNextHandler(bool* defer);
   bool UseAlternateNextHandler(scoped_ptr<ResourceHandler> handler,
                                const std::string& payload_for_old_handler);
 
   bool ReplayReadCompleted(bool* defer);
   void CallReplayReadCompleted();
 
   bool MustDownload();
-  bool HasSupportingPlugin(bool* is_stale);
+  bool GetSupportingPlugin(WebPluginInfo* plugin, bool* stale);

[mmenke, 2015/03/24 14:44:32]

Mind documenting this?  Unclear what the return value means, or stale, for that
matter.

[raymes, 2015/03/25 00:26:32]

As you mentioned in your comment, this function can (and probably should) just
be inlined into IsHandledByPlugin. But for the sake of explanation:

// Returns true if there is a plugin that can handle the current 
// request. If there is such a plugin, it is returned in |plugin|. If 
// the plugin data is out of date and needs to be refreshed from disk,
// |stale| will be set to true and the result will not be valid.

[Deepak, 2015/03/25 05:43:34]

Have made this function inline as it is not getting used anywhere else except in
IsHandledByPlugin().

 
   // Copies data from |read_buffer_| to |next_handler_|.
   bool CopyReadBufferToNextHandler();
 
   // Called on the IO thread once the list of plugins has been loaded.
   void OnPluginsLoaded(const std::vector<WebPluginInfo>& plugins);
 
   enum State {
     STATE_STARTING,
 
@@ skipping 27 matching lines @@
   bool must_download_is_set_;
 
   base::WeakPtrFactory<BufferedResourceHandler> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BufferedResourceHandler);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_LOADER_BUFFERED_RESOURCE_HANDLER_H_