Remove JS dialog dependency from content.

Remove JS dialog dependency from content.

BUG=71097
TEST=all types of javascript dialogs work, onbeforeunload dialogs work too

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

[jam, 2011-06-01 19:20:07]

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
File content/browser/content_browser_client.cc (right):

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
content/browser/content_browser_client.cc:112: IPC::Message* reply_msg) {
shouldn't the default implementation call delegate?

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
content/browser/content_browser_client.h:117: virtual void
RunJavaScriptDialog(JavaScriptDialogDelegate* delegate,
i tend to think of ContentBrowserClient as a last-resort, where code that has no
context to reach into the embedder uses.  why not have TabContents call out to
TabContentsDelegate in this case?

[Avi (use Gerrit), 2011-06-01 19:40:34]

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
File content/browser/content_browser_client.cc (right):

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
content/browser/content_browser_client.cc:112: IPC::Message* reply_msg) {
On 2011/06/01 19:20:08, John Abd-El-Malek wrote:
> shouldn't the default implementation call delegate?

Done.

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7096016/diff/3033/content/browser/content_brow...
content/browser/content_browser_client.h:117: virtual void
RunJavaScriptDialog(JavaScriptDialogDelegate* delegate,
Lemme poke at that.

[Avi (use Gerrit), 2011-06-02 23:14:17]

New version; ptal.

[jam, 2011-06-03 00:21:45]

lgtm

http://codereview.chromium.org/7096016/diff/9001/chrome/browser/external_tab_...
File chrome/browser/external_tab_container_win.h (right):

http://codereview.chromium.org/7096016/diff/9001/chrome/browser/external_tab_...
chrome/browser/external_tab_container_win.h:173: virtual
content::JavaScriptDialogCreator* GetJavaScriptDialogCreator(
this should be with the other TabContentsDelegate methods right?  (i.e. at 169,
and looks like ShowRepostFormWarningDialog is missing a virtual)

http://codereview.chromium.org/7096016/diff/9001/chrome/browser/external_tab_...
chrome/browser/external_tab_container_win.h:174: ) OVERRIDE;
nit: this seems awkward to read.  do you think it'd be more readable if the
function name was on the following line?

http://codereview.chromium.org/7096016/diff/9001/chrome/browser/ui/app_modal_...
File chrome/browser/ui/app_modal_dialogs/message_box_handler.cc (right):

http://codereview.chromium.org/7096016/diff/9001/chrome/browser/ui/app_modal_...
chrome/browser/ui/app_modal_dialogs/message_box_handler.cc:28: class
JavaScriptDialogCreatorImpl : public content::JavaScriptDialogCreator {
nit: ChromeJavaScriptDialogCreator for consistency with other implementations of
content interfaces?

http://codereview.chromium.org/7096016/diff/9001/chrome/browser/ui/app_modal_...
chrome/browser/ui/app_modal_dialogs/message_box_handler.cc:30: static
JavaScriptDialogCreatorImpl* GetInstance();
nit: if you're going to have the class in the cc file, then there's not much
point in declaring the class first and then the methods later.

http://codereview.chromium.org/7096016/diff/9001/content/browser/javascript_d...
File content/browser/javascript_dialogs.h (right):

http://codereview.chromium.org/7096016/diff/9001/content/browser/javascript_d...
content/browser/javascript_dialogs.h:23: class JavaScriptDialogDelegate {
can you give a brief description of each class in a comment?  and also, it seems
that they should each be in a separate file?

[Avi (use Gerrit), 2011-06-03 17:36:44]

Reformatted. I have some followup ideas.