GCP2.0 Device: Receiving printjobs.

GCP2.0 Device: Receiving printjobs.
New request (with timeouts).

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213839

[maksymb, 2013-07-19 22:51:09]

Please take a look at my CL.

[Vitaly Buka (NO REVIEWS), 2013-07-22 03:57:31]

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
File cloud_print/gcp20/prototype/cloud_print_request.cc (right):

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_request.cc:77:
base::Unretained(delegate_)));
That's incorrect.
Here you post task with plain pointer to delegate. Nothing prevents task from
beeing executed after delegate is deleted.

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_request.cc:90: FROM_HERE,
Same here.

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_request.cc:93:
MessageLoop::current()->PostTask(FROM_HERE, base::Bind(parser_, response));
Similar for parser_.
Caller should expect that parser_ will not be called if CreateRequest is
deleted. It's not true for this code.

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_request.cc:97:
base::Bind(&CloudPrintRequest::Delegate::OnFetchError,
Same.

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_request.cc:100: NOTIMPLEMENTED() <<
"HTTP code: " << http_code;
I'd recoment to PostTask only  for CloudPrintRequest:: methods and user WeakPtr

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_requester.cc:313:
base::Bind(&CloudPrintRequester::Delegate::OnServerError,
same issue with delegate_

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
File cloud_print/gcp20/prototype/cloud_print_requester.h (right):

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_requester.h:25: using
cloud_print_response_parser::Printjob;
Don't use using like that 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_requester.h:62: const
std::vector<Printjob>& printjobs) = 0;
OnPrintJobAvailable

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_requester.h:65: virtual void
OnPrintjobDownloaded(const Printjob& printjob) = 0;
Job

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_requester.h:65: virtual void
OnPrintjobDownloaded(const Printjob& printjob) = 0;
PrintJob

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_requester.h:98: void
RequestPrintjob(const Printjob& printjob);
print_job

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/cloud_print_requester.h:101: void
SendPrintjobDone(const std::string& jobid);
job_id

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
File cloud_print/gcp20/prototype/dns_sd_server.cc (right):

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/dns_sd_server.cc:286: // TODO(maksymb): Replace with
weakptr.
Don't leave this. fix ASAP.
Please create new CL and review all PostTask usage in existing code.

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
File cloud_print/gcp20/prototype/printer.cc (right):

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/printer.cc:202:
base::Bind(&Printer::WaitUserConfirmation, this->AsWeakPtr(),
remote this->

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
File cloud_print/gcp20/prototype/printjob_handler.cc (right):

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/printjob_handler.cc:5: #include
"cloud_print/gcp20/prototype/printjob_handler.h"
print_job_handler.h"

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
File cloud_print/gcp20/prototype/printjob_handler.h (right):

https://chromiumcodereview.appspot.com/19866002/diff/10001/cloud_print/gcp20/...
cloud_print/gcp20/prototype/printjob_handler.h:12: class PrintjobHandler {
PrintJobHandler

[maksymb, 2013-07-22 22:56:51]

Please take a look at
cloud_print/gcp20/prototype/cloud_print_requester.cc:313:
reply.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.cc (right):

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:77:
base::Unretained(delegate_)));
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> That's incorrect.
> Here you post task with plain pointer to delegate. Nothing prevents task from
> beeing executed after delegate is deleted.

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:90: FROM_HERE,
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> Same here.

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:93:
MessageLoop::current()->PostTask(FROM_HERE, base::Bind(parser_, response));
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> Similar for parser_.
> Caller should expect that parser_ will not be called if CreateRequest is
> deleted. It's not true for this code.

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:97:
base::Bind(&CloudPrintRequest::Delegate::OnFetchError,
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> Same.

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:100: NOTIMPLEMENTED() <<
"HTTP code: " << http_code;
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> I'd recoment to PostTask only  for CloudPrintRequest:: methods and user
WeakPtr

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.cc:313:
base::Bind(&CloudPrintRequester::Delegate::OnServerError,
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> same issue with delegate_

No, this callback can be called only in this function.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.h (right):

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:25: using
cloud_print_response_parser::Printjob;
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> Don't use using like that 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:62: const
std::vector<Printjob>& printjobs) = 0;
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> OnPrintJobAvailable

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:65: virtual void
OnPrintjobDownloaded(const Printjob& printjob) = 0;
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> PrintJob

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:65: virtual void
OnPrintjobDownloaded(const Printjob& printjob) = 0;
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> Job

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:98: void
RequestPrintjob(const Printjob& printjob);
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> print_job

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:101: void
SendPrintjobDone(const std::string& jobid);
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> job_id

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/dns_sd_server.cc (right):

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/dns_sd_server.cc:286: // TODO(maksymb): Replace with
weakptr.
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> Don't leave this. fix ASAP.
> Please create new CL and review all PostTask usage in existing code.

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:202:
base::Bind(&Printer::WaitUserConfirmation, this->AsWeakPtr(),
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> remote this->

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printjob_handler.cc (right):

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printjob_handler.cc:5: #include
"cloud_print/gcp20/prototype/printjob_handler.h"
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> print_job_handler.h"

Done.

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printjob_handler.h (right):

https://codereview.chromium.org/19866002/diff/10001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printjob_handler.h:12: class PrintjobHandler {
On 2013/07/22 03:57:31, Vitaly Buka wrote:
> PrintJobHandler

Done.

[Vitaly Buka (NO REVIEWS), 2013-07-22 23:46:02]

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:90: VLOG(1) << response;
remove if, you can control VLOG level

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.h (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:33: int server_code, int
server_http_code) = 0;
line break before int server_http_code

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.cc:24: const bool
kFunctionVebose = true;
remove this
just use VLOG with and --v swith to control log level

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/dns_sd_server.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/dns_sd_server.cc:284:
base::MessageLoop::current()->PostDelayedTask(
???

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/print_job_handler.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:20: return
base::FilePath(base::UTF8ToWide(path));
Remove function

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:46: std::string directory =
std::string directory -> FilePath directory

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:56: GetPath(directory +
"/ticket.xml"), ticket.data(), ticket.size());
FilePath::Append

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:63: GetPath(directory +
"/data.pdf"), data.data(), data.size());
FilePath::Append

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/print_job_handler.h (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.h:17: bool SavePrintJob(const
std::string& data, const std::string& ticket,
line break before every param

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.h (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.h:86: };
empty line in between

[maksymb, 2013-07-23 00:40:38]

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:90: VLOG(1) << response;
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> remove if, you can control VLOG level

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.h (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:33: int server_code, int
server_http_code) = 0;
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> line break before int server_http_code

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.cc:24: const bool
kFunctionVebose = true;
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> remove this
> just use VLOG with and --v swith to control log level

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/dns_sd_server.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/dns_sd_server.cc:284:
base::MessageLoop::current()->PostDelayedTask(
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> ???

Sorry, I've forgot to delete this TODO after resolving :)

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/print_job_handler.cc (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:20: return
base::FilePath(base::UTF8ToWide(path));
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> Remove function

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:46: std::string directory =
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> std::string directory -> FilePath directory

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:56: GetPath(directory +
"/ticket.xml"), ticket.data(), ticket.size());
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> FilePath::Append

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:63: GetPath(directory +
"/data.pdf"), data.data(), data.size());
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> FilePath::Append

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/print_job_handler.h (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.h:17: bool SavePrintJob(const
std::string& data, const std::string& ticket,
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> line break before every param

Done.

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.h (right):

https://codereview.chromium.org/19866002/diff/21001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.h:86: };
On 2013/07/22 23:46:02, Vitaly Buka wrote:
> empty line in between

Done.

[Vitaly Buka (NO REVIEWS), 2013-07-23 00:44:11]

lgtm

https://codereview.chromium.org/19866002/diff/43001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/print_job_handler.cc (right):

https://codereview.chromium.org/19866002/diff/43001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:30: if
(!base::DirectoryExists(directory))
if (!base::DirectoryExists(directory) &&
    !file_util::CreateDirectory(directory)) {
}

https://codereview.chromium.org/19866002/diff/43001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:38: if
(!base::DirectoryExists(directory))
same

[maksymb, 2013-07-23 00:57:16]

https://codereview.chromium.org/19866002/diff/43001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/print_job_handler.cc (right):

https://codereview.chromium.org/19866002/diff/43001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:30: if
(!base::DirectoryExists(directory))
On 2013/07/23 00:44:11, Vitaly Buka wrote:
> if (!base::DirectoryExists(directory) &&
>     !file_util::CreateDirectory(directory)) {
> }

Done.

https://codereview.chromium.org/19866002/diff/43001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/print_job_handler.cc:38: if
(!base::DirectoryExists(directory))
On 2013/07/23 00:44:11, Vitaly Buka wrote:
> same

Done.

[gene, 2013-07-23 09:36:23]

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.cc (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:115:
request->fetcher_.reset(URLFetcher::Create(url, method, request.get()));
nit: this is not very clean method to create an object an then modify it's
private members
Here are a few ways to make it cleaner:
- Have constructor do that CloudPrintRequest(url, method, delegate). I don't see
a reason for static CreateGet and CreatePost in this case.

- If you like to have 2 step initialization. Have a non-static Create(url,
method) method. Both CreateGet and CreatePost will do:
  new CloudPrintRequest
  request->Create()

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.h (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:41: virtual std::string
access_token() = 0;
GetAccessToken() - ???
By formatting function a getter you assume a certain simple implementation.
However, you don't know how delegate implementor will actually implement this.
It is better to follow regular naming conversion here.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:44: virtual
scoped_refptr<net::URLRequestContextGetter> context_getter() = 0;
GetContextGetter()
same here

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:49: struct DataForUpload {
Looks like you use this struct only once to pass params.
Since it has only 2 members, I would suggest get rid of this struct and pass
them to function directly.
For example,
static scoped_ptr<CloudPrintRequest> CreatePost(
    const GURL& url,
    const std::string& data_mimetype,
    const std::string& data,
    Delegate* delegate);

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:58: const GURL& url,
nit: looks like you can put params to the function line:
static scoped_ptr<CloudPrintRequest> CreateGet(const GURL& url,
                                                                            
Delegate* delegate);

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:63: const GURL& url,
nit: as above comment?

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:68: void Run(const
ParserCallback& parser);
This is really confusing. Why do you need both delegate and callback????
Looking at the cc file, looks like you call callback immediately after
OnFetchComplete(). Why don't you just add const std::string& parameter to
OnFetchComplete and get rid of callback here?

After reading class that uses CloudPrintRequest I see why you go this way. Still
it will be better to keep only delegate or only callbacks. Since your delegate
functions are called only once, it will be easier to switch everything to
callbacks if you like. How about this API:
    Run(success_callback, error_callback, access_token, context_getter)
Here:
    error_callback(bool timeout, server_api, server_code, http_error_code)
???

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.cc:308: bool success =
cloud_print_response_parser::ParseFetchResponse(
This part is a bit confusing. Reading the code, it feels like not all code path
result in delegate invocation. Correct me if I am wrong.
Current implementation assumes that ParseFetchResponse will call callback in
case of error and also return false in this function, correct?

Do you think it will be simpler to return error parameters here, and this
function will make a decision to call OnServerError or OnPrintJobsAvailable ???

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.h (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:19: typedef
base::Callback<void(const std::string&)> ParserCallback;
Looks like this is defined in 2 places, here and in CloudPrintRequest

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:160: WakeUp();
remove if() and just call WakeUp() ???
since WakeUp() already checking for !IsRegistered()

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:581: if (state == OFFLINE)
switch() {
case
}

here??

[maksymb, 2013-07-23 19:02:37]

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.cc (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.cc:115:
request->fetcher_.reset(URLFetcher::Create(url, method, request.get()));
On 2013/07/23 09:36:23, gene wrote:
> nit: this is not very clean method to create an object an then modify it's
> private members
> Here are a few ways to make it cleaner:
> - Have constructor do that CloudPrintRequest(url, method, delegate). I don't
see
> a reason for static CreateGet and CreatePost in this case.
> 
> - If you like to have 2 step initialization. Have a non-static Create(url,
> method) method. Both CreateGet and CreatePost will do:
>   new CloudPrintRequest
>   request->Create()
>   

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_request.h (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:41: virtual std::string
access_token() = 0;
On 2013/07/23 09:36:23, gene wrote:
> GetAccessToken() - ???
> By formatting function a getter you assume a certain simple implementation.
> However, you don't know how delegate implementor will actually implement this.
> It is better to follow regular naming conversion here.

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:44: virtual
scoped_refptr<net::URLRequestContextGetter> context_getter() = 0;
On 2013/07/23 09:36:23, gene wrote:
> GetContextGetter()
> same here

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:49: struct DataForUpload {
On 2013/07/23 09:36:23, gene wrote:
> Looks like you use this struct only once to pass params.
> Since it has only 2 members, I would suggest get rid of this struct and pass
> them to function directly.
> For example,
> static scoped_ptr<CloudPrintRequest> CreatePost(
>     const GURL& url,
>     const std::string& data_mimetype,
>     const std::string& data,
>     Delegate* delegate);

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:58: const GURL& url,
On 2013/07/23 09:36:23, gene wrote:
> nit: looks like you can put params to the function line:
> static scoped_ptr<CloudPrintRequest> CreateGet(const GURL& url,
>                                                                             
> Delegate* delegate);

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:63: const GURL& url,
On 2013/07/23 09:36:23, gene wrote:
> nit: as above comment?

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_request.h:68: void Run(const
ParserCallback& parser);
On 2013/07/23 09:36:23, gene wrote:
> This is really confusing. Why do you need both delegate and callback????
> Looking at the cc file, looks like you call callback immediately after
> OnFetchComplete(). Why don't you just add const std::string& parameter to
> OnFetchComplete and get rid of callback here?
> 
> After reading class that uses CloudPrintRequest I see why you go this way.
Still
> it will be better to keep only delegate or only callbacks. Since your delegate
> functions are called only once, it will be easier to switch everything to
> callbacks if you like. How about this API:
>     Run(success_callback, error_callback, access_token, context_getter)
> Here:
>     error_callback(bool timeout, server_api, server_code, http_error_code)
> ???

Left Delegate only.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.cc:308: bool success =
cloud_print_response_parser::ParseFetchResponse(
On 2013/07/23 09:36:23, gene wrote:
> This part is a bit confusing. Reading the code, it feels like not all code
path
> result in delegate invocation. Correct me if I am wrong.
> Current implementation assumes that ParseFetchResponse will call callback in
> case of error and also return false in this function, correct?
> 
> Do you think it will be simpler to return error parameters here, and this
> function will make a decision to call OnServerError or OnPrintJobsAvailable
???

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.h (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.h:19: typedef
base::Callback<void(const std::string&)> ParserCallback;
On 2013/07/23 09:36:23, gene wrote:
> Looks like this is defined in 2 places, here and in CloudPrintRequest

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:160: WakeUp();
On 2013/07/23 09:36:23, gene wrote:
> remove if() and just call WakeUp() ???
> since WakeUp() already checking for !IsRegistered() 

Done.

https://codereview.chromium.org/19866002/diff/56001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:581: if (state == OFFLINE)
On 2013/07/23 09:36:23, gene wrote:
> switch() {
> case
> }
> 
> here??

Done.

[gene, 2013-07-24 16:32:17]

lgtm
small nit below

https://codereview.chromium.org/19866002/diff/62001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/19866002/diff/62001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.cc:336: if (!success) {
nit: it will be easier to read
if (success) {
  delegate_->OnPrintJobsAvailable(list);
} else {
  delegate_->OnServerError(error_description);
}

[maksymb, 2013-07-24 17:29:15]

https://codereview.chromium.org/19866002/diff/62001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/19866002/diff/62001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/cloud_print_requester.cc:336: if (!success) {
On 2013/07/24 16:32:17, gene wrote:
> nit: it will be easier to read
> if (success) {
>   delegate_->OnPrintJobsAvailable(list);
> } else {
>   delegate_->OnServerError(error_description);
> }

Done.

[commit-bot: I haz the power, 2013-07-24 22:54:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maksymb@chromium.org/19866002/73001

[commit-bot: I haz the power, 2013-07-25 13:37:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maksymb@chromium.org/19866002/81001

[commit-bot: I haz the power, 2013-07-25 20:42:06]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-25 20:50:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maksymb@chromium.org/19866002/100001

[commit-bot: I haz the power, 2013-07-26 01:32:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maksymb@chromium.org/19866002/100001

[commit-bot: I haz the power, 2013-07-26 11:24:41]

Change committed as 213839