Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
280043003:
Clean up the JavaScript for net-internals/#hsts. (Closed)
|
Created:
6 years, 7 months ago by palmer Modified:
6 years, 7 months ago Reviewers:
eroman CC:
chromium-reviews, eroman, arv+watch_chromium.org, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClean up the JavaScript for net-internals/#hsts.
BUG=371108, 359845
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270018
Patch Set 1 #Patch Set 2 : Rebase, hoping the bots stop acting like goats. #
Total comments: 6
Patch Set 3 : Further clean-up thanks to eroman. #
Total comments: 4
Patch Set 4 : Last bit of cleanup and polish. #Messages
Total messages: 12 (0 generated)
Here's that clean-up you asked for. Thanks — it was fun! I love deleting code.
lgtm https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... File chrome/browser/resources/net_internals/hsts_view.js (right): https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:133: result.public_key_hashes = ''; Modifying the "result" object for display is bad form. See the recommendation below about getting rid of this all-together. https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:139: var staticHashes = []; While you are cleaning up, this can be merged with the code right above (i.e. the "if (typeof result.public_key_hashes = 'undefined')"...). How about: var kStaticHashKeys = ['public_key_hashes', 'preloaded_spki_hashes', 'static_spki_hashes']; var staticHashes = []; for (var i = 0; i < kStaticHashKeys.length; ++i) { var staticHashValue = result[kStaticHashKeys[i]]; if (staticHashValue != undefined) staticHashes.push(staticHashValue); } https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:164: addNodeWithText(this.queryOutputDiv_, 'tt', result[key] || ''); Be aware that this pattern (a || '') will cause a value of 0 to be masked by ''. Probably OK but your use case, but just wanted to make sure you are aware of this. If not, you may consider: var value = result[key]; .... (value == undefined ? '' : value) ....
Thanks! https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... File chrome/browser/resources/net_internals/hsts_view.js (right): https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:133: result.public_key_hashes = ''; On 2014/05/12 21:29:13, eroman wrote: > Modifying the "result" object for display is bad form. See the recommendation > below about getting rid of this all-together. Done. https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:139: var staticHashes = []; On 2014/05/12 21:29:13, eroman wrote: > While you are cleaning up, this can be merged with the code right above (i.e. > the "if (typeof result.public_key_hashes = 'undefined')"...). > > How about: > > var kStaticHashKeys = ['public_key_hashes', 'preloaded_spki_hashes', > 'static_spki_hashes']; > > var staticHashes = []; > for (var i = 0; i < kStaticHashKeys.length; ++i) { > var staticHashValue = result[kStaticHashKeys[i]]; > if (staticHashValue != undefined) > staticHashes.push(staticHashValue); > } Done. https://codereview.chromium.org/280043003/diff/20001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:164: addNodeWithText(this.queryOutputDiv_, 'tt', result[key] || ''); On 2014/05/12 21:29:13, eroman wrote: > Be aware that this pattern (a || '') will cause a value of 0 to be masked by ''. > Probably OK but your use case, but just wanted to make sure you are aware of > this. If not, you may consider: > > var value = result[key]; > .... (value == undefined ? '' : value) .... Done.
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/palmer@chromium.org/280043003/40001
https://codereview.chromium.org/280043003/diff/40001/chrome/browser/resources... File chrome/browser/resources/net_internals/hsts_view.js (right): https://codereview.chromium.org/280043003/diff/40001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:132: if (typeof result.public_key_hashes === 'undefined') Can you delete these three "ifs" now? https://codereview.chromium.org/280043003/diff/40001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:167: var value = result[key]; consider moving this below the definition of "var key" and using it in the other place where "result[key]" is used.
The CQ bit was unchecked by palmer@chromium.org
https://codereview.chromium.org/280043003/diff/40001/chrome/browser/resources... File chrome/browser/resources/net_internals/hsts_view.js (right): https://codereview.chromium.org/280043003/diff/40001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:132: if (typeof result.public_key_hashes === 'undefined') > Can you delete these three "ifs" now? Oh, weird, I thought I had. Done. https://codereview.chromium.org/280043003/diff/40001/chrome/browser/resources... chrome/browser/resources/net_internals/hsts_view.js:167: var value = result[key]; On 2014/05/12 22:08:21, eroman wrote: > consider moving this below the definition of "var key" and using it in the other > place where "result[key]" is used. Done.
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/palmer@chromium.org/280043003/60001
lgtm
Message was sent while issue was closed.
Change committed as 270018 |
