Add origin views to the Security panel.

Add origin views to the Security panel.

BUG=502118, 503170
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201187

[lgarron, 2015-08-21 04:18:09]

Patchset #1 (id:1) has been deleted

[lgarron, 2015-08-21 04:21:03]

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

[lgarron, 2015-08-21 04:21:04]

dgozman@: Could you review this CL?

This is the followup to https://codereview.chromium.org/1284413004 that exposes
the origin info in the panel.

For a sample screenshot of this code in action, see
https://goo.gl/photos/QAMw2jNN8eL7Az12A

[lgarron, 2015-08-21 18:51:52]

Patchset #1 (id:20001) has been deleted

[lgarron, 2015-08-21 18:51:57]

Patchset #1 (id:40001) has been deleted

[lgarron, 2015-08-21 18:57:37]

Patchset #1 (id:60001) has been deleted

[dgozman, 2015-08-21 19:44:37]

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

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:36: //
WebInspector.targetManager.addEventListener(WebInspector.ResourceTreeModel.EventTypes.MainFrameNavigated,
this._clear, this);
Do not commit commented code. Better mention in comment above the
MainFrameNavigated event.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:132:
this._sidebarOriginSection.appendChild(originData.sidebarElement)
nit: semicolon

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:206:
WebInspector.SecurityPanel.Origin;
nit: move to the top (before SecurityPanel.prototype0.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:261: this.small = true
nit: semicolon

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:262:
WebInspector.SidebarTreeElement.call(this, "security-sidebar-tree-item",
WebInspector.UIString(origin));
Don't use UIString for origin. It's meant for string literals, not for
user-originated strings.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:282: for (var className of
this.iconElement.classList)
nit: use {} for more-than-one-liners

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:283: if
(className.indexOf("security-property-") === 0)
startsWith

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:284:
this.iconElement.classList.remove(className);
Removing while iterating could be dangerous. Make a copy
(Array.prototype.slice).

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:372:
WebInspector.SecurityOriginView = function(panel, origin, securityState,
securityDetails)
- JSDoc for params;
- why pass securityState and details, if those are available from |origin|?

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/security/originView.css (right):

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/originView.css:28: .security-origin-view h1 {
We usually use div with specific class which reflects the purpose/content of the
element rather than it's visual (like h1 does).

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/originView.css:40: .security-origin-view
table.details-table {
Looking at the screenshot, you don't really need a table. Just add two divs to
each row, with left div having fixed width.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/originView.css:46: vertical-align: top;
Use the modern display:flex and align-items:center.

[lgarron, 2015-08-21 21:48:46]

Patchset #2 (id:100001) has been deleted

[lgarron, 2015-08-21 21:50:07]

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

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:36: //
WebInspector.targetManager.addEventListener(WebInspector.ResourceTreeModel.EventTypes.MainFrameNavigated,
this._clear, this);
On 2015/08/21 at 19:44:36, dgozman wrote:
> Do not commit commented code. Better mention in comment above the
MainFrameNavigated event.

I don't want to uncomment the MainFrameNavigated event, because it makes the
panel unusable for development. I'll take out the listeners completely.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:132:
this._sidebarOriginSection.appendChild(originData.sidebarElement)
On 2015/08/21 at 19:44:37, dgozman wrote:
> nit: semicolon

Done.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:206:
WebInspector.SecurityPanel.Origin;
On 2015/08/21 at 19:44:36, dgozman wrote:
> nit: move to the top (before SecurityPanel.prototype0.

Done.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:261: this.small = true
On 2015/08/21 at 19:44:37, dgozman wrote:
> nit: semicolon

Done.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:262:
WebInspector.SidebarTreeElement.call(this, "security-sidebar-tree-item",
WebInspector.UIString(origin));
On 2015/08/21 at 19:44:37, dgozman wrote:
> Don't use UIString for origin. It's meant for string literals, not for
user-originated strings.

Okay. So on user-originated strings, we don't do any escaping when using the
element.textContent setter?

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:282: for (var className of
this.iconElement.classList)
On 2015/08/21 at 19:44:37, dgozman wrote:
> nit: use {} for more-than-one-liners

:-D
(Done.)

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:283: if
(className.indexOf("security-property-") === 0)
On 2015/08/21 at 19:44:37, dgozman wrote:
> startsWith

Done.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:284:
this.iconElement.classList.remove(className);
On 2015/08/21 at 19:44:37, dgozman wrote:
> Removing while iterating could be dangerous. Make a copy
(Array.prototype.slice).

Done.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:372:
WebInspector.SecurityOriginView = function(panel, origin, securityState,
securityDetails)
On 2015/08/21 at 19:44:37, dgozman wrote:
> - JSDoc for params;
> - why pass securityState and details, if those are available from |origin|?

JSDoc: Done.

Why pass? I wanted to keep all the business logic in SecurityPanel, so that
SecurityOriginView doesn't make any assumptions about when/where to update.
I also anticipate having multiple security details sections, which will have to
be factored out into different functions that take explicit parameters (namely,
the security details for that particular section).

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/security/originView.css (right):

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/originView.css:28: .security-origin-view h1 {
On 2015/08/21 at 19:44:37, dgozman wrote:
> We usually use div with specific class which reflects the purpose/content of
the element rather than it's visual (like h1 does).

I've removed the use of `h1` and `h2` in the origin view.

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/originView.css:40: .security-origin-view
table.details-table {
On 2015/08/21 at 19:44:37, dgozman wrote:
> Looking at the screenshot, you don't really need a table. Just add two divs to
each row, with left div having fixed width.

A table seems very straightforward to me. How would I do line wraps using divs?

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/originView.css:46: vertical-align: top;
On 2015/08/21 at 19:44:37, dgozman wrote:
> Use the modern display:flex and align-items:center.

(Will do if I change to divs.)

[dgozman, 2015-08-24 21:44:54]

What do you mean by line-wraps in divs?
Text wrap? Set white-space: wrap; flex: auto.
Elements wrap? Set flex-wrap: wrap.

Table is outdated. If you make left column be a div with fixed width, vertical
layout should be good.

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

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:262:
WebInspector.SidebarTreeElement.call(this, "security-sidebar-tree-item",
WebInspector.UIString(origin));
On 2015/08/21 21:50:06, lgarron wrote:
> On 2015/08/21 at 19:44:37, dgozman wrote:
> > Don't use UIString for origin. It's meant for string literals, not for
> user-originated strings.
> 
> Okay. So on user-originated strings, we don't do any escaping when using the
> element.textContent setter?

Using |textContent| is safe, it's not like |innerHTML|. UIString is meant for
localization purposes.

[lgarron, 2015-08-24 23:42:16]

dgozman@: Down with tables! Long live flex! (Could you take another look? :-)

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

https://codereview.chromium.org/1301833003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/security/SecurityPanel.js:262:
WebInspector.SidebarTreeElement.call(this, "security-sidebar-tree-item",
WebInspector.UIString(origin));
On 2015/08/24 at 21:44:54, dgozman wrote:
> On 2015/08/21 21:50:06, lgarron wrote:
> > On 2015/08/21 at 19:44:37, dgozman wrote:
> > > Don't use UIString for origin. It's meant for string literals, not for
> > user-originated strings.
> > 
> > Okay. So on user-originated strings, we don't do any escaping when using the
> > element.textContent setter?
> 
> Using |textContent| is safe, it's not like |innerHTML|. UIString is meant for
localization purposes.

Cool, that matches what I expect. :-)

[dgozman, 2015-08-25 20:21:51]

lgtm

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

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:33: /** @type {!Map<string,
!{securityState: !SecurityAgent.SecurityState, securityDetails:
?NetworkAgent.SecurityDetails}>} */
You store the originView here. Update JSDoc.

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:479:
WebInspector.SecurityDetailsTable = function() {
nit: { on next line

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:489: element: function() {
nit: { on next line

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:508: },
nit: extra comma

[lgarron, 2015-08-25 21:03:07]

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

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:33: /** @type {!Map<string,
!{securityState: !SecurityAgent.SecurityState, securityDetails:
?NetworkAgent.SecurityDetails}>} */
On 2015/08/25 at 20:21:51, dgozman wrote:
> You store the originView here. Update JSDoc.

Good point. I've created a separate WebInspector.SecurityPanel.OriginState
declaration, to document the object more clearly.

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:479:
WebInspector.SecurityDetailsTable = function() {
On 2015/08/25 at 20:21:51, dgozman wrote:
> nit: { on next line

Done.

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:489: element: function() {
On 2015/08/25 at 20:21:51, dgozman wrote:
> nit: { on next line

Done.

https://codereview.chromium.org/1301833003/diff/180001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:508: },
On 2015/08/25 at 20:21:51, dgozman wrote:
> nit: extra comma

Done.

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

https://codereview.chromium.org/1301833003/diff/200001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:50:
WebInspector.SecurityPanel.OriginState;
dgozman@: I've added this definition to use in the Map. Would you mind reviewing
whether the JSDoc annotation is appropriate?

[dgozman, 2015-08-25 23:26:50]

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

https://codereview.chromium.org/1301833003/diff/200001/Source/devtools/front_...
Source/devtools/front_end/security/SecurityPanel.js:50:
WebInspector.SecurityPanel.OriginState;
On 2015/08/25 21:03:07, lgarron wrote:
> dgozman@: I've added this definition to use in the Map. Would you mind
reviewing
> whether the JSDoc annotation is appropriate?

Looks good, but don't align the field names (similar to @param names).

[lgarron, 2015-08-26 00:06:26]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-08-26 00:06:26]

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/1301833003/#ps220001
(title: "Realign.")

[commit-bot: I haz the power, 2015-08-26 00:06:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1301833003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1301833003/220001

[lgarron, 2015-08-26 01:31:30]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-08-26 01:31:30]

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/1301833003/#ps240001
(title: "Mark securityState as explicitly optional in OriginState.")

[commit-bot: I haz the power, 2015-08-26 01:31:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1301833003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1301833003/240001

[commit-bot: I haz the power, 2015-08-26 02:40:34]

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