First cut of Cloud Print Proxy implementation. The code is not enabled for no...

First cut of Cloud Print Proxy implementation. The code is not enabled for now. Soon the cloud print proxy code will move from the browser process to a background process called the service process.
BUG=None
TEST=None for now


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

[M-A Ruel, 2010-04-16 03:18:09]

http://codereview.chromium.org/1566047/diff/45001/35011
File chrome/browser/printing/cloud_print/cloud_print_helpers.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35011#newcode102
chrome/browser/printing/cloud_print/cloud_print_helpers.cc:102:
message_value.release();
remove

http://codereview.chromium.org/1566047/diff/45001/35011#newcode111
chrome/browser/printing/cloud_print/cloud_print_helpers.cc:111: std::string
headers = "Authorization: GoogleLogin auth=" + auth_token;
nit: Please use += instead (multiple times).
std::string::operator + () is badly implemented unless the compiler supports
rvalue reference and pre-VS2010 doesn't.

http://codereview.chromium.org/1566047/diff/45001/35013
File chrome/browser/printing/cloud_print/cloud_print_helpers.h (right):

http://codereview.chromium.org/1566047/diff/45001/35013#newcode15
chrome/browser/printing/cloud_print/cloud_print_helpers.h:15: class URLFetcher;
add an empty line

http://codereview.chromium.org/1566047/diff/45001/35013#newcode28
chrome/browser/printing/cloud_print/cloud_print_helpers.h:28: // A helper method
that parses the response data for any cloud print server
s/A helper method that parses/Parses/

In general, make all the function description's first line imperative, while
trying to be as succinct as possible.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode29
chrome/browser/printing/cloud_print/cloud_print_helpers.h:29: // request. The
method returns false if there was an error in parsing the
s/The method returns/Returns/

http://codereview.chromium.org/1566047/diff/45001/35013#newcode43
chrome/browser/printing/cloud_print/cloud_print_helpers.h:43: // error_count is
an in/out paramater that contains the current number of
We often put the parameter name in between | so you can shorten the sentence and
the word's meaning is clearer. e.g.
- |error_count| Contains the current number of consecutive
    failed attempts. This function increments it. (in/out)

(I usually don't call a static function a method, I don't know what other people
thinks)

Feel free to reword as you prefer or keep it as it was.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode45
chrome/browser/printing/cloud_print/cloud_print_helpers.h:45: // max_retry_count
is the maximum number of retries before we want to give up.
I prefer shorter descriptions. e.g.
|max_retry_count| Number of retries before giving up.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode67
chrome/browser/printing/cloud_print/cloud_print_helpers.h:67:
CloudPrintHelpers() {
CloudPrintHelpers();  would work too.

http://codereview.chromium.org/1566047/diff/45001/35006
File chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h (right):

http://codereview.chromium.org/1566047/diff/45001/35006#newcode48
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:48: void
RegisterPrinters(const cloud_print::PrinterList& printer_list);
I don't understand your namespace convention, some stuff is in cloud_print and
some isn't.

http://codereview.chromium.org/1566047/diff/45001/35006#newcode53
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:53: class Core :
public base::RefCountedThreadSafe<CloudPrintProxyBackend::Core>,
Do you think it would be useful to define this class in the .cc file instead?
You could only declare it here.

http://codereview.chromium.org/1566047/diff/45001/35006#newcode188
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:188: };
DISALLOW_COPY_AND_ASSIGN ?

http://codereview.chromium.org/1566047/diff/45001/35009
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35009#newcode41
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:41:
PrefService* prefs = g_browser_process->local_state();
It's running in the UI thread, right?

http://codereview.chromium.org/1566047/diff/45001/35010
File chrome/browser/printing/cloud_print/job_status_updater.h (right):

http://codereview.chromium.org/1566047/diff/45001/35010#newcode17
chrome/browser/printing/cloud_print/job_status_updater.h:17: // A class that
periodically monitors the status of a local print job and
Periodically monitors ...

http://codereview.chromium.org/1566047/diff/45001/35010#newcode34
chrome/browser/printing/cloud_print/job_status_updater.h:34: // Check the status
of the local print job and send an update.
Checks ...

http://codereview.chromium.org/1566047/diff/45001/35010#newcode57
chrome/browser/printing/cloud_print/job_status_updater.h:57: 
one line only

http://codereview.chromium.org/1566047/diff/45001/35007
File chrome/browser/printing/cloud_print/printer_info.h (right):

http://codereview.chromium.org/1566047/diff/45001/35007#newcode112
chrome/browser/printing/cloud_print/printer_info.h:112: };
Is that class copyable?

http://codereview.chromium.org/1566047/diff/45001/35007#newcode113
chrome/browser/printing/cloud_print/printer_info.h:113: }  // namespace
cloud_print
spacing

http://codereview.chromium.org/1566047/diff/45001/35015
File chrome/browser/printing/cloud_print/printer_info_win.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35015#newcode50
chrome/browser/printing/cloud_print/printer_info_win.cc:50: HMODULE
prntvpt_module = GetModuleHandle(L"prntvpt.dll");
I still don't think it's useful. Simply calling LoadLibrary() should be fine.
This code is not really a performance optimisation.

http://codereview.chromium.org/1566047/diff/45001/35015#newcode130
chrome/browser/printing/cloud_print/printer_info_win.cc:130: }
}  // namespace

http://codereview.chromium.org/1566047/diff/45001/35014
File chrome/browser/printing/cloud_print/printer_job_handler.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35014#newcode24
chrome/browser/printing/cloud_print/printer_job_handler.cc:24:
auth_token_(auth_token), last_caps_hash_(caps_hash), delegate_(delegate),
nit: That's not really readable, I think it'd be better to have one per line,
even if it'll take 14 lines...

[sanjeevr, 2010-04-16 16:29:33]

I made all the changes and uploaded a new patch. I also had to add typedefs for
the various Delegate classes to work around the build error I saw on some
trybots (which I suspect is a VS 2005 bug).

http://codereview.chromium.org/1566047/diff/45001/35011
File chrome/browser/printing/cloud_print/cloud_print_helpers.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35011#newcode102
chrome/browser/printing/cloud_print/cloud_print_helpers.cc:102:
message_value.release();
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> remove

I can't remove this. Since we are returning the dictionary, we  need to detach
from the object (release() here means detach() - I know the terminology is
confusing).

http://codereview.chromium.org/1566047/diff/45001/35011#newcode111
chrome/browser/printing/cloud_print/cloud_print_helpers.cc:111: std::string
headers = "Authorization: GoogleLogin auth=" + auth_token;
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> nit: Please use += instead (multiple times).
> std::string::operator + () is badly implemented unless the compiler supports
> rvalue reference and pre-VS2010 doesn't.

Done.

http://codereview.chromium.org/1566047/diff/45001/35013
File chrome/browser/printing/cloud_print/cloud_print_helpers.h (right):

http://codereview.chromium.org/1566047/diff/45001/35013#newcode15
chrome/browser/printing/cloud_print/cloud_print_helpers.h:15: class URLFetcher;
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> add an empty line

Done.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode28
chrome/browser/printing/cloud_print/cloud_print_helpers.h:28: // A helper method
that parses the response data for any cloud print server
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> s/A helper method that parses/Parses/
> 
> In general, make all the function description's first line imperative, while
> trying to be as succinct as possible.

Done.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode29
chrome/browser/printing/cloud_print/cloud_print_helpers.h:29: // request. The
method returns false if there was an error in parsing the
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> s/The method returns/Returns/

Done.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode43
chrome/browser/printing/cloud_print/cloud_print_helpers.h:43: // error_count is
an in/out paramater that contains the current number of
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> We often put the parameter name in between | so you can shorten the sentence
and
> the word's meaning is clearer. e.g.
> - |error_count| Contains the current number of consecutive
>     failed attempts. This function increments it. (in/out)
> 
> (I usually don't call a static function a method, I don't know what other
people
> thinks)
> 
> Feel free to reword as you prefer or keep it as it was.

Done.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode45
chrome/browser/printing/cloud_print/cloud_print_helpers.h:45: // max_retry_count
is the maximum number of retries before we want to give up.
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> I prefer shorter descriptions. e.g.
> |max_retry_count| Number of retries before giving up.

Done.

http://codereview.chromium.org/1566047/diff/45001/35013#newcode67
chrome/browser/printing/cloud_print/cloud_print_helpers.h:67:
CloudPrintHelpers() {
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> CloudPrintHelpers();  would work too.

I left this as is if that is OK with you.

http://codereview.chromium.org/1566047/diff/45001/35006
File chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h (right):

http://codereview.chromium.org/1566047/diff/45001/35006#newcode48
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:48: void
RegisterPrinters(const cloud_print::PrinterList& printer_list);
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> I don't understand your namespace convention, some stuff is in cloud_print and
> some isn't.

I have kind of followed what the browser_sync code does. The  actual
CloudPrintxxxx classes are not in the namespace. However potentially generic
classes and interfaces like PrinterList and EnumeratePrinters which are
specifically meant for cloud print are in a namespace. I don't have any strong
feelings about this and I can change this any way you suggest.

http://codereview.chromium.org/1566047/diff/45001/35006#newcode53
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:53: class Core :
public base::RefCountedThreadSafe<CloudPrintProxyBackend::Core>,
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> Do you think it would be useful to define this class in the .cc file instead?
> You could only declare it here.

Done.

http://codereview.chromium.org/1566047/diff/45001/35006#newcode188
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:188: };
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> DISALLOW_COPY_AND_ASSIGN ?

Done.

http://codereview.chromium.org/1566047/diff/45001/35009
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35009#newcode41
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:41:
PrefService* prefs = g_browser_process->local_state();
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> It's running in the UI thread, right?

Yes.

http://codereview.chromium.org/1566047/diff/45001/35010
File chrome/browser/printing/cloud_print/job_status_updater.h (right):

http://codereview.chromium.org/1566047/diff/45001/35010#newcode17
chrome/browser/printing/cloud_print/job_status_updater.h:17: // A class that
periodically monitors the status of a local print job and
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> Periodically monitors ...

Done.

http://codereview.chromium.org/1566047/diff/45001/35010#newcode34
chrome/browser/printing/cloud_print/job_status_updater.h:34: // Check the status
of the local print job and send an update.
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> Checks ...

Done.

http://codereview.chromium.org/1566047/diff/45001/35010#newcode57
chrome/browser/printing/cloud_print/job_status_updater.h:57: 
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> one line only

Done.

http://codereview.chromium.org/1566047/diff/45001/35007
File chrome/browser/printing/cloud_print/printer_info.h (right):

http://codereview.chromium.org/1566047/diff/45001/35007#newcode112
chrome/browser/printing/cloud_print/printer_info.h:112: };
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> Is that class copyable?

Added DISALLOW_COPY_...

http://codereview.chromium.org/1566047/diff/45001/35007#newcode113
chrome/browser/printing/cloud_print/printer_info.h:113: }  // namespace
cloud_print
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> spacing

Done.

http://codereview.chromium.org/1566047/diff/45001/35015
File chrome/browser/printing/cloud_print/printer_info_win.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35015#newcode50
chrome/browser/printing/cloud_print/printer_info_win.cc:50: HMODULE
prntvpt_module = GetModuleHandle(L"prntvpt.dll");
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> I still don't think it's useful. Simply calling LoadLibrary() should be fine.
> This code is not really a performance optimisation.

This is not meant to be a performance optimization. I have run into way too many
issues because of DLLs being pinned too long because of extra refcounts. In this
case, it should not matter so I made the change you asked for.

http://codereview.chromium.org/1566047/diff/45001/35015#newcode130
chrome/browser/printing/cloud_print/printer_info_win.cc:130: }
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> }  // namespace

Done.

http://codereview.chromium.org/1566047/diff/45001/35014
File chrome/browser/printing/cloud_print/printer_job_handler.cc (right):

http://codereview.chromium.org/1566047/diff/45001/35014#newcode24
chrome/browser/printing/cloud_print/printer_job_handler.cc:24:
auth_token_(auth_token), last_caps_hash_(caps_hash), delegate_(delegate),
On 2010/04/16 03:18:09, Marc-Antoine Ruel wrote:
> nit: That's not really readable, I think it'd be better to have one per line,
> even if it'll take 14 lines...

Done.

[M-A Ruel, 2010-04-19 19:07:37]

Previous changes ok.

http://codereview.chromium.org/1566047/diff/54001/52010
File chrome/browser/printing/cloud_print/cloud_print_helpers.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52010#newcode156
chrome/browser/printing/cloud_print/cloud_print_helpers.cc:156: void
CloudPrintHelpers::CreateMimeBoundaryForUpload(std::string *out) {
std::string* out

http://codereview.chromium.org/1566047/diff/54001/52012
File chrome/browser/printing/cloud_print/cloud_print_helpers.h (right):

http://codereview.chromium.org/1566047/diff/54001/52012#newcode42
chrome/browser/printing/cloud_print/cloud_print_helpers.h:42: // The paramaters
are as below:
parameters

http://codereview.chromium.org/1566047/diff/54001/52020
File chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52020#newcode69
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:69: //
Prototype for a response handler.
align +2

http://codereview.chromium.org/1566047/diff/54001/52020#newcode95
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:95: //
Registers printer capabalities and defaults for the next printer in the
capabilities

http://codereview.chromium.org/1566047/diff/54001/52020#newcode105
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:105: //
Initialize a job handler object for the specified printer. The job
Initializes

http://codereview.chromium.org/1566047/diff/54001/52020#newcode124
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:124: // Th
eunique id for this proxy
The unique

http://codereview.chromium.org/1566047/diff/54001/52020#newcode141
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:141: bool
new_printers_avaiable_;
available?

http://codereview.chromium.org/1566047/diff/54001/52020#newcode142
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:142: };
Disallow copy on every classes.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode262
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:262: // For the
next printer to be uploaded , create a multi-part post request to
uploaded,

http://codereview.chromium.org/1566047/diff/54001/52020#newcode376
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:376:
RemovePrinterFromList(printer_name);
What happens if:
- Computer A shares network Printer X, registers it.
- Computer B shares network Printer X, see it's already registered so don't
register its endpoint.
- Computer A shuts down.

Probably just worth filing a bug for now.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode404
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:404:
printer_data->GetString(kIdValue, &printer_id);
Maybe worth looking for a "valid" printer_id ? At least the value is a non-empty
string.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode413
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:413:
&printer_info.printer_status);
Q: So individual printer status is only updated on whole printer list refresh?

http://codereview.chromium.org/1566047/diff/54001/52020#newcode414
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:414:
DCHECK(!printer_id.empty());
By then it's a bit late. I think you should bail out if the printer_name,
printer_status or caps_hash aren't valid (empty except for hash isn't valid
length/value).

http://codereview.chromium.org/1566047/diff/54001/52020#newcode451
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:451:
MessageLoop::current()->PostTask(FROM_HERE, next_task);
Even if the response wasn't json? Think about a wifi hotspot that mangles http
requests until logged in.

http://codereview.chromium.org/1566047/diff/54001/52005
File chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h (right):

http://codereview.chromium.org/1566047/diff/54001/52005#newcode14
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:14: #include
"chrome/browser/printing/cloud_print/printer_job_handler.h"
Is this include needed?

http://codereview.chromium.org/1566047/diff/54001/52008
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52008#newcode52
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:52:
token_to_use = WideToUTF8(prefs->GetString(prefs::kCloudPrintAuthToken));
Won't this fail after a DisableForUser() call because of line 64?

http://codereview.chromium.org/1566047/diff/54001/52008#newcode64
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:64:
prefs->ClearPref(prefs::kCloudPrintAuthToken);
I wonder why you do line 33 when you sometimes delete the value.

http://codereview.chromium.org/1566047/diff/54001/52008#newcode105
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:105: void
CloudPrintProxyService::Observe(NotificationType type,
Why do to implement this interface then?

http://codereview.chromium.org/1566047/diff/54001/52011
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.h (right):

http://codereview.chromium.org/1566047/diff/54001/52011#newcode22
chrome/browser/printing/cloud_print/cloud_print_proxy_service.h:22: //
CloudPrintProxyService is the layer between the browser user interface
bit: Layer between ...

http://codereview.chromium.org/1566047/diff/54001/52011#newcode50
chrome/browser/printing/cloud_print/cloud_print_proxy_service.h:50: // Note that
sid can be empty. This is placeholder code until we can get
nit: sid? I know what a SID is but I don't expect others to understand. :) Since
it's a placeholder anyway, just put TODO(sanjeevr) instead.

http://codereview.chromium.org/1566047/diff/54001/52006
File chrome/browser/printing/cloud_print/printer_info.h (right):

http://codereview.chromium.org/1566047/diff/54001/52006#newcode73
chrome/browser/printing/cloud_print/printer_info.h:73: // Gets the capabilties
and defaults for a specific printer.
capabilities

http://codereview.chromium.org/1566047/diff/54001/52006#newcode111
chrome/browser/printing/cloud_print/printer_info.h:111: NotificationState*
state_;
Is it worth to be a scoped_ptr?

http://codereview.chromium.org/1566047/diff/54001/52014
File chrome/browser/printing/cloud_print/printer_info_win.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52014#newcode303
chrome/browser/printing/cloud_print/printer_info_win.cc:303: if
(print_data_mime_type == "application/pdf") {
nit: Maybe worth checking the mime type before opening the printer?

http://codereview.chromium.org/1566047/diff/54001/52014#newcode330
chrome/browser/printing/cloud_print/printer_info_win.cc:330: if
(ERROR_INVALID_PARAMETER != last_error) {
Please check for ERROR_INSUFFICIENT_BUFFER instead and that bytes_needed != 0.
It will trap other unknown failures.

http://codereview.chromium.org/1566047/diff/54001/52014#newcode366
chrome/browser/printing/cloud_print/printer_info_win.cc:366: DWORD last_error =
GetLastError();
remove?

http://codereview.chromium.org/1566047/diff/54001/52014#newcode494
chrome/browser/printing/cloud_print/printer_info_win.cc:494: PRINTER_INFO_2
*printer_info_win =
PRINTER_INFO_2* printer_info_win

http://codereview.chromium.org/1566047/diff/54001/52013
File chrome/browser/printing/cloud_print/printer_job_handler.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52013#newcode66
chrome/browser/printing/cloud_print/printer_job_handler.cc:66: // We want to
ignore the other ones that happen a task is in progress.
... that happens while a task ..é

http://codereview.chromium.org/1566047/diff/54001/52013#newcode114
chrome/browser/printing/cloud_print/printer_job_handler.cc:114: // (could be
printer name, description, status or capabilties).
capabilities

It is weird that printer info is updated at print job level. I'm wondering why
it's not in CloudPrintProxyBackend.

http://codereview.chromium.org/1566047/diff/54001/52013#newcode126
chrome/browser/printing/cloud_print/printer_job_handler.cc:126: // go for free)
go for free = is discarded?

http://codereview.chromium.org/1566047/diff/54001/52015
File chrome/browser/printing/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/1566047/diff/54001/52015#newcode81
chrome/browser/printing/cloud_print/printer_job_handler.h:81: //
print_data_file_path_.value().clear();
?

http://codereview.chromium.org/1566047/diff/54001/52003
File chrome/browser/profile.h (right):

http://codereview.chromium.org/1566047/diff/54001/52003#newcode356
chrome/browser/profile.h:356: virtual CloudPrintProxyService*
GetCloudPrintProxyService() = 0;
Are you sure it belongs to the profile? What's the difference between the
incognito cloud print proxy and the non-incognito one?

[sanjeevr, 2010-04-19 21:17:59]

Made the changes and uploaded a new patch.

http://codereview.chromium.org/1566047/diff/54001/52010
File chrome/browser/printing/cloud_print/cloud_print_helpers.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52010#newcode156
chrome/browser/printing/cloud_print/cloud_print_helpers.cc:156: void
CloudPrintHelpers::CreateMimeBoundaryForUpload(std::string *out) {
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> std::string* out

Done.

http://codereview.chromium.org/1566047/diff/54001/52020
File chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52020#newcode69
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:69: //
Prototype for a response handler.
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> align +2

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode95
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:95: //
Registers printer capabalities and defaults for the next printer in the
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> capabilities

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode105
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:105: //
Initialize a job handler object for the specified printer. The job
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Initializes

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode124
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:124: // Th
eunique id for this proxy
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> The unique

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode141
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:141: bool
new_printers_avaiable_;
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> available?

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode142
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:142: };
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Disallow copy on every classes.

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode262
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:262: // For the
next printer to be uploaded , create a multi-part post request to
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> uploaded,

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode376
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:376:
RemovePrinterFromList(printer_name);
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> What happens if:
> - Computer A shares network Printer X, registers it.
> - Computer B shares network Printer X, see it's already registered so don't
> register its endpoint.
> - Computer A shuts down.
> 
> Probably just worth filing a bug for now.

This is as designed. Computer B will also register printer X. The printer id is
server-generated and so the same printer can be registered to multiple proxies.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode404
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:404:
printer_data->GetString(kIdValue, &printer_id);
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Maybe worth looking for a "valid" printer_id ? At least the value is a
non-empty
> string.

Done.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode413
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:413:
&printer_info.printer_status);
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Q: So individual printer status is only updated on whole printer list refresh?

No, it is updated within the PrinterJobHandler class.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode414
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:414:
DCHECK(!printer_id.empty());
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> By then it's a bit late. I think you should bail out if the printer_name,
> printer_status or caps_hash aren't valid (empty except for hash isn't valid
> length/value).

Status can be empty. Printer name is the only required field. I added a DCHECK
for this case.

http://codereview.chromium.org/1566047/diff/54001/52020#newcode451
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.cc:451:
MessageLoop::current()->PostTask(FROM_HERE, next_task);
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Even if the response wasn't json? Think about a wifi hotspot that mangles http
> requests until logged in.

We don't want to stop registering printers even if the upload for one printer
failed. In the case you mention, retrying will not help. All the printers will
fail to register.

http://codereview.chromium.org/1566047/diff/54001/52005
File chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h (right):

http://codereview.chromium.org/1566047/diff/54001/52005#newcode14
chrome/browser/printing/cloud_print/cloud_print_proxy_backend.h:14: #include
"chrome/browser/printing/cloud_print/printer_job_handler.h"
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Is this include needed?

Done.
Moved to cc file

http://codereview.chromium.org/1566047/diff/54001/52008
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52008#newcode52
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:52:
token_to_use = WideToUTF8(prefs->GetString(prefs::kCloudPrintAuthToken));
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Won't this fail after a DisableForUser() call because of line 64?

In theory (in future). Currently when DisableForUser is called, the user is
signed out and the next time when EnableForUser is called, there will an
auth_token supplied. However, this logic will also change when we move to the
service process.

http://codereview.chromium.org/1566047/diff/54001/52008#newcode64
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:64:
prefs->ClearPref(prefs::kCloudPrintAuthToken);
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> I wonder why you do line 33 when you sometimes delete the value.

??

http://codereview.chromium.org/1566047/diff/54001/52008#newcode105
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:105: void
CloudPrintProxyService::Observe(NotificationType type,
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Why do to implement this interface then?

This was because I anticipated that I will need it. Since I am not using it, I
have deleted this.

http://codereview.chromium.org/1566047/diff/54001/52011
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.h (right):

http://codereview.chromium.org/1566047/diff/54001/52011#newcode22
chrome/browser/printing/cloud_print/cloud_print_proxy_service.h:22: //
CloudPrintProxyService is the layer between the browser user interface
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> bit: Layer between ...

Done.

http://codereview.chromium.org/1566047/diff/54001/52011#newcode50
chrome/browser/printing/cloud_print/cloud_print_proxy_service.h:50: // Note that
sid can be empty. This is placeholder code until we can get
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> nit: sid? I know what a SID is but I don't expect others to understand. :)
Since
> it's a placeholder anyway, just put TODO(sanjeevr) instead.

This code is going away soon.

Done.

http://codereview.chromium.org/1566047/diff/54001/52006
File chrome/browser/printing/cloud_print/printer_info.h (right):

http://codereview.chromium.org/1566047/diff/54001/52006#newcode73
chrome/browser/printing/cloud_print/printer_info.h:73: // Gets the capabilties
and defaults for a specific printer.
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> capabilities

Done.

http://codereview.chromium.org/1566047/diff/54001/52006#newcode111
chrome/browser/printing/cloud_print/printer_info.h:111: NotificationState*
state_;
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Is it worth to be a scoped_ptr?

Not possible because NotificationState is a platform specific define, that has
no type information here.

http://codereview.chromium.org/1566047/diff/54001/52014
File chrome/browser/printing/cloud_print/printer_info_win.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52014#newcode303
chrome/browser/printing/cloud_print/printer_info_win.cc:303: if
(print_data_mime_type == "application/pdf") {
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> nit: Maybe worth checking the mime type before opening the printer?

We were handling multiple MIME types at one point and we will be handling
multiple types again in the near future. I guess we could check for the whole
list up front but that will need 2 checks.

http://codereview.chromium.org/1566047/diff/54001/52014#newcode330
chrome/browser/printing/cloud_print/printer_info_win.cc:330: if
(ERROR_INVALID_PARAMETER != last_error) {
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Please check for ERROR_INSUFFICIENT_BUFFER instead and that bytes_needed != 0.
> It will trap other unknown failures.

But I want to DCHECK on failures other than ERROR_INSUFFICIENT_BUFFER and
ERROR_INVALID_PARAMETER. I can restructure the code to do this but I don't think
it is worthwhile.

http://codereview.chromium.org/1566047/diff/54001/52014#newcode366
chrome/browser/printing/cloud_print/printer_info_win.cc:366: DWORD last_error =
GetLastError();
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> remove?

Done.

http://codereview.chromium.org/1566047/diff/54001/52014#newcode494
chrome/browser/printing/cloud_print/printer_info_win.cc:494: PRINTER_INFO_2
*printer_info_win =
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> PRINTER_INFO_2* printer_info_win

Done.

http://codereview.chromium.org/1566047/diff/54001/52013
File chrome/browser/printing/cloud_print/printer_job_handler.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52013#newcode66
chrome/browser/printing/cloud_print/printer_job_handler.cc:66: // We want to
ignore the other ones that happen a task is in progress.
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> ... that happens while a task ..é

Done.

http://codereview.chromium.org/1566047/diff/54001/52013#newcode114
chrome/browser/printing/cloud_print/printer_job_handler.cc:114: // (could be
printer name, description, status or capabilties).
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> capabilities
> 
> It is weird that printer info is updated at print job level. I'm wondering why
> it's not in CloudPrintProxyBackend.

This is not the print job level. PrinterJobHandler handles all tasks for a
particular printer. JobStatusUpdater is the class to update the status of an
individual job.

http://codereview.chromium.org/1566047/diff/54001/52013#newcode126
chrome/browser/printing/cloud_print/printer_job_handler.cc:126: // go for free)
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> go for free = is discarded?

Means that we don't have a hash for the defaults. If the caps have changed, we
also upload the defaults. Changed the comment slightly.

http://codereview.chromium.org/1566047/diff/54001/52015
File chrome/browser/printing/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/1566047/diff/54001/52015#newcode81
chrome/browser/printing/cloud_print/printer_job_handler.h:81: //
print_data_file_path_.value().clear();
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> ?

There seems to be no way to clear a FilePath (unless I am missing something).

http://codereview.chromium.org/1566047/diff/54001/52003
File chrome/browser/profile.h (right):

http://codereview.chromium.org/1566047/diff/54001/52003#newcode356
chrome/browser/profile.h:356: virtual CloudPrintProxyService*
GetCloudPrintProxyService() = 0;
On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> Are you sure it belongs to the profile? What's the difference between the
> incognito cloud print proxy and the non-incognito one?

This is exactly like GetProfileSyncService.

[M-A Ruel, 2010-04-19 23:01:27]

Basically lgtm with one small change (no need to wait for me)

http://codereview.chromium.org/1566047/diff/54001/52013
File chrome/browser/printing/cloud_print/printer_job_handler.cc (right):

http://codereview.chromium.org/1566047/diff/54001/52013#newcode114
chrome/browser/printing/cloud_print/printer_job_handler.cc:114: // (could be
printer name, description, status or capabilties).
On 2010/04/19 21:17:59, sanjeevr wrote:
> On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> > capabilities
> > 
> > It is weird that printer info is updated at print job level. I'm wondering
why
> > it's not in CloudPrintProxyBackend.
> 
> This is not the print job level. PrinterJobHandler handles all tasks for a
> particular printer. JobStatusUpdater is the class to update the status of an
> individual job.

I was confused.

http://codereview.chromium.org/1566047/diff/54001/52015
File chrome/browser/printing/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/1566047/diff/54001/52015#newcode81
chrome/browser/printing/cloud_print/printer_job_handler.h:81: //
print_data_file_path_.value().clear();
On 2010/04/19 21:17:59, sanjeevr wrote:
> On 2010/04/19 19:07:37, Marc-Antoine Ruel wrote:
> > ?
> 
> There seems to be no way to clear a FilePath (unless I am missing something).

print_data_file_path_ = FilePath();

[sanjeevr, 2010-04-20 04:09:51]

Fixed and submitted.