Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1473153002:
PageLoadMetricsObservers observe individual page loads (Closed)
|
Created:
5 years, 1 month ago by Charlie Harrison Modified:
5 years ago CC:
chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInstead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers observe individual page loads.
This allows observers to keep a much simpler state of the world, because they only get notifications about a single load.
This change forces observers lifetime to be scoped to a single page load.
BUG=546116
Committed: https://crrev.com/63ecf201ac56577842fd86039002e93e8b6d0ebc
Cr-Commit-Position: refs/heads/master@{#363480}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : #Patch Set 4 #Patch Set 5 : #Patch Set 6 : Observe on creation #
Total comments: 28
Patch Set 7 : responded to Randy's comments #Patch Set 8 : #
Total comments: 4
Patch Set 9 : Bryan review #
Total comments: 3
Patch Set 10 : rebase on #362343 #
Total comments: 4
Patch Set 11 : Observers are owned by the PageLoadTracker #Patch Set 12 : comments #Patch Set 13 : rebase and update for s-w-r experiment #
Total comments: 9
Patch Set 14 : Randy review: comment + simpler destructor #
Total comments: 6
Patch Set 15 : Randy/Bryan review: needed to revert ~MWCO destructor for lifecycle reasons #
Total comments: 1
Patch Set 16 : destroy order #Dependent Patchsets: Messages
Total messages: 46 (14 generated)
Description was changed from ========== PageLoadMetricObservers observe individual page loads BUG= ========== to ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change allows observers to observe individual page loads. This allows consumers to construct observer objects per main frame load, so they don't have to persist state or manage cases where multiple loads (e.g. one committed, one redirecting) are occurring at the same time. For observers that are not constructed on-demand (right now there aren't any), this change only expands the per-tab API. BUG= ==========
Description was changed from ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change allows observers to observe individual page loads. This allows consumers to construct observer objects per main frame load, so they don't have to persist state or manage cases where multiple loads (e.g. one committed, one redirecting) are occurring at the same time. For observers that are not constructed on-demand (right now there aren't any), this change only expands the per-tab API. BUG= ========== to ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change allows observers to observe individual page loads. This allows consumers to construct observer objects per main frame load, so they don't have to persist state or manage cases where multiple loads (e.g. one committed, one redirecting) are occurring at the same time. For observers that are not constructed on-demand (right now there aren't any), this change only expands the per-tab API. BUG=546116 ==========
csharrison@chromium.org changed reviewers: + bmcquade@chromium.org, rdsmith@chromium.org
PTAL. This change reflects what we talked about in our sync today. That is, we make the PageLoadTracker a PageLoadMetricsObservable instead of the MetricsWebContentsObserver. The PageLoadTracker registers its observers in its constructor using a hook provided by the embedder interface. I also implemented a default functionality of self-deleting/removing on PageLoadMetricsGoingAway.
On 2015/11/24 22:10:07, csharrison wrote: > PTAL. This change reflects what we talked about in our sync today. > That is, we make the PageLoadTracker a PageLoadMetricsObservable instead of the > MetricsWebContentsObserver. > > The PageLoadTracker registers its observers in its constructor using a hook > provided by the embedder interface. > > I also implemented a default functionality of self-deleting/removing on > PageLoadMetricsGoingAway. Also, if this design works for you both, then I'll re-implement the abort stuff on top of this.
So I like the basic goal (which I take to be to allow consumers to pay attention to a single page load as opposed to have to deal with the various confusions and interleavings that come in from multiple loads going on in the same tab). But I have some concern about the implementation, around ownership, some API issues, and class structure, which I cover in the below comments. (Feel free to ignore the nits; they're the kind of thing I find it easier to note than ignore, but the important part of this review is the architectural concerns. I can always come back to comment nits later :-}.) https://codereview.chromium.org/1473153002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:122: delete this; To me, this really violates the principle of least astonishment--before looking at this routine, there's no way I would have exepcted a routine named SetCommittedURLAndReferrer() to result in a deletion of the object. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:120: // CommittedRelevantLoadEvents are events that occur on committed loads that we nit: There's a preference in the net stack for avoiding use of the word "we" in comments. It's not always clear what the meaning is, especially for non-native english speakers. Can you substitute the class that's doing the tracking for "we" here? https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:200: class PageLoadTracker : public PageLoadMetricsObservable { I know it's not part of this CL, but why the distinction between interface and derived class here (or previously)? PageLoadMetricsObservable isn't a base class anywhere else that I can see (including tests) and I don't think (please correct if I'm wrong) the class usefully hides any implementation details from the consumers; it looks like it's just used within the implementation of the page_load_metrics. If I'm right in all that I'm inclined to think it's abstract without an effect other than making the code confusing to follow. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:221: bool RendererTracked() { return renderer_tracked_; } Note that by the Google style guide these may be named set_renderer_tracked() and renderer_tracked() (see https://google.github.io/styleguide/cppguide.html#Function_Names). It's not a "should" it's a "can", so it's up to you, but it's good to be aware of the option and of how surrounding code names things for consistency (I don't believe that applies here, but if it does, make sure that your usage pattern on these is consistent). https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:257: // List of observers. This must outlive the class. Is the "outlive" sentence still true/useful now that you've gotten rid of the indirection? https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.cc:27: delete this; I'm concerned about the confusion in ownership implied here. This object is, in some sense, self-owned; it deletes itself when the observable signals that it is going away. Except it's not; sometimes the derived classes destroy it in some other context. So is the object "self-owned" by the base class or the derived class? I think there's a reasonably good chance that future hacking in this code would trip over that confusion. I'd suggest instead making the destructor private (make it clear that it must be destroyed by a method of the class) and create a "Destroy()" method that does the above work, which is called from this method and may be called from derived classes. That would also allow nuking "GetObservable()", which feels like a weird thing anyway. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:42: // This constructor will add the object as an observable to the passed in nit: "observable" -> "observer"? https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:64: // load. This will happen some time after commit, the PageLoadTiming struct nit: I think this is better as two separate sentences (i.e. "," -> '." :-}). https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:72: // This is called when the metrics we are observing is tearing down. No nit: "the metrics ... is" reads funny in English. Maybe substitute "metrics" with "<classname> object"? https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:73: // further callbacks will be triggered. If your object has longer lifetime nit: "Your" is problematic for the same reason as "we". Maybe "If the observer has longer ..."? https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:74: // than the observable, override this method and call RemoveObserver() by this nit: "*it should* override this method and call RemoveObserver() *at* this point." (It's perfectly reasonable to call RemoveObserver before this point, but in that case the class isn't overriding this method.) https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:79: void Observe(PageLoadMetricsObservable* observable); I don't see this ever being called; why does it exist?
Thanks for the review, Randy. Let me know if this is what you had in mind for the Destroy() stuff. https://codereview.chromium.org/1473153002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:122: delete this; On 2015/11/28 22:03:13, rdsmith wrote: > To me, this really violates the principle of least astonishment--before looking > at this routine, there's no way I would have exepcted a routine named > SetCommittedURLAndReferrer() to result in a deletion of the object. Yeah I think I agree with you. We can ditch most of the changes to this class actually, I was just trying out a new approach where the observer stop observing a page load as soon as it stops being relevant. I think I may revert these changes to simplify the CL. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:120: // CommittedRelevantLoadEvents are events that occur on committed loads that we On 2015/11/28 22:03:13, rdsmith wrote: > nit: There's a preference in the net stack for avoiding use of the word "we" in > comments. It's not always clear what the meaning is, especially for non-native > english speakers. Can you substitute the class that's doing the tracking for > "we" here? Yupp. I'm trying to break the habit :P https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:200: class PageLoadTracker : public PageLoadMetricsObservable { On 2015/11/28 22:03:13, rdsmith wrote: > I know it's not part of this CL, but why the distinction between interface and > derived class here (or previously)? PageLoadMetricsObservable isn't a base > class anywhere else that I can see (including tests) and I don't think (please > correct if I'm wrong) the class usefully hides any implementation details from > the consumers; it looks like it's just used within the implementation of the > page_load_metrics. If I'm right in all that I'm inclined to think it's abstract > without an effect other than making the code confusing to follow. The reason we made PageLoadMetricsObservable was so consumers of the API are shielded from all the implementation details. Components outside of PageLoadMetrics are only given an object with AddObserver/RemoveObserver methods exposed. Let me know if you think there's a better architecture for this. I actually think it ended up being good. In this case we changed the observee from MetricsWebContentsObserver to PageLoadTracker without having to edit the observers. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:221: bool RendererTracked() { return renderer_tracked_; } On 2015/11/28 22:03:13, rdsmith wrote: > Note that by the Google style guide these may be named set_renderer_tracked() > and renderer_tracked() (see > https://google.github.io/styleguide/cppguide.html#Function_Names). It's not a > "should" it's a "can", so it's up to you, but it's good to be aware of the > option and of how surrounding code names things for consistency (I don't believe > that applies here, but if it does, make sure that your usage pattern on these is > consistent). Good point. I'll change it in this case. I changed one other method to fit this rule (GetCommittedURL => committed_url) https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:257: // List of observers. This must outlive the class. On 2015/11/28 22:03:13, rdsmith wrote: > Is the "outlive" sentence still true/useful now that you've gotten rid of the > indirection? Nope. Good catch. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.cc:27: delete this; On 2015/11/28 22:03:13, rdsmith wrote: > I'm concerned about the confusion in ownership implied here. This object is, in > some sense, self-owned; it deletes itself when the observable signals that it is > going away. Except it's not; sometimes the derived classes destroy it in some > other context. So is the object "self-owned" by the base class or the derived > class? I think there's a reasonably good chance that future hacking in this > code would trip over that confusion. > > I'd suggest instead making the destructor private (make it clear that it must be > destroyed by a method of the class) and create a "Destroy()" method that does > the above work, which is called from this method and may be called from derived > classes. That would also allow nuking "GetObservable()", which feels like a > weird thing anyway. Your suggestion sgtm, I might add a StopObserving() method for consumers that don't share a lifetime with the observable. However, I think it's forbidden to subclass a class with a private destructor, so I'm making it protected. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:42: // This constructor will add the object as an observable to the passed in On 2015/11/28 22:03:14, rdsmith wrote: > nit: "observable" -> "observer"? Done. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:64: // load. This will happen some time after commit, the PageLoadTiming struct On 2015/11/28 22:03:14, rdsmith wrote: > nit: I think this is better as two separate sentences (i.e. "," -> '." :-}). Done. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:72: // This is called when the metrics we are observing is tearing down. No On 2015/11/28 22:03:14, rdsmith wrote: > nit: "the metrics ... is" reads funny in English. Maybe substitute "metrics" > with "<classname> object"? Done. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:73: // further callbacks will be triggered. If your object has longer lifetime On 2015/11/28 22:03:14, rdsmith wrote: > nit: "Your" is problematic for the same reason as "we". Maybe "If the observer > has longer ..."? Done. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:74: // than the observable, override this method and call RemoveObserver() by this On 2015/11/28 22:03:14, rdsmith wrote: > nit: "*it should* override this method and call RemoveObserver() *at* this > point." (It's perfectly reasonable to call RemoveObserver before this point, > but in that case the class isn't overriding this method.) Done. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:79: void Observe(PageLoadMetricsObservable* observable); On 2015/11/28 22:03:14, rdsmith wrote: > I don't see this ever being called; why does it exist? This exists for consumers who want to observe without sharing a lifetime with the observable. So far none of the observers actually need this. I know there are some places in the codebase that are against API changes with no consumers, so I can remove this if you think it's unnecessary.
https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:113: : has_commit_(false), need to set renderer_tracked_ to false here https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.cc:22: observable_->AddObserver(this); I like having the default case be that a PLMObserver observes a single PLMObservable, with lifetime of the observer bound to the observable. Are we intending to support the use case where a single PLMObserver can observe multiple PLMObservables? Does Tarun's use case require that? If so, it seems that we should allow a caller to pass nullptr here, since the PLMObserver may be observing many PLMObservables concurrently (in cases where there are multiple page loads happening in parallel, e.g. a provisional and committed load in a single tab, or two page loads in different tabs). Overall, if we can require that each PLMObserver observes a single PLMObservable, and the PLMObserver's lifetime is scoped to the PLMObservable, I think that makes things simpler and easier to reason about, so I prefer it. But I want to make sure we can support all required use cases if we go that route.
https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:113: : has_commit_(false), On 2015/11/30 17:00:53, Bryan McQuade wrote: > need to set renderer_tracked_ to false here Done. https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/140001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.cc:22: observable_->AddObserver(this); On 2015/11/30 17:00:53, Bryan McQuade wrote: > I like having the default case be that a PLMObserver observes a single > PLMObservable, with lifetime of the observer bound to the observable. > > Are we intending to support the use case where a single PLMObserver can observe > multiple PLMObservables? Does Tarun's use case require that? If so, it seems > that we should allow a caller to pass nullptr here, since the PLMObserver may be > observing many PLMObservables concurrently (in cases where there are multiple > page loads happening in parallel, e.g. a provisional and committed load in a > single tab, or two page loads in different tabs). > > Overall, if we can require that each PLMObserver observes a single > PLMObservable, and the PLMObserver's lifetime is scoped to the PLMObservable, I > think that makes things simpler and easier to reason about, so I prefer it. But > I want to make sure we can support all required use cases if we go that route. Yeah a PLMObserver can observe multiple PLMObservables. In those cases, I guess I was assuming that the derived classes would not call this constructor, but one with no arguments. I'll add that with an additional comment.
https://codereview.chromium.org/1473153002/diff/160001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/160001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.cc:32: if (observable_) what happens if someone calls MWCO::RemoveObserver() directly, for a PLMObserver whose lifetime is scoped to the MWCO? In that case they'd also need to explicitly delete the PLMObserver, but I could imagine it being easy to forget to do that, which leads to a memory leak. It seems unfortunate that callers have to know to call Destroy() and that it is a subtle runtime error to call RemoveObserver() directly. It seems like MWCO::RemoveObserver() is the real signal to indicate that a PLMObserver is no longer associated with the MWCO, and for PLMObservers associated with a single PLMObservable, invoking RemoveObserver should result in deleting of the PLMObserver rather than having to call the alternative method Destroy(). Alternatively, if a caller must call Destroy(), they should not be allowed to call RemoveObserver() by mistake (but I'm not sure how to enforce that). Rethinking this slightly, perhaps these PLMObservers associated with a single PLMObservable should be 'owned' by the PLMObservable. Then, we could have two add observer mechanisms on PLMObservable: void AddObserver(PLMObserver* observer); void AddObserverAndTakeOwnership(scoped_ptr<PLMObserver> observer); When RemoveObserver() is called for a PLMObserver previously passed to AddObserverAndTakeOwnership, the MWCO would be responsible for deleting the owned PLMObserver at some reasonable time after RemoveObserver was invoked, such as via a call to base::MessageLoop::current()->DeleteSoon(FROM_HERE, observer);, or, if RemoveObserver is never called, in the MWCO destructor. PageLoadMetricsEmbedderInterfaceImpl::RegisterObservers() becomes something like: metrics->AddObserverAndTakeOwnership(make_scoped_ptr(new FromGWSPageLoadMetricsObserver())); ... You could take this a step further and make the protected convenience method PLMObserver::StopObserving() just call observable_->RemoveObserver(this), to continue to allow PLMObservers to easily stop observing without having to know about their associated PLMObservable. One small challenge is it's unclear how to tell the PLMObserver what its observable is, for the purpose of supporting StopObserver(). I could imagine having a PLMObserver::SetOwner(PLMObservable*) method, but it seems very odd to make that public, and the only class allowed to call it is PLMObservable, as part of the AddObserverAndTakeOwnership method. Given the intertwined relationship between PLMObserver and PLMObservable I do see this as a place where making PLMObservable a friend of PLMObserver (and making SetOwner private) seems reasonable, though if there is some way to accomplish these goals without friending, that's always preferrable. All this said, there are clearly some shortcomings with this approach (supporting StopObserving cleanly is one) but I think it's worth spending some time to consider whether moving to a model where the MWCO 'owns' PLMObservables that are scoped to the MWCO's lifetime could work. I think that more clearly documents the relationship/lifetimes between the 2 classes. Randy, given your earlier comments about least astonishment, lifetime, ownership, etc, I'm also interested to hear if you think this helps. In general, I feel that calling 'delete this;' is a hint that there might be an opportunity to better model object ownership. https://codereview.chromium.org/1473153002/diff/160001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/160001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:86: void Observe(PageLoadMetricsObservable* observable); I know WC exposes a similar method, but do we need a public Observe() method? I'm inclined to support 2 use cases: 1. simple: PLMObserver observes a single PLMObservable, which is specified in its constructor. PLMObserver's lifetime is scoped to the PLMObservable 2. more flexible: PLMObserver can observe any number of PLMObservables. Some external entity is responsible for managing the PLMObserver's lifetime. What do you think about getting rid of Observe(), or making it private if you see it as a useful helper for just this class to use? All this said, if we do something similar to my other comment, then I think this method can go away, since AddObserver* are the only mechanisms needed to observe.
https://codereview.chromium.org/1473153002/diff/160001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.cc (right): https://codereview.chromium.org/1473153002/diff/160001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.cc:32: if (observable_) On 2015/11/30 19:18:48, Bryan McQuade wrote: > what happens if someone calls MWCO::RemoveObserver() directly, for a PLMObserver > whose lifetime is scoped to the MWCO? In that case they'd also need to > explicitly delete the PLMObserver, but I could imagine it being easy to forget > to do that, which leads to a memory leak. It seems unfortunate that callers have > to know to call Destroy() and that it is a subtle runtime error to call > RemoveObserver() directly. > > It seems like MWCO::RemoveObserver() is the real signal to indicate that a > PLMObserver is no longer associated with the MWCO, and for PLMObservers > associated with a single PLMObservable, invoking RemoveObserver should result in > deleting of the PLMObserver rather than having to call the alternative method > Destroy(). Alternatively, if a caller must call Destroy(), they should not be > allowed to call RemoveObserver() by mistake (but I'm not sure how to enforce > that). > > Rethinking this slightly, perhaps these PLMObservers associated with a single > PLMObservable should be 'owned' by the PLMObservable. Then, we could have two > add observer mechanisms on PLMObservable: > > void AddObserver(PLMObserver* observer); > void AddObserverAndTakeOwnership(scoped_ptr<PLMObserver> observer); > > When RemoveObserver() is called for a PLMObserver previously passed to > AddObserverAndTakeOwnership, the MWCO would be responsible for deleting the > owned PLMObserver at some reasonable time after RemoveObserver was invoked, such > as via a call to base::MessageLoop::current()->DeleteSoon(FROM_HERE, observer);, > or, if RemoveObserver is never called, in the MWCO destructor. > > PageLoadMetricsEmbedderInterfaceImpl::RegisterObservers() becomes something > like: > metrics->AddObserverAndTakeOwnership(make_scoped_ptr(new > FromGWSPageLoadMetricsObserver())); > ... > > > You could take this a step further and make the protected convenience method > PLMObserver::StopObserving() just call observable_->RemoveObserver(this), to > continue to allow PLMObservers to easily stop observing without having to know > about their associated PLMObservable. > > One small challenge is it's unclear how to tell the PLMObserver what its > observable is, for the purpose of supporting StopObserver(). I could imagine > having a PLMObserver::SetOwner(PLMObservable*) method, but it seems very odd to > make that public, and the only class allowed to call it is PLMObservable, as > part of the AddObserverAndTakeOwnership method. Given the intertwined > relationship between PLMObserver and PLMObservable I do see this as a place > where making PLMObservable a friend of PLMObserver (and making SetOwner private) > seems reasonable, though if there is some way to accomplish these goals without > friending, that's always preferrable. > > All this said, there are clearly some shortcomings with this approach > (supporting StopObserving cleanly is one) but I think it's worth spending some > time to consider whether moving to a model where the MWCO 'owns' PLMObservables > that are scoped to the MWCO's lifetime could work. I think that more clearly > documents the relationship/lifetimes between the 2 classes. > > Randy, given your earlier comments about least astonishment, lifetime, > ownership, etc, I'm also interested to hear if you think this helps. In general, > I feel that calling 'delete this;' is a hint that there might be an opportunity > to better model object ownership. I really like having the Observable own the observers. In fact, my original implementation used a ScopedVector to track the observers. I'd be happy to go back to that for the simple case. You don't even need AddObserverAndTakeOwnership, just another AddObserver method that takes a scoped pointer. The fact that it takes a scoped pointer indicates it will take ownership. I don't think it's unreasonable to also make owned observers take the Observable in their constructor, so we don't need a SetOwner method. RE: Remove/StopObserving. The underlying problem is keeping track of the observable_ pointer. I think we should just document that classes have to call StopObserving when they want to stop observing. If an owned observer calls StopObserving, it will be deleted. I think it's reasonable given that the observable is a private member, and it is not exposed anywhere that you shouldn't directly call RemoveObserver().
Ok, so with a bit of cleanup I can see that my primary quarrel with this CL is around what I see as a confused ownership model for PageLoadMetricsObserver. Does it own itself? (Which is weird in its own right, but occasionally the right thing.) If so, who really owns it, the base class or the derived class? Or is it "really" owned by the Observable? The current implementation tries to allow (IIUC) for *all* of these options, which strikes me as an excellent way to confuse consumers and collect bugs as the implementation evolves. Let me suggest a Gordion knot slicing solution: The PageLoadMetricsObservable (actually, the PageLoadTracker) owns all PageLoadMetricsObservers, and that ownership is represented by a scoped vector. The main disadvantage is that you can't decouple the lifetime of the observer from that of the tracker. And that may be a minus (though you'd have a better chance of convincing me of that with a use case in hand; see comment :-}). But the advantages: * We can get rid of this whole issue of PageLoadMetricsObservable, since the initialize function now passes the derived instances of PageLoadMetricsObserver to PageLoadMetrics to own. * We can get rid of PageLoadMetricsGoingAway; that signal is now the destructor of the observer. * Not handling the decoupled lifetime case means we also lose a lot of somewhat confusing methods (all the PageLoadMetricsObserver constructors, Observe()) which is a plus along with the minus. Also note that if we ever did have a decoupled lifetime case, it's not clear how the initialization code would handle it currently, given that it's a static method called for each page load; how could external code insert an existing object in at that point? (It mighr require making the web_contents() accessible, which would require un-abstracting the PageLoadMetricObservable). Also note that it's perfectly possible for consuming code to gasket in a helper object to transmit notifications as they occur if they really need a decoupled lifetime (modulo the above external object insertion problem). The "standard" observer pattern is, AIUI, intended for cases where the object being observed is accessible in some fashion to external code, so that observers can be added and removed at will by that external code. I don't think that's the situation we're in, or one we want to head for. None of these are slam dunk arguments, but overall I think this approach would be much simpler. WDYT? https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:120: // CommittedRelevantLoadEvents are events that occur on committed loads that we On 2015/11/30 16:39:29, csharrison wrote: > On 2015/11/28 22:03:13, rdsmith wrote: > > nit: There's a preference in the net stack for avoiding use of the word "we" > in > > comments. It's not always clear what the meaning is, especially for > non-native > > english speakers. Can you substitute the class that's doing the tracking for > > "we" here? > > Yupp. I'm trying to break the habit :P But you didn't change the comment; is the habit stronger than you thought? :-} https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:200: class PageLoadTracker : public PageLoadMetricsObservable { On 2015/11/30 16:39:29, csharrison wrote: > On 2015/11/28 22:03:13, rdsmith wrote: > > I know it's not part of this CL, but why the distinction between interface and > > derived class here (or previously)? PageLoadMetricsObservable isn't a base > > class anywhere else that I can see (including tests) and I don't think (please > > correct if I'm wrong) the class usefully hides any implementation details from > > the consumers; it looks like it's just used within the implementation of the > > page_load_metrics. If I'm right in all that I'm inclined to think it's > abstract > > without an effect other than making the code confusing to follow. > > The reason we made PageLoadMetricsObservable was so consumers of the API are > shielded from all the implementation details. Components outside of > PageLoadMetrics are only given an object with AddObserver/RemoveObserver methods > exposed. Let me know if you think there's a better architecture for this. > > I actually think it ended up being good. In this case we changed the observee > from MetricsWebContentsObserver to PageLoadTracker without having to edit the > observers. So I disagree with this as an architectural choice, but not violently, so ok :-}. Having said that, let me just give voice to my bias in case there's something useful in it. * I don't expect the concrete class associated with PageLoadMetricsObservable to change very often; maybe if we get this CL wrong it'll change one more time? But even that would surprise me. So I don't think future proofing against changes to that concrete class is a strong argument. * I don't think there's a major goodness in the consumers not being able to see the object they're observing; there might even be value at some point in them having access to that object to be able to call other methods on it. They are consumers; in the abstract them seeing the object they are consuming makes sense. (In practice we've put everything on the observer interface, so this isn't necessary, I just don't consider it a bad thing). * "PageLoadMetricsObserveable" is a pretty abstract name--it doesn't really say what the thing is. As such, it represents just another concept for anyone reading and understanding this code to keep in their head. If there's a value to having that extra concept, that's ok, but in this case I don't see the value, so I see the extra class as just mental overhead. I actually do have a suggestion for a different architecture, but I'll put that in a top level comment. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:79: void Observe(PageLoadMetricsObservable* observable); On 2015/11/30 16:39:29, csharrison wrote: > On 2015/11/28 22:03:14, rdsmith wrote: > > I don't see this ever being called; why does it exist? > > This exists for consumers who want to observe without sharing a lifetime with > the observable. So far none of the observers actually need this. I know there > are some places in the codebase that are against API changes with no consumers, > so I can remove this if you think it's unnecessary. Yeah, if you would. I'm afraid I'm one of those places in the codebase :-}. https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:241: return; nit: Why not push this before the previous clause and skip the previous conditionalization on renderer_tracked()? https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:40: class PageLoadMetricsObserver { I'd like a comment about ownership, given that it's a subtle ownership model (I'd call it self-owned but tied to the PageLoadMetricsObservable lifetime. Alternatively, you could say that it's owned by the PageLoadMetricsObserveable, but in a slightly funky way that doesn't involve a scoped_ptr<> anywhere. Either way, it should be commented (unless I argue you into a different, simpler, ownership model; I'm not yet sure if I'm going to try :-}).
I'm in favor of forcing the observers to be owned by the PageLoadTracker. If a consumer wants to shim an object between us and their longer-lived object that's fine, and arguably the right decision code wise. If one observer is getting all these notifications, they must do the work we are already doing in keeping track of which navigations are going on, etc. This work PageLoadTracker gives you for free. A better solution for consumers in this way is to send notifications from their observer to the long lived object. Each shim observer has a perfect view of one single page load, rather than many notifications from many page loads that must be distilled manually. https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:120: // CommittedRelevantLoadEvents are events that occur on committed loads that we On 2015/12/01 22:34:03, rdsmith wrote: > On 2015/11/30 16:39:29, csharrison wrote: > > On 2015/11/28 22:03:13, rdsmith wrote: > > > nit: There's a preference in the net stack for avoiding use of the word "we" > > in > > > comments. It's not always clear what the meaning is, especially for > > non-native > > > english speakers. Can you substitute the class that's doing the tracking > for > > > "we" here? > > > > Yupp. I'm trying to break the habit :P > > But you didn't change the comment; is the habit stronger than you thought? :-} Oops :O fixed. https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:241: return; On 2015/12/01 22:34:03, rdsmith wrote: > nit: Why not push this before the previous clause and skip the previous > conditionalization on renderer_tracked()? Done.
I convinced myself that the PageLoadTracker should explicitly own the observers. To expand on my previous comment, here's a hypothetical: The consumer of the API wants to log UMA counting the number of redirects a navigation has, tied to some state in an external object. ## Previous approach with a long lived observer The observer must keep a hash map of navigation -> redirect count. The external observer still has to be careful about how it gets added as an observer, just at a tab level and not a page load level. ## New Approach The observer just keeps a counter and increments it OnRedirect. It logs UMA if counter > 0. Instead of adding the external object as an observer, they just tracker->AddObserver(make_scoped_ptr(new Observer(passed_in_external_object))) https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:40: class PageLoadMetricsObserver { On 2015/12/01 22:34:03, rdsmith wrote: > I'd like a comment about ownership, given that it's a subtle ownership model > (I'd call it self-owned but tied to the PageLoadMetricsObservable lifetime. > Alternatively, you could say that it's owned by the PageLoadMetricsObserveable, > but in a slightly funky way that doesn't involve a scoped_ptr<> anywhere. > Either way, it should be commented (unless I argue you into a different, > simpler, ownership model; I'm not yet sure if I'm going to try :-}). Done.
+1 to making PLMObservers owned by PLMObservables. Thanks Charles and Randy! On Tue, Dec 1, 2015 at 5:54 PM <csharrison@chromium.org> wrote: > I'm in favor of forcing the observers to be owned by the PageLoadTracker. > If a > consumer wants to shim an object between us and their longer-lived object > that's > fine, and arguably the right decision code wise. > > If one observer is getting all these notifications, they must do the work > we are > already doing in keeping track of which navigations are going on, etc. This > work > PageLoadTracker gives you for free. > > A better solution for consumers in this way is to send notifications from > their > observer to the long lived object. Each shim observer has a perfect view of > one > single page load, rather than many notifications from many page loads that > must > be distilled manually. > > > > https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... > File > components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > > https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.h:120 > <https://codereview.chromium.org/1473153002/diff/100001/components/page_load_m...> > : > // CommittedRelevantLoadEvents are events that occur on committed loads > that we > On 2015/12/01 22:34:03, rdsmith wrote: > > On 2015/11/30 16:39:29, csharrison wrote: > > > On 2015/11/28 22:03:13, rdsmith wrote: > > > > nit: There's a preference in the net stack for avoiding use of the > word "we" > > > in > > > > comments. It's not always clear what the meaning is, especially > for > > > non-native > > > > english speakers. Can you substitute the class that's doing the > tracking > > for > > > > "we" here? > > > > > > Yupp. I'm trying to break the habit :P > > > But you didn't change the comment; is the habit stronger than you > thought? :-} > > Oops :O fixed. > > > https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... > File > components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > > https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:241 > <https://codereview.chromium.org/1473153002/diff/180001/components/page_load_m...> > : > return; > On 2015/12/01 22:34:03, rdsmith wrote: > > nit: Why not push this before the previous clause and skip the > previous > > conditionalization on renderer_tracked()? > > Done. > > https://codereview.chromium.org/1473153002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Very nice. Thank you for putting up with me. Could you bring the CL description up to date with the current code? I may have some nits after that, but let me nit the new writeup rather than the old one. https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc (right): https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:22: class TestFromGWSPageLoadMetricsObserver Remind me: What's the unit test pattern we recommend for all the different observers? My memory was that there were supposed to be tests for each observer in this file, but it seems like we're only using FromGWS. Is that because we're only testing generic functionality? (In which case in the ideal world we'd have a pure test observer.) Or are we missing tests? https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.h (right): https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.h:23: class PageLoadMetricsEmbedderInterfaceImpl nit, not part of this CL, suggestion: ...InterfaceImpl seems like overkill. Maybe just PageLoadMetricsEmbedder? https://codereview.chromium.org/1473153002/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:400: provisional_loads_.clear(); Are these necessary anymore? Can regular destruction take care of this now? https://codereview.chromium.org/1473153002/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1473153002/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:204: // Caller must guarantee that the embedder_interface pointers outlives this nit: pointer (singular).
Description was changed from ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change allows observers to observe individual page loads. This allows consumers to construct observer objects per main frame load, so they don't have to persist state or manage cases where multiple loads (e.g. one committed, one redirecting) are occurring at the same time. For observers that are not constructed on-demand (right now there aren't any), this change only expands the per-tab API. BUG=546116 ========== to ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers to observe individual page loads. This allows observers to keep a much simpler state of the world, because they only get notifications about a single load. This change forces observers lifetime to be scoped to a single page load. For objects that wish to observe multiple page loads, they must pass messages from observer objects to their object. BUG=546116 ==========
Updated the description. Thanks! https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc (right): https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:22: class TestFromGWSPageLoadMetricsObserver On 2015/12/03 01:20:42, rdsmith wrote: > Remind me: What's the unit test pattern we recommend for all the different > observers? My memory was that there were supposed to be tests for each observer > in this file, but it seems like we're only using FromGWS. Is that because we're > only testing generic functionality? (In which case in the ideal world we'd have > a pure test observer.) Or are we missing tests? We're moving to a test file per observer. Plus, we're adding pure observer specific tests. You could say we're missing tests, but my approach was that observers not owned by us (i.e. google captcha + s-w-r) can do what they please. https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.h (right): https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.h:23: class PageLoadMetricsEmbedderInterfaceImpl On 2015/12/03 01:20:42, rdsmith wrote: > nit, not part of this CL, suggestion: ...InterfaceImpl seems like overkill. > Maybe just PageLoadMetricsEmbedder? Sounds fine by me. Want me to make the change in this CL? https://codereview.chromium.org/1473153002/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:400: provisional_loads_.clear(); On 2015/12/03 01:20:42, rdsmith wrote: > Are these necessary anymore? Can regular destruction take care of this now? Nope. Good catch.
A couple of suggestions WRT the CL description, then LGTM. * "makes observers to observe individual" -> "makes observers observe individual" * "For objects that wish to observe multiple page loads": I'd suggest phrasing the following sentence as "they must create gasket observers that send messages to the original object" or something like that. WRT that last point: Is this model currently possible? I don't see how a pointer to any other object can be gotten from the RegisterObservers routine, so where would the messages be sent? If there's really no way to support this model, we're just teasing by gesturing at how clients would be able to do it in some alternative universe. https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc (right): https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:22: class TestFromGWSPageLoadMetricsObserver On 2015/12/03 13:35:35, csharrison wrote: > On 2015/12/03 01:20:42, rdsmith wrote: > > Remind me: What's the unit test pattern we recommend for all the different > > observers? My memory was that there were supposed to be tests for each > observer > > in this file, but it seems like we're only using FromGWS. Is that because > we're > > only testing generic functionality? (In which case in the ideal world we'd > have > > a pure test observer.) Or are we missing tests? > > We're moving to a test file per observer. Plus, we're adding pure observer > specific tests. You could say we're missing tests, but my approach was that > observers not owned by us (i.e. google captcha + s-w-r) can do what they please. Yeah, except we're probably the only review they're going to get because they're putting their code in our directory, so we may want to be in the habit of at least raising the issue of testing; there's no one else guarding the gate. (No action requested on the CL; this is a meta-issue.) https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.h (right): https://codereview.chromium.org/1473153002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.h:23: class PageLoadMetricsEmbedderInterfaceImpl On 2015/12/03 13:35:35, csharrison wrote: > On 2015/12/03 01:20:42, rdsmith wrote: > > nit, not part of this CL, suggestion: ...InterfaceImpl seems like overkill. > > Maybe just PageLoadMetricsEmbedder? > > Sounds fine by me. Want me to make the change in this CL? As you wish.
Description was changed from ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers to observe individual page loads. This allows observers to keep a much simpler state of the world, because they only get notifications about a single load. This change forces observers lifetime to be scoped to a single page load. For objects that wish to observe multiple page loads, they must pass messages from observer objects to their object. BUG=546116 ========== to ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers observe individual page loads. This allows observers to keep a much simpler state of the world, because they only get notifications about a single load. This change forces observers lifetime to be scoped to a single page load. BUG=546116 ==========
https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:26: bool navigation_from_gws_; can this be private? https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.cc (right): https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:55: RecordGoogleCaptchaEvent(GOOGLE_CAPTCHA_SOLVED); can you loop mdw in to review just this file? https://codereview.chromium.org/1473153002/diff/260001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/260001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:41: return navigation_handle->IsInMainFrame() && I'm wondering if we should be including some or part of this logic in page_load_metrics_util for the observers to use. * the logic that decides if the browser_url is non-HTTPS? seems important for filtering out NTP * IsSamePage filtering seems important (do any observers want same page navigations?) * IsErrorPage seems like it might be important, though I guess all 3 current observers filter on URL and thus should exclude these * non-HTML filtering seems important Do any of our current observers want to track stats for any of these types of pages?
csharrison@chromium.org changed reviewers: + mdw@chromium.org
Randy: I had to revert the change to ~MetricsWebContentsObserver, because we need to be sure |embedded_interface_| is destroyed after the PageLoadTrackers. Sorry my bad for not testing this :/ I added a comment explaining why this is needed. Matt: Can you review your observer changes? I added the SOLVED code to the Redirect case to be a bit more explicit. Setting saw_solution_ will only affect a single page load, so it won't corrupt subsequent loads. https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:26: bool navigation_from_gws_; On 2015/12/03 22:10:36, Bryan McQuade wrote: > can this be private? Yep. Done. https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.cc (right): https://codereview.chromium.org/1473153002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:55: RecordGoogleCaptchaEvent(GOOGLE_CAPTCHA_SOLVED); On 2015/12/03 22:10:36, Bryan McQuade wrote: > can you loop mdw in to review just this file? Yep https://codereview.chromium.org/1473153002/diff/260001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/260001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:41: return navigation_handle->IsInMainFrame() && On 2015/12/03 22:10:36, Bryan McQuade wrote: > I'm wondering if we should be including some or part of this logic in > page_load_metrics_util for the observers to use. > > * the logic that decides if the browser_url is non-HTTPS? seems important for > filtering out NTP > * IsSamePage filtering seems important (do any observers want same page > navigations?) > * IsErrorPage seems like it might be important, though I guess all 3 current > observers filter on URL and thus should exclude these > * non-HTML filtering seems important > > Do any of our current observers want to track stats for any of these types of > pages? Let's expose it when we need to, I don't think any of the current observers need it. As is, the fact that PageLoadTiming information is / is not there is a proxy for this. FYI I'm adding an additional check in the GoogleCaptcha observer for same-page. In general, I don't think any observer is going to have the same constraints, so it's best to give them something that just works in the general case (timing information), but has the necessary information for general purpose filters.
> Matt: Can you review your observer changes? I added the SOLVED code to the > Redirect case to be a bit more explicit. Setting saw_solution_ will only affect > a single page load, so it won't corrupt subsequent loads. Yes, I will review, but please hold off landing this change until I get a chance to manually test that it works correctly. Will do that soon.
lgtm
On 2015/12/03 22:43:37, Bryan McQuade wrote: > lgtm Matt: sgtm, thanks!
On 2015/12/03 23:28:53, csharrison wrote: > On 2015/12/03 22:43:37, Bryan McQuade wrote: > > lgtm > > Matt: sgtm, thanks! I'll try to have this done first thing tomorrow morning (if not sooner).
Tested and works fine - lgtm
Thanks, everyone. Randy, I removed the comment about longer-lived observers for now.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1473153002/#ps280001 (title: "Randy/Bryan review: needed to revert ~MWCO destructor for lifecycle reasons")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473153002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473153002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
csharrison@chromium.org changed reviewers: + isherman@chromium.org
isherman@, can you take a look at the minor change to histograms.xml?
histograms.xml lgtm
https://codereview.chromium.org/1473153002/diff/280001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1473153002/diff/280001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:398: // The PageLoadTrackers must be deleted before the |embedded_interface_|. nit: I've value a suffixed "because they contain a pointer to the |embedded_interface|. suggestion: This could also be managed by changing the order of members in the class declaration (and I believe often is, in Chrome). Data members are constructed first-to-last and destroyed last-to-first. A comment (there, in this case) would still be appropriate.
I used your suggestion Randy, thanks!
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, mdw@chromium.org, isherman@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1473153002/#ps300001 (title: "destroy order")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473153002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473153002/300001
Message was sent while issue was closed.
Description was changed from ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers observe individual page loads. This allows observers to keep a much simpler state of the world, because they only get notifications about a single load. This change forces observers lifetime to be scoped to a single page load. BUG=546116 ========== to ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers observe individual page loads. This allows observers to keep a much simpler state of the world, because they only get notifications about a single load. This change forces observers lifetime to be scoped to a single page load. BUG=546116 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers observe individual page loads. This allows observers to keep a much simpler state of the world, because they only get notifications about a single load. This change forces observers lifetime to be scoped to a single page load. BUG=546116 ========== to ========== Instead of PageLoadMetricsObservers observing load events occurring in a tab, this change makes observers observe individual page loads. This allows observers to keep a much simpler state of the world, because they only get notifications about a single load. This change forces observers lifetime to be scoped to a single page load. BUG=546116 Committed: https://crrev.com/63ecf201ac56577842fd86039002e93e8b6d0ebc Cr-Commit-Position: refs/heads/master@{#363480} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/63ecf201ac56577842fd86039002e93e8b6d0ebc Cr-Commit-Position: refs/heads/master@{#363480} |
