Cleanup: merging three different constants.h into a single header.

Cleanup: merging three different constants.h into a single header.

[alexeypa (please no reviews), 2012-06-18 20:15:45]

Please take a look.

[simonmorris, 2012-06-18 20:30:12]

This widens the scope of each constant. Are you sure the cost of doing so
outweighs the benefit of having fewer files?

If so,

LGTM

http://codereview.chromium.org/10579002/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10579002/diff/1/remoting/remoting.gyp#newcode512
remoting/remoting.gyp:512: 'remoting_base',
Is this necessary?

[alexeypa (please no reviews), 2012-06-18 20:33:29]

On 2012/06/18 20:30:12, simonmorris wrote:
> This widens the scope of each constant. Are you sure the cost of doing so
> outweighs the benefit of having fewer files?

I think it does. It is pretty confusing to have identically named files in three
locations.

[alexeypa (please no reviews), 2012-06-18 20:34:02]

http://codereview.chromium.org/10579002/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10579002/diff/1/remoting/remoting.gyp#newcode512
remoting/remoting.gyp:512: 'remoting_base',
On 2012/06/18 20:30:12, simonmorris wrote:
> Is this necessary?

Yes. It needs values from constants.cc.

[commit-bot: I haz the power, 2012-06-18 20:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10579002/1

[commit-bot: I haz the power, 2012-06-18 20:59:29]

Try job failure for 10579002-1 (retry) on linux_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-18 21:09:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10579002/1

[Wez, 2012-06-18 21:15:52]

On 2012/06/18 20:15:45, alexeypa wrote:
> Please take a look.

This seems the wrong direction to go in; better to place the constant
definitions in locations appropriate to their function - we have lots of
constants in the code and the current constants.h layout makes it unclear which
belong in the code and which in this/these header(s).

[Do not use (sergeyu), 2012-06-18 21:28:06]

On Mon, Jun 18, 2012 at 2:15 PM, <wez@chromium.org> wrote:

> On 2012/06/18 20:15:45, alexeypa wrote:
>
>> Please take a look.
>>
>
> This seems the wrong direction to go in; better to place the constant
> definitions in locations appropriate to their function - we have lots of
> constants in the code and the current constants.h layout makes it unclear
> which
> belong in the code and which in this/these header(s).
>

+1. I think ideally we shouldn't have any constants.h at all (to be fair,
it was I who introduced base/constants.h in the first place :( ). All
constants could go to classes/headers that matches the area where they are
used. Big constants.h with a lot of unrelated junk are hard to maintain, so
it's better to avoid creating one when possible.



>
>
https://chromiumcodereview.**appspot.com/10579002/<https://chromiumcodereview...
>


-- 
Sergey Ulanov

[alexeypa (please no reviews), 2012-06-18 21:36:34]

On 2012/06/18 21:15:52, Wez wrote:
> This seems the wrong direction to go in; better to place the constant
> definitions in locations appropriate to their function - we have lots of
> constants in the code and the current constants.h layout makes it unclear
which
> belong in the code and which in this/these header(s).

/unchecking the CQ bit.../

The constants that go to base/constants.h tend to be used across modules. The
only exception is kChromotingBotJid. It is used only in host code and I didn't
touch it in this CL.

Host exit codes are used by the host and Windows service code.

HOST_PLUGIN_XXX are "referenced" by remoting.gyp so I consider that a
cross-module use. Practically it is because one will need to modify both
simultaneously.

In short my point is that it would be nice to have a single place where a
constant is defined. However it is not helpful that all those files are named
constants.h. :-) Feel free to suggest were you think it will be better to put
them.

[Lambros, 2012-06-18 22:33:02]

> In short my point is that it would be nice to have a single place where a
> constant is defined. However it is not helpful that all those files are named
> constants.h. :-) Feel free to suggest were you think it will be better to put
> them.
I've done a search in Chrome for all files containing the word 'constants'. 
There are a lot of files named 'constants.h' and 'constants.cc'.  There are also
many instances of adding a prefix reflecting the scope of the constants (even
when the prefix name is already a folder in the path), for example:
./chrome/browser/policy/policy_constants.h
./chrome/browser/ui/panels/panel_constants.h
./chrome/common/chrome_constants.h
./chrome/common/extensions/extension_constants.h
./chrome/common/net/gaia/gaia_constants.h

So I vote for:
base/constants.h,
host/host_constants.h
client/client_constants.h

[Do not use (sergeyu), 2012-06-19 00:16:47]

This is obviously a bikeshed discussion. Let's not waste too much time on
it. Having said that below is my last opinion on that issue :)

Yes I agree that there may be some strings for which it still makes sense
to have base/constants.h. kChromotingXmlNamespace is an example of such
constants. What I think we should avoid is to making that file a dumping
ground for all sorts of unrelated constants (take a look at libjingle's
constants.cc file as an example of unmanageable mess we can get here). For
example all channel names, currently defined in base/constants.h, but don't
really belong there because they used only by the protocol
code. kMimeTypeTextUtf8 is in the same category - it used only by some
clipboard code.

On Mon, Jun 18, 2012 at 3:33 PM, <lambroslambrou@chromium.org> wrote:

>  In short my point is that it would be nice to have a single place where a
>> constant is defined. However it is not helpful that all those files are
>> named
>> constants.h. :-) Feel free to suggest were you think it will be better to
>> put
>> them.
>>
> I've done a search in Chrome for all files containing the word 'constants'.
> There are a lot of files named 'constants.h' and 'constants.cc'.  There
> are also
> many instances of adding a prefix reflecting the scope of the constants
> (even
> when the prefix name is already a folder in the path), for example:
> ./chrome/browser/policy/**policy_constants.h
> ./chrome/browser/ui/panels/**panel_constants.h
> ./chrome/common/chrome_**constants.h
> ./chrome/common/extensions/**extension_constants.h
> ./chrome/common/net/gaia/gaia_**constants.h
>
> So I vote for:
> base/constants.h,
> host/host_constants.h
> client/client_constants.h
>
>
>
http://codereview.chromium.**org/10579002/<http://codereview.chromium.org/105...
>



-- 
Sergey Ulanov