Chrome proxy refactoring.

Chrome proxy refactoring.Adding myself to the OWNERS file for handling cloud print proxy issues.Split proxy code from cloud_print_proxy_backend to a separate pieces:  - auth code goes to cloud_print_auth.h/cc  - connector logic goes to cloud_print_connector.h/cc (printer enumeration, registration and deletion)  - wiring backend/frontend threads, notifications with other parts will stay in cloud_print_backend.h/ccMade proxy logic more straightforward.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107758

[Albert Bodenhamer, 2011-10-25 01:17:28]

Just getting started but here are a few initial comments.  I'll do the rest
tomorrow.

http://codereview.chromium.org/8387011/diff/4005/chrome/service/OWNERS
File chrome/service/OWNERS (left):

http://codereview.chromium.org/8387011/diff/4005/chrome/service/OWNERS#oldcode4
chrome/service/OWNERS:4: sanjeevr@chromium.org
Probably should remove sanjeev

http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/c...
File chrome/service/cloud_print/cloud_print_auth.cc (right):

http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_auth.cc:41: VLOG(1) << "CP_AUTH:
Authenticating with LSID";
Does this need to be VLOG?  There was just a thread on chromium-dev suggesting
that people use DLOG wherever possible.

http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_auth.cc:44: // the
CloudPrintProxyCoreThread and we cannot really do anything else until
Can you enforce the thread with a dcheck?

http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_auth.cc:46: std::string user_agent =
"ChromiumBrowser";
Put the string in resources or a pref or constant?

http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_auth.cc:156: // Since we specify
inifinite retries on network errors, this should never
inifinite -> infinite

[gene1, 2011-10-25 01:52:56]

Done. Please take a look.
I've moved treading comment to the cloud_print_proxy_backend.cc, where it
dchecks the right thread.


On Mon, Oct 24, 2011 at 6:17 PM, <abodenha@chromium.org> wrote:

> Just getting started but here are a few initial comments.  I'll do the rest
> tomorrow.
>
>
>
http://codereview.chromium.**org/8387011/diff/4005/chrome/**service/OWNERS<ht...
> File chrome/service/OWNERS (left):
>
> http://codereview.chromium.**org/8387011/diff/4005/chrome/**
>
service/OWNERS#oldcode4<http://codereview.chromium.org/8387011/diff/4005/chrome/service/OWNERS#oldcode4>
> chrome/service/OWNERS:4: sanjeevr@chromium.org
> Probably should remove sanjeev
>
> http://codereview.chromium.**org/8387011/diff/1028/chrome/**
>
service/cloud_print/cloud_**print_auth.cc<http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/cloud_print_auth.cc>
> File chrome/service/cloud_print/**cloud_print_auth.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/1028/chrome/**
>
service/cloud_print/cloud_**print_auth.cc#newcode41<http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/cloud_print_auth.cc#newcode41>
> chrome/service/cloud_print/**cloud_print_auth.cc:41: VLOG(1) << "CP_AUTH:
> Authenticating with LSID";
> Does this need to be VLOG?  There was just a thread on chromium-dev
> suggesting that people use DLOG wherever possible.
>
> http://codereview.chromium.**org/8387011/diff/1028/chrome/**
>
service/cloud_print/cloud_**print_auth.cc#newcode44<http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/cloud_print_auth.cc#newcode44>
> chrome/service/cloud_print/**cloud_print_auth.cc:44: // the
> CloudPrintProxyCoreThread and we cannot really do anything else until
> Can you enforce the thread with a dcheck?
>
> http://codereview.chromium.**org/8387011/diff/1028/chrome/**
>
service/cloud_print/cloud_**print_auth.cc#newcode46<http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/cloud_print_auth.cc#newcode46>
> chrome/service/cloud_print/**cloud_print_auth.cc:46: std::string
> user_agent = "ChromiumBrowser";
> Put the string in resources or a pref or constant?
>
> http://codereview.chromium.**org/8387011/diff/1028/chrome/**
>
service/cloud_print/cloud_**print_auth.cc#newcode156<http://codereview.chromium.org/8387011/diff/1028/chrome/service/cloud_print/cloud_print_auth.cc#newcode156>
> chrome/service/cloud_print/**cloud_print_auth.cc:156: // Since we specify
> inifinite retries on network errors, this should never
> inifinite -> infinite
>
>
http://codereview.chromium.**org/8387011/<http://codereview.chromium.org/8387...
>

[Albert Bodenhamer, 2011-10-26 23:58:09]

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
File chrome/service/cloud_print/cloud_print_auth.cc (right):

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_auth.cc:41: VLOG(1) << "CP_AUTH:
Authenticating with LSID";
Add comment explaining why VLOG is needed as we discussed.

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
File chrome/service/cloud_print/cloud_print_connector.cc (right):

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_connector.cc:67: VLOG(1) << "CP_PROXY:
Stopping proxy, id: " << proxy_id_;
Should we use the term proxy or connector?  I know we've been using them
interchangeably but this refactoring could be a good chance to get some
consistency here.

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_connector.cc:446: // TODO(gene): We
probably should not try indefinitely here. Just once or
Consider attaching a bug to a TODO.

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
File chrome/service/cloud_print/cloud_print_helpers.cc (right):

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_helpers.cc:276: LOG(ERROR) << "CP_PROXY:
Missing OAuth token for request";
VLOG?  DLOG?

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/c...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:406: //  request_ =
NULL;
Remove

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/j...
File chrome/service/cloud_print/job_status_updater.cc (right):

http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/j...
chrome/service/cloud_print/job_status_updater.cc:93: // TODO(gene): We might
consider stop processing if we get auth error here.
Bug?

[gene1, 2011-10-27 00:27:23]

Done. Please take a look.
some comment inline :)

On Wed, Oct 26, 2011 at 4:58 PM, <abodenha@chromium.org> wrote:

>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_auth.cc<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_auth.cc>
> File chrome/service/cloud_print/**cloud_print_auth.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_auth.cc#newcode41<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_auth.cc#newcode41>
>
> chrome/service/cloud_print/**cloud_print_auth.cc:41: VLOG(1) << "CP_AUTH:
> Authenticating with LSID";
> Add comment explaining why VLOG is needed as we discussed.
>

done.


>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_connector.cc<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_connector.cc>
> File chrome/service/cloud_print/**cloud_print_connector.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_connector.cc#newcode67<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_connector.cc#newcode67>
> chrome/service/cloud_print/**cloud_print_connector.cc:67: VLOG(1) <<
> "CP_PROXY: Stopping proxy, id: " << proxy_id_;
> Should we use the term proxy or connector?  I know we've been using them
> interchangeably but this refactoring could be a good chance to get some
> consistency here.
>

connector is a right word now. done.


>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_connector.cc#newcode446<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_connector.cc#newcode446>
> chrome/service/cloud_print/**cloud_print_connector.cc:446: // TODO(gene):
> We probably should not try indefinitely here. Just once or
> Consider attaching a bug to a TODO.
>

done. I've created bug to track this issue.


> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_helpers.cc<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_helpers.cc>
> File chrome/service/cloud_print/**cloud_print_helpers.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_helpers.cc#newcode276<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_helpers.cc#newcode276>
> chrome/service/cloud_print/**cloud_print_helpers.cc:276: LOG(ERROR) <<
> "CP_PROXY: Missing OAuth token for request";
> VLOG?  DLOG?
>

I left LOG here and added comment.


>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_proxy_backend.cc<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_proxy_backend.cc>
> File chrome/service/cloud_print/**cloud_print_proxy_backend.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/cloud_**print_proxy_backend.cc#**newcode406<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/cloud_print_proxy_backend.cc#newcode406>
> chrome/service/cloud_print/**cloud_print_proxy_backend.cc:**406: //
> request_ = NULL;
> Remove
>

done, thanks :)

>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/job_**status_updater.cc<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/job_status_updater.cc>
> File chrome/service/cloud_print/**job_status_updater.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/8005/chrome/**
>
service/cloud_print/job_**status_updater.cc#newcode93<http://codereview.chromium.org/8387011/diff/8005/chrome/service/cloud_print/job_status_updater.cc#newcode93>
> chrome/service/cloud_print/**job_status_updater.cc:93: // TODO(gene): We
> might consider stop processing if we get auth error here.
> Bug?
>

After some thoughts, I am returning STOP_PROCESSING here now. Added comment
to explain why.


>
>
http://codereview.chromium.**org/8387011/<http://codereview.chromium.org/8387...
>

[Albert Bodenhamer, 2011-10-27 15:21:40]

LGTM with one concern

The word "proxy" is still used in many places, mostly in existing code.  Should
we take this chance to make it all consistent?

[gene1, 2011-10-27 18:37:09]

Thanks! I am going to replace proxy with connector in all log strings to
make all visible output consistent.

On Thu, Oct 27, 2011 at 8:21 AM, <abodenha@chromium.org> wrote:

> LGTM with one concern
>
> The word "proxy" is still used in many places, mostly in existing code.
>  Should
> we take this chance to make it all consistent?
>
>
http://codereview.chromium.**org/8387011/<http://codereview.chromium.org/8387...
>

[Albert Bodenhamer, 2011-10-27 18:38:03]

LGTM

On Thu, Oct 27, 2011 at 11:37 AM, Gene Gutnik <gene@google.com> wrote:

> Thanks! I am going to replace proxy with connector in all log strings to
> make all visible output consistent.
>
>
> On Thu, Oct 27, 2011 at 8:21 AM, <abodenha@chromium.org> wrote:
>
>> LGTM with one concern
>>
>> The word "proxy" is still used in many places, mostly in existing code.
>>  Should
>> we take this chance to make it all consistent?
>>
>>
http://codereview.chromium.**org/8387011/<http://codereview.chromium.org/8387...
>>
>
>


-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[gene1, 2011-10-27 18:42:11]

Scott,
Do you mind taking a look? I need lgtm from somebody from the OWNERS file.
Gene

On Thu, Oct 27, 2011 at 11:37 AM, Albert Bodenhamer
<abodenha@chromium.org>wrote:

> LGTM
>
>
> On Thu, Oct 27, 2011 at 11:37 AM, Gene Gutnik <gene@google.com> wrote:
>
>> Thanks! I am going to replace proxy with connector in all log strings to
>> make all visible output consistent.
>>
>>
>> On Thu, Oct 27, 2011 at 8:21 AM, <abodenha@chromium.org> wrote:
>>
>>> LGTM with one concern
>>>
>>> The word "proxy" is still used in many places, mostly in existing code.
>>>  Should
>>> we take this chance to make it all consistent?
>>>
>>>
http://codereview.chromium.**org/8387011/<http://codereview.chromium.org/8387...
>>>
>>
>>
>
>
> --
> Albert Bodenhamer | Software Engineer |
abodenha@chromium.<abodenha@google.com>
> org
>

[Scott Byer, 2011-10-27 22:22:50]

Some spelling errors to fix.

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
File chrome/service/cloud_print/cloud_print_connector.cc (right):

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_connector.cc:41: pedning_tasks_.clear();
spelling: pending_tasks

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_connector.cc:358: task.type =
PEDNING_PRINTERS_AVAILABLE;
spelling -> PENDING

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right):

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_url_fetcher.cc:73: // If we get auth
error, notify delegate and check if it wants to procceed.
nit: spelling

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_url_fetcher.cc:112: // there is no
reasons to retry, request will never succeed.
nit: "there is no reason to retry"

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
File chrome/service/cloud_print/cloud_print_url_fetcher.h (right):

http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_url_fetcher.h:87: // Returning
CONTINUE_PROCESSING will treat auth error as a netwrok error.
nit: spelling

[gene1, 2011-10-27 22:33:08]

Thanks, done.

On Thu, Oct 27, 2011 at 3:22 PM, <scottbyer@chromium.org> wrote:

> Some spelling errors to fix.
>
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_connector.cc<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_connector.cc>
> File chrome/service/cloud_print/**cloud_print_connector.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_connector.cc#newcode41<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_connector.cc#newcode41>
> chrome/service/cloud_print/**cloud_print_connector.cc:41:
> pedning_tasks_.clear();
> spelling: pending_tasks
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_connector.cc#newcode358<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_connector.cc#newcode358>
> chrome/service/cloud_print/**cloud_print_connector.cc:358: task.type =
> PEDNING_PRINTERS_AVAILABLE;
> spelling -> PENDING
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_url_fetcher.cc<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_url_fetcher.cc>
> File chrome/service/cloud_print/**cloud_print_url_fetcher.cc (right):
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_url_fetcher.cc#newcode73<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_url_fetcher.cc#newcode73>
> chrome/service/cloud_print/**cloud_print_url_fetcher.cc:73: // If we get
> auth error, notify delegate and check if it wants to procceed.
> nit: spelling
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_url_fetcher.cc#**newcode112<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_url_fetcher.cc#newcode112>
> chrome/service/cloud_print/**cloud_print_url_fetcher.cc:**112: // there is
> no reasons to retry, request will never succeed.
> nit: "there is no reason to retry"
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_url_fetcher.h<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_url_fetcher.h>
> File chrome/service/cloud_print/**cloud_print_url_fetcher.h (right):
>
> http://codereview.chromium.**org/8387011/diff/25001/chrome/**
>
service/cloud_print/cloud_**print_url_fetcher.h#newcode87<http://codereview.chromium.org/8387011/diff/25001/chrome/service/cloud_print/cloud_print_url_fetcher.h#newcode87>
> chrome/service/cloud_print/**cloud_print_url_fetcher.h:87: // Returning
> CONTINUE_PROCESSING will treat auth error as a netwrok error.
> nit: spelling
>
>
http://codereview.chromium.**org/8387011/<http://codereview.chromium.org/8387...
>

[Scott Byer, 2011-10-27 22:35:44]

lgtm