Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

This CL moves the following functions of ash_util.h from the 'chrome'
namespace to the 'ash_util' namespace:

- ShouldOpenAshOnStartup
- IsRunningInMash
- IsAcceleratorDeprecated

BUG=697053
Review-Url: https://codereview.chromium.org/2718333002
Cr-Commit-Position: refs/heads/master@{#453864}
Committed: https://chromium.googlesource.com/chromium/src/+/052ae58bf06d71d46567ea3c1e8952c24ec69b29

[fwang, 2017-02-28 09:20:31]

The CQ bit was checked by fwang@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 09:20:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fwang, 2017-02-28 09:21:50]

Description was changed from

==========
Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

This CL moves the following functions of ash_util.h from the 'chrome'
namespace to the 'ash_util' namespace:

- ShouldOpenAshOnStartup
- IsRunningInMash
- IsAcceleratorDeprecated

BUG=None
==========

to

==========
Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

This CL moves the following functions of ash_util.h from the 'chrome'
namespace to the 'ash_util' namespace:

- ShouldOpenAshOnStartup
- IsRunningInMash
- IsAcceleratorDeprecated

BUG=None
==========

[commit-bot: I haz the power, 2017-02-28 09:23:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 09:23:49]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fwang, 2017-02-28 10:14:55]

The CQ bit was checked by fwang@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 10:15:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 11:03:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 11:03:23]

Dry run: This issue passed the CQ dry run.

[fwang, 2017-02-28 11:05:10]

fwang@igalia.com changed reviewers:
+ achuith@chromium.org, sky@chromium.org

[fwang, 2017-02-28 11:05:11]

PTAL

[James Cook, 2017-02-28 15:08:30]

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

[James Cook, 2017-02-28 15:08:31]

drive-by thank you for cleaning this up!

[achuithb, 2017-02-28 15:35:42]

lgtm

https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_util.cc (right):

https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_util.cc:65: bool IsAcceleratorDeprecated(const
ui::Accelerator& accelerator) {
Looks like this is only used in one place? Wonder if it makes sense to move this
logic to browser_view.cc

[fwang, 2017-02-28 15:54:24]

On 2017/02/28 15:35:42, achuithb wrote:
> lgtm
> 
>
https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
> File chrome/browser/ui/ash/ash_util.cc (right):
> 
>
https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
> chrome/browser/ui/ash/ash_util.cc:65: bool IsAcceleratorDeprecated(const
> ui::Accelerator& accelerator) {
> Looks like this is only used in one place? Wonder if it makes sense to move
this
> logic to browser_view.cc

I wondered that too, but I guess if it was put in ash_util, the plan was to
re-use it in other places. Maybe someone cc' here can comment.

[fwang, 2017-02-28 15:54:42]

Description was changed from

==========
Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

This CL moves the following functions of ash_util.h from the 'chrome'
namespace to the 'ash_util' namespace:

- ShouldOpenAshOnStartup
- IsRunningInMash
- IsAcceleratorDeprecated

BUG=None
==========

to

==========
Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

This CL moves the following functions of ash_util.h from the 'chrome'
namespace to the 'ash_util' namespace:

- ShouldOpenAshOnStartup
- IsRunningInMash
- IsAcceleratorDeprecated

BUG=697053
==========

[sky, 2017-02-28 17:40:27]

https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_util.h (right):

https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_util.h:21: namespace ash_util {
Generally we don't use namespaces like this in chrome code. See long threads on
chromium-dev for details. tl;dr here is no namespace.

[James Cook, 2017-02-28 17:47:25]

https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_util.h (right):

https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_util.h:21: namespace ash_util {
On 2017/02/28 17:40:26, sky wrote:
> Generally we don't use namespaces like this in chrome code. See long threads
on
> chromium-dev for details. tl;dr here is no namespace.

I think we use namespaces like this where there is a group of related utility
functions, in order to avoid a class with only static members:

https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

"Sometimes it is useful 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. For a header
myproject/foo_bar.h, for example, write

namespace myproject {
namespace foo_bar {
void Function1();
void Function2();
}  // namespace foo_bar
}  // namespace myproject"

I thought the "don't use namespaces in chrome" guidance was to not use
"chrome::" inside src/chrome.

[fwang, 2017-02-28 19:11:12]

Thank you everybody for the feedback. So

- Should I remove IsAcceleratorDeprecated from the ash_util module?

- Should I use anonymous or ash_util namespace?

[sky, 2017-02-28 20:49:40]

On Tue, Feb 28, 2017 at 9:47 AM,  <jamescook@chromium.org> wrote:
>
>
https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
> File chrome/browser/ui/ash/ash_util.h (right):
>
>
https://codereview.chromium.org/2718333002/diff/20001/chrome/browser/ui/ash/a...
> chrome/browser/ui/ash/ash_util.h:21: namespace ash_util {
> On 2017/02/28 17:40:26, sky wrote:
>> Generally we don't use namespaces like this in chrome code. See long
> threads on
>> chromium-dev for details. tl;dr here is no namespace.
>
> I think we use namespaces like this where there is a group of related
> utility functions, in order to avoid a class with only static members:

Sorry if I wasn't being clear. I wasn't suggesting a class either,
just no namespace.

>
>
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...
>
> "Sometimes it is useful 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. For a header myproject/foo_bar.h, for
> example, write
>
> namespace myproject {
> namespace foo_bar {
> void Function1();
> void Function2();
> } // namespace foo_bar
> } // namespace myproject"
>
> I thought the "don't use namespaces in chrome" guidance was to not use
> "chrome::" inside src/chrome.

Generally for code in src/chrome that translates to no namespace. See
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...
for the thread. I'm specifically referring to "Use namespaces based on
the path, and not invent special namespaces for a single file." Which
goes against the style you just mentioned. But styles change.

I'm ok with the existing namespace, but I don't think it's necessary.
But whether we have the namespace is separate from the change. So,
feel free not to change the namespace at all.

  -Scott
>
> https://codereview.chromium.org/2718333002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-02-28 20:50:33]

LGTM

[fwang, 2017-03-01 05:35:11]

The CQ bit was checked by fwang@igalia.com

[fwang, 2017-03-01 05:35:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2718333002/#ps40001
(title: "Rebase after https://codereview.chromium.org/2721883002")

[commit-bot: I haz the power, 2017-03-01 05:35:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fwang, 2017-03-01 05:50:29]

@achuithb: Regarding IsAcceleratorDeprecated, the rationale for adding it to
ash_util is in https://codereview.chromium.org/1177773002#msg25 ; it seems it
will be removed at some point, so I guess we can ignore that.

@sky: @James Cook: Once this lands, I'll open a follow-up CL to remove the
ash_util namespace for these three functions and the others from ash_util.

[commit-bot: I haz the power, 2017-03-01 06:29:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488346511250080,
"parent_rev": "e26bb9456f6affe7c75cf3e19f0b36de4957cbf3", "commit_rev":
"052ae58bf06d71d46567ea3c1e8952c24ec69b29"}

[commit-bot: I haz the power, 2017-03-01 06:30:31]

Description was changed from

==========
Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

This CL moves the following functions of ash_util.h from the 'chrome'
namespace to the 'ash_util' namespace:

- ShouldOpenAshOnStartup
- IsRunningInMash
- IsAcceleratorDeprecated

BUG=697053
==========

to

==========
Move ash_util functions from 'chrome' namespace to 'ash_util' namespace

This CL moves the following functions of ash_util.h from the 'chrome'
namespace to the 'ash_util' namespace:

- ShouldOpenAshOnStartup
- IsRunningInMash
- IsAcceleratorDeprecated

BUG=697053

Review-Url: https://codereview.chromium.org/2718333002
Cr-Commit-Position: refs/heads/master@{#453864}
Committed:
https://chromium.googlesource.com/chromium/src/+/052ae58bf06d71d46567ea3c1e89...
==========

[commit-bot: I haz the power, 2017-03-01 06:30:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/052ae58bf06d71d46567ea3c1e89...

[achuithb, 2017-03-01 09:20:22]

On 2017/03/01 05:50:29, fwang wrote:
> @achuithb: Regarding IsAcceleratorDeprecated, the rationale for adding it to
> ash_util is in https://codereview.chromium.org/1177773002#msg25 ; it seems it
> will be removed at some point, so I guess we can ignore that.

Ok