Remove use of ResourceRequestInfo::GetRequestID from cert enrollment.

Remove use of ResourceRequestInfo::GetRequestID from cert enrollment.

That use is both wrong (request IDs are global) and unnecessary since cert
enrollment is not a multi-step process. There's some stub code and indirections
for prompting to add a cert that's never actually used. Collapse it all to
simplify things. Move other random things out of SSLTabHelper which is also
no longer necessary.

(If anything, this stuff should probably go through the downloads
infrastructure to inherit all the user-gesture and throttling checks.)

Also move the AddCertificate hook from render_view_id to render_frame_id.

BUG=376003
TEST=Visit https://davidben.net/mixed-content-test.html; Click button. Lock
     icon should change to lock icon with warning.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275503

[davidben, 2014-05-27 21:07:12]

Some of this SSLTabHelper stuff was written by an intern who I think wasn't
entirely sure what he was doing. I say this because that intern was me. :-)

All of that state I think was to support some kind of prompting for adding a
certificate which we don't do anymore. It doesn't seem worth keeping all that
scaffolding around, and if we want to play games with prompting and such, this
probably should go through the downloads system to inherit whatever user gesture
and request limiting mechanisms we have anyway.

I have one question below about the last bit of SSLTabHelper. I think we should
be able to do away with it altogether at this point since it doesn't have much
purpose in life anymore. (Its original purpose was to track that request ID map
which isn't used and makes incorrect assumptions about request IDs being global
anyway.)

https://codereview.chromium.org/302653006/diff/1/chrome/browser/ssl/ssl_add_c...
File chrome/browser/ssl/ssl_add_certificate.cc (right):

https://codereview.chromium.org/302653006/diff/1/chrome/browser/ssl/ssl_add_c...
chrome/browser/ssl/ssl_add_certificate.cc:33: class
SSLAddCertificateInfoBarDelegate : public ConfirmInfoBarDelegate {
Is this layering alright? To have ChromeContentBrowserClient call some function
in chrome/browser/ssl which includes various bits of UI code? The dependencies
shouldn't be any worse than before---I'm just collapsing some of the bits in the
middle, but I dunno if this is correct for chrome/.

https://codereview.chromium.org/302653006/diff/1/chrome/browser/ssl/ssl_tab_h...
File chrome/browser/ssl/ssl_tab_helper.cc (right):

https://codereview.chromium.org/302653006/diff/1/chrome/browser/ssl/ssl_tab_h...
chrome/browser/ssl/ssl_tab_helper.cc:23:
browser->VisibleSSLStateChanged(web_contents());
Given that SSLTabHelper's only purpose in life at this point is to plumb some
WebContentsObserver signal to the Browser, I rather suspect we can get rid of
it. Is the preferred way to do this to add this hook to WebContentsDelegate,
make Browser a WebContentsObserver, or add some dummy object to be the
WebContentsObserver as this guy is now?

https://codereview.chromium.org/302653006/diff/1/content/browser/loader/certi...
File content/browser/loader/certificate_resource_handler.cc (right):

https://codereview.chromium.org/302653006/diff/1/content/browser/loader/certi...
content/browser/loader/certificate_resource_handler.cc:121: info->GetChildID(),
info->GetRenderFrameID());
I suspect this'll need to change again later if we go with not associating the
navigation request with an RFH, but in the meantime, here's one fewer
render_view_id consumer.

[davidben, 2014-05-28 19:29:36]

Updated to remove SSLTabHelper. Per conversation, DidChangeVisibleSSLState is
now on the WebContentsDelegate.

+benm for Android-side changes; this'll require downstream changes because a
hook moved from WebContentsObserver to TabObserver. I have the downstream
changes ready, though I don't know how you usually coordinate this kind of
thing.

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (left):

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:420: #endif  // !defined(OS_ANDROID)
This block isn't necessary. InterstitialPageImpl::Show calls
controller_->SetTransientEntry right after calling OverrideEntry. That triggers
a NavigiationStateChanged which will update the toolbar as appropriate.

[jam, 2014-05-28 20:06:38]

lgtm, please get a reviewer from chrome/browser/ssl/OWNERS to look at those
files too

[davidben, 2014-05-28 20:13:42]

+wtc for chrome/browser/ssl. Squashing a lot of things since we never ended up
using any of that machinery.

[benm (inactive), 2014-05-29 09:25:07]

aw lgtm

[wtc, 2014-05-29 19:31:46]

chrome/browser/ssl in patch set 3 LGTM.

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
File chrome/browser/ssl/ssl_add_certificate.cc (right):

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
chrome/browser/ssl/ssl_add_certificate.cc:35: // Creates an SSL cert result
infobar and delegate.

Nit: I don't understand what "SSL cert result" means. Did you mean "SSL cert
enrollment result" or "SSL add cert result"?

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
chrome/browser/ssl/ssl_add_certificate.cc:45: : ConfirmInfoBarDelegate(),

Nit: I think this base class initializer can be omitted since it doesn't take
any argument.

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
chrome/browser/ssl/ssl_add_certificate.cc:81: return false;  // Hiding the
infobar just as the dialog opens looks weird.

Nit: I had a little trouble understanding this comment. I guess it should be
parsed as:
  Hiding the infobar just as the dialog opens, looks weird.

But the use of that comma is uncommon in English. Perhaps we can rephrase it as:
  It looks weird to hide the infobar just as the dialog opens.

[davidben, 2014-05-29 19:51:01]

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
File chrome/browser/ssl/ssl_add_certificate.cc (right):

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
chrome/browser/ssl/ssl_add_certificate.cc:35: // Creates an SSL cert result
infobar and delegate.
On 2014/05/29 19:31:46, wtc wrote:
> 
> Nit: I don't understand what "SSL cert result" means. Did you mean "SSL cert
> enrollment result" or "SSL add cert result"?

Done.

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
chrome/browser/ssl/ssl_add_certificate.cc:45: : ConfirmInfoBarDelegate(),
On 2014/05/29 19:31:46, wtc wrote:
> 
> Nit: I think this base class initializer can be omitted since it doesn't take
> any argument.

Done.

https://codereview.chromium.org/302653006/diff/40001/chrome/browser/ssl/ssl_a...
chrome/browser/ssl/ssl_add_certificate.cc:81: return false;  // Hiding the
infobar just as the dialog opens looks weird.
On 2014/05/29 19:31:46, wtc wrote:
> 
> Nit: I had a little trouble understanding this comment. I guess it should be
> parsed as:
>   Hiding the infobar just as the dialog opens, looks weird.
> 
> But the use of that comma is uncommon in English. Perhaps we can rephrase it
as:
>   It looks weird to hide the infobar just as the dialog opens.

Done. (That comment was just copied over actually.)

[wtc, 2014-05-29 20:11:15]

chrome/browser/ssl in patch set 4 LGTM.

[davidben, 2014-06-05 17:09:16]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-06-05 17:10:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/302653006/60001

[commit-bot: I haz the power, 2014-06-05 17:39:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-05 17:42:16]

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

[commit-bot: I haz the power, 2014-06-05 17:42:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[davidben, 2014-06-05 19:13:26]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-06-05 19:14:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/302653006/80001

[commit-bot: I haz the power, 2014-06-05 20:09:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13481)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/16570)

[commit-bot: I haz the power, 2014-06-05 20:13:04]

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

[commit-bot: I haz the power, 2014-06-05 20:13:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[davidben, 2014-06-06 18:26:59]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-06-06 18:27:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/302653006/100001

[commit-bot: I haz the power, 2014-06-06 19:35:25]

Change committed as 275503

[Yusuf, 2014-06-09 18:14:14]

Actually the API removed WebContentsObserverAndroid and TabObserver was being
used downstream. How should we be getting this information now?

And for future cases, would you mind getting an Android OWNER approval also? So
that we are informed of coming changes and make preparations for the roll?

[gone, 2014-06-09 18:28:17]

On 2014/06/09 18:14:14, Yusuf wrote:
> Actually the API removed WebContentsObserverAndroid and TabObserver was being
> used downstream. How should we be getting this information now?
> 
> And for future cases, would you mind getting an Android OWNER approval also?
So
> that we are informed of coming changes and make preparations for the roll?

Heads up: https://chrome-internal-review.googlesource.com/#/c/164847/

[Yusuf, 2014-06-09 18:30:09]

On 2014/06/09 18:28:17, dfalcantara wrote:
> On 2014/06/09 18:14:14, Yusuf wrote:
> > Actually the API removed WebContentsObserverAndroid and TabObserver was
being
> > used downstream. How should we be getting this information now?
> > 
> > And for future cases, would you mind getting an Android OWNER approval also?
> So
> > that we are informed of coming changes and make preparations for the roll?
> 
> Heads up: https://chrome-internal-review.googlesource.com/#/c/164847/

False alarm on my end. Sorry about that.

[davidben, 2014-06-09 18:32:22]

On 2014/06/09 18:14:14, Yusuf wrote:
> Actually the API removed WebContentsObserverAndroid and TabObserver was being
> used downstream. How should we be getting this information now?

I moved it to the TabObserver and made a companion CL to update downstream
Chrome for Android. Should it have been elsewhere?

> And for future cases, would you mind getting an Android OWNER approval also?
So
> that we are informed of coming changes and make preparations for the roll?

Hrm. I added benm@ based on I think
components/web_contents_delegate_android/OWNERS (or maybe it was just what git
cl upload chose, not sure). But I see now he only wrote "aw lgtm" for I guess
Android WebView? So that was miscommunicated. Sorry about that.

I did ping the Android sheriffs though. The downstream changes should have
happened in tandem. I made the relevant change and made sure to get that
reviewed before landing this one. It should have been taken care of, unless I
missed something.