Add the PPAPI X509 Certificate interface and implementation.

Add the PPAPI X509 Certificate interface and implementation.

Adds the interface for accessing X509 certificate fields. Note that the interface uses a GetField(field) method for accessing various fields of the certificate and all resuls are returned as pp::Var. This greatly simplifies the implementation of the interface and process of adding/changing fields so it is probably better (at least in the short term for flash).

BUG=114626
TEST=out/Debug/ui_tests --gtest_filter=*PPAPITest.*X509Certificate*
NOTRY=true

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130654

[Ryan Sleevi, 2012-03-15 02:27:56]

Hi Raymes,

No big gripes about this - a necessary implementation, I suppose :) I see that
the use of getters like this is not unprecedented within PPAPI (eg: the URL
Request Info and URL Response Info), so these seem totally appropriate to allow
great flexibility.

I've annotated below what I believe the types should be. I'm not a fan at all of
having to enumerate specific fields for issuer and subject, as I mentioned in
person, however, it seems PPAPI doesn't support array types?

We can always revisit that for a general API later, but I'm thinking something
like Windows' CertRDNValueToStr / NSS' CERT_DecodeAVAValue.

Please let me know if there are additional files / implementation I should
review - I only reviewed this .idl file.

Thanks.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
File ppapi/api/private/ppb_flash_x509_certificate.idl (right):

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:24:
PP_FLASH_X509CERTIFICATE_ISSUER_ORGANIZATION_UNIT_NAME_ = 5,
19 - 24: UTF-8 strings (PP_VARTYPE_STRING ?)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:25:
PP_FLASH_X509CERTIFICATE_ISSUER_UNIQUE_ID_ = 6,
25: A raw byte array (PP_VARTYPE_ARRAY_BUFFER ?)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:32:
PP_FLASH_X509CERTIFICATE_PRINCIPAL_ORGANIZATION_UNIT_NAME_ = 12,
27 - 32: UTF-8 strings (PP_VARTYPE_STRING ?)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:33:
PP_FLASH_X509CERTIFICATE_PRINCIPAL_UNIQUE_ID_ = 13,
33: A raw byte array (PP_VARTYPE_ARRAY_BUFFER ?)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:35:
PP_FLASH_X509CERTIFICATE_VERSION = 14,
35: Some enum (X509v1, X509v2, X509v3)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:36:
PP_FLASH_X509CERTIFICATE_SERIAL_NUMBER = 15,
36: A raw byte array (PP_VARTYPE_ARRAY_BUFFER ?)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:37:
PP_FLASH_X509CERTIFICATE_ALGORITHM_OID = 16,
37: A string (which will surely cheese someone in the IETF mailing lists off if
they're watching this - OID haters gonna hate) - PP_VARTYPE_STRING ?

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:38:
PP_FLASH_X509CERTIFICATE_ALGORITHM_PARAMATERS = 17,
38: If polymorphic structs are allowed, I'd like to do that with the different
alg types we support (RSA, ECDSA, etc)

Otherwise, I'd suggest renaming this
PP_FLASH_X509CERTIFICATE_ALGORITHM_PARAMETERS_RAW or _DER (just to highlight
that it's the /raw/ data)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:39:
PP_FLASH_X509CERTIFICATE_VALIDITY_DATE_RANGE = 18,
39: This seems like it should be X509CERTIFICATE_VALIDITY_NOTBEFORE and
X509CERTIFICATE_VALIDITY_NOTAFTER

The return type should either be PP_Time OR a PP_VARTYPE_STRING containing the
canonical form.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:40:
PP_FLASH_X509CERTIFICATE_SUBJECT_PUBLIC_KEY_ALGORITHM_OID = 19,
40: A string (same as 37) - PP_VARTYPE_STRING ?

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:41:
PP_FLASH_X509CERTIFICATE_SUBJECT_PUBLIC_KEY = 20,
41: A raw byte array - PP_VARTYPE_ARRAY_BUFFER ?

Does Flapper not also require the parameters? (Both
SubjectPublicKeyInfo.algorithm and signatureAlgorithm are AlgorithmIdentifiers,
which are structs of OID + Parameters (raw and/or interpreted)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:42:
PP_FLASH_X509CERTIFICATE_DER = 21,
42: I'm slightly inclined to call this _RAW, similar to my comments above - but
I think whatever we choose (_RAW or _DER), all the names should match.

I'm assuming PP_VARTYPE_ARRAY_BUFFER?

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:70: [in] str_t bytes,
I don't see where str_t is defined in the existing src/ppapi/c/ files.

Perhaps "const char* data" (similar to ppb_var.h) or simply void* ? (Similar to
the PDF / pp_array_output.h code ?)

http://codereview.chromium.org/9693024/diff/8002/ppapi/cpp/private/flash_x509...
File ppapi/cpp/private/flash_x509_certificate.h (right):

http://codereview.chromium.org/9693024/diff/8002/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.h:14: class Var;
Shouldn't this be in namespace flash { } as well (pp::flash::X509Certificate) to
match the other files within ppapi/cpp/private/flash_* ?

[raymes, 2012-03-15 20:32:14]

> I've annotated below what I believe the types should be. I'm not a fan at all
of
> having to enumerate specific fields for issuer and subject, as I mentioned in
> person, however, it seems PPAPI doesn't support array types?

Correct, however there are plans to support an array type (at least in theory,
the type is there) so this could be altered to allow for that in the future.

> Please let me know if there are additional files / implementation I should
> review - I only reviewed this .idl file.

The only other file of interest for you is
http://codereview.chromium.org/9405038/diff/49001/content/browser/renderer_ho....
There's nothing really for you to review yet, but note the conversion function
which I will implement to extract the fields.

Thanks the review.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
File ppapi/api/private/ppb_flash_x509_certificate.idl (right):

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:24:
PP_FLASH_X509CERTIFICATE_ISSUER_ORGANIZATION_UNIT_NAME_ = 5,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 19 - 24: UTF-8 strings (PP_VARTYPE_STRING ?)

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:25:
PP_FLASH_X509CERTIFICATE_ISSUER_UNIQUE_ID_ = 6,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 25: A raw byte array (PP_VARTYPE_ARRAY_BUFFER ?)

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:32:
PP_FLASH_X509CERTIFICATE_PRINCIPAL_ORGANIZATION_UNIT_NAME_ = 12,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 27 - 32: UTF-8 strings (PP_VARTYPE_STRING ?)

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:33:
PP_FLASH_X509CERTIFICATE_PRINCIPAL_UNIQUE_ID_ = 13,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 33: A raw byte array (PP_VARTYPE_ARRAY_BUFFER ?)

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:35:
PP_FLASH_X509CERTIFICATE_VERSION = 14,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 35: Some enum (X509v1, X509v2, X509v3)

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:36:
PP_FLASH_X509CERTIFICATE_SERIAL_NUMBER = 15,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 36: A raw byte array (PP_VARTYPE_ARRAY_BUFFER ?)

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:37:
PP_FLASH_X509CERTIFICATE_ALGORITHM_OID = 16,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 37: A string (which will surely cheese someone in the IETF mailing lists off
if
> they're watching this - OID haters gonna hate) - PP_VARTYPE_STRING ?

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:38:
PP_FLASH_X509CERTIFICATE_ALGORITHM_PARAMATERS = 17,
AFAIK they aren't supported but there may be some clever way to do this in the
future (like a PP_Var dict?). For now I took your advice on RAW, which is what
flapper wants anyway.

On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 38: If polymorphic structs are allowed, I'd like to do that with the different
> alg types we support (RSA, ECDSA, etc)
> 
> Otherwise, I'd suggest renaming this
> PP_FLASH_X509CERTIFICATE_ALGORITHM_PARAMETERS_RAW or _DER (just to highlight
> that it's the /raw/ data)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:39:
PP_FLASH_X509CERTIFICATE_VALIDITY_DATE_RANGE = 18,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 39: This seems like it should be X509CERTIFICATE_VALIDITY_NOTBEFORE and
> X509CERTIFICATE_VALIDITY_NOTAFTER
> 
> The return type should either be PP_Time OR a PP_VARTYPE_STRING containing the
> canonical form.

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:40:
PP_FLASH_X509CERTIFICATE_SUBJECT_PUBLIC_KEY_ALGORITHM_OID = 19,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 40: A string (same as 37) - PP_VARTYPE_STRING ?

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:41:
PP_FLASH_X509CERTIFICATE_SUBJECT_PUBLIC_KEY = 20,
Nope =/
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 41: A raw byte array - PP_VARTYPE_ARRAY_BUFFER ?
> 
> Does Flapper not also require the parameters? (Both
> SubjectPublicKeyInfo.algorithm and signatureAlgorithm are
AlgorithmIdentifiers,
> which are structs of OID + Parameters (raw and/or interpreted)

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:42:
PP_FLASH_X509CERTIFICATE_DER = 21,
On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> 42: I'm slightly inclined to call this _RAW, similar to my comments above -
but
> I think whatever we choose (_RAW or _DER), all the names should match.
> 
> I'm assuming PP_VARTYPE_ARRAY_BUFFER?

Done.

http://codereview.chromium.org/9693024/diff/8002/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:70: [in] str_t bytes,
str_t is the idl type for const char*.

On 2012/03/15 02:27:56, Ryan Sleevi wrote:
> I don't see where str_t is defined in the existing src/ppapi/c/ files.
> 
> Perhaps "const char* data" (similar to ppb_var.h) or simply void* ? (Similar
to
> the PDF / pp_array_output.h code ?)

[Ryan Sleevi, 2012-03-16 00:08:40]

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
File ppapi/api/private/ppb_flash_x509_certificate.idl (right):

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_x509_certificate.idl:19: //
<code>PP_VARTYPE_STRING</code>.
This is definitely outside of my domain, so feel free to ignore, but I feel like
for consistency you should document each field individually, as is done for
ppb_url_request_info.idl / ppb_url_response_info.idl.

I can understand the argument that they're "private" and thus the public
documentation may not be as important, but it feels important from
style/consistency to follow the same documentation.

Feel free to refer to sections of RFC 5280 (similar to how URLRequestProperty
refers to RFC 2616)

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_x509_certificate.idl:39: // 0=X509v1, 1=X509v2,
2=X509v3.
Can this be a real pepper enum type?

Or is that extra overhead?

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_x509_certificate.idl:47: //
<code>PP_VARTYPE_STRING</code>.
PP_VARTYPE_STRING -> PP_VARTYPE_ARRAY_BUFFER

[raymes, 2012-03-16 17:18:25]

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
File ppapi/api/private/ppb_flash_x509_certificate.idl (right):

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_x509_certificate.idl:19: //
<code>PP_VARTYPE_STRING</code>.
I definitely agree that consistency is better =). I updated the documentation to
match the way ppb_url_request_info.idl does it. I also put a reference at the
top to RFC5280. The documentation of each field could definitely be improved,
but I don't feel like it's worth it at this stage, especially if the interface
changes.

On 2012/03/16 00:08:40, Ryan Sleevi wrote:
> This is definitely outside of my domain, so feel free to ignore, but I feel
like
> for consistency you should document each field individually, as is done for
> ppb_url_request_info.idl / ppb_url_response_info.idl.
> 
> I can understand the argument that they're "private" and thus the public
> documentation may not be as important, but it feels important from
> style/consistency to follow the same documentation.
> 
> Feel free to refer to sections of RFC 5280 (similar to how URLRequestProperty
> refers to RFC 2616)

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_x509_certificate.idl:39: // 0=X509v1, 1=X509v2,
2=X509v3.
Pepper can return an int which could be cast to an enum. I've updated the CL
with that approach. I don't feel strongly one way or the other.

On 2012/03/16 00:08:40, Ryan Sleevi wrote:
> Can this be a real pepper enum type?
> 
> Or is that extra overhead?

http://codereview.chromium.org/9693024/diff/11047/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_x509_certificate.idl:47: //
<code>PP_VARTYPE_STRING</code>.
On 2012/03/16 00:08:40, Ryan Sleevi wrote:
> PP_VARTYPE_STRING -> PP_VARTYPE_ARRAY_BUFFER

Done.

[yzshen1, 2012-03-20 06:48:01]

First part of comments.

http://codereview.chromium.org/9693024/diff/9051/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_tcp_socket.h (right):

http://codereview.chromium.org/9693024/diff/9051/content/browser/renderer_hos...
content/browser/renderer_host/pepper_tcp_socket.h:60: // Extract the certificate
field data from a |net::X509Certificate| into
- Would you please explain why we should put GetCertificateFields in
PepperTCPSocket?
- Please use third person singular for method comments. (and also line 64).

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
File ppapi/api/private/ppb_flash_x509_certificate.idl (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:38:
PP_FLASH_X509CERTIFICATE_ISSUER_ORGANIZATION_UNIT_NAME_ = 5,
Please remove the trailing underscore. (There are several enum values that you
need to do the same thing.)

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:116: enum
PPB_Flash_X509_Certificate_Version {
Please remove the underscore between X509 and Certificate, in order to be
consistent. (Same for enum values.)

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:140: * Initialize a
<code>PPB_Flash_X509Certificate</code> from the DER-encoded
nit: Initialize -> Initializes.

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:145: PP_Bool Init([in]
PP_Resource resource,
- Is it necessary to be synchronous? If anything can be done at the flash side
to make this async, we should probably do that.
- What is the reasons that we don't want to merge this with Create? (If we make
it async, then initialization makes sense to be a separate step.)
- If we would like to keep this method, it is better to be named as
Initialize().

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
File ppapi/cpp/private/flash_x509_certificate.cc (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.cc:14: template <> const char*
interface_name<PPB_Flash_X509Certificate>() {
Please use the version number explicitly.
You could take cpp/mouse_lock.cpp as an example.

Should also change has_interface / get_interface below.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.cc:27: if
(has_interface<PPB_Flash_X509Certificate>() && instance.pp_instance()) {
I think usually there is no need to check instance.pp_instance() in cpp wrapper.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
File ppapi/cpp/private/flash_x509_certificate.h (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.h:16: class X509Certificate : public
Resource {
This should be in the flash namespace.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.h:26: // Create a new certificate from
a DER-encoded representation. Returns true
Create -> Creates.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.h:29: // Return the specified field as
a |PP_Var|.
Return -> Returns.

|PP_Var| should be Var.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.cc (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.cc:133: bool
PPB_Flash_X509Certificate_Shared::ParseDER(
Why not making it a pure virtual method?

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:5: #ifndef
PPAPI_SHARED_IMPL_PRIVATE_FLASH_X509_CERTIFICATE_IMPL_H_
You missed 'PPB_' in the middle.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:23: class
PPB_X509Certificate_Fields {
It seems better to me to use base::ListValue to replace
PPB_X509Certificate_Fields. And move things like GetFieldAsPPVar into
PPB_Flash_X509Certificate_Shared.

You don't need to add IPC::ParamTraits in that case.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:25: virtual
~PPB_X509Certificate_Fields();
Will this be a base class? If not, please remove the virtual destructor.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:55:
AsPPB_Flash_X509Certificate_API() OVERRIDE;
Wrong indent.

[raymes, 2012-03-21 20:28:07]

Thanks for the initial comments =)

http://codereview.chromium.org/9693024/diff/9051/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_tcp_socket.h (right):

http://codereview.chromium.org/9693024/diff/9051/content/browser/renderer_hos...
content/browser/renderer_host/pepper_tcp_socket.h:60: // Extract the certificate
field data from a |net::X509Certificate| into
On 2012/03/20 06:48:01, yzshen1 wrote:
> - Would you please explain why we should put GetCertificateFields in
> PepperTCPSocket?

The certificates are related to the SSL socket functionality (e.g. the servers
certificate will be transmitted to the client upon an SSLHandshake). I guess the
link is a bit unclear here, because I haven't added the functionality which
allows you to get the servers certificate, or to add trusted/untrusted
certificates. That functionality is in a subsequent CL. The alternative is to
have these as free-standing functions in a separate file, but I think keeping
them here makes more sense for now.

> - Please use third person singular for method comments. (and also line 64).

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
File ppapi/api/private/ppb_flash_x509_certificate.idl (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:38:
PP_FLASH_X509CERTIFICATE_ISSUER_ORGANIZATION_UNIT_NAME_ = 5,
On 2012/03/20 06:48:01, yzshen1 wrote:
> Please remove the trailing underscore. (There are several enum values that you
> need to do the same thing.)

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:116: enum
PPB_Flash_X509_Certificate_Version {
On 2012/03/20 06:48:01, yzshen1 wrote:
> Please remove the underscore between X509 and Certificate, in order to be
> consistent. (Same for enum values.)

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_x509_certificate.idl:145: PP_Bool Init([in]
PP_Resource resource,
On 2012/03/20 06:48:01, yzshen1 wrote:
> - Is it necessary to be synchronous? If anything can be done at the flash side
> to make this async, we should probably do that.

The flash side is a sync-call from AS. I discussed with Trung and he recommended
going with the sync-call since there would be no easy way to do it with a
callback.

> - What is the reasons that we don't want to merge this with Create? (If we
make
> it async, then initialization makes sense to be a separate step.)

Since I realized this should probably be async, I decided to make it an Init,
even though it is synchronous for now. I thought there would be fewer changes to
the interface and more consistent if we introduced an async version later.

> - If we would like to keep this method, it is better to be named as
> Initialize().

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
File ppapi/cpp/private/flash_x509_certificate.cc (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.cc:14: template <> const char*
interface_name<PPB_Flash_X509Certificate>() {
On 2012/03/20 06:48:01, yzshen1 wrote:
> Please use the version number explicitly.
> You could take cpp/mouse_lock.cpp as an example.
> 
> Should also change has_interface / get_interface below.

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.cc:27: if
(has_interface<PPB_Flash_X509Certificate>() && instance.pp_instance()) {
On 2012/03/20 06:48:01, yzshen1 wrote:
> I think usually there is no need to check instance.pp_instance() in cpp
wrapper.

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
File ppapi/cpp/private/flash_x509_certificate.h (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.h:16: class X509Certificate : public
Resource {
On 2012/03/20 06:48:01, yzshen1 wrote:
> This should be in the flash namespace.

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.h:26: // Create a new certificate from
a DER-encoded representation. Returns true
On 2012/03/20 06:48:01, yzshen1 wrote:
> Create -> Creates.

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/cpp/private/flash_x509...
ppapi/cpp/private/flash_x509_certificate.h:29: // Return the specified field as
a |PP_Var|.
On 2012/03/20 06:48:01, yzshen1 wrote:
> Return -> Returns.
> 
> |PP_Var| should be Var.

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.cc (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.cc:133: bool
PPB_Flash_X509Certificate_Shared::ParseDER(
On 2012/03/20 06:48:01, yzshen1 wrote:
> Why not making it a pure virtual method?

ParseDER is only used when constructing an X509 certificate resource from the
byte array. In a subsequent CL, the TCP Socket resource can also construct an
X509 certificate resource to return the servers certificate. In this case it is
constructed from the shared implementation and there is no need to construct a
derived implementation as ParseDER will never be called. This unfinished CL
(http://codereview.chromium.org/9699100/diff/1059/ppapi/shared_impl/private/tc...)
might make it a bit more clear. Let me know if you want to discuss more or have
alternative suggestions on how it should work =)

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:5: #ifndef
PPAPI_SHARED_IMPL_PRIVATE_FLASH_X509_CERTIFICATE_IMPL_H_
On 2012/03/20 06:48:01, yzshen1 wrote:
> You missed 'PPB_' in the middle.

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:23: class
PPB_X509Certificate_Fields {
Hmm, I see your point. I think it's a tradeoff. I feel like this abstraction
makes it more obvious what this data is, especially on the browser side where we
would otherwise be constructing a raw base::ListValue without much context as to
how it should be constructed. 

Either way is fine with me, if you feel strongly I will change the code.

On 2012/03/20 06:48:01, yzshen1 wrote:
> It seems better to me to use base::ListValue to replace
> PPB_X509Certificate_Fields. And move things like GetFieldAsPPVar into
> PPB_Flash_X509Certificate_Shared.
> 
> You don't need to add IPC::ParamTraits in that case.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:25: virtual
~PPB_X509Certificate_Fields();
On 2012/03/20 06:48:01, yzshen1 wrote:
> Will this be a base class? If not, please remove the virtual destructor.

Done.

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:55:
AsPPB_Flash_X509Certificate_API() OVERRIDE;
On 2012/03/20 06:48:01, yzshen1 wrote:
> Wrong indent.

Done.

[yzshen1, 2012-03-22 23:32:40]

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h (right):

http://codereview.chromium.org/9693024/diff/9051/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:23: class
PPB_X509Certificate_Fields {
Okay. It is fine if you have thought it through.
On 2012/03/21 20:28:08, raymes wrote:
> Hmm, I see your point. I think it's a tradeoff. I feel like this abstraction
> makes it more obvious what this data is, especially on the browser side where
we
> would otherwise be constructing a raw base::ListValue without much context as
to
> how it should be constructed. 
> 
> Either way is fine with me, if you feel strongly I will change the code.
> 
> On 2012/03/20 06:48:01, yzshen1 wrote:
> > It seems better to me to use base::ListValue to replace
> > PPB_X509Certificate_Fields. And move things like GetFieldAsPPVar into
> > PPB_Flash_X509Certificate_Shared.
> > 
> > You don't need to add IPC::ParamTraits in that case.
>

http://codereview.chromium.org/9693024/diff/21001/ppapi/cpp/private/flash_x50...
File ppapi/cpp/private/flash_x509_certificate.h (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/cpp/private/flash_x50...
ppapi/cpp/private/flash_x509_certificate.h:35: } // namespace flash
two spaces before //, please.

http://codereview.chromium.org/9693024/diff/21001/ppapi/ppapi_sources.gypi
File ppapi/ppapi_sources.gypi (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/ppapi_sources.gypi#ne...
ppapi/ppapi_sources.gypi:260: 'cpp/private/flash_x509_certificate.h',
sort, please. (same for 403 and 404)

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
File ppapi/proxy/ppb_flash_x509_certificate_proxy.cc (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
ppapi/proxy/ppb_flash_x509_certificate_proxy.cc:8: #include
"ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h"
sort, please.

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
ppapi/proxy/ppb_flash_x509_certificate_proxy.cc:20:
FlashX509Certificate(ResourceObjectType type, PP_Instance instance);
If OBJECT_IS_PROXY is the only value that you will use, you could probably
remove |type|.

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
File ppapi/proxy/ppb_flash_x509_certificate_proxy.h (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:11: #include
"ppapi/c/private/ppb_flash_x509_certificate.h"
You don't need this in the .h file, right?

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:13: #include
"ppapi/proxy/ppapi_proxy_export.h"
It seems you don't need this, too.

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:21:
PPB_Flash_X509Certificate_Proxy(Dispatcher* dispatcher);
explicit, please.

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/ppb_flash_x509_...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:26: virtual bool
OnMessageReceived(const IPC::Message& msg);
OVERRIDE?

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/resource_creati...
File ppapi/proxy/resource_creation_proxy.h (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/proxy/resource_creati...
ppapi/proxy/resource_creation_proxy.h:141: virtual PP_Resource
CreateX509Certificate(PP_Instance instance) OVERRIDE;
Use the full name, please.
CreateFlashxxx or CreateX509CertificatePrivate (if you change the name).

http://codereview.chromium.org/9693024/diff/21001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:23: class
PPB_X509Certificate_Fields {
Does it need PPAPI_SHARED_EXPORT?

http://codereview.chromium.org/9693024/diff/21001/ppapi/tests/test_flash_x509...
File ppapi/tests/test_flash_x509_certificate.cc (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/tests/test_flash_x509...
ppapi/tests/test_flash_x509_certificate.cc:13: unsigned char google_der[] = {
- We usually put it in an anonymous namespace.
- use const and kGoogleDer.

http://codereview.chromium.org/9693024/diff/21001/ppapi/tests/test_flash_x509...
File ppapi/tests/test_flash_x509_certificate.h (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/tests/test_flash_x509...
ppapi/tests/test_flash_x509_certificate.h:5: #ifndef
PAPPI_TESTS_TEST_FLASH_X509_CERTIFICATE_H_
PPAPI, please.

http://codereview.chromium.org/9693024/diff/21001/ppapi/thunk/interfaces_ppb_...
File ppapi/thunk/interfaces_ppb_private.h (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/thunk/interfaces_ppb_...
ppapi/thunk/interfaces_ppb_private.h:49:
PROXIED_IFACE(PPB_Flash_X509Certificate,
I think Flash-related interfaces are not put here.
Please see InterfaceList::AddFlashInterfaces.

That being said, I think maybe you want to consider renaming this to
PPB_X509Certificate_Private.

http://codereview.chromium.org/9693024/diff/21001/ppapi/thunk/ppb_flash_x509_...
File ppapi/thunk/ppb_flash_x509_certificate_thunk.cc (right):

http://codereview.chromium.org/9693024/diff/21001/ppapi/thunk/ppb_flash_x509_...
ppapi/thunk/ppb_flash_x509_certificate_thunk.cc:8: #include
"ppapi/thunk/thunk.h"
sort, please.

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/mock_p...
File webkit/plugins/ppapi/mock_plugin_delegate.cc (right):

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/mock_p...
webkit/plugins/ppapi/mock_plugin_delegate.cc:347: return true;
I think the convention is to return failure?

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:62: class PPB_X509Certificate_Fields;
sort, please.

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:526: // For PPB_Flash_X509_Certificate
Add a period at the end, please.

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.cc (right):

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.cc:27:
::ppapi::PPB_X509Certificate_Fields* result) {
Wrong indent.

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.cc:29: if
(!plugin_delegate) {
{} is not necessary.

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.h (right):

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.h:22: static PP_Resource
CreateResource(PP_Instance instance);
wrong indent. (And the line below.)

http://codereview.chromium.org/9693024/diff/21001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.h:23:
PPB_Flash_X509Certificate_Impl(::ppapi::ResourceObjectType type,
nit: if OBJECT_IS_IMPL is the only value to use, you could omit this parameter.

[raymes, 2012-03-26 16:05:57]

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/cpp/private/f...
File ppapi/cpp/private/flash_x509_certificate.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/cpp/private/f...
ppapi/cpp/private/flash_x509_certificate.h:35: } // namespace flash
On 2012/03/22 23:32:40, yzshen1 wrote:
> two spaces before //, please.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/ppapi_sources...
File ppapi/ppapi_sources.gypi (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/ppapi_sources...
ppapi/ppapi_sources.gypi:260: 'cpp/private/flash_x509_certificate.h',
On 2012/03/22 23:32:40, yzshen1 wrote:
> sort, please. (same for 403 and 404)

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_x509_certificate_proxy.cc (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_x509_certificate_proxy.cc:8: #include
"ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h"
On 2012/03/22 23:32:40, yzshen1 wrote:
> sort, please.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_x509_certificate_proxy.cc:20:
FlashX509Certificate(ResourceObjectType type, PP_Instance instance);
On 2012/03/22 23:32:40, yzshen1 wrote:
> If OBJECT_IS_PROXY is the only value that you will use, you could probably
> remove |type|.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_x509_certificate_proxy.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:11: #include
"ppapi/c/private/ppb_flash_x509_certificate.h"
On 2012/03/22 23:32:40, yzshen1 wrote:
> You don't need this in the .h file, right?

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:13: #include
"ppapi/proxy/ppapi_proxy_export.h"
On 2012/03/22 23:32:40, yzshen1 wrote:
> It seems you don't need this, too.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:21:
PPB_Flash_X509Certificate_Proxy(Dispatcher* dispatcher);
On 2012/03/22 23:32:40, yzshen1 wrote:
> explicit, please.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_x509_certificate_proxy.h:26: virtual bool
OnMessageReceived(const IPC::Message& msg);
On 2012/03/22 23:32:40, yzshen1 wrote:
> OVERRIDE?

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/resourc...
File ppapi/proxy/resource_creation_proxy.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/proxy/resourc...
ppapi/proxy/resource_creation_proxy.h:141: virtual PP_Resource
CreateX509Certificate(PP_Instance instance) OVERRIDE;
On 2012/03/22 23:32:40, yzshen1 wrote:
> Use the full name, please.
> CreateFlashxxx or CreateX509CertificatePrivate (if you change the name).

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_flash_x509_certificate_shared.h:23: class
PPB_X509Certificate_Fields {
On 2012/03/22 23:32:40, yzshen1 wrote:
> Does it need PPAPI_SHARED_EXPORT?

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/tests/test_fl...
File ppapi/tests/test_flash_x509_certificate.cc (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/tests/test_fl...
ppapi/tests/test_flash_x509_certificate.cc:13: unsigned char google_der[] = {
On 2012/03/22 23:32:40, yzshen1 wrote:
> - We usually put it in an anonymous namespace.
> - use const and kGoogleDer.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/tests/test_fl...
File ppapi/tests/test_flash_x509_certificate.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/tests/test_fl...
ppapi/tests/test_flash_x509_certificate.h:5: #ifndef
PAPPI_TESTS_TEST_FLASH_X509_CERTIFICATE_H_
On 2012/03/22 23:32:40, yzshen1 wrote:
> PPAPI, please.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/thunk/interfa...
File ppapi/thunk/interfaces_ppb_private.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/thunk/interfa...
ppapi/thunk/interfaces_ppb_private.h:49:
PROXIED_IFACE(PPB_Flash_X509Certificate,
On 2012/03/22 23:32:40, yzshen1 wrote:
> I think Flash-related interfaces are not put here.
> Please see InterfaceList::AddFlashInterfaces.
> 
> That being said, I think maybe you want to consider renaming this to
> PPB_X509Certificate_Private.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/thunk/ppb_fla...
File ppapi/thunk/ppb_flash_x509_certificate_thunk.cc (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/ppapi/thunk/ppb_fla...
ppapi/thunk/ppb_flash_x509_certificate_thunk.cc:8: #include
"ppapi/thunk/thunk.h"
On 2012/03/22 23:32:40, yzshen1 wrote:
> sort, please.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
File webkit/plugins/ppapi/mock_plugin_delegate.cc (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
webkit/plugins/ppapi/mock_plugin_delegate.cc:347: return true;
On 2012/03/22 23:32:40, yzshen1 wrote:
> I think the convention is to return failure?

oh, whoops. Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
webkit/plugins/ppapi/plugin_delegate.h:62: class PPB_X509Certificate_Fields;
On 2012/03/22 23:32:40, yzshen1 wrote:
> sort, please.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
webkit/plugins/ppapi/plugin_delegate.h:526: // For PPB_Flash_X509_Certificate
On 2012/03/22 23:32:40, yzshen1 wrote:
> Add a period at the end, please.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.cc (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.cc:27:
::ppapi::PPB_X509Certificate_Fields* result) {
On 2012/03/22 23:32:40, yzshen1 wrote:
> Wrong indent.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.cc:29: if
(!plugin_delegate) {
On 2012/03/22 23:32:40, yzshen1 wrote:
> {} is not necessary.

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.h (right):

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.h:22: static PP_Resource
CreateResource(PP_Instance instance);
On 2012/03/22 23:32:40, yzshen1 wrote:
> wrong indent. (And the line below.)

Done.

https://chromiumcodereview.appspot.com/9693024/diff/21001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_x509_certificate_impl.h:23:
PPB_Flash_X509Certificate_Impl(::ppapi::ResourceObjectType type,
On 2012/03/22 23:32:40, yzshen1 wrote:
> nit: if OBJECT_IS_IMPL is the only value to use, you could omit this
parameter.

Done.

[yzshen1, 2012-03-26 17:51:08]

http://codereview.chromium.org/9693024/diff/35001/ppapi/cpp/private/x509_cert...
File ppapi/cpp/private/x509_certificate_private.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/cpp/private/x509_cert...
ppapi/cpp/private/x509_certificate_private.cc:53: field));
wrong indent.

http://codereview.chromium.org/9693024/diff/35001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:89:
PPB_X509Certificate_Fields* fields) :
Either move this colon to the beginning of the next line, or append the next
line at the end of this line (and adjust indent).

Same for line 81.

http://codereview.chromium.org/9693024/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:92: {
move it to the end of the previous line.

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/all_c_includes.h
File ppapi/tests/all_c_includes.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/all_c_includes....
ppapi/tests/all_c_includes.h:114: #include
"ppapi/c/private/ppb_x509_certificate_private.h"
sort, please.

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
File ppapi/tests/test_x509_certificate_private.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
ppapi/tests/test_x509_certificate_private.cc:86: } // namespace
two spaces before //

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
File ppapi/tests/test_x509_certificate_private.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
ppapi/tests/test_x509_certificate_private.h:13: class X509Certificate;
Why you need this declaration?

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/interfaces_ppb_...
File ppapi/thunk/interfaces_ppb_private.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/interfaces_ppb_...
ppapi/thunk/interfaces_ppb_private.h:51:
PROXIED_IFACE(PPB_X509Certificate_Private,
Do you need to also declare it in the PROXIED_API section?

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/ppb_x509_certif...
File ppapi/thunk/ppb_x509_certificate_private_thunk.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/ppb_x509_certif...
ppapi/thunk/ppb_x509_certificate_private_thunk.cc:17:
EnterX509CertificatePrivate;
4 space indent.

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:61: class PPB_X509Certificate_Fields;
If you sort ignoring class/struct, you should move PPB_HostResolver_Shared as
well.

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/ppb_x5...
File webkit/plugins/ppapi/ppb_x509_certificate_private_impl.cc (right):

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/ppb_x5...
webkit/plugins/ppapi/ppb_x509_certificate_private_impl.cc:25: bool
PPB_X509Certificate_Private_Impl::ParseDER(const std::vector<char>& der,
Move the parameter to a separate line.

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource_creation_impl.cc:26: #include
"webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h"
sort!

[raymes, 2012-03-26 19:31:34]

http://codereview.chromium.org/9693024/diff/35001/ppapi/cpp/private/x509_cert...
File ppapi/cpp/private/x509_certificate_private.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/cpp/private/x509_cert...
ppapi/cpp/private/x509_certificate_private.cc:53: field));
On 2012/03/26 17:51:08, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/9693024/diff/35001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:89:
PPB_X509Certificate_Fields* fields) :
On 2012/03/26 17:51:08, yzshen1 wrote:
> Either move this colon to the beginning of the next line, or append the next
> line at the end of this line (and adjust indent).
> 
> Same for line 81.

Done.

http://codereview.chromium.org/9693024/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:92: {
On 2012/03/26 17:51:08, yzshen1 wrote:
> move it to the end of the previous line.

Done.

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/all_c_includes.h
File ppapi/tests/all_c_includes.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/all_c_includes....
ppapi/tests/all_c_includes.h:114: #include
"ppapi/c/private/ppb_x509_certificate_private.h"
On 2012/03/26 17:51:08, yzshen1 wrote:
> sort, please.
sorry, the sort algorithm in my brain is broken =(
Done.

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
File ppapi/tests/test_x509_certificate_private.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
ppapi/tests/test_x509_certificate_private.cc:86: } // namespace
On 2012/03/26 17:51:08, yzshen1 wrote:
> two spaces before //

Done.

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
File ppapi/tests/test_x509_certificate_private.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/test_x509_certi...
ppapi/tests/test_x509_certificate_private.h:13: class X509Certificate;
On 2012/03/26 17:51:08, yzshen1 wrote:
> Why you need this declaration?

I guess it's stale. Done.

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/interfaces_ppb_...
File ppapi/thunk/interfaces_ppb_private.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/interfaces_ppb_...
ppapi/thunk/interfaces_ppb_private.h:51:
PROXIED_IFACE(PPB_X509Certificate_Private,
On 2012/03/26 17:51:08, yzshen1 wrote:
> Do you need to also declare it in the PROXIED_API section?

I added it, but not certain what it does?

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/ppb_x509_certif...
File ppapi/thunk/ppb_x509_certificate_private_thunk.cc (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/ppb_x509_certif...
ppapi/thunk/ppb_x509_certificate_private_thunk.cc:17:
EnterX509CertificatePrivate;
On 2012/03/26 17:51:08, yzshen1 wrote:
> 4 space indent.

Done.

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:61: class PPB_X509Certificate_Fields;
On 2012/03/26 17:51:08, yzshen1 wrote:
> If you sort ignoring class/struct, you should move PPB_HostResolver_Shared as
> well.

Done.

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/ppb_x5...
File webkit/plugins/ppapi/ppb_x509_certificate_private_impl.cc (right):

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/ppb_x5...
webkit/plugins/ppapi/ppb_x509_certificate_private_impl.cc:25: bool
PPB_X509Certificate_Private_Impl::ParseDER(const std::vector<char>& der,
On 2012/03/26 17:51:08, yzshen1 wrote:
> Move the parameter to a separate line.

Done.

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/9693024/diff/35001/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource_creation_impl.cc:26: #include
"webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h"
On 2012/03/26 17:51:08, yzshen1 wrote:
> sort!

Done.

[yzshen1, 2012-03-26 20:15:25]

lgtm

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/all_c_includes.h
File ppapi/tests/all_c_includes.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/tests/all_c_includes....
ppapi/tests/all_c_includes.h:114: #include
"ppapi/c/private/ppb_x509_certificate_private.h"
Use vim! :)
On 2012/03/26 19:31:34, raymes wrote:
> On 2012/03/26 17:51:08, yzshen1 wrote:
> > sort, please.
> sorry, the sort algorithm in my brain is broken =(
> Done.

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/interfaces_ppb_...
File ppapi/thunk/interfaces_ppb_private.h (right):

http://codereview.chromium.org/9693024/diff/35001/ppapi/thunk/interfaces_ppb_...
ppapi/thunk/interfaces_ppb_private.h:51:
PROXIED_IFACE(PPB_X509Certificate_Private,


http://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/thunk/interfaces_ppb...


On 2012/03/26 19:31:34, raymes wrote:
> On 2012/03/26 17:51:08, yzshen1 wrote:
> > Do you need to also declare it in the PROXIED_API section?
> 
> I added it, but not certain what it does?

[raymes, 2012-03-26 20:35:05]

+OWNERS

jam: content/
tony: webkit/glue
viettrungluu: webkit/plugins/ppapi

[tony, 2012-03-26 21:09:35]

webkit/glue/webkit_glue.gypi LGTM

[viettrungluu, 2012-03-27 17:10:56]

LGTM with nits.

http://codereview.chromium.org/9693024/diff/39008/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9693024/diff/39008/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.h:327: virtual bool
X509CertificateParseDER(
Nit: Please keep these in the same order as in plugin_delegate.h (namely, after
|RemoveNetworkListObserver()|). Ditto in the .cc file.

http://codereview.chromium.org/9693024/diff/39008/webkit/plugins/ppapi/ppb_x5...
File webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h (right):

http://codereview.chromium.org/9693024/diff/39008/webkit/plugins/ppapi/ppb_x5...
webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h:23:
PPB_X509Certificate_Private_Impl(PP_Instance instance);
Nit: I think constructors are supposed to go first, even before static methods.

http://codereview.chromium.org/9693024/diff/39008/webkit/plugins/ppapi/ppb_x5...
webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h:30:
DISALLOW_COPY_AND_ASSIGN(PPB_X509Certificate_Private_Impl);
DISALLOW_IMPLICIT_CONSTRUCTORS?

Also, you should probably include base/basictypes.h for this, I guess.

[raymes, 2012-03-27 17:40:22]

http://codereview.chromium.org/9693024/diff/39008/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9693024/diff/39008/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.h:327: virtual bool
X509CertificateParseDER(
On 2012/03/27 17:10:57, viettrungluu wrote:
> Nit: Please keep these in the same order as in plugin_delegate.h (namely,
after
> |RemoveNetworkListObserver()|). Ditto in the .cc file.

Done.

http://codereview.chromium.org/9693024/diff/39008/webkit/plugins/ppapi/ppb_x5...
File webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h (right):

http://codereview.chromium.org/9693024/diff/39008/webkit/plugins/ppapi/ppb_x5...
webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h:23:
PPB_X509Certificate_Private_Impl(PP_Instance instance);
On 2012/03/27 17:10:57, viettrungluu wrote:
> Nit: I think constructors are supposed to go first, even before static
methods.

Done.

http://codereview.chromium.org/9693024/diff/39008/webkit/plugins/ppapi/ppb_x5...
webkit/plugins/ppapi/ppb_x509_certificate_private_impl.h:30:
DISALLOW_COPY_AND_ASSIGN(PPB_X509Certificate_Private_Impl);
On 2012/03/27 17:10:57, viettrungluu wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS?
> 
> Also, you should probably include base/basictypes.h for this, I guess.

Done.

[jam, 2012-03-27 19:19:43]

On 2012/03/26 20:35:05, raymes wrote:
> +OWNERS
> 
> jam: content/
please send this to brettw, it's more up his alley

[raymes, 2012-03-27 19:23:46]

+brettw for content/ approval.

[brettw, 2012-03-29 18:05:01]

LGTM on content.

http://codereview.chromium.org/9693024/diff/45001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9693024/diff/45001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:778: if (der.size() == 0)
{
Don't use {} for single-line conditionals to match the rest of the file.

[commit-bot: I haz the power, 2012-03-30 04:25:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9693024/49001

[commit-bot: I haz the power, 2012-03-30 05:14:27]

Try job failure for 9693024-49001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-02 15:58:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9693024/49001

[commit-bot: I haz the power, 2012-04-02 16:53:21]

Try job failure for 9693024-49001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-02 20:07:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9693024/49001

[commit-bot: I haz the power, 2012-04-02 20:58:25]

Try job failure for 9693024-49001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-04 00:13:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9693024/72005

[commit-bot: I haz the power, 2012-04-04 04:21:13]

Try job failure for 9693024-72005 (previous was lost) (retry) on win_rel for
steps "browser_tests, unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-04 15:42:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9693024/72005

[commit-bot: I haz the power, 2012-04-04 17:05:04]

Try job failure for 9693024-72005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-04 17:11:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9693024/72005

[commit-bot: I haz the power, 2012-04-04 17:15:52]

Change committed as 130654