Change Origin bound certs -> Domain bound certs.

Change Origin bound certs -> Domain bound certs.

BUG=115348
TEST=unit tests, manually checked 'Origin Bound Certs' contents after browsing
TBR=jam@chromium.org,willchan@chromium.org

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

[mattm, 2012-03-07 03:53:05]

This changes origin bound certs to be domain bound instead.  The actual changes
are small (a few lines in OriginBoundCertService::GetOriginBoundCert).  The rest
is renaming some variables to make things clearer, updating tests, and renaming
all the BrowsingDataRemover stuff.

All the origin_bound_* and OriginBound* names were left as-is since otherwise
this would be a massive cl.  I did rename all the browsing data related stuff
since that seemed cleaner to me, but I'm open to suggestions either way as to
where to draw the renaming line.

I also left the command line flag and option as-is, to avoid people needing to
update their scripts / enterprise policies / etc.

wtc: net stuff
mkwst: browsing_data stuff

rch: fyi

[Ryan Hamilton, 2012-03-07 04:46:06]

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.h:106: static int GenerateCert(const
std::string& domain,
Does this include the host and port for the domain?

[Mike West, 2012-03-07 10:31:47]

I think this needs to be a much bigger CL.

I'd be fine with you splitting renaming up into a separate CL, but calling
something an "origin bound" certificate while treating it as a "domain bound"
certificate is confusing. If you don't rename the services and stores and etc, I
think it will slowly drive people insane. :)

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.cc:723: net::OriginBoundCertService*
origin_bound_cert_service =
domain_bound_cert_service?

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:199: class
RemoveOriginBoundCertTester : public BrowsingDataRemoverTester {
RemoveDomainBoundCertTester

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:204:
origin_bound_cert_service();
Is this still called "origin_bound_cert_service"? If so, can you change it? :)

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:208: return
ob_cert_service_->cert_count();
Nit: db_cert_service

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:211: // Add an domain bound
cert for |origin| with specific creation and expiry
Nit: fix the variable name here and in the function definition.

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:221: // Add an origin bound
cert for |origin|, with the current time as the
Nit: fix the comment here, and the variable name below.

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:230: net::OriginBoundCertStore*
GetCertStore() {
Same naming question.

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:235:
net::OriginBoundCertService* ob_cert_service_;
Same naming question.

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:514:
TEST_F(BrowsingDataRemoverTest, RemoveOriginBoundCertForever) {
RemoveDomainBoundCertForever

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:528:
TEST_F(BrowsingDataRemoverTest, RemoveOriginBoundCertLastHour) {
RemoveDomainBoundCertLastHour

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/net/sqlite_or...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/9617039/diff/5001/chrome/browser/net/sqlite_or...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:422: NOTREACHED() << "Could
not delete an origin bound cert from the DB.";
"domain-bound cert"?

Honestly, this is confusing. :) `sqlite_origin_bound_cert_store` seems like it
should include origin-bound certificates. Not domain-bound certificates. If
you're changing the behavior, I think you need to change the names here too.

http://codereview.chromium.org/9617039/diff/5001/net/base/default_origin_boun...
File net/base/default_origin_bound_cert_store.h (right):

http://codereview.chromium.org/9617039/diff/5001/net/base/default_origin_boun...
net/base/default_origin_bound_cert_store.h:34: // DefaultOriginBoundCertStore,
but it's really a DefaultDomainBoundCertStore.
Are the historical reasons strong enough to not simply rename the class
everywhere?

[wtc, 2012-03-08 01:58:45]

rch: can you review this CL?  I may not be able to review
it until tomorrow (Thursday) afternoon.  Thanks...

[wtc, 2012-03-08 02:13:45]

Review comments on Patch Set 2:

I reviewed all the changes in src/net.  They look good to me.

After seeing the code, I agree that not renaming completely
may drive the reader of this code crazy.  We should rename
completely soon.

I wonder if some of the classes should have a neutral name,
so we can use them for both 'origin' and 'domain'.  I am
worried that origin-bound certs may come back in the future.

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.cc:324: std::string domain =
GetDomainForHost(GURL(origin).host());

Should we also check domain.empty()?  See the origin.empty()
check at line 319 above.

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:20: // certificates are specification is

Typo: are specification is

remove "are", or say "are specified in".

[Ryan Hamilton, 2012-03-08 20:04:10]

On 2012/03/08 01:58:45, wtc wrote:
> rch: can you review this CL?  I may not be able to review
> it until tomorrow (Thursday) afternoon.  Thanks...

Looks like you completed your review.  I actually made a comment on Tuesday
asking if "domain" was the host:port or just the etld+1 in the methods which
take std::string domain.  I wonder if that parameter name is potentially
perilous.

[wtc, 2012-03-09 20:11:24]

rch: yes, I managed to complete the review on Wednesday.

mattm, rch: regarding my suggestion of using a neutral name
for classes that don't care whether the certs are origin or
domain-bound, I have some suggestions:
  server-bound cert
  channel-binding cert
  cookie-binding cert

Do you like any of these?  Or should we just use "domain-bound cert"
until origin-bound certs come back to haunt us?

[mattm, 2012-03-15 01:48:44]

Ok here's an attempt at renaming everything.  I followed wtc's suggested of
renaming the things that don't care to ServerBoundFoo.

I didn't do the file renames since I couldn't get rietveld to show the diffs on
all of them properly.  I'll do that in a separate CL.

Like before, I left the command line flag and policy name the same for
compatability.  I also didn't change the sqlite filename, I can do that in a
separate CL too if you want.

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.cc:324: std::string domain =
GetDomainForHost(GURL(origin).host());
On 2012/03/08 02:13:45, wtc wrote:
> 
> Should we also check domain.empty()?  See the origin.empty()
> check at line 319 above.

Done.

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.h:106: static int GenerateCert(const
std::string& domain,
On 2012/03/07 04:46:06, Ryan Hamilton wrote:
> Does this include the host and port for the domain?

just the host.

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/9617039/diff/5001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:20: // certificates are specification is
On 2012/03/08 02:13:45, wtc wrote:
> 
> Typo: are specification is
> 
> remove "are", or say "are specified in".

Done.

[wtc, 2012-03-15 23:46:38]

Patch set 4 LGTM.

Why did you need to change the zip files?

I have some questions and suggested changes.

http://codereview.chromium.org/9617039/diff/13052/chrome/browser/net/sqlite_o...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/9617039/diff/13052/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:178: "creation_time FROM
origin_bound_certs"));

Somewhere in this file, please document why the SQL statement
uses the table name 'origin_bound_certs'.

http://codereview.chromium.org/9617039/diff/13052/chrome/common/extensions/do...
File chrome/common/extensions/docs/samples.json (right):

http://codereview.chromium.org/9617039/diff/13052/chrome/common/extensions/do...
chrome/common/extensions/docs/samples.json:527: "source_hash":
"6e227746c25a1b9765dbd27e3eb0e7403dee2c65",

Why is this change necessary?

http://codereview.chromium.org/9617039/diff/13052/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/net_error_list.h#n...
net/base/net_error_list.h:280:
NET_ERROR(SERVER_BOUND_CERT_GENERATION_TYPE_MISMATCH, -152)

We probably should not rename these error codes because they
may have shown up in bug reports.

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:27: // A class for creating and fetching
domain bound certs.

Should this be "server bound certs"?

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:80: // GetServerBoundCert(). After a
request is canceled, its completion

Typo: GetServerBoundCert => GetDomainBoundCert ?

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:103: // with a LazyInstance, which is not
allowed on a worker thread.

I suggest renaming 'server' to 'server_name_key' or
'server_identifier' or something like that to imply that
it could be a string derived from the server name.

At a minimum, please document the 'server' argument.

http://codereview.chromium.org/9617039/diff/13052/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/ssl_config_service...
net/base/ssl_config_service.h:73: bool server_bound_certs_enabled;  // True if
TLS origin bound cert extension

This probably should not be renamed, or should be renamed
domain_bound_certs_enabled.

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util.h
File net/base/x509_util.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util.h#newcode23
net/base/x509_util.h:23: // Domain, serial number and validity period are given
as

"Domain" should match the new name of the second parameter.

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util.h#newcode29
net/base/x509_util.h:29: bool NET_EXPORT_PRIVATE
CreateServerBoundCertEC(crypto::ECPrivateKey* key,

This function should be named either OriginBound or
DomainBound.

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_nss_unit...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_nss_unit...
net/base/x509_util_nss_unittest.cc:77: void VerifyServerBoundCert(const
std::string& server,

I believe in this file we should be referring to domain
bound certs.

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_openssl.cc
File net/base/x509_util_openssl.cc (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_openssl....
net/base/x509_util_openssl.cc:18: bool CreateServerBoundCertEC(

This should be DomainBound.

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socke...
net/socket/ssl_client_socket.h:152: // an domain bound certificate.

Nit: an domain => a domain

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.h:102: STATE_GET_DB_CERT_COMPLETE,

Nit: let's spell out DOMAIN_BOUND because "DB" looks like
the abbreviation for "database".

http://codereview.chromium.org/9617039/diff/13052/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/9617039/diff/13052/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:66: ServerBoundCertService*
sbc_service,

Let's rename sbc_service to server_bound_cert_service.

[mattm, 2012-03-16 22:22:00]

On 2012/03/15 23:46:38, wtc wrote:
> Why did you need to change the zip files?

That's from running chrome/common/extensions/docs/build/build.py to update the
extension API docs.

http://codereview.chromium.org/9617039/diff/13052/chrome/browser/net/sqlite_o...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/9617039/diff/13052/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:178: "creation_time FROM
origin_bound_certs"));
On 2012/03/15 23:46:38, wtc wrote:
> 
> Somewhere in this file, please document why the SQL statement
> uses the table name 'origin_bound_certs'.

Done.

http://codereview.chromium.org/9617039/diff/13052/chrome/common/extensions/do...
File chrome/common/extensions/docs/samples.json (right):

http://codereview.chromium.org/9617039/diff/13052/chrome/common/extensions/do...
chrome/common/extensions/docs/samples.json:527: "source_hash":
"6e227746c25a1b9765dbd27e3eb0e7403dee2c65",
On 2012/03/15 23:46:38, wtc wrote:
> 
> Why is this change necessary?

Since the CL changes the extension interface, I have to run
chrome/common/extensions/docs/build/build.py to update the extension API docs.

http://codereview.chromium.org/9617039/diff/13052/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/net_error_list.h#n...
net/base/net_error_list.h:280:
NET_ERROR(SERVER_BOUND_CERT_GENERATION_TYPE_MISMATCH, -152)
On 2012/03/15 23:46:38, wtc wrote:
> 
> We probably should not rename these error codes because they
> may have shown up in bug reports.

Done.

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:27: // A class for creating and fetching
domain bound certs.
On 2012/03/15 23:46:38, wtc wrote:
> 
> Should this be "server bound certs"?

Done.

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:80: // GetServerBoundCert(). After a
request is canceled, its completion
On 2012/03/15 23:46:38, wtc wrote:
> 
> Typo: GetServerBoundCert => GetDomainBoundCert ?

Done.

http://codereview.chromium.org/9617039/diff/13052/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:103: // with a LazyInstance, which is not
allowed on a worker thread.
On 2012/03/15 23:46:38, wtc wrote:
> 
> I suggest renaming 'server' to 'server_name_key' or
> 'server_identifier' or something like that to imply that
> it could be a string derived from the server name.
> 
> At a minimum, please document the 'server' argument.

Done.

http://codereview.chromium.org/9617039/diff/13052/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/ssl_config_service...
net/base/ssl_config_service.h:73: bool server_bound_certs_enabled;  // True if
TLS origin bound cert extension
On 2012/03/15 23:46:38, wtc wrote:
> 
> This probably should not be renamed, or should be renamed
> domain_bound_certs_enabled.

done (renamed to domain_bound_certs_enabled)

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util.h
File net/base/x509_util.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util.h#newcode23
net/base/x509_util.h:23: // Domain, serial number and validity period are given
as
On 2012/03/15 23:46:38, wtc wrote:
> 
> "Domain" should match the new name of the second parameter.

Done.

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util.h#newcode29
net/base/x509_util.h:29: bool NET_EXPORT_PRIVATE
CreateServerBoundCertEC(crypto::ECPrivateKey* key,
On 2012/03/15 23:46:38, wtc wrote:
> 
> This function should be named either OriginBound or
> DomainBound.

Done.

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_nss_unit...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_nss_unit...
net/base/x509_util_nss_unittest.cc:77: void VerifyServerBoundCert(const
std::string& server,
On 2012/03/15 23:46:38, wtc wrote:
> 
> I believe in this file we should be referring to domain
> bound certs.

Done.

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_openssl.cc
File net/base/x509_util_openssl.cc (right):

http://codereview.chromium.org/9617039/diff/13052/net/base/x509_util_openssl....
net/base/x509_util_openssl.cc:18: bool CreateServerBoundCertEC(
On 2012/03/15 23:46:38, wtc wrote:
> 
> This should be DomainBound.

Done.

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socke...
net/socket/ssl_client_socket.h:152: // an domain bound certificate.
On 2012/03/15 23:46:38, wtc wrote:
> 
> Nit: an domain => a domain

Done.

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/9617039/diff/13052/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.h:102: STATE_GET_DB_CERT_COMPLETE,
On 2012/03/15 23:46:38, wtc wrote:
> 
> Nit: let's spell out DOMAIN_BOUND because "DB" looks like
> the abbreviation for "database".

Done.

http://codereview.chromium.org/9617039/diff/13052/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/9617039/diff/13052/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:66: ServerBoundCertService*
sbc_service,
On 2012/03/15 23:46:38, wtc wrote:
> 
> Let's rename sbc_service to server_bound_cert_service.

Done.

[mattm, 2012-03-16 22:33:11]

Adding OWNERS reviewers.

darin: content/public, content, webkit
willchan: chrome/browser/profiles
mnissler: chrome/app/policy
mihaip: chrome/common/extensions/api

[Mattias Nissler (ping if slow), 2012-03-17 10:46:36]

chrome/app/policy LGTM

[wtc, 2012-03-19 22:59:36]

Patch set 6 LGTM.

http://codereview.chromium.org/9617039/diff/27001/net/base/x509_util_nss_unit...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/9617039/diff/27001/net/base/x509_util_nss_unit...
net/base/x509_util_nss_unittest.cc:77: void VerifyServerBoundCert(const
std::string& domain,

This function should be named VerifyDomainBoundCert.

[Mihai Parparita -not on Chrome, 2012-03-19 23:15:43]

LGTM

http://codereview.chromium.org/9617039/diff/27001/chrome/common/extensions/ap...
File chrome/common/extensions/api/browsingData.json (right):

http://codereview.chromium.org/9617039/diff/27001/chrome/common/extensions/ap...
chrome/common/extensions/api/browsingData.json:82: "serverBoundCertificates": {
FYI: This is technically not a backwards-compatible API change (we generally
don't remove methods once they're out of experimental), but since
originBoundCertificates is only on the dev channel, it should be safe to do.

[wtc, 2012-03-19 23:24:10]

http://codereview.chromium.org/9617039/diff/27001/chrome/common/extensions/ap...
File chrome/common/extensions/api/browsingData.json (right):

http://codereview.chromium.org/9617039/diff/27001/chrome/common/extensions/ap...
chrome/common/extensions/api/browsingData.json:82: "serverBoundCertificates": {

On 2012/03/19 23:15:43, Mihai Parparita wrote:
> FYI: This is technically not a backwards-compatible API change (we generally
> don't remove methods once they're out of experimental), but since
> originBoundCertificates is only on the dev channel, it should be safe to do.

Hmm... thanks for pointing this out.

We can consider keeping the old name and just change the
description to say "domain-bound certificates" or
"orgin-bound and domain-bound certificates".

[Mihai Parparita -not on Chrome, 2012-03-19 23:26:32]

On 2012/03/19 23:24:10, wtc wrote:
> Hmm... thanks for pointing this out.
> 
> We can consider keeping the old name and just change the
> description to say "domain-bound certificates" or
> "orgin-bound and domain-bound certificates".

I don't think it's worth making the API more complex for the sake of theoretical
backwards compatibility. However, Mike is probably a better judge of how (if
any) people are using this API.

[Mike West, 2012-03-20 11:31:41]

On 2012/03/19 23:26:32, Mihai Parparita wrote:
> On 2012/03/19 23:24:10, wtc wrote:
> > Hmm... thanks for pointing this out.
> > 
> > We can consider keeping the old name and just change the
> > description to say "domain-bound certificates" or
> > "orgin-bound and domain-bound certificates".
> 
> I don't think it's worth making the API more complex for the sake of
theoretical
> backwards compatibility. However, Mike is probably a better judge of how (if
> any) people are using this API.

I think I can count the number of developers already using this API on one hand.
:) Changing to `serverBoundCertificates` is fine. As long as this lands before
the API hits stable, I don't have any concerns about changing the name.

BrowsingDataRemover LGTM.

[darin (slow to review), 2012-03-20 19:34:56]

LGTM for webkit/ and content/shell.  You should just TBR=jam@chromium.org for
the content/public change.  It does not require him to review it before you
commit.