Clean-up coding style

Clean-up coding style

BUG=None (cleanup only, does not change behavior)

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

[leejongsoo, 2014-05-09 17:44:43]

Please review this.

Thanks.

[Yoyo Zhou, 2014-05-12 17:19:46]

https://codereview.chromium.org/278013002/diff/1/extensions/common/features/j...
File extensions/common/features/json_feature_provider_source.cc (right):

https://codereview.chromium.org/278013002/diff/1/extensions/common/features/j...
extensions/common/features/json_feature_provider_source.cc:7: #include <string>
Generally if we include a header in the .h file, we don't need it in the
corresponding .cc file.

https://codereview.chromium.org/278013002/diff/1/extensions/common/features/p...
File extensions/common/features/permission_feature.cc (right):

https://codereview.chromium.org/278013002/diff/1/extensions/common/features/p...
extensions/common/features/permission_feature.cc:7: #include <string>
Same here.

[leejongsoo, 2014-05-17 14:40:11]

fix parts that Yoyo comments

[leejongsoo, 2014-05-17 14:44:56]

I updated the code as reviewer's comment.
Please review this.

Thanks.

[leejongsoo, 2014-05-18 06:42:04]

The CQ bit was checked by leejongsoo@gmail.com

[leejongsoo, 2014-05-18 06:42:05]

The CQ bit was unchecked by leejongsoo@gmail.com

[Finnur, 2014-05-19 11:22:14]

Couple of comments... 

First of all, thanks for the patch. It looks like you are brand new to the
project and it is always good to see new people join. However, there are a few
rules and etiquette that is helpful to learn and follow...

1) When asking someone to review it helps to set the expectations as to *why*
you are adding that person. Is it because you want that person to look at only a
specific part of the change or all of it? Is it because someone else has
reviewed the whole thing and you just need OWNERS approval for a file or two? I
think for all files in this changlist, Yoyo and I have the exact same review
privileges, so I would probably be redundant.

2) Because you didn't specify a reason, I'm left with assuming you wanted me to
look at all of it. But when I look at the CL I get the feeling that it isn't
significant enough to warrant an extra set of eyes.

3) All changelists should have BUG=Something in the description. A small cleanup
patch like this should probably say:
BUG=None (cleanup only, does not change behavior).
Normally, we link to the bug number, as in BUG=123456789.

4) You have a small hurdle to overcome before being able to submit patches. See
below.

https://codereview.chromium.org/278013002/diff/20001/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/278013002/diff/20001/AUTHORS#newcode170
AUTHORS:170: Jongsoo Lee <leejongsoo@gmail.com>
We need a signed Contributor License Agreement before we can accept patches from
you. See:
http://www.chromium.org/developers/contributing-code#TOC-Get-your-code-ready

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
File extensions/common/features/api_feature.cc (right):

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
extensions/common/features/api_feature.cc:7: #include <string>
nit: Don't add the include in both the .cc and its .h.

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
extensions/common/features/complex_feature.cc:7: #include <set>
Set is used in the .h file and should therefore appear there (and not here).

[leejongsoo, 2014-05-20 01:19:06]

Thanks for your kind guide.

As your comment, I updated the description of this issue and completed the
Individual Contributor License Agreement.

On 2014/05/19 11:22:14, Finnur wrote:
> Couple of comments... 
> 
> First of all, thanks for the patch. It looks like you are brand new to the
> project and it is always good to see new people join. However, there are a few
> rules and etiquette that is helpful to learn and follow...
> 
> 1) When asking someone to review it helps to set the expectations as to *why*
> you are adding that person. Is it because you want that person to look at only
a
> specific part of the change or all of it? Is it because someone else has
> reviewed the whole thing and you just need OWNERS approval for a file or two?
I
> think for all files in this changlist, Yoyo and I have the exact same review
> privileges, so I would probably be redundant.
> 
> 2) Because you didn't specify a reason, I'm left with assuming you wanted me
to
> look at all of it. But when I look at the CL I get the feeling that it isn't
> significant enough to warrant an extra set of eyes.
> 
> 3) All changelists should have BUG=Something in the description. A small
cleanup
> patch like this should probably say:
> BUG=None (cleanup only, does not change behavior).
> Normally, we link to the bug number, as in BUG=123456789.
> 
> 4) You have a small hurdle to overcome before being able to submit patches.
See
> below.
> 
> https://codereview.chromium.org/278013002/diff/20001/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/278013002/diff/20001/AUTHORS#newcode170
> AUTHORS:170: Jongsoo Lee <mailto:leejongsoo@gmail.com>
> We need a signed Contributor License Agreement before we can accept patches
from
> you. See:
> http://www.chromium.org/developers/contributing-code#TOC-Get-your-code-ready
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> File extensions/common/features/api_feature.cc (right):
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> extensions/common/features/api_feature.cc:7: #include <string>
> nit: Don't add the include in both the .cc and its .h.
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> File extensions/common/features/complex_feature.cc (right):
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> extensions/common/features/complex_feature.cc:7: #include <set>
> Set is used in the .h file and should therefore appear there (and not here).

[limasdf, 2014-05-20 05:25:22]

@leejongsoo, There're some nits are not fixed yet.

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
File extensions/common/features/api_feature.cc (right):

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
extensions/common/features/api_feature.cc:7: #include <string>
On 2014/05/19 11:22:14, Finnur wrote:
> nit: Don't add the include in both the .cc and its .h.
here

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
extensions/common/features/complex_feature.cc:7: #include <set>
On 2014/05/19 11:22:14, Finnur wrote:
> Set is used in the .h file and should therefore appear there (and not here).

here

[leejongsoo, 2014-05-20 14:27:01]

@limasdf, Sorry for the faulty patch. I checked all modified files.

On 2014/05/20 05:25:22, limasdf wrote:
> @leejongsoo, There're some nits are not fixed yet.
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> File extensions/common/features/api_feature.cc (right):
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> extensions/common/features/api_feature.cc:7: #include <string>
> On 2014/05/19 11:22:14, Finnur wrote:
> > nit: Don't add the include in both the .cc and its .h.
> here
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> File extensions/common/features/complex_feature.cc (right):
> 
>
https://codereview.chromium.org/278013002/diff/20001/extensions/common/featur...
> extensions/common/features/complex_feature.cc:7: #include <set>
> On 2014/05/19 11:22:14, Finnur wrote:
> > Set is used in the .h file and should therefore appear there (and not here).
> 
> here

[Finnur, 2014-05-20 14:33:21]

LGTM

[Finnur, 2014-05-20 14:33:37]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2014-05-20 14:34:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leejongsoo@gmail.com/278013002/40001

[commit-bot: I haz the power, 2014-05-20 16:25:13]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-20 17:48:18]

Change committed as 271698