Link to the mixed content filter from security panel

Link to the mixed content filter from security panel

BUG=504537
Committed: https://crrev.com/b5c71127df2efb24c486ab9e2f58274ea6809ba6
git-svn-id: svn://svn.chromium.org/blink/trunk@202002 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[estark, 2015-09-03 23:43:26]

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

[estark, 2015-09-03 23:43:26]

Hey lgarron, could you please look this over for devtools style?

[pfeldman, 2015-09-03 23:48:48]

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

[pfeldman, 2015-09-03 23:48:48]

Please provide screenshot.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/n...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/n...
Source/devtools/front_end/network/NetworkPanel.js:645:
WebInspector.NetworkPanel.revealAndFilter = function(filterType, filterValue) {
{ goes next line.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/n...
Source/devtools/front_end/network/NetworkPanel.js:647:
panel._networkLogView._textFilterUI.setValue(filterType + ":" + filterValue);
You should not access private member of logview from here, please expose
explicit method there.

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

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityModel.js:68: var data =
{"securityState": securityState, "explanations": explanations || [],
"mixedContentStatus": mixedContentStatus, "schemeIsCryptographic":
schemeIsCryptographic};
It is always better to define a class / typedef it and cast data on the receiver
side to that type.

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

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:381: var
mixedContentExplanation;
4 space indent.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:383: mixedContentExplanation
= {
These plain JS objects are always compiler-unfriendly and should be avoided via
adding typedefs and casting them.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:403: function
showDisplayedMixedContentInNetworkPanel(e) {
{ goes next line, missing annotation.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:408: function
showBlockOverriddenMixedContentInNetworkPanel(e) {
ditto

[lgarron, 2015-09-04 00:44:36]

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

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:521: 
Nit: unnecessary whitespace change.

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

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:60: * @param
{!SecurityAgent.MixedContentStatus=} mixedContentStatus
@param {?SecurityAgent.MixedContentStatus} mixedContentStatus

= represents an optional parameter. In this case, the parameter is guaranteed to
be provided (by the way we call it), but may be null (question mark prefix.)

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:61: * @param {boolean=}
schemeIsCryptographic
Hmm, so, I believe JSDoc won't allow an nullable boolean, and the idiomatic
Javascript checks conflate "not present" with "false". (One reasons I avoided
the issue by placing the mixed content explanation generation in the model.)

How about we just make `schemeIsCryptographic and `mixedContentStatus`
non-optional in protocol.json?

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:76:
this._updateSecurityState(securityState, explanations,
event.data.mixedContentStatus, event.data.schemeIsCryptographic);
I believe DevTools convention is to cast these (like securityState and
explanations above).

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:383: mixedContentExplanation
= {
On 2015/09/03 at 23:48:48, pfeldman wrote:
> These plain JS objects are always compiler-unfriendly and should be avoided
via adding typedefs and casting them.

estark@: It seems I forgot to do this when I added these to SecurityModel.js

mixedContentExplanation = /** @type {!SecurityAgent. SecurityStateExplanation}
*/ ({
    ...
});

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:395: if
(mixedContentExplanation) {
I don't know if if's done anywhere in DevTools, but I personally don't like
relying on a hoisted unassigned variable to be falsey for a conditional check.

(I'd either:
- use an explicit boolean to keep track of whether t
- remove this if and place `if (mixedContentStatus.displayedInsecureContent ||
mixedContentStatus.ranInsecureContent)` around the three relevant blocks
- explicitly set mixedContentExplanation to null and cast to /** @type
{?SecurityAgent. SecurityStateExplanation} */ here)

[estark, 2015-09-04 02:45:31]

Attached a screenshot to the bug.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/n...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/n...
Source/devtools/front_end/network/NetworkPanel.js:645:
WebInspector.NetworkPanel.revealAndFilter = function(filterType, filterValue) {
On 2015/09/03 23:48:48, pfeldman wrote:
> { goes next line.

Done.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/n...
Source/devtools/front_end/network/NetworkPanel.js:647:
panel._networkLogView._textFilterUI.setValue(filterType + ":" + filterValue);
On 2015/09/03 23:48:48, pfeldman wrote:
> You should not access private member of logview from here, please expose
> explicit method there.

Done.

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

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityModel.js:68: var data =
{"securityState": securityState, "explanations": explanations || [],
"mixedContentStatus": mixedContentStatus, "schemeIsCryptographic":
schemeIsCryptographic};
On 2015/09/03 23:48:48, pfeldman wrote:
> It is always better to define a class / typedef it and cast data on the
receiver
> side to that type.

Not sure if I did this right, PTAL.

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

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:521: 
On 2015/09/04 00:44:35, lgarron wrote:
> Nit: unnecessary whitespace change.

Done.

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

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:60: * @param
{!SecurityAgent.MixedContentStatus=} mixedContentStatus
On 2015/09/04 00:44:35, lgarron wrote:
> @param {?SecurityAgent.MixedContentStatus} mixedContentStatus
> 
> = represents an optional parameter. In this case, the parameter is guaranteed
to
> be provided (by the way we call it), but may be null (question mark prefix.)

Done.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:61: * @param {boolean=}
schemeIsCryptographic
On 2015/09/04 00:44:35, lgarron wrote:
> Hmm, so, I believe JSDoc won't allow an nullable boolean, and the idiomatic
> Javascript checks conflate "not present" with "false". (One reasons I avoided
> the issue by placing the mixed content explanation generation in the model.)
> 
> How about we just make `schemeIsCryptographic and `mixedContentStatus`
> non-optional in protocol.json?

Converted schemeIsCryptographic to a boolean in SecurityModel.

If we want to make these arguments non-optional in protocol.json, that seems
fine, but I'd rather do it in a separate CL.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:76:
this._updateSecurityState(securityState, explanations,
event.data.mixedContentStatus, event.data.schemeIsCryptographic);
On 2015/09/04 00:44:35, lgarron wrote:
> I believe DevTools convention is to cast these (like securityState and
> explanations above).

Done.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:381: var
mixedContentExplanation;
On 2015/09/03 23:48:48, pfeldman wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:383: mixedContentExplanation
= {
On 2015/09/03 23:48:48, pfeldman wrote:
> These plain JS objects are always compiler-unfriendly and should be avoided
via
> adding typedefs and casting them.

Not sure if I did this right, PTAL.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:395: if
(mixedContentExplanation) {
On 2015/09/04 00:44:35, lgarron wrote:
> I don't know if if's done anywhere in DevTools, but I personally don't like
> relying on a hoisted unassigned variable to be falsey for a conditional check.
> 
> (I'd either:
> - use an explicit boolean to keep track of whether t
> - remove this if and place `if (mixedContentStatus.displayedInsecureContent ||
> mixedContentStatus.ranInsecureContent)` around the three relevant blocks
> - explicitly set mixedContentExplanation to null and cast to /** @type
> {?SecurityAgent. SecurityStateExplanation} */ here)

Done (conditioned the whole block around displayed/ranInsecureContent)

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:403: function
showDisplayedMixedContentInNetworkPanel(e) {
On 2015/09/03 23:48:48, pfeldman wrote:
> { goes next line, missing annotation.

Done.

https://codereview.chromium.org/1327593005/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/security/SecurityPanel.js:408: function
showBlockOverriddenMixedContentInNetworkPanel(e) {
On 2015/09/03 23:48:48, pfeldman wrote:
> ditto

Done.

[estark, 2015-09-08 18:11:08]

friendly ping

[pfeldman, 2015-09-09 18:49:48]

lgtm

[estark, 2015-09-09 19:38:40]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-09-09 19:38:46]

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

[commit-bot: I haz the power, 2015-09-09 21:11:25]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202002

[commit-bot: I haz the power, 2015-09-23 12:03:21]

Patchset 5 (id:??) landed as
https://crrev.com/b5c71127df2efb24c486ab9e2f58274ea6809ba6