1376652 - Include security/sandbox/chromium-shim/ in ThirdPartyPaths.txt

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Jan Keromnes [:janx]]

Bug 1102215 moved security/sandbox/chromium/base/shim/ out to security/sandbox/chromium-shim/, and ThirdPartyPaths.txt should probably be updated.

Comment 1

[Jan Keromnes [:janx]]

Attachment: Add security/sandbox/chromium-shim/ to ThirdPartyPaths.txt.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Are you sure? 
seems like code that we modified / own

Comment 3

[Jan Keromnes [:janx]]

(Please use needinfo, otherwise I'll probably miss questions)

(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> Are you sure? 
> seems like code that we modified / own

It does look like something we forked, but for example the top comment mentions that the coding style is kept intentionally close to Chromium's in order to facilitate future merges (so we probably shouldn't clang-format it to Mozilla style).

Additionally, the entire folder used to live under security/sandbox/chromium/, so it was covered by ThirdPartyPaths.txt originally, and this changed (maybe unintentionally) when it was moved out.

Ted, any thoughts on ignoring security/sandbox/chromium-shim/ while doing static analysis fixes or other automated rewriting?

Comment 4

[(not currently active) Ted Mielczarek]

I'm afraid I don't know whether that code is considered third-party or not! Bob: can you answer Jan's question in comment 3?

Comment 5

[Bob Owen (:bobowen)]

(In reply to Jan Keromnes [:janx] from comment #3)
> (Please use needinfo, otherwise I'll probably miss questions)
> 
> (In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> > Are you sure? 
> > seems like code that we modified / own
> 
> It does look like something we forked, but for example the top comment
> mentions that the coding style is kept intentionally close to Chromium's in
> order to facilitate future merges (so we probably shouldn't clang-format it
> to Mozilla style).
> 
> Additionally, the entire folder used to live under
> security/sandbox/chromium/, so it was covered by ThirdPartyPaths.txt
> originally, and this changed (maybe unintentionally) when it was moved out.
> 
> Ted, any thoughts on ignoring security/sandbox/chromium-shim/ while doing
> static analysis fixes or other automated rewriting?

This is our code, so we should probably be running static analysis on it, but we don't really want to do any automated rewriting.

Comment 6

[Jan Keromnes [:janx]]

(In reply to Bob Owen (:bobowen) from comment #5)
> This is our code, so we should probably be running static analysis on it,
> but we don't really want to do any automated rewriting.

Sylvestre, is ThirdPartyPaths.txt a good place for folders where we want to run static analysis, but not do any automated rewriting in?

If it's not clear what to do with this folder, I'll opt for doing nothing and close this bug as WONTFIX.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

To me, we want to run static analysis on all codes (including thirdpartypaths.txt libraries) as third party bugs might impact Firefox at the end.

Bug we don't want to run coding style tools (eslint, clang-format, etc) on them.

As it is our code, it should not be on that list and we should wontfix it.

Comment 8

[Jan Keromnes [:janx]]

Works for me. Thanks!