libchrome: Several upstreamable fixes from libchrome

libchrome: Several upstreamable fixes from libchrome

libchrome is a library in AOSP that contains parts of base/,
components/, dbus/ and sandbox/ from Chrome. It uses stricter build
warnings than Chrome (-Wunused-parameter, -Wsign-compare, -Wsign-promo),
so uprevving is kind of painful. This change has all the cleanly
upstreamable patches so that the next libchrome uprev is easier.

BUG=None
TEST=trybots, build in AOSP

[Luis Héctor Chávez, 2016-05-21 05:46:48]

lhchavez@chromium.org changed reviewers:
+ danakj@chromium.org, derat@chromium.org, rsleevi@chromium.org,
satorux@chromium.org

[Luis Héctor Chávez, 2016-05-21 05:46:48]

PTAL

danakj base/
derat components/timers/
satorux dbus/
rsleevi crypto/

All of these fixes are optional, but each one takes about 1-2 dev-minutes per
libchrome uprev, so they add up! I'm also open to make the changes in a slightly
different way (e.g. defining a macro somewhere in base/ to annotate unused
parameters).

[Ryan Sleevi, 2016-05-21 10:33:33]

Why is Android doing that? That's been something we've explicitly discouraged
multiple times?

It might be helpful to annotate or link to why of the source, as in many cases,
the existing code is more clearly/obviously correct.

https://codereview.chromium.org/1997153002/diff/60001/crypto/secure_hash.cc
File crypto/secure_hash.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/crypto/secure_hash.cc#n...
crypto/secure_hash.cc:25: SecureHashSHA256(const SecureHashSHA256& other) :
SecureHash() {
Why. The existing code is doing the right thing.

[danakj, 2016-05-23 02:59:55]

https://codereview.chromium.org/1997153002/diff/60001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/command_line.cc#ne...
base/command_line.cc:152: CommandLine::CommandLine(NoProgram /* no_program */)
Just remove the name

https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

"Parameter names may be omitted only if the parameter is unused and its purpose
is obvious."

The type name here makes it obvious

https://codereview.chromium.org/1997153002/diff/60001/base/debug/alias.cc
File base/debug/alias.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/debug/alias.cc#new...
base/debug/alias.cc:15: void Alias(const void* /* var */) {}
just remove the name, its not saying anything as |var|

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
File base/debug/stack_trace_posix.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:69: #if !defined(__UCLIBC__) &&
!defined(USE_SYMBOLIZE)
Can you explain?

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:116: #else
comment on what this is #else-ing

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:117: 
no need for the whitespace

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:120: 
ditto

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_li...
File base/files/file_util_linux.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_li...
base/files/file_util_linux.cc:27: switch
(static_cast<uintmax_t>(statfs_buf.f_type)) {
Why is this needed?

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_po...
File base/files/file_util_posix.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_po...
base/files/file_util_posix.cc:849: (void)executable;  // avoid unused warning
when !defined(OS_LINUX)
capitalize and punctuate comment senetences

https://codereview.chromium.org/1997153002/diff/60001/base/lazy_instance.h
File base/lazy_instance.h (right):

https://codereview.chromium.org/1997153002/diff/60001/base/lazy_instance.h#ne...
base/lazy_instance.h:100: static void Delete(Type* /* instance */) {}
delete the name, it adds no value

https://codereview.chromium.org/1997153002/diff/60001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/logging.cc#newcode459
base/logging.cc:459: void MakeCheckOpValueString(std::ostream* os, const
std::nullptr_t& /* p */) {
delete the name, it adds nothing

https://codereview.chromium.org/1997153002/diff/60001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1997153002/diff/60001/base/memory/ref_counted...
base/memory/ref_counted.h:428: bool operator==(const scoped_refptr<T>& lhs,
std::nullptr_t /* null */) {
delete the name

https://codereview.chromium.org/1997153002/diff/60001/base/memory/ref_counted...
base/memory/ref_counted.h:433: bool operator==(std::nullptr_t /* null */, const
scoped_refptr<T>& rhs) {
delete

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
File base/memory/shared_memory_android.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
base/memory/shared_memory_android.cc:55: bool SharedMemory::Delete(const
std::string& /* name */) {
delete

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
base/memory/shared_memory_android.cc:61: bool SharedMemory::Open(const
std::string& /* name */, bool /* read_only */) {
the |name| isn't doing much, but bool isnt clear without a name..

which brings me to why are we doing this? why can't you build chrome with the
same warnings we build chrome with?

I am not excited about trying to maintain unused variable warnings for code that
we don't care t enable that warning on. it's going to bitrot always and we'll
have to deal with followup fixes all the time.

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
base/memory/shared_memory_posix.cc:465: bool
SharedMemory::ShareToProcessCommon(ProcessHandle /* process */,
delete name

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_libevent.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:91: MessagePumpLibevent* /* pump */)
{
delete

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:101: MessagePumpLibevent* /* pump */)
{
delete

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:200: static void timer_callback(int
/* fd */, short /* events */, void* context) {
these need names to understand them though.. making them comments makes code
that doesn't follow the google style guide AFAICT. Can you contradict that?

[satorux1, 2016-05-24 01:22:14]

dbus/... LGTM. Maybe you can split changes in the directory into a separate
patch? That way you can submit this part quickly.

[Daniel Erat, 2016-05-24 14:33:20]

lgtm for components/timers.

(i hate -Wunused-parameter. can we just stop setting that on android?)

[Luis Héctor Chávez, 2016-05-24 15:27:51]

lhchavez@chromium.org changed reviewers:
- rsleevi@chromium.org

[Luis Héctor Chávez, 2016-05-24 15:27:53]

Thanks for taking the time to review this! PTAAL.

https://codereview.chromium.org/1997153002/diff/60001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/command_line.cc#ne...
base/command_line.cc:152: CommandLine::CommandLine(NoProgram /* no_program */)
On 2016/05/23 02:59:54, danakj wrote:
> Just remove the name
> 
>
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
> 
> "Parameter names may be omitted only if the parameter is unused and its
purpose
> is obvious."
> 
> The type name here makes it obvious

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/debug/alias.cc
File base/debug/alias.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/debug/alias.cc#new...
base/debug/alias.cc:15: void Alias(const void* /* var */) {}
On 2016/05/23 02:59:54, danakj wrote:
> just remove the name, its not saying anything as |var|

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
File base/debug/stack_trace_posix.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:69: #if !defined(__UCLIBC__) &&
!defined(USE_SYMBOLIZE)
On 2016/05/23 02:59:54, danakj wrote:
> Can you explain?

DemangleSymbols is only used when neither __UCLIBC__ nor USE_SYMBOLIZE are
defined.

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:116: #else
On 2016/05/23 02:59:54, danakj wrote:
> comment on what this is #else-ing

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:117: 
On 2016/05/23 02:59:54, danakj wrote:
> no need for the whitespace

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:120: 
On 2016/05/23 02:59:54, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_li...
File base/files/file_util_linux.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_li...
base/files/file_util_linux.cc:27: switch
(static_cast<uintmax_t>(statfs_buf.f_type)) {
On 2016/05/23 02:59:54, danakj wrote:
> Why is this needed?

Added a comment.

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_po...
File base/files/file_util_posix.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/files/file_util_po...
base/files/file_util_posix.cc:849: (void)executable;  // avoid unused warning
when !defined(OS_LINUX)
On 2016/05/23 02:59:54, danakj wrote:
> capitalize and punctuate comment senetences

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/lazy_instance.h
File base/lazy_instance.h (right):

https://codereview.chromium.org/1997153002/diff/60001/base/lazy_instance.h#ne...
base/lazy_instance.h:100: static void Delete(Type* /* instance */) {}
On 2016/05/23 02:59:54, danakj wrote:
> delete the name, it adds no value

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/logging.cc#newcode459
base/logging.cc:459: void MakeCheckOpValueString(std::ostream* os, const
std::nullptr_t& /* p */) {
On 2016/05/23 02:59:54, danakj wrote:
> delete the name, it adds nothing

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1997153002/diff/60001/base/memory/ref_counted...
base/memory/ref_counted.h:428: bool operator==(const scoped_refptr<T>& lhs,
std::nullptr_t /* null */) {
On 2016/05/23 02:59:54, danakj wrote:
> delete the name

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/memory/ref_counted...
base/memory/ref_counted.h:433: bool operator==(std::nullptr_t /* null */, const
scoped_refptr<T>& rhs) {
On 2016/05/23 02:59:54, danakj wrote:
> delete

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
File base/memory/shared_memory_android.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
base/memory/shared_memory_android.cc:55: bool SharedMemory::Delete(const
std::string& /* name */) {
On 2016/05/23 02:59:54, danakj wrote:
> delete

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
base/memory/shared_memory_android.cc:61: bool SharedMemory::Open(const
std::string& /* name */, bool /* read_only */) {
On 2016/05/23 02:59:54, danakj wrote:
> the |name| isn't doing much, but bool isnt clear without a name..
> 
> which brings me to why are we doing this? why can't you build chrome with the
> same warnings we build chrome with?
> 
> I am not excited about trying to maintain unused variable warnings for code
that
> we don't care t enable that warning on. it's going to bitrot always and we'll
> have to deal with followup fixes all the time.

The problem is that some of these changes are in headers that are included in
other projects, so -Wno-unused-parameter would need to be added in a lot of
places.

I'll raise this with the maintainers of libchrome in both AOSP and Chrome OS :(

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/memory/shared_memo...
base/memory/shared_memory_posix.cc:465: bool
SharedMemory::ShareToProcessCommon(ProcessHandle /* process */,
On 2016/05/23 02:59:54, danakj wrote:
> delete name

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_libevent.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:91: MessagePumpLibevent* /* pump */)
{
On 2016/05/23 02:59:55, danakj wrote:
> delete

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:101: MessagePumpLibevent* /* pump */)
{
On 2016/05/23 02:59:54, danakj wrote:
> delete

Done.

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:200: static void timer_callback(int
/* fd */, short /* events */, void* context) {
On 2016/05/23 02:59:54, danakj wrote:
> these need names to understand them though.. making them comments makes code
> that doesn't follow the google style guide AFAICT. Can you contradict that?

The google style guide does allow parameter names as comments when they are
unused and are not obvious:
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

It does not have whitespace around the name, though, so I'll change that.

https://codereview.chromium.org/1997153002/diff/60001/crypto/secure_hash.cc
File crypto/secure_hash.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/crypto/secure_hash.cc#n...
crypto/secure_hash.cc:25: SecureHashSHA256(const SecureHashSHA256& other) :
SecureHash() {
On 2016/05/21 10:33:33, Ryan Sleevi wrote:
> Why. The existing code is doing the right thing.

Nevermind, I realized the branch I was building this on used an older version of
gcc. Doesn't reproduce with other versions of gcc or clang, so removed this.

[Primiano Tucci (use gerrit), 2016-05-24 16:43:51]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-05-24 16:43:52]

//base/trace_event LGTM, but I agree with the comment above that this might have
been split into "trivial syntax changes" vs more structural changes.

[Alexei Svitkine (slow), 2016-05-24 17:39:11]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-05-24 17:39:12]

not lgtm for base/metrics

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:1143: double CustomHistogram::GetBucketSize(Count
/*current*/, uint32_t /*i*/) const {
I don't think either commenting out the param or omitting is consistent with the
style of the Chromium codebase. (Not counting Blink which has a style that
evolved from the WebKit style.)

I would like to see this discussion on chromium-dev@ before approving these
changes.

[Luis Héctor Chávez, 2016-05-24 17:45:07]

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:1143: double CustomHistogram::GetBucketSize(Count
/*current*/, uint32_t /*i*/) const {
On 2016/05/24 17:39:12, Alexei Svitkine (OOO til Tues) wrote:
> I don't think either commenting out the param or omitting is consistent with
the
> style of the Chromium codebase. (Not counting Blink which has a style that
> evolved from the WebKit style.)
> 
> I would like to see this discussion on chromium-dev@ before approving these
> changes.

Chromium follows the Google style, which does allow for omitting param names
when unused if the name is obvious, and comment it if the name is not:
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

[Alexei Svitkine (slow), 2016-05-24 17:48:54]

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:1143: double CustomHistogram::GetBucketSize(Count
/*current*/, uint32_t /*i*/) const {
On 2016/05/24 17:45:07, Luis Héctor Chávez wrote:
> On 2016/05/24 17:39:12, Alexei Svitkine (OOO til Tues) wrote:
> > I don't think either commenting out the param or omitting is consistent with
> the
> > style of the Chromium codebase. (Not counting Blink which has a style that
> > evolved from the WebKit style.)
> > 
> > I would like to see this discussion on chromium-dev@ before approving these
> > changes.
> 
> Chromium follows the Google style, which does allow for omitting param names
> when unused if the name is obvious, and comment it if the name is not:
>
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

Sure, but Chromium also chooses certain patterns for consistency with the rest
of the codebase. For example Google style guide allows both Foo* bar; and Foo
*bar; but Chromium prefers the former.

This is another such case currently in my experience of Chromium code. If we
want to change the preference or allow the inconsistency between different
files, it should be discussed on chromium-dev@.

[danakj, 2016-05-24 18:39:52]

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1997153002/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:1143: double CustomHistogram::GetBucketSize(Count
/*current*/, uint32_t /*i*/) const {
On 2016/05/24 17:45:07, Luis Héctor Chávez wrote:
> On 2016/05/24 17:39:12, Alexei Svitkine (OOO til Tues) wrote:
> > I don't think either commenting out the param or omitting is consistent with
> the
> > style of the Chromium codebase. (Not counting Blink which has a style that
> > evolved from the WebKit style.)
> > 
> > I would like to see this discussion on chromium-dev@ before approving these
> > changes.
> 
> Chromium follows the Google style, which does allow for omitting param names
> when unused if the name is obvious, and comment it if the name is not:
>
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

Omitting is not the same as commenting out, and leaving a comment seems to imply
the name is not obvious.

[danakj, 2016-05-24 18:42:43]

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_libevent.cc (right):

https://codereview.chromium.org/1997153002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:200: static void timer_callback(int
/* fd */, short /* events */, void* context) {
On 2016/05/24 15:27:53, Luis Héctor Chávez wrote:
> On 2016/05/23 02:59:54, danakj wrote:
> > these need names to understand them though.. making them comments makes code
> > that doesn't follow the google style guide AFAICT. Can you contradict that?
> 
> The google style guide does allow parameter names as comments when they are
> unused and are not obvious:
>
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
> 
> It does not have whitespace around the name, though, so I'll change that.

Oh I see. It's not in the initial list of rules, but at the end of that section:
"Unused parameters that might not be obvious should comment out the variable
name in the function definition"

Okay.

[Primiano Tucci (use gerrit), 2016-05-27 17:27:00]

Description was changed from

==========
libchrome: Several upstreamable fixes from libchrome

libchrome is a library in AOSP that contains parts of base/,
components/, dbus/ and sandbox/ from Chrome. It uses stricter build
warnings than Chrome (-Wunused-parameter, -Wsign-compare, -Wsign-promo),
so uprevving is kind of painful. This change has all the cleanly
upstreamable patches so that the next libchrome uprev is easier.

BUG=None
TEST=trybots, build in AOSP
==========

to

==========
libchrome: Several upstreamable fixes from libchrome

libchrome is a library in AOSP that contains parts of base/,
components/, dbus/ and sandbox/ from Chrome. It uses stricter build
warnings than Chrome (-Wunused-parameter, -Wsign-compare, -Wsign-promo),
so uprevving is kind of painful. This change has all the cleanly
upstreamable patches so that the next libchrome uprev is easier.

BUG=None
TEST=trybots, build in AOSP
==========

[Primiano Tucci (use gerrit), 2016-05-27 17:27:00]

primiano@chromium.org changed reviewers:
- primiano@chromium.org

[Ken Rockot(use gerrit already), 2016-06-02 16:47:06]

rockot@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-06-03 08:15:08]

primiano@chromium.org changed reviewers:
- primiano@chromium.org