Use file_util::GetFileSystemType() in crypto/nss_util.cc.

Use file_util::GetFileSystemType() in crypto/nss_util.cc.

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

[wtc, 2014-02-04 01:26:02]

Patch set 2 LGTM. Thanks.

Please omit the BaseTimeToPRTime change from this CL.

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:16: #if defined(OS_OPENBSD)

I would just have OS_OPENBSD use file_util::GetFileSystemType also. If people
are still porting Chromium to OpenBSD, they can implement
file_util::GetFileSystemType for OpenBSD.

But your change is fine by me, too.

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:916: #endif  // defined(ARCH_CPU_X86_64)

Nit: I usually omit this kind of comment when the ifdef'd section is very short.

If you add this comment, you should also add a comment to the #endif on line 913
for consistency.

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.h#newcod...
crypto/nss_util.h:151: CRYPTO_EXPORT int64 BaseTimeToPRTime(const base::Time&
time);

Are you sure this change is necessary? base::Time can be passed by value very
cheaply, and base/time/time.h itself passes Time, TimeTicks, and TimeDelta by
value.

[Lei Zhang, 2014-02-04 01:34:21]

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:16: #if defined(OS_OPENBSD)
On 2014/02/04 01:26:02, wtc wrote:
> 
> I would just have OS_OPENBSD use file_util::GetFileSystemType also. If people
> are still porting Chromium to OpenBSD, they can implement
> file_util::GetFileSystemType for OpenBSD.
> 
> But your change is fine by me, too.

GetFileSystemType is Linux-only right now. I don't know who is actively working
on OpenBSD support, so I'm going to leave it alone.

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:916: #endif  // defined(ARCH_CPU_X86_64)
On 2014/02/04 01:26:02, wtc wrote:
> 
> Nit: I usually omit this kind of comment when the ifdef'd section is very
short.
> 
> If you add this comment, you should also add a comment to the #endif on line
913
> for consistency.

I added it to line 913 as well. Generally I omit it too, but when there's nested
#if's, it's hard to make out which #endif corresponds to which #if.

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/147933003/diff/20001/crypto/nss_util.h#newcod...
crypto/nss_util.h:151: CRYPTO_EXPORT int64 BaseTimeToPRTime(const base::Time&
time);
On 2014/02/04 01:26:02, wtc wrote:
> 
> Are you sure this change is necessary? base::Time can be passed by value very
> cheaply, and base/time/time.h itself passes Time, TimeTicks, and TimeDelta by
> value.

It just stood out since we generally pass by const-ref. Reverted.

[Lei Zhang, 2014-02-04 01:34:29]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-04 01:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/147933003/70001

[wtc, 2014-02-04 01:40:43]

Patch set 3 has a typo.

https://codereview.chromium.org/147933003/diff/70001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/147933003/diff/70001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:913: #endif  // defined(ARCH_CPU_ARMEL)

This comment should say "// defined(__ARM_PCS_VFP)".

[Lei Zhang, 2014-02-04 01:41:00]

The CQ bit was unchecked by thestig@chromium.org

[Lei Zhang, 2014-02-04 01:42:17]

On 2014/02/04 01:40:43, wtc wrote:
> Patch set 3 has a typo.
> 
> https://codereview.chromium.org/147933003/diff/70001/crypto/nss_util.cc
> File crypto/nss_util.cc (right):
> 
>
https://codereview.chromium.org/147933003/diff/70001/crypto/nss_util.cc#newco...
> crypto/nss_util.cc:913: #endif  // defined(ARCH_CPU_ARMEL)
> 
> This comment should say "// defined(__ARM_PCS_VFP)".

Done. Matching #if's is hard.

[Lei Zhang, 2014-02-04 01:42:20]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-04 01:43:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/147933003/180001

[commit-bot: I haz the power, 2014-02-04 02:44:03]

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

[commit-bot: I haz the power, 2014-02-04 02:44:04]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Lei Zhang, 2014-02-04 02:44:55]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-04 02:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/147933003/180001

[commit-bot: I haz the power, 2014-02-04 04:31:33]

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

[commit-bot: I haz the power, 2014-02-04 04:31:33]

Retried try job too often on ios_dbg_simulator for step(s) url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[Lei Zhang, 2014-02-04 05:55:10]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-04 05:55:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/147933003/180001

[commit-bot: I haz the power, 2014-02-04 12:27:51]

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

[commit-bot: I haz the power, 2014-02-04 12:27:52]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Lei Zhang, 2014-02-05 06:37:33]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-05 06:43:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/147933003/180001

[commit-bot: I haz the power, 2014-02-05 08:37:51]

Change committed as 248923