Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2440723002:
predictors: Make ResourcePrefetchPredictor observable. (Closed)
Descriptionpredictors: Make ResourcePrefetchPredictor observable.
This observer is needed for browser tests.
+ a small refactoring of ResourcePrefetchPredictor nested classes and
function parameters.
BUG=631966
Committed: https://crrev.com/fdf3eb2acfa3444030d3988b664d00325a3e109d
Cr-Commit-Position: refs/heads/master@{#427707}
Patch Set 1 #Patch Set 2 : Fix a mess with access modifiers. #
Total comments: 17
Patch Set 3 : ObserverList -> Observer* #Patch Set 4 : Make observer private #Patch Set 5 : . #
Total comments: 16
Patch Set 6 : Small fixes. #
Total comments: 10
Patch Set 7 : Work around observer and predictor lifetime. #Patch Set 8 : Tests for observer. #
Total comments: 31
Patch Set 9 : They are not friends anymore. #Patch Set 10 : Make things simpler. #Patch Set 11 : Rename TestObserver to TestDelegate #
Total comments: 7
Patch Set 12 : Revert renaming. #Patch Set 13 : Move TestObserver outside ResourcePrefetchPredictor class. #
Dependent Patchsets: Messages
Total messages: 39 (11 generated)
Description was changed from ========== Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 ========== to ========== Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 ==========
Description was changed from ========== Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 ========== to ========== predictors: Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 ==========
Hello! This is a first step to implement browser tests for resource prefetch predictor. In browser tests, I need to check state of predictor database (or its in-memory cache) after navigation. There wasn't any way to wait for updates reliable. Now it's possible through callback from ResourcePrefetchPredictor.
alexilin@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
On 2016/10/20 16:45:22, alexilin wrote: > Hello! > > This is a first step to implement browser tests for resource prefetch predictor. > > In browser tests, I need to check state of predictor database (or its in-memory > cache) after navigation. There wasn't any way to wait for updates reliable. Now > it's possible through callback from ResourcePrefetchPredictor. Add forgotten reviewers...
if we need many observers, this would look reasonable (see question below, I was not sure we need many observers) also, if we indeed need to maintain a list of observers, a few unittests would be nice to cover this added functionality. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:73: typedef predictors::ResourcePrefetchPredictor::URLRequestSummary our new style prefers type aliases: using URLRequestSummary = predictors::ResourcePrefetchPredictor::URLRequestSummary; BUT! this would be not needed if URLRequestSummary were in namespace predictors, which I suggested in another comment. Just need to put the anonymous namespace into namespace predictors: namespace predictors { namespace { class GetUrlVisitCountTask { ... }; ... } // namespace // ResourcePrefetchPredictor static functions. ... } // namespace predictors https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:372: ResourcePrefetchPredictor::ResourcePrefetchPredictor( By default ObserverList would have NotificationType being NOTIFY_ALL, which means that it would notify observers being inserted by other observers while being notified. The alternative is NOTIFY_EXISTING_ONLY. Which one do we need? https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:105: struct PageRequestSummary { What is the value of having PageRequestSummary in this class? I see a drawback: it is a longer name. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:112: // Stores all subresources requests within a single navigation, from initial nit: s/subresources/subresource/ (I know, I know, this is not being changed) https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:127: }; private: DISALLOW_COPY_AND_ASSIGN(Observer); https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:163: void StartPrefetching(const GURL& main_frame_url); thanks for making only one public section, looks clearer now https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:353: base::ObserverList<Observer> observers_; Do we need many observers or just one would be enough for our tests? Can you list the idea of observers that are going to be introduced?
Pasko PTAL. Observer could be private because browser test class will be a friend of ResourcePrefetchPredictor anyway. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:73: typedef predictors::ResourcePrefetchPredictor::URLRequestSummary On 2016/10/21 12:12:52, pasko wrote: > our new style prefers type aliases: > using URLRequestSummary = > predictors::ResourcePrefetchPredictor::URLRequestSummary; > > BUT! > > this would be not needed if URLRequestSummary were in namespace predictors, > which I suggested in another comment. > > Just need to put the anonymous namespace into namespace predictors: > > namespace predictors { > > namespace { > > class GetUrlVisitCountTask { > ... > }; > > ... > } // namespace > > // ResourcePrefetchPredictor static functions. > > ... > > } // namespace predictors Done for now. Let's consider namespace changing in the next iteration of review. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:372: ResourcePrefetchPredictor::ResourcePrefetchPredictor( On 2016/10/21 12:12:52, pasko wrote: > By default ObserverList would have NotificationType being NOTIFY_ALL, which > means that it would notify observers being inserted by other observers while > being notified. The alternative is NOTIFY_EXISTING_ONLY. Which one do we need? Non-actual since we store single observer. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:105: struct PageRequestSummary { On 2016/10/21 12:12:53, pasko wrote: > What is the value of having PageRequestSummary in this class? I see a drawback: > it is a longer name. We could extract the URLRequestSummary class from ResourcePrefetchPredictor declaration too. And move it to resource_prefetch_common.h. Or keep in this header. WDYT? https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:112: // Stores all subresources requests within a single navigation, from initial On 2016/10/21 12:12:52, pasko wrote: > nit: s/subresources/subresource/ > > (I know, I know, this is not being changed) Done. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:127: }; On 2016/10/21 12:12:52, pasko wrote: > private: > DISALLOW_COPY_AND_ASSIGN(Observer); Done. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:163: void StartPrefetching(const GURL& main_frame_url); On 2016/10/21 12:12:52, pasko wrote: > thanks for making only one public section, looks clearer now Acknowledged. https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:353: base::ObserverList<Observer> observers_; On 2016/10/21 12:12:52, pasko wrote: > Do we need many observers or just one would be enough for our tests? > > Can you list the idea of observers that are going to be introduced? I see only one application point for these observers for now, it's browser tests. From offline discussion: it was decided to replace list of observers by reference to single observer and to mark that it's observer for testing.
looks better, how about tests? https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:105: struct PageRequestSummary { On 2016/10/21 13:38:04, alexilin wrote: > On 2016/10/21 12:12:53, pasko wrote: > > What is the value of having PageRequestSummary in this class? I see a > drawback: > > it is a longer name. > > We could extract the URLRequestSummary class from ResourcePrefetchPredictor > declaration too. And move it to resource_prefetch_common.h. Or keep in this > header. WDYT? Frankly I do not really understand the purpose of resource_prefetch_common.h. Moving the declaration there won't make PageRequestSummary be available in less files, hence probably won't increase overall maintainability, so it would be better to stay in resource_prefetch_predictor because that's where the declaration is mostly used. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:19: #include "base/observer_list.h" not needed any more? https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:187: class Observer { s/Observer/TestObserver/ https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:193: Observer() = default; if this is to disallow instantiating this class, then there is another way: virtual void OnNavigationLearned(...) = 0; https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:194: virtual ~Observer() {} if the destructor is protected, then it becomes problematic to use this class with std::unique_ptr<> I'd suggest public, and should go before the method declarations. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:344: // Sets an |observer| to be notified when the resource prefetch predictor data s/an/the/ or: s/an// https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:345: // changes. Use only for testing. Better omit the "Use only for testing" comment because there is a Chrome-wide convention to name methods with suffix "ForTesting" only if those are not called in production. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:349: void RemoveObserverForTesting(Observer* observer); providing the same observer for removal looks a little inconvenient as an API, the user has to keep the pointer for removal. How about just having SetObserverForTesting(...) where providing a nullptr indicates that the observer is removed? In the comment we also need to write that the observer needs to be removed before it is destructed, otherwise we risk getting use-after-free in tests.
https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:105: struct PageRequestSummary { On 2016/10/21 14:41:35, pasko wrote: > On 2016/10/21 13:38:04, alexilin wrote: > > On 2016/10/21 12:12:53, pasko wrote: > > > What is the value of having PageRequestSummary in this class? I see a > > drawback: > > > it is a longer name. > > > > We could extract the URLRequestSummary class from ResourcePrefetchPredictor > > declaration too. And move it to resource_prefetch_common.h. Or keep in this > > header. WDYT? > > Frankly I do not really understand the purpose of resource_prefetch_common.h. > Moving the declaration there won't make PageRequestSummary be available in less > files, hence probably won't increase overall maintainability, so it would be > better to stay in resource_prefetch_predictor because that's where the > declaration is mostly used. Ok, it turned out that I can't take PageRequestSummary from ResourcePrefetchPredictorClass without taking out URLRequestSummary that leads to changes in many files. Maybe in other CL? https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:19: #include "base/observer_list.h" On 2016/10/21 14:41:36, pasko wrote: > not needed any more? Done. I still don't know how to keep list of includes nicely. This is the thing in C++ I don't like most of all. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:187: class Observer { On 2016/10/21 14:41:36, pasko wrote: > s/Observer/TestObserver/ Done. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:193: Observer() = default; On 2016/10/21 14:41:36, pasko wrote: > if this is to disallow instantiating this class, then there is another way: > > virtual void OnNavigationLearned(...) = 0; No, this is for enabling default constructor explicitly because the explicit declaration of copy constructor (from DISALLOW_COPY_AND_ASSIGN) inhibits the implicit generation of default constructor. It could be made public but I don't see a reason for it. I don't want to force all subclasses to implement all observer methods (well, we have only one for now) similarly to WebContentsObserver so that's why I don't want to add abstract methods. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:194: virtual ~Observer() {} On 2016/10/21 14:41:36, pasko wrote: > if the destructor is protected, then it becomes problematic to use this class > with std::unique_ptr<> > > I'd suggest public, and should go before the method declarations. Done. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:344: // Sets an |observer| to be notified when the resource prefetch predictor data On 2016/10/21 14:41:36, pasko wrote: > s/an/the/ or: s/an// > Done. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:345: // changes. Use only for testing. On 2016/10/21 14:41:36, pasko wrote: > Better omit the "Use only for testing" comment because there is a Chrome-wide > convention to name methods with suffix "ForTesting" only if those are not called > in production. Done. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:349: void RemoveObserverForTesting(Observer* observer); On 2016/10/21 14:41:36, pasko wrote: > providing the same observer for removal looks a little inconvenient as an API, > the user has to keep the pointer for removal. > > How about just having SetObserverForTesting(...) where providing a nullptr > indicates that the observer is removed? > > In the comment we also need to write that the observer needs to be removed > before it is destructed, otherwise we risk getting use-after-free in tests. Your statement is contradictory a little bit. You say that it's inconvenient for users to keep pointer for the observer but then you say that user has to explicitly remove observer before it is destructed. So user anyway should be aware of connection between observer destruction and it's removing. Probably, it'd be better to force all observers to be scoped, i.e. explicitly define follow constructor and desctructor: TestObserver(ResourcePrefetchPredictor* predictor) { predictor->AddObserver(this); } virtual ~TestObserver() { predictor->RemoveObserver(this); }
overall looks good, modulo tests https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:105: struct PageRequestSummary { On 2016/10/21 16:04:18, alexilin wrote: > On 2016/10/21 14:41:35, pasko wrote: > > On 2016/10/21 13:38:04, alexilin wrote: > > > On 2016/10/21 12:12:53, pasko wrote: > > > > What is the value of having PageRequestSummary in this class? I see a > > > drawback: > > > > it is a longer name. > > > > > > We could extract the URLRequestSummary class from ResourcePrefetchPredictor > > > declaration too. And move it to resource_prefetch_common.h. Or keep in this > > > header. WDYT? > > > > Frankly I do not really understand the purpose of resource_prefetch_common.h. > > Moving the declaration there won't make PageRequestSummary be available in > less > > files, hence probably won't increase overall maintainability, so it would be > > better to stay in resource_prefetch_predictor because that's where the > > declaration is mostly used. > > Ok, it turned out that I can't take PageRequestSummary from > ResourcePrefetchPredictorClass without taking out URLRequestSummary that leads > to changes in many files. Maybe in other CL? Ah, right. OK. https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:193: Observer() = default; On 2016/10/21 16:04:19, alexilin wrote: > On 2016/10/21 14:41:36, pasko wrote: > > if this is to disallow instantiating this class, then there is another way: > > > > virtual void OnNavigationLearned(...) = 0; > > No, this is for enabling default constructor explicitly because the explicit > declaration of copy constructor (from DISALLOW_COPY_AND_ASSIGN) inhibits the > implicit generation of default constructor. > It could be made public but I don't see a reason for it. > I don't want to force all subclasses to implement all observer methods (well, we > have only one for now) similarly to WebContentsObserver so that's why I don't > want to add abstract methods. Ah, oh, I did not know DISALLOW_COPY_AND_ASSIGN prevents implicit default constructor. Well, C++ .. Now it makes sense, thanks! https://codereview.chromium.org/2440723002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:349: void RemoveObserverForTesting(Observer* observer); On 2016/10/21 16:04:19, alexilin wrote: > On 2016/10/21 14:41:36, pasko wrote: > > providing the same observer for removal looks a little inconvenient as an API, > > the user has to keep the pointer for removal. > > > > How about just having SetObserverForTesting(...) where providing a nullptr > > indicates that the observer is removed? > > > > In the comment we also need to write that the observer needs to be removed > > before it is destructed, otherwise we risk getting use-after-free in tests. > > Your statement is contradictory a little bit. You say that it's inconvenient for > users to keep pointer for the observer but then you say that user has to > explicitly remove observer before it is destructed. So user anyway should be > aware of connection between observer destruction and it's removing. > > Probably, it'd be better to force all observers to be scoped, i.e. explicitly > define follow constructor and desctructor: > > TestObserver(ResourcePrefetchPredictor* predictor) { > predictor->AddObserver(this); > } > > virtual ~TestObserver() { > predictor->RemoveObserver(this); > } This scoped observer would be useful if we are sure that ResourcePrefetchPredictor will always outlive the observer, which I was not sure about. We can also add a ScopedObserverForTesting, just like ScopedObserver<>, but instead of AddObserver it would call AddObserverForTesting, and such. But yeah, it seems that observers that are always scoped are quite usable for us. I was not sure how the tests are structured, will they be creating and destroying ResourcePrefetchPredictor? I suppose not, then yeah, keeping the observer scoped helps. But there still seems to be no value in checking that the observer is the same on removal, checking that there is no observer at AddObserver is sufficient protection against mistakes IMO. https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:152: // Stops prefetching that may be in progress corresponding to |navigation_id|. huh, I did not notice this during one of the previous reviews. These 2 methods are no longer driven by navigation ids. While we are here, please update the comments.
https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:103: // Stores the data is learned from single navigation. nit: Stores the data learned from a single navigation. https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:185: // ResourcePrefetchPredictor::SetObserverForTesting. Can you add a comment specifying on which thread the observer is going to be called? https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:346: void SetObserverForTesting(TestObserver* observer); Stupid question: can we use a unique_ptr<> to hold the observer? This way we don't have to worry about lifetime issues, and reseting the observer cleans up the previous one.
https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:346: void SetObserverForTesting(TestObserver* observer); On 2016/10/21 17:16:24, Benoit L wrote: > Stupid question: can we use a unique_ptr<> to hold the observer? > This way we don't have to worry about lifetime issues, and reseting the observer > cleans up the previous one. Not stupid at all. It's hard to say for me without seeing how tests would use it. I was lazy to think about proper ownership and thought that having a raw pointer here would maximally move the ownership complexity towards tests (which would be not complex at all, I suspect). I more often find testing-only related injections to happen via raw pointers. So that would be a weak argument towards consistency. Also, what if a class inherits from several observers? It would not be good if each of the notification interfaces owned the object. But your point is good: grabbing the observer via unique_ptr makes the ownership super clear, no need to have lengthy explanations in comments. Seems usable as well. I am OK either way.
This is the last for today. Tests to be continued... https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:103: // Stores the data is learned from single navigation. On 2016/10/21 17:16:24, Benoit L wrote: > nit: Stores the data learned from a single navigation. Done. https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:152: // Stops prefetching that may be in progress corresponding to |navigation_id|. On 2016/10/21 16:47:34, pasko wrote: > huh, I did not notice this during one of the previous reviews. These 2 methods > are no longer driven by navigation ids. While we are here, please update the > comments. Done. https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:185: // ResourcePrefetchPredictor::SetObserverForTesting. On 2016/10/21 17:16:24, Benoit L wrote: > Can you add a comment specifying on which thread the observer is going to be > called? Done. https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:346: void SetObserverForTesting(TestObserver* observer); On 2016/10/21 17:38:22, pasko wrote: > On 2016/10/21 17:16:24, Benoit L wrote: > > Stupid question: can we use a unique_ptr<> to hold the observer? > > This way we don't have to worry about lifetime issues, and reseting the > observer > > cleans up the previous one. > > Not stupid at all. > > It's hard to say for me without seeing how tests would use it. I was lazy to > think about proper ownership and thought that having a raw pointer here would > maximally move the ownership complexity towards tests (which would be not > complex at all, I suspect). I more often find testing-only related injections to > happen via raw pointers. So that would be a weak argument towards consistency. > > Also, what if a class inherits from several observers? It would not be good if > each of the notification interfaces owned the object. > > But your point is good: grabbing the observer via unique_ptr makes the ownership > super clear, no need to have lengthy explanations in comments. Seems usable as > well. > > I am OK either way. > Well, it's not great to transfer the observer ownership to ResourcePrefetchPredictor from the side of browser tests. And then it won't be observer pattern rather I just pass callback to the class. I don't say that it's bad but this is not the thing that I would like to have in browser tests. I inspired https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... class and made the similar thing. Please, check out updates. To illustrate browser test's point of view here is sketchy implementation of observer that I want to have. class ResourcePrefetchPredictorTestObserver : public ResourcePrefetchPredictor::TestObserver { public: explicit ResourcePrefetchPredictorTestObserver(ResourcePrefetchPredictor* predictor) : TestObserver(predictor), message_loop_runner_(new content::MessageLoopRunner) {} void Wait() { message_loop_runner_->Run(); } protected: // ResourcePrefetchPredictor::Observer void OnNavigationLearned(size_t url_visit_count, const ResourcePrefetchPredictor::PageRequestSummary& summary) override { message_loop_runner_->Quit(); } private: scoped_refptr<content::MessageLoopRunner> message_loop_runner_; DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTestObserver); }; And example of use: ResourcePrefetchPredictorTestObserver observer(predictor_); ui_test_utils::NavigateToURL(browser(), url); observer.Wait();
https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:346: void SetObserverForTesting(TestObserver* observer); On 2016/10/21 18:37:15, alexilin wrote: > On 2016/10/21 17:38:22, pasko wrote: > > On 2016/10/21 17:16:24, Benoit L wrote: > > > Stupid question: can we use a unique_ptr<> to hold the observer? > > > This way we don't have to worry about lifetime issues, and reseting the > > observer > > > cleans up the previous one. > > > > Not stupid at all. > > > > It's hard to say for me without seeing how tests would use it. I was lazy to > > think about proper ownership and thought that having a raw pointer here would > > maximally move the ownership complexity towards tests (which would be not > > complex at all, I suspect). I more often find testing-only related injections > to > > happen via raw pointers. So that would be a weak argument towards consistency. > > > > Also, what if a class inherits from several observers? It would not be good if > > each of the notification interfaces owned the object. > > > > But your point is good: grabbing the observer via unique_ptr makes the > ownership > > super clear, no need to have lengthy explanations in comments. Seems usable as > > well. > > > > I am OK either way. > > > > Well, it's not great to transfer the observer ownership to > ResourcePrefetchPredictor from the side of browser tests. And then it won't be > observer pattern rather I just pass callback to the class. I don't say that it's > bad but this is not the thing that I would like to have in browser tests. > I inspired > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... > class and made the similar thing. Please, check out updates. > > To illustrate browser test's point of view here is sketchy implementation of > observer that I want to have. > > class ResourcePrefetchPredictorTestObserver : public > ResourcePrefetchPredictor::TestObserver { > public: > explicit ResourcePrefetchPredictorTestObserver(ResourcePrefetchPredictor* > predictor) > : TestObserver(predictor), > message_loop_runner_(new content::MessageLoopRunner) {} > > void Wait() { message_loop_runner_->Run(); } > > protected: > // ResourcePrefetchPredictor::Observer > void OnNavigationLearned(size_t url_visit_count, const > ResourcePrefetchPredictor::PageRequestSummary& summary) override { > message_loop_runner_->Quit(); > } > > private: > scoped_refptr<content::MessageLoopRunner> message_loop_runner_; > DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTestObserver); > }; > > And example of use: > > ResourcePrefetchPredictorTestObserver observer(predictor_); > ui_test_utils::NavigateToURL(browser(), url); > observer.Wait(); Ok, thank you for the clarification. Observers usually use ObserverList, but that's likely overkill here. Note that WebContentsObserver unregisters itself in the destructor.
I've finished with tests for this CL. Note last test in resource_prefetch_predictor_unittest.cc. It illustrates how predictor and observer are connected with each other. The answer to Benoit's remark about unsubscription: yes, I handle it.
lgtm, thanks.
thank you for the tests, now I finally get a better idea of how learn-navigation is going to be tested. See below for suggestions on how to simplify it. It will be not necessarily less error-prone, but I think it is better to move the test code maximally to the test side, eliminating the complications of the main class. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:385: predictor_->observer_ = nullptr; yuck! does this compile because the inner class is a friend to the outer? Let's try to avoid such tight coupling. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1187: DCHECK(observer->predictor_ == this); DCHECKs in tests should be avoided. Also, this check is done at the cost of having, ResourcePrefetchPredictor as a friend to the observer, which complicates understanding the interfaces of both. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:104: struct PageRequestSummary { struct vs. class: the style guide says: "structs should be used for passive objects that carry data, and may have associated constants, but lack any functionality other than access/setting the data members." This one contains GURL and vector, which are not "passive objects", so the overall object therefore is not "passive". Hence, it should be treated as a class (with extra boilerplate, .. true .., but it is valuable to describe what is allowed to be done on this class, and what is not allowed. Otherwise it is hard to follow in the code whether the vector of request is ever changed after the object creation. Seems that is only being moved, and a copy-c-tor added with one more field. In this case feel free to add a TODO and do it in another change. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:106: PageRequestSummary(const PageRequestSummary& other); hm, just noticed: public copy-constructor is error-prone, can we avoid it please? this is especially worrisome because the object holds a vector (hence it is likely not cheap to copy). https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:125: const PageRequestSummary& summary) {} I think a single mock would be enough to hook into all methods. Then it would be cleaner if TestObserver were just pure virtual, and the mock could manage its lifetime and know about the way the ResourcePrefetchPredictor works. The point of having a single observer injected by a raw pointer was to free the ResourcePrefetchPredictor from knowing too much about the observers. So let's keep it this way? https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:128: // Observer is tied to a single ResourcePrefetchPredictor for its entire it is not perfectly clear what 'its' refers to, ResourcePrefetchPredictor or TestObserver https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:291: mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>( If there is a ResetPredictor() somewhere in the test, what happens with an EXPECT_CALL(*mock_observer_.get(), ...) that was issued before it? The approach with mock_tables_ seems to be working because this object is not swapped, just re-attached to the new ResourcePrefetchPredictor on every ResetPredictor(). Seems like consistency here would eliminate unpleasant surprises. Would be nice. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:291: mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>( It would be nice to avoid this having a 'global' mock_observer_ for all tests with complicated resetting, registration and deregistration, friendship between the TestObserver and the ResourcePrefetchPredictor. I suspect it is not really important to hook into LearnNavigation between resets of the ResourcePrefetchPredictor (not sure here, there could be reasons, let me know). We could instead have a std::unique_ptr<MockTestObserver> in each test that needs the hook, and the hook would de-register itself in its destructor. The constructor for such observer _could_ take expected learnings as a parameter, which can be EXPECT-ed in the destructor. This still has the flexibility of .reset() on the observer to perform the checks and installing the new observer. This way the two classes would avoid tight coupling: for the predictor, the observer is just a pointer - minimal code in production. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1592: // Test that observer imforms predictor about its death. nit: s/imforms/informs/
On 2016/10/24 17:59:39, pasko wrote: > thank you for the tests, now I finally get a better idea of how learn-navigation > is going to be tested. See below for suggestions on how to simplify it. It will > be not necessarily less error-prone, but I think it is better to move the test > code maximally to the test side, eliminating the complications of the main > class. > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > File chrome/browser/predictors/resource_prefetch_predictor.cc (right): > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.cc:385: > predictor_->observer_ = nullptr; > yuck! does this compile because the inner class is a friend to the outer? Let's > try to avoid such tight coupling. > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.cc:1187: > DCHECK(observer->predictor_ == this); > DCHECKs in tests should be avoided. Also, this check is done at the cost of > having, ResourcePrefetchPredictor as a friend to the observer, which complicates > understanding the interfaces of both. > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > File chrome/browser/predictors/resource_prefetch_predictor.h (right): > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.h:104: struct > PageRequestSummary { > struct vs. class: > > the style guide says: "structs should be used for passive objects that carry > data, and may have associated constants, but lack any functionality other than > access/setting the data members." > > This one contains GURL and vector, which are not "passive objects", so the > overall object therefore is not "passive". Hence, it should be treated as a > class (with extra boilerplate, .. true .., but it is valuable to describe what > is allowed to be done on this class, and what is not allowed. Otherwise it is > hard to follow in the code whether the vector of request is ever changed after > the object creation. > > Seems that is only being moved, and a copy-c-tor added with one more field. In > this case feel free to add a TODO and do it in another change. > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.h:106: > PageRequestSummary(const PageRequestSummary& other); > hm, just noticed: public copy-constructor is error-prone, can we avoid it > please? this is especially worrisome because the object holds a vector (hence it > is likely not cheap to copy). > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.h:125: const > PageRequestSummary& summary) {} > I think a single mock would be enough to hook into all methods. > > Then it would be cleaner if TestObserver were just pure virtual, and the mock > could manage its lifetime and know about the way the ResourcePrefetchPredictor > works. The point of having a single observer injected by a raw pointer was to > free the ResourcePrefetchPredictor from knowing too much about the observers. So > let's keep it this way? > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.h:128: // Observer is tied > to a single ResourcePrefetchPredictor for its entire > it is not perfectly clear what 'its' refers to, ResourcePrefetchPredictor or > TestObserver > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:291: > mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>( > If there is a ResetPredictor() somewhere in the test, what happens with an > EXPECT_CALL(*mock_observer_.get(), ...) that was issued before it? > > The approach with mock_tables_ seems to be working because this object is not > swapped, just re-attached to the new ResourcePrefetchPredictor on every > ResetPredictor(). Seems like consistency here would eliminate unpleasant > surprises. Would be nice. > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:291: > mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>( > It would be nice to avoid this having a 'global' mock_observer_ for all tests > with complicated resetting, registration and deregistration, friendship between > the TestObserver and the ResourcePrefetchPredictor. > > I suspect it is not really important to hook into LearnNavigation between resets > of the ResourcePrefetchPredictor (not sure here, there could be reasons, let me > know). > > We could instead have a std::unique_ptr<MockTestObserver> in each test that > needs the hook, and the hook would de-register itself in its destructor. The > constructor for such observer _could_ take expected learnings as a parameter, > which can be EXPECT-ed in the destructor. This still has the flexibility of > .reset() on the observer to perform the checks and installing the new observer. > This way the two classes would avoid tight coupling: for the predictor, the > observer is just a pointer - minimal code in production. > > https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1592: // Test > that observer imforms predictor about its death. > nit: s/imforms/informs/ Ooops, I missed that. I totally agree with Egor that avoiding friend classes is better when we can manage.
Let's have some chat. Benoit: https://memegen.googleplex.com/5270515979124736 :) https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:385: predictor_->observer_ = nullptr; On 2016/10/24 17:59:38, pasko wrote: > yuck! does this compile because the inner class is a friend to the outer? Let's > try to avoid such tight coupling. Then we have to make SetObserverForTesting method public. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1187: DCHECK(observer->predictor_ == this); On 2016/10/24 17:59:38, pasko wrote: > DCHECKs in tests should be avoided. Also, this check is done at the cost of > having, ResourcePrefetchPredictor as a friend to the observer, which complicates > understanding the interfaces of both. What's wrong with DCHECKs in tests? It's one way to make sure that code works correctly no matter whether it's test code or production. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:104: struct PageRequestSummary { On 2016/10/24 17:59:38, pasko wrote: > struct vs. class: > > the style guide says: "structs should be used for passive objects that carry > data, and may have associated constants, but lack any functionality other than > access/setting the data members." > > This one contains GURL and vector, which are not "passive objects", so the > overall object therefore is not "passive". Hence, it should be treated as a > class (with extra boilerplate, .. true .., but it is valuable to describe what > is allowed to be done on this class, and what is not allowed. Otherwise it is > hard to follow in the code whether the vector of request is ever changed after > the object creation. > > Seems that is only being moved, and a copy-c-tor added with one more field. In > this case feel free to add a TODO and do it in another change. I have another opinion. PageRequestSummary should be considered as struct because it only holds data. No matter that its fields is not PODs or non-class types, PageRequestSummary itself is used only to keep and pass data accross. It even doesn't have set/get accessors to be as simple as possible. For example, look at URLRequestSummary struct above or ResourcePrefetchPredictorConfig (https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...) or FrameNavigateParams (https://cs.chromium.org/chromium/src/content/public/common/frame_navigate_par...) https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:106: PageRequestSummary(const PageRequestSummary& other); On 2016/10/24 17:59:38, pasko wrote: > hm, just noticed: public copy-constructor is error-prone, can we avoid it > please? this is especially worrisome because the object holds a vector (hence it > is likely not cheap to copy). It will be hard to avoid this because of following compiler error: "error: [chromium-style] Complex class/struct needs an explicit out-of-line copy constructor." This class has already had implicit public copy constructor before that. The other thing we can do is DISALLOW_COPY_AND_ASSIGN, but I think that it will be a little bit strange to have uncopyable struct. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:125: const PageRequestSummary& summary) {} On 2016/10/24 17:59:38, pasko wrote: > I think a single mock would be enough to hook into all methods. > > Then it would be cleaner if TestObserver were just pure virtual, and the mock > could manage its lifetime and know about the way the ResourcePrefetchPredictor > works. The point of having a single observer injected by a raw pointer was to > free the ResourcePrefetchPredictor from knowing too much about the observers. So > let's keep it this way? IIUC, you suggested to move all implementation details from TestObserver to mock class in resource_prefetch_predictor_unittest.cc But I'll have different implementations for unit tests and browser tests. It seems like we need to have lifetime management in both of them. Please, explain me your idea in more details. Thanks. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:128: // Observer is tied to a single ResourcePrefetchPredictor for its entire On 2016/10/24 17:59:38, pasko wrote: > it is not perfectly clear what 'its' refers to, ResourcePrefetchPredictor or > TestObserver Done. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:291: mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>( On 2016/10/24 17:59:38, pasko wrote: > If there is a ResetPredictor() somewhere in the test, what happens with an > EXPECT_CALL(*mock_observer_.get(), ...) that was issued before it? > > The approach with mock_tables_ seems to be working because this object is not > swapped, just re-attached to the new ResourcePrefetchPredictor on every > ResetPredictor(). Seems like consistency here would eliminate unpleasant > surprises. Would be nice. Yeah, it's a kind of dangerous situation. Let's try to rewrite it. All control happens in a single thread so there won't be any race conditions between resets. I don't want reinvent EXPECT_CALL and pass expected arguments as parameters of the mock class constructor. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1592: // Test that observer imforms predictor about its death. On 2016/10/24 17:59:38, pasko wrote: > nit: s/imforms/informs/ Done.
some chat: Done. ;) https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:385: predictor_->observer_ = nullptr; On 2016/10/25 11:38:09, alexilin wrote: > On 2016/10/24 17:59:38, pasko wrote: > > yuck! does this compile because the inner class is a friend to the outer? > Let's > > try to avoid such tight coupling. > > Then we have to make SetObserverForTesting method public. I believe this is how it is usually done in Chromium. So that's what I wanted initially, it was not super clear, sorry about that. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1187: DCHECK(observer->predictor_ == this); On 2016/10/25 11:38:09, alexilin wrote: > On 2016/10/24 17:59:38, pasko wrote: > > DCHECKs in tests should be avoided. Also, this check is done at the cost of > > having, ResourcePrefetchPredictor as a friend to the observer, which > complicates > > understanding the interfaces of both. > > What's wrong with DCHECKs in tests? It's one way to make sure that code works > correctly no matter whether it's test code or production. Style guide: Don't use these macros in tests, as they crash the test binary and leave bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which report failures gracefully and can continue running other tests. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:104: struct PageRequestSummary { On 2016/10/25 11:38:09, alexilin wrote: > On 2016/10/24 17:59:38, pasko wrote: > > struct vs. class: > > > > the style guide says: "structs should be used for passive objects that carry > > data, and may have associated constants, but lack any functionality other than > > access/setting the data members." > > > > This one contains GURL and vector, which are not "passive objects", so the > > overall object therefore is not "passive". Hence, it should be treated as a > > class (with extra boilerplate, .. true .., but it is valuable to describe what > > is allowed to be done on this class, and what is not allowed. Otherwise it is > > hard to follow in the code whether the vector of request is ever changed after > > the object creation. > > > > Seems that is only being moved, and a copy-c-tor added with one more field. In > > this case feel free to add a TODO and do it in another change. > > I have another opinion. PageRequestSummary should be considered as struct > because it only holds data. No matter that its fields is not PODs or non-class > types, PageRequestSummary itself is used only to keep and pass data accross. It > even doesn't have set/get accessors to be as simple as possible. > For example, look at URLRequestSummary struct above or > ResourcePrefetchPredictorConfig > (https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...) > or FrameNavigateParams > (https://cs.chromium.org/chromium/src/content/public/common/frame_navigate_par...) OK, you are right, it is indeed common that non-trivial objects are struct members. This makes it sometimes complex to reason about these structs (for instance, the insides of GURL are memory-managed non-trivially), but you convinced me that these pitfalls are commonly expected in the codebase. No need to change it then. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:106: PageRequestSummary(const PageRequestSummary& other); On 2016/10/25 11:38:09, alexilin wrote: > On 2016/10/24 17:59:38, pasko wrote: > > hm, just noticed: public copy-constructor is error-prone, can we avoid it > > please? this is especially worrisome because the object holds a vector (hence > it > > is likely not cheap to copy). > > It will be hard to avoid this because of following compiler error: > "error: [chromium-style] Complex class/struct needs an explicit out-of-line copy > constructor." > This class has already had implicit public copy constructor before that. > The other thing we can do is DISALLOW_COPY_AND_ASSIGN, but I think that it will > be a little bit strange to have uncopyable struct. Oh, makes sense, thank you for explaining. I forgot about this error. Also, I presume we need this copy constructor to be able to store these structs in a vector. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:125: const PageRequestSummary& summary) {} On 2016/10/25 11:38:09, alexilin wrote: > On 2016/10/24 17:59:38, pasko wrote: > > I think a single mock would be enough to hook into all methods. > > > > Then it would be cleaner if TestObserver were just pure virtual, and the mock > > could manage its lifetime and know about the way the ResourcePrefetchPredictor > > works. The point of having a single observer injected by a raw pointer was to > > free the ResourcePrefetchPredictor from knowing too much about the observers. > So > > let's keep it this way? > > IIUC, you suggested to move all implementation details from TestObserver to mock > class in resource_prefetch_predictor_unittest.cc > But I'll have different implementations for unit tests and browser tests. It > seems like we need to have lifetime management in both of them. > Please, explain me your idea in more details. Thanks. Oops. I forgot that the de-registration logic still is preferable to be in this class. So it seems that this model would be better: 1. TestObserver de-registers itself from the ResourcePrefetchPredictor in d-tor 2. tests guarantee that ResourcePrefetchPredictor always outlives the TestObserver 3. discussed offline: the mock for browser tests would be different, hence de-registering makes sense to put in some common class 4. discussed offline: if the second test observer is needed in a test, the first will be .reset() prior to registering the 2nd one Also, please just forward-declare the TestObserver here, and define it in some place for test utilities. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:291: mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>( On 2016/10/25 11:38:09, alexilin wrote: > On 2016/10/24 17:59:38, pasko wrote: > > If there is a ResetPredictor() somewhere in the test, what happens with an > > EXPECT_CALL(*mock_observer_.get(), ...) that was issued before it? > > > > The approach with mock_tables_ seems to be working because this object is not > > swapped, just re-attached to the new ResourcePrefetchPredictor on every > > ResetPredictor(). Seems like consistency here would eliminate unpleasant > > surprises. Would be nice. > > Yeah, it's a kind of dangerous situation. > Let's try to rewrite it. > > All control happens in a single thread so there won't be any race conditions > between resets. > > I don't want reinvent EXPECT_CALL and pass expected arguments as parameters of > the mock class constructor. this latter sgtm
No memes this time. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:385: predictor_->observer_ = nullptr; On 2016/10/25 14:05:40, pasko wrote: > On 2016/10/25 11:38:09, alexilin wrote: > > On 2016/10/24 17:59:38, pasko wrote: > > > yuck! does this compile because the inner class is a friend to the outer? > > Let's > > > try to avoid such tight coupling. > > > > Then we have to make SetObserverForTesting method public. > > I believe this is how it is usually done in Chromium. So that's what I wanted > initially, it was not super clear, sorry about that. Acknowledged. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1187: DCHECK(observer->predictor_ == this); On 2016/10/25 14:05:40, pasko wrote: > On 2016/10/25 11:38:09, alexilin wrote: > > On 2016/10/24 17:59:38, pasko wrote: > > > DCHECKs in tests should be avoided. Also, this check is done at the cost of > > > having, ResourcePrefetchPredictor as a friend to the observer, which > > complicates > > > understanding the interfaces of both. > > > > What's wrong with DCHECKs in tests? It's one way to make sure that code works > > correctly no matter whether it's test code or production. > > Style guide: Don't use these macros in tests, as they crash the test binary and > leave bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, > which report failures gracefully and can continue running other tests. > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Yes, you are right. Although I can't say that this code is truly "test" code. I've found some places in the codebase where DCHECK is called inside *ForTesting function but not inside test file. Evidence: https://cs.chromium.org/chromium/src/base/threading/thread.cc?rcl=1477379968&... https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:104: struct PageRequestSummary { On 2016/10/25 14:05:40, pasko wrote: > On 2016/10/25 11:38:09, alexilin wrote: > > On 2016/10/24 17:59:38, pasko wrote: > > > struct vs. class: > > > > > > the style guide says: "structs should be used for passive objects that carry > > > data, and may have associated constants, but lack any functionality other > than > > > access/setting the data members." > > > > > > This one contains GURL and vector, which are not "passive objects", so the > > > overall object therefore is not "passive". Hence, it should be treated as a > > > class (with extra boilerplate, .. true .., but it is valuable to describe > what > > > is allowed to be done on this class, and what is not allowed. Otherwise it > is > > > hard to follow in the code whether the vector of request is ever changed > after > > > the object creation. > > > > > > Seems that is only being moved, and a copy-c-tor added with one more field. > In > > > this case feel free to add a TODO and do it in another change. > > > > I have another opinion. PageRequestSummary should be considered as struct > > because it only holds data. No matter that its fields is not PODs or non-class > > types, PageRequestSummary itself is used only to keep and pass data accross. > It > > even doesn't have set/get accessors to be as simple as possible. > > For example, look at URLRequestSummary struct above or > > ResourcePrefetchPredictorConfig > > > (https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...) > > or FrameNavigateParams > > > (https://cs.chromium.org/chromium/src/content/public/common/frame_navigate_par...) > > OK, you are right, it is indeed common that non-trivial objects are struct > members. This makes it sometimes complex to reason about these structs (for > instance, the insides of GURL are memory-managed non-trivially), but you > convinced me that these pitfalls are commonly expected in the codebase. No need > to change it then. Done. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:106: PageRequestSummary(const PageRequestSummary& other); On 2016/10/25 14:05:40, pasko wrote: > On 2016/10/25 11:38:09, alexilin wrote: > > On 2016/10/24 17:59:38, pasko wrote: > > > hm, just noticed: public copy-constructor is error-prone, can we avoid it > > > please? this is especially worrisome because the object holds a vector > (hence > > it > > > is likely not cheap to copy). > > > > It will be hard to avoid this because of following compiler error: > > "error: [chromium-style] Complex class/struct needs an explicit out-of-line > copy > > constructor." > > This class has already had implicit public copy constructor before that. > > The other thing we can do is DISALLOW_COPY_AND_ASSIGN, but I think that it > will > > be a little bit strange to have uncopyable struct. > > Oh, makes sense, thank you for explaining. I forgot about this error. Also, I > presume we need this copy constructor to be able to store these structs in a > vector. Done. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:125: const PageRequestSummary& summary) {} On 2016/10/25 14:05:40, pasko wrote: > On 2016/10/25 11:38:09, alexilin wrote: > > On 2016/10/24 17:59:38, pasko wrote: > > > I think a single mock would be enough to hook into all methods. > > > > > > Then it would be cleaner if TestObserver were just pure virtual, and the > mock > > > could manage its lifetime and know about the way the > ResourcePrefetchPredictor > > > works. The point of having a single observer injected by a raw pointer was > to > > > free the ResourcePrefetchPredictor from knowing too much about the > observers. > > So > > > let's keep it this way? > > > > IIUC, you suggested to move all implementation details from TestObserver to > mock > > class in resource_prefetch_predictor_unittest.cc > > But I'll have different implementations for unit tests and browser tests. It > > seems like we need to have lifetime management in both of them. > > Please, explain me your idea in more details. Thanks. > > Oops. I forgot that the de-registration logic still is preferable to be in this > class. So it seems that this model would be better: > > 1. TestObserver de-registers itself from the ResourcePrefetchPredictor in d-tor > 2. tests guarantee that ResourcePrefetchPredictor always outlives the > TestObserver > 3. discussed offline: the mock for browser tests would be different, hence > de-registering makes sense to put in some common class > 4. discussed offline: if the second test observer is needed in a test, the first > will be .reset() prior to registering the 2nd one > > Also, please just forward-declare the TestObserver here, and define it in some > place for test utilities. Everything done except the last one. Discussed offline: forward-declare thing would't work because ResourcePrefetchPredictor calls OnNavigationLearned method. If we move declaration to test utilities file then TestObserver class won't be resolved in non-test build. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:291: mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>( On 2016/10/25 14:05:40, pasko wrote: > On 2016/10/25 11:38:09, alexilin wrote: > > On 2016/10/24 17:59:38, pasko wrote: > > > If there is a ResetPredictor() somewhere in the test, what happens with an > > > EXPECT_CALL(*mock_observer_.get(), ...) that was issued before it? > > > > > > The approach with mock_tables_ seems to be working because this object is > not > > > swapped, just re-attached to the new ResourcePrefetchPredictor on every > > > ResetPredictor(). Seems like consistency here would eliminate unpleasant > > > surprises. Would be nice. > > > > Yeah, it's a kind of dangerous situation. > > Let's try to rewrite it. > > > > All control happens in a single thread so there won't be any race conditions > > between resets. > > > > I don't want reinvent EXPECT_CALL and pass expected arguments as parameters of > > the mock class constructor. > > this latter sgtm Done.
TestObserver renamed to TestDelegate because we don't support Observer's add/remove semantics and it looks more consistent with existing code.
On 2016/10/25 17:54:17, alexilin wrote: > TestObserver renamed to TestDelegate because we don't support Observer's > add/remove semantics and it looks more consistent with existing code. As far as I understand the convention, Delegates are used in Chromium when some part of the work of a class is _delegated_ to another class. This is not the case here, what this for-testing only class is doing is not helping the main class, rather just observing what is being done.
overall looking good with two non-trivial suggestions and some rant: 1. s/TestDelegate/TestObserver/ (or discuss, as usual) 2. move TestObserver out of ResourcePrefetchPredictor https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1187: DCHECK(observer->predictor_ == this); On 2016/10/25 15:10:58, alexilin wrote: > On 2016/10/25 14:05:40, pasko wrote: > > On 2016/10/25 11:38:09, alexilin wrote: > > > On 2016/10/24 17:59:38, pasko wrote: > > > > DCHECKs in tests should be avoided. Also, this check is done at the cost > of > > > > having, ResourcePrefetchPredictor as a friend to the observer, which > > > complicates > > > > understanding the interfaces of both. > > > > > > What's wrong with DCHECKs in tests? It's one way to make sure that code > works > > > correctly no matter whether it's test code or production. > > > > Style guide: Don't use these macros in tests, as they crash the test binary > and > > leave bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of > macros, > > which report failures gracefully and can continue running other tests. > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > Yes, you are right. > Although I can't say that this code is truly "test" code. I've found some places > in the codebase where DCHECK is called inside *ForTesting function but not > inside test file. > Evidence: > https://cs.chromium.org/chromium/src/base/threading/thread.cc?rcl=1477379968&... This is not too bad, but bad :) Within 4 lines of definition of various ForTesting functions, I found this many DCHECKs: shell> git grep -A 4 'ForTesting(.* {' | grep 'DCHECK(' | wc -l 54 Compared to overall amount of ForTesting definitions: shell> git grep 'ForTesting(.* {' -- '*.h' '*.cc' | wc -l 795 This fraction is not negligible, people do DCHECKs in test code, but I still think that removing it here is more beneficial than keeping it, and certainly more consistent with the codebase overall. https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:119: class TestDelegate { can this be moved outside of ResourcePrefetchPredictor? Accessing fields of the ResourcePrefetchPredictor is not necessary for the TestObserver (and even for delegate), so it would be good to keep this separation explicit. https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:119: class TestDelegate { I realized that this class can be forward-declared in resource_prefetch_predictor.h and declared for realz in resource_prefetch_predictor_test_util.h. This would require including resource_prefetch_predictor_test_util.h into resource_prefetch_predictor.cc, which is arguably not the nest thing to do. Up to you/lizeb@. If you guys like to keep this header extra clean of testing details, move to another header. (forward-declaring nested classes is also possible, look: https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...) https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:121: virtual ~TestDelegate(); A comment would be worth adding IMO: // De-registers itself from |predictor_| on destruction.
Concerning Delegate vs. Observer: Observers make some work too, not 'just observing what is being done'. The difference is how it's related with the responsibility of the main class. And definition of this could be tricky and it's a subject for varying interpretation. I don't see big difference between these meanings and I was focused on the fact, that classes usually support many observers. Moving TestObserver out needs to be discussed further. https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2440723002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1187: DCHECK(observer->predictor_ == this); On 2016/10/26 12:05:40, pasko wrote: > On 2016/10/25 15:10:58, alexilin wrote: > > On 2016/10/25 14:05:40, pasko wrote: > > > On 2016/10/25 11:38:09, alexilin wrote: > > > > On 2016/10/24 17:59:38, pasko wrote: > > > > > DCHECKs in tests should be avoided. Also, this check is done at the cost > > of > > > > > having, ResourcePrefetchPredictor as a friend to the observer, which > > > > complicates > > > > > understanding the interfaces of both. > > > > > > > > What's wrong with DCHECKs in tests? It's one way to make sure that code > > works > > > > correctly no matter whether it's test code or production. > > > > > > Style guide: Don't use these macros in tests, as they crash the test binary > > and > > > leave bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of > > macros, > > > which report failures gracefully and can continue running other tests. > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > > > Yes, you are right. > > Although I can't say that this code is truly "test" code. I've found some > places > > in the codebase where DCHECK is called inside *ForTesting function but not > > inside test file. > > Evidence: > > > https://cs.chromium.org/chromium/src/base/threading/thread.cc?rcl=1477379968&... > > This is not too bad, but bad :) > > Within 4 lines of definition of various ForTesting functions, I found this many > DCHECKs: > shell> git grep -A 4 'ForTesting(.* {' | grep 'DCHECK(' | wc -l > 54 > > Compared to overall amount of ForTesting definitions: > shell> git grep 'ForTesting(.* {' -- '*.h' '*.cc' | wc -l > 795 > > This fraction is not negligible, people do DCHECKs in test code, but I still > think that removing it here is more beneficial than keeping it, and certainly > more consistent with the codebase overall. Acknowledged. https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:119: class TestDelegate { On 2016/10/26 12:05:41, pasko wrote: > I realized that this class can be forward-declared in > resource_prefetch_predictor.h and declared for realz in > resource_prefetch_predictor_test_util.h. > > This would require including resource_prefetch_predictor_test_util.h into > resource_prefetch_predictor.cc, which is arguably not the nest thing to do. Up > to you/lizeb@. If you guys like to keep this header extra clean of testing > details, move to another header. > > (forward-declaring nested classes is also possible, look: > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...) I'll try to advocate existing implementation. For now we have nice naming ResourcePrefetchPredictor::TestObserver. It's actually common practice in Chromium to put observers/delegates (even TestObservers) inside the main class. Declaring TestObserver in resource_prefetch_predictor.h outside the main class looks polluting for predictors namespace. I support 'one header / one class' point of view. We could declare it inside resource_prefetch_predictor_test_util.h but when we should define it then? All definitions for functions inside resource_prefetch_predictor_test_util.h are in resource_prefetch_predictor_test_util.cc and this file is included only for unit_tests build. So we will have to include it in chrome build that will be more expensive and excessive. Another option that I could admit is to create new .h .cc specially for TestObserver. Though there already is ResourcePrefetchPredictorObserver class that's doing not related things and creation of ResorucePrefetchPredictorTestObserver could be confusing for readers. https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:121: virtual ~TestDelegate(); On 2016/10/26 12:05:41, pasko wrote: > A comment would be worth adding IMO: > // De-registers itself from |predictor_| on destruction. Done.
> Observers make some work too, not 'just observing what is being done'. The > difference is how it's related with the responsibility of the main class. And > definition of this could be tricky and it's a subject for varying > interpretation. I don't see big difference between these meanings and I was > focused on the fact, that classes usually support many observers. Still TestDelegate is slightly less obvious than TestObserver to me, since ResourcePrefetchPredictor does not really _delegate_ any piece of work to TestDelegate, it only notifies the latter. OK. I am not going to argue more. TestDelegate is OK with me, lizeb@ has the final word. https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:119: class TestDelegate { On 2016/10/26 13:09:59, alexilin wrote: > On 2016/10/26 12:05:41, pasko wrote: > > I realized that this class can be forward-declared in > > resource_prefetch_predictor.h and declared for realz in > > resource_prefetch_predictor_test_util.h. > > > > This would require including resource_prefetch_predictor_test_util.h into > > resource_prefetch_predictor.cc, which is arguably not the nest thing to do. Up > > to you/lizeb@. If you guys like to keep this header extra clean of testing > > details, move to another header. > > > > (forward-declaring nested classes is also possible, look: > > > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...) > > I'll try to advocate existing implementation. > For now we have nice naming ResourcePrefetchPredictor::TestObserver. I would argue that this long name is used only in a couple of .cc files, which keeps the value of the name small. The name predictors::TestObserver is good enough IMO. > It's actually common practice in Chromium to put observers/delegates (even > TestObservers) inside the main class. Being a common practice does not necessarily mean it is a good practice for this case. I do not see how consistency with this practice will make the code more readable. Consistency is good to maintain within a file, a component, but when it reaches Chromium-wide code, it is not really of a big value. For example, we constantly break consistency by using new C++ features in the new code, but not updating the old code accordingly. A good way to discover why observers are often nested classes is to ask chromium-dev@, there we can argue on what the best practice is. For now I cannot see such topic raised. > Declaring TestObserver in resource_prefetch_predictor.h outside the main class > looks polluting for predictors namespace. I support 'one header / one class' > point of view. predictors namespace is not large, no worries about that > We could declare it inside resource_prefetch_predictor_test_util.h but when we > should define it then? All definitions for functions inside > resource_prefetch_predictor_test_util.h are in > resource_prefetch_predictor_test_util.cc and this file is included only for > unit_tests build. So we will have to include it in chrome build that will be > more expensive and excessive. > Another option that I could admit is to create new .h .cc specially for > TestObserver. Though there already is ResourcePrefetchPredictorObserver class > that's doing not related things and creation of > ResorucePrefetchPredictorTestObserver could be confusing for readers. I think it is a good tradeoff to: (a) have two classes in one file, and (b) keeping the maximum separation between them. A common practice is actually to create an abstract class (in a separate file), then have an Impl class with common functionality (one line of code for us, in two files), and then two derived classes from that. For the functionality we have I think it would be a gross overkill (and we discussed this offline AFAIR). I am still convinced that moving this class out makes the code easier to read by not exposing private members of ResourcePrefetchPredictor to TestObserver.
Already renamed back to TestObserver. TestObserver is moved out of ResourcePrefetchPredictor. https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2440723002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:119: class TestDelegate { On 2016/10/26 13:48:35, pasko wrote: > On 2016/10/26 13:09:59, alexilin wrote: > > On 2016/10/26 12:05:41, pasko wrote: > > > I realized that this class can be forward-declared in > > > resource_prefetch_predictor.h and declared for realz in > > > resource_prefetch_predictor_test_util.h. > > > > > > This would require including resource_prefetch_predictor_test_util.h into > > > resource_prefetch_predictor.cc, which is arguably not the nest thing to do. > Up > > > to you/lizeb@. If you guys like to keep this header extra clean of testing > > > details, move to another header. > > > > > > (forward-declaring nested classes is also possible, look: > > > > > > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...) > > > > I'll try to advocate existing implementation. > > For now we have nice naming ResourcePrefetchPredictor::TestObserver. > > I would argue that this long name is used only in a couple of .cc files, which > keeps the value of the name small. The name predictors::TestObserver is good > enough IMO. > > > It's actually common practice in Chromium to put observers/delegates (even > > TestObservers) inside the main class. > > Being a common practice does not necessarily mean it is a good practice for this > case. I do not see how consistency with this practice will make the code more > readable. Consistency is good to maintain within a file, a component, but when > it reaches Chromium-wide code, it is not really of a big value. For example, we > constantly break consistency by using new C++ features in the new code, but not > updating the old code accordingly. > > A good way to discover why observers are often nested classes is to ask > chromium-dev@, there we can argue on what the best practice is. For now I cannot > see such topic raised. > > > Declaring TestObserver in resource_prefetch_predictor.h outside the main class > > looks polluting for predictors namespace. I support 'one header / one class' > > point of view. > > predictors namespace is not large, no worries about that > > > We could declare it inside resource_prefetch_predictor_test_util.h but when we > > should define it then? All definitions for functions inside > > resource_prefetch_predictor_test_util.h are in > > resource_prefetch_predictor_test_util.cc and this file is included only for > > unit_tests build. So we will have to include it in chrome build that will be > > more expensive and excessive. > > Another option that I could admit is to create new .h .cc specially for > > TestObserver. Though there already is ResourcePrefetchPredictorObserver class > > that's doing not related things and creation of > > ResorucePrefetchPredictorTestObserver could be confusing for readers. > > I think it is a good tradeoff to: (a) have two classes in one file, and (b) > keeping the maximum separation between them. A common practice is actually to > create an abstract class (in a separate file), then have an Impl class with > common functionality (one line of code for us, in two files), and then two > derived classes from that. For the functionality we have I think it would be a > gross overkill (and we discussed this offline AFAIR). > > I am still convinced that moving this class out makes the code easier to read by > not exposing private members of ResourcePrefetchPredictor to TestObserver. Done.
lgtm, thank you!
The CQ bit was checked by alexilin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexilin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org Link to the patchset: https://codereview.chromium.org/2440723002/#ps240001 (title: "Move TestObserver outside ResourcePrefetchPredictor class.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== predictors: Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 ========== to ========== predictors: Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 ========== to ========== predictors: Make ResourcePrefetchPredictor observable. This observer is needed for browser tests. + a small refactoring of ResourcePrefetchPredictor nested classes and function parameters. BUG=631966 Committed: https://crrev.com/fdf3eb2acfa3444030d3988b664d00325a3e109d Cr-Commit-Position: refs/heads/master@{#427707} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/fdf3eb2acfa3444030d3988b664d00325a3e109d Cr-Commit-Position: refs/heads/master@{#427707} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
