Move common cloud print methods from service/cloud_print to common/cloud_print.

Move common cloud print code from service/cloud_print to common/cloud_print.

Sharable constants and methods are moved to common/cloud_print so they can be shared by all cloud print related features.  Classes/constants/methods in service/cloud_print are moved into namespace cloud_print. Tests are added. 

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

[msw, 2012-11-09 23:18:17]

Sorry for the slightly delayed review, I hope you don't mind trying to make the
suggested changes.

https://codereview.chromium.org/11360151/diff/4002/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/chrome_common.gypi#...
chrome/chrome_common.gypi:91: 'common/cloud_print/cloud_print_consts.cc',
Rename these *constants.h, the more common name ~(83 vs 6):
http://code.google.com/p/chromium/source/search?q=file:.*.constants.h+-third_...
http://code.google.com/p/chromium/source/search?q=file:.*.consts.h+-third_party

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
File chrome/common/cloud_print/cloud_print_consts.cc (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:9: // Values in parameters when
updating or registering a printer at cloud print
nit: remove duplicated comments.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:21: // Values in the respone
JSON from the cloud print server
nit: remove duplicated comments.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:37: // Printer tag names. Don't
need prefixes. They will be added on submit.
nit: put this comment in the header; split the list there too.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:42: // Certain cloud print
requests require Chrome's X-CloudPrint-Proxy header.
nit: put this comment in the header.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
File chrome/common/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_helpers.h:9: #include <set>
This looks unused here, please remove.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_helpers.h:16: class ListValue;
This looks unused here, please remove.

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_consts.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_consts.h:5: #ifndef
CHROME_SERVICE_CLOUD_PRINT_CLOUD_PRINT_CONSTS_H_
For a simple constants file, I'd suggest just moving the whole file to
chrome/common, instead of splitting it up (then you can also ignore my nits
therein if you wish).

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_helpers.h:5: #ifndef
CHROME_SERVICE_CLOUD_PRINT_CLOUD_PRINT_HELPERS_H_
I know I originally split up this file into
chrome/common/cloud_print/cloud_print_helpers.h|cc to access a subset of
helpers, but at this point I'd suggest moving the remainder of this file there
to consolidate the helpers in one spot (presuming there's nothing
service-dependent here).

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
ditto for unit tests, move it to common instead of splitting it up.

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_wipeout.cc (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_wipeout.cc:38: printer_id,
nit: match indent change on these lines with the line above.

[Chen Yu, 2012-11-13 14:02:14]

Thank you very much for the review, Mike! PTAL.

https://codereview.chromium.org/11360151/diff/4002/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/chrome_common.gypi#...
chrome/chrome_common.gypi:91: 'common/cloud_print/cloud_print_consts.cc',
On 2012/11/09 23:18:17, msw wrote:
> Rename these *constants.h, the more common name ~(83 vs 6):
>
http://code.google.com/p/chromium/source/search?q=file:.*.constants.h+-third_...
>
http://code.google.com/p/chromium/source/search?q=file:.*.consts.h+-third_party

Done.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
File chrome/common/cloud_print/cloud_print_consts.cc (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:9: // Values in parameters when
updating or registering a printer at cloud print
On 2012/11/09 23:18:17, msw wrote:
> nit: remove duplicated comments.

Done.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:21: // Values in the respone
JSON from the cloud print server
On 2012/11/09 23:18:17, msw wrote:
> nit: remove duplicated comments.

Done.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:37: // Printer tag names. Don't
need prefixes. They will be added on submit.
On 2012/11/09 23:18:17, msw wrote:
> nit: put this comment in the header; split the list there too.

Done.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_consts.cc:42: // Certain cloud print
requests require Chrome's X-CloudPrint-Proxy header.
On 2012/11/09 23:18:17, msw wrote:
> nit: put this comment in the header.

Done.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
File chrome/common/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_helpers.h:9: #include <set>
On 2012/11/09 23:18:17, msw wrote:
> This looks unused here, please remove.

Done.

https://codereview.chromium.org/11360151/diff/4002/chrome/common/cloud_print/...
chrome/common/cloud_print/cloud_print_helpers.h:16: class ListValue;
On 2012/11/09 23:18:17, msw wrote:
> This looks unused here, please remove.

Done.

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_consts.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_consts.h:5: #ifndef
CHROME_SERVICE_CLOUD_PRINT_CLOUD_PRINT_CONSTS_H_
Thanks for the suggestions! I moved those not service specific constants to
browser/common/cloud_print

There are still some in service/cloud_print/cloud_print_consts.h. (Here is the
link for the convenience:
https://codereview.chromium.org/11360151/diff/7007/chrome/service/cloud_print...)

One section is about tag names; they are different for different usage (for
example, we use "__apns__" for ios in chrome_to_mobile job receiving)

One is about the strings for the reasons of job fetching. I think they could be
different in different scenarios. So I keep them there.

The rest is about retry limit and waiting time. I keep them there as I think
they could be different in different usages.

What do you think? Thank you!


On 2012/11/09 23:18:17, msw wrote:
> For a simple constants file, I'd suggest just moving the whole file to
> chrome/common, instead of splitting it up (then you can also ignore my nits
> therein if you wish).

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_helpers.h:5: #ifndef
CHROME_SERVICE_CLOUD_PRINT_CLOUD_PRINT_HELPERS_H_
I moved most of them except those depends on 
(1) PrintJobStatus or PrintJobDetails (they are defined in service: 
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/service/cloud_print...),

and (2) TokenStore
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/service/cloud_print...)

On 2012/11/09 23:18:17, msw wrote:
> I know I originally split up this file into
> chrome/common/cloud_print/cloud_print_helpers.h|cc to access a subset of
> helpers, but at this point I'd suggest moving the remainder of this file there
> to consolidate the helpers in one spot (presuming there's nothing
> service-dependent here).

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_wipeout.cc (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_wipeout.cc:38: printer_id,
On 2012/11/09 23:18:17, msw wrote:
> nit: match indent change on these lines with the line above.

Done.

[msw, 2012-11-17 00:22:30]

Please be more thoroughly descriptive in the CL description (mention consts,
tests, etc.) and assign BUG=, filing an issue if needed.

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_consts.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_consts.h:5: #ifndef
CHROME_SERVICE_CLOUD_PRINT_CLOUD_PRINT_CONSTS_H_
On 2012/11/13 14:02:15, Chen Yu wrote:
> Thanks for the suggestions! I moved those not service specific constants to
> browser/common/cloud_print
> 
> There are still some in service/cloud_print/cloud_print_consts.h. (Here is the
> link for the convenience:
>
https://codereview.chromium.org/11360151/diff/7007/chrome/service/cloud_print...)
> 
> One section is about tag names; they are different for different usage (for
> example, we use "__apns__" for ios in chrome_to_mobile job receiving)
> 
> One is about the strings for the reasons of job fetching. I think they could
be
> different in different scenarios. So I keep them there.
> 
> The rest is about retry limit and waiting time. I keep them there as I think
> they could be different in different usages.
> 
> What do you think? Thank you!
> 
> 
> On 2012/11/09 23:18:17, msw wrote:
> > For a simple constants file, I'd suggest just moving the whole file to
> > chrome/common, instead of splitting it up (then you can also ignore my nits
> > therein if you wish).
> 

I'd marginally prefer to keep all the constants defined together, even if some
are only used within chrome/service, just so there's a single constants file.
That said, it's really up to cloud print OWNERS.

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_helpers.h:5: #ifndef
CHROME_SERVICE_CLOUD_PRINT_CLOUD_PRINT_HELPERS_H_
On 2012/11/13 14:02:15, Chen Yu wrote:
> I moved most of them except those depends on 
> (1) PrintJobStatus or PrintJobDetails (they are defined in service: 
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/service/cloud_print...),
> 
> and (2) TokenStore
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/service/cloud_print...)
> 
> On 2012/11/09 23:18:17, msw wrote:
> > I know I originally split up this file into
> > chrome/common/cloud_print/cloud_print_helpers.h|cc to access a subset of
> > helpers, but at this point I'd suggest moving the remainder of this file
there
> > to consolidate the helpers in one spot (presuming there's nothing
> > service-dependent here).
> 

sounds good.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_constants.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.cc:9: const char
kCloudPrintUserAgent[] = "GoogleCloudPrintProxy";
This constant is defined in both files :(

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.cc:12: const char
kProxyAuthUserAgent[] = "ChromiumBrowser";
This constant is defined in both files :(

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_constants.h (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.h:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
Why are you omitting and removing (c) from some files? Please be consistent.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.h:12: // The string to be
appended to the user-agent for cloudprint requests.
nit: "cloud print"

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.h:23: // Values in parameters
when updating or registering a printer at cloud print
nit: use the original comment or consider:
"Values used to register or update a printer with the cloud print service."

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:23: cloud_print::PrinterTags
PreparePrinterTags(
nit: move the anon namespace into the cloud_print namespace and nix all the
cloud_print:: specifiers.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:30: using base::SysInfo;
nit: this doesn't help much, just specify base:: in the two uses below.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:38: std::string
HashPrinterTags(const cloud_print::PrinterTags& strings) {
nit: comment.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:77: std::string
path(cloud_print::AppendPathToUrl(
nit: nix all the cloud_print:: specifiers.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:224: return
std::string("multipart/form-data; boundary=") += mime_boundary;
Why +=? shouldn't this just be +?

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:238: it !=
printer_tags_prepared.end(); ++it) {
nit: I prefer wrapping within the parens (indent one more space).

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:253: it != printer_tags.end();
++it) {
ditto nit: indent one more space.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:261: std::string
msg(proxy_tag_prefix);
nit: use stringprintf here

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:268: std::string
tags_hash_msg(tags_hash_tag_name);
nit: use stringprintf here

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:281: for (it = tags.begin(); it
!= tags.end(); ++it) {
return std::find(tags.begin(), tags.end(), tag_dry_run_flag) != tags.end();

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:289: std::string header;
nit: use stringprintf here

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:21: typedef
std::map<std::string, std::string> PrinterTags;
nit: comment on the significance of printer tags.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:70: // Returns the mime type of
multipart with |mime_boundary|.
nit: MIME

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:76: // Returns an MD5 hash for
printer tags from |printer_tags| and the default tags
nit: remove "printer tags from", |printer_tags| alone is sufficient; perhaps
also change "tags required by cloud print server" with just "required tags".

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:80: // Returns an post data for
printer tags from |printer_tags| and the default
nit: s/an/the/ and perhaps just say "Get the post data for |printer_tags| and
the default required tags."

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:88: // Returns true is tags
indicate a dry run (test) job.
nit: s/is/if/

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:92: // Created CloudPrint auth
header from the auth token.
nit: "Get the cloud print auth header from |auth_token|."

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers_unittest.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:13: using namespace
cloud_print;
nit: Put everything here in the cloud_print namespace instead.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:17: void
CheckURLs(const GURL& server_base_url) {
nit: Test calling helpers in the order they are declared.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:47: }
Add cases for the other helpers: GetUrlForJobDelete, GetUrlForJobStatusUpdate,
GetUrlForUserMessage, GetUrlForGetAuthCode.
And if you don't mind, add tests for the other GetUrl helpers that are already
there: GetUrlForSearch, GetUrlForSubmit.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:49:
TEST(CloudPrintHelpersTest, GetURLs) {
For bonus points, add more tests for other helper functions that you're moving,
and those already in the file :)

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_auth.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_auth.cc:12: #include
"chrome/service/cloud_print/cloud_print_helpers.h"
This isn't used any more, please remove.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_auth.cc:41:
cloud_print::kProxyAuthUserAgent,
nit: consider putting this file's contents in the cloud_print namespace and
removing the specifiers.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_connector.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_connector.cc:18: #include
"chrome/service/cloud_print/cloud_print_consts.h"
Please remove if now unused.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_connector.cc:43:
ReportUserMessage(cloud_print::kPrintSystemFailedMessageId,
nit: consider putting this file's contents in the cloud_print namespace and
removing the specifiers.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.cc:7: #include
"base/json/json_reader.h"
nit: please go through this list and remove any unused includes.
Example: base/sys_info.h is no longer used here.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.cc:84: return
cloud_print::IsDryRunJob(tags, kTagDryRunFlag);
Do you expect other dry run flags to be specified? Otherwise the common/ impl
can just use this flag itself and you can remove this impl.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.h:16: class DictionaryValue;
Remove this unnecessary forward decl.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (left):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:48: url =
CloudPrintHelpers::GetUrlForJobStatusUpdate(
You should leave these test cases (three calls to GetUrlForJobStatusUpdate) here
or add them to the unit test file in common/.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:80: url =
CloudPrintHelpers::GetUrlForUserMessage(server_base_url,
You should leave these test cases (calls to GetUrlForUserMessage and
GetUrlForGetAuthCode) here or add them to the unit test file in common/.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:20: #include
"chrome/service/cloud_print/cloud_print_consts.h"
ditto nit to remove this include if unused.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:378:
subscription.channel = cloud_print::kCloudPrintPushNotificationsSource;
ditto nit: put this file in the cloud_print namespace and remove the specifiers.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_url_fetcher.cc:11: #include
"chrome/service/cloud_print/cloud_print_consts.h"
ditto nit: remove this include if unused.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_url_fetcher.cc:212:
base::StringAppendF(&user_agent, " %s", cloud_print::kCloudPrintUserAgent);
ditto nit: put this file in the cloud_print namespace and remove the specifiers.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_wipeout.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_wipeout.cc:9: #include
"chrome/service/cloud_print/cloud_print_helpers.h"
Remove this unused include.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_wipeout.cc:37: GURL url =
cloud_print::GetUrlForPrinterDelete(cloud_print_server_url_,
ditto nit: put this file in the cloud_print namespace and remove the specifiers.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/printer_job_handler.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/printer_job_handler.cc:17: #include
"chrome/service/cloud_print/cloud_print_consts.h"
ditto nit to remove this include if unused.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/printer_job_handler.cc:261: if
(json_data->GetList(cloud_print::kJobListValue, &job_list) && job_list) {
ditto nit: put this file in the cloud_print namespace and remove the specifiers.

[Chen Yu, 2012-11-26 12:07:06]

Thank you very much, Mike! Very sorry for being slow on it. Please take another
look. Thanks!

I am going to file a bug and update the bug number.

Yu

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
File chrome/service/cloud_print/cloud_print_consts.h (right):

https://codereview.chromium.org/11360151/diff/4002/chrome/service/cloud_print...
chrome/service/cloud_print/cloud_print_consts.h:5: #ifndef
CHROME_SERVICE_CLOUD_PRINT_CLOUD_PRINT_CONSTS_H_
On 2012/11/17 00:22:30, msw wrote:
> On 2012/11/13 14:02:15, Chen Yu wrote:
> > Thanks for the suggestions! I moved those not service specific constants to
> > browser/common/cloud_print
> > 
> > There are still some in service/cloud_print/cloud_print_consts.h. (Here is
the
> > link for the convenience:
> >
>
https://codereview.chromium.org/11360151/diff/7007/chrome/service/cloud_print...)
> > 
> > One section is about tag names; they are different for different usage (for
> > example, we use "__apns__" for ios in chrome_to_mobile job receiving)
> > 
> > One is about the strings for the reasons of job fetching. I think they could
> be
> > different in different scenarios. So I keep them there.
> > 
> > The rest is about retry limit and waiting time. I keep them there as I think
> > they could be different in different usages.
> > 
> > What do you think? Thank you!
> > 
> > 
> > On 2012/11/09 23:18:17, msw wrote:
> > > For a simple constants file, I'd suggest just moving the whole file to
> > > chrome/common, instead of splitting it up (then you can also ignore my
nits
> > > therein if you wish).
> > 
> 
> I'd marginally prefer to keep all the constants defined together, even if some
> are only used within chrome/service, just so there's a single constants file.
> That said, it's really up to cloud print OWNERS.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_constants.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.cc:9: const char
kCloudPrintUserAgent[] = "GoogleCloudPrintProxy";
On 2012/11/17 00:22:30, msw wrote:
> This constant is defined in both files :(

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.cc:12: const char
kProxyAuthUserAgent[] = "ChromiumBrowser";
On 2012/11/17 00:22:30, msw wrote:
> This constant is defined in both files :(

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_constants.h (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.h:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
This file is actually new created, and it is to follow
https://code.google.com/p/chromium/issues/detail?id=140977
(I undo the (c) removal in other files)

On 2012/11/17 00:22:30, msw wrote:
> Why are you omitting and removing (c) from some files? Please be consistent.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.h:12: // The string to be
appended to the user-agent for cloudprint requests.
On 2012/11/17 00:22:30, msw wrote:
> nit: "cloud print"

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_constants.h:23: // Values in parameters
when updating or registering a printer at cloud print
On 2012/11/17 00:22:30, msw wrote:
> nit: use the original comment or consider:
> "Values used to register or update a printer with the cloud print service."

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:23: cloud_print::PrinterTags
PreparePrinterTags(
On 2012/11/17 00:22:30, msw wrote:
> nit: move the anon namespace into the cloud_print namespace and nix all the
> cloud_print:: specifiers.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:30: using base::SysInfo;
On 2012/11/17 00:22:30, msw wrote:
> nit: this doesn't help much, just specify base:: in the two uses below.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:38: std::string
HashPrinterTags(const cloud_print::PrinterTags& strings) {
On 2012/11/17 00:22:30, msw wrote:
> nit: comment.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:77: std::string
path(cloud_print::AppendPathToUrl(
On 2012/11/17 00:22:30, msw wrote:
> nit: nix all the cloud_print:: specifiers.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:224: return
std::string("multipart/form-data; boundary=") += mime_boundary;
On 2012/11/17 00:22:30, msw wrote:
> Why +=? shouldn't this just be +?

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:238: it !=
printer_tags_prepared.end(); ++it) {
Changed to call HashPrinterTags() 
On 2012/11/17 00:22:30, msw wrote:
> nit: I prefer wrapping within the parens (indent one more space).

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:253: it != printer_tags.end();
++it) {
On 2012/11/17 00:22:30, msw wrote:
> ditto nit: indent one more space.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:261: std::string
msg(proxy_tag_prefix);
On 2012/11/17 00:22:30, msw wrote:
> nit: use stringprintf here

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:268: std::string
tags_hash_msg(tags_hash_tag_name);
On 2012/11/17 00:22:30, msw wrote:
> nit: use stringprintf here

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:281: for (it = tags.begin(); it
!= tags.end(); ++it) {
On 2012/11/17 00:22:30, msw wrote:
> return std::find(tags.begin(), tags.end(), tag_dry_run_flag) != tags.end();

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:289: std::string header;
On 2012/11/17 00:22:30, msw wrote:
> nit: use stringprintf here

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:21: typedef
std::map<std::string, std::string> PrinterTags;
On 2012/11/17 00:22:30, msw wrote:
> nit: comment on the significance of printer tags.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:70: // Returns the mime type of
multipart with |mime_boundary|.
On 2012/11/17 00:22:30, msw wrote:
> nit: MIME

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:76: // Returns an MD5 hash for
printer tags from |printer_tags| and the default tags
On 2012/11/17 00:22:30, msw wrote:
> nit: remove "printer tags from", |printer_tags| alone is sufficient; perhaps
> also change "tags required by cloud print server" with just "required tags".

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:80: // Returns an post data for
printer tags from |printer_tags| and the default
On 2012/11/17 00:22:30, msw wrote:
> nit: s/an/the/ and perhaps just say "Get the post data for |printer_tags| and
> the default required tags."

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:88: // Returns true is tags
indicate a dry run (test) job.
On 2012/11/17 00:22:30, msw wrote:
> nit: s/is/if/

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.h:92: // Created CloudPrint auth
header from the auth token.
On 2012/11/17 00:22:30, msw wrote:
> nit: "Get the cloud print auth header from |auth_token|."

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers_unittest.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:13: using namespace
cloud_print;
On 2012/11/17 00:22:30, msw wrote:
> nit: Put everything here in the cloud_print namespace instead.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:17: void
CheckURLs(const GURL& server_base_url) {
On 2012/11/17 00:22:30, msw wrote:
> nit: Test calling helpers in the order they are declared.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:47: }
On 2012/11/17 00:22:30, msw wrote:
> Add cases for the other helpers: GetUrlForJobDelete, GetUrlForJobStatusUpdate,
> GetUrlForUserMessage, GetUrlForGetAuthCode.
> And if you don't mind, add tests for the other GetUrl helpers that are already
> there: GetUrlForSearch, GetUrlForSubmit.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:49:
TEST(CloudPrintHelpersTest, GetURLs) {
On 2012/11/17 00:22:30, msw wrote:
> For bonus points, add more tests for other helper functions that you're
moving,
> and those already in the file :)

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_auth.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_auth.cc:12: #include
"chrome/service/cloud_print/cloud_print_helpers.h"
On 2012/11/17 00:22:30, msw wrote:
> This isn't used any more, please remove.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_auth.cc:41:
cloud_print::kProxyAuthUserAgent,
On 2012/11/17 00:22:30, msw wrote:
> nit: consider putting this file's contents in the cloud_print namespace and
> removing the specifiers.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_connector.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_connector.cc:18: #include
"chrome/service/cloud_print/cloud_print_consts.h"
On 2012/11/17 00:22:30, msw wrote:
> Please remove if now unused.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_connector.cc:43:
ReportUserMessage(cloud_print::kPrintSystemFailedMessageId,
On 2012/11/17 00:22:30, msw wrote:
> nit: consider putting this file's contents in the cloud_print namespace and
> removing the specifiers.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.cc:7: #include
"base/json/json_reader.h"
On 2012/11/17 00:22:30, msw wrote:
> nit: please go through this list and remove any unused includes.
> Example: base/sys_info.h is no longer used here.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.cc:84: return
cloud_print::IsDryRunJob(tags, kTagDryRunFlag);
On 2012/11/17 00:22:30, msw wrote:
> Do you expect other dry run flags to be specified? Otherwise the common/ impl
> can just use this flag itself and you can remove this impl.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.h:16: class DictionaryValue;
On 2012/11/17 00:22:30, msw wrote:
> Remove this unnecessary forward decl.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (left):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:48: url =
CloudPrintHelpers::GetUrlForJobStatusUpdate(
On 2012/11/17 00:22:30, msw wrote:
> You should leave these test cases (three calls to GetUrlForJobStatusUpdate)
here
> or add them to the unit test file in common/.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:80: url =
CloudPrintHelpers::GetUrlForUserMessage(server_base_url,
On 2012/11/17 00:22:30, msw wrote:
> You should leave these test cases (calls to GetUrlForUserMessage and
> GetUrlForGetAuthCode) here or add them to the unit test file in common/.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:20: #include
"chrome/service/cloud_print/cloud_print_consts.h"
On 2012/11/17 00:22:30, msw wrote:
> ditto nit to remove this include if unused.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:378:
subscription.channel = cloud_print::kCloudPrintPushNotificationsSource;
On 2012/11/17 00:22:30, msw wrote:
> ditto nit: put this file in the cloud_print namespace and remove the
specifiers.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_url_fetcher.cc:11: #include
"chrome/service/cloud_print/cloud_print_consts.h"
On 2012/11/17 00:22:30, msw wrote:
> ditto nit: remove this include if unused.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_url_fetcher.cc:212:
base::StringAppendF(&user_agent, " %s", cloud_print::kCloudPrintUserAgent);
On 2012/11/17 00:22:30, msw wrote:
> ditto nit: put this file in the cloud_print namespace and remove the
specifiers.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_wipeout.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_wipeout.cc:9: #include
"chrome/service/cloud_print/cloud_print_helpers.h"
On 2012/11/17 00:22:30, msw wrote:
> Remove this unused include.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_wipeout.cc:37: GURL url =
cloud_print::GetUrlForPrinterDelete(cloud_print_server_url_,
On 2012/11/17 00:22:30, msw wrote:
> ditto nit: put this file in the cloud_print namespace and remove the
specifiers.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
File chrome/service/cloud_print/printer_job_handler.cc (right):

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/printer_job_handler.cc:17: #include
"chrome/service/cloud_print/cloud_print_consts.h"
On 2012/11/17 00:22:30, msw wrote:
> ditto nit to remove this include if unused.

Done.

https://codereview.chromium.org/11360151/diff/14002/chrome/service/cloud_prin...
chrome/service/cloud_print/printer_job_handler.cc:261: if
(json_data->GetList(cloud_print::kJobListValue, &job_list) && job_list) {
On 2012/11/17 00:22:30, msw wrote:
> ditto nit: put this file in the cloud_print namespace and remove the
specifiers.

Done.

[Chen Yu, 2012-11-27 23:34:32]

Mike, do you know anyone from cloud print service OWNERS? Thanks!

[msw, 2012-11-29 18:43:13]

Please do file an issue for BUG=. It looks like sanjeevr touched a bit of this
code, as have cloud_print/service/OWNERS gene@ and vitalybuka; cc'ed here, can
one of you review? I'll take another quick pass too.

[msw, 2012-11-29 19:57:36]

LGTM with nits; thanks for all the cleanup and extra tests!

https://codereview.chromium.org/11360151/diff/25007/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://codereview.chromium.org/11360151/diff/25007/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:23: PrinterTags
PreparePrinterTags(const PrinterTags& printer_tags) {
nit: sorry if I was unclear, please keep PreparePrinterTags and HashPrinterTags
within an anonymous namespace within the cloud_print namespace, just to
reinforce that these are local functions.

https://codereview.chromium.org/11360151/diff/25007/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers.cc:229: for
(PrinterTags::const_iterator it = printer_tags_prepared.begin();
This was previously using the "un-prepared" |printer_tags| is the behavior
change to using |printer_tags_prepared| correct?

https://codereview.chromium.org/11360151/diff/25007/chrome/common/cloud_print...
File chrome/common/cloud_print/cloud_print_helpers_unittest.cc (right):

https://codereview.chromium.org/11360151/diff/25007/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:7: #include <vector>
nit: vector appears to be unused here, but i may be wrong.

https://codereview.chromium.org/11360151/diff/25007/chrome/common/cloud_print...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:18: void
CheckURLs(const GURL& server_base_url) {
optional nit: keep CheckURLs and the tests within an anonymous namespace within
the cloud_print namespace, just to reinforce that they're only used locally.

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers.h (right):

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.h:29: // Returns an post data for
printer tags in the given |printer_info|.
nit: remove 'an' or correct to 'any'.

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.h:34: // Returns true is tags
indicate a dry run (test) job.
nit: s/is/if/

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers.h:37: // Created CloudPrint auth
header from the auth token stored in the store.
nit: "cloud print"

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (right):

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:15: void
CheckJobStatusURLs(const GURL& server_base_url) {
ditto optional nit: put locals in anon namespace in the cloud_print namespace.

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_token_store.h (right):

https://codereview.chromium.org/11360151/diff/25007/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_token_store.h:10: // This class serves as
the single repository for cloud print auth tokens. This
nit: move class comment down, just above the decl.

[Chen Yu, 2012-11-30 17:35:46]

Thanks!

Gene, would you mind also having a look?

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers.cc:23: PrinterTags
PreparePrinterTags(const PrinterTags& printer_tags) {
On 2012/11/29 19:57:36, msw wrote:
> nit: sorry if I was unclear, please keep PreparePrinterTags and
HashPrinterTags
> within an anonymous namespace within the cloud_print namespace, just to
> reinforce that these are local functions.

Done.

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers.cc:229: for
(PrinterTags::const_iterator it = printer_tags_prepared.begin();
Yes, that was a mistake (sorry), and it is corrected here.
On 2012/11/29 19:57:36, msw wrote:
> This was previously using the "un-prepared" |printer_tags| is the behavior
> change to using |printer_tags_prepared| correct?

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers_unittest.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:7: #include <vector>
On 2012/11/29 19:57:36, msw wrote:
> nit: vector appears to be unused here, but i may be wrong.

Done.

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:18: void
CheckURLs(const GURL& server_base_url) {
On 2012/11/29 19:57:36, msw wrote:
> optional nit: keep CheckURLs and the tests within an anonymous namespace
within
> the cloud_print namespace, just to reinforce that they're only used locally.

Done.

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
File chrome/service/cloud_print/cloud_print_helpers.h (right):

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
chrome/service/cloud_print/cloud_print_helpers.h:29: // Returns an post data for
printer tags in the given |printer_info|.
On 2012/11/29 19:57:36, msw wrote:
> nit: remove 'an' or correct to 'any'.

Done.

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
chrome/service/cloud_print/cloud_print_helpers.h:34: // Returns true is tags
indicate a dry run (test) job.
On 2012/11/29 19:57:36, msw wrote:
> nit: s/is/if/

Done.

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
chrome/service/cloud_print/cloud_print_helpers.h:37: // Created CloudPrint auth
header from the auth token stored in the store.
On 2012/11/29 19:57:36, msw wrote:
> nit: "cloud print"

Done.

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:15: void
CheckJobStatusURLs(const GURL& server_base_url) {
On 2012/11/29 19:57:36, msw wrote:
> ditto optional nit: put locals in anon namespace in the cloud_print namespace.

Done.

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
File chrome/service/cloud_print/cloud_print_token_store.h (right):

https://chromiumcodereview.appspot.com/11360151/diff/25007/chrome/service/clo...
chrome/service/cloud_print/cloud_print_token_store.h:10: // This class serves as
the single repository for cloud print auth tokens. This
On 2012/11/29 19:57:36, msw wrote:
> nit: move class comment down, just above the decl.

Done.

[msw, 2012-11-30 18:23:08]

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers.cc:19: namespace {
No, what I mean is that you can *nest* an anonymous namespace within the
cloud_print namespace, like this:

namespace cloud_print {

namespace {

// Local functions that use cloud_print::foo without the namespace qualifier.

}  // namespace

// Public cloud_print:: functions declared in the header and used elsewhere.

}  // cloud_print

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers_unittest.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:14: namespace {
Ditto: nested namespace.

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/service/clo...
File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/service/clo...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:13: namespace {
Ditto: nested namespace.

[Chen Yu, 2012-11-30 19:37:17]

Sorry. PTAL, thanks!

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers.cc:19: namespace {
On 2012/11/30 18:23:08, msw wrote:
> No, what I mean is that you can *nest* an anonymous namespace within the
> cloud_print namespace, like this:
> 
> namespace cloud_print {
> 
> namespace {
> 
> // Local functions that use cloud_print::foo without the namespace qualifier.
> 
> }  // namespace
> 
> // Public cloud_print:: functions declared in the header and used elsewhere.
> 
> }  // cloud_print

Sorry, Done.

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers_unittest.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers_unittest.cc:14: namespace {
On 2012/11/30 18:23:08, msw wrote:
> Ditto: nested namespace.

Done.

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/service/clo...
File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/service/clo...
chrome/service/cloud_print/cloud_print_helpers_unittest.cc:13: namespace {
On 2012/11/30 18:23:08, msw wrote:
> Ditto: nested namespace.

Done.

[msw, 2012-11-30 20:12:00]

Still LGTM (with one nit); thanks again and sorry for the miscommunication.

https://chromiumcodereview.appspot.com/11360151/diff/20089/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/20089/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers.cc:25: cloud_print::PrinterTags
PreparePrinterTags(
nit: now you can remove the cloud_print:: qualifiers in this code.

[gene, 2012-11-30 20:13:58]

lgtm

On 2012/11/30 19:37:17, Chen Yu wrote:
> Sorry. PTAL, thanks!
> 
>
https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
> File chrome/common/cloud_print/cloud_print_helpers.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
> chrome/common/cloud_print/cloud_print_helpers.cc:19: namespace {
> On 2012/11/30 18:23:08, msw wrote:
> > No, what I mean is that you can *nest* an anonymous namespace within the
> > cloud_print namespace, like this:
> > 
> > namespace cloud_print {
> > 
> > namespace {
> > 
> > // Local functions that use cloud_print::foo without the namespace
qualifier.
> > 
> > }  // namespace
> > 
> > // Public cloud_print:: functions declared in the header and used elsewhere.
> > 
> > }  // cloud_print
> 
> Sorry, Done.
> 
>
https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
> File chrome/common/cloud_print/cloud_print_helpers_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/common/clou...
> chrome/common/cloud_print/cloud_print_helpers_unittest.cc:14: namespace {
> On 2012/11/30 18:23:08, msw wrote:
> > Ditto: nested namespace.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/service/clo...
> File chrome/service/cloud_print/cloud_print_helpers_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11360151/diff/29004/chrome/service/clo...
> chrome/service/cloud_print/cloud_print_helpers_unittest.cc:13: namespace {
> On 2012/11/30 18:23:08, msw wrote:
> > Ditto: nested namespace.
> 
> Done.

[commit-bot: I haz the power, 2012-11-30 20:57:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chenyu@chromium.org/11360151/15054

[commit-bot: I haz the power, 2012-11-30 20:58:05]

Presubmit check for 11360151-15054 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 3.4s to calculate.

[Chen Yu, 2012-11-30 21:01:10]

+thakis for chrome gypi OWNERS

Nico, do you mind having a look at the chrome gyp files? Thanks!

https://chromiumcodereview.appspot.com/11360151/diff/20089/chrome/common/clou...
File chrome/common/cloud_print/cloud_print_helpers.cc (right):

https://chromiumcodereview.appspot.com/11360151/diff/20089/chrome/common/clou...
chrome/common/cloud_print/cloud_print_helpers.cc:25: cloud_print::PrinterTags
PreparePrinterTags(
Sorry, forgot to clean up this file :( Thanks! Done. 
On 2012/11/30 20:12:00, msw wrote:
> nit: now you can remove the cloud_print:: qualifiers in this code.

[Nico, 2012-11-30 21:03:24]

lgtm

[commit-bot: I haz the power, 2012-12-03 00:12:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chenyu@chromium.org/11360151/29028

[commit-bot: I haz the power, 2012-12-03 03:58:28]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-03 06:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chenyu@chromium.org/11360151/29028

[commit-bot: I haz the power, 2012-12-03 08:34:48]

Change committed as 170715