[Android] Refactor: move files to chrome/browser/notifications/channels

[Android] Refactor: move files to chrome/browser/notifications/channels

- The number of files in chrome/browser/notifications was getting rather
large

- This simply moves all the channel-related code into a new sub-package

BUG=700377
Review-Url: https://codereview.chromium.org/2863623002
Cr-Commit-Position: refs/heads/master@{#469631}
Committed: https://chromium.googlesource.com/chromium/src/+/ad369409491735cc0120a7cc090f476a36c55bd6

[awdf, 2017-05-04 15:15:56]

awdf@chromium.org changed reviewers:
+ johnme@chromium.org

[johnme, 2017-05-04 15:22:40]

lgtm with nits

https://codereview.chromium.org/2863623002/diff/1/chrome/android/java_sources...
File chrome/android/java_sources.gni (right):

https://codereview.chromium.org/2863623002/diff/1/chrome/android/java_sources...
chrome/android/java_sources.gni:577:
"java/src/org/chromium/chrome/browser/notifications/channels/ChannelDefinitions.java",
Nit: the rest of this file seems to sort classes above packages, so please
reorder these.

https://codereview.chromium.org/2863623002/diff/1/chrome/android/java_sources...
chrome/android/java_sources.gni:1697:
"junit/src/org/chromium/chrome/browser/notifications/channels/ChannelDefinitionsTest.java",
Ditto.

[awdf, 2017-05-04 15:46:11]

https://codereview.chromium.org/2863623002/diff/1/chrome/android/java_sources...
File chrome/android/java_sources.gni (right):

https://codereview.chromium.org/2863623002/diff/1/chrome/android/java_sources...
chrome/android/java_sources.gni:577:
"java/src/org/chromium/chrome/browser/notifications/channels/ChannelDefinitions.java",
On 2017/05/04 15:22:39, johnme wrote:
> Nit: the rest of this file seems to sort classes above packages, so please
> reorder these.

Done.

https://codereview.chromium.org/2863623002/diff/1/chrome/android/java_sources...
chrome/android/java_sources.gni:1697:
"junit/src/org/chromium/chrome/browser/notifications/channels/ChannelDefinitionsTest.java",
On 2017/05/04 15:22:39, johnme wrote:
> Ditto.

Done.

[awdf, 2017-05-04 15:46:21]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-05-04 15:46:21]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/2863623002/#ps20001
(title: "reorder files in java_sources.gni")

[commit-bot: I haz the power, 2017-05-04 15:46:46]

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-05-04 15:57:47]

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

[commit-bot: I haz the power, 2017-05-04 15:57:49]

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...)

[whywhat, 2017-05-04 16:00:29]

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

[whywhat, 2017-05-04 16:00:29]

Aren't there just 22 files, including OWNERS? IMO, it is not many for a
directory/package. Is that the only justification for the move? Are you planning
to add many more files under one of those in near future?

This CL will likely cause "git blame" and 59 branch merge headaches. Not to say
the cognitive overhead of deeper folder structures (tree view in the editor has
to be wider, one has to type and/or read more characters in the imports or when
referring to the file on the command line, etc).

[awdf, 2017-05-04 16:21:58]

On 2017/05/04 16:00:29, whywhat wrote:
> Aren't there just 22 files, including OWNERS? IMO, it is not many for a
> directory/package. Is that the only justification for the move? Are you
planning
> to add many more files under one of those in near future?
> 
> This CL will likely cause "git blame" and 59 branch merge headaches. Not to
say
> the cognitive overhead of deeper folder structures (tree view in the editor
has
> to be wider, one has to type and/or read more characters in the imports or
when
> referring to the file on the command line, etc).

Personally I find the cognitive overload of searching through a long list of
files greater than when I can jump-to/discount an obvious subdirectory from the
list.

I'd hope git blame could cope with a simple rename such as this one. We're not
planning on merging any more channels code to 59. I guess it's possible someone
will want to merge code that depends on ChannelDefinitions and this will break
their import (hopefully unlikely at this point), but at least that will be easy
enough to resolve.

Is there any official guidance on Chromium's policy on levels of subdirectories?
If not I think it's reasonable to leave it up to the owners of the parent
directory. I note there are some subdirectories of chrome/browser/media for
example, so doesn't seem like adding a subdirectory to
chrome/browser/notifications is going too far.

[Bernhard Bauer, 2017-05-04 16:22:23]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org, miguelg@chromium.org, peter@chromium.org

[Bernhard Bauer, 2017-05-04 16:22:23]

I'm going defer to the (other) notifications/ OWNERS on whether the churn is
worth it or not. If they are in favor, I'm happy to rubberstamp.

[awdf, 2017-05-04 16:27:45]

On 2017/05/04 16:22:23, Bernhard Bauer wrote:
> I'm going defer to the (other) notifications/ OWNERS on whether the churn is
> worth it or not. If they are in favor, I'm happy to rubberstamp.

Checking with nyquist@ & dtrainor@ who own ChannelDefinitions.java which is
affected by the change..

[Peter Beverloo, 2017-05-04 16:56:41]

lgtm

Anton, I think you bring up good points, but as Anita's the primary owner of the
directory I feel that this ultimately is her decision.

[whywhat, 2017-05-04 17:19:02]

On 2017/05/04 at 16:21:58, awdf wrote:
> On 2017/05/04 16:00:29, whywhat wrote:
> > Aren't there just 22 files, including OWNERS? IMO, it is not many for a
> > directory/package. Is that the only justification for the move? Are you
planning
> > to add many more files under one of those in near future?
> > 
> > This CL will likely cause "git blame" and 59 branch merge headaches. Not to
say
> > the cognitive overhead of deeper folder structures (tree view in the editor
has
> > to be wider, one has to type and/or read more characters in the imports or
when
> > referring to the file on the command line, etc).
> 
> Personally I find the cognitive overload of searching through a long list of
files greater than when I can jump-to/discount an obvious subdirectory from the
list.

Hm, most editors I know let one incrementally search for a file using the file
name or even the class name. This CL might solve the problem for this particular
directory but not for the whole code base which AFAIK still has lots of
directories with 20+ files in them.

I guess you look at these files way more often than me and if having them in a
subpackage improves your workflow, go for it :)

> 
> I'd hope git blame could cope with a simple rename such as this one. We're not
planning on merging any more channels code to 59. I guess it's possible someone
will want to merge code that depends on ChannelDefinitions and this will break
their import (hopefully unlikely at this point), but at least that will be easy
enough to resolve.

It's risky to merge to the branch in general, it's riskier if there're merge
conflicts. I think even if I change an import statement around the
notification.channels import, git drover will fail to merge. Worst case I'll
have to checkout the branch locally, apply the patch and see if the tree still
builds, passes the tests and so on. Because apart from carefully staring,
nothing else AFAIK prevents me from submitting a drover patch with a typo in the
package name.

As for git blame, I tried that on recently moved MediaControlCastButtonElement
(FWIW I didn't support having an elements/ subdir either) both in code search
and from the command line and it didn't show any previous history to me. Maybe
I'm using it wrong though.

> 
> Is there any official guidance on Chromium's policy on levels of
subdirectories? If not I think it's reasonable to leave it up to the owners of
the parent directory. I note there are some subdirectories of
chrome/browser/media for example, so doesn't seem like adding a subdirectory to
chrome/browser/notifications is going too far.

chrome/browser/media has subdirectories for different features (video flinging,
presentation api and media playback notification). I didn't find anything in the
Chromium policy on the matter.

[whywhat, 2017-05-04 17:21:54]

I just wanted to highlight the cons and learn more about the pros.
Maybe I'll do the same move in the few directories I own :)

[whywhat, 2017-05-04 17:22:06]

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

[Miguel Garcia, 2017-05-05 09:57:28]

lgtm

[awdf, 2017-05-05 10:33:04]

The CQ bit was checked by awdf@chromium.org

[commit-bot: I haz the power, 2017-05-05 10:33:13]

CQ is trying da patch.

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

[awdf, 2017-05-05 10:38:12]

The CQ bit was unchecked by awdf@chromium.org

[awdf, 2017-05-05 10:38:17]

The CQ bit was unchecked by awdf@chromium.org

[awdf, 2017-05-05 11:17:49]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-05-05 11:17:49]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
peter@chromium.org, miguelg@chromium.org
Link to the patchset: https://codereview.chromium.org/2863623002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-05 11:17:57]

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-05-05 12:24:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493983069412670,
"parent_rev": "e6326a5c9d539fc7cc7e404077b777ac6903fc85", "commit_rev":
"ad369409491735cc0120a7cc090f476a36c55bd6"}

[commit-bot: I haz the power, 2017-05-05 12:24:51]

Description was changed from

==========
[Android] Refactor: move files to chrome/browser/notifications/channels

- The number of files in chrome/browser/notifications was getting rather
large

- This simply moves all the channel-related code into a new sub-package

BUG=700377
==========

to

==========
[Android] Refactor: move files to chrome/browser/notifications/channels

- The number of files in chrome/browser/notifications was getting rather
large

- This simply moves all the channel-related code into a new sub-package

BUG=700377

Review-Url: https://codereview.chromium.org/2863623002
Cr-Commit-Position: refs/heads/master@{#469631}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad369409491735cc0120a7cc090f...
==========

[commit-bot: I haz the power, 2017-05-05 12:24:52]

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