Change window opening behavior for HtmlDialogTabContentsDelegate.

Change window opening behavior for HtmlDialogTabContentsDelegate.
Also changed code to use existing Browser APIs and preserved the desired disposition.
See issue for more info.

BUG=http://code.google.com/p/chromium-os/issues/detail?id=8013
TEST=See issue

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

[sky, 2010-10-25 19:00:11]

http://codereview.chromium.org/4055004/diff/1/2
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/4055004/diff/1/2#newcode33
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:33: Browser* browser
= BrowserList::FindBrowserWithProfile(profile_);
You should you don't want to find only tabbed browsers?

http://codereview.chromium.org/4055004/diff/1/2#newcode44
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:44:
browser->OpenURL(url, referrer, disposition, transition);
Same comment as on 60 applies here.

http://codereview.chromium.org/4055004/diff/1/2#newcode60
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:60:
browser->AddTabContents(new_contents, disposition,
You need to make sure disposition is not NEW_WINDOW/NEW_POPUP/OFF_THE_RECORD,
otherwise if GetBrowser creates a new browser you'll leak it. In fact you should
have add a DCHECK to make sure this never happens.

[stevenjb, 2010-10-25 23:55:53]

http://codereview.chromium.org/4055004/diff/1/2
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/4055004/diff/1/2#newcode33
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:33: Browser* browser
= BrowserList::FindBrowserWithProfile(profile_);
On 2010/10/25 19:00:11, sky wrote:
> You should you don't want to find only tabbed browsers?
Ah, indeed. Browser::GetTbbedBrowser() seems to be what I want here.

http://codereview.chromium.org/4055004/diff/1/2#newcode60
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:60:
browser->AddTabContents(new_contents, disposition,
On 2010/10/25 19:00:11, sky wrote:
> You need to make sure disposition is not NEW_WINDOW/NEW_POPUP/OFF_THE_RECORD,
> otherwise if GetBrowser creates a new browser you'll leak it. In fact you
should
> have add a DCHECK to make sure this never happens.

Changed this to deal with common, straightforward cases, and to safely show a
warning and do nothing for unexpected use cases like NEW_POPUP or
OFF_THE_RECORD.

[Ben Goodger (Google), 2010-10-26 02:53:31]

I do want to look at this but have been OOO today, so I will endeavour to
look at this tomorrow (Tuesday) morning. Sorry for the latency!

-Ben

On Fri, Oct 22, 2010 at 11:12 AM, <stevenjb@chromium.org> wrote:

> Reviewers: Ben Goodger, sky,
>
> Description:
> Change window opening behavior for HtmlDialogTabContentsDelegate.
> Also changed code to use existing Browser APIs and preserved the desired
> disposition.
> See issue for more info.
>
> BUG=http://code.google.com/p/chromium-os/issues/detail?id=8013
> TEST=See issue
>
> Please review this at http://codereview.chromium.org/4055004/show
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h
>  M chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc
>  M chrome/browser/dom_ui/html_dialog_tab_contents_delegate_unittest.cc
>
>
>

[sky, 2010-10-26 15:35:10]

http://codereview.chromium.org/4055004/diff/5001/6001
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/4055004/diff/5001/6001#newcode47
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:47: LOG(WARNING) <<
"Html dialog has no matching browser.";
Can't you handle all the dispositions, but deleting the browser when done?

http://codereview.chromium.org/4055004/diff/5001/6002
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h (right):

http://codereview.chromium.org/4055004/diff/5001/6002#newcode63
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h:63: virtual Browser*
GetBrowser(WindowOpenDisposition* disposition);
Update the comment here. In fact the old comment was a bit garbage anyway.

[Ben Goodger (Google), 2010-10-26 16:21:06]

http://codereview.chromium.org/4055004/diff/5001/6001
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/4055004/diff/5001/6001#newcode57
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:57: Browser* browser
= GetBrowser(&disposition);
This is sufficiently complicated now that I would like you to investigate using
src/chrome/browser/browser_navigator.h for it.

[stevenjb, 2010-10-27 00:30:47]

So, browser::Navigate() is lovely, but for this use case I need it to work when
I don't already have a browser instance.

What I think I want is NavigateParams(Profile, WindowOpenDisposition, GURL,
PageTransition::Type) that will find or create a matching browser and set all of
the parameters. In addition to the HtmlDialog use case, I could use the same
method to fix a bug with tabbed settings where they will open in an Incognito
window if that happens to be the last active window, which is not what we want;
for Sync and other settings we always want to open the settings tab in a non
Incognito window, creating one if none exist.

I will take a stab at this. Let me know if you think this is the wrong direction
or have any suggestions.

[sky, 2010-10-27 15:59:16]

On 2010/10/27 00:30:47, Steven Bennetts wrote:
> So, browser::Navigate() is lovely, but for this use case I need it to work
when
> I don't already have a browser instance.
> 
> What I think I want is NavigateParams(Profile, WindowOpenDisposition, GURL,
> PageTransition::Type) that will find or create a matching browser and set all
of
> the parameters. In addition to the HtmlDialog use case, I could use the same
> method to fix a bug with tabbed settings where they will open in an Incognito
> window if that happens to be the last active window, which is not what we
want;
> for Sync and other settings we always want to open the settings tab in a non
> Incognito window, creating one if none exist.
> 
> I will take a stab at this. Let me know if you think this is the wrong
direction
> or have any suggestions.

This approach SGTM

  -Scott

[Ben Goodger (Google), 2010-10-27 23:38:42]

I would love it if you can take a stab at providing support for NULL browser
in NavigateParams. I haven't had time to do this yet so if you could that'd
be fab.

-Ben

On Tue, Oct 26, 2010 at 5:30 PM, <stevenjb@chromium.org> wrote:

> So, browser::Navigate() is lovely, but for this use case I need it to work
> when
> I don't already have a browser instance.
>
> What I think I want is NavigateParams(Profile, WindowOpenDisposition, GURL,
> PageTransition::Type) that will find or create a matching browser and set
> all of
> the parameters. In addition to the HtmlDialog use case, I could use the
> same
> method to fix a bug with tabbed settings where they will open in an
> Incognito
> window if that happens to be the last active window, which is not what we
> want;
> for Sync and other settings we always want to open the settings tab in a
> non
> Incognito window, creating one if none exist.
>
> I will take a stab at this. Let me know if you think this is the wrong
> direction
> or have any suggestions.
>
>
>
> http://codereview.chromium.org/4055004/show
>

[stevenjb, 2010-11-03 01:24:11]

OK, finally had time to do this. I had to add a Profile parameter to
NavigateParams, but it's much cleaner than anything else I've come up with.

[stevenjb, 2010-11-03 22:13:31]

I've fixed the HtmlDialogTabContentsDelegate tests and added some new ones to
BrowserNavigatorTest. They appear to be passing.

[Ben Goodger (Google), 2010-11-05 16:39:42]

I really like what you've done here. LGTM. Thanks for cleaning this up!

[stevenjb, 2010-11-05 20:55:23]

On 2010/11/05 16:39:42, Ben Goodger wrote:
> I really like what you've done here. LGTM. Thanks for cleaning this up!

Thanks, my pleasure.

I rebased and updated some comments in HtmlDialogTabContentsDelegate that I
noticed were out of date.

Will push when the tree is open.