namespace policy in //chrome

[Thiago Farina]

Hi,

Does new code in chrome needs to be in chrome namespace? There is a
mess of different namespaces used under //chrome (no, I'm not
suggesting nor volunteering to fix anything), some files base their
namespace on the file name, others on the path from the root, others
just use chrome, and so on.

I'm asking because my preference is for *NO* namespaces, although it
may be confusing (and could cause conflits) it makes the code easier
to read IMO.

I'm also asking because this will pop up in reviews; some reviewers
will ask for code to be added in chrome namespace, as already
happened.

Regards,

--
Thiago

[Nico Weber]

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/X_MSSCmNsWE mentioned that we don't want to migrate existing code to chrome::. My reading of that thread is that we don't generally want to use chrome:: for new code in chrome/ either.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev

[Darin Fisher]

Right. I think the observation is that top-level projects (those that are not libraries consumed by other projects) should not have a namespace. Libraries on the other hand should have a namespace that matches the top-level directory name. For example, src/base uses base:: as its namespace. I think we have fairly broad agreement on this approach.

Personally, I'm not a fan of other usage of namespaces derived from a file's name (e.g., file_util::) or namespaces that correspond to a deep path (e.g., chrome_browser_net::). I prefer descriptive class names.

-Darin

[Peter Kasting]

On Tue, Sep 10, 2013 at 11:20 AM, Nico Weber <tha...@chromium.org> wrote:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/X_MSSCmNsWE mentioned that we don't want to migrate existing code to chrome::. My reading of that thread is that we don't generally want to use chrome:: for new code in chrome/ either.

I would be happy to see chrome:: nuked entirely.  The few existing places it's used just seem unnecessarily verbose, and the fact that it's used only occasionally is confusing.

PK

[Darin Fisher]

+1

--

[Gabriel Charette]

On Sep 11, 2013 9:00 AM, "Gabriel Charette" <g...@google.com> wrote:

One argument I remember in favor of the chrome:: namespace is for enums. Otherwise they pollute the global namespace.

e.g., https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/host_desktop.h

If we agree that the enums shouldn't live in the global namespace then we could still have that file's methods live in the global namespace, but it would require the impl to use chrome:: to refer to its own enum which is kind of weird as well IMO.

Cheers,
Gab

[Maciej Pawlowski]

On 2013-09-10 21:57, Peter Kasting wrote:
> I would be happy to see chrome:: nuked entirely. The few existing
> places it's used just seem unnecessarily verbose, and the fact that
> it's used only occasionally is confusing.

For the record, having chrome:: helps in projects which use chromium
code here and there, but generally treat it like an external library,
not an application accessed from within. IMO any body of code that is
not entirely monolithic, but can be used and extended by third parties,
should be in a namespace. It may seem overly verbose from the inside but
makes life easier for everyone else.

--
BR
Maciej Pawlowski
Opera Desktop Wroclaw

[Shezan Baig]

+1

I'm not sure it actually makes it overly verbose.  If everything in src/chrome is inside the chrome:: namespace, then "chrome::" can be omitted within src/chrome, so it really shouldn't make a difference for Chrome developers.  However, it does make it easier for those who build on top of src/chrome.

[Jói Sigurðsson]

src/chrome isn't designed to be built on top of. If features are being
used from src/chrome, they should be moved to src/components, see
http://www.chromium.org/developers/design-documents/browser-components

Cheers,
Jói

[Shezan Baig]

On Wednesday, September 11, 2013 9:22:00 AM UTC-4, Jói Sigurðsson wrote:

src/chrome isn't designed to be built on top of. If features are being
used from src/chrome, they should be moved to src/components, see
http://www.chromium.org/developers/design-documents/browser-components

Cheers,
Jói 

This is helpful - thanks!

Right now, I am interested in using the spellcheck code in my modules (currently) above src/chrome:

    src/chrome/browser/spellcheck/
    src/chrome/renderer/spellcheck/
    (+ a few files from src/chrome/common/)

Is anybody else working on componentizing this code?  I've made some local modifications to make spellcheck use content::BrowserContext to remove dependency on chrome's Profile, but I am still using it from the existing src/chrome location.

I could start uploading CLs for this work, and eventually moving it to src/components, but I was wondering:

1) is anybody else already working on this?
2) is this something that would be accepted by Chrome developers?

Thanks,

-shez-

[John Abd-El-Malek]

For the record, I really agree with all the previous emails about why we shouldn't use chrome:// namespace, or other namespaces for subdirectories.

I think HostDesktopType is a good example of a properly named enum where each value has the name of the enum first. HOST_DESKTOP_TYPE_NATIVE doesn't pollute the namespace IMO, as opposed if the values were named NATIVE, ASH etc.

On Wed, Sep 11, 2013 at 6:00 AM, Gabriel Charette <g...@google.com> wrote:

One argument I remember in favor of the chrome:: namespace is for enums. Otherwise they pollute the global namespace.

e.g., https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/host_desktop.h

If we agree that the enums shouldn't live in the global namespace then we could still have that file's methods live in the global namespace, but it would require the impl to use chrome:: to refer to its own enum which is kind of weird as well IMO.

Cheers,
Gab

On Sep 10, 2013 5:32 PM, "Darin Fisher" <da...@chromium.org> wrote:

[Thiago Farina]

On Wed, Sep 11, 2013 at 10:41 AM, Shezan Baig <shezb...@gmail.com> wrote:
> On Wednesday, September 11, 2013 9:22:00 AM UTC-4, Jói Sigurðsson wrote:
>>
>> src/chrome isn't designed to be built on top of. If features are being
>> used from src/chrome, they should be moved to src/components, see
>> http://www.chromium.org/developers/design-documents/browser-components
>>
>> Cheers,
>> Jói
>
>
>
> This is helpful - thanks!
>
> Right now, I am interested in using the spellcheck code in my modules
> (currently) above src/chrome:
>
> src/chrome/browser/spellcheck/
> src/chrome/renderer/spellcheck/
> (+ a few files from src/chrome/common/)
>
> Is anybody else working on componentizing this code?

I'm not aware of anyone working on this. I remember that someone from
Motorola take a stab, but the effort didn't go forward.

There is a bug filed for componetizing it. It is crbug.com/152350.

> I've made some local
> modifications to make spellcheck use content::BrowserContext to remove
> dependency on chrome's Profile, but I am still using it from the existing
> src/chrome location.
>
> I could start uploading CLs for this work, and eventually moving it to
> src/components, but I was wondering:
>
> 1) is anybody else already working on this?
> 2) is this something that would be accepted by Chrome developers?
>

Not sure, I copied some people that I saw working on this area of the codebase.

--
Thiago

[Thiago Farina]

On Tue, Sep 10, 2013 at 3:36 PM, Darin Fisher <da...@chromium.org> wrote:
> Right. I think the observation is that top-level projects (those that are
> not libraries consumed by other projects) should not have a namespace.
> Libraries on the other hand should have a namespace that matches the
> top-level directory name. For example, src/base uses base:: as its
> namespace. I think we have fairly broad agreement on this approach.
>
> Personally, I'm not a fan of other usage of namespaces derived from a file's
> name (e.g., file_util::) or namespaces that correspond to a deep path (e.g.,
> chrome_browser_net::). I prefer descriptive class names.
>

Does this all apply to new code been added for tests?

If I'm adding a new foo_test_helpers.h or a foo_test_utils.h or
whatever, should the types there been added in a "namespace test" or
they can go in global namespace or that is just the decision of the
reviewer/owner of the area and the author should follow the preference
of the owner?

--
Thiago

[Rouslan Solomakhin]

Yes, I think we would accept the patch (after a proper code review, of course).

[Rachel Blum]

I sent this out a while ago, but didn't "reply all", so reposting:

There is work on this, yes - https://codereview.chromium.org/23868013/ does the BrowserContext view switch.

We're very happy to accept contributions.

[Paweł Hajdan, Jr.]

On Wed, Sep 11, 2013 at 9:12 AM, Thiago Farina <tfa...@chromium.org> wrote:

Does this all apply to new code been added for tests?

If I'm adding a new foo_test_helpers.h or a foo_test_utils.h or
whatever, should the types there been added in a "namespace test" or
they can go in global namespace or that is just the decision of the
reviewer/owner of the area and the author should follow the preference
of the owner?

I discourage usage of "namespace test".

What I've mostly seen is either global namespace or anonymous namespace in .cc file where applicable.

Feel free to add me to reviews where this becomes an issue.

Paweł 

[Peter Kasting]

On Wed, Sep 11, 2013 at 7:59 AM, John Abd-El-Malek <j...@chromium.org> wrote:

For the record, I really agree with all the previous emails about why we shouldn't use chrome:// namespace, or other namespaces for subdirectories.

Based on the commentary in this thread, I have filed http://crbug.com/289619 to remove existing chrome:: usage.

PK

[Maciej Pawlowski]

On 2013-09-10 20:36, Darin Fisher wrote:
> Right. I think the observation is that top-level projects (those that
> are not libraries consumed by other projects) should not have a namespace.
>

Code evolves, something that was initially designed to be part of the
top-level project may very well become used as a library (as evidenced
by the current effort to turn stuff into Components). Good code (with
little dependencies, extensible, testable, understandable) will tend to
be reused - and everyone wants to write good code, right?

How do you know which parts of the code can be reused and which are so
hard-core-monolithic that they will never be? If you have code that's so
non-reusable, it's often a sign of poor design, maybe it should be
rewritten, instead of being locked out.

While I agree that having chrome:: used randomly isn't consistent, I'd
rather have it added everywhere, not removed everywhere.

[Anthony Berent]

But our mechanism for reuse of Chrome code is to move the code into components, and when something is moved into a component it should surely be given a namespace name appropriate to the component (not chrome::). 

[Colin Blundell]

(From the right address this time)

On Thu, Sep 12, 2013 at 9:48 AM, Maciej Pawlowski <mpawl...@opera.com> wrote:

On 2013-09-10 20:36, Darin Fisher wrote:

Right. I think the observation is that top-level projects (those that are not libraries consumed by other projects) should not have a namespace.

Code evolves, something that was initially designed to be part of the top-level project may very well become used as a library (as evidenced by the current effort to turn stuff into Components). Good code (with little dependencies, extensible, testable, understandable) will tend to be reused - and everyone wants to write good code, right?

I suspect that most people agree with this point. For development of new features, developing them within //components is indeed preferable IMO opinion for the reasons that you mention. However, the components effort is a relatively new one, and thus most of the organization of the Chromium codebase predates it. In particular, it is a non-goal to turn //chrome into a repository of reusable code: rather, the goal is to move code that would be useful for multiple end products out of //chrome (so that over the long term, //chrome would hopefully become smaller). 

How do you know which parts of the code can be reused and which are so hard-core-monolithic that they will never be? If you have code that's so non-reusable, it's often a sign of poor design, maybe it should be rewritten, instead of being locked out.

While I agree that having chrome:: used randomly isn't consistent, I'd rather have it added everywhere, not removed everywhere.

When code is moved out of //chrome into a component that is intended for reuse, it gets namespaced at that point. Again, it is a non-goal to have //chrome itself contain code intended for reuse by multiple end products.

 

--
BR
Maciej Pawlowski
Opera Desktop Wroclaw

[John Abd-El-Malek]

Ok, looks like we have consensus on that. Can we also reach consensus on other namespaces in src\chrome? Darin had said "Personally, I'm not a fan of other usage of namespaces derived from a file's name (e.g., file_util::) or namespaces that correspond to a deep path (e.g., chrome_browser_net::). I prefer descriptive class names.". I have already stated that I agree on this.

Others?

[David Michael]

On Thu, Sep 12, 2013 at 2:41 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Ok, looks like we have consensus on that. Can we also reach consensus on other namespaces in src\chrome? Darin had said "Personally, I'm not a fan of other usage of namespaces derived from a file's name (e.g., file_util::) or namespaces that correspond to a deep path (e.g., chrome_browser_net::). I prefer descriptive class names.". I have already stated that I agree on this.

file_util is basically just a bag of utility methods. They could be placed in a class, but they'd be static, so the class would be kind of funny. It also would mean some boilerplate (e.g., private constructor to make sure we don't create the class, and "static" before all method names.

I personally find a namespace a better fit in this kind of situation. But I'll go with the flow if consensus is we should use a class for this.

 

Others?

On Wed, Sep 11, 2013 at 11:22 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Sep 11, 2013 at 7:59 AM, John Abd-El-Malek <j...@chromium.org> wrote:

For the record, I really agree with all the previous emails about why we shouldn't use chrome:// namespace, or other namespaces for subdirectories.

Based on the commentary in this thread, I have filed http://crbug.com/289619 to remove existing chrome:: usage.

PK

[Ryan Sleevi]

On Thu, Sep 12, 2013 at 1:50 PM, David Michael <dmic...@chromium.org> wrote:

On Thu, Sep 12, 2013 at 2:41 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Ok, looks like we have consensus on that. Can we also reach consensus on other namespaces in src\chrome? Darin had said "Personally, I'm not a fan of other usage of namespaces derived from a file's name (e.g., file_util::) or namespaces that correspond to a deep path (e.g., chrome_browser_net::). I prefer descriptive class names.". I have already stated that I agree on this.

file_util is basically just a bag of utility methods. They could be placed in a class, but they'd be static, so the class would be kind of funny. It also would mean some boilerplate (e.g., private constructor to make sure we don't create the class, and "static" before all method names.

I personally find a namespace a better fit in this kind of situation. But I'll go with the flow if consensus is we should use a class for this.

Right, file_util seems to be consistent with the Google C++ Style Guide on two points:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmember,_Static_Member,_and_Global_Functions#Nonmember,_Static_Member,_and_Global_Functions

"Sometimes it is useful, or even necessary, to define a function not bound to a class instance. Such a function can be either a static member or a nonmember function. Nonmember functions should not depend on external variables, and should nearly always exist in a namespace. Rather than creating classes only to group static member functions which do not share static data, use namespaces instead."

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

"With named namespaces, choose the name based on the project, and possibly its path."

I'm curious for the proposal for improving file_util, since invariably its use as an example here will influence further naming decisions.

[Peter Kasting]

On Thu, Sep 12, 2013 at 1:53 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

"Sometimes it is useful, or even necessary, to define a function not bound to a class instance. Such a function can be either a static member or a nonmember function. Nonmember functions should not depend on external variables, and should nearly always exist in a namespace. Rather than creating classes only to group static member functions which do not share static data, use namespaces instead."

This seems pretty clear, for the case of "bag of global methods".

I suspect in some cases, (namespaced) globals could reasonably become static members on a pre-existing related class, to reduce the frequency of this case.

We also have cases where there are larger namespaces encompassing many classes and methods, e.g. history:: and extensions::.  It's not clear to me whether this is a good or a bad thing.  I'd like to see one of these chosen as an example by folks who understand its contents, so that we can list the pros and cons of such a case and make a more informed decision.

PK