Print Preview: Open print preview tab in the right position when the source is a popup.

Print Preview: Open print preview tab in the right position when the source is a popup.

BUG=95351
TEST=Included.

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

[Lei Zhang, 2011-09-08 05:22:31]

sky: Main reviewer. I have a question for you in the comment below.
altimofeev: Is it ok to extend BrowserNavigatorTest?

http://codereview.chromium.org/7845015/diff/1/chrome/browser/printing/print_p...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/7845015/diff/1/chrome/browser/printing/print_p...
chrome/browser/printing/print_preview_tab_controller.cc:323: if
(!current_browser->is_type_popup()) {
sky: should this be !is_type_popup() or is_type_tabbed() ? I don't know what
type_panel is.

[altimofeev, 2011-09-08 12:43:40]

> Is it ok to extend BrowserNavigatorTest?

I think no. Actually, you don't need it - the only method you used is
MakeNavigateParams(), which is trivial.

The reason I think it is a bad idea is the following. BrowserNavigatorTest is
used for testing navigation itself, so it does some side work, which is
redundant in other cases.

[sky, 2011-09-08 17:31:44]

As altimofeev says, I would put the test closer to the printing tests.

  -Scott

http://codereview.chromium.org/7845015/diff/1/chrome/browser/printing/print_p...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/7845015/diff/1/chrome/browser/printing/print_p...
chrome/browser/printing/print_preview_tab_controller.cc:323: if
(!current_browser->is_type_popup()) {
On 2011/09/08 05:22:32, Lei Zhang wrote:
> sky: should this be !is_type_popup() or is_type_tabbed() ? I don't know what
> type_panel is.

It should be is_type_tabbed. Panels are very close a popup, they only show one
tab. Panels are very similar to the panels you see on chromeos.

[Lei Zhang, 2011-09-09 05:16:09]

Thanks for your input.

sky: PTAL at patch set.

[sky, 2011-09-09 15:55:34]

LGTM

[commit-bot: I haz the power, 2011-09-09 23:08:41]

Change committed as 100527