Componentize component_updater: Move URLRequestPrepackagedInterceptor from content/ to net/

net/url_request/url_request_prepackaged_interceptor.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 CHROME_BROWSER_COMPONENT_UPDATER_COMPONENT_UPDATER_INTERCEPTOR_H_
-#define CHROME_BROWSER_COMPONENT_UPDATER_COMPONENT_UPDATER_INTERCEPTOR_H_
+#ifndef NET_URL_REQUEST_URL_REQUEST_PREPACKAGED_INTERCEPTOR_H_
+#define NET_URL_REQUEST_URL_REQUEST_PREPACKAGED_INTERCEPTOR_H_
 
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
 
 class GURL;
 
 namespace base {
 class FilePath;
+class TaskRunner;
 }
 
-namespace content {
+namespace net {
 
 // Intercepts HTTP requests and gives pre-defined responses to specified URLs.
 // The pre-defined responses are loaded from files on disk.  The interception
 // occurs while the URLRequestPrepackagedInterceptor is alive.
 class URLRequestPrepackagedInterceptor {

[wtc, 2014/08/26 18:15:59]

I think chrome/browser/net is a better home for this class because all the files
using this class are under chrome/browser.

[Ryan Sleevi, 2014/08/26 18:21:09]

Except that they wish to use it from iOS

[tommycli, 2014/08/26 18:21:34]

I can't do that, as components/ cannot depend on chrome/.

I could offer to make a new component under /components/url_request, but that's
probably overkill for just one cc/h file.

  public:
   // Registers an interceptor for urls using |scheme| and |hostname|. Urls
   // passed to "SetResponse" are required to use |scheme| and |hostname|.
-  URLRequestPrepackagedInterceptor(const std::string& scheme,
-                                   const std::string& hostname);
+  URLRequestPrepackagedInterceptor(
+      const std::string& scheme,
+      const std::string& hostname,
+      const scoped_refptr<base::TaskRunner>& io_task_runner,

[Sorin Jianu, 2014/08/26 20:32:52]

Any reason we are passing ref counted pointers by reference? See
http://www.chromium.org/developers/smart-pointer-guidelines 

[tommycli, 2014/08/26 21:06:58]

Quote from that page:

The one exception is functions can takes a const scoped_refptr& as a parameter
which may avoid extra refcount churn.

[Sorin Jianu, 2014/08/26 23:31:50]

ok, sg, using CS shows that passing scoped_refptr by ref is more common too.

+      const scoped_refptr<base::TaskRunner>& worker_task_runner);
   virtual ~URLRequestPrepackagedInterceptor();
 
   // When requests for |url| arrive, respond with the contents of |path|. The
   // hostname and scheme of |url| must match the corresponding parameters
   // passed as constructor arguments.
   void SetResponse(const GURL& url, const base::FilePath& path);
 
   // Identical to SetResponse except that query parameters are ignored on
   // incoming URLs when comparing against |url|.
   void SetResponseIgnoreQuery(const GURL& url, const base::FilePath& path);
 
   // Returns how many requests have been issued that have a stored reply.
   int GetHitCount();
 
  private:
   class Delegate;
 
   const std::string scheme_;
   const std::string hostname_;
 
-  // After creation, |delegate_| lives on the IO thread, and a task to delete
-  // it is posted from ~URLRequestPrepackagedInterceptor().
+  const scoped_refptr<base::TaskRunner> io_task_runner_;

[Sorin Jianu, 2014/08/26 20:32:52]

I wonder what is the meaning of the "const" object here. Why invariant are we
trying to enforce?

[tommycli, 2014/08/26 21:06:58]

I thought: After we initialize its value, it cannot be reassigned to point to
something else?

[Sorin Jianu, 2014/08/26 23:31:49]

I think this is a strange idiom. It is hard to count by CS (not enough context
to filter the declarations of class members) but it seems to me, empirically,
that people declare more non-const members than const members.

You are right to say that it prevents future assignments to the smart pointer
member, but the idiom is odd in the sense that that it gives the impression that
the programmer intended to forbid calling non-const functions on the object that
the smart pointer wraps. 

Unlike class members of builtin types or unmanaged user-define types, which can
be easily and accidentally modified by the members of the object, it requires
effort to change a smart pointer. I assume that's why people don't bother with
the const declaration here.

I would not use a const declaration here but that is only a personal preference.

[tommycli, 2014/08/26 23:45:22]

Oh I see. You're saying it gives the false impression that the object itself is
const.

Okay. Sounds good. I changed the pointer to non-const.

+
+  // After creation, |delegate_| lives on the thread of the |io_task_runner_|,
+  // and a task to delete it is posted from ~URLRequestPrepackagedInterceptor().
   Delegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestPrepackagedInterceptor);
 };
 
 // Specialization of URLRequestPrepackagedInterceptor where scheme is "http" and
 // hostname is "localhost".
 class URLLocalHostRequestPrepackagedInterceptor
     : public URLRequestPrepackagedInterceptor {
  public:
-  URLLocalHostRequestPrepackagedInterceptor();
+  URLLocalHostRequestPrepackagedInterceptor(
+      const scoped_refptr<base::TaskRunner>& io_task_runner,
+      const scoped_refptr<base::TaskRunner>& worker_task_runner);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(URLLocalHostRequestPrepackagedInterceptor);
 };
 
-}  // namespace content
+}  // namespace net
 
-#endif  // CHROME_BROWSER_COMPONENT_UPDATER_COMPONENT_UPDATER_INTERCEPTOR_H_
+#endif  // NET_URL_REQUEST_URL_REQUEST_PREPACKAGED_INTERCEPTOR_H_