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

Print Preview: Promote the print preview tab controller to the print preview manager. Store preview data in the print preview manager.

BUG=64125
TEST=included

[James Hawkins, 2011-01-13 06:38:21]

-kmadhusu, since she's not a full committer.

[James Hawkins, 2011-01-13 06:43:27]

May I see a high-level design diagram that shows what you're trying to
accomplish here?

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.h (right):

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.h:10: // tabs, and the
associated initiator tabs and print preview data.
This really feels shoe-horned in here.  The name of the class implies that it's
only responsibility is to Control Print Preview Tabs.

"This class manages X, and Y...oh and Z."

[kmadhusu, 2011-01-13 18:03:13]

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:28: ++it) {
nit: This can fit in the previous line.

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:198:
preview_data_map_[preview_tab->tab_contents()] =
Rather than making calls to the same function consecutively, it would be nice to
pull out "preview_tab->tab_contents()".

[Lei Zhang, 2011-01-13 19:47:49]

On 2011/01/13 06:43:27, James Hawkins wrote:
> May I see a high-level design diagram that shows what you're trying to
> accomplish here?
> 
>
http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
> File chrome/browser/printing/print_preview_tab_controller.h (right):
> 
>
http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
> chrome/browser/printing/print_preview_tab_controller.h:10: // tabs, and the
> associated initiator tabs and print preview data.
> This really feels shoe-horned in here.  The name of the class implies that
it's
> only responsibility is to Control Print Preview Tabs.
> 
> "This class manages X, and Y...oh and Z."

The idea is that we need to save the generated print preview somewhere and it
has to be associated with the print preview tab, since that's the only context
we have when we are in the DOM UI code. The place that naturally comes to mind
is in the TabContents, but we discussed [1] on chromium-dev about stabilizing
the TabContents API and putting new functionality in different classes. Which is
why I'm putting this in the PrintPreviewTabController.

I don't believe this is shoe-horned in. "Controlling tabs" is vague and it
doesn't exclude controlling the content associated with the print preview tabs.
Putting this in a separate class will just add more work, since the new class
will have to talk to the PrintPreviewTabController to figure out whether a given
TabContents is valid of not.

[1] project carnitas thread

[Lei Zhang, 2011-01-13 20:59:18]

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:28: ++it) {
On 2011/01/13 18:03:14, kmadhusu wrote:
> nit: This can fit in the previous line. 

Done.

http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:198:
preview_data_map_[preview_tab->tab_contents()] =
On 2011/01/13 18:03:14, kmadhusu wrote:
> Rather than making calls to the same function consecutively, it would be nice
to
> pull out "preview_tab->tab_contents()".

Done.

[James Hawkins, 2011-01-13 21:04:37]

On 2011/01/13 19:47:49, Lei Zhang wrote:
> On 2011/01/13 06:43:27, James Hawkins wrote:
> > May I see a high-level design diagram that shows what you're trying to
> > accomplish here?
> > 
> >
>
http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
> > File chrome/browser/printing/print_preview_tab_controller.h (right):
> > 
> >
>
http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
> > chrome/browser/printing/print_preview_tab_controller.h:10: // tabs, and the
> > associated initiator tabs and print preview data.
> > This really feels shoe-horned in here.  The name of the class implies that
> it's
> > only responsibility is to Control Print Preview Tabs.
> > 
> > "This class manages X, and Y...oh and Z."
> 
> The idea is that we need to save the generated print preview somewhere and it
> has to be associated with the print preview tab, since that's the only context
> we have when we are in the DOM UI code. The place that naturally comes to mind
> is in the TabContents, but we discussed [1] on chromium-dev about stabilizing
> the TabContents API and putting new functionality in different classes. Which
is
> why I'm putting this in the PrintPreviewTabController.
> 
> I don't believe this is shoe-horned in. "Controlling tabs" is vague and it
> doesn't exclude controlling the content associated with the print preview
tabs.

It's pretty explicit; the class controls print preview tabs.

> Putting this in a separate class will just add more work, since the new class
> will have to talk to the PrintPreviewTabController to figure out whether a
given
> TabContents is valid of not.
> 
> [1] project carnitas thread

I'm not proposing you add anything to TabContents.  As I said previously, I'd
like to see a class design and flow chart of what you're trying to accomplish
here.

[Lei Zhang, 2011-01-13 22:22:05]

On 2011/01/13 21:04:37, James Hawkins wrote:
> On 2011/01/13 19:47:49, Lei Zhang wrote:
> > On 2011/01/13 06:43:27, James Hawkins wrote:
> > > May I see a high-level design diagram that shows what you're trying to
> > > accomplish here?
> > > 
> > >
> >
>
http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
> > > File chrome/browser/printing/print_preview_tab_controller.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6221005/diff/12001/chrome/browser/printing/pri...
> > > chrome/browser/printing/print_preview_tab_controller.h:10: // tabs, and
the
> > > associated initiator tabs and print preview data.
> > > This really feels shoe-horned in here.  The name of the class implies that
> > it's
> > > only responsibility is to Control Print Preview Tabs.
> > > 
> > > "This class manages X, and Y...oh and Z."
> > 
> > The idea is that we need to save the generated print preview somewhere and
it
> > has to be associated with the print preview tab, since that's the only
context
> > we have when we are in the DOM UI code. The place that naturally comes to
mind
> > is in the TabContents, but we discussed [1] on chromium-dev about
stabilizing
> > the TabContents API and putting new functionality in different classes.
Which
> is
> > why I'm putting this in the PrintPreviewTabController.
> > 
> > I don't believe this is shoe-horned in. "Controlling tabs" is vague and it
> > doesn't exclude controlling the content associated with the print preview
> tabs.
> 
> It's pretty explicit; the class controls print preview tabs.

In that case, this class deserves a promotion to PrintPreviewManager. If I were
to add PrintPreviewDataController instead, then BrowserProcess would get bigger
with ever single little controller we add. Not to mention
PrintPreviewDataController will largely be duplicating code from
PrintPreviewTabController.

> > Putting this in a separate class will just add more work, since the new
class
> > will have to talk to the PrintPreviewTabController to figure out whether a
> given
> > TabContents is valid of not.
> > 
> > [1] project carnitas thread
> 
> I'm not proposing you add anything to TabContents.  As I said previously, I'd
> like to see a class design and flow chart of what you're trying to accomplish
> here.

I expanded on the purpose of PrintPreviewManager in the header. I added the
changes in chrome/browser/dom_ui, so you can see how this is used to retrieve
the preview data.