Convert iOS to use X509CertificateBytes.

Convert iOS to use X509CertificateBytes.

BUG=671420
Review-Url: https://codereview.chromium.org/2864133002
Cr-Commit-Position: refs/heads/master@{#472024}
Committed: https://chromium.googlesource.com/chromium/src/+/1a07e633e4bcb14569d37908e64eae0198840007

[mattm, 2017-05-09 00:11:12]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 00:19:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 01:35:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 01:35:24]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mattm, 2017-05-09 03:25:27]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 03:26:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 03:51:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 03:51:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattm, 2017-05-09 23:20:33]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 23:21:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-09 23:21:52]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 23:23:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-09 23:35:45]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 23:37:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 00:14:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 00:14:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mattm, 2017-05-10 01:07:18]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 01:08:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-10 01:08:44]

Description was changed from

==========
*WIP* Convert iOS to use X509CertificateBytes.

BUG=671420
==========

to

==========
Convert iOS to use X509CertificateBytes.

BUG=671420
==========

[mattm, 2017-05-10 01:12:10]

mattm@chromium.org changed reviewers:
+ eroman@chromium.org

[mattm, 2017-05-10 01:12:11]

https://codereview.chromium.org/2864133002/diff/140001/components/search_engi...
File components/search_engines/BUILD.gn (right):

https://codereview.chromium.org/2864133002/diff/140001/components/search_engi...
components/search_engines/BUILD.gn:48: public_deps = [
don't worry about these non-net DEPS changes, I'm gonna split them out into
separate CLs.

https://codereview.chromium.org/2864133002/diff/140001/net/cert/x509_util_ios...
File net/cert/x509_util_ios_and_mac.h (right):

https://codereview.chromium.org/2864133002/diff/140001/net/cert/x509_util_ios...
net/cert/x509_util_ios_and_mac.h:26:
CreateSecCertificateArrayForX509Certificate(X509Certificate* cert);
This is just moved from x509_util_mac since it's the same on both ios & mac.

[eroman, 2017-05-10 01:52:52]

Can you explain the BUILD changes and why things need to be changed to
public_deps?

Other than that looks good!

[mattm, 2017-05-10 01:56:04]

On 2017/05/10 01:52:52, eroman wrote:
> Can you explain the BUILD changes and why things need to be changed to
> public_deps?
> 
> Other than that looks good!

More of the issue with missing net deps causing boringssl include problems. See
descriptions of these CLs for more detail:
https://codereview.chromium.org/2872033003/
https://codereview.chromium.org/2873203002/
https://codereview.chromium.org/2872153002/

[commit-bot: I haz the power, 2017-05-10 03:12:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 03:12:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[mattm, 2017-05-11 20:01:01]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 20:01:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-11 20:06:35]

mattm@chromium.org changed reviewers:
+ eugenebut@chromium.org, thakis@chromium.org

[mattm, 2017-05-11 20:06:36]

+owners
eugenebut: ios/
thakis: chrome/browser/ui/certificate_viewer_mac.mm

[Nico, 2017-05-11 20:14:09]

my one line lgtm

https://codereview.chromium.org/2864133002/diff/160001/net/cert/x509_util_ios...
File net/cert/x509_util_ios_and_mac.h (right):

https://codereview.chromium.org/2864133002/diff/160001/net/cert/x509_util_ios...
net/cert/x509_util_ios_and_mac.h:12: #include "net/base/net_export.h"
I think chrome/ios often just uses the _mac files and having ios_and_mac files
is pretty uncommon.

[Eugene But (OOO till 7-30), 2017-05-11 20:40:34]

ios lgtm. Do you want to write tests for the new code?

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
File ios/web/net/crw_cert_verification_controller_unittest.mm (right):

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
ios/web/net/crw_cert_verification_controller_unittest.mm:45: (__bridge
NSArray*)chain.get(), kHostName);
Should this be static_cast or reinterpret_cast ?

https://codereview.chromium.org/2864133002/diff/160001/net/cert/x509_util_ios...
File net/cert/x509_util_ios_and_mac.h (right):

https://codereview.chromium.org/2864133002/diff/160001/net/cert/x509_util_ios...
net/cert/x509_util_ios_and_mac.h:12: #include "net/base/net_export.h"
On 2017/05/11 20:14:09, Nico wrote:
> I think chrome/ios often just uses the _mac files and having ios_and_mac files
> is pretty uncommon.
+1 Chrome for iOS does not have any ios_and_mac files. _mac are common.

[commit-bot: I haz the power, 2017-05-11 23:15:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 23:15:13]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-05-12 01:04:15]

On 2017/05/11 20:40:34, Eugene But wrote:
> ios lgtm. Do you want to write tests for the new code?

I believe all the code should be covered by existing tests, but maybe there
could be some value to testing the x509_util functions directly as well? Dunno
if it's worth it.

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
File ios/web/net/crw_cert_verification_controller_unittest.mm (right):

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
ios/web/net/crw_cert_verification_controller_unittest.mm:45: (__bridge
NSArray*)chain.get(), kHostName);
On 2017/05/11 20:40:34, Eugene But wrote:
> Should this be static_cast or reinterpret_cast ?

I'm not really that familiar with all this, I was just going based reading
https://developer.apple.com/library/content/documentation/CoreFoundation/Conc....
I can try changing it if you think that's better.

https://codereview.chromium.org/2864133002/diff/160001/net/cert/x509_util_ios...
File net/cert/x509_util_ios_and_mac.h (right):

https://codereview.chromium.org/2864133002/diff/160001/net/cert/x509_util_ios...
net/cert/x509_util_ios_and_mac.h:12: #include "net/base/net_export.h"
On 2017/05/11 20:40:34, Eugene But wrote:
> On 2017/05/11 20:14:09, Nico wrote:
> > I think chrome/ios often just uses the _mac files and having ios_and_mac
files
> > is pretty uncommon.
> +1 Chrome for iOS does not have any ios_and_mac files. _mac are common.

In this case we have some functions that are mac only, some that are the same
basic function but with slightly different impls, and some that are fully the
same. I could put them all in one _mac file with ifdefs but I wasn't sure if
that's cleaner or not.

[eroman, 2017-05-12 01:12:19]

lgtm

[Eugene But (OOO till 7-30), 2017-05-12 01:12:37]

> I believe all the code should be covered by existing tests, but maybe there
> could be some value to testing the x509_util functions directly as well? Dunno
> if it's worth it.

There is a lot of newly written code (f.e. in x509_util_ios_and_mac) and that
code does not have unit tests. It is worth writing unit tests, otherwise how do
you know that no one will break your code during the refactoring?

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
File ios/web/net/crw_cert_verification_controller_unittest.mm (right):

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
ios/web/net/crw_cert_verification_controller_unittest.mm:45: (__bridge
NSArray*)chain.get(), kHostName);
On 2017/05/12 01:04:15, mattm wrote:
> On 2017/05/11 20:40:34, Eugene But wrote:
> > Should this be static_cast or reinterpret_cast ?
> 
> I'm not really that familiar with all this, I was just going based reading
>
https://developer.apple.com/library/content/documentation/CoreFoundation/Conc....
> I can try changing it if you think that's better.
C-style casting is disallowed by Style Guide:
https://google.github.io/styleguide/cppguide.html#Casting

But I don't know if C++ casting will work for bridging.

[mattm, 2017-05-16 01:24:43]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 01:26:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-16 01:27:20]

On 2017/05/12 01:12:37, Eugene But wrote:
> > I believe all the code should be covered by existing tests, but maybe there
> > could be some value to testing the x509_util functions directly as well?
Dunno
> > if it's worth it.
> 
> There is a lot of newly written code (f.e. in x509_util_ios_and_mac) and that
> code does not have unit tests. It is worth writing unit tests, otherwise how
do
> you know that no one will break your code during the refactoring?

added some tests.

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
File ios/web/net/crw_cert_verification_controller_unittest.mm (right):

https://codereview.chromium.org/2864133002/diff/160001/ios/web/net/crw_cert_v...
ios/web/net/crw_cert_verification_controller_unittest.mm:45: (__bridge
NSArray*)chain.get(), kHostName);
On 2017/05/12 01:12:37, Eugene But wrote:
> On 2017/05/12 01:04:15, mattm wrote:
> > On 2017/05/11 20:40:34, Eugene But wrote:
> > > Should this be static_cast or reinterpret_cast ?
> > 
> > I'm not really that familiar with all this, I was just going based reading
> >
>
https://developer.apple.com/library/content/documentation/CoreFoundation/Conc....
> > I can try changing it if you think that's better.
> C-style casting is disallowed by Style Guide:
> https://google.github.io/styleguide/cppguide.html#Casting
> 
> But I don't know if C++ casting will work for bridging.

Ok, I guess static_cast works here since this file isn't ARC. changed.

[commit-bot: I haz the power, 2017-05-16 01:49:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 01:49:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Eugene But (OOO till 7-30), 2017-05-16 02:31:22]

Thank you for the tests! still lgtm

[mattm, 2017-05-16 04:41:39]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-05-16 04:41:39]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2864133002/#ps180001
(title: "static_cast, more unittest")

[commit-bot: I haz the power, 2017-05-16 04:42:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 05:55:53]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494909699128610,
"parent_rev": "386add8ced87b5b0c91d4f91b0f3e41be71c3d2c", "commit_rev":
"1a07e633e4bcb14569d37908e64eae0198840007"}

[commit-bot: I haz the power, 2017-05-16 05:56:04]

Description was changed from

==========
Convert iOS to use X509CertificateBytes.

BUG=671420
==========

to

==========
Convert iOS to use X509CertificateBytes.

BUG=671420

Review-Url: https://codereview.chromium.org/2864133002
Cr-Commit-Position: refs/heads/master@{#472024}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a07e633e4bcb14569d37908e64e...
==========

[commit-bot: I haz the power, 2017-05-16 05:56:05]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/1a07e633e4bcb14569d37908e64e...