PrintPreview:[MAC & WIN] Added code to show the native printer management dialog.

PrintPreview:[MAC & WIN] Added code to show the native printer management dialog.

BUG=none
TEST=Enable print preview flag. Preview a webpage. Select the "Manage Printers..." option in the destination list. Native printer management dialog is displayed.

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

[kmadhusu, 2011-04-27 22:57:56]

thestig: Added code to show the native printer management dialog on mac and
windows. If this LG to you, I will add other reviewers.

[Lei Zhang, 2011-04-27 23:46:36]

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
File chrome/browser/printing/printer_manager_dialog.h (right):

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
chrome/browser/printing/printer_manager_dialog.h:13: // UI to manage the network
printers.
Can you mention this is used for print preview? Also, local printers need to be
managed too.

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
File chrome/browser/printing/printer_manager_dialog_linux.cc (right):

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
chrome/browser/printing/printer_manager_dialog_linux.cc:12: NOTIMPLEMENTED();
As a starting point, you can run: system-config-printer

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:112: if (lastSelectedPrinterIndex !=
selectedPrinter)
no need for this if statement.

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:348:
addDestinationListOption(localStrings.getString('managePrinters'), false);
nit: can you keep the new line after this?

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:358: * Adds an option to the
destination list.
nit: the printer destination list.

[kmadhusu, 2011-04-27 23:54:52]

Chris: Please review the mac code.

[kmadhusu, 2011-04-28 00:08:26]

thestig: Addressed review comments.

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
File chrome/browser/printing/printer_manager_dialog.h (right):

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
chrome/browser/printing/printer_manager_dialog.h:13: // UI to manage the network
printers.
On 2011/04/27 23:46:36, Lei Zhang wrote:
> Can you mention this is used for print preview? Also, local printers need to
be
> managed too.

Done.

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
File chrome/browser/printing/printer_manager_dialog_linux.cc (right):

http://codereview.chromium.org/6903083/diff/22/chrome/browser/printing/printe...
chrome/browser/printing/printer_manager_dialog_linux.cc:12: NOTIMPLEMENTED();
On 2011/04/27 23:46:36, Lei Zhang wrote:
> As a starting point, you can run: system-config-printer

Thanks for the pointer. I will submit a separate CL for this.

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:112: if (lastSelectedPrinterIndex !=
selectedPrinter)
On 2011/04/27 23:46:36, Lei Zhang wrote:
> no need for this if statement.

Done.

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:348:
addDestinationListOption(localStrings.getString('managePrinters'), false);
On 2011/04/27 23:46:36, Lei Zhang wrote:
> nit: can you keep the new line after this?

Done.

http://codereview.chromium.org/6903083/diff/22/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:358: * Adds an option to the
destination list.
On 2011/04/27 23:46:36, Lei Zhang wrote:
> nit: the printer destination list.

Done.

[csilv, 2011-04-28 00:28:37]

LGTM (mac specific code)

[Lei Zhang, 2011-04-28 01:14:12]

LGTM with nits below:

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
File chrome/browser/printing/printer_manager_dialog_win.cc (right):

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
chrome/browser/printing/printer_manager_dialog_win.cc:10: #include
"base/file_util.h"
nit: you only need file_path.h.

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
chrome/browser/printing/printer_manager_dialog_win.cc:28:
PathService::Get(base::DIR_SYSTEM, &shell32dll);
only need to call PathService::Get() once:

FilePath sys_dir;
PathService::Get(base::DIR_SYSTEM, &sys_dir);
FilePath rundll32 = sys_dir.AppendASCII("rundll32.exe");
FilePath shell32dll = sys_dir.AppendASCII("shell32.dll");

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
chrome/browser/printing/printer_manager_dialog_win.cc:43: DCHECK(thread);
nit: you don't need this DCHECK. If |thread| is null, You'll crash immediately
afterwards anyway. You can just call
g_browser_process->file_thread()->message_loop()...

[kmadhusu, 2011-04-28 15:44:21]

thestig: Fixed nits.

Thanks.

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
File chrome/browser/printing/printer_manager_dialog_win.cc (right):

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
chrome/browser/printing/printer_manager_dialog_win.cc:10: #include
"base/file_util.h"
On 2011/04/28 01:14:12, Lei Zhang wrote:
> nit: you only need file_path.h.

Done.

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
chrome/browser/printing/printer_manager_dialog_win.cc:28:
PathService::Get(base::DIR_SYSTEM, &shell32dll);
On 2011/04/28 01:14:12, Lei Zhang wrote:
> only need to call PathService::Get() once:
> 
> FilePath sys_dir;
> PathService::Get(base::DIR_SYSTEM, &sys_dir);
> FilePath rundll32 = sys_dir.AppendASCII("rundll32.exe");
> FilePath shell32dll = sys_dir.AppendASCII("shell32.dll");

Done.

http://codereview.chromium.org/6903083/diff/2006/chrome/browser/printing/prin...
chrome/browser/printing/printer_manager_dialog_win.cc:43: DCHECK(thread);
On 2011/04/28 01:14:12, Lei Zhang wrote:
> nit: you don't need this DCHECK. If |thread| is null, You'll crash immediately
> afterwards anyway. You can just call
> g_browser_process->file_thread()->message_loop()...

Done.