Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1327593003:
DevTools: [security] open certificate viewer from devtools security overview (blink) (Closed)
|
Created:
5 years, 3 months ago by pfeldman Modified:
5 years, 3 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, yurys+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/remotes/origin/master Project:
blink Visibility:
Public. |
DescriptionDevTools: [security] open certificate viewer from devtools security overview (blink)
BUG=506468
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201676
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201763
Patch Set 1 #
Total comments: 8
Patch Set 2 : review comments addressed #Patch Set 3 : typo fixed too #
Total comments: 2
Patch Set 4 : rebaselined #
Messages
Total messages: 21 (8 generated)
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org, estark@chromium.org, lgarron@chromium.org
https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/SecurityPanel.js:371: WebInspector.targetManager.mainTarget().networkManager.showCertificateViewer(/** @type {number} */ (explanation.certificateId)); We might want to pass the this._target from the security panel here.
LGTM with nits. :-) https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/SecurityPanel.js:368: function showCertificaite(e) Nit: s/showCertificaite/showCertificate (I'd actually also prefer showCertificateViewer(), but I don't have strong reasons.) https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/SecurityPanel.js:371: WebInspector.targetManager.mainTarget().networkManager.showCertificateViewer(/** @type {number} */ (explanation.certificateId)); On 2015/09/02 at 01:34:14, pfeldman wrote: > We might want to pass the this._target from the security panel here. I would personally add showCertificate() to WebInspector.SecurityPanel, then pass `panel` into the WebInspector.SecurityMainView constructor and call: certificateAnchor.addEventListener("click", this._panel.showCertificateViewer, false); https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/security/mainView.css (right): https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/mainView.css:30: .security-certificate-id { Why not security-certificate-link?
lgtm https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/SecurityPanel.js:360: var certificateAnchor = text.createChild("div", "security-certificate-id link"); div -> a? https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/SecurityPanel.js:371: WebInspector.targetManager.mainTarget().networkManager.showCertificateViewer(/** @type {number} */ (explanation.certificateId)); On 2015/09/02 01:34:14, pfeldman wrote: > We might want to pass the this._target from the security panel here. Should not matter actually.
lgtm
https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/SecurityPanel.js:360: var certificateAnchor = text.createChild("div", "security-certificate-id link"); On 2015/09/02 19:02:41, dgozman wrote: > div -> a? Needs to be block. https://codereview.chromium.org/1327593003/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/security/SecurityPanel.js:368: function showCertificaite(e) On 2015/09/02 01:43:38, lgarron wrote: > Nit: s/showCertificaite/showCertificate > > (I'd actually also prefer showCertificateViewer(), but I don't have strong > reasons.) Done.
The CQ bit was checked by pfeldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, dgozman@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1327593003/#ps20001 (title: "review comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1327593003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327593003/20001
The CQ bit was unchecked by pfeldman@chromium.org
The CQ bit was checked by pfeldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, dgozman@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1327593003/#ps40001 (title: "typo fixed too")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1327593003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327593003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201676
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1318003004/ by caseq@chromium.org. The reason for reverting is: Broke the front_end compilation..
Message was sent while issue was closed.
https://codereview.chromium.org/1327593003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1327593003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:368: function showCertificateViewer(e) I think you need: certificateAnchor.addEventListener("click", this.showCertificateViewer, false);
Message was sent while issue was closed.
https://codereview.chromium.org/1327593003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1327593003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:368: function showCertificateViewer(e) On 2015/09/03 02:15:43, lgarron wrote: > I think you need: > > > certificateAnchor.addEventListener("click", this.showCertificateViewer, false); No, I don't. the reason is that showCertificateViewer needs to land first.
The CQ bit was checked by pfeldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, dgozman@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1327593003/#ps60001 (title: "rebaselined")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1327593003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327593003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201763 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
