Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
340863002:
Add danakj as OWNERS for content/common/cc_messages_unittest.cc (Closed)
|
Created:
6 years, 6 months ago by danakj Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, enne (OOO) Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd danakj as OWNERS for content/common/cc_messages_unittest.cc
R=piman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278663
Patch Set 1 #
Total comments: 4
Patch Set 2 : owners: . #Patch Set 3 : owners: . #Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS File content/common/OWNERS (right): https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS#newcode55 content/common/OWNERS:55: per-file cc_messages*=danakj@chromium.org all IPC message files need a person from the security review to sign off on (see the comment on line 21).
https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS File content/common/OWNERS (right): https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS#newcode55 content/common/OWNERS:55: per-file cc_messages*=danakj@chromium.org On 2014/06/18 17:14:58, jam wrote: > all IPC message files need a person from the security review to sign off on (see > the comment on line 21). I was mimicing the WebSocket above. I can change this to .cc and _unittest.cc if you prefer? Was not trying to sidestep the security team rules on the .h, I assumed this worked out from the WebSocket rule, I guess not?
changed it to cc_messages*.cc
https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS File content/common/OWNERS (right): https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS#newcode55 content/common/OWNERS:55: per-file cc_messages*=danakj@chromium.org On 2014/06/18 17:28:00, danakj wrote: > On 2014/06/18 17:14:58, jam wrote: > > all IPC message files need a person from the security review to sign off on > (see > > the comment on line 21). > > I was mimicing the WebSocket above. I can change this to .cc and _unittest.cc if > you prefer? The websocket exception is for websocket.*, which won't cover websocket_messages.h (i.e. security team still has to review that IPC file) cc_messages.cc has the IPC param traits, which is also something that security team wants to review. > Was not trying to sidestep the security team rules on the .h, I > assumed this worked out from the WebSocket rule, I guess not? no problem, I didn't mean to imply that you're trying to sidestep that.
https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS File content/common/OWNERS (right): https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS#newcode55 content/common/OWNERS:55: per-file cc_messages*=danakj@chromium.org On 2014/06/18 18:04:16, jam wrote: > cc_messages.cc has the IPC param traits, which is also something that security > team wants to review. Is it? There is no rule for that today. We have to change the header file to add a new thing to a message.
On 2014/06/18 18:08:21, danakj wrote: > https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS > File content/common/OWNERS (right): > > https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS#newcode55 > content/common/OWNERS:55: per-file mailto:cc_messages*=danakj@chromium.org > On 2014/06/18 18:04:16, jam wrote: > > cc_messages.cc has the IPC param traits, which is also something that security > > team wants to review. > > Is it? There is no rule for that today. We have to change the header file to add > a new thing to a message. I'm not sure what you mean. That file has custom IPC traits. A bug there would have security ramifications, so that's why they're treated as IPC messages files.
On Wed, Jun 18, 2014 at 12:22 PM, <jam@chromium.org> wrote: > On 2014/06/18 18:08:21, danakj wrote: > >> https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS >> File content/common/OWNERS (right): >> > > > https://codereview.chromium.org/340863002/diff/1/content/ > common/OWNERS#newcode55 > >> content/common/OWNERS:55: per-file mailto:cc_messages*=danakj@ >> chromium.org >> >> On 2014/06/18 18:04:16, jam wrote: >> > cc_messages.cc has the IPC param traits, which is also something that >> > security > >> > team wants to review. >> > > Is it? There is no rule for that today. We have to change the header file >> to >> > add > >> a new thing to a message. >> > > I'm not sure what you mean. > > That file has custom IPC traits. A bug there would have security > ramifications, > so that's why they're treated as IPC messages files. > I mean the security team rules in here are *_message*.h, they exclude .cc files. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/06/18 19:26:00, danakj wrote: > On Wed, Jun 18, 2014 at 12:22 PM, <mailto:jam@chromium.org> wrote: > > > On 2014/06/18 18:08:21, danakj wrote: > > > >> https://codereview.chromium.org/340863002/diff/1/content/common/OWNERS > >> File content/common/OWNERS (right): > >> > > > > > > https://codereview.chromium.org/340863002/diff/1/content/ > > common/OWNERS#newcode55 > > > >> content/common/OWNERS:55: per-file mailto:cc_messages*=danakj@ > >> http://chromium.org > >> > >> On 2014/06/18 18:04:16, jam wrote: > >> > cc_messages.cc has the IPC param traits, which is also something that > >> > > security > > > >> > team wants to review. > >> > > > > Is it? There is no rule for that today. We have to change the header file > >> to > >> > > add > > > >> a new thing to a message. > >> > > > > I'm not sure what you mean. > > > > That file has custom IPC traits. A bug there would have security > > ramifications, > > so that's why they're treated as IPC messages files. > > > > I mean the security team rules in here are *_message*.h, they exclude .cc > files. > that seems like an oversight. +tsepez
> that seems like an oversight. +tsepez Ideally Security-Team would be rejecting CLs that introduce new custom marshaled parameters, which is why I only owned the .h files. Security-Team should also own the .cc's as well.
PTAL
Tom said he'd add the .cc files for the security folks, which will be nice.
lgtm
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/340863002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 278663 |
