Implement the feedbackPrivate API.

Implement the feedbackPrivate API.
This CL implements the feedbackPrivate API. There is one change the the API from the original spec - the attached file is now an object with a "name" and "blob" field, since there is no [good] way to pull a filename from a blob object (a blob's interface is supposed to be independent of it's underlying backing implementation). The UI that uses this API will be in a separate CL. Its id is included in the whitelist.

Tests are being tracked by crbug.com/246301 and will follow shortly.

R=asargent@chromium.org
BUG=241635
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206931

[asargent_no_longer_on_chrome, 2013-06-17 19:32:40]

I still need to look at the chrome/renderer stuff (will get to that shortly),
but here are a handful of nits and suggestions. 

Overall seems fine though.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/blob_reader.h (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/blob_reader.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_FEEDBACK_PRIVATE_BLOB_READER_H_
It seems like this header and the .cc file could live somewhere more general,
like chrome/browser/common/ (or even in content/public/browser/ ). Maybe consult
joi@ ?

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/blob_reader.h:40: Profile*
profile_;
nit: I don't see you actually using |profile_| anywhere in the .cc file - can
you remove the need to pass one in? (That might allow you to have this class
live in content/public/browser/)

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
(right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:11:
#include "chrome/browser/extensions/api/feedback_private/blob_reader.h"
nit: I don't actually see you using the BlobReader class here - can this include
be removed?

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:28:
g_factory = LAZY_INSTANCE_INITIALIZER;
style nit: shouldn't line 28 here have a 4-character indent?

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:54: if
(profile_ && ExtensionSystem::Get(profile_)->event_router()) {
Will the feedback extension always be installed, or only in some deployments? To
make this more efficient, you could also first test if there are any listeners
by calling HasEventListener on the event router, and avoid the work of creating
the Event object only to have BroadcastEvent throw it away if there aren't any
listeners.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:92:
::OnGetSystemInformationCompleted(const SystemInformationList& sys_info) {
optional suggestion: could you just rename this function "Finish" or "Complete"
or something, instead of "OnGetSystemInformationCompleted" (which requires this
line-wrapping)? The fact that it's for the getSystemInformation function is
already conveyed by the class name.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:157:
success == true ? api::feedback_private::STATUS_SUCCESS :
nit: can you remove the "== true" part and just have "success ?" ?

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/feedback_service.h (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_service.h:43: // Start
to gather system information.
nit: a blank line before line 43 would aid readability

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_service.h:52: virtual
base::WeakPtr<FeedbackService> GetWeakPtr() = 0;
nit: can you just extend SupportsWeakPtr to get this without the explicit
declaration here?

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/feedback_private.idl (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/feedback_private.idl:37: // Screenshot to send with
this feedback. This need to be a base64 encoded
typo: "This need" -> "This needs"

Also, is the "base64 encoded" part of this comment still accurate? If we're
dealing with a Blob, wouldn't we just pass the raw bytes instead of a
base64-encoded string representing the bytes?

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/feedback_private.idl:56: void(SystemInformation[]
systemInformation);
Since this is a private API it's ok for this to be an unstructured list of
key/value pairs, but to graduate to publicly supported it might be nice to see
if we could declare (or at least document) all the values we know will be
present. 

Also note that we have systemInfo.{cpu,memory,display} which provide some system
information. We'll eventually want to make sure to eliminate any duplication in
the information they provide and what you return from getSystemInformation here.
(Perhaps by pushing additional things you need into one or more of the
namespaces in systemInfo.* ?)

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/e...
File chrome/common/extensions/extension_manifest_constants.cc (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/e...
chrome/common/extensions/extension_manifest_constants.cc:39: const char
kFeedbackPrivate[] = "feedbackPrivate";
nit: Do you need this? This is only needed if you have a manifest key (not just
a named permission value). E.g.

{
  "name": "My feedback extension",
  "version": "0.5",
  "description": "My awesome extension to collect feedback",
  "permissions": ["tabs", "feedbackPrivate"],
  "feedbackPrivate": ??????  <-- would something go here?
  ...
}

[asargent_no_longer_on_chrome, 2013-06-17 20:50:14]

lgtm

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:211:
"gfdkimpbcpahaombhbimeihdjnejgicl" // Feedback UI.
In case you need it (probably not in this case, but just FYI):

http://shortn/_hPPpuPq7HM

[rkc, 2013-06-17 21:48:19]

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/blob_reader.h (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/blob_reader.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_FEEDBACK_PRIVATE_BLOB_READER_H_
On 2013/06/17 19:32:40, Antony Sargent wrote:
> It seems like this header and the .cc file could live somewhere more general,
> like chrome/browser/common/ (or even in content/public/browser/ ). Maybe
consult
> joi@ ?

This is a really basic user of URLFetcher, I almost contemplated putting the
entire class in the feedback_service.cc file itself. I'll speak with joi@ to see
if we can make this use case (read from a URL, dump it to string) a common class
called something more like URLReader? I'll start a separate CL to move this
class once I've spoken to him.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/blob_reader.h:40: Profile*
profile_;
On 2013/06/17 19:32:40, Antony Sargent wrote:
> nit: I don't see you actually using |profile_| anywhere in the .cc file - can
> you remove the need to pass one in? (That might allow you to have this class
> live in content/public/browser/)

I don't need to save the profile (hence removing this from here) but I do need
to use it in the .cc file (line 22:
fetcher_->SetRequestContext(profile->GetRequestContext());)

Also removed the unused weak_ptr_factory_.

Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
(right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:11:
#include "chrome/browser/extensions/api/feedback_private/blob_reader.h"
On 2013/06/17 19:32:40, Antony Sargent wrote:
> nit: I don't actually see you using the BlobReader class here - can this
include
> be removed?

Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:28:
g_factory = LAZY_INSTANCE_INITIALIZER;
On 2013/06/17 19:32:40, Antony Sargent wrote:
> style nit: shouldn't line 28 here have a 4-character indent?

Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:54: if
(profile_ && ExtensionSystem::Get(profile_)->event_router()) {
On 2013/06/17 19:32:40, Antony Sargent wrote:
> Will the feedback extension always be installed, or only in some deployments?
To
> make this more efficient, you could also first test if there are any listeners
> by calling HasEventListener on the event router, and avoid the work of
creating
> the Event object only to have BroadcastEvent throw it away if there aren't any
> listeners. 

Since the current feedback UI is available in all deployments of Chrome, we'd
want the new app based feedback UI to do the same. So I believe that the
feedback component app will be always installed with Chrome.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:92:
::OnGetSystemInformationCompleted(const SystemInformationList& sys_info) {
On 2013/06/17 19:32:40, Antony Sargent wrote:
> optional suggestion: could you just rename this function "Finish" or
"Complete"
> or something, instead of "OnGetSystemInformationCompleted" (which requires
this
> line-wrapping)? The fact that it's for the getSystemInformation function is
> already conveyed by the class name. 
> 

I agree, this was really looking quite ugly. Changed to OnCompleted.
Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:157:
success == true ? api::feedback_private::STATUS_SUCCESS :
On 2013/06/17 19:32:40, Antony Sargent wrote:
> nit: can you remove the "== true" part and just have "success ?" ?
> 

Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/feedback_service.h (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_service.h:43: // Start
to gather system information.
On 2013/06/17 19:32:40, Antony Sargent wrote:
> nit: a blank line before line 43 would aid readability

Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_service.h:52: virtual
base::WeakPtr<FeedbackService> GetWeakPtr() = 0;
On 2013/06/17 19:32:40, Antony Sargent wrote:
> nit: can you just extend SupportsWeakPtr to get this without the explicit
> declaration here?

So we can't really have FeedbackService : SupportWeakPtr since we never really
instantiate FeedbackService. Having FeedbackService derive from SupportsWeakPtr
doesn't work since then the AsWeakPtr method will return a FeedbackService
pointer, but the code in FeedbackServiceImpl needs a FeedbackServiceImpl weak
pointer - so FeedbackServiceImpl needs to directly derive from SupportsWeakPtr.
Hence changed the FeedbackServiceImpl classes to use SupportsWeakPtr instead of
the boilerplate weak_ptr_factory_ code. 


We still need this method because we need a weak pointer in
FeedbackService::SendFeedback, so the FeedbackServiceImpl needs a method via
which it can return it's weak pointer which we can use in the base class method.

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:211:
"gfdkimpbcpahaombhbimeihdjnejgicl" // Feedback UI.
On 2013/06/17 20:50:14, Antony Sargent wrote:
> In case you need it (probably not in this case, but just FYI):
> 
> http://shortn/_hPPpuPq7HM

No real reason not to use the hash instead.
Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/feedback_private.idl (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/feedback_private.idl:37: // Screenshot to send with
this feedback. This need to be a base64 encoded
On 2013/06/17 19:32:40, Antony Sargent wrote:
> typo: "This need" -> "This needs"
> 
> Also, is the "base64 encoded" part of this comment still accurate? If we're
> dealing with a Blob, wouldn't we just pass the raw bytes instead of a
> base64-encoded string representing the bytes?

Ah, old comment, changed.
I had originally started with sending the data as base64 till mpcomplete@
suggested on the API review that I change this to blobs.

Done.

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/feedback_private.idl:56: void(SystemInformation[]
systemInformation);
On 2013/06/17 19:32:40, Antony Sargent wrote:
> Since this is a private API it's ok for this to be an unstructured list of
> key/value pairs, but to graduate to publicly supported it might be nice to see
> if we could declare (or at least document) all the values we know will be
> present. 
> 
> Also note that we have systemInfo.{cpu,memory,display} which provide some
system
> information. We'll eventually want to make sure to eliminate any duplication
in
> the information they provide and what you return from getSystemInformation
here.
> (Perhaps by pushing additional things you need into one or more of the
> namespaces in systemInfo.* ?)

I'll think about how to integrate this information with systemInfo. Currently
the system information that this API pulls is from a dedicated fetcher where
devs add logs specifically to have them show up in about:system and to go to
feedback reports. So the 'purpose' of those logs is different than those from
systemInfo; but we can try to ensure that we don't gather logs again which are
already available from systemInfo. I'll investigate this further.

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/e...
File chrome/common/extensions/extension_manifest_constants.cc (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/common/extensions/e...
chrome/common/extensions/extension_manifest_constants.cc:39: const char
kFeedbackPrivate[] = "feedbackPrivate";
On 2013/06/17 19:32:40, Antony Sargent wrote:
> nit: Do you need this? This is only needed if you have a manifest key (not
just
> a named permission value). E.g.
> 
> {
>   "name": "My feedback extension",
>   "version": "0.5",
>   "description": "My awesome extension to collect feedback",
>   "permissions": ["tabs", "feedbackPrivate"],
>   "feedbackPrivate": ??????  <-- would something go here?
>   ...
> }
> 

Ah, I misunderstood the purpose. I thought this was needed if I was adding a
permission with the name feedbackPrivate.

Removed.

[rkc, 2013-06-17 21:54:04]

Added thakis@ for OWNER's reviews on,

chrome/renderer/resources/renderer_resources.grd
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

[asargent_no_longer_on_chrome, 2013-06-17 22:12:58]

(still lgtm, just responding to comments)

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/blob_reader.h (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/blob_reader.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_FEEDBACK_PRIVATE_BLOB_READER_H_
On 2013/06/17 21:48:19, Rahul Chaturvedi wrote:
> On 2013/06/17 19:32:40, Antony Sargent wrote:
> > It seems like this header and the .cc file could live somewhere more
general,
> > like chrome/browser/common/ (or even in content/public/browser/ ). Maybe
> consult
> > joi@ ?
> 
> This is a really basic user of URLFetcher, I almost contemplated putting the
> entire class in the feedback_service.cc file itself. I'll speak with joi@ to
see
> if we can make this use case (read from a URL, dump it to string) a common
class
> called something more like URLReader? I'll start a separate CL to move this
> class once I've spoken to him.

Good point. Is this even used from more than one file? If not, it could make
sense to move into an anonymous namespace in the .cc file. Anyhow, waiting to
see what Joi thinks and potentially doing a follow-up CL sounds fine to me.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/blob_reader.h:40: Profile*
profile_;
On 2013/06/17 21:48:19, Rahul Chaturvedi wrote:
> On 2013/06/17 19:32:40, Antony Sargent wrote:
> > nit: I don't see you actually using |profile_| anywhere in the .cc file -
can
> > you remove the need to pass one in? (That might allow you to have this class
> > live in content/public/browser/)
> 
> I don't need to save the profile (hence removing this from here) but I do need
> to use it in the .cc file (line 22:
> fetcher_->SetRequestContext(profile->GetRequestContext());)
> 
> Also removed the unused weak_ptr_factory_.
> 
> Done.
Oops, I missed that (I searched for "profile_" but not just "profile"). You
might consider just passing in the RequestContextGetter instead of the Profile.

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/feedback_service.h (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/feedback_service.h:52: virtual
base::WeakPtr<FeedbackService> GetWeakPtr() = 0;
On 2013/06/17 21:48:19, Rahul Chaturvedi wrote:
> On 2013/06/17 19:32:40, Antony Sargent wrote:
> > nit: can you just extend SupportsWeakPtr to get this without the explicit
> > declaration here?
> 
> So we can't really have FeedbackService : SupportWeakPtr since we never really
> instantiate FeedbackService. Having FeedbackService derive from
SupportsWeakPtr
> doesn't work since then the AsWeakPtr method will return a FeedbackService
> pointer, but the code in FeedbackServiceImpl needs a FeedbackServiceImpl weak
> pointer - so FeedbackServiceImpl needs to directly derive from
SupportsWeakPtr.
> Hence changed the FeedbackServiceImpl classes to use SupportsWeakPtr instead
of
> the boilerplate weak_ptr_factory_ code. 
> 
> 
> We still need this method because we need a weak pointer in
> FeedbackService::SendFeedback, so the FeedbackServiceImpl needs a method via
> which it can return it's weak pointer which we can use in the base class
method.

Ok.

[Nico, 2013-06-17 22:15:05]

On 2013/06/17 21:54:04, Rahul Chaturvedi wrote:
> Added thakis@ for OWNER's reviews on,
> 
> chrome/renderer/resources/renderer_resources.grd
> chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

These two files lgtm.

[commit-bot: I haz the power, 2013-06-17 22:17:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/17111003/12001

[commit-bot: I haz the power, 2013-06-17 22:17:41]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 548.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
11fbd7925f75bbc30ae97aa53985cfdedd54d453..90d6b2499814bda44b2011c8ccf52200c412ddf3
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -548,6 +548,9 @@ enum HistogramValue {
   SYSTEMINFO_MEMORY_GET,
   ACTIVITYLOGPRIVATE_GETEXTENSIONACTIVITIES,
   RUNTIME_GETPACKAGEDIRECTORYENTRY,
+  FEEDBACKPRIVATE_GETUSEREMAIL,
+  FEEDBACKPRIVATE_GETSYSTEMINFORMATION,
+  FEEDBACKPRIVATE_SENDFEEDBACK,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-06-17 22:32:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/17111003/20001

[commit-bot: I haz the power, 2013-06-17 22:52:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-17 23:02:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/17111003/37001

[kinuko, 2013-06-18 06:00:56]

https://codereview.chromium.org/17111003/diff/37001/chrome/renderer/extension...
File chrome/renderer/extensions/feedback_private_custom_bindings.cc (right):

https://codereview.chromium.org/17111003/diff/37001/chrome/renderer/extension...
chrome/renderer/extensions/feedback_private_custom_bindings.cc:17:
args.GetReturnValue().Set(v8::String::New(blob.url().spec().data()));
Drive-by comment. WebBlob::url() returns Blob's internal URL, which is
automatically unregistered when the Blob's reference is GC'ed.  We have a
standard way to get a Blob URL in JS layer
(http://www.w3.org/TR/FileAPI/#dfn-createObjectURL) which actually adds a
reference to the Blob so that the Blob and its data is alive until we do call
revokeObjectURL.

Is there a reason we don't use the standard way to get a 'public' Blob URL in
feedback_private_custom_bindings.js but use this V8 wrapper code?  Are we sure
the blob reference is around when we use the URL?

[commit-bot: I haz the power, 2013-06-18 08:10:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/17111003/37001

[Jói, 2013-06-18 10:29:08]

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/feedback_private/blob_reader.h (right):

https://codereview.chromium.org/17111003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/feedback_private/blob_reader.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_FEEDBACK_PRIVATE_BLOB_READER_H_
On 2013/06/17 22:12:58, Antony Sargent wrote:
> On 2013/06/17 21:48:19, Rahul Chaturvedi wrote:
> > On 2013/06/17 19:32:40, Antony Sargent wrote:
> > > It seems like this header and the .cc file could live somewhere more
> general,
> > > like chrome/browser/common/ (or even in content/public/browser/ ). Maybe
> > consult
> > > joi@ ?
> > 
> > This is a really basic user of URLFetcher, I almost contemplated putting the
> > entire class in the feedback_service.cc file itself. I'll speak with joi@ to
> see
> > if we can make this use case (read from a URL, dump it to string) a common
> class
> > called something more like URLReader? I'll start a separate CL to move this
> > class once I've spoken to him.
> 
> Good point. Is this even used from more than one file? If not, it could make
> sense to move into an anonymous namespace in the .cc file. Anyhow, waiting to
> see what Joi thinks and potentially doing a follow-up CL sounds fine to me. 

I would recommend following the principle of narrowest scope here; if this is
used only by one file, you can move it in to the anonymous namespace in the .cc
file using it.  If it is used by more than one, then move it to whatever the
narrowest common scope for those is (e.g. possibly
chrome/browser/extensions/api).  Only if it is intended to be reused by multiple
subsystems and such reuse is going to occur in the near foreseeable future,
should it be moved to somewhere "common" like its own component (under
//components) or some such.  Also, FYI, //chrome/browser/common is gone, or at
least going, as part of a change I reviewed recently.

[commit-bot: I haz the power, 2013-06-18 11:10:00]

Change committed as 206931