Move Chrome Native File System blocklist to own file [chromium/src : main]
Evan Stade (Gerrit)
Evan Stade added 2 comments![]( "Open in Gerrit")
Hi Shelley, thanks for the detailed CL description. I sympathize with the difficulty of maintaining downstream patches. However my thoughts are that this list belongs next to the code that uses it for two main reasons:
1) The blocklist is not independent from the code. To actually implement the same blocklist, you probably do need to use the same code. This is illustrated by the fact that `CheckPathAgainstBlocklist`/`ShouldBlockAccessToPath` are not simple functions. Extracting the blocklist and writing other code against it will inevitably lead to behavioral differences. For example, that approach would not pick up crrev.com/0dfa4991c6b522df6c130e948f8d2054d3618282
2) In general, we do prefer to inline constants in implementation files if they're only relevant to that one file. To do otherwise is to suggest to the reader that these constants need to be accessed by more than one bit of implementation. In this case, in the context of Chromium, that implication would be false. And the result is that it's pretty likely that someone will come along, be confused (bad enough on its own), and perhaps even reverse this refactorization. There is nothing in Chromium stopping this from happening.
This is a more specific way of voicing the general rule that "Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design
decision." [1]
[1] https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#Code-guidelines
inline constexpr const int kNoBasePathKey = -1;(Easy to fix, but FWIW) we avoid adding constants to the global scope.
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
Related details
- Daseul Lee
- Marijn Kruisselbrink
- Shelley Vohr
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marijn Kruisselbrink (Gerrit)
Marijn Kruisselbrink added 1 comment![]( "Open in Gerrit")
Hi Shelley, thanks for the detailed CL description. I sympathize with the difficulty of maintaining downstream patches. However my thoughts are that this list belongs next to the code that uses it for two main reasons:
1) The blocklist is not independent from the code. To actually implement the same blocklist, you probably do need to use the same code. This is illustrated by the fact that `CheckPathAgainstBlocklist`/`ShouldBlockAccessToPath` are not simple functions. Extracting the blocklist and writing other code against it will inevitably lead to behavioral differences. For example, that approach would not pick up crrev.com/0dfa4991c6b522df6c130e948f8d2054d3618282
2) In general, we do prefer to inline constants in implementation files if they're only relevant to that one file. To do otherwise is to suggest to the reader that these constants need to be accessed by more than one bit of implementation. In this case, in the context of Chromium, that implication would be false. And the result is that it's pretty likely that someone will come along, be confused (bad enough on its own), and perhaps even reverse this refactorization. There is nothing in Chromium stopping this from happening.
This is a more specific way of voicing the general rule that "Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design
decision." [1][1] https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#Code-guidelines
I'll defer to Daseul and Evan for actually reviewing this, but it might be reasonable to move the block list with its supporting code to a separate .h/.cc file. The .h file would then only expose some kind of "CheckPathAgainstBlocklist" method while the blocklist and all the code referencing it would still live entirely in (its own) implementation file. Of course it would still be true that nothing stops anybody from reverting such a refactor (and not sure if that would even help the electron case here), but I think an argument could be made that "file blocklist behavior" is enough of a separate thing to warrant having its own files, even if they are only used from one other file.
Related details
- Daseul Lee
- Evan Stade
- Shelley Vohr
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shelley Vohr (Gerrit)
Shelley Vohr added 1 comment![]( "Open in Gerrit")
Marijn KruisselbrinkHi Shelley, thanks for the detailed CL description. I sympathize with the difficulty of maintaining downstream patches. However my thoughts are that this list belongs next to the code that uses it for two main reasons:
1) The blocklist is not independent from the code. To actually implement the same blocklist, you probably do need to use the same code. This is illustrated by the fact that `CheckPathAgainstBlocklist`/`ShouldBlockAccessToPath` are not simple functions. Extracting the blocklist and writing other code against it will inevitably lead to behavioral differences. For example, that approach would not pick up crrev.com/0dfa4991c6b522df6c130e948f8d2054d3618282
2) In general, we do prefer to inline constants in implementation files if they're only relevant to that one file. To do otherwise is to suggest to the reader that these constants need to be accessed by more than one bit of implementation. In this case, in the context of Chromium, that implication would be false. And the result is that it's pretty likely that someone will come along, be confused (bad enough on its own), and perhaps even reverse this refactorization. There is nothing in Chromium stopping this from happening.
This is a more specific way of voicing the general rule that "Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design
decision." [1][1] https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#Code-guidelines
I'll defer to Daseul and Evan for actually reviewing this, but it might be reasonable to move the block list with its supporting code to a separate .h/.cc file. The .h file would then only expose some kind of "CheckPathAgainstBlocklist" method while the blocklist and all the code referencing it would still live entirely in (its own) implementation file. Of course it would still be true that nothing stops anybody from reverting such a refactor (and not sure if that would even help the electron case here), but I think an argument could be made that "file blocklist behavior" is enough of a separate thing to warrant having its own files, even if they are only used from one other file.
With regard to:
"Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design decision."
I agree and do understand this! This CL was underpinned by my thinking that in this case both can be true - that code should primarily benefit the project, but that this change (or a modified version pending review) could stand alone as a reasonable change within the project to encapsulate a chunk of related code and simplify some related concerns.
Related details
- Daseul Lee
- Evan Stade
- Marijn Kruisselbrink
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
Marijn KruisselbrinkHi Shelley, thanks for the detailed CL description. I sympathize with the difficulty of maintaining downstream patches. However my thoughts are that this list belongs next to the code that uses it for two main reasons:
1) The blocklist is not independent from the code. To actually implement the same blocklist, you probably do need to use the same code. This is illustrated by the fact that `CheckPathAgainstBlocklist`/`ShouldBlockAccessToPath` are not simple functions. Extracting the blocklist and writing other code against it will inevitably lead to behavioral differences. For example, that approach would not pick up crrev.com/0dfa4991c6b522df6c130e948f8d2054d3618282
2) In general, we do prefer to inline constants in implementation files if they're only relevant to that one file. To do otherwise is to suggest to the reader that these constants need to be accessed by more than one bit of implementation. In this case, in the context of Chromium, that implication would be false. And the result is that it's pretty likely that someone will come along, be confused (bad enough on its own), and perhaps even reverse this refactorization. There is nothing in Chromium stopping this from happening.
This is a more specific way of voicing the general rule that "Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design
decision." [1][1] https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#Code-guidelines
Shelley VohrI'll defer to Daseul and Evan for actually reviewing this, but it might be reasonable to move the block list with its supporting code to a separate .h/.cc file. The .h file would then only expose some kind of "CheckPathAgainstBlocklist" method while the blocklist and all the code referencing it would still live entirely in (its own) implementation file. Of course it would still be true that nothing stops anybody from reverting such a refactor (and not sure if that would even help the electron case here), but I think an argument could be made that "file blocklist behavior" is enough of a separate thing to warrant having its own files, even if they are only used from one other file.
With regard to:
"Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design decision."
I agree and do understand this! This CL was underpinned by my thinking that in this case both can be true - that code should primarily benefit the project, but that this change (or a modified version pending review) could stand alone as a reasonable change within the project to encapsulate a chunk of related code and simplify some related concerns.
There are almost 3000 lines in `chrome_file_system_access_permission_context.cc` so I agree that it could conceivably be beneficial to split up somewhat. I don't feel that pulling these constants out of the file is the way to do that (for the reasons listed above).
I will leave it up to Daseul, but if there's some way in which refactoring this file makes sense in the service of Chromium, then that is of course fine to pursue. If the end result of that lessens the burden of downstream maintainers, then terrific! But the explicit justification for the current CL is the benefit of a particular downstream project, and that alone is insufficient to motivate a change.
Related details
- Daseul Lee
- Marijn Kruisselbrink
- Shelley Vohr
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daseul Lee (Gerrit)
Daseul Lee added 1 comment![]( "Open in Gerrit")
Marijn KruisselbrinkHi Shelley, thanks for the detailed CL description. I sympathize with the difficulty of maintaining downstream patches. However my thoughts are that this list belongs next to the code that uses it for two main reasons:
1) The blocklist is not independent from the code. To actually implement the same blocklist, you probably do need to use the same code. This is illustrated by the fact that `CheckPathAgainstBlocklist`/`ShouldBlockAccessToPath` are not simple functions. Extracting the blocklist and writing other code against it will inevitably lead to behavioral differences. For example, that approach would not pick up crrev.com/0dfa4991c6b522df6c130e948f8d2054d3618282
2) In general, we do prefer to inline constants in implementation files if they're only relevant to that one file. To do otherwise is to suggest to the reader that these constants need to be accessed by more than one bit of implementation. In this case, in the context of Chromium, that implication would be false. And the result is that it's pretty likely that someone will come along, be confused (bad enough on its own), and perhaps even reverse this refactorization. There is nothing in Chromium stopping this from happening.
This is a more specific way of voicing the general rule that "Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design
decision." [1][1] https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#Code-guidelines
Shelley VohrI'll defer to Daseul and Evan for actually reviewing this, but it might be reasonable to move the block list with its supporting code to a separate .h/.cc file. The .h file would then only expose some kind of "CheckPathAgainstBlocklist" method while the blocklist and all the code referencing it would still live entirely in (its own) implementation file. Of course it would still be true that nothing stops anybody from reverting such a refactor (and not sure if that would even help the electron case here), but I think an argument could be made that "file blocklist behavior" is enough of a separate thing to warrant having its own files, even if they are only used from one other file.
Evan StadeWith regard to:
"Code in the Chromium project should be in service of other code in the Chromium project. This is important so developers can understand the constraints informing a design decision."
I agree and do understand this! This CL was underpinned by my thinking that in this case both can be true - that code should primarily benefit the project, but that this change (or a modified version pending review) could stand alone as a reasonable change within the project to encapsulate a chunk of related code and simplify some related concerns.
There are almost 3000 lines in `chrome_file_system_access_permission_context.cc` so I agree that it could conceivably be beneficial to split up somewhat. I don't feel that pulling these constants out of the file is the way to do that (for the reasons listed above).
I will leave it up to Daseul, but if there's some way in which refactoring this file makes sense in the service of Chromium, then that is of course fine to pursue. If the end result of that lessens the burden of downstream maintainers, then terrific! But the explicit justification for the current CL is the benefit of a particular downstream project, and that alone is insufficient to motivate a change.
Thanks for all the input. I do agree this Context file has grown too much and deserves refactoring (and has been on my radar), but first I'd like to understand where/how the blocklist would be used from?
As Evan mentioned, that blocklist alone is only part of the "blocklist" check, so refactoring that list only itself is already a feature parity. There are also checks for symbolic link, local UNC path, profile dir, safe browsing check, which are added over the course of time via various security issues. Any plans to incorporate these safeguards?
Related details
- Evan Stade
- Marijn Kruisselbrink
- Shelley Vohr
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shelley Vohr (Gerrit)
Shelley Vohr abandoned this change.
Shelley Vohr abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nuance Tulel (Gerrit)
Nuance Tulel added 1 comment![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nuance Tulel (Gerrit)
Nuance Tulel added 1 comment![]( "Open in Gerrit")
https://source.chromium.org/chromium/infra/infra_superproject/+/main:https://t.me/SoSoValueCommunity_02/1/19730https://t.me/SoSoValueCommunity_02/1/19730https://t.me/blurrr_dating_bot?start=_tgr_I5L9NRo4ODJlhttps://tonhub.com/ton-connect?r={"manifestUrl":"https://app.ston.fi/tonconnect-manifest.json","items":[{"name":"ton_addr"}]}&v=2&id=27ab42a9e22313a345874b18662321a7558d8dd1280bbedfaf9067ca9961d07&ret=googlechrome://&_thb=1
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |