Print Preview: Set a print job's title and url correctly.

Print Preview: Set a print job's title and url correctly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80825

[Lei Zhang, 2011-03-31 18:14:38]

I believe this is an easier way to set a print job's title and url. Rather than
having the information going browser -> renderer -> browser, we keep it in the
browser process altogether.

Avi says I can use TabContentsWrapper::GetCurrentWrapperForContents() here, so
if this LG to you, I'll also CC him on the TCW bits.

[kmadhusu, 2011-03-31 22:37:48]

LGTM with a nit.

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:76:
wrapper->print_view_manager()->OverrideTitleAndUrl(tab_contents());
if (wrapper)
  wrapper->print_view_manager()->OverrideTitleAndUrl(tab_contents());

[Lei Zhang, 2011-03-31 22:42:40]

avi: Please take a look at the TCW bits.

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:76:
wrapper->print_view_manager()->OverrideTitleAndUrl(tab_contents());
On 2011/03/31 22:37:48, kmadhusu wrote:
> if (wrapper)
>   wrapper->print_view_manager()->OverrideTitleAndUrl(tab_contents());

avi: Would we ever run into a case where |wrapper| is NULL?

[stuartmorgan, 2011-04-01 22:34:14]

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:42: else
else is unnecessary here.

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.h (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.h:35: // Override the title and url
for this PrintViewManager's PrintJobs.
The paramater is somewhat surprising given the description; can you flesh out
the description here to make it explicit that it's extract the tab_contents
title and URL to use?

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.h:135: bool overridden_title_url_;
overridden_title_and_url_ ? I was trying to figure out what a title_url was for
a minute.

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/ui/tab_conten...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:14: #include
"chrome/browser/printing/print_view_manager.h"
Why do we need this?

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:25: class
PrintViewManager;
If we do need the header, we don't need this.

[Lei Zhang, 2011-04-04 22:21:03]

It occurred to me (in my sleep, of course) that overriding the url is
unnecessary. The url is only used in the header/footers, and that's already part
of the generated preview. The print preview tab that displays the preview does
not need the url for anything.

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:42: else
On 2011/04/01 22:34:14, stuartmorgan wrote:
> else is unnecessary here.

Copy + Paste. In any case, I'm reverting this particular change.

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.h (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.h:35: // Override the title and url
for this PrintViewManager's PrintJobs.
On 2011/04/01 22:34:14, stuartmorgan wrote:
> The paramater is somewhat surprising given the description; can you flesh out
> the description here to make it explicit that it's extract the tab_contents
> title and URL to use?

Done.

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/ui/tab_conten...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:14: #include
"chrome/browser/printing/print_view_manager.h"
On 2011/04/01 22:34:14, stuartmorgan wrote:
> Why do we need this?

Yes, we do for print_view_manager().

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:25: class
PrintViewManager;
On 2011/04/01 22:34:14, stuartmorgan wrote:
> If we do need the header, we don't need this.

Done.

[stuartmorgan, 2011-04-06 10:49:17]

LGTM (although I wonder if not having to bleed the header would be worth moving
the getter implementation into the implementation file)

[Avi (use Gerrit), 2011-04-06 11:21:32]

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/6783005/diff/5011/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:76:
wrapper->print_view_manager()->OverrideTitleAndUrl(tab_contents());
On 2011/03/31 22:42:40, Lei Zhang wrote:
> avi: Would we ever run into a case where |wrapper| is NULL?

No. Only in oddball testing situations.

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:75:
TabContentsWrapper::GetCurrentWrapperForContents(print_preview_tab);
Are we in a callback from content/? If not, please followup with a CL to plumb
TCW though the printing system rather than TC, and pull this line.

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:73:
print_view_manager_.reset(new printing::PrintViewManager(contents));
Is this a per-tab observer or a Tab Helper? If it provides public functions,
it's a Tab Helper; move it up 15 lines to be with its friends.

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:160:
scoped_ptr<printing::PrintViewManager> print_view_manager_;
If this is a Tab Helper, put it in the Tab Helper section below.

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:163:
scoped_ptr<printing::PrintPreviewMessageHandler> print_preview_;
No. This is a class that just sits there per-tab and watches. This goes in the
"per-tab observers" section below. I missed when you put it here. Please move
it.

[Lei Zhang, 2011-04-07 00:53:20]

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:75:
TabContentsWrapper::GetCurrentWrapperForContents(print_preview_tab);
On 2011/04/06 11:21:32, Avi wrote:
> Are we in a callback from content/? If not, please followup with a CL to plumb
> TCW though the printing system rather than TC, and pull this line.

For PrintPreview, we have a lot of classes in chrome/ that inherit from classes
in content/. Thus they have to deal with TCs and not TCWs. In this particular
case, PrintPreviewMessageHandler is a subclass of TabContentObserver, which
knows nothing about TCWs.

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:73:
print_view_manager_.reset(new printing::PrintViewManager(contents));
On 2011/04/06 11:21:32, Avi wrote:
> Is this a per-tab observer or a Tab Helper? If it provides public functions,
> it's a Tab Helper; move it up 15 lines to be with its friends.

It's a tab helper then.

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:160:
scoped_ptr<printing::PrintViewManager> print_view_manager_;
On 2011/04/06 11:21:32, Avi wrote:
> If this is a Tab Helper, put it in the Tab Helper section below.

Done.

http://codereview.chromium.org/6783005/diff/14001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:163:
scoped_ptr<printing::PrintPreviewMessageHandler> print_preview_;
On 2011/04/06 11:21:32, Avi wrote:
> No. This is a class that just sits there per-tab and watches. This goes in the
> "per-tab observers" section below. I missed when you put it here. Please move
> it.

Done.

[Avi (use Gerrit), 2011-04-07 13:48:38]

LGTM then.