Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7264019:
Readability review for gfeher (Closed)
|
Created:
9 years, 6 months ago by gfeher Modified:
9 years, 4 months ago CC:
chromium-reviews, Paweł Hajdan Jr., Mattias Nissler (ping if slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReadability review for gfeher
Testing infrastructure and tests for the cloud policy subsystem.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95606
Patch Set 1 : " #Patch Set 2 : rebase on CLs 7147015 and 7298012 #
Total comments: 15
Patch Set 3 : addressed comments #Patch Set 4 : changes from phajdan.jr's test refactoring CLs: 7524033, 7537033, 7465041 #Patch Set 5 : remove "virtual" from EventLogger's destructor #Patch Set 6 : remove trailing newlines (this causes unchanged files to disappear from this CL) #Patch Set 7 : rebase #
Messages
Total messages: 12 (0 generated)
Hello, I've added you as a reviewer to my readability changelist in the Chromium code base. I am looking forward to your comments. Thanks! Gábor
BTW, I'm going to be OOO from Wednesday and back on next Tuesday. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... File chrome/browser/policy/delayed_work_scheduler.cc (right): http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... chrome/browser/policy/delayed_work_scheduler.cc:32: callback_ = callback; Gah! This seems risky and is thread-unsafe in the obvious way. (1) What is the thread-safety of this class meant to be? (2) Why are you storing the callback in an instance variable and calling DoDelayedWork, instead of simply passing the closure to timer_.Start()? http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... File chrome/browser/policy/delayed_work_scheduler.h (right): http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... chrome/browser/policy/delayed_work_scheduler.h:29: base::Closure callback_; I'm confused by this. Callbacks are designed to be heap-allocated objects, and non-reusable ones are self-deleting. Do you really mean to have a Closure instead of a Closure* here, and pass a const Closure& to PostDelayedWork?! This seems highly nonstandard. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... File chrome/browser/policy/logging_work_scheduler.h (right): http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:37: // timestamps. Again, what's the thread safety? And is this class meant to be part of the API of this file? Seeing a bunch of helper classes before I know what this file is event meant to do is not particularly helpful. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:41: virtual ~EventLogger(); Is this meant to be subclassed? http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:55: static int CountEvents(const std::vector<int64>& events, google3 code is using namespace std. (Not sure if Chromium is as well) http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:59: class Task { Just declare Task here, and give the full class declaration inside the .cc file. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:118: explicit LoggingWorkScheduler(EventLogger* logger); Does the scheduler take ownership of the logger? Not clear. Ditto the thread safety.
Thanks for starting this. I've included some pointers to the Chromium code base and added more comments. I am going to be OOO from Friday and back on the 1st of August. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... File chrome/browser/policy/delayed_work_scheduler.cc (right): http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... chrome/browser/policy/delayed_work_scheduler.cc:32: callback_ = callback; On 2011/07/19 19:41:46, zunger wrote: > Gah! This seems risky and is thread-unsafe in the obvious way. > > (1) What is the thread-safety of this class meant to be? It should only interact with one thread, the so-called UI thread in Chromium. I'll document this. > (2) Why are you storing the callback in an instance variable and calling > DoDelayedWork, instead of simply passing the closure to timer_.Start()? timer_.Start() can not take closures as arguments. Please see: http://src.chromium.org/viewvc/chrome/trunk/src/base/timer.h?view=markup http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... File chrome/browser/policy/delayed_work_scheduler.h (right): http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delaye... chrome/browser/policy/delayed_work_scheduler.h:29: base::Closure callback_; Callbacks in the Chromium codebase seem to work differently. Please see: http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?view=markup In short, the comment in their header says that they should be stored by copy, and the copies take care of ref-counting the shared data they store. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... File chrome/browser/policy/logging_work_scheduler.h (right): http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:37: // timestamps. This is part of the public API. I've added some explanations at the beginning of this file. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:41: virtual ~EventLogger(); No. Shall I remove virtual then? http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:55: static int CountEvents(const std::vector<int64>& events, On 2011/07/19 19:41:46, zunger wrote: > google3 code is using namespace std. (Not sure if Chromium is as well) This is interesting because the C++ style guide seem to be forbidding "using namespace": http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... But anyways, I've grepped the Chromium source tree for "using namespace std" and there were only a very few matches, all from third-party libraries. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:59: class Task { On 2011/07/19 19:41:46, zunger wrote: > Just declare Task here, and give the full class declaration inside the .cc file. Done. http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:118: explicit LoggingWorkScheduler(EventLogger* logger); On 2011/07/19 19:41:46, zunger wrote: > Does the scheduler take ownership of the logger? Not clear. Ditto the thread > safety. Done. (I've only talked about thread safety at the beginning of this file.)
zunger: ping?
On Wed, Jul 20, 2011 at 09:36, <gfeher@chromium.org> wrote: > Thanks for starting this. I've included some pointers to the Chromium code > base > and added more comments. > > I am going to be OOO from Friday and back on the 1st of August. > > > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/delayed_work_**scheduler.cc<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delayed_work_scheduler.cc> > File chrome/browser/policy/delayed_**work_scheduler.cc (right): > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/delayed_work_**scheduler.cc#newcode32<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delayed_work_scheduler.cc#newcode32> > chrome/browser/policy/delayed_**work_scheduler.cc:32: callback_ = > callback; > On 2011/07/19 19:41:46, zunger wrote: > >> Gah! This seems risky and is thread-unsafe in the obvious way. >> > > (1) What is the thread-safety of this class meant to be? >> > It should only interact with one thread, the so-called UI thread in > Chromium. I'll document this. OK. > > > (2) Why are you storing the callback in an instance variable and >> > calling > >> DoDelayedWork, instead of simply passing the closure to >> > timer_.Start()? > > timer_.Start() can not take closures as arguments. > Please see: > http://src.chromium.org/**viewvc/chrome/trunk/src/base/** > timer.h?view=markup<http://src.chromium.org/viewvc/chrome/trunk/src/base/timer.h?view=markup> > > Ugh. Got it... but that's a lousy interface for the timer class. > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/delayed_work_**scheduler.h<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delayed_work_scheduler.h> > File chrome/browser/policy/delayed_**work_scheduler.h (right): > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/delayed_work_**scheduler.h#newcode29<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/delayed_work_scheduler.h#newcode29> > chrome/browser/policy/delayed_**work_scheduler.h:29: base::Closure > callback_; > Callbacks in the Chromium codebase seem to work differently. Please see: > http://src.chromium.org/**viewvc/chrome/trunk/src/base/** > callback.h?view=markup<http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?view=markup> > > In short, the comment in their header says that they should be stored by > copy, and the copies take care of ref-counting the shared data they > store. We should probably branch Chromium into a separate readability process... I'll branch that matter off separately. > > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h> > File chrome/browser/policy/logging_**work_scheduler.h (right): > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h#newcode37<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h#newcode37> > chrome/browser/policy/logging_**work_scheduler.h:37: // timestamps. > This is part of the public API. I've added some explanations at the > beginning of this file. > > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h#newcode41<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h#newcode41> > chrome/browser/policy/logging_**work_scheduler.h:41: virtual > ~EventLogger(); > No. Shall I remove virtual then? > > Yes, please. It gives the wrong signal to other users otherwise. > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h#newcode55<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h#newcode55> > chrome/browser/policy/logging_**work_scheduler.h:55: static int > CountEvents(const std::vector<int64>& events, > On 2011/07/19 19:41:46, zunger wrote: > >> google3 code is using namespace std. (Not sure if Chromium is as well) >> > > This is interesting because the C++ style guide seem to be forbidding > "using namespace": > http://google-styleguide.**googlecode.com/svn/trunk/** > cppguide.xml?showone=**Namespaces#Namespaces<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namespaces#Namespaces> > > It's a specific exception, in base/port.h since the earliest days of google1. > But anyways, I've grepped the Chromium source tree for "using namespace > std" and there were only a very few matches, all from third-party > libraries. > > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h#newcode59<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h#newcode59> > chrome/browser/policy/logging_**work_scheduler.h:59: class Task { > On 2011/07/19 19:41:46, zunger wrote: > >> Just declare Task here, and give the full class declaration inside the >> > .cc file. > > Done. > > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h#newcode118<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h#newcode118> > chrome/browser/policy/logging_**work_scheduler.h:118: explicit > LoggingWorkScheduler(**EventLogger* logger); > On 2011/07/19 19:41:46, zunger wrote: > >> Does the scheduler take ownership of the logger? Not clear. Ditto the >> > thread > >> safety. >> > > Done. (I've only talked about thread safety at the beginning of this > > file.) > > http://codereview.chromium.**org/7264019/<http://codereview.chromium.org/7264... >
Hi, I have removed the "virtual" keyword from EventLogger. Do you think we can proceed with this review, or shall we look for another reviewer who feels more comfortable with readability-reviewing Chromium code? Gabor p.s.: In other developments, I've found that the following global refactoring-CLs from Paweł had some effect on some of the files in this CL. I think the changes are quite small, but you can check them by looking at the delta between "Patch set 3" and "Patch set 4" in this CL. http://codereview.chromium.org/7524033 http://codereview.chromium.org/7537033 http://codereview.chromium.org/7465041 http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... File chrome/browser/policy/logging_work_scheduler.h (right): http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/loggin... chrome/browser/policy/logging_work_scheduler.h:41: virtual ~EventLogger(); On 2011/07/20 16:36:30, gfeher wrote: > No. Shall I remove virtual then? Done.
LGTM.
OK, LGTM. I'm not sure if there's some place I'm supposed to push an LG button in this tool -- let me know if there is. On Thu, Aug 4, 2011 at 03:26, <gfeher@chromium.org> wrote: > Hi, > > I have removed the "virtual" keyword from EventLogger. Do you think we can > proceed with this review, or shall we look for another reviewer who feels > more > comfortable with readability-reviewing Chromium code? > > Gabor > > p.s.: In other developments, I've found that the following global > refactoring-CLs from Paweł had some effect on some of the files in this CL. > I > think the changes are quite small, but you can check them by looking at the > delta between "Patch set 3" and "Patch set 4" in this CL. > http://codereview.chromium.**org/7524033<http://codereview.chromium.org/7524033> > http://codereview.chromium.**org/7537033<http://codereview.chromium.org/7537033> > http://codereview.chromium.**org/7465041<http://codereview.chromium.org/7465041> > > > > > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h> > File chrome/browser/policy/logging_**work_scheduler.h (right): > > http://codereview.chromium.**org/7264019/diff/4001/chrome/** > browser/policy/logging_work_**scheduler.h#newcode41<http://codereview.chromium.org/7264019/diff/4001/chrome/browser/policy/logging_work_scheduler.h#newcode41> > chrome/browser/policy/logging_**work_scheduler.h:41: virtual > ~EventLogger(); > On 2011/07/20 16:36:30, gfeher wrote: > >> No. Shall I remove virtual then? >> > > Done. > > http://codereview.chromium.**org/7264019/<http://codereview.chromium.org/7264... >
Thank you. Adding the letters LGTM makes any comment an LGTM. (Like this...) Julian: please owner-approve this change.
LGTM. Aren't you still in the owners list yourself? ;)
Commit queue patch verification failed without an error message. Something went wrong, probably a crash, a hickup or simply the monkeys went out for diner. Ping the relevant dude on a on-needed basis.
Change committed as 95606 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
