[DevTools] Show a explanation when all resources in the page have been requested securely (no-mixed…

[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
TEST=Visit 'https://sha256.badssl.com/', enable/open the Security Panel, verify there is an explanation in the list reading 'All resources on this page are served securely.' with a green bullet (secure) annotation.

Committed: https://crrev.com/eefb629f1a7f6d05d3fede01f3dc0fb9b7f64e28
Cr-Commit-Position: refs/heads/master@{#353558}

[mmccoy, 2015-10-01 13:35:28]

mmccoy@chromium.org changed reviewers:
+ yurys@chromium.org

[mmccoy, 2015-10-01 13:35:38]

The CQ bit was checked by mmccoy@chromium.org

[commit-bot: I haz the power, 2015-10-01 13:36:09]

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

[commit-bot: I haz the power, 2015-10-01 13:36:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 13:36:11]

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.

[mmccoy, 2015-10-05 13:37:11]

mmccoy@chromium.org changed reviewers:
+ alph@chromium.org
- yurys@chromium.org

[paulirish, 2015-10-05 21:28:08]

paulirish@chromium.org changed reviewers:
+ estark@chromium.org, lgarron@chromium.org
- alph@chromium.org

[paulirish, 2015-10-05 21:28:26]

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

[paulirish, 2015-10-05 21:28:27]

lucas to review. after lgtm add dgozman to review.

[mmccoy, 2015-10-08 19:21:40]

On 2015/10/05 at 21:28:27, paulirish wrote:
> lucas to review. after lgtm add dgozman to review.

bump! :)

[lgarron, 2015-10-09 00:16:31]

LGTM; thanks for the poke today.

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

https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Sour...
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

[mmccoy, 2015-10-09 00:29:56]

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

[mmccoy, 2015-10-09 00:31:18]

On 2015/10/09 at 00:16:31, lgarron wrote:
> LGTM; thanks for the poke today.
> 
>
https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):
> 
>
https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Sour...
> 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

Thanks, Lucas!

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

[dgozman, 2015-10-09 03:52:55]

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

https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Sour...
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/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:594: if
(!filterKey || !networkFilterFn) { return; }
nit: drop {} and place return on next line.

[mmccoy, 2015-10-09 16:22:03]

On 2015/10/09 at 03:52:55, dgozman wrote:
>
https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):
> 
>
https://codereview.chromium.org/1383503003/diff/60001/third_party/WebKit/Sour...
> 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/Sour...
> third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:594: if
(!filterKey || !networkFilterFn) { return; }
> nit: drop {} and place return on next line.

Done.

Thanks for the comments guys, changes made.

[dgozman, 2015-10-09 18:11:34]

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

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

[mmccoy, 2015-10-09 18:31:59]

On 2015/10/09 at 18:11:34, dgozman wrote:
>
https://codereview.chromium.org/1383503003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):
> 
>
https://codereview.chromium.org/1383503003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:647:
var explanation = this._addExplanation(mixedContentExplanation);
> Revert all the changes here?

Done!

[dgozman, 2015-10-09 18:46:54]

lgtm

[mmccoy, 2015-10-09 19:03:49]

The CQ bit was checked by mmccoy@chromium.org

[mmccoy, 2015-10-09 19:03:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
lgarron@chromium.org, paulirish@chromium.org
Link to the patchset: https://codereview.chromium.org/1383503003/#ps100001
(title: "Reverting additional logic in _addMixedContentExplanation")

[commit-bot: I haz the power, 2015-10-09 19:07:41]

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

[commit-bot: I haz the power, 2015-10-09 20:17:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-09 20:17:10]

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_...)

[mmccoy, 2015-10-12 16:53:49]

The CQ bit was checked by mmccoy@chromium.org

[mmccoy, 2015-10-12 16:53:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
lgarron@chromium.org, dgozman@chromium.org, paulirish@chromium.org
Link to the patchset: https://codereview.chromium.org/1383503003/#ps120001
(title: "Add additional expected output to block-mixed-content layout test")

[commit-bot: I haz the power, 2015-10-12 16:54:53]

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

[commit-bot: I haz the power, 2015-10-12 18:14:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-12 18:14:53]

Patchset 7 (id:??) landed as
https://crrev.com/eefb629f1a7f6d05d3fede01f3dc0fb9b7f64e28
Cr-Commit-Position: refs/heads/master@{#353558}