Add a sidebar to the DevTools Security panel.

Add a sidebar to the DevTools Security panel.

Followup CLs will add additional items to the sidebar.

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

[lgarron, 2015-06-25 22:55:53]

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

[lgarron, 2015-06-25 22:55:54]

dgozman: I've based this CL off the Audits Panel [1], but had to make some
decisions that might need to change. Would you mind reviewing?

In particular, the sidebar is constructed in the constructor (currently without
any sections).
The "main view" (which is currently still the only view) has been split into its
own class, similar to the Audits Panel's launcher view.

See https://crbug.com/503167#c2 for a screenshot, and
https://crbug.com/502118#c2 for a mock of the goal.


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

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

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

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:14: // Construct sidebar
Please remove comments which follow the code and not explain why the code is
written in this way.

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:15: var sidebarTree = new
TreeOutline();
TreeOutlineInShadow

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:64: set visibleView(x)
Getters and setters are deprecated. Use methods instead.

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:141: setSecurityState:
function(newSecurityState) {
{ on next line

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:205: _addExplanation:
function(explanation) {
{ on next line

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:221: // It's safe to call
this even when this._securityState is undefined.
This is a great comment which describes not what is happening, but why.

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

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/securityPanel.css:6: .security.panel
.security-main-view {
You can omit .security.panel where you have .security-main-view in selectors.

[lgarron, 2015-06-26 16:33:32]

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

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:14: // Construct sidebar
On 2015/06/26 at 10:50:49, dgozman wrote:
> Please remove comments which follow the code and not explain why the code is
written in this way.

Done

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:15: var sidebarTree = new
TreeOutline();
On 2015/06/26 at 10:50:49, dgozman wrote:
> TreeOutlineInShadow

Done.
However, this breaks the CSS for the sidebar, and I can't figure out how to fix
it.
Is there a good example I can follow for that?

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:64: set visibleView(x)
On 2015/06/26 at 10:50:49, dgozman wrote:
> Getters and setters are deprecated. Use methods instead.

Done.

Should other cases (e.g. Audits panel) be updated, then?

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:141: setSecurityState:
function(newSecurityState) {
On 2015/06/26 at 10:50:49, dgozman wrote:
> { on next line

Done.
(I'm used to same-line, so I always have to remind myself consciously. Is there
a way to add this to the presubmit check?)

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:205: _addExplanation:
function(explanation) {
On 2015/06/26 at 10:50:49, dgozman wrote:
> { on next line

Done.

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

https://codereview.chromium.org/1215493002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/security/securityPanel.css:6: .security.panel
.security-main-view {
On 2015/06/26 at 10:50:49, dgozman wrote:
> You can omit .security.panel where you have .security-main-view in selectors.

Done.

I've also removed .security-main-view before .security-explanation in the
selectors below.

[dgozman, 2015-06-28 16:22:34]

lgtm modulo comments

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

https://codereview.chromium.org/1215493002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:58: getVisibleView:
function()
This is not used. Remove.

https://codereview.chromium.org/1215493002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:66: setVisibleView:
function(view)
Make it private.

https://codereview.chromium.org/1215493002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:131:
this.iconElement.classList.add("lock-icon");
For this to work, split off lockIcon.css and registerRequiredCSS here as well as
in other places it's used.

[lgarron, 2015-06-29 07:37:10]

dgozman@: Thanks, registerRequiredCSS() turns out to be quite approachable! I
know you've already LGTMed, but would would you mind taking one last look to
make sure I'm not introducing something you don't expect?

I assumed that the shadow root (sidebarTree) is the right place to attach the
CSS, so I went with that, and refactored some CSS.

[dgozman, 2015-06-29 08:11:47]

still lgtm

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

https://codereview.chromium.org/1215493002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:18:
sidebarTree.registerRequiredCSS("security/lockIcon.css");
I'd rather attach this in SecurityMainViewSidebarTreeElement. It's cheap.

[lgarron, 2015-06-29 08:22:56]

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

https://codereview.chromium.org/1215493002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:18:
sidebarTree.registerRequiredCSS("security/lockIcon.css");
On 2015/06/29 at 08:11:47, dgozman wrote:
> I'd rather attach this in SecurityMainViewSidebarTreeElement. It's cheap.

Hmm, I wasn't sure how to do that, since registerRequiredCSS can't be called on
a WebInspector.SidebarTreeElement.
Should I:
- Pass sidebarTree into the SecurityMainViewSidebarTreeElement constructor and
call sidebarTree.registerRequiredCSS()?
- Make SecurityMainViewSidebarTreeElement extend something that has
registerRequiredCSS, like WebInspector.Widget or WebInspector.Section?
- Do something else?

It isn't obvious to me. :-(

[dgozman, 2015-06-29 08:30:56]

On 2015/06/29 08:22:56, lgarron wrote:
>
https://codereview.chromium.org/1215493002/diff/60001/Source/devtools/front_e...
> File Source/devtools/front_end/security/SecurityPanel.js (right):
> 
>
https://codereview.chromium.org/1215493002/diff/60001/Source/devtools/front_e...
> Source/devtools/front_end/security/SecurityPanel.js:18:
> sidebarTree.registerRequiredCSS("security/lockIcon.css");
> On 2015/06/29 at 08:11:47, dgozman wrote:
> > I'd rather attach this in SecurityMainViewSidebarTreeElement. It's cheap.
> 
> Hmm, I wasn't sure how to do that, since registerRequiredCSS can't be called
on
> a WebInspector.SidebarTreeElement.
My bad. Sorry for confusion. You did the right thing here! Basically, we usually
pick the closest WI.View, but sidebar element is not a view.

[lgarron, 2015-06-29 08:32:20]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-06-29 08:32:32]

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

[commit-bot: I haz the power, 2015-06-29 09:23:00]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197972