Plausible fix for crash in GetCertificateChain.

Plausible fix for crash in GetCertificateChain.

The call to get the certificate chain from native assumes a valid
ContentViewCore. Switch to using WebContents which will get properly
invalidated/handled if the tab which launches this dialog closes before
the dialog is presented.

Also includes a slight change to avoid bouncing Java->native->Java by
allowing the dialog to be created from Java.

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

[Yaron, 2014-06-11 22:50:33]

https://codereview.chromium.org/330583002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(left):

https://codereview.chromium.org/330583002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:51:
mDialog.setOnCancelListener(new DialogInterface.OnCancelListener() {
I think this leak was always present. It's true that I moved construction from
c++ to java, but if you look at the previous static |Show| method, it was
already unmanaged.

[Ted C, 2014-06-11 23:32:41]

lgtm!  I was thinking of doing this exact change recently...laziness paid off

https://codereview.chromium.org/330583002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/330583002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:155:
return new WebsiteSettingsPopup(context, webContents);
any reason for having this return the popup?  It isn't really actionable and is
driven entirely by native.

I would probably just say call this method show (rename the native method to
showDialog) and make it a bit easier to read in the embedders.  Also, javadoc
:-)

https://codereview.chromium.org/330583002/diff/20001/chrome/browser/ui/androi...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/330583002/diff/20001/chrome/browser/ui/androi...
chrome/browser/ui/android/website_settings_popup_android.cc:37: if
(!web_contents) {
personally I would drop the braces.

https://codereview.chromium.org/330583002/diff/20001/chrome/browser/ui/androi...
chrome/browser/ui/android/website_settings_popup_android.cc:74:
content::WebContents::FromJavaWebContents(java_web_contents);
this is indented 5 instead of 4

[Yaron, 2014-06-12 01:04:28]

thanks

https://codereview.chromium.org/330583002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/330583002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:155:
return new WebsiteSettingsPopup(context, webContents);
On 2014/06/11 23:32:41, Ted C wrote:
> any reason for having this return the popup?  It isn't really actionable and
is
> driven entirely by native.
> 
> I would probably just say call this method show (rename the native method to
> showDialog) and make it a bit easier to read in the embedders.  Also, javadoc
> :-) 

Done.

https://codereview.chromium.org/330583002/diff/20001/chrome/browser/ui/androi...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/330583002/diff/20001/chrome/browser/ui/androi...
chrome/browser/ui/android/website_settings_popup_android.cc:37: if
(!web_contents) {
On 2014/06/11 23:32:41, Ted C wrote:
> personally I would drop the braces.

Done.

https://codereview.chromium.org/330583002/diff/20001/chrome/browser/ui/androi...
chrome/browser/ui/android/website_settings_popup_android.cc:74:
content::WebContents::FromJavaWebContents(java_web_contents);
On 2014/06/11 23:32:41, Ted C wrote:
> this is indented 5 instead of 4

Done.

[Yaron, 2014-06-12 01:04:31]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-06-12 01:05:54]

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

[commit-bot: I haz the power, 2014-06-12 06:47:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  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...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-12 13:10:01]

Change committed as 276653