Add a minimal Security panel to DevTools (behind a hidden experiment).

Add a minimal Security panel to DevTools (behind a hidden experiment).

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

[lgarron, 2015-06-03 04:09:02]

I just installed the Java SDK, so the presubmit check looked at my annotations
for the first time. I still need to figure out what to do with the errors:

Java executable: /usr/bin/java
Checking duplicate files across modules...
Compiling frontend...
Compiling InjectedScriptSource.js...
Verifying JSDoc comments...
Validating InjectedScriptSource.js...

JSDoc validator output:
/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:22:
ERROR - Access to "document" property of global object is disallowed
        document.getElementById("lock-icon").className = "";
        ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:23:
ERROR - Access to "document" property of global object is disallowed
        document.getElementById("lock-icon").classList.add(event.data);
        ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:25:
ERROR - Access to "document" property of global object is disallowed
        document.getElementById("security-state").innerHTML = event.data;
        ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:28:
ERROR - @return annotation is required for API functions that return value
    createSecurityStateSection: function()
    ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:30:
ERROR - Access to "document" property of global object is disallowed
        var securityStateSection = document.createElement("div");
                                   ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:32:
ERROR - Access to "document" property of global object is disallowed
        var lockIcon = document.createElement("div");
                       ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:37:
ERROR - Access to "document" property of global object is disallowed
        var securityState = document.createElement("div");
                            ^

Total errors: 7

Modular compilation output:
Module platform compiled successfully: 0 error(s), 0 warning(s), 93.7% typed
Module common compiled successfully: 0 error(s), 0 warning(s), 92.5% typed
Module host compiled successfully: 0 error(s), 0 warning(s), 92.2% typed
Module ui compiled successfully: 0 error(s), 0 warning(s), 88.6% typed
Module ui_lazy compiled successfully: 0 error(s), 0 warning(s), 89.0% typed
Module sdk compiled successfully: 0 error(s), 0 warning(s), 91.3% typed
Module workspace compiled successfully: 0 error(s), 0 warning(s), 92.4% typed
Module bindings compiled successfully: 0 error(s), 0 warning(s), 91.8% typed
Module components compiled successfully: 0 error(s), 0 warning(s), 90.0% typed
Module promises compiled successfully: 0 error(s), 0 warning(s), 90.0% typed
Module toolbox_bootstrap compiled successfully: 0 error(s), 0 warning(s), 93.6%
typed
Module extensions compiled successfully: 0 error(s), 0 warning(s), 89.3% typed
Module elements compiled successfully: 0 error(s), 0 warning(s), 89.1% typed
Module accessibility compiled successfully: 0 error(s), 0 warning(s), 89.1%
typed
Module source_frame compiled successfully: 0 error(s), 0 warning(s), 89.6% typed
Module snippets compiled successfully: 0 error(s), 0 warning(s), 91.8% typed
Module sources compiled successfully: 0 error(s), 0 warning(s), 88.9% typed
Module audits compiled successfully: 0 error(s), 0 warning(s), 89.1% typed
Module components_lazy compiled successfully: 0 error(s), 0 warning(s), 89.7%
typed
Module timeline compiled successfully: 0 error(s), 0 warning(s), 89.5% typed
Module layers compiled successfully: 0 error(s), 0 warning(s), 89.5% typed
Module console compiled successfully: 0 error(s), 0 warning(s), 90.0% typed
Module network compiled successfully: 0 error(s), 0 warning(s), 89.6% typed
Module cm_modes compiled successfully: 0 error(s), 0 warning(s), 89.6% typed
Module emulation compiled successfully: 0 error(s), 0 warning(s), 89.8% typed
Module main compiled successfully: 0 error(s), 0 warning(s), 89.1% typed
Module resources compiled successfully: 0 error(s), 0 warning(s), 89.3% typed
Module script_formatter_worker compiled successfully: 0 error(s), 0 warning(s),
92.6% typed
Module devtools_app compiled successfully: 0 error(s), 0 warning(s), 92.2% typed
Module heap_snapshot_worker compiled successfully: 0 error(s), 0 warning(s),
83.7% typed
Module temp_storage_shared_worker compiled successfully: 0 error(s), 0
warning(s), 95.5% typed
Module settings compiled successfully: 0 error(s), 0 warning(s), 90.0% typed
Module profiler compiled successfully: 0 error(s), 0 warning(s), 89.7% typed
Module screencast compiled successfully: 0 error(s), 0 warning(s), 89.8% typed
Module security compile failed: 14 errors

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:7:
WARNING - Bad type annotation. Unknown type WebInspector.Panel
 * @extends {WebInspector.Panel}
             ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:8:
WARNING - Bad type annotation. Unknown type WebInspector.TargetManager.Observer
 * @implements {WebInspector.TargetManager.Observer}
                ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:14:
WARNING - Property targetManager never defined on WebInspector
    WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
                 ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:14:
WARNING - Property addModelListener never defined on WebInspector.targetManager
    WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
                               ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:14:
WARNING - Property SecurityManager never defined on WebInspector
    WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
                                                             ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:14:
WARNING - Property SecurityManager never defined on WebInspector
    WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
                                                                                
          ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:14:
WARNING - Property EventTypes never defined on WebInspector.SecurityManager
    WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
                                                                                
                          ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:14:
WARNING - Property SecurityStateChanged never defined on
WebInspector.SecurityManager.EventTypes
    WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
                                                                                
                                     ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:47:
WARNING - Bad type annotation. Unknown type WebInspector.Target
      * @param {!WebInspector.Target} target
                 ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:59:
WARNING - Property inspectorView never defined on WebInspector
   
WebInspector.inspectorView.setCurrentPanel(WebInspector.SecurityPanel._instance());
                 ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:59:
WARNING - Property setCurrentPanel never defined on WebInspector.inspectorView
   
WebInspector.inspectorView.setCurrentPanel(WebInspector.SecurityPanel._instance());
                               ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:74:
WARNING - Bad type annotation. Unknown type WebInspector.PanelFactory
 * @implements {WebInspector.PanelFactory}
                ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:83:
WARNING - Bad type annotation. Unknown type WebInspector.Panel
     * @return {!WebInspector.Panel}
                 ^

/Users/lgarron/chromium/src/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:85:
WARNING - property createPanel not defined on any superclass of
WebInspector.SecurityPanelFactory
    createPanel: function()
    ^

0 error(s), 14 warning(s), 94.0% typed

Total Closure errors: 14

InjectedScriptSource.js compilation output:
0 error(s), 0 warning(s), 91.2% typed

Validate InjectedScriptSource.js output:
OK

ERRORS DETECTED

[dgozman, 2015-06-03 10:12:34]

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

[dgozman, 2015-06-03 10:12:35]

Basic comments to help you get used to devtools style.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/sdk/SecurityManager.js (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/SecurityManager.js:10:
WebInspector.SecurityManager = function(target)
Move this to security module, do not add to sdk.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:22:
document.getElementById("lock-icon").className = "";
Instead of lookup, just save elements to a field.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:32: var lockIcon =
document.createElement("div");
Do not use |document|. Lookup utility method in platform/DOMExtensions.js.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:38: securityState.innerHTML
= "N/A";
No innerHTML, use textContent.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:57:
WebInspector.SecurityPanel.show = function()
You don't need this.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/module.json (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/module.json:11: "dependencies": [],
You probably have to depend on platform, host, and sdk.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/securityPanel.css:10: .security.panel
#lock-icon, .security.panel #security-state {
No ids please.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/securityPanel.css:23: .security.panel
#lock-icon.SECURE {background-image: url(Images/securityStateSecure.png);}
Can we avoid CAPS?

[lgarron, 2015-06-11 22:54:47]

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/sdk/SecurityManager.js (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/SecurityManager.js:10:
WebInspector.SecurityManager = function(target)
On 2015/06/03 at 10:12:35, dgozman wrote:
> Move this to security module, do not add to sdk.

What is the reason to add something to sdk vs. not?

In an email thread with Pavel, I asked whether to make a SecurityManager or
SecurityModel, and pfeldman@ suggested the latter – but I can't get that to
work.

In either case, do I still extend WebInspector.SDKModel?

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:22:
document.getElementById("lock-icon").className = "";
On 2015/06/03 at 10:12:35, dgozman wrote:
> Instead of lookup, just save elements to a field.

Done.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:32: var lockIcon =
document.createElement("div");
On 2015/06/03 at 10:12:35, dgozman wrote:
> Do not use |document|. Lookup utility method in platform/DOMExtensions.js.

Done.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:38: securityState.innerHTML
= "N/A";
On 2015/06/03 at 10:12:35, dgozman wrote:
> No innerHTML, use textContent.

Cool, that's what I was looking for. Done.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:57:
WebInspector.SecurityPanel.show = function()
On 2015/06/03 at 10:12:35, dgozman wrote:
> You don't need this.

Done.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/module.json (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/module.json:11: "dependencies": [],
On 2015/06/03 at 10:12:35, dgozman wrote:
> You probably have to depend on platform, host, and sdk.

What are each of these? The other panels don't have these in their `module.json`
files.
How do I learn how dependencies work?

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/securityPanel.css:10: .security.panel
#lock-icon, .security.panel #security-state {
On 2015/06/03 at 10:12:35, dgozman wrote:
> No ids please.

Done.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/securityPanel.css:23: .security.panel
#lock-icon.SECURE {background-image: url(Images/securityStateSecure.png);}
On 2015/06/03 at 10:12:35, dgozman wrote:
> Can we avoid CAPS?

Done.

[dgozman, 2015-06-12 06:20:22]

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/sdk/SecurityManager.js (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/SecurityManager.js:10:
WebInspector.SecurityManager = function(target)
On 2015/06/11 22:54:46, lgarron wrote:
> On 2015/06/03 at 10:12:35, dgozman wrote:
> > Move this to security module, do not add to sdk.
> 
> What is the reason to add something to sdk vs. not?
If the thing is self-contained, and nobody depends on it, we should not put it
in sdk.

> 
> In an email thread with Pavel, I asked whether to make a SecurityManager or
> SecurityModel, and pfeldman@ suggested the latter – but I can't get that to
> work.
> 
> In either case, do I still extend WebInspector.SDKModel?

Yes.

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/module.json (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/module.json:11: "dependencies": [],
On 2015/06/11 22:54:47, lgarron wrote:
> On 2015/06/03 at 10:12:35, dgozman wrote:
> > You probably have to depend on platform, host, and sdk.
> 
> What are each of these? The other panels don't have these in their
`module.json`
> files.
> How do I learn how dependencies work?

If you use something (class, function) from a file of another module, you have
to declare the dependency here.
Most of the DOM helpers are in platform module. Targets and models are in sdk.
Not sure about host, maybe you won't need it directly. Do not include unless
compiler complains.

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

Patchset #3 (id:40001) has been deleted

[lgarron, 2015-06-12 19:56:02]

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/module.json (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/module.json:11: "dependencies": [],
On 2015/06/12 at 06:20:22, dgozman wrote:
> On 2015/06/11 22:54:47, lgarron wrote:
> > On 2015/06/03 at 10:12:35, dgozman wrote:
> > > You probably have to depend on platform, host, and sdk.
> > 
> > What are each of these? The other panels don't have these in their
`module.json`
> > files.
> > How do I learn how dependencies work?
> 
> If you use something (class, function) from a file of another module, you have
to declare the dependency here.
> Most of the DOM helpers are in platform module. Targets and models are in sdk.
Not sure about host, maybe you won't need it directly. Do not include unless
compiler complains.

Makes sense. I didn't know anything about the compiler, but I'm assuming `git cl
presubmit --upload` gives the relevant output.

I found that the compiler is just as happy without "platform" and "host", but
needs "sdk". It also seems to need "ui". So, right now I have:
"dependencies": ["platform", "ui", "sdk"]

Is there something else I should be using in "ui"?

https://codereview.chromium.org/1159163005/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/1159163005/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sdk/Target.js:116: this.securityManager = new
WebInspector.SecurityManager(this);
If I move SecurityManager.js to security/ instead of sdk/, then the compiler
complains about this. However, removing it breaks the functionality we need.
What should I do as an alternative?

[lgarron, 2015-06-12 23:45:10]

Patchset #5 (id:100001) has been deleted

[dgozman, 2015-06-15 16:58:42]

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/security/module.json (right):

https://codereview.chromium.org/1159163005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/module.json:11: "dependencies": [],
> Makes sense. I didn't know anything about the compiler, but I'm assuming `git
cl
> presubmit --upload` gives the relevant output.
Yep.

> I found that the compiler is just as happy without "platform" and "host", but
> needs "sdk". It also seems to need "ui". So, right now I have:
> "dependencies": ["platform", "ui", "sdk"]
Dependencies are transitive (just like includes), so platform is indirectly
included in compilation through sdk/ui.

> Is there something else I should be using in "ui"?
Not sure what you mean. Use what you need.

https://codereview.chromium.org/1159163005/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/1159163005/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sdk/Target.js:116: this.securityManager = new
WebInspector.SecurityManager(this);
On 2015/06/12 19:56:02, lgarron wrote:
> If I move SecurityManager.js to security/ instead of sdk/, then the compiler
> complains about this. However, removing it breaks the functionality we need.
> What should I do as an alternative?

Just don't create it here. See WebInspector.AnimationModel.fromTarget for
example.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/sdk/SecurityManager.js (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/sdk/SecurityManager.js:14: this._target = target;
Not used. Remove.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/sdk/SecurityManager.js:24:
WebInspector.SecurityManager.prototype = {
Don't you need a |securityState()| getter?

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/sdk/SecurityManager.js:25: targetAdded: function ()
{},
Remove this.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/sdk/SecurityManager.js:45: // console.log("DISPATCHING
security state", securityState);
Remove commented code.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/sdk/SecurityManager.js:47: return;
Not needed.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:8: * @implements
{WebInspector.TargetManager.Observer}
You don't need this.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:14:
WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
Will you have security for different frames or workers? Perhaps, you should call
SecurityModel.fromTarget(WebInspector.targetManager.mainTarget()) and listen to
that particular instance.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:16:
this.element.appendChild(this._createSecurityStateSection());
I'd inline it here.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:20: _updateSecurityState:
function (newSecurityState)
No space after "function" anywhere, please.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:20: _updateSecurityState:
function (newSecurityState)
JSDoc please.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:27: this.securityState =
newSecurityState;
this._securityState

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:32: _onSecurityStateChanged:
function (event)
JSDoc please.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:37:
_createSecurityStateSection: function()
JSDoc please.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:41: this.lockIcon =
createElementWithClass("div", "lock-icon");
this._lockIcon since it's private.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:42:
securityStateSection.appendChild(this.lockIcon);
Shortcut code: |this._lockIcon = securityStateSection.createChild("div",
"lock-icon")|.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:44: this.securityStateText =
createElementWithClass("div", "security-state");
this._securityStateText

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:45:
securityStateSection.appendChild(this.securityStateText);
createChild

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:47:
this._updateSecurityState("unknown");
Better move the default "unknown" state into model, and ask it explicitly here.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:56: targetAdded:
function(target)
Remove.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:64: targetRemoved:
function(target)
Remove.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:7: margin: 1em;
4 spaces everywhere.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:10: .security.panel
.lock-icon, .security.panel .security-state {
One selector per line.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:20: background-image:
url(Images/securityStateHttp.png);
Don't use default one.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:23: .security.panel
.lock-icon.unknown  {background-image: url(Images/securityStateHttp.png);}
Use "lock-icon-unknown" and similar to prevent clashes with other places.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:24: .security.panel
.lock-icon.http     {background-image: url(Images/securityStateHttp.png);}
Please format as other rules.

[lgarron, 2015-06-15 22:38:09]

dgozman@: Thanks, that was pretty helpful. I've done everything I could figure
out how to do, and left comments on the rest.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/sdk/SecurityManager.js (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/sdk/SecurityManager.js:24:
WebInspector.SecurityManager.prototype = {
On 2015/06/15 at 16:58:41, dgozman wrote:
> Don't you need a |securityState()| getter?

Depends on how we use this as a model/manager.
We will probably have to add a protocol command to retrieve the initial security
state, which would effectively do the same.
Is it still worth creating a getter for a cached value?

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:14:
WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
On 2015/06/15 at 16:58:41, dgozman wrote:
> Will you have security for different frames or workers? Perhaps, you should
call SecurityModel.fromTarget(WebInspector.targetManager.mainTarget()) and
listen to that particular instance.

We might someday, but it's fine not to have them for now.
I tried
WebInspector.SecurityModel.fromTarget(WebInspector.targetManager.mainTarget()),
but it appears that WebInspector.targetManager.mainTarget() is null here if the
SecurityPanel is the current panel when DevTools is brought up. This breaks the
panel and makes the linter sad. Is there a more robust alternative?

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:47:
this._updateSecurityState("unknown");
On 2015/06/15 at 16:58:41, dgozman wrote:
> Better move the default "unknown" state into model, and ask it explicitly
here.

Sounds like a good use for a securityState() getter in the manager, so I've done
that. :-)

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:20: background-image:
url(Images/securityStateHttp.png);
On 2015/06/15 at 16:58:42, dgozman wrote:
> Don't use default one.

What do you recommend instead?

- No background
- CSS background fallback
- Fallback icon
- Create an icon for "unknown" and also use it for fallback?

[lgarron, 2015-06-16 01:03:34]

(Just uploaded the latest patch. It failed earlier in the day because of the
WebInspector.targetManager.mainTarget() linter issue in
https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
.)

[dgozman, 2015-06-16 13:42:59]

Almost there!

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:14:
WebInspector.targetManager.addModelListener(WebInspector.SecurityManager,
WebInspector.SecurityManager.EventTypes.SecurityStateChanged,
this._onSecurityStateChanged, this);
On 2015/06/15 22:38:08, lgarron wrote:
> On 2015/06/15 at 16:58:41, dgozman wrote:
> > Will you have security for different frames or workers? Perhaps, you should
> call SecurityModel.fromTarget(WebInspector.targetManager.mainTarget()) and
> listen to that particular instance.
> 
> We might someday, but it's fine not to have them for now.
> I tried
>
WebInspector.SecurityModel.fromTarget(WebInspector.targetManager.mainTarget()),
> but it appears that WebInspector.targetManager.mainTarget() is null here if
the
> SecurityPanel is the current panel when DevTools is brought up. This breaks
the
> panel and makes the linter sad. Is there a more robust alternative?

Oh, I see. Then you should bring back TargetManager.Observer, grab the first
target in targetAdded notification and work with it. Don't forget to cleanup in
targetRemoved.

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/120001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:20: background-image:
url(Images/securityStateHttp.png);
On 2015/06/15 22:38:08, lgarron wrote:
> On 2015/06/15 at 16:58:42, dgozman wrote:
> > Don't use default one.
> 
> What do you recommend instead?
> 
> - No background
> - CSS background fallback
> - Fallback icon
> - Create an icon for "unknown" and also use it for fallback?

No background. It should always have a valid class though.

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityModel.js (right):

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityModel.js:29: securityState:
function() {
{ on next line.

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityModel.js:52:
WebInspector.SecurityDispatcher = function(manager)
manager->model

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:16: // Create security state
section
nit: full stop please.

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:17: var securityStateSection
= createElement("div");
... = this.element.createChild("div");

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:24: .security.panel
.lock-icon-unknown  {background-image: url(Images/securityStateHttp.png);}
Each CSS properity should be on a separate line.

[lgarron, 2015-06-16 19:25:43]

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityModel.js (right):

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityModel.js:52:
WebInspector.SecurityDispatcher = function(manager)
On 2015/06/16 at 13:42:58, dgozman wrote:
> manager->model

Done.

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:16: // Create security state
section
On 2015/06/16 at 13:42:58, dgozman wrote:
> nit: full stop please.

Done.

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:17: var securityStateSection
= createElement("div");
On 2015/06/16 at 13:42:58, dgozman wrote:
> ... = this.element.createChild("div");

Done, but won't that mutate the DOM multiple times during construction? I
thought it was best practice to construct it a non-trivial structure off-DOM and
add it at the end.

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:24: .security.panel
.lock-icon-unknown  {background-image: url(Images/securityStateHttp.png);}
On 2015/06/16 at 13:42:58, dgozman wrote:
> Each CSS properity should be on a separate line.

Awwwwww. :-(

Okay.

[dgozman, 2015-06-16 19:43:36]

lgtm with comments

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/140001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:17: var securityStateSection
= createElement("div");
On 2015/06/16 19:25:43, lgarron wrote:
> On 2015/06/16 at 13:42:58, dgozman wrote:
> > ... = this.element.createChild("div");
> 
> Done, but won't that mutate the DOM multiple times during construction? I
> thought it was best practice to construct it a non-trivial structure off-DOM
and
> add it at the end.

It will. However, if you don't force layout, it should have the same performance
as constructing detached DOM nodes.

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:67: if (target ==
this._target) {
===

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:70: delete this._target;
Clear security state indication in UI as well.

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
File Source/devtools/front_end/security/securityPanel.css (right):

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
Source/devtools/front_end/security/securityPanel.css:21: background-image:
url(Images/securityStateHttp.png);
Remove this one, as we discussed.

[lgarron, 2015-06-16 19:57:08]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-06-16 19:57:08]

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/1159163005/#ps180001
(title: "Address final comments.")

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

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

[lgarron, 2015-06-16 20:17:47]

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
File Source/devtools/front_end/security/SecurityPanel.js (right):

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:67: if (target ==
this._target) {
On 2015/06/16 at 19:43:36, dgozman wrote:
> ===

:-D

https://codereview.chromium.org/1159163005/diff/160001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:70: delete this._target;
On 2015/06/16 at 19:43:36, dgozman wrote:
> Clear security state indication in UI as well.

Done.

[commit-bot: I haz the power, 2015-06-16 21:17:40]

Committed patchset #8 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197199