Remove the "set noparent" from the chrome\browser\ui subdirectories. This is causing a lot of pain …

Remove the "set noparent" from the chrome\browser\ui subdirectories. This is causing a lot of pain for people doing refactorings. Developers have two choices:
-wait for days until all OWNERS respond, which is soul-sucking
-ignore the OWNERS check and just force-commit, which is something we don't want to make acceptable since it moves the onus from a top level owner to decide whether they can approve a wide refactoring to individual contributors. Ideally there'd be no way to force.

For background, Ben had added most of these 7 weeks ago when he added "set noparent" to chrome\browser\ui because he didn't want to be inundated with review requests for subdirectories. The exceptions are:

cocoa: which got "set noparent" a few weeks before Ben's actions
sync: which Avi added when he moved the code from chrome\browser\ui to chrome\browser\ui\sync (so there's no change from before his move compared to with this patch)

So for cocoa folks: I think we can all agree that the owners in chrome\browser\ui will only approve changes in cocoa subdirectory that are trivial (i.e. renaming functions etc). For anything substantive, they will redirect.

I have also added comments in chrome\browser\ui\OWNERS about this.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124195

[jam, 2012-02-27 19:32:37]

Reviewers

Ben:
M      chrome\browser\ui\panels\OWNERS
M      chrome\browser\ui\tab_contents\OWNERS
D      chrome\browser\ui\omnibox\OWNERS
M      chrome\browser\ui\webui\OWNERS
D      chrome\browser\ui\tabs\OWNERS
M      chrome\browser\ui\intents\OWNERS
M      chrome\browser\ui\sync\OWNERS
D      chrome\browser\ui\search_engines\OWNERS
M      chrome\browser\ui\OWNERS

Mark
M      chrome\browser\ui\cocoa\OWNERS

[Mark Mentovai, 2012-02-27 19:57:06]

http://codereview.chromium.org/9481010/diff/2001/chrome/browser/ui/cocoa/OWNERS
File chrome/browser/ui/cocoa/OWNERS (left):

http://codereview.chromium.org/9481010/diff/2001/chrome/browser/ui/cocoa/OWNE...
chrome/browser/ui/cocoa/OWNERS:1: set noparent
As I mentioned earlier, the reason we have this set to “noparent” is to make up
for the inability to enforce Objective-C readability project-wide. Before we
added this, we saw a good number of changes being made in this directory that
ought never have been made because the author had a poor grasp of the language.
I think it should be rare enough to see a refactoring in this directory that
doesn’t touch anything Objective, but “make refactoring easier” is the only
reason proffered for this change. On the other hand, I don’t see the need for
Ben or Scott to have to refer someone to a cocoa OWNER manually and on a
case-by-case basis for the more common occurrences involving substantive
changes.

[jam, 2012-02-27 20:02:56]

http://codereview.chromium.org/9481010/diff/2001/chrome/browser/ui/cocoa/OWNERS
File chrome/browser/ui/cocoa/OWNERS (left):

http://codereview.chromium.org/9481010/diff/2001/chrome/browser/ui/cocoa/OWNE...
chrome/browser/ui/cocoa/OWNERS:1: set noparent
On 2012/02/27 19:57:06, Mark Mentovai wrote:
> As I mentioned earlier, the reason we have this set to “noparent” is to make
up
> for the inability to enforce Objective-C readability project-wide. Before we
> added this, we saw a good number of changes being made in this directory that
> ought never have been made because the author had a poor grasp of the
language.

I agree changes were misreviewed before you added this. I think that was because
there was NO owners checks at that time, since chrome\browser\ui didn't have
"set no parent", so effectively this OWNERS file was ignored. sesek added this
"set noparent" in Dec 20. Ben added the "set noparent" to the above directory
after that, on Jan 20.

> I think it should be rare enough to see a refactoring in this directory that
> doesn’t touch anything Objective, but “make refactoring easier” is the only
> reason proffered for this change.

I've hit this myself many times, and so have others I've talked to. Even if it
is rare, we want to be in a world where this no acceptable reason to ignore
OWNERS (i.e. what google3 enforces).

> On the other hand, I don’t see the need for
> Ben or Scott to have to refer someone to a cocoa OWNER manually and on a
> case-by-case basis for the more common occurrences involving substantive
> changes.

People touching code in ui/cocoa shouldn't be sending the change to ben/scott
initially. Just like in google3 no one will send a toplevel owner a change for
review that only touches a subdir with its own OWNERS file.

The reason to remove "set noparent" is simply for project wide refactorings, and
that's the only time that ben/scott will ok changes in subdirs.

[jam, 2012-02-27 20:39:02]

On 2012/02/27 20:02:56, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/9481010/diff/2001/chrome/browser/ui/cocoa/OWNERS
> File chrome/browser/ui/cocoa/OWNERS (left):
> 
>
http://codereview.chromium.org/9481010/diff/2001/chrome/browser/ui/cocoa/OWNE...
> chrome/browser/ui/cocoa/OWNERS:1: set noparent
> On 2012/02/27 19:57:06, Mark Mentovai wrote:
> > As I mentioned earlier, the reason we have this set to “noparent” is to make
> up
> > for the inability to enforce Objective-C readability project-wide. Before we
> > added this, we saw a good number of changes being made in this directory
that
> > ought never have been made because the author had a poor grasp of the
> language.
> 
> I agree changes were misreviewed before you added this. I think that was
because
> there was NO owners checks at that time, since chrome\browser\ui didn't have
> "set no parent", so effectively this OWNERS file was ignored. sesek added this
> "set noparent" in Dec 20. Ben added the "set noparent" to the above directory
> after that, on Jan 20.
> 
> > I think it should be rare enough to see a refactoring in this directory that
> > doesn’t touch anything Objective, but “make refactoring easier” is the only
> > reason proffered for this change.
> 
> I've hit this myself many times, and so have others I've talked to. Even if it
> is rare, we want to be in a world where this no acceptable reason to ignore
> OWNERS (i.e. what google3 enforces).
> 
> > On the other hand, I don’t see the need for
> > Ben or Scott to have to refer someone to a cocoa OWNER manually and on a
> > case-by-case basis for the more common occurrences involving substantive
> > changes.
> 
> People touching code in ui/cocoa shouldn't be sending the change to ben/scott
> initially. Just like in google3 no one will send a toplevel owner a change for
> review that only touches a subdir with its own OWNERS file.
> 
> The reason to remove "set noparent" is simply for project wide refactorings,
and
> that's the only time that ben/scott will ok changes in subdirs.

since data is good, I searched code reviews for refactoring changes last week
that touched chrome/browser/ui/cocoa that the "set noparent" got in the way of:
http://codereview.chromium.org/9420007/
https://chromiumcodereview.appspot.com/9416070/ (by fluke this happened to be
reviewed by Avi who works on content refactoring, but I only sent it to him
because of the timezone and otherwise I would have had to TBR)
https://chromiumcodereview.appspot.com/9423005/
https://chromiumcodereview.appspot.com/9456019/
http://codereview.chromium.org/9430025/
http://codereview.chromium.org/9465030/
http://codereview.chromium.org/9460039/

[Peter Kasting, 2012-02-27 22:30:05]

I thought the solution to "project-wide refactorings are painful" was to find a
way to allow the topmost-level OWNERS to approve such changes all at once?

When I conceded that plan in the conversation with you I explicitly said that
was conditional on not removing "set noparent" from directories which currently
have it, and you agreed.

I don't understand why there's now a change proposal here to remove set noparent
in order to make project-wide refactorings less painful.  And I continue to not
want the omnibox subdir to lose its "noparent" status.

[Mark Mentovai, 2012-02-27 23:34:17]

John Abd-El-Malek wrote:
> Let me be overly verbose: I understand you added it as a substitute
> for readability review. What I'm saying is that when you added, there
> were no OWNERS file so that was the only way to enforce it. Now,
> there's a parent OWNERS file, and ben/scott/peter won't review mac
> code, and would only lgtm refactoring changes like the ones I pointed
> out.

Cutting to the chase, we would have used “set noparent” even if there was an
enclosing OWNERS file present and even if the enclosing OWNERS file also
contained “set noparent.” chrome/browser/ui/cocoa/OWNERS as currently checked in
is exactly as we intended it, and exactly as I would like it to remain. This
should bring the discussion to a close, at least as it relates to that one file.

[jam, 2012-02-29 01:51:38]

Ben: ptal. I've restored the deleted files but without the "set noparent" so
that people know who to send these reviews to (Rietveld is showing the wrong
status though)

[Ben Goodger (Google), 2012-02-29 16:26:53]

lgtm