Remove chrome:: namespace from feedback_dialog_utils [chromium/src : main]
Jincheol Jo (Gerrit)
Jincheol Jo added 1 comment![]( "Open in Gerrit")
Related details
- Hidehiko Abe
- Jimmy Gong
- Xiangdong Kong
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 2 comments![]( "Open in Gerrit")
cc: tluk@ for browser code structure.
namespace chrome {I saw some discussion about similar pattern.
https://chromium-review.googlesource.com/c/chromium/src/+/6472403/comments/1da4c962_b2600f31
and there looks no very clear consensus? The linked bug also looked very old and so based on old C++ specs.
quoting: the description of the original comment
Methods in the global scope should be used rarely and have distinct names; prefer static methods on classes where possible.
I think this is still conceptually valid. In this specific case, everything that are being distributed into global namespace is feedback dialog related. Can we just group them into the case?
Any discussions did you have with surrounding/expert people already? If so, could you share the reference so that I can take a look?
Related details
- Jimmy Gong
- Jincheol Jo
- Tom Lukaszewicz
- Xiangdong Kong
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jincheol Jo (Gerrit)
Jincheol Jo added 1 comment![]( "Open in Gerrit")
namespace chrome {I saw some discussion about similar pattern.
https://chromium-review.googlesource.com/c/chromium/src/+/6472403/comments/1da4c962_b2600f31
and there looks no very clear consensus? The linked bug also looked very old and so based on old C++ specs.
quoting: the description of the original comment
Methods in the global scope should be used rarely and have distinct names; prefer static methods on classes where possible.
I think this is still conceptually valid. In this specific case, everything that are being distributed into global namespace is feedback dialog related. Can we just group them into the case?
Any discussions did you have with surrounding/expert people already? If so, could you share the reference so that I can take a look?
Hi. Hidehiko.
1. When you say “group them into the case,” do you mean putting them into a feedback_dialog_utils class or namespace?
2. I haven’t discussed this with any experts — I just made the contribution based on that issue.
Thanks
Related details
- Hidehiko Abe
- Jimmy Gong
- Tom Lukaszewicz
- Xiangdong Kong
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 1 comment![]( "Open in Gerrit")
namespace chrome {Jincheol JoI saw some discussion about similar pattern.
https://chromium-review.googlesource.com/c/chromium/src/+/6472403/comments/1da4c962_b2600f31
and there looks no very clear consensus? The linked bug also looked very old and so based on old C++ specs.
quoting: the description of the original comment
Methods in the global scope should be used rarely and have distinct names; prefer static methods on classes where possible.
I think this is still conceptually valid. In this specific case, everything that are being distributed into global namespace is feedback dialog related. Can we just group them into the case?
Any discussions did you have with surrounding/expert people already? If so, could you share the reference so that I can take a look?
Hi. Hidehiko.
1. When you say “group them into the case,” do you mean putting them into a feedback_dialog_utils class or namespace?
2. I haven’t discussed this with any experts — I just made the contribution based on that issue.
Thanks
Marked as unresolved.
“group them into the case,” do you mean putting them into a feedback_dialog_utils class or namespace?
something like that, but TBD details. (Up to the owner of feedback module)
I haven’t discussed this with any experts — I just made the contribution based on that issue.
I'd recommend to do this with holding on this CL review, as starting point to move forward. We can resume the CL review, once we have clear consensus/recommendations from the experts.
Related details
- Jimmy Gong
- Jincheol Jo
- Tom Lukaszewicz
- Xiangdong Kong
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 1 comment![]( "Open in Gerrit")
namespace chrome {Jincheol JoI saw some discussion about similar pattern.
https://chromium-review.googlesource.com/c/chromium/src/+/6472403/comments/1da4c962_b2600f31
and there looks no very clear consensus? The linked bug also looked very old and so based on old C++ specs.
quoting: the description of the original comment
Methods in the global scope should be used rarely and have distinct names; prefer static methods on classes where possible.
I think this is still conceptually valid. In this specific case, everything that are being distributed into global namespace is feedback dialog related. Can we just group them into the case?
Any discussions did you have with surrounding/expert people already? If so, could you share the reference so that I can take a look?
Hidehiko AbeHi. Hidehiko.
1. When you say “group them into the case,” do you mean putting them into a feedback_dialog_utils class or namespace?
2. I haven’t discussed this with any experts — I just made the contribution based on that issue.
Thanks
Marked as unresolved.
“group them into the case,” do you mean putting them into a feedback_dialog_utils class or namespace?
something like that, but TBD details. (Up to the owner of feedback module)
I haven’t discussed this with any experts — I just made the contribution based on that issue.
I'd recommend to do this with holding on this CL review, as starting point to move forward. We can resume the CL review, once we have clear consensus/recommendations from the experts.
+1 on hidehiko's comment above - we should re-open this discussion and cc others from the CL above and get to consensus before proceeding further with this refactor.
Related details
- Hidehiko Abe
- Jimmy Gong
- Jincheol Jo
- Xiangdong Kong
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 1 comment![]( "Open in Gerrit")
namespace chrome {Jincheol JoI saw some discussion about similar pattern.
https://chromium-review.googlesource.com/c/chromium/src/+/6472403/comments/1da4c962_b2600f31
and there looks no very clear consensus? The linked bug also looked very old and so based on old C++ specs.
quoting: the description of the original comment
Methods in the global scope should be used rarely and have distinct names; prefer static methods on classes where possible.
I think this is still conceptually valid. In this specific case, everything that are being distributed into global namespace is feedback dialog related. Can we just group them into the case?
Any discussions did you have with surrounding/expert people already? If so, could you share the reference so that I can take a look?
Hidehiko AbeHi. Hidehiko.
1. When you say “group them into the case,” do you mean putting them into a feedback_dialog_utils class or namespace?
2. I haven’t discussed this with any experts — I just made the contribution based on that issue.
Thanks
Tom LukaszewiczMarked as unresolved.
“group them into the case,” do you mean putting them into a feedback_dialog_utils class or namespace?
something like that, but TBD details. (Up to the owner of feedback module)
I haven’t discussed this with any experts — I just made the contribution based on that issue.
I'd recommend to do this with holding on this CL review, as starting point to move forward. We can resume the CL review, once we have clear consensus/recommendations from the experts.
+1 on hidehiko's comment above - we should re-open this discussion and cc others from the CL above and get to consensus before proceeding further with this refactor.
Marked as unresolved.
Related details
- Hidehiko Abe
- Jimmy Gong
- Jincheol Jo
- Xiangdong Kong
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |