Fix AppModalDialogHelper to deal with a TabStrip without an active tab

Fix AppModalDialogHelper to deal with a TabStrip without an active tab

BUG=444540
Committed: https://crrev.com/d62556a407054de867016fb5bb3af1437eb7b763
Cr-Commit-Position: refs/heads/master@{#309425}

[Bernhard Bauer, 2014-12-22 13:18:34]

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

[Bernhard Bauer, 2014-12-22 13:18:36]

Wait, how can the dialog be null? In the stack trace on crash/,
JavaScriptAppModalDialog::CreateNativeDialog() calls
CreateNativeJavaScriptDialog(this, parent_window), but CreateNativeDialog() is a
virtual call, so |this| can't be null (and if it were, we should probably still
bail out earlier).

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/blocked_co...
File chrome/browser/ui/blocked_content/app_modal_dialog_helper.cc (right):

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/blocked_co...
chrome/browser/ui/blocked_content/app_modal_dialog_helper.cc:19: if
(dialog_host) {
Why do you even construct this object if you don't do anything for a null dialog
host?

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/cocoa/java...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/cocoa/java...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:228: new
AppModalDialogHelper(dialog ? dialog->web_contents() : NULL)),
nullptr

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/cocoa/java...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:228: new
AppModalDialogHelper(dialog ? dialog->web_contents() : NULL)),
Also, instead of doing this `dialog ? dialog->web_contents() : nullptr` thing
everywhere, why don't you just pass the JavaScriptAppModalDialog* in?

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/cocoa/java...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:237: switch
(dialog_->javascript_message_type()) {
|dialog_| might be NULL here.

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/views/chro...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/819083002/diff/1/chrome/browser/ui/views/chro...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:29: new
AppModalDialogHelper(parent ? parent->web_contents() : NULL)) {}
nullptr

[jochen (gone - plz use gerrit), 2014-12-22 13:40:42]

windows had more useful stack traces. turns out that we can have tab strips
without active tabs

[tfarina, 2014-12-22 13:56:44]

On 2014/12/22 13:40:42, jochen (slow) wrote:
> windows had more useful stack traces. turns out that we can have tab strips
> without active tabs

Is worth describing how one can get to this in the CL description? Or is this
security sensitive information?

[jochen (gone - plz use gerrit), 2014-12-22 13:59:56]

On 2014/12/22 at 13:56:44, tfarina wrote:
> On 2014/12/22 13:40:42, jochen (slow) wrote:
> > windows had more useful stack traces. turns out that we can have tab strips
> > without active tabs
> 
> Is worth describing how one can get to this in the CL description? Or is this
security sensitive information?

i don't know how to get into that state

[Bernhard Bauer, 2014-12-22 14:23:55]

Interesting that this can happen...

LGTM

[jochen (gone - plz use gerrit), 2014-12-22 14:26:39]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-12-22 14:27:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/819083002/20001

[commit-bot: I haz the power, 2014-12-22 15:08:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-22 15:09:49]

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