Created HtmlDialogTabContentsDelegate, which encapsulates the

Created HtmlDialogTabContentsDelegate, which encapsulates the TabContentsDelegate behavior for HTML dialogs.  Made all three implementations (gtk, win32, and cocoa) use it.

This also makes HTML dialogs not tied to a Browser instance.  Also, unlike the current behavior, any links followed from an HTML dialog box will be opened in a non-incognito browser, even if the HTML dialog was launched from an incognito browser.  According to beng, this is okay.

BUG=28609
TEST=unittests, manual testing on linux/windows/os x with sync setup wizard

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

[akalin, 2009-12-02 02:08:00]

+tim for views/ changes, + zork for gtk changes, +shess for cocoa changes and
everything else.

[tim (not reviewing), 2009-12-02 05:31:06]

I'm still looking, but wanted to say thanks for doing this!

http://codereview.chromium.org/441011/diff/8007/8011
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h (right):

http://codereview.chromium.org/441011/diff/8007/8011#newcode31
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:31: void
IgnoreAllFollowingTabContentsEvents();
Did you consider/ do you like the name 'Detach()' instead ? Seems to line up
with the actual impl...

http://codereview.chromium.org/441011/diff/8007/8011#newcode62
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:62: Profile* profile_;
 // weak, non-incognito
not sure about the comment here.. maybe just 'The original profile' (original is
just the common term used for non-incognito).  Remind me why we need to be
obsessed with having the original profile in the first place?

[akalin (wrong akalin), 2009-12-02 09:29:28]

On Tue, Dec 1, 2009 at 9:31 PM,  <tim@chromium.org> wrote:
> http://codereview.chromium.org/441011/diff/8007/8011#newcode62
> chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:62: Profile*
> profile_;  // weak, non-incognito
> not sure about the comment here.. maybe just 'The original profile'
> (original is just the common term used for non-incognito).  Remind me
> why we need to be obsessed with having the original profile in the first
> place?

Incognito profiles are deleted when the last incognito window is
closed, so if we hang on to it we'll end up dereferencing freed memory
in some cases (on OS X).

-- 
Frederick Akalin
Software Engineer

[Zachary Kuznia, 2009-12-02 20:00:23]

LGTM on the gtk changes.

http://codereview.chromium.org/441011/diff/8007/8010
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/441011/diff/8007/8010#newcode1
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:1: // Copyright (c)
2006-2008 The Chromium Authors. All rights reserved.
2009

http://codereview.chromium.org/441011/diff/8007/8011
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h (right):

http://codereview.chromium.org/441011/diff/8007/8011#newcode1
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:1: // Copyright (c)
2006-2008 The Chromium Authors. All rights reserved.
2009

[akalin, 2009-12-02 22:25:01]

On 2009/12/02 05:31:06, timsteele wrote:
> I'm still looking, but wanted to say thanks for doing this!
> 
> http://codereview.chromium.org/441011/diff/8007/8011#newcode31
> chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:31: void
> IgnoreAllFollowingTabContentsEvents();
> Did you consider/ do you like the name 'Detach()' instead ? Seems to line up
> with the actual impl...

That's a better name.  Changed.

> http://codereview.chromium.org/441011/diff/8007/8011#newcode62
> chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:62: Profile*
profile_;
>  // weak, non-incognito
> not sure about the comment here.. maybe just 'The original profile' (original
is
> just the common term used for non-incognito).  Remind me why we need to be
> obsessed with having the original profile in the first place?

Fixed.

> http://codereview.chromium.org/441011/diff/8007/8010
> File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):
> 
> http://codereview.chromium.org/441011/diff/8007/8010#newcode1
> chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:1: // Copyright (c)
> 2006-2008 The Chromium Authors. All rights reserved.
> 2009

Fixed.

> http://codereview.chromium.org/441011/diff/8007/8011
> File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h (right):
> 
> http://codereview.chromium.org/441011/diff/8007/8011#newcode1
> chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:1: // Copyright (c)
> 2006-2008 The Chromium Authors. All rights reserved.
> 2009

Fixed.

Tim, shess, please take another look.

[tim (not reviewing), 2009-12-03 02:57:31]

views and dom_ui LGTM

http://codereview.chromium.org/441011/diff/12006/14019
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h (right):

http://codereview.chromium.org/441011/diff/12006/14019#newcode62
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:62: Profile* profile_;
 // weak, always an original profile
Still not sure what 'weak' is about, but it should be capital and a period at
the end either way :)

[Scott Hess - ex-Googler, 2009-12-03 19:17:04]

LGTM, except that that I think you're breaking the multiple-inheritance style
guidelines in HtmlDialogView (and possibly others).  I don't claim enough C++ Fu
to make any sort of ruling or suggestion on this.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Multiple_Inher...

[akalin (wrong akalin), 2009-12-03 23:18:23]

On Wed, Dec 2, 2009 at 6:57 PM,  <tim@chromium.org> wrote:
> views and dom_ui LGTM
>
> http://codereview.chromium.org/441011/diff/12006/14019#newcode62
> chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:62: Profile*
> profile_;  // weak, always an original profile
> Still not sure what 'weak' is about, but it should be capital and a
> period at the end either way :)

'weak' as in 'weak pointer/reference'.  Capitalized and added period.

On Thu, Dec 3, 2009 at 11:17 AM,  <shess@chromium.org> wrote:
> LGTM, except that that I think you're breaking the multiple-inheritance
> style
> guidelines in HtmlDialogView (and possibly others).  I don't claim enough
> C++ Fu
> to make any sort of ruling or suggestion on this.

Oh, you're right.  That's easy enough to fix (just make HtmlDialogView
contain a HtmlDialogTabContentsDelegate instance instead of
inheriting, which is really what it should do anyway), but I'd prefer
to do it in a follow-up CL.  Added TODO.

Submitting soon!

-- 
Frederick Akalin
Software Engineer