[DevTools] Upgrade Security Origin Panel to MD [chromium/src : master]
Sarah Chan (Gerrit)
Hey Eric,
Can you PTAL? This should be the final CL for updating the security panel to MD.
Thanks!
Sarah
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Eric Lawrence (Gerrit)
This looks good to me in general. In the screenshot, the "View Requests..." and "Open full certificate details" are both buttons.
On Windows today, the first is a link and the latter is a button; does something in the stylesheet that I overlooked change them both to buttons for Windows too?
Patch set 5:Code-Review +1
3 comments:
<#document-fragment >
<STYLE type=text/css >
</STYLE>
<STYLE type=text/css >
</STYLE>
<STYLE type=text/css >
</STYLE>
<CONTENT >
</CONTENT>
</#document-fragment>I don't understand where this content is coming from.
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:
Patch Set #5, Line 46:
asyncFor my own education: Why do we use "async e" here but only "e" in the similar function createCertificateViewerButtonForCert below?
Patch Set #5, Line 881:
// TODO(lgarron): Fix the issue and then remove this section. See comment in SecurityPanel._processRequest().Can you either remove this line (since I seriously doubt we're ever going to make a change here) or update the owner to someone else?
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Sarah Chan (Gerrit)
Patch Set 5: Code-Review+1
(3 comments)
This looks good to me in general. In the screenshot, the "View Requests..." and "Open full certificate details" are both buttons.
On Windows today, the first is a link and the latter is a button; does something in the stylesheet that I overlooked change them both to buttons for Windows too?
Thanks Eric! No, the CSS doesn't change them both to buttons. I modified SecurityPanel.js so that it will display a button instead of a link.
3 comments:
<#document-fragment >
<STYLE type=text/css >
</STYLE>
<STYLE type=text/css >
</STYLE>
<STYLE type=text/css >
</STYLE>
<CONTENT >
</CONTENT>
</#document-fragment>I don't understand where this content is coming from.
Yes, this is a bit weird but it's part of the text button.
The same thing shows up for the "Open full certificate details" button https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/devtools/security/security-details-updated-with-security-state-expected.txt?rcl=410c205dcccaaeca0070d25b500d23bbccf7da77&l=152
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:
Patch Set #5, Line 46:
async
For my own education: Why do we use "async e" here but only "e" in the similar function createCertif […]
I suspect it's because of this:
var names = await SDK.multitargetNetworkManager.getCertificate(origin)
createCertificateViewerButtonForCert receives the name in the param, while createCertificateViewerButtonForOrigin needs to retrieve the name from the originNow that you mentioned it, I don't think the async for originNetworkButton is necessary. I removed it
Patch Set #5, Line 881:
noteSection.createChild('div').textContent =Can you either remove this line (since I seriously doubt we're ever going to make a change here) or […]
Done
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Eric Lawrence (Gerrit)
Thanks!
2 comments:
<#document-fragment >
<STYLE type=text/css >
</STYLE>
<STYLE type=text/css >
</STYLE>
<STYLE type=text/css >
</STYLE>
<CONTENT >
</CONTENT>
</#document-fragment>
Yes, this is a bit weird but it's part of the text button. […]
interesting, thanks.
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:
Patch Set #5, Line 46:
async
I suspect it's because of this: […]
Oh, interesting. I haven't looked at the async/await stuff in JavaScript before but I do recall in C# that you have to have async if you want to use await in the body.
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Sarah Chan (Gerrit)
Sarah Chan would like Dmitry Gozman to review this change.
[DevTools] Upgrade Security Origin Panel to MD
Update the DevTools Security Origin Page to MD.
Screenshot: https://screenshot.googleplex.com/jVEM0iFFMkj
Bug:617311
Change-Id: Ibc73636e7b809952a75efd97353805e7e72f689c
---
M third_party/WebKit/LayoutTests/http/tests/devtools/security/origin-view-then-interstitial-expected.txt
M third_party/WebKit/LayoutTests/http/tests/devtools/security/security-details-updated-with-security-state-expected.txt
M third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
M third_party/WebKit/Source/devtools/front_end/security/originView.css
4 files changed, 91 insertions(+), 27 deletions(-)
Sarah Chan (Gerrit)
Hello Dmitry,
Can you PTAL? This should be my final CL for upgrading the Security panel to MD.
Thanks!
Sarah
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Dmitry Gozman (Gerrit)
Patch set 6:Code-Review +1
1 comment:
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:
Patch Set #6, Line 773:
'Origin'ls`Origin` for future localization.
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Sarah Chan (Gerrit)
1 comment:
Patch Set #6, Line 773:
'Origin'ls`Origin` for future localization.
I don't think so. What is the process for localization in DevTools?
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Dmitry Gozman (Gerrit)
1 comment:
Patch Set #6, Line 773:
'Origin'
I don't think so. […]
There is no localization yet, but we mark all UI strings with ls`my string` (previously Common.UIString) for future possibility.
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
1 comment:
Patch Set #6, Line 773:
'Origin'I don't think so. […]
You now need to template all user readable strings as ls`Origin` in DevTools. Previously we were using UIString() for that.
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Sarah Chan (Gerrit)
1 comment:
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:
Patch Set #6, Line 773:
Common.UYou now need to template all user readable strings as ls`Origin` in DevTools. […]
Thanks for the clarification! I just added Common.UIString and changed it to `Origin`. Can you let me know if I'm doing this correctly?
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Eric Lawrence (Gerrit)
Patch Set 8:
(1 comment)
I think the suggestion is that Common.UIString("foo") is now deprecated and the new thing to do is
ls`foo`
Sarah Chan (Gerrit)
Patch Set 8:
Patch Set 8:
(1 comment)
I think the suggestion is that Common.UIString("foo") is now deprecated and the new thing to do is
ls`foo`
Thanks Eric!
1 comment:
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:
Patch Set #6, Line 773:
ls`OrigiThanks for the clarification! I just added Common.UIString and changed it to `Origin`. […]
Done
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Sarah Chan (Gerrit)
Patch set 9:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix the string" https://chromium-review.googlesource.com/c/824952/9
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/824952/9
Bot data: {"action": "start", "triggered_at": "2017-12-19T03:27:19.0Z", "cq_cfg_revision": "449ceedce2207329148e0a132f0c79a46cde3381", "revision": "4becd24963dcb063f0773380fc8a449082cd94fd"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux-chromeos-rel on master.tryserver.chromium.chromiumos (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.chromiumos/builders/linux-chromeos-rel/builds/26306)
Sarah Chan (Gerrit)
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix the string" https://chromium-review.googlesource.com/c/824952/9
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/824952/9
Bot data: {"action": "start", "triggered_at": "2017-12-19T04:50:17.0Z", "cq_cfg_revision": "449ceedce2207329148e0a132f0c79a46cde3381", "revision": "4becd24963dcb063f0773380fc8a449082cd94fd"}
To view, visit change 824952. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
[DevTools] Upgrade Security Origin Panel to MD
Update the DevTools Security Origin Page to MD.
Screenshot: https://screenshot.googleplex.com/jVEM0iFFMkj
Bug: 617311
Change-Id: Ibc73636e7b809952a75efd97353805e7e72f689c
Reviewed-on: https://chromium-review.googlesource.com/824952
Commit-Queue: Sarah Chan <spq...@chromium.org>
Reviewed-by: Dmitry Gozman <dgo...@chromium.org>
Reviewed-by: Eric Lawrence <elaw...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524958}
---
M third_party/WebKit/LayoutTests/http/tests/devtools/security/origin-view-then-interstitial-expected.txt
M third_party/WebKit/LayoutTests/http/tests/devtools/security/security-details-updated-with-security-state-expected.txt
M third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
M third_party/WebKit/Source/devtools/front_end/security/originView.css
4 files changed, 91 insertions(+), 27 deletions(-)