Move useful printing backend code from chrome/service/cloud_print to printing...

Move useful printing backend code from chrome/service/cloud_print to printing/backend.

BUG=none
TEST=none

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

[Lei Zhang, 2010-10-20 09:04:15]

Hi folks,

I want to move some core printer enumeration capability out of
chrome/service/cloud_print, so print preview can get the printer list in its UI.

I only did the CUPS side and haven't touched the Windows printing code yet yet.
Before I hammer away at the Windows code, I would like you to take a look and
make sure this looks ok.

http://codereview.chromium.org/3945003/diff/1/17
File printing/printing.gyp (right):

http://codereview.chromium.org/3945003/diff/1/17#newcode42
printing/printing.gyp:42: 'pdf_metafile_mac.cc',
The diff here looks confusing. All I did was run !sort on the list.

[M-A Ruel, 2010-10-20 14:42:34]

some nits, nothing huge.

http://codereview.chromium.org/3945003/diff/1/9
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3945003/diff/1/9#newcode16
chrome/service/cloud_print/print_system_cups.cc:16: #include "base/file_path.h"
Maybe an include or two aren't necessary anymore? Maybe not.

http://codereview.chromium.org/3945003/diff/1/12
File printing/backend/cups_helper.cc (right):

http://codereview.chromium.org/3945003/diff/1/12#newcode51
printing/backend/cups_helper.cc:51: static Lock init_gcrypt_lock;
This code is dependent on not using -fno-threadsafe-statics. It's fine since we
assume this code is to always be compiled with gcc, but what about clang?

Using a singleton instead would make the code slightly safer, at least w.r.t.
assuming thread safe static local variable initialization.

http://codereview.chromium.org/3945003/diff/1/14
File printing/backend/print_backend.h (right):

http://codereview.chromium.org/3945003/diff/1/14#newcode54
printing/backend/print_backend.h:54: PrinterCapsAndDefaults* printer_info) = 0;
please align

http://codereview.chromium.org/3945003/diff/1/14#newcode59
printing/backend/print_backend.h:59: // Call this function to obtain print
backend for the specified print server.
// Allocates a print backend for ...

http://codereview.chromium.org/3945003/diff/1/15
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/3945003/diff/1/15#newcode23
printing/backend/print_backend_cups.cc:23: explicit PrintBackendCUPS(const GURL&
print_server_url);
Please define the virtual destructor even if it does nothing.

[sanjeevr, 2010-10-21 17:25:38]

As I mentioned in my comment, I think you should simply replace PrintSystemXXX
with PrintBackendXXX (I am not even sure that the rename is needed). 

Also, FYI, Gene is working on a caching version of PrintSystemCUPS that caches
the printer list and periodically checks for updates. I don't think you will
need that for the browser.

http://codereview.chromium.org/3945003/diff/1/9
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3945003/diff/1/9#newcode332
chrome/service/cloud_print/print_system_cups.cc:332: printing::init_gcrypt();
I don't think you need this any more. You are calling init_gcrypt in
PrintBackendCUPS::CreateInstance

http://codereview.chromium.org/3945003/diff/1/9#newcode343
chrome/service/cloud_print/print_system_cups.cc:343: int
PrintSystemCUPS::PrintFile(const char* name, const char* filename,
Shouldn't these methods be part of the backend too? (PrintFile, GetJobs and
SpoolPrintJob)

In fact, I think we should just replace PrintSystemCUPS with PrintBackendCUPS. I
don't see what extra functionality PrintSystemXXX provided.

[Lei Zhang, 2010-10-21 22:14:39]

Sanjeev, my motivation for doing this is to reuse the printer enumeration code
without creating a dependency from src/chrome/browser/dom_ui to
src/chrome/service/cloud_print. I can't move all the code to src/printing/ for
the reason listed below.

http://codereview.chromium.org/3945003/diff/1/9
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3945003/diff/1/9#newcode343
chrome/service/cloud_print/print_system_cups.cc:343: int
PrintSystemCUPS::PrintFile(const char* name, const char* filename,
On 2010/10/21 17:25:38, sanjeevr wrote:
> Shouldn't these methods be part of the backend too? (PrintFile, GetJobs and
> SpoolPrintJob)
> 
> In fact, I think we should just replace PrintSystemCUPS with PrintBackendCUPS.
I
> don't see what extra functionality PrintSystemXXX provided.

If I put this code into src/printing, I'd need to do the same with the Windows
spooling code, but the Windows code #includes from src/chrome/service, and we
can't have a printing -> chrome/service dependency.

[Scott Byer, 2010-10-21 22:33:41]

NIt:

http://codereview.chromium.org/3945003/diff/1/15
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/3945003/diff/1/15#newcode82
printing/backend/print_backend_cups.cc:82: LOG(INFO) << "CUPS: Enumerated " <<
printer_list->size() << " printers.";
LOG(INFO) -> VLOG(1)

(and below)

[sanjeevr, 2010-10-22 06:15:52]

I see. Hmmm, looks like the Windows code could use some refactoring to make this
possible. Seems bad to put in just the enumeration code but not the actual
spooling code in the backend class.

I think we can start with what you have and then move more stuff over.


On 2010/10/21 22:14:39, Lei Zhang wrote:
> Sanjeev, my motivation for doing this is to reuse the printer enumeration code
> without creating a dependency from src/chrome/browser/dom_ui to
> src/chrome/service/cloud_print. I can't move all the code to src/printing/ for
> the reason listed below.
> 
> http://codereview.chromium.org/3945003/diff/1/9
> File chrome/service/cloud_print/print_system_cups.cc (right):
> 
> http://codereview.chromium.org/3945003/diff/1/9#newcode343
> chrome/service/cloud_print/print_system_cups.cc:343: int
> PrintSystemCUPS::PrintFile(const char* name, const char* filename,
> On 2010/10/21 17:25:38, sanjeevr wrote:
> > Shouldn't these methods be part of the backend too? (PrintFile, GetJobs and
> > SpoolPrintJob)
> > 
> > In fact, I think we should just replace PrintSystemCUPS with
PrintBackendCUPS.
> I
> > don't see what extra functionality PrintSystemXXX provided.
> 
> If I put this code into src/printing, I'd need to do the same with the Windows
> spooling code, but the Windows code #includes from src/chrome/service, and we
> can't have a printing -> chrome/service dependency.

[sanjeevr, 2010-10-22 19:27:32]

LGTM with a comment

http://codereview.chromium.org/3945003/diff/40001/41019
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/3945003/diff/40001/41019#newcode184
printing/backend/print_backend_cups.cc:184: const DictionaryValue*
print_backend_settings) {
Shouldn't we just pass in the url as a string instead of passing in a
DictionaryValue? Or do you foresee other parameters that we might want to pass
in?

[Lei Zhang, 2010-10-22 20:25:27]

http://codereview.chromium.org/3945003/diff/1/9
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3945003/diff/1/9#newcode16
chrome/service/cloud_print/print_system_cups.cc:16: #include "base/file_path.h"
On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> Maybe an include or two aren't necessary anymore? Maybe not.

Removed file_util.h.

http://codereview.chromium.org/3945003/diff/1/9#newcode332
chrome/service/cloud_print/print_system_cups.cc:332: printing::init_gcrypt();
On 2010/10/21 17:25:38, sanjeevr wrote:
> I don't think you need this any more. You are calling init_gcrypt in
> PrintBackendCUPS::CreateInstance

Removed.

http://codereview.chromium.org/3945003/diff/1/12
File printing/backend/cups_helper.cc (right):

http://codereview.chromium.org/3945003/diff/1/12#newcode51
printing/backend/cups_helper.cc:51: static Lock init_gcrypt_lock;
On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> This code is dependent on not using -fno-threadsafe-statics. It's fine since
we
> assume this code is to always be compiled with gcc, but what about clang?
> 
> Using a singleton instead would make the code slightly safer, at least w.r.t.
> assuming thread safe static local variable initialization.

Done.

http://codereview.chromium.org/3945003/diff/1/14
File printing/backend/print_backend.h (right):

http://codereview.chromium.org/3945003/diff/1/14#newcode54
printing/backend/print_backend.h:54: PrinterCapsAndDefaults* printer_info) = 0;
On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> please align

Done.

http://codereview.chromium.org/3945003/diff/1/14#newcode59
printing/backend/print_backend.h:59: // Call this function to obtain print
backend for the specified print server.
On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> // Allocates a print backend for ...

Done.

http://codereview.chromium.org/3945003/diff/1/15
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/3945003/diff/1/15#newcode23
printing/backend/print_backend_cups.cc:23: explicit PrintBackendCUPS(const GURL&
print_server_url);
On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> Please define the virtual destructor even if it does nothing.

Done.

http://codereview.chromium.org/3945003/diff/40001/41019#newcode184
printing/backend/print_backend_cups.cc:184: 
On 2010/10/22 19:27:32, sanjeevr wrote:
> Shouldn't we just pass in the url as a string instead of passing in a
> DictionaryValue? Or do you foresee other parameters that we might want to pass
> in?

I left it as a DictionaryValue so we can easily pass in new values in the
future.

[sanjeevr, 2010-10-25 17:09:44]

Still LGTM from me.

On 2010/10/22 20:25:27, Lei Zhang wrote:
> http://codereview.chromium.org/3945003/diff/1/9
> File chrome/service/cloud_print/print_system_cups.cc (right):
> 
> http://codereview.chromium.org/3945003/diff/1/9#newcode16
> chrome/service/cloud_print/print_system_cups.cc:16: #include
"base/file_path.h"
> On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> > Maybe an include or two aren't necessary anymore? Maybe not.
> 
> Removed file_util.h.
> 
> http://codereview.chromium.org/3945003/diff/1/9#newcode332
> chrome/service/cloud_print/print_system_cups.cc:332: printing::init_gcrypt();
> On 2010/10/21 17:25:38, sanjeevr wrote:
> > I don't think you need this any more. You are calling init_gcrypt in
> > PrintBackendCUPS::CreateInstance
> 
> Removed.
> 
> http://codereview.chromium.org/3945003/diff/1/12
> File printing/backend/cups_helper.cc (right):
> 
> http://codereview.chromium.org/3945003/diff/1/12#newcode51
> printing/backend/cups_helper.cc:51: static Lock init_gcrypt_lock;
> On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> > This code is dependent on not using -fno-threadsafe-statics. It's fine since
> we
> > assume this code is to always be compiled with gcc, but what about clang?
> > 
> > Using a singleton instead would make the code slightly safer, at least
w.r.t.
> > assuming thread safe static local variable initialization.
> 
> Done.
> 
> http://codereview.chromium.org/3945003/diff/1/14
> File printing/backend/print_backend.h (right):
> 
> http://codereview.chromium.org/3945003/diff/1/14#newcode54
> printing/backend/print_backend.h:54: PrinterCapsAndDefaults* printer_info) =
0;
> On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> > please align
> 
> Done.
> 
> http://codereview.chromium.org/3945003/diff/1/14#newcode59
> printing/backend/print_backend.h:59: // Call this function to obtain print
> backend for the specified print server.
> On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> > // Allocates a print backend for ...
> 
> Done.
> 
> http://codereview.chromium.org/3945003/diff/1/15
> File printing/backend/print_backend_cups.cc (right):
> 
> http://codereview.chromium.org/3945003/diff/1/15#newcode23
> printing/backend/print_backend_cups.cc:23: explicit PrintBackendCUPS(const
GURL&
> print_server_url);
> On 2010/10/20 14:42:35, Marc-Antoine Ruel wrote:
> > Please define the virtual destructor even if it does nothing.
> 
> Done.
> 
> http://codereview.chromium.org/3945003/diff/40001/41019#newcode184
> printing/backend/print_backend_cups.cc:184: 
> On 2010/10/22 19:27:32, sanjeevr wrote:
> > Shouldn't we just pass in the url as a string instead of passing in a
> > DictionaryValue? Or do you foresee other parameters that we might want to
pass
> > in?
> 
> I left it as a DictionaryValue so we can easily pass in new values in the
> future.