Add new methods to create constrained windows for print preview and release the internal TabConte...

Add new methods to create constrained windows for print preview and release the internal TabContents when asked to.

BUG=none
TEST=included
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105575

[Evan Stade, 2011-10-11 02:49:17]

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc:133: return
constrained_window;
seems like you could just modify this to return the delegate, then callers would
use either delegate->window() or delegate->tab() as they saw fit.

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
File chrome/browser/ui/webui/constrained_html_ui.h (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui.h:30: virtual void
ReleaseTabContentsOnDialogClose() = 0;
what is the point of this? seems like it's only used in a test

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
File chrome/browser/ui/webui/constrained_html_ui_browsertest.cc (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui_browsertest.cc:142:
constrained_delegate->ReleaseTabContentsOnDialogClose();
so doesn't this leak?

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui_browsertest.cc:147:
constrained_window_count());
4 more indent

[Lei Zhang, 2011-10-11 03:15:58]

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc:133: return
constrained_window;
On 2011/10/11 02:49:17, Evan Stade wrote:
> seems like you could just modify this to return the delegate, then callers
would
> use either delegate->window() or delegate->tab() as they saw fit.

Do you really want me to change ConstrainedHtmlUIDelegate, or are you just
trying to not have lines 141-145 duplicate lines 128-132?

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
File chrome/browser/ui/webui/constrained_html_ui.h (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui.h:30: virtual void
ReleaseTabContentsOnDialogClose() = 0;
On 2011/10/11 02:49:17, Evan Stade wrote:
> what is the point of this? seems like it's only used in a test

I'm splitting this chunk of code into a separate CL to avoid a 40 file CL. It
will be used for print preview, but in this CL there's only a test to exercise
it. I'd highly prefer not to have 20 patch sets and 10 reviewers.

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
File chrome/browser/ui/webui/constrained_html_ui_browsertest.cc (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui_browsertest.cc:142:
constrained_delegate->ReleaseTabContentsOnDialogClose();
On 2011/10/11 02:49:17, Evan Stade wrote:
> so doesn't this leak?

In this test, yes we leak |new_tab|. In its intended use, no, because
PrintPreviewTabController will destroy the tab eventually.

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui_browsertest.cc:147:
constrained_window_count());
On 2011/10/11 02:49:17, Evan Stade wrote:
> 4 more indent

This is just copy + pasted. I'll change the other occurrences.

[Paweł Hajdan Jr., 2011-10-11 22:16:48]

Drive-by, and this is #1 cause of leak in tests. Really.

http://codereview.chromium.org/8220026/diff/8/chrome/browser/ui/webui/constra...
File chrome/browser/ui/webui/constrained_html_ui_browsertest.cc (right):

http://codereview.chromium.org/8220026/diff/8/chrome/browser/ui/webui/constra...
chrome/browser/ui/webui/constrained_html_ui_browsertest.cc:150: delete new_tab;
Why not scoped_ptr? Otherwise if any of ASSERTs fail you got a leak.

[Evan Stade, 2011-10-11 22:54:59]

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc:133: return
constrained_window;
On 2011/10/11 03:15:58, Lei Zhang wrote:
> On 2011/10/11 02:49:17, Evan Stade wrote:
> > seems like you could just modify this to return the delegate, then callers
> would
> > use either delegate->window() or delegate->tab() as they saw fit.
> 
> Do you really want me to change ConstrainedHtmlUIDelegate, or are you just
> trying to not have lines 141-145 duplicate lines 128-132?

yea I don't want the duplication and I don't like the idea of adding a special
factory function just for print preview. However you choose to resolve this is
fine. I just thought adding tab and window getters to the
ConstrainedHtmlUIDelegate interface would be one easy way to do this.

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
File chrome/browser/ui/webui/constrained_html_ui.h (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui.h:30: virtual void
ReleaseTabContentsOnDialogClose() = 0;
On 2011/10/11 03:15:58, Lei Zhang wrote:
> On 2011/10/11 02:49:17, Evan Stade wrote:
> > what is the point of this? seems like it's only used in a test
> 
> I'm splitting this chunk of code into a separate CL to avoid a 40 file CL. It
> will be used for print preview, but in this CL there's only a test to exercise
> it. I'd highly prefer not to have 20 patch sets and 10 reviewers.

it's impossible for me to review a design without a use case that exercises it,
but I do agree that smaller CLs are better than bigger ones. Thus I suggest that
you create the minimal CL which actually makes use of this function.

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
File chrome/browser/ui/webui/constrained_html_ui_browsertest.cc (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui_browsertest.cc:142:
constrained_delegate->ReleaseTabContentsOnDialogClose();
On 2011/10/11 03:15:58, Lei Zhang wrote:
> On 2011/10/11 02:49:17, Evan Stade wrote:
> > so doesn't this leak?
> 
> In this test, yes we leak |new_tab|. In its intended use, no, because
> PrintPreviewTabController will destroy the tab eventually.

i see that you have plugged this leak

[Lei Zhang, 2011-10-11 23:04:57]

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
File chrome/browser/ui/webui/constrained_html_ui.h (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/webui/cons...
chrome/browser/ui/webui/constrained_html_ui.h:30: virtual void
ReleaseTabContentsOnDialogClose() = 0;
On 2011/10/11 22:54:59, Evan Stade wrote:
> On 2011/10/11 03:15:58, Lei Zhang wrote:
> > On 2011/10/11 02:49:17, Evan Stade wrote:
> > > what is the point of this? seems like it's only used in a test
> > 
> > I'm splitting this chunk of code into a separate CL to avoid a 40 file CL.
It
> > will be used for print preview, but in this CL there's only a test to
exercise
> > it. I'd highly prefer not to have 20 patch sets and 10 reviewers.
> 
> it's impossible for me to review a design without a use case that exercises
it,
> but I do agree that smaller CLs are better than bigger ones. Thus I suggest
that
> you create the minimal CL which actually makes use of this function.

Sorry, much of the bigger CL is monolithic. You can look at it though:
http://codereview.chromium.org/8136027/

The use case is as follows:
- user presses print
- the constrained window needs to go away
- the print preview TabContents (in the constrained window) needs to stay alive
until it finishes printing

So what we do is:
- BackgroundPrintingManager takes ownership of the PP TabContents. Holds on to
it until printing finishes.
- Tell the constrained window to "ReleaseTabContentsOnDialogClose"
- Close the constrained window

[Lei Zhang, 2011-10-11 23:41:25]

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc (right):

http://codereview.chromium.org/8220026/diff/2003/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_html_delegate_gtk.cc:133: return
constrained_window;
On 2011/10/11 22:54:59, Evan Stade wrote:
> On 2011/10/11 03:15:58, Lei Zhang wrote:
> > On 2011/10/11 02:49:17, Evan Stade wrote:
> > > seems like you could just modify this to return the delegate, then callers
> > would
> > > use either delegate->window() or delegate->tab() as they saw fit.
> > 
> > Do you really want me to change ConstrainedHtmlUIDelegate, or are you just
> > trying to not have lines 141-145 duplicate lines 128-132?
> 
> yea I don't want the duplication and I don't like the idea of adding a special
> factory function just for print preview. However you choose to resolve this is
> fine. I just thought adding tab and window getters to the
> ConstrainedHtmlUIDelegate interface would be one easy way to do this.

Done.

http://codereview.chromium.org/8220026/diff/8/chrome/browser/ui/webui/constra...
File chrome/browser/ui/webui/constrained_html_ui_browsertest.cc (right):

http://codereview.chromium.org/8220026/diff/8/chrome/browser/ui/webui/constra...
chrome/browser/ui/webui/constrained_html_ui_browsertest.cc:150: delete new_tab;
On 2011/10/11 22:16:48, Paweł Hajdan Jr. wrote:
> Why not scoped_ptr? Otherwise if any of ASSERTs fail you got a leak.

Done.

[Lei Zhang, 2011-10-12 02:36:14]

PTAL at patch set 7.

[Lei Zhang, 2011-10-13 09:17:33]

Rebased in patch set 8.

[Paweł Hajdan Jr., 2011-10-13 19:53:04]

Code I commented in the drive-by LGTM, thanks.

[Evan Stade, 2011-10-13 23:59:13]

lgtm

http://codereview.chromium.org/8220026/diff/15001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_ui.cc (right):

http://codereview.chromium.org/8220026/diff/15001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_ui.cc:250: wrapper);
nit: don't need to store delegate variable

[Lei Zhang, 2011-10-14 17:31:05]

http://codereview.chromium.org/8220026/diff/15001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_ui.cc (right):

http://codereview.chromium.org/8220026/diff/15001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_ui.cc:250: wrapper);
On 2011/10/13 23:59:14, Evan Stade wrote:
> nit: don't need to store delegate variable

As in you prefer:

ConstrainedWindow* dialog =
ConstrainedHtmlUI::CreateConstrainedHtmlDialog(profile, delegate_,
wrapper)->window(); ?

or did you mean no need for |dialog| ?

[Evan Stade, 2011-10-14 18:04:49]

On 2011/10/14 17:31:05, Lei Zhang wrote:
>
http://codereview.chromium.org/8220026/diff/15001/chrome/browser/ui/login/log...
> File chrome/browser/ui/login/login_prompt_ui.cc (right):
> 
>
http://codereview.chromium.org/8220026/diff/15001/chrome/browser/ui/login/log...
> chrome/browser/ui/login/login_prompt_ui.cc:250: wrapper);
> On 2011/10/13 23:59:14, Evan Stade wrote:
> > nit: don't need to store delegate variable
> 
> As in you prefer:
> 
> ConstrainedWindow* dialog =
> ConstrainedHtmlUI::CreateConstrainedHtmlDialog(profile, delegate_,
> wrapper)->window(); ?

this

> 
> or did you mean no need for |dialog| ?