Follow-up on http://codereview.chromium.org/2861019 to address Powel's commen...

Follow-up on http://codereview.chromium.org/2861019 to address Powel's comments.

BUG=none
TEST=none


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

[Nirnimesh, 2010-06-25 01:49:44]

http://codereview.chromium.org/2821018/diff/1/2
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2821018/diff/1/2#newcode96
chrome/browser/automation/automation_provider.cc:96: 
Pawel:

I'm tempted to use this from automation_provider_observers.cc as well.
Wouldn't it be better if this was *not* in anonymous namespace, in that case?

[Paweł Hajdan Jr., 2010-06-25 05:52:17]

http://codereview.chromium.org/2821018/diff/1/2
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2821018/diff/1/2#newcode96
chrome/browser/automation/automation_provider.cc:96: 
On 2010/06/25 01:49:44, Nirnimesh wrote:
> I'm tempted to use this from automation_provider_observers.cc as well.
> Wouldn't it be better if this was *not* in anonymous namespace, in that case?

Okay, in that case I'd just prefer moving it to a separate file, like
automation_json_util or something like that, to declutter the automation
provider.

John, the method contents look fine to me, thanks for the change. Let's just
move it to a separate file for Nirnimesh.

[John Grabowski, 2010-06-28 20:26:19]

I'm not sure a new file is justified for "just one" helper that is only ~5 lines
of code.

How about I make it static, and we all agree to create a new utils file once we
cross some reasonable threshold (e.g. 2 helpers)?

[Paweł Hajdan Jr., 2010-06-29 07:05:30]

On 2010/06/28 20:26:19, John Grabowski wrote:
> I'm not sure a new file is justified for "just one" helper that is only ~5
lines
> of code.
> 
> How about I make it static, and we all agree to create a new utils file once
we
> cross some reasonable threshold (e.g. 2 helpers)?

Sounds fine to me.

[John Grabowski, 2010-06-29 17:34:28]

LGTM then?

jrg


On Tue, Jun 29, 2010 at 12:05 AM, <phajdan.jr@chromium.org> wrote:

> On 2010/06/28 20:26:19, John Grabowski wrote:
>
>> I'm not sure a new file is justified for "just one" helper that is only ~5
>>
> lines
>
>> of code.
>>
>
>  How about I make it static, and we all agree to create a new utils file
>> once
>>
> we
>
>> cross some reasonable threshold (e.g. 2 helpers)?
>>
>
> Sounds fine to me.
>
>
> http://codereview.chromium.org/2821018/show
>

[Paweł Hajdan Jr., 2010-06-29 17:43:19]

Sorry for missing the update. However, it seems you have uploaded it a while
after sending the review e-mail.

Yeah, LGTM. Please just fix a tiny nit.

http://codereview.chromium.org/2821018/diff/6001/7002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2821018/diff/6001/7002#newcode443
chrome/browser/automation/automation_provider.h:443: static std::string
JSONErrorString(std::string err);
nit: "std::string err" -> "const std::string& err"

[John Grabowski, 2010-06-29 17:48:31]

Will do

On Tue, Jun 29, 2010 at 10:43 AM, <phajdan.jr@chromium.org> wrote:

> Sorry for missing the update. However, it seems you have uploaded it a
> while
> after sending the review e-mail.
>
> Yeah, LGTM. Please just fix a tiny nit.
>
>
> http://codereview.chromium.org/2821018/diff/6001/7002
> File chrome/browser/automation/automation_provider.h (right):
>
> http://codereview.chromium.org/2821018/diff/6001/7002#newcode443
> chrome/browser/automation/automation_provider.h:443: static std::string
> JSONErrorString(std::string err);
> nit: "std::string err" -> "const std::string& err"
>
>
> http://codereview.chromium.org/2821018/show
>