[DevTools] Show a explanation when all resources in the page have been requested securely (no-mixed… (issue 1383503003 by mmccoy@chromium.org)

0 views
Skip to first unread message

mmc...@chromium.org

unread,
Oct 1, 2015, 9:35:39 AM10/1/15
to yu...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
Reviewers: yurys,

Description:
[DevTools] Show a explanation when all resources in the page have been
requested
securely (no-mixed-content).

* Adds a secure/green bullet point to SecurityPanel when all resources on
the
page have been transfered securely.

BUG=527232

Please review this at https://codereview.chromium.org/1383503003/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+30, -7 lines):
A +
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html
A
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure-expected.txt
M third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js


Index:
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure-expected.txt
diff --git
a/third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure-expected.txt
b/third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure-expected.txt
new file mode 100644
index
0000000000000000000000000000000000000000..98b3a3fb7bbf8bf75d8fcc7041e77dfccdb26650
--- /dev/null
+++
b/third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure-expected.txt
@@ -0,0 +1,15 @@
+Tests addition of explanation when all page resources are transferred
securely (no-mixed-content).
+
+<DIV class=security-explanation security-explanation-secure >
+ <DIV class=security-property security-property-secure >
+ </DIV>
+ <DIV class=security-explanation-text >
+ <DIV class=security-explanation-title >
+Secure Resources
+ </DIV>
+ <DIV >
+All resources on this page are served securely.
+ </DIV>
+ </DIV>
+</DIV>
+
Index:
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html
diff --git
a/third_party/WebKit/LayoutTests/http/tests/inspector/security/security-blocked-mixed-content.html
b/third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html
similarity index 76%
copy from
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-blocked-mixed-content.html
copy to
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html
index
412fcc2a3f95dc3903b38ac9fd279cda77da37a5..5bce51546cf6aaaa8f952274d8a5608f9204c94d
100644
---
a/third_party/WebKit/LayoutTests/http/tests/inspector/security/security-blocked-mixed-content.html
+++
b/third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html
@@ -9,13 +9,13 @@ var initialize_SecurityTest = function() {
function test()
{
/** @type {!SecurityAgent.MixedContentStatus} */
- var mixedContentStatus = { ranInsecureContent: false,
displayedInsecureContent: false, ranInsecureContentStyle:
SecurityAgent.SecurityState.Insecure, displayedInsecureContentStyle:
SecurityAgent.SecurityState.Neutral };
+ // var mixedContentStatus = { ranInsecureContent: false,
displayedInsecureContent: false, ranInsecureContentStyle:
SecurityAgent.SecurityState.Secure, displayedInsecureContentStyle:
SecurityAgent.SecurityState.Neutral };
+ var mixedContentStatus = { ranInsecureContent: false,
displayedInsecureContent: false};
+
var targets = WebInspector.targetManager.targets();

targets[0].model(WebInspector.SecurityModel).dispatchEventToListeners(WebInspector.SecurityModel.EventTypes.SecurityStateChanged,
new WebInspector.PageSecurityState(SecurityAgent.SecurityState.Secure, [],
mixedContentStatus, true));

var request = new WebInspector.NetworkRequest(targets[0],
0, "http://foo.test", "https://foo.test", 0, 0, null);
- request.setBlockedReason(NetworkAgent.BlockedReason.MixedContent);
- request.mixedContentType = "blockable";

targets[0].model(WebInspector.NetworkManager).dispatchEventToListeners(WebInspector.NetworkManager.EventTypes.RequestFinished,
request);

var explanations =
WebInspector.SecurityPanel._instance()._mainView.contentElement.getElementsByClassName("security-explanation");
@@ -26,6 +26,6 @@ function test()
</script>
</head>
<body onload="runTest()">
-<p>Tests active mixed content blocking in the security panel.</p>
+<p>Tests addition of explanation when all page resources are transferred
securely (no-mixed-content).</p>
</body>
</html>
Index:
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
diff --git
a/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
b/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
index
d70c4720d8ca9ede027e320a3888678457f90d1c..502c26b46bf824d234b715ed6f01b6ed51799523
100644
--- a/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
+++ b/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
@@ -539,6 +539,10 @@ WebInspector.SecurityMainView.prototype = {

this._addMixedContentExplanation(this._mixedContentStatus.displayedInsecureContentStyle,
WebInspector.UIString("Mixed Content"), WebInspector.UIString("The site
includes HTTP resources."),
WebInspector.NetworkLogView.MixedContentFilterValues.Displayed,
showDisplayedMixedContentInNetworkPanel);
}

+ if (this._mixedContentStatus &&
(!this._mixedContentStatus.displayedInsecureContent
&& !this._mixedContentStatus.ranInsecureContent)) {
+
this._addMixedContentExplanation(SecurityAgent.SecurityState.Secure,
WebInspector.UIString("Secure Resources"), WebInspector.UIString("All
resources on this page are served securely."));
+ }
+
if
(this._panel.filterRequestCount(WebInspector.NetworkLogView.MixedContentFilterValues.Blocked)
> 0)

this._addMixedContentExplanation(SecurityAgent.SecurityState.Info,
WebInspector.UIString("Blocked mixed content"), WebInspector.UIString("Your
page requested insecure resources that were blocked."),
WebInspector.NetworkLogView.MixedContentFilterValues.Blocked,
showBlockedMixedContentInNetworkPanel);

@@ -574,8 +578,8 @@ WebInspector.SecurityMainView.prototype = {
* @param {!SecurityAgent.SecurityState} securityState
* @param {string} summary
* @param {string} description
- * @param {!WebInspector.NetworkLogView.MixedContentFilterValues}
filterKey
- * @param {!Function} networkFilterFn
+ * @param {!WebInspector.NetworkLogView.MixedContentFilterValues=}
filterKey
+ * @param {!Function=} networkFilterFn
*/
_addMixedContentExplanation: function(securityState, summary,
description, filterKey, networkFilterFn)
{
@@ -585,8 +589,12 @@ WebInspector.SecurityMainView.prototype = {
"description": description
});

+ var explanation = this._addExplanation(mixedContentExplanation);
+
+ if (!filterKey || !networkFilterFn) { return; }
+
var filterRequestCount = this._panel.filterRequestCount(filterKey);
- var requestsAnchor =
this._addExplanation(mixedContentExplanation).createChild("div", "security-mixed-content
link");
+ var requestsAnchor =
explanation.createChild("div", "security-mixed-content link");
if (filterRequestCount > 0) {
requestsAnchor.textContent = WebInspector.UIString("View %d
request%s in Network Panel", filterRequestCount, (filterRequestCount >
1 ? "s" : ""));
} else {


commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2015, 9:36:10 AM10/1/15
to mmc...@chromium.org, yu...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1383503003/20001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1383503003/20001


https://codereview.chromium.org/1383503003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2015, 9:36:11 AM10/1/15
to mmc...@chromium.org, yu...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

https://codereview.chromium.org/1383503003/

mmc...@chromium.org

unread,
Oct 5, 2015, 9:37:24 AM10/5/15
to al...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
https://codereview.chromium.org/1383503003/

paul...@chromium.org

unread,
Oct 5, 2015, 5:28:27 PM10/5/15
to mmc...@chromium.org, est...@chromium.org, lga...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
lucas to review. after lgtm add dgozman to review.

https://codereview.chromium.org/1383503003/

mmc...@chromium.org

unread,
Oct 8, 2015, 3:21:40 PM10/8/15
to est...@chromium.org, lga...@chromium.org, paul...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
On 2015/10/05 at 21:28:27, paulirish wrote:
> lucas to review. after lgtm add dgozman to review.

bump! :)

https://codereview.chromium.org/1383503003/

lga...@chromium.org

unread,
Oct 8, 2015, 8:16:32 PM10/8/15
to mmc...@chromium.org, est...@chromium.org, paul...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
LGTM; thanks for the poke today.


https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
File
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js#newcode543
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:543:
this._addMixedContentExplanation(SecurityAgent.SecurityState.Secure,
WebInspector.UIString("Secure Resources"), WebInspector.UIString("All
resources on this page are served securely."));
I'm a bit split on this. On the one hand, this patch complicates
_addMixedContentExplanation in order to allow it to handle this case as
if it were just:


this._addExplanation(/** @type
{!SecurityAgent.SecurityStateExplanation} */ ({
"securityState": SecurityAgent.SecurityState.Secure,
"summary": WebInspector.UIString("Secure Resources"),
"description": WebInspector.UIString("All resources on
this page are served securely.")
}));

On the other hand, calling _addMixedContentExplanation for all mixed
content states makes sense if we want to do something special for mixed
content explanations (e.g. styling). On the third hand, we don't do that
kind of styling right now, and I don't feel we need to set up our code
to guard for it.
On the fourth hand, now that it's written, it doesn't hurt.

So what I'm saying is that I guess I'm okay with it. :-P

https://codereview.chromium.org/1383503003/

mmc...@chromium.org

unread,
Oct 8, 2015, 8:31:18 PM10/8/15
to est...@chromium.org, lga...@chromium.org, paul...@chromium.org, dgo...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
Thanks, Lucas!

Dmitry, would you mind having a look as owner? Thanks!

https://codereview.chromium.org/1383503003/

dgo...@chromium.org

unread,
Oct 8, 2015, 11:52:55 PM10/8/15
to mmc...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org

https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
File
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js#newcode543
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:543:
this._addMixedContentExplanation(SecurityAgent.SecurityState.Secure,
WebInspector.UIString("Secure Resources"), WebInspector.UIString("All
resources on this page are served securely."));
On 2015/10/09 00:16:31, lgarron wrote:
> I'm a bit split on this. On the one hand, this patch complicates
> _addMixedContentExplanation in order to allow it to handle this case
as if it
> were just:


> this._addExplanation(/** @type
> {!SecurityAgent.SecurityStateExplanation} */ ({
> "securityState": SecurityAgent.SecurityState.Secure,
> "summary": WebInspector.UIString("Secure Resources"),
> "description": WebInspector.UIString("All resources on
this page
> are served securely.")
> }));

> On the other hand, calling _addMixedContentExplanation for all mixed
content
> states makes sense if we want to do something special for mixed
content
> explanations (e.g. styling). On the third hand, we don't do that kind
of styling
> right now, and I don't feel we need to set up our code to guard for
it.
> On the fourth hand, now that it's written, it doesn't hurt.

> So what I'm saying is that I guess I'm okay with it. :-P

I think it's better to use |_addExplanation| here.

https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js#newcode594
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:594:
if (!filterKey || !networkFilterFn) { return; }
nit: drop {} and place return on next line.

https://codereview.chromium.org/1383503003/

mmc...@chromium.org

unread,
Oct 9, 2015, 12:22:03 PM10/9/15
to est...@chromium.org, lga...@chromium.org, paul...@chromium.org, dgo...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
Done.

Thanks for the comments guys, changes made.

https://codereview.chromium.org/1383503003/

dgo...@chromium.org

unread,
Oct 9, 2015, 2:11:34 PM10/9/15
to mmc...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org

https://codereview.chromium.org/1383503003/diff/80001/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
File
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1383503003/diff/80001/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js#newcode647
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:647:
var explanation = this._addExplanation(mixedContentExplanation);
Revert all the changes here?

https://codereview.chromium.org/1383503003/

mmc...@chromium.org

unread,
Oct 9, 2015, 2:31:59 PM10/9/15
to dgo...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
Done!

https://codereview.chromium.org/1383503003/

dgo...@chromium.org

unread,
Oct 9, 2015, 2:46:55 PM10/9/15
to mmc...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
lgtm



https://codereview.chromium.org/1383503003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 9, 2015, 3:07:41 PM10/9/15
to mmc...@chromium.org, dgo...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1383503003/100001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1383503003/100001


https://codereview.chromium.org/1383503003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 9, 2015, 4:17:10 PM10/9/15
to mmc...@chromium.org, dgo...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
Try jobs failed on following builders:
win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/118840)

https://codereview.chromium.org/1383503003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 12, 2015, 12:54:54 PM10/12/15
to mmc...@chromium.org, dgo...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1383503003/120001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1383503003/120001


https://codereview.chromium.org/1383503003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 12, 2015, 2:14:08 PM10/12/15
to mmc...@chromium.org, dgo...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
Committed patchset #7 (id:120001)

https://codereview.chromium.org/1383503003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 12, 2015, 2:14:54 PM10/12/15
to mmc...@chromium.org, dgo...@chromium.org, est...@chromium.org, lga...@chromium.org, paul...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, est...@chromium.org, kozyatins...@chromium.org, lga...@chromium.org, lushnik...@chromium.org, pfel...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org, yu...@chromium.org
Patchset 7 (id:??) landed as
https://crrev.com/eefb629f1a7f6d05d3fede01f3dc0fb9b7f64e28
Cr-Commit-Position: refs/heads/master@{#353558}

https://codereview.chromium.org/1383503003/
Reply all
Reply to author
Forward
0 new messages