Make a clean interface for dialog callbacks.

For JavaScript dialogs, remove As[Type] members, expand delegates to handle those cases.

BUG=84604
TEST=no visible change

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91128

[Avi (use Gerrit), 2011-06-29 21:55:41]

John's out; here, have some fun.

[Evan Martin, 2011-06-29 22:01:52]

Can you make the review description more descriptive?  (E.g. "introduce a new
delegate class, remove AsFooBar() methods because we can now do X, Y, Z"

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
File chrome/browser/ui/app_modal_dialogs/app_modal_dialog_queue.h (right):

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
chrome/browser/ui/app_modal_dialogs/app_modal_dialog_queue.h:86:
std::deque<AppModalDialog*> app_modal_dialog_queue_;
This change is unrelated to the refactoring, no?

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
File chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h (right):

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h:84:
content::JavaScriptDialogDelegate* delegate_;
This is confusing; AppModalDialog already has a protected delegate_ member.

[Avi (use Gerrit), 2011-06-29 22:09:29]

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
File chrome/browser/ui/app_modal_dialogs/app_modal_dialog_queue.h (right):

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
chrome/browser/ui/app_modal_dialogs/app_modal_dialog_queue.h:86:
std::deque<AppModalDialog*> app_modal_dialog_queue_;
Required. The refactoring requires the ability to walk over the queue, and
std::queue doesn't let you.

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
File chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h (right):

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h:84:
content::JavaScriptDialogDelegate* delegate_;
On 2011/06/29 22:01:52, Evan Martin wrote:
> This is confusing; AppModalDialog already has a protected delegate_ member.

Right, but this as a subclass requires the expanded subclass delegate. I'm not
sure of a better clean way to do it; thoughts?

[Evan Martin, 2011-06-29 22:53:09]

What do you think of this?

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
File chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h (right):

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h:84:
content::JavaScriptDialogDelegate* delegate_;
On 2011/06/29 22:09:29, Avi wrote:
> On 2011/06/29 22:01:52, Evan Martin wrote:
> > This is confusing; AppModalDialog already has a protected delegate_ member.
> 
> Right, but this as a subclass requires the expanded subclass delegate. I'm not
> sure of a better clean way to do it; thoughts?

I polled my office and they came up with:

content::JavaScriptDialogDelegate* delegate() const {
  return static_cast<content::JavaScriptDialogDelegate*>(delegate_);
}

[Avi (use Gerrit), 2011-06-29 23:19:48]

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
File chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h (right):

http://codereview.chromium.org/7283022/diff/1/chrome/browser/ui/app_modal_dia...
chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h:84:
content::JavaScriptDialogDelegate* delegate_;
On 2011/06/29 22:53:09, Evan Martin wrote:
> On 2011/06/29 22:09:29, Avi wrote:
> > On 2011/06/29 22:01:52, Evan Martin wrote:
> > > This is confusing; AppModalDialog already has a protected delegate_
member.
> > 
> > Right, but this as a subclass requires the expanded subclass delegate. I'm
not
> > sure of a better clean way to do it; thoughts?
> 
> I polled my office and they came up with:
> 
> content::JavaScriptDialogDelegate* delegate() const {
>   return static_cast<content::JavaScriptDialogDelegate*>(delegate_);
> }

You were objecting to the storage rather than the override with the covariant
return type? Oh, I can do that. Done.

[Evan Martin, 2011-06-29 23:33:31]

LGTM

http://codereview.chromium.org/7283022/diff/6001/chrome/browser/ui/app_modal_...
File chrome/browser/ui/app_modal_dialogs/app_modal_dialog.h (right):

http://codereview.chromium.org/7283022/diff/6001/chrome/browser/ui/app_modal_...
chrome/browser/ui/app_modal_dialogs/app_modal_dialog.h:82: // and receive
results.
This sentence has one too many words, I think.  At least, I can't understand it.

[Avi (use Gerrit), 2011-06-29 23:46:38]

OK. Will land tomorrow; the tree is in no shape tonight.

http://codereview.chromium.org/7283022/diff/6001/chrome/browser/ui/app_modal_...
File chrome/browser/ui/app_modal_dialogs/app_modal_dialog.h (right):

http://codereview.chromium.org/7283022/diff/6001/chrome/browser/ui/app_modal_...
chrome/browser/ui/app_modal_dialogs/app_modal_dialog.h:82: // and receive
results.
On 2011/06/29 23:33:31, Evan Martin wrote:
> This sentence has one too many words, I think.  At least, I can't understand
it.

Done.