Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1038033002:
New Task Manager - Phase 1.1: Implement Browser Process Task Providing (Closed)
DescriptionNew Task Manager - Phase 1.1: Implement Browser Process Task Providing
R=nick@chromium.org
BUG=470956
TEST=unit_tests --gtest_filter=BrowserProcessTaskProviderTest.*
Committed: https://crrev.com/963710c4f9d8a593d8f78d33428a269b7f26e5e3
Cr-Commit-Position: refs/heads/master@{#323929}
Patch Set 1 #
Total comments: 57
Patch Set 2 : Addressing Nick's comments #
Total comments: 1
Patch Set 3 : Removed unconventional comments #
Total comments: 8
Patch Set 4 : Removed is_first_in_group, moved inherited methods to public: in BrowserProcessTask #Patch Set 5 : As agreed, made network_usage_ calculation accurate. #
Total comments: 71
Patch Set 6 : Addressing Lei Zhang's comments #
Total comments: 6
Patch Set 7 : Addressing more Lei's and Nick's comments #Patch Set 8 : Adding OWNERS file #Patch Set 9 : Converting size_t's to int64's #Messages
Total messages: 30 (5 generated)
Hi Nick, Kindly review. Thanks!
https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc:40: if (ResourceBundle::HasSharedInstance()) { Seems like there's no closing bracket for this (or rather, there is, but as a result the Windows version doesn't compile). https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc:48: default_icon->MakeThreadSafe(); The indentation here seems off. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc:67: // ---------------------------------------------------------------------- Not needed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc:84: // ---------------------------------------------------------------------- Not needed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.h:8: #include "chrome/browser/task_management/private/tasks_providers/task.h" Three additional levels of hierarchical organization seems excessive here. Can we figure out a way to do this with at most one additional level of directory structure? Spend a little time clicking around src/chrome/browser, and notice how flat things are on average. Let's try to be consistent with that. The goal of putting the implementation bits in a private/ directory, presumably, is to make it easier for readers of the code to identify the public-facing API and not waste time looking at the internals? That goal is achieved only if private/ is understood by convention to have that meaning ... and I do not think it does. Apart from the extensions private APIs (in which context "private" means a privilege granted only to trusted or built-in extensions), src/chrome has zero uses of "private" as a directory name like this. One convention we do have that helps us find the public entry point for a bunch of C++ files, in Google C++ style, is this: chrome/browser/foo_bar/foo_bar.h is the probably the class you'd start looking at. In truth, if you want to hide the guts, throwing them in a subdirectory does the job. The subdirectory doesn't need to announce "no visitors" by calling itself "private"; just give it a descriptive name. I.e., you don't need task_manager/private/intestines, just call it task_manager/intestines, and people will probably not be interested in looking at the intestines. ALSO: when thinking of directory and class names, I usually think it's helpful to limit repetitions of a single word. In the name of this class, for instance, "browser_process" appears twice and "task" appears three times. So maybe, could this be just .../task_management/providers/browser_process_task.h ? Something like that? https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.h:29: // ---------------------------------------------------------------------- In Google C++ style (and so, Chromium style) we don't really need a "// Fields:" block comment. The data members of a class always come last in the "private:" section, so you know to look for them there. If you want a comment here (e.g. for the sake of having vertical whitespace), a better choice is to comment the meaning of the data member, like: // The task representing the browser's one and only main process. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.cc:11: // ---------------------------------------------------------------------- Not needed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.cc:22: // ---------------------------------------------------------------------- Not needed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.h:26: void StopUpdating() override; I would not make these "private:". They are public methods of TaskProvider, so logically they are a part of this class's public interface. And practically, if I have a BrowserProcessTaskProvider*, I can upcast to the TaskProvider type, so I can call these methods whether or not you mark them private here. In my mind, marking a method private is making a promise that I only have to look for invocations in the corresponding .cc file, and in the .cc files of declared friends. But that's not true here. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.h:30: // ---------------------------------------------------------------------- Not needed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc:20: ~BrowserProcessTaskProviderTest() override {} FYI, You can safely omit this. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc:32: scoped_refptr<Task> provided_task_; FYI, though the style guide would prefer this to be private, protected data members in a test fixture are probably okay -- I see lots of instances: https://code.google.com/p/chromium/codesearch#search/&q=protected%5C:%5Cn.*%5... https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc:56: // process task. Not required, but a good practice to keep in mind: Oftentimes explanatory comments like this in tests are also good candidates for making error statements ... like so: EXPECT(...) << "Nonzero origin PID " << pid << " should not match the browser process task" https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/task.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:11: int64 one_second_time_delta = base::TimeDelta::FromSeconds(1).InSeconds(); This is a pretty fancy way of saying "int64 var = 1;" :) One of the reasons we have a TimeDelta class, is to lessen the risk of units-conversion bugs that crop up when you store time intervals in ints. I think you don't need this constant. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:48: void Task::Refresh(const base::TimeDelta& update_time) { I'd probably call this variable sampling_interval or update_duration, since it's an interval. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:56: network_usage_ = current_byte_count_ * (1 / update_time_secs); This math seems buggy. The else branch is executed if update_time_secs has the integer value 1 or 0, in which cases the result is identical to the if branch. What were you trying to do? Would you be better off using floating point, like: network_usage_ = static_cast<int>(current_byte_count / update_time.InSecondsF()); https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:68: // ---------------------------------------------------------------------- Remove this comment. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:77: icon_(icon), Just mentioning it in case you didn't know: copying ImageSkias like this is supposed to be very fast, since it'll just take a reference to the underlying pixel content. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:79: is_first_in_group_(true) { "is_first_in_group_" doesn't make sense to me as an explicit data member of a Task. Since Tasks that share a group have independent lifetimes, when the first task in a group goes away, the second one needs to be updated, so you'd need a mechanism (e.g. a list of tasks in a group) of finding the second one. And so, if you have such a mechanism, and want to know what the first task in the group is, why not just look at the list? https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:87: // ---------------------------------------------------------------------- Remove this comment. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/task.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:20: // represents one row in the task manager table. multiple tasks can share the multiple -> Multiple https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:22: // table. See |task_management::TaskGroup| which represents a process possibly TaskGroup doesn't exist right now. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:25: // A Task is RefCountedThreadSafe and hence refrences have to be scoped_refptr. "refrences" typo https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:26: class Task : public base::RefCountedThreadSafe<Task> { What lifetime problem does reference counting solve here? I am not yet convinced that tasks need to be RefCounted, let alone RefCountedThreadSafe. In the old TM the Resources didn't need to be RefCounted (the ResourceProviders were, and I'm also not sure they needed to be). Tasks, in my mind, ought to be owned either by the TaskProvider or else the TaskManager, and don't need to be thread safe. Refcounting extends the lifetime of an object until all owners go away. Because side-effects of destruction are very common in C++, this can lead to these side-effects being triggered at inconsistent and unexpected times, since the timing depends on the destruction order of the objects that own the refptr. And when you consider threading, most objects don't really want their dtors to run on threads other than the thread they're created on. A good chunk of the bugs I've encountered in Google Chrome had a situation like this as one of the aggravating factors. There is a place for refcounting, but I have come to prefer explicit ownership (scoped_ptr members in the owner, rawptr references on-thread, and WeakPtr references when bound to closures that will run async) whenever possible, just like I've come to prefer single-threaded objects. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:64: virtual blink::WebCache::ResourceTypeStats GetWebCacheStats() const; These merit a comment. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:83: protected: public:, protected: and private: declarations get a 1-space indent. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:93: // ---------------------------------------------------------------------- Not needed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:96: size_t task_id_; It is encouraged (but not required) to comment your data member declarations. Not all of these need it ... but here I'd say it's worth explaining what task_id_, means since often there are all kinds of int IDs floating around, and it's easy to confuse them. Also: I think this shouldn't be a size_t; it should be int or int64. There's no reason for it to behave differently on 32 bit vs. 64 bit builds. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:100: bool is_first_in_group_; These data members should all be private. Derived classes should use getters and setters as needed. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:103: static size_t last_id_; This approach (a static ID) means that tasks can only be minted on the UI thread, which might be further evidence that Task doesn't need to be RefCountedThreadSafe. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/task_provider.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task_provider.h:11: Merits a class comment. The comment before the old ResourceProvider class can be a starting point. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments In particular you should explain the contract between the provider and its observer, and the behaviors that concrete implementations of TaskProvider must implement. E.g.: when StartUpdating is called, the provider need to notify the observer of preexisting tasks? https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task_provider.h:34: // ---------------------------------------------------------------------- Not needed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task_provider.h:36: TaskProviderObserver* observer_; This should be private. The methods of the base class are the only ones that should be able to update the observer_ value.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc:40: if (ResourceBundle::HasSharedInstance()) { On 2015/03/27 18:16:20, ncarter wrote: > Seems like there's no closing bracket for this (or rather, there is, but as a > result the Windows version doesn't compile). Good catch! :) https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.cc:48: default_icon->MakeThreadSafe(); On 2015/03/27 18:16:20, ncarter wrote: > The indentation here seems off. Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.h:8: #include "chrome/browser/task_management/private/tasks_providers/task.h" On 2015/03/27 18:16:20, ncarter wrote: > Three additional levels of hierarchical organization seems excessive here. Can > we figure out a way to do this with at most one additional level of directory > structure? Spend a little time clicking around src/chrome/browser, and notice > how flat things are on average. Let's try to be consistent with that. > > The goal of putting the implementation bits in a private/ directory, presumably, > is to make it easier for readers of the code to identify the public-facing API > and not waste time looking at the internals? That goal is achieved only if > private/ is understood by convention to have that meaning ... and I do not think > it does. Apart from the extensions private APIs (in which context "private" > means a privilege granted only to trusted or built-in extensions), src/chrome > has zero uses of "private" as a directory name like this. > > One convention we do have that helps us find the public entry point for a bunch > of C++ files, in Google C++ style, is this: chrome/browser/foo_bar/foo_bar.h is > the probably the class you'd start looking at. > > In truth, if you want to hide the guts, throwing them in a subdirectory does the > job. The subdirectory doesn't need to announce "no visitors" by calling itself > "private"; just give it a descriptive name. I.e., you don't need > task_manager/private/intestines, just call it task_manager/intestines, and > people will probably not be interested in looking at the intestines. > > ALSO: when thinking of directory and class names, I usually think it's helpful > to limit repetitions of a single word. In the name of this class, for instance, > "browser_process" appears twice and "task" appears three times. > > So maybe, could this be just > .../task_management/providers/browser_process_task.h ? Something like that? Modified as agreed! https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task.h:29: // ---------------------------------------------------------------------- On 2015/03/27 18:16:20, ncarter wrote: > In Google C++ style (and so, Chromium style) we don't really need a "// Fields:" > block comment. The data members of a class always come last in the "private:" > section, so you know to look for them there. > > If you want a comment here (e.g. for the sake of having vertical whitespace), a > better choice is to comment the meaning of the data member, like: > > // The task representing the browser's one and only main process. Removed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.h:26: void StopUpdating() override; On 2015/03/27 18:16:21, ncarter wrote: > I would not make these "private:". They are public methods of TaskProvider, so > logically they are a part of this class's public interface. And practically, if > I have a BrowserProcessTaskProvider*, I can upcast to the TaskProvider type, so > I can call these methods whether or not you mark them private here. > > In my mind, marking a method private is making a promise that I only have to > look for invocations in the corresponding .cc file, and in the .cc files of > declared friends. But that's not true here. But they weren't public in the task provider in the previous patch. They were protected. In this new patch however I made them private in both TaskProvider and BrowserProcessTaskProvider. By the way, it's a convention that I see followed here a lot. for example take a look at views::View and ash::ActionableView which inherits from it. You'll see overridden public methods from View are taken protected in ActionableView. There are many other examples like this. However your comment is convincing. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc:20: ~BrowserProcessTaskProviderTest() override {} On 2015/03/27 18:16:21, ncarter wrote: > FYI, You can safely omit this. Yep, I know, thanks! :) https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc:56: // process task. On 2015/03/27 18:16:21, ncarter wrote: > Not required, but a good practice to keep in mind: Oftentimes explanatory > comments like this in tests are also good candidates for making error statements > ... like so: > > EXPECT(...) << "Nonzero origin PID " << pid << " should not match the browser > process task" Removed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/task.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:11: int64 one_second_time_delta = base::TimeDelta::FromSeconds(1).InSeconds(); On 2015/03/27 18:16:21, ncarter wrote: > This is a pretty fancy way of saying "int64 var = 1;" :) > > One of the reasons we have a TimeDelta class, is to lessen the risk of > units-conversion bugs that crop up when you store time intervals in ints. I > think you don't need this constant. Removed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:48: void Task::Refresh(const base::TimeDelta& update_time) { On 2015/03/27 18:16:21, ncarter wrote: > I'd probably call this variable sampling_interval or update_duration, since it's > an interval. Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:56: network_usage_ = current_byte_count_ * (1 / update_time_secs); On 2015/03/27 18:16:21, ncarter wrote: > This math seems buggy. The else branch is executed if update_time_secs has the > integer value 1 or 0, in which cases the result is identical to the if branch. > What were you trying to do? > > Would you be better off using floating point, like: > > network_usage_ = static_cast<int>(current_byte_count / > update_time.InSecondsF()); > Fixed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:77: icon_(icon), On 2015/03/27 18:16:21, ncarter wrote: > Just mentioning it in case you didn't know: copying ImageSkias like this is > supposed to be very fast, since it'll just take a reference to the underlying > pixel content. Yep, I already know that, but thanks a lot for making sure! :) https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.cc:79: is_first_in_group_(true) { On 2015/03/27 18:16:21, ncarter wrote: > "is_first_in_group_" doesn't make sense to me as an explicit data member of a > Task. Since Tasks that share a group have independent lifetimes, when the first > task in a group goes away, the second one needs to be updated, so you'd need a > mechanism (e.g. a list of tasks in a group) of finding the second one. And so, > if you have such a mechanism, and want to know what the first task in the group > is, why not just look at the list? This is the responsibility of the TaskGroup in the future. Upon addition / removal of tasks from their group this field will be updated properly. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/task.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:20: // represents one row in the task manager table. multiple tasks can share the On 2015/03/27 18:16:21, ncarter wrote: > multiple -> Multiple Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:22: // table. See |task_management::TaskGroup| which represents a process possibly On 2015/03/27 18:16:22, ncarter wrote: > TaskGroup doesn't exist right now. I know but it will. Do you want me to remove that comment until it does? https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:25: // A Task is RefCountedThreadSafe and hence refrences have to be scoped_refptr. On 2015/03/27 18:16:22, ncarter wrote: > "refrences" typo Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:26: class Task : public base::RefCountedThreadSafe<Task> { On 2015/03/27 18:16:21, ncarter wrote: > What lifetime problem does reference counting solve here? > > I am not yet convinced that tasks need to be RefCounted, let alone > RefCountedThreadSafe. In the old TM the Resources didn't need to be RefCounted > (the ResourceProviders were, and I'm also not sure they needed to be). > > Tasks, in my mind, ought to be owned either by the TaskProvider or else the > TaskManager, and don't need to be thread safe. > > Refcounting extends the lifetime of an object until all owners go away. Because > side-effects of destruction are very common in C++, this can lead to these > side-effects being triggered at inconsistent and unexpected times, since the > timing depends on the destruction order of the objects that own the refptr. And > when you consider threading, most objects don't really want their dtors to run > on threads other than the thread they're created on. A good chunk of the bugs > I've encountered in Google Chrome had a situation like this as one of the > aggravating factors. There is a place for refcounting, but I have come to prefer > explicit ownership (scoped_ptr members in the owner, rawptr references > on-thread, and WeakPtr references when bound to closures that will run async) > whenever possible, just like I've come to prefer single-threaded objects. Removed as agreed. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:64: virtual blink::WebCache::ResourceTypeStats GetWebCacheStats() const; On 2015/03/27 18:16:22, ncarter wrote: > These merit a comment. Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:83: protected: On 2015/03/27 18:16:21, ncarter wrote: > public:, protected: and private: declarations get a 1-space indent. > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Yes I know that. I'm not sure why they weren't formated properly. Thanks! Done! https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task.h:100: bool is_first_in_group_; On 2015/03/27 18:16:21, ncarter wrote: > These data members should all be private. Derived classes should use getters and > setters as needed. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/task_provider.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task_provider.h:11: On 2015/03/27 18:16:22, ncarter wrote: > Merits a class comment. The comment before the old ResourceProvider class can be > a starting point. > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments > > In particular you should explain the contract between the provider and its > observer, and the behaviors that concrete implementations of TaskProvider must > implement. E.g.: when StartUpdating is called, the provider need to notify the > observer of preexisting tasks? Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task_provider.h:34: // ---------------------------------------------------------------------- On 2015/03/27 18:16:22, ncarter wrote: > Not needed. Done. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/task_provider.h:36: TaskProviderObserver* observer_; On 2015/03/27 18:16:22, ncarter wrote: > This should be private. The methods of the base class are the only ones that > should be able to update the observer_ value. Done.
https://codereview.chromium.org/1038033002/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/browser_process/browser_process_task.cc (right): https://codereview.chromium.org/1038033002/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/browser_process/browser_process_task.cc:67: // ---------------------------------------------------------------------- I showed this .cc file to two top-level content/ owners (who do a lot of reviews), and they independently took issue with these comments. One said "this is simply not Chromium style." Please remove these. Sorry.
Removed the unconventional comments.
https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.h (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/browser_process_task_provider.h:26: void StopUpdating() override; On 2015/03/31 00:19:19, afakhry wrote: > On 2015/03/27 18:16:21, ncarter wrote: > > I would not make these "private:". They are public methods of TaskProvider, so > > logically they are a part of this class's public interface. And practically, > if > > I have a BrowserProcessTaskProvider*, I can upcast to the TaskProvider type, > so > > I can call these methods whether or not you mark them private here. > > > > In my mind, marking a method private is making a promise that I only have to > > look for invocations in the corresponding .cc file, and in the .cc files of > > declared friends. But that's not true here. > > But they weren't public in the task provider in the previous patch. They were > protected. In this new patch however I made them private in both TaskProvider > and BrowserProcessTaskProvider. > > By the way, it's a convention that I see followed here a lot. for example take a > look at views::View and ash::ActionableView which inherits from it. You'll see > overridden public methods from View are taken protected in ActionableView. There > are many other examples like this. However your comment is convincing. Apologies, I think I was confusing the inheritance story of Task with that of TaskProvider. This comment was meant to be about the sort of situation that exists with Task/BrowserProcessTask, where public methods in the base class change to private. What's going on with TaskProvider is okay in the most recent patch. And I've added a comment on BrowserProcessTask with my concerns there. https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... File chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc (right): https://codereview.chromium.org/1038033002/diff/1/chrome/browser/task_managem... chrome/browser/task_management/private/tasks_providers/browser_process/tests/browser_process_task_unittest.cc:56: // process task. On 2015/03/31 00:19:19, afakhry wrote: > On 2015/03/27 18:16:21, ncarter wrote: > > Not required, but a good practice to keep in mind: Oftentimes explanatory > > comments like this in tests are also good candidates for making error > statements > > ... like so: > > > > EXPECT(...) << "Nonzero origin PID " << pid << " should not match the browser > > process task" > > Removed. I hope the removal wasn't due to my comment? To be clear, I wasn't saying "comments bad", I was saying "comments good, sometimes you can upgrade comments to printfs, your call.". https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/browser_process/browser_process_task.h (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/browser_process/browser_process_task.h:25: void Refresh(const base::TimeDelta& update_time) override; Make these public, since they're public in the base class (I made this comment on the previous patch, but directed it at the wrong file). https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/task.cc (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/task.cc:58: network_usage_ = current_byte_count_ / update_time_secs; I really think using a floating point divide here (/ InSecondsF()) is appropriate. For a 1.5 second duration this computation is needlessly skewed. For a 0.2 second duration it's a needless divide by zero. Not sure quite what was up with the original code, but maybe (a) it was trying to do something like a 1-second windowed moving average for shorter update_durations and just failed, or maybe (b) it just had a latent bug that we never hit because the duration only took on a value of 1 second or 2 seconds exactly... https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/task.h:85: void set_is_first_in_group(bool val) { is_first_in_group_ = val; } Representing "first in group" as a bool member of Task seems like a suboptimal choice to me, but we can't productively talk about alternative approaches since the group code isn't under review yet. That puts me in a bit of a bind as a reviewer. Could you just remove these lines from the patch, and add them back in during a later patch set, when they're needed by some caller? Then we'd be able to have a real discussion.
https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/task.cc (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/task.cc:58: network_usage_ = current_byte_count_ / update_time_secs; On 2015/03/31 22:31:32, ncarter wrote: > I really think using a floating point divide here (/ InSecondsF()) is > appropriate. For a 1.5 second duration this computation is needlessly skewed. > For a 0.2 second duration it's a needless divide by zero. > > Not sure quite what was up with the original code, but maybe (a) it was trying > to do something like a 1-second windowed moving average for shorter > update_durations and just failed, or maybe (b) it just had a latent bug that we > never hit because the duration only took on a value of 1 second or 2 seconds > exactly... I don't think that this floating point operation is really needed. There's no need to handle a situation that will never occur. Our |network_usage_| is measured in bytes per seconds and it's an int64. Our update duration will be given from that TaskManager which will get them based on his observers needs, and the API for providing observers is planned to be: TaskManager::AddObserver(TaskManagerObserver* observer, int desired_update_interval, int enabled_refresh_flags); So it's going to be an 'int' as well. I don't think we'll ever need to have an update interval of 1.2 seconds, right?
https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/browser_process/browser_process_task.h (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/browser_process/browser_process_task.h:25: void Refresh(const base::TimeDelta& update_time) override; On 2015/03/31 22:31:32, ncarter wrote: > Make these public, since they're public in the base class (I made this comment > on the previous patch, but directed it at the wrong file). Done. https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/task.cc (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/task.cc:58: network_usage_ = current_byte_count_ / update_time_secs; On 2015/03/31 23:22:09, afakhry wrote: > On 2015/03/31 22:31:32, ncarter wrote: > > I really think using a floating point divide here (/ InSecondsF()) is > > appropriate. For a 1.5 second duration this computation is needlessly skewed. > > For a 0.2 second duration it's a needless divide by zero. > > > > Not sure quite what was up with the original code, but maybe (a) it was trying > > to do something like a 1-second windowed moving average for shorter > > update_durations and just failed, or maybe (b) it just had a latent bug that > we > > never hit because the duration only took on a value of 1 second or 2 seconds > > exactly... > > I don't think that this floating point operation is really needed. There's no > need to handle a situation that will never occur. Our |network_usage_| is > measured in bytes per seconds and it's an int64. Our update duration will be > given from that TaskManager which will get them based on his observers needs, > and the API for providing observers is planned to be: > > TaskManager::AddObserver(TaskManagerObserver* observer, > int desired_update_interval, > int enabled_refresh_flags); > > So it's going to be an 'int' as well. I don't think we'll ever need to have an > update interval of 1.2 seconds, right? We probably might not even need a TimeDelta object to achieve this. https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/task.h:85: void set_is_first_in_group(bool val) { is_first_in_group_ = val; } On 2015/03/31 22:31:32, ncarter wrote: > Representing "first in group" as a bool member of Task seems like a suboptimal > choice to me, but we can't productively talk about alternative approaches since > the group code isn't under review yet. That puts me in a bit of a bind as a > reviewer. > > Could you just remove these lines from the patch, and add them back in during a > later patch set, when they're needed by some caller? Then we'd be able to have a > real discussion. Done.
https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/task.cc (right): https://codereview.chromium.org/1038033002/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/task.cc:58: network_usage_ = current_byte_count_ / update_time_secs; On 2015/03/31 23:44:14, afakhry wrote: > On 2015/03/31 23:22:09, afakhry wrote: > > On 2015/03/31 22:31:32, ncarter wrote: > > > I really think using a floating point divide here (/ InSecondsF()) is > > > appropriate. For a 1.5 second duration this computation is needlessly > skewed. > > > For a 0.2 second duration it's a needless divide by zero. > > > > > > Not sure quite what was up with the original code, but maybe (a) it was > trying > > > to do something like a 1-second windowed moving average for shorter > > > update_durations and just failed, or maybe (b) it just had a latent bug that > > we > > > never hit because the duration only took on a value of 1 second or 2 seconds > > > exactly... > > > > I don't think that this floating point operation is really needed. There's no > > need to handle a situation that will never occur. Our |network_usage_| is > > measured in bytes per seconds and it's an int64. Our update duration will be > > given from that TaskManager which will get them based on his observers needs, > > and the API for providing observers is planned to be: > > > > TaskManager::AddObserver(TaskManagerObserver* observer, > > int desired_update_interval, > > int enabled_refresh_flags); > > > > So it's going to be an 'int' as well. I don't think we'll ever need to have an > > update interval of 1.2 seconds, right? > > We probably might not even need a TimeDelta object to achieve this. What downside do you see to using InSecondsF()? Why not make this code compute the average correctly for the range of possible input values, and make it future-proof? There's a broad convention of using TimeDeltas instead of ints to represent time ( http://crbug.com/108171 ). That would apply to the second argument of TaskManager::AddObserver too -- in this example the argument would be that AddObserver(this, TimeDelta::FromSeconds(1), FLAGS) is going to be more readable than AddObserver(this, 1, FLAGS) -- it's clear that the 1 is in seconds, not milliseconds or microseconds. As for maybe not needing a TimeDelta -- TimeDelta is essentially free, it's just an int64, really.
As agreed on IM, made the network_usage_ calculation accurate.
lgtm
afakhry@chromium.org changed reviewers: + thestig@chromium.org
@thestig: Could you please give us an OWNERS review? Thanks!
https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task.cc (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task.cc:29: gfx::ImageSkia* default_icon = nullptr; nit: |g_default_icon| https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task.cc:58: return command_line && I don't think anyone checks the ForCurrentProcess() return value for NULL. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task.cc:108: used_sqlite_memory_ = static_cast<size_t>(sqlite3_memory_used()); I don't see sqlite3_memory_used() implemented anywhere. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task_provider.cc (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_provider.cc:10: : browser_process_task_() { I don't think this is needed? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_provider.cc:19: if (origin_pid == 0 && child_id == -1) Since I don't understand what the parameters to GetTaskOfUrlRequest() are exactly, I also don't understand why these magic values designate the browser process. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h:21: Task* GetTaskOfUrlRequest(int origin_pid, Unless someone needs to call BrowserProcessTaskProvider::GetTaskOfUrlRequest() directly, this can be private too. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:29: NOTIFICATION, /* A notification process. */ Is this a thing? I'm just curious what it is. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:40: Task(const base::string16& title, Document parameters. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:41: const gfx::ImageSkia& icon, nit: indentation. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:48: // This is the unique ID of the BrowserChildProcessHost/RenderProcessHost. It What does this provide over process_handle() ? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:50: virtual int GetChildProcessUniqueID() const = 0; Is the browser a "child" process? Are all return values valid? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:52: virtual base::string16 GetProfileName() const; Can you clarify what "profile" means in this case? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:52: virtual base::string16 GetProfileName() const; By making this and the following virtual methods const, you are restricting what implementations can do. For example, if getting the profile name requires an one time expensive operation, and the value can be cached for later queries, that's not possibly if the method is const. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:54: virtual int GetRoutingID() const; What is the "routing id" exactly? There are several classes that all hand out routing ids, but they are not interchangable. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:57: // memory, in this case a default invalid value of -1U will be returned. Why can't the invalid value just be 0? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:58: virtual size_t GetSqliteMemoryUsed() const; Why do you have ReportsWebCacheStats(), but no ReportsSqliteMemoryUsed() ? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:70: // Will be called to let the task refresh itself between refresh cycles. Can you put Refresh() and OnBytesRead() on top? Right now it's a section of getters, these methods, and more getters. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:77: void OnBytesRead(int64 bytes_read); Maybe OnNetworkBytesRead() ? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:79: int64 network_usage() const { return network_usage_; } You should document these too. For example, is this return value in bytes? Does -1 indicate an error? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:81: base::string16 title() const { return title_; } You can return a const base::string16& for the non-POD return types. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:86: static int64 last_id_; Just put this in an anonymous namespace in the .cc file as |g_last_id|, unless you need some friendly test to be able to access this variable directly. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:92: base::string16 title_; These never change, make them const? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider.cc (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.cc:30: DCHECK(observer_); There's no real need to DCHECK a pointer right before dereferencing it. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.h:24: virtual Task* GetTaskOfUrlRequest(int origin_pid, I looked in net/url_request/url_request.h and I still don't understand what these parameters are. URLRequest doesn't refer to "pid" or "child" or "route" at all. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.h:28: void SetObserver(TaskProviderObserver* observer); Given the impl, shouldn't this state: - it's invalid to call SetObserver() when there's already an observer - ClearObserver() cannot be called when there's no observer https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider_observer.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider_observer.h:19: virtual void TaskRemoved(Task* task) = 0; Is |task| still valid when given to the observer? You may want to document pure virtual methods, even if they appear obvious. https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:2847: 'browser/task_management/providers/browser_process/browser_process_task_provider.cc', Do you need to have a browser_process sub-directory, and files within it named browser_process_foo? Maybe just drop the sub-directory? ls chrome/browser|wc -l returns 349 and ls chrome/browser/extensions|wc -l return 519 and nobody is crying yet. (AFAIK)
Addressing Lei Zhang's comments. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task.cc (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task.cc:29: gfx::ImageSkia* default_icon = nullptr; On 2015/04/01 22:47:50, Lei Zhang wrote: > nit: |g_default_icon| Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task.cc:58: return command_line && On 2015/04/01 22:47:50, Lei Zhang wrote: > I don't think anyone checks the ForCurrentProcess() return value for NULL. Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task.cc:108: used_sqlite_memory_ = static_cast<size_t>(sqlite3_memory_used()); On 2015/04/01 22:47:50, Lei Zhang wrote: > I don't see sqlite3_memory_used() implemented anywhere. It's implemented in 'src/third_party/sqlite/amalgamation/sqlite3.c'. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task_provider.cc (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_provider.cc:10: : browser_process_task_() { On 2015/04/01 22:47:50, Lei Zhang wrote: > I don't think this is needed? It's not needed, but it's good practice, and better documentation. It also helps indexers like that of Eclipse to find where that object was created. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_provider.cc:19: if (origin_pid == 0 && child_id == -1) On 2015/04/01 22:47:50, Lei Zhang wrote: > Since I don't understand what the parameters to GetTaskOfUrlRequest() are > exactly, I also don't understand why these magic values designate the browser > process. I added detailed comments to this function's declaration in the base class. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h:21: Task* GetTaskOfUrlRequest(int origin_pid, On 2015/04/01 22:47:50, Lei Zhang wrote: > Unless someone needs to call BrowserProcessTaskProvider::GetTaskOfUrlRequest() > directly, this can be private too. This is one of the discussions I had with ncarter@ about whether to keep it public as in the base class or to hide it here. We agreed that as long as it's public in the base class then we keep it public in the derived, since it's possible to something like: Base* base = static_cast<Base*>(derived*); base->PublicInBase(); https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:29: NOTIFICATION, /* A notification process. */ On 2015/04/01 22:47:51, Lei Zhang wrote: > Is this a thing? I'm just curious what it is. I think ncarter@ can answer this question better than me. This was in the old task manager and was taken as is. I've never seen a notification process. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:40: Task(const base::string16& title, On 2015/04/01 22:47:51, Lei Zhang wrote: > Document parameters. Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:41: const gfx::ImageSkia& icon, On 2015/04/01 22:47:50, Lei Zhang wrote: > nit: indentation. Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:48: // This is the unique ID of the BrowserChildProcessHost/RenderProcessHost. It On 2015/04/01 22:47:51, Lei Zhang wrote: > What does this provide over process_handle() ? It's totally different. As the comment says it's not the PID nor the process handle. It's the unique ID of the process host instance. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:50: virtual int GetChildProcessUniqueID() const = 0; On 2015/04/01 22:47:51, Lei Zhang wrote: > Is the browser a "child" process? Are all return values valid? In the case of the browser process, the return value is 0 since there's no browser process host. But for renderers and other child processes which they have hosts in the browser process with valid unique IDs we return those. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:52: virtual base::string16 GetProfileName() const; On 2015/04/01 22:47:50, Lei Zhang wrote: > By making this and the following virtual methods const, you are restricting what > implementations can do. For example, if getting the profile name requires an one > time expensive operation, and the value can be cached for later queries, that's > not possibly if the method is const. I understand, in this case however this will only be set once upon the construction of a renderer task and it will never change afterwards. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:52: virtual base::string16 GetProfileName() const; On 2015/04/01 22:47:51, Lei Zhang wrote: > Can you clarify what "profile" means in this case? Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:54: virtual int GetRoutingID() const; On 2015/04/01 22:47:51, Lei Zhang wrote: > What is the "routing id" exactly? There are several classes that all hand out > routing ids, but they are not interchangable. It's the routing ID of the render view host if this task represents a renderer, otherwise it will return 0. A comment has been added. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:57: // memory, in this case a default invalid value of -1U will be returned. On 2015/04/01 22:47:51, Lei Zhang wrote: > Why can't the invalid value just be 0? 0 is a valid value for memory usage. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:58: virtual size_t GetSqliteMemoryUsed() const; On 2015/04/01 22:47:51, Lei Zhang wrote: > Why do you have ReportsWebCacheStats(), but no ReportsSqliteMemoryUsed() ? In SqliteMemory we augmented this check in the return value by checking for -1U. We can't do this however in WebCache stats since it's a large object with many fields. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:70: // Will be called to let the task refresh itself between refresh cycles. On 2015/04/01 22:47:51, Lei Zhang wrote: > Can you put Refresh() and OnBytesRead() on top? Right now it's a section of > getters, these methods, and more getters. Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:77: void OnBytesRead(int64 bytes_read); On 2015/04/01 22:47:50, Lei Zhang wrote: > Maybe OnNetworkBytesRead() ? Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:79: int64 network_usage() const { return network_usage_; } On 2015/04/01 22:47:50, Lei Zhang wrote: > You should document these too. For example, is this return value in bytes? Does > -1 indicate an error? I added comments to the fields which they get. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:81: base::string16 title() const { return title_; } On 2015/04/01 22:47:50, Lei Zhang wrote: > You can return a const base::string16& for the non-POD return types. Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:86: static int64 last_id_; On 2015/04/01 22:47:51, Lei Zhang wrote: > Just put this in an anonymous namespace in the .cc file as |g_last_id|, unless > you need some friendly test to be able to access this variable directly. Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:92: base::string16 title_; On 2015/04/01 22:47:51, Lei Zhang wrote: > These never change, make them const? Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider.cc (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.cc:30: DCHECK(observer_); On 2015/04/01 22:47:51, Lei Zhang wrote: > There's no real need to DCHECK a pointer right before dereferencing it. Yes but it would provide better debugging clues as to what went wrong as opposed to just getting a SIGSEGV. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.h:24: virtual Task* GetTaskOfUrlRequest(int origin_pid, On 2015/04/01 22:47:51, Lei Zhang wrote: > I looked in net/url_request/url_request.h and I still don't understand what > these parameters are. URLRequest doesn't refer to "pid" or "child" or "route" at > all. I agree it's confusing. You need to look at content::ResourceRequestInfo. I added more comments to make it clearer. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.h:28: void SetObserver(TaskProviderObserver* observer); On 2015/04/01 22:47:51, Lei Zhang wrote: > Given the impl, shouldn't this state: > > - it's invalid to call SetObserver() when there's already an observer > - ClearObserver() cannot be called when there's no observer Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider_observer.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider_observer.h:19: virtual void TaskRemoved(Task* task) = 0; On 2015/04/01 22:47:51, Lei Zhang wrote: > Is |task| still valid when given to the observer? You may want to document pure > virtual methods, even if they appear obvious. You're right. I added more documentation. https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:2847: 'browser/task_management/providers/browser_process/browser_process_task_provider.cc', On 2015/04/01 22:47:52, Lei Zhang wrote: > Do you need to have a browser_process sub-directory, and files within it named > browser_process_foo? Maybe just drop the sub-directory? > > ls chrome/browser|wc -l returns 349 > and > ls chrome/browser/extensions|wc -l return 519 > > and nobody is crying yet. (AFAIK) The idea was to keep the private interfaces in the top directory and related implementation files are grouped together in sub-directories.
https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:48: // This is the unique ID of the BrowserChildProcessHost/RenderProcessHost. It On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:51, Lei Zhang wrote: > > What does this provide over process_handle() ? > > It's totally different. As the comment says it's not the PID nor the process > handle. > > It's the unique ID of the process host instance. Is the process host to not a 1:1 mapping with the process handle, with some easy way to look up one from the other? I just don't understand how this will be used, so I'm wondering why it's needed. Is it just here because we need this info often so it's provided for convenience? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:50: virtual int GetChildProcessUniqueID() const = 0; On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:51, Lei Zhang wrote: > > Is the browser a "child" process? Are all return values valid? > > In the case of the browser process, the return value is 0 since there's no > browser process host. > > But for renderers and other child processes which they have hosts in the browser > process with valid unique IDs we return those. Can you document this in the comments? What is the valid range for IDs? Is 0 valid? -1? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:58: virtual size_t GetSqliteMemoryUsed() const; On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:51, Lei Zhang wrote: > > Why do you have ReportsWebCacheStats(), but no ReportsSqliteMemoryUsed() ? > > In SqliteMemory we augmented this check in the return value by checking for -1U. > > We can't do this however in WebCache stats since it's a large object with many > fields. So why not do ReportsFoo() / GetFoo() for all types for consistency, and forgo the sentinel value? Then the caller code can look like: if (ReportsFoo()) foo = GetFoo() if (ReportsBar()) bar = GetBar() instead of: if (ReportsFoo()) foo = GetFoo() bar = GetBar() if (bar == -1) // handle invalid case https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:81: base::string16 title() const { return title_; } On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:50, Lei Zhang wrote: > > You can return a const base::string16& for the non-POD return types. > > Done. Same goes for icon() and process_handle(). https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:92: base::string16 title_; On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:51, Lei Zhang wrote: > > These never change, make them const? > > Done. Can |title_| and |icon_| ever change? https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.h:24: virtual Task* GetTaskOfUrlRequest(int origin_pid, On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:51, Lei Zhang wrote: > > I looked in net/url_request/url_request.h and I still don't understand what > > these parameters are. URLRequest doesn't refer to "pid" or "child" or "route" > at > > all. > > I agree it's confusing. You need to look at content::ResourceRequestInfo. > > I added more comments to make it clearer. Would it make sense to just say "the task associated with a ResourceRequestInfo" and let the reader figure out ResourceRequestInfo maps to a given URLRequest. Is it very important to mention URLRequest here? https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:2847: 'browser/task_management/providers/browser_process/browser_process_task_provider.cc', On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:52, Lei Zhang wrote: > > Do you need to have a browser_process sub-directory, and files within it named > > browser_process_foo? Maybe just drop the sub-directory? > > > > ls chrome/browser|wc -l returns 349 > > and > > ls chrome/browser/extensions|wc -l return 519 > > > > and nobody is crying yet. (AFAIK) > > The idea was to keep the private interfaces in the top directory and related > implementation files are grouped together in sub-directories. OTOH, #include "chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h" doesn't fit within 80 columns. I think you are trying way too hard to give your files hierarchy. how about chrome/browser/task_management/providers/browser_process_task_provider_impl.h? One can easily spot the interface files as the shorter named files without _impl in their names. https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider_observer.h (right): https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider_observer.h:23: // the observer and and references to it must not be kept. typo: and and
https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task_unittest.cc (right): https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_unittest.cc:28: EXPECT_FALSE(true); Just call FAIL() here. https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_unittest.cc:66: EXPECT_NE(nullptr, provided_task_); This should be ASSERT and not EXPECT. If this fails, the entire unit_tests process crashes.
https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:29: NOTIFICATION, /* A notification process. */ On 2015/04/03 01:51:01, afakhry wrote: > On 2015/04/01 22:47:51, Lei Zhang wrote: > > Is this a thing? I'm just curious what it is. > > I think ncarter@ can answer this question better than me. This was in the old > task manager and was taken as is. I've never seen a notification process. We can remove this. jam@ deleted the balloon notification code about a year ago; we just missed this spot. [ see https://codereview.chromium.org/231723006 ]
https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:92: base::string16 title_; On 2015/04/03 02:39:37, Lei Zhang wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:51, Lei Zhang wrote: > > > These never change, make them const? > > > > Done. > > Can |title_| and |icon_| ever change? Probably. A page can change its title (or favicon) and these definitely ought to show up in the taskmanager. Historically we did that via tearing down and recreating the Task, but afaik that was out of laziness. https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:2847: 'browser/task_management/providers/browser_process/browser_process_task_provider.cc', On 2015/04/03 02:39:37, Lei Zhang wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:52, Lei Zhang wrote: > > > Do you need to have a browser_process sub-directory, and files within it > named > > > browser_process_foo? Maybe just drop the sub-directory? > > > > > > ls chrome/browser|wc -l returns 349 > > > and > > > ls chrome/browser/extensions|wc -l return 519 > > > > > > and nobody is crying yet. (AFAIK) > > > > The idea was to keep the private interfaces in the top directory and related > > implementation files are grouped together in sub-directories. > > OTOH, #include > "chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h" > doesn't fit within 80 columns. I think you are trying way too hard to give your > files hierarchy. > > how about > chrome/browser/task_management/providers/browser_process_task_provider_impl.h? > One can easily spot the interface files as the shorter named files without _impl > in their names. FWIW, no levels of hierarchy after providers/ was my suggestion as well earlier in the review. I still would prefer at most one level of directory hierarchy, because the convention in chrome seems to be a relatively flat organization. I'm not sure I like the _impl pattern that Lei suggests. Usually when we have foo_impl.cc it's the sole non-test implementation of the interface foo.h. In this case we have many implementations of the taskprovider and task interfaces; seems clumsy to me to call them all 'Impls. I'm not bothered by the idea of task.h living alongside browser_process_task.h and web_contents_task.h.
https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:29: NOTIFICATION, /* A notification process. */ On 2015/04/03 03:32:00, ncarter wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:51, Lei Zhang wrote: > > > Is this a thing? I'm just curious what it is. > > > > I think ncarter@ can answer this question better than me. This was in the old > > task manager and was taken as is. I've never seen a notification process. > > We can remove this. jam@ deleted the balloon notification code about a year ago; > we just missed this spot. [ see https://codereview.chromium.org/231723006 ] Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:48: // This is the unique ID of the BrowserChildProcessHost/RenderProcessHost. It On 2015/04/03 02:39:37, Lei Zhang wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:51, Lei Zhang wrote: > > > What does this provide over process_handle() ? > > > > It's totally different. As the comment says it's not the PID nor the process > > handle. > > > > It's the unique ID of the process host instance. > > Is the process host to not a 1:1 mapping with the process handle, with some easy > way to look up one from the other? I just don't understand how this will be > used, so I'm wondering why it's needed. Is it just here because we need this > info often so it's provided for convenience? It is here for convenience indeed, we'll need this often when we refresh the task's NaCl Gdb debug stub port. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:50: virtual int GetChildProcessUniqueID() const = 0; On 2015/04/03 02:39:37, Lei Zhang wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:51, Lei Zhang wrote: > > > Is the browser a "child" process? Are all return values valid? > > > > In the case of the browser process, the return value is 0 since there's no > > browser process host. > > > > But for renderers and other child processes which they have hosts in the > browser > > process with valid unique IDs we return those. > > Can you document this in the comments? What is the valid range for IDs? Is 0 > valid? -1? Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:58: virtual size_t GetSqliteMemoryUsed() const; On 2015/04/03 02:39:37, Lei Zhang wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:51, Lei Zhang wrote: > > > Why do you have ReportsWebCacheStats(), but no ReportsSqliteMemoryUsed() ? > > > > In SqliteMemory we augmented this check in the return value by checking for > -1U. > > > > We can't do this however in WebCache stats since it's a large object with many > > fields. > > So why not do ReportsFoo() / GetFoo() for all types for consistency, and forgo > the sentinel value? Then the caller code can look like: > > if (ReportsFoo()) > foo = GetFoo() > if (ReportsBar()) > bar = GetBar() > > instead of: > > if (ReportsFoo()) > foo = GetFoo() > bar = GetBar() > if (bar == -1) > // handle invalid case Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task.h:81: base::string16 title() const { return title_; } On 2015/04/03 02:39:37, Lei Zhang wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:50, Lei Zhang wrote: > > > You can return a const base::string16& for the non-POD return types. > > > > Done. > > Same goes for icon() and process_handle(). Done. https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider.h (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider.h:24: virtual Task* GetTaskOfUrlRequest(int origin_pid, On 2015/04/03 02:39:37, Lei Zhang wrote: > On 2015/04/03 01:51:01, afakhry wrote: > > On 2015/04/01 22:47:51, Lei Zhang wrote: > > > I looked in net/url_request/url_request.h and I still don't understand what > > > these parameters are. URLRequest doesn't refer to "pid" or "child" or > "route" > > at > > > all. > > > > I agree it's confusing. You need to look at content::ResourceRequestInfo. > > > > I added more comments to make it clearer. > > Would it make sense to just say "the task associated with a ResourceRequestInfo" > and let the reader figure out ResourceRequestInfo maps to a given URLRequest. Is > it very important to mention URLRequest here? It's important to mention here that the only user of these interfaces will be the task manager, this will not be exposed at all to the outside world. That being said, I improved the comments a little bit. https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1038033002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:2847: 'browser/task_management/providers/browser_process/browser_process_task_provider.cc', On 2015/04/03 04:09:03, ncarter wrote: > On 2015/04/03 02:39:37, Lei Zhang wrote: > > On 2015/04/03 01:51:01, afakhry wrote: > > > On 2015/04/01 22:47:52, Lei Zhang wrote: > > > > Do you need to have a browser_process sub-directory, and files within it > > named > > > > browser_process_foo? Maybe just drop the sub-directory? > > > > > > > > ls chrome/browser|wc -l returns 349 > > > > and > > > > ls chrome/browser/extensions|wc -l return 519 > > > > > > > > and nobody is crying yet. (AFAIK) > > > > > > The idea was to keep the private interfaces in the top directory and related > > > implementation files are grouped together in sub-directories. > > > > OTOH, #include > > > "chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h" > > doesn't fit within 80 columns. I think you are trying way too hard to give > your > > files hierarchy. > > > > how about > > chrome/browser/task_management/providers/browser_process_task_provider_impl.h? > > One can easily spot the interface files as the shorter named files without > _impl > > in their names. > > FWIW, no levels of hierarchy after providers/ was my suggestion as well earlier > in the review. I still would prefer at most one level of directory hierarchy, > because the convention in chrome seems to be a relatively flat organization. > > I'm not sure I like the _impl pattern that Lei suggests. Usually when we have > foo_impl.cc it's the sole non-test implementation of the interface foo.h. In > this case we have many implementations of the taskprovider and task interfaces; > seems clumsy to me to call them all 'Impls. > > I'm not bothered by the idea of task.h living alongside browser_process_task.h > and web_contents_task.h. Ok, I yielded although I'm not too happy about it :(. At least for web_contents in the future I strongly believe they should have their own sub-dir. There will be a lot of related files. https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/browser_process/browser_process_task_unittest.cc (right): https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_unittest.cc:28: EXPECT_FALSE(true); On 2015/04/03 02:44:14, Lei Zhang wrote: > Just call FAIL() here. Done. https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/browser_process/browser_process_task_unittest.cc:66: EXPECT_NE(nullptr, provided_task_); On 2015/04/03 02:44:14, Lei Zhang wrote: > This should be ASSERT and not EXPECT. If this fails, the entire unit_tests > process crashes. Done. https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/task_provider_observer.h (right): https://codereview.chromium.org/1038033002/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/task_provider_observer.h:23: // the observer and and references to it must not be kept. On 2015/04/03 02:39:37, Lei Zhang wrote: > typo: and and Done.
lgtm, you may want to add chrome/browser/task_management/OWNERS with 2+ owners.
Done, added OWNERS file. Thanks!
Converted size_t's to int64's
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1038033002/#ps200001 (title: "Converting size_t's to int64's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038033002/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/963710c4f9d8a593d8f78d33428a269b7f26e5e3 Cr-Commit-Position: refs/heads/master@{#323929} |
