Require manifests for native messaging hosts.

Require manifests for native messaging hosts.

Previously native messaging hosts were discovered by looking in the user 
data directory. That approach is hard to use and is not reliable. Now 
chrome will look for a host manifest file in a system-specific directory 
and load that file to find host binary.

BUG=142915
TBR=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185189

[Matt Perry, 2013-02-16 02:00:51]

handful of nits, mostly LGTM

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_messaging_apitest.cc
(right):

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_apitest.cc:43:
ASSERT_TRUE(serializer.Serialize(*manifest));
nit: maybe move all this to a helper function

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc
(right):

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:51:
scoped_ptr<NativeMessagingHostManifest> result(
unused

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:60:
*error_message = "Invalid manifest file.";
Better: "Invalid value for <key>"

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:60:
*error_message = "Invalid manifest file.";
Also, maybe description should be optional.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:67:
*error_message = "Unknown native messaging host type: " + type;
Better for consistency: "Invalid value for type: " + type

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:76:
*error_message = "allowed_origins must be list of strings.";
Better for consistency: "Invalid value for allowed_origins. Expected a list of
strings."

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:76:
*error_message = "allowed_origins must be list of strings.";
Isn't allowed_origins optional?

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:87:
allowed_origins_.insert(origin);
are these strings, or URL patterns? Extensions use the URLPattern class.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:34:
base::SplitString(value, ',', &hosts);
note this means that you can't have commas or equals in the native host name or
path. seems fine, but you should document that at least.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:138:
error_message = "Invalid name specified in the manifest.";
"Name specified in the manifest does not match"?

[Sergey Ulanov, 2013-02-20 00:05:36]

PTAL. Addressed all comments and added unittests.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_messaging_apitest.cc
(right):

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_apitest.cc:43:
ASSERT_TRUE(serializer.Serialize(*manifest));
On 2013/02/16 02:00:51, Matt Perry wrote:
> nit: maybe move all this to a helper function

Done.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc
(right):

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:51:
scoped_ptr<NativeMessagingHostManifest> result(
On 2013/02/16 02:00:51, Matt Perry wrote:
> unused

Done.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:60:
*error_message = "Invalid manifest file.";
On 2013/02/16 02:00:51, Matt Perry wrote:
> Also, maybe description should be optional.

I think it's better if it is required. It doesn't require much effort for a
developer to write the description, and it better if all hosts have a
description in case we start using it somehow.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:60:
*error_message = "Invalid manifest file.";
On 2013/02/16 02:00:51, Matt Perry wrote:
> Better: "Invalid value for <key>"

Done.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:67:
*error_message = "Unknown native messaging host type: " + type;
On 2013/02/16 02:00:51, Matt Perry wrote:
> Better for consistency: "Invalid value for type: " + type

Done.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:76:
*error_message = "allowed_origins must be list of strings.";
On 2013/02/16 02:00:51, Matt Perry wrote:
> Better for consistency: "Invalid value for allowed_origins. Expected a list of
> strings."

Done.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:76:
*error_message = "allowed_origins must be list of strings.";
On 2013/02/16 02:00:51, Matt Perry wrote:
> Isn't allowed_origins optional?

No, it's not optional. Access to each native host is only allowed for a fixed
list of extensions.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.cc:87:
allowed_origins_.insert(origin);
On 2013/02/16 02:00:51, Matt Perry wrote:
> are these strings, or URL patterns? Extensions use the URLPattern class.

Updated this code to use URLPattern and URLPatternSet.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:34:
base::SplitString(value, ',', &hosts);
On 2013/02/16 02:00:51, Matt Perry wrote:
> note this means that you can't have commas or equals in the native host name
or
> path. seems fine, but you should document that at least.

Current design doc says that these names should be in the alphabet [a-z0-9._],
i.e. commas in the name are not allowed. I've added checks for it in this CL.
This code doesn't work for the case when comma is in the path, but I don't think
we need to worry about it given that this switch is just for testing.

https://codereview.chromium.org/12285015/diff/3026/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:138:
error_message = "Invalid name specified in the manifest.";
On 2013/02/16 02:00:51, Matt Perry wrote:
> "Name specified in the manifest does not match"?

Done.

[Matt Perry, 2013-02-20 00:44:20]

lgtm

https://codereview.chromium.org/12285015/diff/17001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_messaging_host_manifest.h
(right):

https://codereview.chromium.org/12285015/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.h:31:
static bool IsValidHostName(const std::string& name);
This confused me at first, thinking "hostname" referred to a web hostname. How
about IsValidName?

[Sergey Ulanov, 2013-02-20 00:56:17]

https://codereview.chromium.org/12285015/diff/17001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_messaging_host_manifest.h
(right):

https://codereview.chromium.org/12285015/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest.h:31:
static bool IsValidHostName(const std::string& name);
On 2013/02/20 00:44:20, Matt Perry wrote:
> This confused me at first, thinking "hostname" referred to a web hostname. How
> about IsValidName?

Done.

[Sergey Ulanov, 2013-02-27 23:08:28]

+thakis in TBR for chrome/chrome_browser_extensions.gypi

[commit-bot: I haz the power, 2013-02-27 23:09:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12285015/35001

[commit-bot: I haz the power, 2013-02-28 00:12:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12285015/49003

[commit-bot: I haz the power, 2013-02-28 01:48:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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-02-28 02:13:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12285015/57004

[commit-bot: I haz the power, 2013-02-28 04:02:16]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-28 07:07:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12285015/62003

[commit-bot: I haz the power, 2013-02-28 09:19:04]

Change committed as 185189

[Nico, 2013-02-28 09:25:30]

gypi lgtm