Remove one content::SSLStatus usage in the renderer.

Remove one content::SSLStatus usage in the renderer.

Move the console warning when visiting localhost sites with bad certs can be done in the browser. Also remove the DHE warning since it's not needed anymore per the bug.

BUG=598073, 598109
Committed: https://crrev.com/661e32a163aa0561de98a31e3d0bc43314e301dc
Cr-Commit-Position: refs/heads/master@{#408474}

[jam, 2016-07-27 23:13:54]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 23:14:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-07-27 23:16:16]

Description was changed from

==========
Remove one content::SSLStatus usage in the renderer.

Move the console warning when visiting localhost sites with bad certs can be
done in the browser. Also remove the DHE warning since it's not needed anymore
per the bug.

BUG=598073,598109
==========

to

==========
Remove one content::SSLStatus usage in the renderer.

Move the console warning when visiting localhost sites with bad certs can be
done in the browser. Also remove the DHE warning since it's not needed anymore
per the bug.

BUG=598073,598109
==========

[jam, 2016-07-27 23:16:16]

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

[commit-bot: I haz the power, 2016-07-28 00:01:56]

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

[commit-bot: I haz the power, 2016-07-28 00:01:56]

Dry run: This issue passed the CQ dry run.

[estark, 2016-07-28 16:29:53]

https://codereview.chromium.org/2190783002/diff/1/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2190783002/diff/1/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/core_tab_helper.cc:260: content::NavigationEntry*
entry =
Do we need to check for a null |entry| here?

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_frame_observer.cc (left):

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_frame_observer.cc:32: #include
"net/ssl/ssl_cipher_suite_names.h"
If you can indeed remove the DHE console warning (see comment below), then I
think you can get rid of this and line 33 as well.

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_frame_observer.cc:277: // DHE is deprecated and
will be removed in M52. See https://crbug.com/598109.
I'm not sure we can remove this yet. Per the description on
https://codereview.chromium.org/2056343006, there's still a field trial and
admin policy to re-enable DHE. Maybe you can add davidben to check?

[jam, 2016-07-28 16:59:12]

jam@chromium.org changed reviewers:
+ davidben@chromium.org

[jam, 2016-07-28 16:59:13]

https://codereview.chromium.org/2190783002/diff/1/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2190783002/diff/1/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/core_tab_helper.cc:260: content::NavigationEntry*
entry =
On 2016/07/28 16:29:53, estark wrote:
> Do we need to check for a null |entry| here?

I don't think so, since this is updated when a provisional load is committed.

RenderFrameHostImpl::OnDidCommitProvisionalLoad which updates the last committed
entry is always fired before RenderFrameHostImpl::OnDidCommitProvisionalLoad
which is what fires this callback

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_frame_observer.cc (left):

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_frame_observer.cc:32: #include
"net/ssl/ssl_cipher_suite_names.h"
On 2016/07/28 16:29:53, estark wrote:
> If you can indeed remove the DHE console warning (see comment below), then I
> think you can get rid of this and line 33 as well.

Done.

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_frame_observer.cc:277: // DHE is deprecated and
will be removed in M52. See https://crbug.com/598109.
On 2016/07/28 16:29:53, estark wrote:
> I'm not sure we can remove this yet. Per the description on
> https://codereview.chromium.org/2056343006, there's still a field trial and
> admin policy to re-enable DHE. Maybe you can add davidben to check?

ah, my reading of the comment made it appear that it will go away for sure.

Since this is just a developer comment tho, it seems it can go away. Either we
remove it, and then it's not needed. Or we decide to bring it back, in which
case the warning doesn't apply anymore.

Sure, adding davidben.

[jam, 2016-07-28 16:59:58]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 17:00:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-07-28 17:06:17]

(Haven't looked at the rest of the CL, just the read I was added for.)

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_frame_observer.cc (left):

https://codereview.chromium.org/2190783002/diff/1/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_frame_observer.cc:277: // DHE is deprecated and
will be removed in M52. See https://crbug.com/598109.
On 2016/07/28 16:59:12, jam wrote:
> On 2016/07/28 16:29:53, estark wrote:
> > I'm not sure we can remove this yet. Per the description on
> > https://codereview.chromium.org/2056343006, there's still a field trial and
> > admin policy to re-enable DHE. Maybe you can add davidben to check?
> 
> ah, my reading of the comment made it appear that it will go away for sure.
> 
> Since this is just a developer comment tho, it seems it can go away. Either we
> remove it, and then it's not needed. Or we decide to bring it back, in which
> case the warning doesn't apply anymore.
> 
> Sure, adding davidben.

It's possible we'll have bring it back and/or try again later or give up, but
lgtm to take it out. If nothing else, the version and date in there will be a
lie at that point anyway. :-)

[commit-bot: I haz the power, 2016-07-28 17:56:36]

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

[commit-bot: I haz the power, 2016-07-28 17:56:36]

Dry run: This issue passed the CQ dry run.

[estark, 2016-07-28 19:59:35]

lgtm

https://codereview.chromium.org/2190783002/diff/1/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2190783002/diff/1/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/core_tab_helper.cc:260: content::NavigationEntry*
entry =
On 2016/07/28 16:59:12, jam wrote:
> On 2016/07/28 16:29:53, estark wrote:
> > Do we need to check for a null |entry| here?
> 
> I don't think so, since this is updated when a provisional load is committed.
> 
> RenderFrameHostImpl::OnDidCommitProvisionalLoad which updates the last
committed
> entry is always fired before RenderFrameHostImpl::OnDidCommitProvisionalLoad
> which is what fires this callback

Ah, gotcha, thanks for the explanation.

https://codereview.chromium.org/2190783002/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_frame_observer.cc (left):

https://codereview.chromium.org/2190783002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_frame_observer.cc:33: #include
"net/ssl/ssl_connection_status_flags.h"
This can go too, I believe.

[jam, 2016-07-28 20:14:42]

The CQ bit was checked by jam@chromium.org

[jam, 2016-07-28 20:14:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2190783002/#ps40001
(title: "remove unneeded header")

[commit-bot: I haz the power, 2016-07-28 20:15:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 21:21:00]

Description was changed from

==========
Remove one content::SSLStatus usage in the renderer.

Move the console warning when visiting localhost sites with bad certs can be
done in the browser. Also remove the DHE warning since it's not needed anymore
per the bug.

BUG=598073,598109
==========

to

==========
Remove one content::SSLStatus usage in the renderer.

Move the console warning when visiting localhost sites with bad certs can be
done in the browser. Also remove the DHE warning since it's not needed anymore
per the bug.

BUG=598073,598109
==========

[commit-bot: I haz the power, 2016-07-28 21:21:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 21:24:04]

Description was changed from

==========
Remove one content::SSLStatus usage in the renderer.

Move the console warning when visiting localhost sites with bad certs can be
done in the browser. Also remove the DHE warning since it's not needed anymore
per the bug.

BUG=598073,598109
==========

to

==========
Remove one content::SSLStatus usage in the renderer.

Move the console warning when visiting localhost sites with bad certs can be
done in the browser. Also remove the DHE warning since it's not needed anymore
per the bug.

BUG=598073,598109

Committed: https://crrev.com/661e32a163aa0561de98a31e3d0bc43314e301dc
Cr-Commit-Position: refs/heads/master@{#408474}
==========

[commit-bot: I haz the power, 2016-07-28 21:24:06]

Patchset 3 (id:??) landed as
https://crrev.com/661e32a163aa0561de98a31e3d0bc43314e301dc
Cr-Commit-Position: refs/heads/master@{#408474}