SSLTCP (pseudo-SSL with fake handshake and unencrypted data) support for p2p socket.

SSLTCP (pseudo-SSL with fake handshake and unencrypted data) support for p2p socket.

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

[Ronghua Wu (Left Chromium), 2013-06-15 00:23:35]

I'm still adding tests for the tcp server case. Apart from that this is ready
for review. Especially I would like you to take a look see if you are fine with
the approach.

[Mallinath (Gone from Chromium), 2013-06-17 22:13:22]

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
File content/browser/renderer_host/p2p/ssltcp_helper.cc (right):

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.cc:5: #include
"content/browser/renderer_host/p2p/ssltcp_helper.h"
I discovered this lately, there is already code which does this. Take a look at
jingle/glue/fake_ssl_client_socket.cc

[juberti, 2013-06-17 22:19:53]

On 2013/06/15 00:23:35, Ronghua Wu wrote:
> I'm still adding tests for the tcp server case. Apart from that this is ready
> for review. Especially I would like you to take a look see if you are fine
with
> the approach.

FWIW there's no need to support SSLTCP server behavior in Chrome. We only
connect outward right now; if we need this later we can easily add it.

[Sergey Ulanov, 2013-06-18 20:01:03]

On 2013/06/17 22:19:53, juberti wrote:
> On 2013/06/15 00:23:35, Ronghua Wu wrote:
> > I'm still adding tests for the tcp server case. Apart from that this is
ready
> > for review. Especially I would like you to take a look see if you are fine
> with
> > the approach.
> 
> FWIW there's no need to support SSLTCP server behavior in Chrome. We only
> connect outward right now; if we need this later we can easily add it.

I think I mentioned this somewhere: AFAIK the reason we have is server sockets
disabled is to workaround firewall message on Windows. It's a windows-only
problem, so maybe we can enable it for Mac and ChromeOS?

[Sergey Ulanov, 2013-06-18 20:01:26]

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:28: int id, bool ssl)
nit: one argument per line please.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:49: if
(ssltcp_helper_.get()) {
nit: don't need .get() here and in Init().

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:50: // Init ssltcp helper
to server mode.
You wouldn't need this comment if you used an enum instead of bool:
  ssltcp_helper_->Init(SssltcpHelper::SERVER);

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:337: return
MaybeHandleSslHello(&cur, &packet_size, &consumed);
I think this should be handled in DidCompleteRead(). ProcessInput() should only
be called after hello has been received.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:408: expected_len =
ssltcp_helper_->hello_message_size();
Why do we need this here? P2PSocketHostTcpBase::Send() should take care of hello
messages.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.h (right):

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.h:32:
P2PSocketHostTcpBase(IPC::Sender* message_sender, int id, bool ssl);
can you please define some enum (e.g. SslMode) with two values instead of using
bool here (it helps to make the calling code more readable).

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.h:58: scoped_ptr<SsltcpHelper>
ssltcp_helper_;
does this need to be protected? I think P2PSocketHostTcp and
P2PSocketHostStunTcp shouldn't care if this is a SSL socket or not.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.h:60: net::IPEndPoint
remote_address_;
This still can private.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
File content/browser/renderer_host/p2p/ssltcp_helper.cc (right):

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.cc:11: const char
kSslClientHello[] = {
Are these standard? Can other implementation use slightly different message?

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.cc:63:
client_hello_message_(client_hello_message_size()),
client_hello_message_(kSslClientHello, sizeof(kSslClientHello)), then you don't
need memcpy below.

same for the next field.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.cc:106: bool
SsltcpHelper::IsHelloMessage(const char* message) {
I think it would be better to pass size of the buffer here. Then the caller
wouldn't have to worry about comparing message size with
remote_hello_message_size().

Also you can call it ProcessHelloMessage() and set hello_received_ right here,
so the caller wouldn't need to worry about it.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.cc:107: const char* expect =
client_ ? kSslServerHello : kSslClientHello;
 const std::string& expected = client_ ? server_hello_message_ :
client_hello_message_;
And then compare the strings. You also wound't need remote_hello_message_size()
in that case.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.cc:108: return (memcmp(expect,
message, remote_hello_message_size()) == 0);
nit: IMHO it's less readable with these parentheses, but feel free to keep them
if you disagree.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
File content/browser/renderer_host/p2p/ssltcp_helper.h (right):

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.h:15: size_t
client_hello_message_size();
These do not seem to be used anywhere except in ssltcp_helper.cc. If you really
need to make them available outside make them static class memebers.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.h:18: class CONTENT_EXPORT
SsltcpHelper {
Add comments to explain what this class is used for.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.h:23: void Init(bool client);
instead of using bool define enum Mode { CLIENT, SERVER } and use it here.

[Sergey Ulanov, 2013-06-18 20:05:49]

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
File content/browser/renderer_host/p2p/ssltcp_helper.cc (right):

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/ssltcp_helper.cc:5: #include
"content/browser/renderer_host/p2p/ssltcp_helper.h"
On 2013/06/17 22:13:22, mallinath2 wrote:
> I discovered this lately, there is already code which does this. Take a look
at
> jingle/glue/fake_ssl_client_socket.cc

That's a good point. Looks like you can just swap the underlying the socket with
a FakeSSLClientSocket and it will handle fake SSL handshake for you.

[Ronghua Wu (Left Chromium), 2013-06-18 20:19:14]

On 2013/06/18 20:05:49, Sergey Ulanov wrote:
>
https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
> File content/browser/renderer_host/p2p/ssltcp_helper.cc (right):
> 
>
https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
> content/browser/renderer_host/p2p/ssltcp_helper.cc:5: #include
> "content/browser/renderer_host/p2p/ssltcp_helper.h"
> On 2013/06/17 22:13:22, mallinath2 wrote:
> > I discovered this lately, there is already code which does this. Take a look
> at
> > jingle/glue/fake_ssl_client_socket.cc
> 
> That's a good point. Looks like you can just swap the underlying the socket
with
> a FakeSSLClientSocket and it will handle fake SSL handshake for you.

Correct. I'm updating my cl to use the fakesslclientsocket. However, I got a
presubmit error when tried to upload. Are we not suppose to include any
jingle/glue header in content renderer? I do see the
jingle/jingle.gyp:jingle_glue is list as the dependency in content_renderer.gypi
though.

You added one or more #includes that violate checkdeps rules.
  content/browser/renderer_host/p2p/socket_host_tcp.cc
      Illegal include: "jingle/glue/fake_ssl_client_socket.h"
    Because of no rule applying.

[Ronghua Wu (Left Chromium), 2013-06-18 20:25:19]

On 2013/06/18 20:19:14, Ronghua Wu wrote:
> On 2013/06/18 20:05:49, Sergey Ulanov wrote:
> >
>
https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
> > File content/browser/renderer_host/p2p/ssltcp_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
> > content/browser/renderer_host/p2p/ssltcp_helper.cc:5: #include
> > "content/browser/renderer_host/p2p/ssltcp_helper.h"
> > On 2013/06/17 22:13:22, mallinath2 wrote:
> > > I discovered this lately, there is already code which does this. Take a
look
> > at
> > > jingle/glue/fake_ssl_client_socket.cc
> > 
> > That's a good point. Looks like you can just swap the underlying the socket
> with
> > a FakeSSLClientSocket and it will handle fake SSL handshake for you.
> 
> Correct. I'm updating my cl to use the fakesslclientsocket. However, I got a
> presubmit error when tried to upload. Are we not suppose to include any
> jingle/glue header in content renderer? I do see the
> jingle/jingle.gyp:jingle_glue is list as the dependency in
content_renderer.gypi
> though.
> 
> You added one or more #includes that violate checkdeps rules.
>   content/browser/renderer_host/p2p/socket_host_tcp.cc
>       Illegal include: "jingle/glue/fake_ssl_client_socket.h"
>     Because of no rule applying.

hmm, I'm actually changing the content/browser, which I guess doesn't allow to
depend on jingle/glue?

Maybe move that 2 files to content/browser/renderer_host/p2p?

[Sergey Ulanov, 2013-06-18 20:53:30]

On 2013/06/18 20:25:19, Ronghua Wu wrote:
> On 2013/06/18 20:19:14, Ronghua Wu wrote:
> > On 2013/06/18 20:05:49, Sergey Ulanov wrote:
> > >
> >
>
https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
> > > File content/browser/renderer_host/p2p/ssltcp_helper.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
> > > content/browser/renderer_host/p2p/ssltcp_helper.cc:5: #include
> > > "content/browser/renderer_host/p2p/ssltcp_helper.h"
> > > On 2013/06/17 22:13:22, mallinath2 wrote:
> > > > I discovered this lately, there is already code which does this. Take a
> look
> > > at
> > > > jingle/glue/fake_ssl_client_socket.cc
> > > 
> > > That's a good point. Looks like you can just swap the underlying the
socket
> > with
> > > a FakeSSLClientSocket and it will handle fake SSL handshake for you.
> > 
> > Correct. I'm updating my cl to use the fakesslclientsocket. However, I got a
> > presubmit error when tried to upload. Are we not suppose to include any
> > jingle/glue header in content renderer? I do see the
> > jingle/jingle.gyp:jingle_glue is list as the dependency in
> content_renderer.gypi
> > though.
> > 
> > You added one or more #includes that violate checkdeps rules.
> >   content/browser/renderer_host/p2p/socket_host_tcp.cc
> >       Illegal include: "jingle/glue/fake_ssl_client_socket.h"
> >     Because of no rule applying.
> 
> hmm, I'm actually changing the content/browser, which I guess doesn't allow to
> depend on jingle/glue?

Content layer depends on libjingle, so I don't see any issues adding dependency
on jingle/glue. Add a new DEPS file in renderer_host/p2p or update
content/browser/DEPS.

> 
> Maybe move that 2 files to content/browser/renderer_host/p2p?

[Ronghua Wu (Left Chromium), 2013-06-18 22:19:48]

* Switched to use FakeSSLClientSocket.
* Since we don't have FakeSSLServerSocket, so I will leave the SSLTCP server as
a TODO.

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.h (right):

https://codereview.chromium.org/16516003/diff/37003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.h:32:
P2PSocketHostTcpBase(IPC::Sender* message_sender, int id, bool ssl);
On 2013/06/18 20:01:26, Sergey Ulanov wrote:
> can you please define some enum (e.g. SslMode) with two values instead of
using
> bool here (it helps to make the calling code more readable).

Reused P2PSocketType.

[Sergey Ulanov, 2013-06-19 00:03:10]

LGTM when my comments are addressed.

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
File content/browser/renderer_host/p2p/DEPS (right):

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
content/browser/renderer_host/p2p/DEPS:2: "+jingle/glue",
After thinking more about it - I think it's better to keep this dependency next
to third_party/libjingle. Move it to content/DEPS?

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:78:
socket_.reset(tcp_socket.release());
nit: change this to socket_ = tcp_socket.PassAs<net::StreamSocket>()

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:292: }
DCHECK that type is supported by this class. Same for  P2PSocketHostStunTcp

[Ronghua Wu (Left Chromium), 2013-06-19 00:17:30]

+brettw for 
content/DEPS
content/common/p2p_sockets.h
content/content_browser.gypi

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
File content/browser/renderer_host/p2p/DEPS (right):

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
content/browser/renderer_host/p2p/DEPS:2: "+jingle/glue",
On 2013/06/19 00:03:10, Sergey Ulanov wrote:
> After thinking more about it - I think it's better to keep this dependency
next
> to third_party/libjingle. Move it to content/DEPS?

ok

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:78:
socket_.reset(tcp_socket.release());
On 2013/06/19 00:03:10, Sergey Ulanov wrote:
> nit: change this to socket_ = tcp_socket.PassAs<net::StreamSocket>()

Done.

https://codereview.chromium.org/16516003/diff/97001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:292: }
On 2013/06/19 00:03:10, Sergey Ulanov wrote:
> DCHECK that type is supported by this class. Same for  P2PSocketHostStunTcp

Done.

[Ronghua Wu (Left Chromium), 2013-06-19 20:28:09]

ping brettw

[Ronghua Wu (Left Chromium), 2013-06-20 20:24:03]

+creis, piman in case brettw is not available.

We would like to catch M29. Please help review:
content/DEPS
content/common/p2p_sockets.h
content/content_browser.gypi

[piman, 2013-06-20 20:53:14]

https://codereview.chromium.org/16516003/diff/76003/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/16516003/diff/76003/content/DEPS#newcode58
content/DEPS:58: "+jingle/glue",
So far, jingle has only been called from the renderer process. I don't think we
should blanket-allow jingle to be used everywhere in content, especially on the
browser side.
It looks like you only need a single class from there - which even looks like a
test class, so I'm confused.
If it really needs to be used in the browser, is there a way to move it to a
place that doesn't require openning the floodgates? If not, can we make the DEPS
as specific as possible (i.e. only that file, with a comment as to why)?

https://codereview.chromium.org/16516003/diff/76003/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/16516003/diff/76003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:76: socket_.reset(new
jingle_glue::FakeSSLClientSocket(tcp_socket.release()));
Is this production code? Why are we using a "fake" socket? Or is that class
misnamed?

[Ronghua Wu (Left Chromium), 2013-06-20 21:05:28]

https://codereview.chromium.org/16516003/diff/76003/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/16516003/diff/76003/content/DEPS#newcode58
content/DEPS:58: "+jingle/glue",
On 2013/06/20 20:53:15, piman wrote:
> So far, jingle has only been called from the renderer process. I don't think
we
> should blanket-allow jingle to be used everywhere in content, especially on
the
> browser side.
> It looks like you only need a single class from there - which even looks like
a
> test class, so I'm confused.
> If it really needs to be used in the browser, is there a way to move it to a
> place that doesn't require openning the floodgates? If not, can we make the
DEPS
> as specific as possible (i.e. only that file, with a comment as to why)?

content is already depends on libjingle, so we thought it's fine to add
jingle/glue which is just a thin layer on top of libjingle.

What about add a DEPS to content/browser/renderer_host/p2p/, where  jingle is
used.

https://codereview.chromium.org/16516003/diff/76003/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/16516003/diff/76003/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:76: socket_.reset(new
jingle_glue::FakeSSLClientSocket(tcp_socket.release()));
On 2013/06/20 20:53:15, piman wrote:
> Is this production code? Why are we using a "fake" socket? Or is that class
> misnamed?

I would say it's the name that's misleading. This is production code, we already
use it in jingle/glue/xmpp_client_socket_factory.cc.

From the fake_ssl_client_socket.h:

// This StreamSocket implementation is to be used with servers that
// accept connections on port 443 but don't really use SSL.  For
// example, the Google Talk servers do this to bypass proxies.  (The
// connection is upgraded to TLS as part of the XMPP negotiation, so
// security is preserved.)  A "fake" SSL handshake is done immediately
// after connection to fool proxies into thinking that this is a real
// SSL connection.
//
// NOTE: This StreamSocket implementation does *not* do a real SSL
// handshake nor does it do any encryption!

[piman, 2013-06-20 21:13:39]

On Thu, Jun 20, 2013 at 2:05 PM, <ronghuawu@chromium.org> wrote:

>
>
https://codereview.chromium.**org/16516003/diff/76003/**content/DEPS<https://...
> File content/DEPS (right):
>
> https://codereview.chromium.**org/16516003/diff/76003/**
>
content/DEPS#newcode58<https://codereview.chromium.org/16516003/diff/76003/content/DEPS#newcode58>
> content/DEPS:58: "+jingle/glue",
> On 2013/06/20 20:53:15, piman wrote:
>
>> So far, jingle has only been called from the renderer process. I don't
>>
> think we
>
>> should blanket-allow jingle to be used everywhere in content,
>>
> especially on the
>
>> browser side.
>> It looks like you only need a single class from there - which even
>>
> looks like a
>
>> test class, so I'm confused.
>> If it really needs to be used in the browser, is there a way to move
>>
> it to a
>
>> place that doesn't require openning the floodgates? If not, can we
>>
> make the DEPS
>
>> as specific as possible (i.e. only that file, with a comment as to
>>
> why)?
>
> content is already depends on libjingle, so we thought it's fine to add
> jingle/glue which is just a thin layer on top of libjingle.
>
> What about add a DEPS to content/browser/renderer_host/**p2p/, where
> jingle is used.


That would be preferable. According to git gs, I don't even think content/
should depend on third_party/jingle, it's not used except on the renderer
side (which has it in DEPS), and zygote for initialization (we could have a
very specific DEPS just for that).

>
>
> https://codereview.chromium.**org/16516003/diff/76003/**
>
content/browser/renderer_host/**p2p/socket_host_tcp.cc<https://codereview.chromium.org/16516003/diff/76003/content/browser/renderer_host/p2p/socket_host_tcp.cc>
> File content/browser/renderer_host/**p2p/socket_host_tcp.cc (right):
>
> https://codereview.chromium.**org/16516003/diff/76003/**
>
content/browser/renderer_host/**p2p/socket_host_tcp.cc#**newcode76<https://codereview.chromium.org/16516003/diff/76003/content/browser/renderer_host/p2p/socket_host_tcp.cc#newcode76>
> content/browser/renderer_host/**p2p/socket_host_tcp.cc:76:
> socket_.reset(new
> jingle_glue::**FakeSSLClientSocket(tcp_**socket.release()));
> On 2013/06/20 20:53:15, piman wrote:
>
>> Is this production code? Why are we using a "fake" socket? Or is that
>>
> class
>
>> misnamed?
>>
>
> I would say it's the name that's misleading. This is production code, we
> already use it in jingle/glue/xmpp_client_**socket_factory.cc.
>
> From the fake_ssl_client_socket.h:
>
> // This StreamSocket implementation is to be used with servers that
> // accept connections on port 443 but don't really use SSL.  For
> // example, the Google Talk servers do this to bypass proxies.  (The
> // connection is upgraded to TLS as part of the XMPP negotiation, so
> // security is preserved.)  A "fake" SSL handshake is done immediately
> // after connection to fool proxies into thinking that this is a real
> // SSL connection.
> //
> // NOTE: This StreamSocket implementation does *not* do a real SSL
> // handshake nor does it do any encryption!
>
>
https://codereview.chromium.**org/16516003/<https://codereview.chromium.org/1...
>

[Ronghua Wu (Left Chromium), 2013-06-20 21:21:43]

PTAL

Maybe we can move the fake_ssl_client_socket.h to net/socekt/ in a follow up cl?

[piman, 2013-06-20 21:26:04]

lgtm

[commit-bot: I haz the power, 2013-06-21 04:26:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/16516003/136003

[juberti, 2013-06-21 04:35:09]

lgtm

Has this been tested against a real SSLTCP server?

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

Change committed as 207810

[Ronghua Wu (Left Chromium), 2013-06-21 18:58:44]

Thought the wrapper is already used in other code, so I didn't test with a
real ssltcp server. I did it this morning by creating a ssl tcp socket and
connect to talk.google.com:443. I was able to see the ssl client/server
hello message got exchanged. And the connection was able to established.
I'm not sure how to test the stun ssltcp case. Config our turn server to
listen on 443?  Is there a better way to verify this?


On Thu, Jun 20, 2013 at 9:35 PM, <juberti@google.com> wrote:

> lgtm
>
> Has this been tested against a real SSLTCP server?
>
>
https://codereview.chromium.**org/16516003/<https://codereview.chromium.org/1...
>