[cleanup]Remove use of chrome:: namespace in "chrome/browser/lifetime/application_lifetime.h" [chromium/src : main]
Vishwa Kalubowila (Gerrit)
Vishwa Kalubowila added 3 comments![]( "Open in Gerrit")
Mark Mentovaihi. in the future, please do not send CLs to 19 reviewers.
Vishwa Kalubowilahi. in the future, please do not send CLs to 19 reviewers.
I agree. `git cl split` would have been a good way to divvy this up.
As I understand it, this can’t actually land as long as grt’s Code-Review: -1 stands.
Greg ThompsonThanks for the feedback. I agree that 19 reviewers is too many, I will use git cl split to manage them better for future CLs. Can you please suggest way that can be done about this?
Vishwa KalubowilaThanks for linking to the bug. It seems to me that the project's policies on namespace use has changed quite a bit since 2013, so I'm a bit surprised to see this effort getting traction now. That said, enough other folks in the project seem to be onboard with this, so I'll flip my CR-1 to a CR+1.
Done
[cleanup]Remove use of chrome:: namespace in "chrome/browser/lifetime/*"Vishwa Kalubowilathis is inaccurate
Done
This CL removes usage of the chrome:: namespace in various files, based on
the discussion in chromium-dev. The goal is to reduce inconsistent andVishwa Kalubowilato what discussion are you referring?
Vishwa Kalubowilahi, Thanks for reviewing! I’ve updated and mentioned the issue.
Done
Related details
- Achuith Bhandarkar
- Alex Rudenko
- Dana Fried
- Danila Kuzmin
- Denis Kuznetsov
- Edman Anjos
- Elaine Chien
- Erik Chen
- Greg Thompson
- Marijn Kruisselbrink
- Mark Mentovai
- Mark Pearson
- Nicholas Verne
- Rafał Godlewski
- Ramin Halavati
- Stefan Kuhne
- Vasilii Sukhanov
- Xiyuan Xia
- Yury Khmel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vishwa Kalubowila (Gerrit)
Vishwa Kalubowila added 3 comments![]( "Open in Gerrit")
Vishwa KalubowilaLGTM for c/b/ui only. Do not submit until all approvals.
Done
namespace chrome{Vishwa KalubowilaWhy keeping this? And if we want to keep it, please restore the white spaces here and L60.
Done
FROM_HERE, base::BindOnce(&chrome::AttemptExit));Vishwa Kalubowilarestore `&`
Done
Vishwa Kalubowila (Gerrit)
Vishwa Kalubowila added 1 comment![]( "Open in Gerrit")
Vishwa KalubowilaLGTM for c/b/ui only. Do not submit until all approvals.
Done
Please let me know if there’s anything else you’d like me to change before I proceed. Thank you!
Yury Khmel (Gerrit)
Yury Khmel voted Code-Review+1![]( "Open in Gerrit")
- Achuith Bhandarkar
- Alex Rudenko
- Dana Fried
- Danila Kuzmin
- Denis Kuznetsov
- Edman Anjos
- Elaine Chien
- Erik Chen
- Greg Thompson
- Marijn Kruisselbrink
- Mark Mentovai
- Mark Pearson
- Nicholas Verne
- Rafał Godlewski
- Ramin Halavati
- Stefan Kuhne
- Vasilii Sukhanov
- Vishwa Kalubowila
- Xiyuan Xia
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
I'm not convinced I love this change because having the chrome:: prefix for functions that would otherwise be in the global namespace helps to differentiate them from e.g. member functions.
It makes the code somewhat less readable.
Related details
- Achuith Bhandarkar
- Alex Rudenko
Erik Chen (Gerrit)
Erik Chen added 2 comments![]( "Open in Gerrit")
Erik Chenauthors and CL lgtm, need to fix commit message
this has not been resolved.
I'm not convinced I love this change because having the chrome:: prefix for functions that would otherwise be in the global namespace helps to differentiate them from e.g. member functions.
It makes the code somewhat less readable.
I'm generally inclined to agree with Dana. However, this CL links a 12-year old bug that claims there was consensus to remove this, and there's a bunch of CLs to do so. I don't think this is a big deal either way, and I value consistency over my personal preferences for minor styling.
Related details
- Achuith Bhandarkar
- Alex Rudenko
- Danila Kuzmin
- Denis Kuznetsov
- Edman Anjos
- Elaine Chien
- Greg Thompson
- Marijn Kruisselbrink
- Mark Mentovai
- Mark Pearson
- Nicholas Verne
Xiyuan Xia (Gerrit)
Xiyuan Xia added 1 comment![]( "Open in Gerrit")
Erik ChenI'm not convinced I love this change because having the chrome:: prefix for functions that would otherwise be in the global namespace helps to differentiate them from e.g. member functions.
It makes the code somewhat less readable.
I'm generally inclined to agree with Dana. However, this CL links a 12-year old bug that claims there was consensus to remove this, and there's a bunch of CLs to do so. I don't think this is a big deal either way, and I value consistency over my personal preferences for minor styling.
+1
It is mixed for me too. It feels more clear to keep `AttemptUserExit` etc inside `chrome::` since it is a chrome browser specific function. `AttemptUserExit` is kind of general and not clear without context. And `chrome::` could provide that context that the exit is for chrome.
But again, I am on the fence. So people speak up.
Related details
- Achuith Bhandarkar
- Alex Rudenko
- Danila Kuzmin
- Denis Kuznetsov
- Edman Anjos
- Elaine Chien
- Greg Thompson
- Marijn Kruisselbrink
- Mark Mentovai
- Mark Pearson
- Nicholas Verne
- Ramin Halavati
- Stefan Kuhne
- Vasilii Sukhanov
- Vishwa Kalubowila
Mark Pearson (Gerrit)
Mark Pearson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Pearson
| Code-Review | +1 |
1 comment
restoring +1 for the file I previously reviewed
(I do not feel the need to participate in discussions about this changelist.)
Related details
- Achuith Bhandarkar
- Alex Rudenko
- Danila Kuzmin
- Denis Kuznetsov
- Edman Anjos
- Elaine Chien
- Greg Thompson
- Marijn Kruisselbrink
- Mark Mentovai
- Nicholas Verne
- Ramin Halavati
- Stefan Kuhne
- Vasilii Sukhanov
- Vishwa Kalubowila
Code-Coverage
Code-Review
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
Erik ChenI'm not convinced I love this change because having the chrome:: prefix for functions that would otherwise be in the global namespace helps to differentiate them from e.g. member functions.
It makes the code somewhat less readable.
Xiyuan XiaI'm generally inclined to agree with Dana. However, this CL links a 12-year old bug that claims there was consensus to remove this, and there's a bunch of CLs to do so. I don't think this is a big deal either way, and I value consistency over my personal preferences for minor styling.
+1
It is mixed for me too. It feels more clear to keep `AttemptUserExit` etc inside `chrome::` since it is a chrome browser specific function. `AttemptUserExit` is kind of general and not clear without context. And `chrome::` could provide that context that the exit is for chrome.
But again, I am on the fence. So people speak up.
As I wrote in an earlier patch set: "It seems to me that the project's policies on namespace use have changed quite a bit since 2013, so I'm a bit surprised to see this effort getting traction now." I'm on the fence, as the use of "chrome::" was somewhat arbitrary. Going to the style guide, it says "With few exceptions, place code in a namespace." (https://google.github.io/styleguide/cppguide.html#Namespaces) If we want to go toward rather than away from that, we should consider what we would do if this were new code. Would we advocate `lifetime::` or even `browser::lifetime::` here?
Related details
- Achuith Bhandarkar
- Alex Rudenko
- Danila Kuzmin
- Denis Kuznetsov
- Edman Anjos
- Elaine Chien
Stefan Kuhne (Gerrit)
Stefan Kuhne added 1 comment![]( "Open in Gerrit")
Erik ChenI'm not convinced I love this change because having the chrome:: prefix for functions that would otherwise be in the global namespace helps to differentiate them from e.g. member functions.
It makes the code somewhat less readable.
Xiyuan XiaI'm generally inclined to agree with Dana. However, this CL links a 12-year old bug that claims there was consensus to remove this, and there's a bunch of CLs to do so. I don't think this is a big deal either way, and I value consistency over my personal preferences for minor styling.
Greg Thompson+1
It is mixed for me too. It feels more clear to keep `AttemptUserExit` etc inside `chrome::` since it is a chrome browser specific function. `AttemptUserExit` is kind of general and not clear without context. And `chrome::` could provide that context that the exit is for chrome.
But again, I am on the fence. So people speak up.
As I wrote in an earlier patch set: "It seems to me that the project's policies on namespace use have changed quite a bit since 2013, so I'm a bit surprised to see this effort getting traction now." I'm on the fence, as the use of "chrome::" was somewhat arbitrary. Going to the style guide, it says "With few exceptions, place code in a namespace." (https://google.github.io/styleguide/cppguide.html#Namespaces) If we want to go toward rather than away from that, we should consider what we would do if this were new code. Would we advocate `lifetime::` or even `browser::lifetime::` here?
In general I also think that removing ```chrome::``` is removing a bit of the clarity we had. So +1 for the mixed feelings of removing this, and if it would be me, I'd rather would keep it.
Related details
- Achuith Bhandarkar
- Alex Rudenko
- Danila Kuzmin
- Denis Kuznetsov
- Edman Anjos
- Elaine Chien
- Marijn Kruisselbrink
- Mark Mentovai
- Nicholas Verne
- Ramin Halavati
- Vasilii Sukhanov
- Vishwa Kalubowila
Denis Kuznetsov (Gerrit)
Denis Kuznetsov added 1 comment![]( "Open in Gerrit")
Erik ChenI'm not convinced I love this change because having the chrome:: prefix for functions that would otherwise be in the global namespace helps to differentiate them from e.g. member functions.
It makes the code somewhat less readable.
Xiyuan XiaI'm generally inclined to agree with Dana. However, this CL links a 12-year old bug that claims there was consensus to remove this, and there's a bunch of CLs to do so. I don't think this is a big deal either way, and I value consistency over my personal preferences for minor styling.
Greg Thompson+1
It is mixed for me too. It feels more clear to keep `AttemptUserExit` etc inside `chrome::` since it is a chrome browser specific function. `AttemptUserExit` is kind of general and not clear without context. And `chrome::` could provide that context that the exit is for chrome.
But again, I am on the fence. So people speak up.
Stefan KuhneAs I wrote in an earlier patch set: "It seems to me that the project's policies on namespace use have changed quite a bit since 2013, so I'm a bit surprised to see this effort getting traction now." I'm on the fence, as the use of "chrome::" was somewhat arbitrary. Going to the style guide, it says "With few exceptions, place code in a namespace." (https://google.github.io/styleguide/cppguide.html#Namespaces) If we want to go toward rather than away from that, we should consider what we would do if this were new code. Would we advocate `lifetime::` or even `browser::lifetime::` here?
In general I also think that removing ```chrome::``` is removing a bit of the clarity we had. So +1 for the mixed feelings of removing this, and if it would be me, I'd rather would keep it.
+1 to mixed feelings, I understand the reason for reducing noise, but in this case I am gravitating towards keeping the namespace (for reason Xiyuan mentioned)
Related details
- Achuith Bhandarkar
- Danila Kuzmin
Edman Anjos (Gerrit)
Edman Anjos added 1 comment![]( "Open in Gerrit")
removing myself from reviewers to clear my reviews inbox. feel free to re-add me if you need my review.
Related details
- Achuith Bhandarkar
- Elaine Chien
- Mark Mentovai
- Nicholas Verne
- Ramin Halavati
- Vasilii Sukhanov
- Vishwa Kalubowila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Verne (Gerrit)
Nicholas Verne voted Code-Review+1![]( "Open in Gerrit")
- Achuith Bhandarkar
- Elaine Chien
- Mark Mentovai
- Vasilii Sukhanov
- Vishwa Kalubowila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Vishwa Kalubowila (Gerrit)
Message from Vishwa Kalubowila![]( "Open in Gerrit")
- Achuith Bhandarkar
- Alex Rudenko
- Dana Fried
- Danila Kuzmin
- Denis Kuznetsov
- Elaine Chien
- Erik Chen
- Greg Thompson
- Marijn Kruisselbrink
- Mark Mentovai
- Mark Pearson
- Nicholas Verne
- Rafał Godlewski
- Ramin Halavati
- Renato Silva
- Stefan Kuhne
- Vasilii Sukhanov
- Vishwa Kalubowila
- Xiyuan Xia
- Yury Khmel
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. |
Vishwa Kalubowila (Gerrit)
Vishwa Kalubowila added 1 comment![]( "Open in Gerrit")
Hi, I’ve fixed the merge conflicts and updated the patch set. Could you please take another look at the code when you have time? Thanks for your time and review!
Related details
- Achuith Bhandarkar
- Alex Rudenko
- Dana Fried
- Danila Kuzmin
- Denis Kuznetsov
- Elaine Chien
- Erik Chen
- Greg Thompson
- Marijn Kruisselbrink
- Mark Mentovai
- Mark Pearson
- Nicholas Verne
- Rafał Godlewski
- Ramin Halavati
- Renato Silva
- Stefan Kuhne
- Vasilii Sukhanov
- Xiyuan Xia
- Yury Khmel
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. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | -1 |
Erik ChenI'm not convinced I love this change because having the chrome:: prefix for functions that would otherwise be in the global namespace helps to differentiate them from e.g. member functions.
It makes the code somewhat less readable.
Xiyuan XiaI'm generally inclined to agree with Dana. However, this CL links a 12-year old bug that claims there was consensus to remove this, and there's a bunch of CLs to do so. I don't think this is a big deal either way, and I value consistency over my personal preferences for minor styling.
Greg Thompson+1
It is mixed for me too. It feels more clear to keep `AttemptUserExit` etc inside `chrome::` since it is a chrome browser specific function. `AttemptUserExit` is kind of general and not clear without context. And `chrome::` could provide that context that the exit is for chrome.
But again, I am on the fence. So people speak up.
Stefan KuhneAs I wrote in an earlier patch set: "It seems to me that the project's policies on namespace use have changed quite a bit since 2013, so I'm a bit surprised to see this effort getting traction now." I'm on the fence, as the use of "chrome::" was somewhat arbitrary. Going to the style guide, it says "With few exceptions, place code in a namespace." (https://google.github.io/styleguide/cppguide.html#Namespaces) If we want to go toward rather than away from that, we should consider what we would do if this were new code. Would we advocate `lifetime::` or even `browser::lifetime::` here?
Denis KuznetsovIn general I also think that removing ```chrome::``` is removing a bit of the clarity we had. So +1 for the mixed feelings of removing this, and if it would be me, I'd rather would keep it.
+1 to mixed feelings, I understand the reason for reducing noise, but in this case I am gravitating towards keeping the namespace (for reason Xiyuan mentioned)
@vishwa.k...@codimite.com: I think it's great that you want to contribute to Chromium. I don't feel that work on this `chrome` namespace bug advances Chromium enough to justify the time required by you to create changes like this or by Chromium OWNERS to review them. Any improvement in code quality from CLs for this issue is marginal at best while polluting git history. I am going to put a CR-1 on this change. If other top-level owners wish to override me, go right ahead; just please remove me from the R list.
I don't intend to discourage you from contributing to Chromium! If you wish to contribute, I think your time would be better spent working on a true bug in an area of the codebase that interests you. We welcome help in improving Chromium for our users' and fellow developers' sake.
Related details
- Achuith Bhandarkar
- Dana Fried
- Denis Kuznetsov
- Elaine Chien
- Erik Chen
- Marijn Kruisselbrink
- Mark Mentovai
- Mark Pearson
- Nicholas Verne
- Rafał Godlewski
- Ramin Halavati
- Renato Silva
- Stefan Kuhne
- Vasilii Sukhanov
- Vishwa Kalubowila
- Xiyuan Xia
- Yury Khmel
Code-Coverage
Code-Owners
Code-Review