Surface lock icon explanations in the DevTools Security panel.

Surface lock icon explanations in the DevTools Security panel.

BUG=501551
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197487

[lgarron, 2015-06-18 02:55:16]

lgarron@chromium.org changed reviewers:
+ dgozman@chromium.org

[lgarron, 2015-06-18 02:55:17]

dgozman@: I've tried to follow everything I learned from last CL. Would you mind
reviewing this?

See https://code.google.com/p/chromium/issues/detail?id=501551#c2 for a
screenshot of how this looks in practice.

(Note: This change depends on https://codereview.chromium.org/1195533002 but
that will hopefully land fairly soon.)

[dgozman, 2015-06-18 10:50:49]

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityModel.js (right):

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:25: /** @typedef {!{
Just put this in a single line.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:66: /** @typedef {!{
We don't usually typdef events, since you will cast in handler anyway. Just pass
object as an event data.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:85:
this._model.dispatchEventToListeners(WebInspector.SecurityModel.EventTypes.SecurityStateChanged,
event);
event->data

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:18:
securityStateSection.createChild("hr");
This is ok for now, but as you add more content we should switch from flow
layout with <hr> and similar to box layout with flex.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:26: _addExplanation:
function(explanation) {
JSDoc please.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:48:
this._securityStateText.textContent = "Page security state: " +
this._securityState;
Wrap all user-visible text in WebInspector.UIString. For example,
WebInspector.UIString("Page security state: %s", this._securityState).

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:51:
explanations.map(this._addExplanation.bind(this));
We usually use for..of for this.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/securityPanel.css:75: .security.panel
.security-explanation .lock-icon {
Blank lines between rules.

[lgarron, 2015-06-18 18:16:18]

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityModel.js (right):

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:25: /** @typedef {!{
On 2015/06/18 at 10:50:48, dgozman wrote:
> Just put this in a single line.

Done.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:66: /** @typedef {!{
On 2015/06/18 at 10:50:48, dgozman wrote:
> We don't usually typdef events, since you will cast in handler anyway. Just
pass object as an event data.

Done.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:85:
this._model.dispatchEventToListeners(WebInspector.SecurityModel.EventTypes.SecurityStateChanged,
event);
On 2015/06/18 at 10:50:48, dgozman wrote:
> event->data

Good point. Done.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:18:
securityStateSection.createChild("hr");
On 2015/06/18 at 10:50:49, dgozman wrote:
> This is ok for now, but as you add more content we should switch from flow
layout with <hr> and similar to box layout with flex.

Acknowledged.
(I tried a few ideas like div borders, but this was simple, and looked good
enough.)

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:26: _addExplanation:
function(explanation) {
On 2015/06/18 at 10:50:49, dgozman wrote:
> JSDoc please.

Done.

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:48:
this._securityStateText.textContent = "Page security state: " +
this._securityState;
On 2015/06/18 at 10:50:49, dgozman wrote:
> Wrap all user-visible text in WebInspector.UIString. For example,
> WebInspector.UIString("Page security state: %s", this._securityState).

Done.
(We have our own [vs]printf? Awesome! 😎)

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:51:
explanations.map(this._addExplanation.bind(this));
On 2015/06/18 at 10:50:49, dgozman wrote:
> We usually use for..of for this.

Done.
(I did not know that for...of was introduced in ES 6. That's awesome; I've
wanted it since... I've ever programmed Javascript.)

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1179353002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/securityPanel.css:75: .security.panel
.security-explanation .lock-icon {
On 2015/06/18 at 10:50:49, dgozman wrote:
> Blank lines between rules.

Done.

[dgozman, 2015-06-19 11:33:36]

lgtm modulo some nits

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityModel.js (right):

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:66: var data = ({
nit: remove () and make this one-liner.

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:26: 
nit: extra blank lines.

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:55: for (var explanation of
explanations) {
nit: no {} for one-line blocks.

[lgarron, 2015-06-19 16:05:09]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-06-19 16:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1179353002/#ps100001
(title: "Address nits.")

[commit-bot: I haz the power, 2015-06-19 16:06:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1179353002/100001

[commit-bot: I haz the power, 2015-06-19 17:15:46]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197487

[lgarron, 2015-06-19 17:35:41]

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityModel.js (right):

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityModel.js:66: var data = ({
On 2015/06/19 at 11:33:36, dgozman wrote:
> nit: remove () and make this one-liner.

Done.


(Forgot to remove these while removing the @type annotation).

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:26: 
On 2015/06/19 at 11:33:36, dgozman wrote:
> nit: extra blank lines.

Removed.

https://codereview.chromium.org/1179353002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:55: for (var explanation of
explanations) {
On 2015/06/19 at 11:33:36, dgozman wrote:
> nit: no {} for one-line blocks.

Done.

(Even if I like my unconditional for-braces. :-P)