Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1784243005:
Add origin_trials OWNERS files to the various origin_trials directories. (Closed)
|
Created:
4 years, 9 months ago by Marijn Kruisselbrink Modified:
4 years, 8 months ago CC:
abarth-chromium, blink-reviews, chasej+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@move-trial-token-code Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd origin_trials OWNERS files to the various origin_trials directories.
Puts the actual list of owners in content/common/origin_trials/OWNERS with
the other directories refering to that file.
Committed: https://crrev.com/06010eabdf60846e08026b4e402844b336a800a3
Cr-Commit-Position: refs/heads/master@{#388858}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : smaller set of owners #Patch Set 3 : fix comment #Patch Set 4 : no blink OWNERS #
Messages
Total messages: 29 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
mek@chromium.org changed reviewers: + iclelland@chromium.org
Not sure who all should be OWNERS of origin_trials code, but I figure it probably makes sense to at least have some consistent set of owners.
On 2016/03/11 21:21:34, Marijn Kruisselbrink wrote: > Not sure who all should be OWNERS of origin_trials code, but I figure it > probably makes sense to at least have some consistent set of owners. I'm fine with this, if higher-up-owners are as well. There was concern at some point that the EF was so cross-cutting that changes should really be reviewed by owners with wider scope. (I've no idea how to search Rietveld for old, closed issues, but chasej@ had one about 6 months ago like this). I think that the scope has been defined sufficiently now that this shouldn't be so much of an issue anymore. Anyway, non-owner lgtm. (And TIL about file inclusion in OWNERS files, thanks!)
On 2016/03/14 15:32:16, iclelland wrote: > On 2016/03/11 21:21:34, Marijn Kruisselbrink wrote: > > Not sure who all should be OWNERS of origin_trials code, but I figure it > > probably makes sense to at least have some consistent set of owners. > > I'm fine with this, if higher-up-owners are as well. There was concern at some > point that the EF was so cross-cutting that changes should really be reviewed by > owners with wider scope. (I've no idea how to search Rietveld for old, closed > issues, but chasej@ had one about 6 months ago like this). I think that the > scope has been defined sufficiently now that this shouldn't be so much of an > issue anymore. > > Anyway, non-owner lgtm. > > (And TIL about file inclusion in OWNERS files, thanks!) The CL that Ian refers to is here: https://codereview.chromium.org/1784243005/ There, the idea was that owners in directories under third_party/WebKit/Source/core should already be owners for core/. Based on that approach, there should not be an OWNERS file under .../core/origin_trials.
chasej@chromium.org changed reviewers: + chasej@chromium.org
https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... File content/common/origin_trials/OWNERS (right): https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... content/common/origin_trials/OWNERS:7: chasej@chromium.org This should be removed, as I don't have committer status.
On 2016/03/14 at 15:56:58, chasej wrote: > On 2016/03/14 15:32:16, iclelland wrote: > > On 2016/03/11 21:21:34, Marijn Kruisselbrink wrote: > > > Not sure who all should be OWNERS of origin_trials code, but I figure it > > > probably makes sense to at least have some consistent set of owners. > > > > I'm fine with this, if higher-up-owners are as well. There was concern at some > > point that the EF was so cross-cutting that changes should really be reviewed by > > owners with wider scope. (I've no idea how to search Rietveld for old, closed > > issues, but chasej@ had one about 6 months ago like this). I think that the > > scope has been defined sufficiently now that this shouldn't be so much of an > > issue anymore. The cross-cutting aspects like bindings related stuff will still require owners with wider scope anyway... > > > > Anyway, non-owner lgtm. > > > > (And TIL about file inclusion in OWNERS files, thanks!) > > The CL that Ian refers to is here: > https://codereview.chromium.org/1784243005/ That's a link to this CL, presumably you meant to link to something else? > There, the idea was that owners in directories under third_party/WebKit/Source/core should already be owners for core/. Based on that approach, there should not be an OWNERS file under .../core/origin_trials. Hmm yeah, I keep forgetting about that peculiarity of blink owners and its divergence with general chromium policies... It seems to be a topic that comes up for discussion about once a year (https://groups.google.com/a/chromium.org/d/msg/blink-dev/StkI3k4Pm5U/i5PdztzM... has the 2014 and 2015 versions of the discussion), but never really reaches any conclusion as far as I can tell... (and "no owners in subdirectories of core/ that aren't also owners in core/" also isn't actually being followed currently; in practice it all seems to come down to personal preference of whichever reviewer you pick...). https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... File content/common/origin_trials/OWNERS (right): https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... content/common/origin_trials/OWNERS:7: chasej@chromium.org On 2016/03/14 at 15:57:52, chasej wrote: > This should be removed, as I don't have committer status. I don't think it is a strict requirement for OWNERS to also be committers (rather the commit queue makes sure that an LGTM is present both from all the required owners and from at least one committer), but if you'd rather not be an OWNER, I can remove you of course.
On 2016/03/14 19:14:52, Marijn Kruisselbrink wrote: > On 2016/03/14 at 15:56:58, chasej wrote: > > On 2016/03/14 15:32:16, iclelland wrote: > > > On 2016/03/11 21:21:34, Marijn Kruisselbrink wrote: > > > > Not sure who all should be OWNERS of origin_trials code, but I figure it > > > > probably makes sense to at least have some consistent set of owners. > > > > > > I'm fine with this, if higher-up-owners are as well. There was concern at > some > > > point that the EF was so cross-cutting that changes should really be > reviewed by > > > owners with wider scope. (I've no idea how to search Rietveld for old, > closed > > > issues, but chasej@ had one about 6 months ago like this). I think that the > > > scope has been defined sufficiently now that this shouldn't be so much of an > > > issue anymore. > > The cross-cutting aspects like bindings related stuff will still require owners > with wider scope anyway... > > > > > > > Anyway, non-owner lgtm. > > > > > > (And TIL about file inclusion in OWNERS files, thanks!) > > > > The CL that Ian refers to is here: > > https://codereview.chromium.org/1784243005/ > > That's a link to this CL, presumably you meant to link to something else? Copy/paste fail, correct link: https://codereview.chromium.org/1412943010/ > > > There, the idea was that owners in directories under > third_party/WebKit/Source/core should already be owners for core/. Based on that > approach, there should not be an OWNERS file under .../core/origin_trials. > > Hmm yeah, I keep forgetting about that peculiarity of blink owners and its > divergence with general chromium policies... It seems to be a topic that comes > up for discussion about once a year > (https://groups.google.com/a/chromium.org/d/msg/blink-dev/StkI3k4Pm5U/i5PdztzM... > has the 2014 and 2015 versions of the discussion), but never really reaches any > conclusion as far as I can tell... (and "no owners in subdirectories of core/ > that aren't also owners in core/" also isn't actually being followed currently; > in practice it all seems to come down to personal preference of whichever > reviewer you pick...). > > https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... > File content/common/origin_trials/OWNERS (right): > > https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... > content/common/origin_trials/OWNERS:7: mailto:chasej@chromium.org > On 2016/03/14 at 15:57:52, chasej wrote: > > This should be removed, as I don't have committer status. > > I don't think it is a strict requirement for OWNERS to also be committers > (rather the commit queue makes sure that an LGTM is present both from all the > required owners and from at least one committer), but if you'd rather not be an > OWNER, I can remove you of course. I see, I thought owners had to be committers. If not, then yes I would like to be an owner.
On 2016/03/14 at 19:57:55, chasej wrote: > On 2016/03/14 19:14:52, Marijn Kruisselbrink wrote: > > On 2016/03/14 at 15:56:58, chasej wrote: > > > On 2016/03/14 15:32:16, iclelland wrote: > > > > On 2016/03/11 21:21:34, Marijn Kruisselbrink wrote: > > > > > Not sure who all should be OWNERS of origin_trials code, but I figure it > > > > > probably makes sense to at least have some consistent set of owners. > > > > > > > > I'm fine with this, if higher-up-owners are as well. There was concern at > > some > > > > point that the EF was so cross-cutting that changes should really be > > reviewed by > > > > owners with wider scope. (I've no idea how to search Rietveld for old, > > closed > > > > issues, but chasej@ had one about 6 months ago like this). I think that the > > > > scope has been defined sufficiently now that this shouldn't be so much of an > > > > issue anymore. > > > > The cross-cutting aspects like bindings related stuff will still require owners > > with wider scope anyway... > > > > > > > > > > Anyway, non-owner lgtm. > > > > > > > > (And TIL about file inclusion in OWNERS files, thanks!) > > > > > > The CL that Ian refers to is here: > > > https://codereview.chromium.org/1784243005/ > > > > That's a link to this CL, presumably you meant to link to something else? > Copy/paste fail, correct link: https://codereview.chromium.org/1412943010/ Thanks, I'll check with elliot, although it seems unlikely for him to have changed his mind. > > > > > There, the idea was that owners in directories under > > third_party/WebKit/Source/core should already be owners for core/. Based on that > > approach, there should not be an OWNERS file under .../core/origin_trials. > > > > Hmm yeah, I keep forgetting about that peculiarity of blink owners and its > > divergence with general chromium policies... It seems to be a topic that comes > > up for discussion about once a year > > (https://groups.google.com/a/chromium.org/d/msg/blink-dev/StkI3k4Pm5U/i5PdztzM... > > has the 2014 and 2015 versions of the discussion), but never really reaches any > > conclusion as far as I can tell... (and "no owners in subdirectories of core/ > > that aren't also owners in core/" also isn't actually being followed currently; > > in practice it all seems to come down to personal preference of whichever > > reviewer you pick...). > > > > https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... > > File content/common/origin_trials/OWNERS (right): > > > > https://codereview.chromium.org/1784243005/diff/40001/content/common/origin_t... > > content/common/origin_trials/OWNERS:7: mailto:chasej@chromium.org > > On 2016/03/14 at 15:57:52, chasej wrote: > > > This should be removed, as I don't have committer status. > > > > I don't think it is a strict requirement for OWNERS to also be committers > > (rather the commit queue makes sure that an LGTM is present both from all the > > required owners and from at least one committer), but if you'd rather not be an > > OWNER, I can remove you of course. > I see, I thought owners had to be committers. If not, then yes I would like to be an owner. Actually, yeah, there does seem to be such a policy (https://www.chromium.org/developers/owners-files#TOC-Who-should-be-in-an-OWNE...), although that policy would also exclude iclelland from being an owner since he hasn't been a committer for more than 6 months yet, so not sure how much of that policy is actually followed in practice.
On 2016/03/14 20:48:50, Marijn Kruisselbrink wrote: > Actually, yeah, there does seem to be such a policy > (https://www.chromium.org/developers/owners-files#TOC-Who-should-be-in-an-OWNE...), > although that policy would also exclude iclelland from being an owner since he > hasn't been a committer for more than 6 months yet As of today, that particular issue has been resolved :)
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
not lgtm, this gives people out in content the power to add core owners without any review... https://codereview.chromium.org/1784243005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OWNERS (right): https://codereview.chromium.org/1784243005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OWNERS:1: file://content/common/origin_trials/OWNERS we shouldn't depend on content like this, also this means someone in content can lgtm a new owner that has access to every API in core.
esprehn@chromium.org changed reviewers: + jam@chromium.org
note that in general this patch is not okay, you can't add file:// refs to owners in content like this.
On 2016/04/14 at 20:39:44, esprehn wrote: > note that in general this patch is not okay, you can't add file:// refs to owners in content like this. So are you saying non OWNERS files in blink should refer to OWNERS files outside of blink? Or does that only apply for core and similar directories? In particular what about modules/push_messaging and public/platform/modules/push_messaging? Is that okay because it's in modules, or would you have objected to that as well if you would have seen it? And yeah, I wasn't sure about the blink side of this patch, since I don't really understand the policies around core/ and core/subdir OWNERS. Which is why I hoped to get a better understanding of that before sending this CL out for OWNERS review (hence the email I send you which I assume is how you came across this CL).
On 2016/04/14 at 20:53:06, mek wrote: > On 2016/04/14 at 20:39:44, esprehn wrote: > > note that in general this patch is not okay, you can't add file:// refs to owners in content like this. > > So are you saying non OWNERS files in blink should refer to OWNERS files outside of blink? Or does that only apply for core and similar directories? In particular what about modules/push_messaging and public/platform/modules/push_messaging? Is that okay because it's in modules, or would you have objected to that as well if you would have seen it? Yes those should not have been added. Being an OWNER of code in components/ or content/ does not mean you're ready to work within the constraints of the blink codebase. > > And yeah, I wasn't sure about the blink side of this patch, since I don't really understand the policies around core/ and core/subdir OWNERS. Which is why I hoped to get a better understanding of that before sending this CL out for OWNERS review (hence the email I send you which I assume is how you came across this CL). Yeah I think we need more explicit policies or a presubmit or something. :)
As the policy around OWNERS files indeed seems to indicate that only full committers should be owners, I'm no longer adding chasej@ to OWNERS files (for now). Also (after discussing with esprehn) changed the blink core/origin_trials OWNERS file to just include myself for now.
You're not a core/ owner though, and haven't contributed much code to core: https://blink.lc/chromium/log/third_party/WebKit/Source/core?qt=author&q=mek%... I don't think you need this OWNERS file yet. There's no reason to plant a flag when adding a feature. :) Lets just leave that file out.
On 2016/04/20 at 22:51:18, esprehn wrote: > You're not a core/ owner though, and haven't contributed much code to core: > https://blink.lc/chromium/log/third_party/WebKit/Source/core?qt=author&q=mek%... > > I don't think you need this OWNERS file yet. There's no reason to plant a flag when adding a feature. :) > > Lets just leave that file out. Okay, done. Sorry for the misunderstanding.
lgtm
lgtm
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org Link to the patchset: https://codereview.chromium.org/1784243005/#ps100001 (title: "no blink OWNERS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1784243005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1784243005/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add origin_trials OWNERS files to the various origin_trials directories. Puts the actual list of owners in content/common/origin_trials/OWNERS with the other directories refering to that file. ========== to ========== Add origin_trials OWNERS files to the various origin_trials directories. Puts the actual list of owners in content/common/origin_trials/OWNERS with the other directories refering to that file. Committed: https://crrev.com/06010eabdf60846e08026b4e402844b336a800a3 Cr-Commit-Position: refs/heads/master@{#388858} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/06010eabdf60846e08026b4e402844b336a800a3 Cr-Commit-Position: refs/heads/master@{#388858} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
