Print Preview: Store preview data in the print preview handler....

Print Preview: Store preview data in PrintPreviewUIHTMLSource.

BUG=64125
TEST=included

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

[Lei Zhang, 2011-01-25 22:03:45]

So in order to store data in the PrintPreviewHandler, I have to access it
through print_preview_tab->dom_ui(). However, chrome/browser/renderer_host/DEPS
says I cannot access methods of a TabContents inside RVH. I tried that in patch
set 2 and got the DEPS check failure.

Per the suggestion in the DEPS file, I worked around this in patch set 3. I
called up through RenderViewHostDelegate, which in turn added to the
TabContent's implementation of RVHDelegate.

This works, but IMO it's much more indirect and require modifying more code than
the other proposal in: http://codereview.chromium.org/6221005/. I.e. code review
6221005 patch set 2 is a lot simpler than this.

[brettw, 2011-01-26 20:38:31]

I just saw this. I was quite surprised by the current PrintPreview code. It uses
TabContents extensively in render_view_host which is one of the biggest no-nos
in the entire system. I'll send email on this, we should not be adding to the
current design.

[Lei Zhang, 2011-01-27 00:05:47]

On 2011/01/26 20:38:31, brettw wrote:
> I just saw this. I was quite surprised by the current PrintPreview code. It
uses
> TabContents extensively in render_view_host which is one of the biggest no-nos
> in the entire system. I'll send email on this, we should not be adding to the
> current design.

Following jam's email yesterday, I moved the print preview code out into
chrome/browser/printing/print_previwe_message_handler.cc. Please take a look at
patch set 5.

[brettw, 2011-01-27 00:27:35]

LGTM for TabContents/RenderViewHost changes. I didn't look at the printing
files.

[James Hawkins, 2011-01-31 18:39:37]

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.h (left):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:9: #include "base/scoped_ptr.h"
This is right to remove scoped_ptr.h, but now we need to include ref_counted.h
to get scoped_refptr.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:15: }  // namespace printing
I realize there was disagreement on the list recently about this, but the style
guide still says namespaces should be commented.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.h (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:26: typedef
std::pair<base::SharedMemory*, uint32> PrintPreviewData;
Document this typedef.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:31: // Get the print preview
|data|. The data is valid as long as the
s/Get/Gets/

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:35: // Save the print preview
|data|. PrintPreviewHandler owns the data and is
s/Save/Saves/ and now that I look at the method name, this should probably be
"Sets".

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:38: // b) when PrintPreviewHandler
goes away.
nit: You should probably be more descriptive than 'goes away': is destroyed, is
destructed, etc.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui.cc (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.cc:88:
PrintPreviewUIHTMLSource::PrintPreviewUIHTMLSource(PrintPreviewHandler* handler)
After reading through the flow, it seems the best place to store
PrintPreviewData is actually PrintPreviewUIHTMLSource. That will simplify things
a bit, and get rid of a level of indirection by not having to store a pointer to
PrintPreviewHandler here.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui.h (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.h:15: }
Comment the namespace.

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

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:71: // Send the
PrintingDone msg for now.
What do you mean by 'for now'?

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.h (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:18:
~PrintPreviewMessageHandler();
virtual

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:22: //
WebNavigationObserver implementation.
Blank line above this line.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:26: TabContents*
GetOrCreatePrintPreviewTab();
Document.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:28: TabContents* owner_;
Document.

[brettw, 2011-01-31 18:57:15]

Thanks for helping clean this up (I didn't actually do a code review).

[Lei Zhang, 2011-01-31 22:15:51]

I addressed most of James's comments in patch set 6. I'll take a look at storing
the print preview data in PrintPreviewUIHTMLSource.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.h (left):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:9: #include "base/scoped_ptr.h"
On 2011/01/31 18:39:38, James Hawkins wrote:
> This is right to remove scoped_ptr.h, but now we need to include ref_counted.h
> to get scoped_refptr.

weak_ptr.h includes ref_counted.h, but I added it since we do use scoped_refptr.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:15: }  // namespace printing
On 2011/01/31 18:39:38, James Hawkins wrote:
> I realize there was disagreement on the list recently about this, but the
style
> guide still says namespaces should be commented.

I've asked for the style guide to be updated to reflect the decision that having
the comment doesn't make sense for short forward declarations.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.h (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:26: typedef
std::pair<base::SharedMemory*, uint32> PrintPreviewData;
On 2011/01/31 18:39:38, James Hawkins wrote:
> Document this typedef.

Done.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:31: // Get the print preview
|data|. The data is valid as long as the
On 2011/01/31 18:39:38, James Hawkins wrote:
> s/Get/Gets/

Done.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:35: // Save the print preview
|data|. PrintPreviewHandler owns the data and is
On 2011/01/31 18:39:38, James Hawkins wrote:
> s/Save/Saves/ and now that I look at the method name, this should probably be
> "Sets".

There's no function named "SetsFoo". Everything in chrome/ is SetFoo. I.e.
WebPluginProxy::SetCookie(), RenderWidget::SetBackground(),
TabContents::SetIsCrashed(), etc etc.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:38: // b) when PrintPreviewHandler
goes away.
On 2011/01/31 18:39:38, James Hawkins wrote:
> nit: You should probably be more descriptive than 'goes away': is destroyed,
is
> destructed, etc.

Done.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui.h (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.h:15: }
On 2011/01/31 18:39:38, James Hawkins wrote:
> Comment the namespace.

In the last round of style discussion at the beginning of the month, Ben decided
to we don't need it.

https://groups.google.com/a/chromium.org/group/chromium-dev/msg/94ea152ff05ab952

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

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:71: // Send the
PrintingDone msg for now.
On 2011/01/31 18:39:38, James Hawkins wrote:
> What do you mean by 'for now'?

It was copy and pasted from RVH. Removed.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.h (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:18:
~PrintPreviewMessageHandler();
On 2011/01/31 18:39:38, James Hawkins wrote:
> virtual

Done.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:22: //
WebNavigationObserver implementation.
On 2011/01/31 18:39:38, James Hawkins wrote:
> Blank line above this line.

Done.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:26: TabContents*
GetOrCreatePrintPreviewTab();
On 2011/01/31 18:39:38, James Hawkins wrote:
> Document.

Done.

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:28: TabContents* owner_;
On 2011/01/31 18:39:38, James Hawkins wrote:
> Document.

Done.

[James Hawkins, 2011-02-01 01:02:38]

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.h (right):

http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.h:35: // Save the print preview
|data|. PrintPreviewHandler owns the data and is
On 2011/01/31 18:39:38, James Hawkins wrote:
> s/Save/Saves/ and now that I look at the method name, this should probably be
> "Sets".

By "this", I was referring to the comment.

[Lei Zhang, 2011-02-01 02:37:29]

Ok. I moved PrintPreviewUIHTMLSource out into its own file. It's mostly the same
code as the previous patch set, just shuffled around.

On 2011/02/01 01:02:38, James Hawkins wrote:
>
http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
> File chrome/browser/dom_ui/print_preview_handler.h (right):
> 
>
http://codereview.chromium.org/6334010/diff/43001/chrome/browser/dom_ui/print...
> chrome/browser/dom_ui/print_preview_handler.h:35: // Save the print preview
> |data|. PrintPreviewHandler owns the data and is
> On 2011/01/31 18:39:38, James Hawkins wrote:
> > s/Save/Saves/ and now that I look at the method name, this should probably
be
> > "Sets".
> 
> By "this", I was referring to the comment.

Done.

[James Hawkins, 2011-02-03 00:53:52]

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui.cc (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.cc:13:
html_source_(make_scoped_refptr(new PrintPreviewUIHTMLSource())) {
Why do you need to use make_scoped_refptr() here?

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.cc:20: html_source_->AddRef();
The scoped_refptr constructor which accepts T* calls AddRef() and Release() for
you.

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui_html_source.cc (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui_html_source.cc:23: void
SetLocalizedStrings(DictionaryValue* localized_strings) {
Why is this done in PrintPreviewUIHTMLSouce instead of PrintPreviewHandler?

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui_html_source.h (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui_html_source.h:24: explicit
PrintPreviewUIHTMLSource();
explicit should only be used for constructors with one parameter.

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui_html_source.h:44: // Current print
preview data, owned by PrintPreviewHandler.
nit: It would be more appropriate to state, "the contents of which are owned by
PrintPreviewHandler."

[Lei Zhang, 2011-02-03 02:19:32]

In addition to addressing the comments in patch set 8, I realized the test fails
on CrOS, so I disabled the test there, and I will run it through the CrOS
trybot.

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui.cc (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.cc:13:
html_source_(make_scoped_refptr(new PrintPreviewUIHTMLSource())) {
On 2011/02/03 00:53:52, James Hawkins wrote:
> Why do you need to use make_scoped_refptr() here?

Because html_source_ is a scoped_refptr<PrintPreviewUIHTMLSource>, not a
PrintPreviewUIHTMLSource*. Perhaps it was confusing since html_source()'s
signature is says it returns PrintPreviewUIHTMLSource*, but the code says
"return html_source_", so I changed it to return html_source_.get().

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.cc:20: html_source_->AddRef();
On 2011/02/03 00:53:52, James Hawkins wrote:
> The scoped_refptr constructor which accepts T* calls AddRef() and Release()
for
> you.

I was having problems where |html_source_| was getting destroyed too early, but
those problems magically went away, so I removed the AddRef()/Release() calls.

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui_html_source.cc (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui_html_source.cc:23: void
SetLocalizedStrings(DictionaryValue* localized_strings) {
On 2011/02/03 00:53:52, James Hawkins wrote:
> Why is this done in PrintPreviewUIHTMLSouce instead of PrintPreviewHandler?

Because we need the localized strings to render chrome://print, which happens in
PrintPreviewUIHTMLSource::StartDataRequest() below. Why would we want to put
this in PrintPreviewHandler?

This is just moving PrintPreviweUIHTMLSource out to a separate file. There's no
code change here. You reviewed this originally in r61425.
http://codereview.chromium.org/3516010.

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui_html_source.h (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui_html_source.h:24: explicit
PrintPreviewUIHTMLSource();
On 2011/02/03 00:53:52, James Hawkins wrote:
> explicit should only be used for constructors with one parameter.

Fixed.

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui_html_source.h:44: // Current print
preview data, owned by PrintPreviewHandler.
On 2011/02/03 00:53:52, James Hawkins wrote:
> nit: It would be more appropriate to state, "the contents of which are owned
by
> PrintPreviewHandler."

Done.

[James Hawkins, 2011-02-03 21:51:40]

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui.cc (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.cc:13:
html_source_(make_scoped_refptr(new PrintPreviewUIHTMLSource())) {
On 2011/02/03 02:19:32, Lei Zhang wrote:
> On 2011/02/03 00:53:52, James Hawkins wrote:
> > Why do you need to use make_scoped_refptr() here?
> 
> Because html_source_ is a scoped_refptr<PrintPreviewUIHTMLSource>, not a
> PrintPreviewUIHTMLSource*. Perhaps it was confusing since html_source()'s
> signature is says it returns PrintPreviewUIHTMLSource*, but the code says
> "return html_source_", so I changed it to return html_source_.get().

ref_counted.h:222 -
scoped_refptr(T* p) {...}

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui_html_source.cc (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui_html_source.cc:23: void
SetLocalizedStrings(DictionaryValue* localized_strings) {
On 2011/02/03 02:19:32, Lei Zhang wrote:
> On 2011/02/03 00:53:52, James Hawkins wrote:
> > Why is this done in PrintPreviewUIHTMLSouce instead of PrintPreviewHandler?
> 
> Because we need the localized strings to render chrome://print, which happens
in
> PrintPreviewUIHTMLSource::StartDataRequest() below. Why would we want to put
> this in PrintPreviewHandler?
> 
> This is just moving PrintPreviweUIHTMLSource out to a separate file. There's
no
> code change here. You reviewed this originally in r61425.
> http://codereview.chromium.org/3516010.

Ah, XHandler::GetLocalizedValues() is only for OptionsUI.  I'll talk to arv
about refactoring this.

[Lei Zhang, 2011-02-03 21:58:22]

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_ui.cc (right):

http://codereview.chromium.org/6334010/diff/43015/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_ui.cc:13:
html_source_(make_scoped_refptr(new PrintPreviewUIHTMLSource())) {
On 2011/02/03 21:51:40, James Hawkins wrote:
> On 2011/02/03 02:19:32, Lei Zhang wrote:
> > On 2011/02/03 00:53:52, James Hawkins wrote:
> > > Why do you need to use make_scoped_refptr() here?
> > 
> > Because html_source_ is a scoped_refptr<PrintPreviewUIHTMLSource>, not a
> > PrintPreviewUIHTMLSource*. Perhaps it was confusing since html_source()'s
> > signature is says it returns PrintPreviewUIHTMLSource*, but the code says
> > "return html_source_", so I changed it to return html_source_.get().
> 
> ref_counted.h:222 -
> scoped_refptr(T* p) {...}

Removed.

[James Hawkins, 2011-02-03 22:16:18]

LGTM, thanks.