Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1136523004:
[Sessions] Add detailed logging of SessionRestore events. (Closed)
|
Created:
5 years, 7 months ago by chrisha Modified:
5 years, 6 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, asvitkine+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis makes SessionRestoreStatsCollector aware of 'deferred' tabs and adds detailed logging of SessionRestore events.
BUG=472772
Committed: https://crrev.com/f3c87f3238e503d995f3690d592d454715ff9cea
Cr-Commit-Position: refs/heads/master@{#335413}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Refactored per sky@'s suggestion. #
Total comments: 29
Patch Set 3 : Refactor and rebase. #
Total comments: 18
Patch Set 4 : Sweeping refactor. #
Total comments: 40
Patch Set 5 : Address review comments. #
Total comments: 7
Patch Set 6 : Pulled class state transition logic to a helper function. #Patch Set 7 : Now with unittests! #Patch Set 8 : Fixed a few typos. #Patch Set 9 : Rebased. #
Total comments: 12
Patch Set 10 : Address comments, remove use of gmock. #Patch Set 11 : Reformatted. #
Total comments: 12
Patch Set 12 : Addressed nits. #
Total comments: 20
Patch Set 13 : Address asvitkine's comments. #
Total comments: 2
Patch Set 14 : Fix typo that breaks compile. #Patch Set 15 : Fix clang warnings. #Patch Set 16 : Fix for unittests involving back/forward navigation. #Patch Set 17 : Fix broken ChromeOS ExtensionEventObserverTests. #Patch Set 18 : Fix a browser test, add another unit test. #Patch Set 19 : Fix new unittest. #Patch Set 20 : Fix typo. #Messages
Total messages: 62 (20 generated)
chrisha@chromium.org changed reviewers: + sky@chromium.org
PTAL? sky, a sanity check of the basic approach and concept here? Particularly curious if you think there's a better place for me to grab the "deferred tab loaded due to user action" event than what I have in NavigationControllerImpl. Thanks!
It seems like all of our session restore metrics are questionable once the network goes offline or we get memory pressure. I suspect we may want to only record time based ones if we don't hit either of these. https://codereview.chromium.org/1136523004/diff/1/chrome/browser/sessions/ses... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/1136523004/diff/1/chrome/browser/sessions/ses... chrome/browser/sessions/session_restore.cc:186: UMA_HISTOGRAM_COUNTS_100(content::kSessionRestoreTabCount, Please try to keep all stats collection in SessionRestoreStatsCollector.
Moving the finch experiment to a separate CL: https://codereview.chromium.org/1134783002/ Will follow up with a modification of the stats collector on this CL.
Okay, all of the metrics have been moved to the stats collector. Most everything can be inferred internally, but I needed to add a 'hook' that the session recall calls to notify the stats collector that a tab loading was deferred. There remains some cleanup to do, but I was hoping for your opinion regarding the basic approach and stats being tracked.
session_restore_stats_collector is complex enough now that it warrants tests. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:30: kSessionRestoreActionsUma_Initiated = 0, SESSION_RESTORE_ACTION_UMA_INITIATED (see chrome style guide). https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:79: // stats collector will always exist. DCHECK(shared_collector_) https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:179: UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstLoaded", Seems like we should never log this if any tab was deferred. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:340: Would be nice to have some DCHECKs here that tab is in the right list. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:349: if (tabs_tracked_.empty() && render_widget_hosts_loading_.empty()) { Lets say tabs_tracked_ goes empty, adoesn't that mean you're going to hit this code n more times where n is the number of deferred tabs? https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:370: NavigationController* tab) { can you make this take session_restore_id_ so that it's clearer the callsite should be supplying the id. This way if someone wants to call RegisterForNotifications() at some random later date they have to think about what the id is. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:394: if (tabs_deferred_.count(tab) > 0) nit: return tabs_deffered_.count(tab) > 0 https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.cc:52: void TabLoader::SetTabLoadingEnabled(bool enable_tab_loading) { This likely needs to invalidate a bunch of histograms too.
Okay, this has been significantly refactored. Still could use tests, but I'm going to hold off on that until we agree upon the basic structure. PTAnotherL? https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:30: kSessionRestoreActionsUma_Initiated = 0, On 2015/05/12 15:27:56, sky wrote: > SESSION_RESTORE_ACTION_UMA_INITIATED (see chrome style guide). Done. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:79: // stats collector will always exist. On 2015/05/12 15:27:56, sky wrote: > DCHECK(shared_collector_) Done. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:179: UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstLoaded", On 2015/05/12 15:27:56, sky wrote: > Seems like we should never log this if any tab was deferred. I'm not sure I agree... there will always be at least one tab loaded (there will be at least one visible tab), so why wouldn't we emit this? https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:340: On 2015/05/12 15:27:56, sky wrote: > Would be nice to have some DCHECKs here that tab is in the right list. I moved to using a map that tracks the 'deferred' state. Made the logic much easier pretty much everywhere. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:349: if (tabs_tracked_.empty() && render_widget_hosts_loading_.empty()) { On 2015/05/12 15:27:56, sky wrote: > Lets say tabs_tracked_ goes empty, adoesn't that mean you're going to hit this > code n more times where n is the number of deferred tabs? Yes, that's true. This is no worse than before in that it will be hit once for each tab involved in the session restore. Deferred tabs are a subset of those. This is also made a little clearer by moving to a single tabs_tracked_ map. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:370: NavigationController* tab) { On 2015/05/12 15:27:56, sky wrote: > can you make this take session_restore_id_ so that it's clearer the callsite > should be supplying the id. This way if someone wants to call > RegisterForNotifications() at some random later date they have to think about > what the id is. After hashing this out with Georges, we really don't need the whole session restore ID. I'm now philosophically on board with the whole "multiple session restores happening at once is seen as a single session restore event". However, the deferred tabs now create a longer lifespan for the stats collector, which could cause it to start collecting stats again for subsequent session restores that *aren't* coincident with the first ones. To deal with this I've moved to a 2 stage end-of-life management: 1. When only deferred tabs remain to be tracked, the stats collector detaches itself from the shared_collector_. This allows a new collector to be created for new session restores. 2. When no tabs at all remain to be tracked, the collector deletes itself as before. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:394: if (tabs_deferred_.count(tab) > 0) On 2015/05/12 15:27:56, sky wrote: > nit: return tabs_deffered_.count(tab) > 0 (I have a tendency to write code this way because it means I can easily put a breakpoint on either of the two conditions. Philosophical argument, but in this particular case I don't care either way.) Done. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.cc:52: void TabLoader::SetTabLoadingEnabled(bool enable_tab_loading) { On 2015/05/12 15:27:56, sky wrote: > This likely needs to invalidate a bunch of histograms too. No more information is provided at the site of this call then is provided via the 'DeferTab' calls below. Any histograms that need to be invalidated can be done so entirely inside of the stats collector. What histograms did you have in mind, exactly? https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:297: // TODO(chrisha): Is it worth explicitly tracking these events as well? Consider this a question... your thoughts?
https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:179: UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstLoaded", On 2015/05/13 21:18:02, chrisha wrote: > On 2015/05/12 15:27:56, sky wrote: > > Seems like we should never log this if any tab was deferred. > > I'm not sure I agree... there will always be at least one tab loaded (there will > be at least one visible tab), so why wouldn't we emit this? Two cases to consider: . what happens when you close tabs that are loading before they stop? . what happens if the network is unavailable? https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:349: if (tabs_tracked_.empty() && render_widget_hosts_loading_.empty()) { On 2015/05/13 21:18:02, chrisha wrote: > On 2015/05/12 15:27:56, sky wrote: > > Lets say tabs_tracked_ goes empty, adoesn't that mean you're going to hit this > > code n more times where n is the number of deferred tabs? > > Yes, that's true. This is no worse than before in that it will be hit once for > each tab involved in the session restore. Deferred tabs are a subset of those. Not sure there. I think you're change makes it worse, but that's separate. We should only report this once. > This is also made a little clearer by moving to a single tabs_tracked_ map. Yes, I think that will help. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:370: NavigationController* tab) { On 2015/05/13 21:18:02, chrisha wrote: > On 2015/05/12 15:27:56, sky wrote: > > can you make this take session_restore_id_ so that it's clearer the callsite > > should be supplying the id. This way if someone wants to call > > RegisterForNotifications() at some random later date they have to think about > > what the id is. > > After hashing this out with Georges, we really don't need the whole session > restore ID. I'm now philosophically on board with the whole "multiple session > restores happening at once is seen as a single session restore event". I have to think about that more. I'm concerned that if the two don't start close enough some of these metrics are going to be misleading. > However, the deferred tabs now create a longer lifespan for the stats collector, > which could cause it to start collecting stats again for subsequent session > restores that *aren't* coincident with the first ones. To deal with this I've > moved to a 2 stage end-of-life management: > > 1. When only deferred tabs remain to be tracked, the stats collector detaches > itself from the shared_collector_. This allows a new collector to be created for > new session restores. > 2. When no tabs at all remain to be tracked, the collector deletes itself as > before. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.cc:52: void TabLoader::SetTabLoadingEnabled(bool enable_tab_loading) { On 2015/05/13 21:18:02, chrisha wrote: > On 2015/05/12 15:27:56, sky wrote: > > This likely needs to invalidate a bunch of histograms too. > > No more information is provided at the site of this call then is provided via > the 'DeferTab' calls below. Any histograms that need to be invalidated can be > done so entirely inside of the stats collector. > > What histograms did you have in mind, exactly? My concern is that any time based histograms are now going to be delayed until tab loading turns back on. For example, the time to paint would be totally misleading.
As I commented previously I think all the actual UMA calls should be consolidated. What I would like to see is SessionRestoreStatsCollector responsible for generating a struct that gives all the details needed to do the individual UMA calls. This way you can easily test this class (it's complex enough that it warrants tests). You could still keep the UMA calls in SessionRestoreStatsCollector, but do it based on the struct that is generated with all the stats to record. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_delegate.cc (right): https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_delegate.cc:93: SessionRestoreStatsCollector::DeferTab(tab_controller); I would rather not expose a bunch of static functions on SessionRestoreStatsController. It starts getting harder to reason and know about state. Could you pass in the set of deferred tabs to TrackTabs? Just one function that way. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:5: // Declares a utility class for tracking tabs involved in a session restore. Move this description above class description. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:18: // collectors may exist simultaneously, however at most one may be the active It's not clear what 'active' means here. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:22: // closed, the session restore will destroy itself. 'session restore' -> stats collector. But it appears you aren't consistent in your comments here, eg "session restore stats tracker" on 7, "collector" on 10... I recommend using SessionRestoreStatsCollector everywhere. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:45: public base::RefCounted<SessionRestoreStatsCollector> { There isn't a need for this class to be ref counted anymore. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:62: using RenderWidgetHostMap = std::map<content::RenderWidgetHost*, bool>; Bools can be a bit mysterious, where as enums are generally less confusing. Could you use an enum here? https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:113: // The following 3 maps are used to track tabs at different points in their Seems like you're tracking a bunch of state per tab in numerous maps that would be better served by a dedicated structure with a map from tab to state, eg: struct TabState { bool was_deferred; A bool is fine here as you always reference via .was_deferred // I'm not positive you need both of these, an enum may better capture the state. bool waiting_for_paint; bool is_loading; }; https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:147: // tabs_tracked_ map. Generally we use | around variable and parameter names in comments, eg |tabs_tracked_|.
On 2015/05/14 15:47:05, sky wrote: > As I commented previously I think all the actual UMA calls should be > consolidated. What I would like to see is SessionRestoreStatsCollector > responsible for generating a struct that gives all the details needed to do the > individual UMA calls. This way you can easily test this class (it's complex > enough that it warrants tests). You could still keep the UMA calls in > SessionRestoreStatsCollector, but do it based on the struct that is generated > with all the stats to record. Yeah, this seems like a great idea. I also like your suggestion to simply have one single map of tabs and to track them via state enumeration. (Rewriting this from scratch, this is what I would do.) The 3-set thing is simply because that's how it was done before. I'm not introducing new notions of states for tabs (unloaded, loading, loaded, painted), I simply inferred these states from a thorough reading of the code. I've addressed some of your deeper questions inline. I'll wait for answers on these before cleaning things up much more. Note that there's no particular rush for this, as it won't make M44 regardless :)
Did you push your comments? On Thu, May 14, 2015 at 12:38 PM, <chrisha@chromium.org> wrote: > On 2015/05/14 15:47:05, sky wrote: >> >> As I commented previously I think all the actual UMA calls should be >> consolidated. What I would like to see is SessionRestoreStatsCollector >> responsible for generating a struct that gives all the details needed to >> do > > the >> >> individual UMA calls. This way you can easily test this class (it's >> complex >> enough that it warrants tests). You could still keep the UMA calls in >> SessionRestoreStatsCollector, but do it based on the struct that is >> generated >> with all the stats to record. > > > Yeah, this seems like a great idea. I also like your suggestion to simply > have > one single map of tabs and to track them via state enumeration. (Rewriting > this > from scratch, this is what I would do.) The 3-set thing is simply because > that's > how it was done before. I'm not introducing new notions of states for tabs > (unloaded, loading, loaded, painted), I simply inferred these states from a > thorough reading of the code. > > I've addressed some of your deeper questions inline. I'll wait for answers > on > these before cleaning things up much more. Note that there's no particular > rush > for this, as it won't make M44 regardless :) > > https://codereview.chromium.org/1136523004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Silly me, didn't push my comments :/ https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:179: UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstLoaded", On 2015/05/14 00:12:40, sky wrote: > On 2015/05/13 21:18:02, chrisha wrote: > > On 2015/05/12 15:27:56, sky wrote: > > > Seems like we should never log this if any tab was deferred. > > > > I'm not sure I agree... there will always be at least one tab loaded (there > will > > be at least one visible tab), so why wouldn't we emit this? > > Two cases to consider: > . what happens when you close tabs that are loading before they stop? > . what happens if the network is unavailable? These are concerns regardless of whether or not there are deferred tabs. The stats collector doesn't do anything different now than it did for deferred tabs. If we didn't change the stats collector to understand them and emit a few more events, a deferred tab is indistinguishable from a tab that was loaded by the tab loader arbitrarily far after the session restore initiated. So if you want to address these specific issues, let's deal with them separately. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:349: if (tabs_tracked_.empty() && render_widget_hosts_loading_.empty()) { On 2015/05/14 00:12:40, sky wrote: > On 2015/05/13 21:18:02, chrisha wrote: > > On 2015/05/12 15:27:56, sky wrote: > > > Lets say tabs_tracked_ goes empty, adoesn't that mean you're going to hit > this > > > code n more times where n is the number of deferred tabs? > > > > Yes, that's true. This is no worse than before in that it will be hit once for > > each tab involved in the session restore. Deferred tabs are a subset of those. > > Not sure there. I think you're change makes it worse, but that's separate. We > should only report this once. The 'deferred state' of a tab doesn't change it's lifetime, nor its flow through the various maps/sets that the stats collector tracks. At its heart, a tab goes through a few simple states: - not loading - loading - loaded - painted (if we bother to track this) RemoveTab can only be called on a tab at most once, so no more processing is done here. The number of tabs entering the stats collector via TrackTabs remains unchanged, so no extra work is done there. Calls to DeferTab only modify the state of a tab that is already being tracked. This represents a minor amount of extra work, O(1) per deferred tab, to a max of O(n). The deferred state is only used to change the exact type of UMA records emitted, and to decide when to detach from being the shared collector. The *lifetime* of the shared collector is significantly longer, and this has memory overhead, but no more entries into the stats collector in the worst case than already exist. Since the lifetime is much longer many more useless UPDATE_BACKING_STORE calls could be seen. However, to deal with this I've disabled these callbacks as soon as first paint has been observed. By the time the stats collector is detached from being the shared collector, it will be receiving no more of these events. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:370: NavigationController* tab) { On 2015/05/14 00:12:40, sky wrote: > On 2015/05/13 21:18:02, chrisha wrote: > > On 2015/05/12 15:27:56, sky wrote: > > > can you make this take session_restore_id_ so that it's clearer the callsite > > > should be supplying the id. This way if someone wants to call > > > RegisterForNotifications() at some random later date they have to think > about > > > what the id is. > > > > After hashing this out with Georges, we really don't need the whole session > > restore ID. I'm now philosophically on board with the whole "multiple session > > restores happening at once is seen as a single session restore event". > > I have to think about that more. I'm concerned that if the two don't start close > enough some of these metrics are going to be misleading. > > > However, the deferred tabs now create a longer lifespan for the stats > collector, > > which could cause it to start collecting stats again for subsequent session > > restores that *aren't* coincident with the first ones. To deal with this I've > > moved to a 2 stage end-of-life management: > > > > 1. When only deferred tabs remain to be tracked, the stats collector detaches > > itself from the shared_collector_. This allows a new collector to be created > for > > new session restores. > > 2. When no tabs at all remain to be tracked, the collector deletes itself as > > before. > This is effectively preserving the existing behavior. A stats collector will now detach from being the shared collector at the same time as before (all tabs loaded, first paint seen), and actually even *sooner* if some tabs are deferred. Thus, the window of time when multiple session restores collapse to one can only actually get smaller compared to the previous situation. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.cc:52: void TabLoader::SetTabLoadingEnabled(bool enable_tab_loading) { On 2015/05/14 00:12:40, sky wrote: > On 2015/05/13 21:18:02, chrisha wrote: > > On 2015/05/12 15:27:56, sky wrote: > > > This likely needs to invalidate a bunch of histograms too. > > > > No more information is provided at the site of this call then is provided via > > the 'DeferTab' calls below. Any histograms that need to be invalidated can be > > done so entirely inside of the stats collector. > > > > What histograms did you have in mind, exactly? > > My concern is that any time based histograms are now going to be delayed until > tab loading turns back on. For example, the time to paint would be totally > misleading. We only track time to first paint, and this concerns itself with the foreground tab only. Since at least one tab is always loaded (the visible tab), then a first paint *will* always happen. If a restore has n tabs, and d get deferred, the stats collector will effectively behave as an "old" stats collector over (n - d) tabs. The tab loader will never be "turned back on", as memory pressure is currently only a one way switch: automatic tab loading stops permanently once memory pressure is seen. (Making this a long term observer of memory pressure, and resuming tab loading is a whole other story, and one that I think we should eventually address. The session restore becomes less of a concept at that point, and eventually we'll simply have some kind of session tab manager, which decides when to load tabs, when to evict them, monitors resources, etc. Probably need an entirely new approach to metrics at that point.) https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_delegate.cc (right): https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_delegate.cc:93: SessionRestoreStatsCollector::DeferTab(tab_controller); On 2015/05/14 15:47:04, sky wrote: > I would rather not expose a bunch of static functions on > SessionRestoreStatsController. It starts getting harder to reason and know about > state. Could you pass in the set of deferred tabs to TrackTabs? Just one > function that way. This is an edge case. When memory pressure fires the tabs are already being tracked (already having been submitted to the collector via TrackTabs). We need a mechanism to inform the stats collector that they are deliberately *not* being loaded, and an additional static function seems the easiest way to do this. The above code is only for the experiment, and simply reuses that function. This is an edge case in that TrackTabs and DeferTab are being called back-to-back, but in intended real usage of this, DeferTab will be called with a time delay after TrackTabs. We *could* augment RestoredTab with an 'is_deferred' state, and simply resubmit the list to TrackTabs. Any preference?
https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:179: UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstLoaded", On 2015/05/14 21:27:01, chrisha wrote: > On 2015/05/14 00:12:40, sky wrote: > > On 2015/05/13 21:18:02, chrisha wrote: > > > On 2015/05/12 15:27:56, sky wrote: > > > > Seems like we should never log this if any tab was deferred. > > > > > > I'm not sure I agree... there will always be at least one tab loaded (there > > will > > > be at least one visible tab), so why wouldn't we emit this? > > > > Two cases to consider: > > . what happens when you close tabs that are loading before they stop? > > . what happens if the network is unavailable? > > These are concerns regardless of whether or not there are deferred tabs. The > stats collector doesn't do anything different now than it did for deferred tabs. > If we didn't change the stats collector to understand them and emit a few more > events, a deferred tab is indistinguishable from a tab that was loaded by the > tab loader arbitrarily far after the session restore initiated. > > So if you want to address these specific issues, let's deal with them > separately. Fair enough. As long as they get dealt with. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:349: if (tabs_tracked_.empty() && render_widget_hosts_loading_.empty()) { On 2015/05/14 21:27:02, chrisha wrote: > On 2015/05/14 00:12:40, sky wrote: > > On 2015/05/13 21:18:02, chrisha wrote: > > > On 2015/05/12 15:27:56, sky wrote: > > > > Lets say tabs_tracked_ goes empty, adoesn't that mean you're going to hit > > this > > > > code n more times where n is the number of deferred tabs? > > > > > > Yes, that's true. This is no worse than before in that it will be hit once > for > > > each tab involved in the session restore. Deferred tabs are a subset of > those. > > > > Not sure there. I think you're change makes it worse, but that's separate. We > > should only report this once. > > The 'deferred state' of a tab doesn't change it's lifetime, nor its flow through > the various maps/sets that the stats collector tracks. At its heart, a tab goes > through a few simple states: > > - not loading > - loading > - loaded > - painted (if we bother to track this) > > RemoveTab can only be called on a tab at most once, so no more processing is > done here. The number of tabs entering the stats collector via TrackTabs remains > unchanged, so no extra work is done there. Calls to DeferTab only modify the > state of a tab that is already being tracked. This represents a minor amount of > extra work, O(1) per deferred tab, to a max of O(n). The deferred state is only > used to change the exact type of UMA records emitted, and to decide when to > detach from being the shared collector. > > The *lifetime* of the shared collector is significantly longer, and this has > memory overhead, but no more entries into the stats collector in the worst case > than already exist. > > Since the lifetime is much longer many more useless UPDATE_BACKING_STORE calls > could be seen. However, to deal with this I've disabled these callbacks as soon > as first paint has been observed. By the time the stats collector is detached > from being the shared collector, it will be receiving no more of these events. Ok. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:370: NavigationController* tab) { On 2015/05/14 21:27:02, chrisha wrote: > On 2015/05/14 00:12:40, sky wrote: > > On 2015/05/13 21:18:02, chrisha wrote: > > > On 2015/05/12 15:27:56, sky wrote: > > > > can you make this take session_restore_id_ so that it's clearer the > callsite > > > > should be supplying the id. This way if someone wants to call > > > > RegisterForNotifications() at some random later date they have to think > > about > > > > what the id is. > > > > > > After hashing this out with Georges, we really don't need the whole session > > > restore ID. I'm now philosophically on board with the whole "multiple > session > > > restores happening at once is seen as a single session restore event". > > > > I have to think about that more. I'm concerned that if the two don't start > close > > enough some of these metrics are going to be misleading. > > > > > However, the deferred tabs now create a longer lifespan for the stats > > collector, > > > which could cause it to start collecting stats again for subsequent session > > > restores that *aren't* coincident with the first ones. To deal with this > I've > > > moved to a 2 stage end-of-life management: > > > > > > 1. When only deferred tabs remain to be tracked, the stats collector > detaches > > > itself from the shared_collector_. This allows a new collector to be created > > for > > > new session restores. > > > 2. When no tabs at all remain to be tracked, the collector deletes itself as > > > before. > > > > This is effectively preserving the existing behavior. A stats collector will now > detach from being the shared collector at the same time as before (all tabs > loaded, first paint seen), and actually even *sooner* if some tabs are deferred. > Thus, the window of time when multiple session restores collapse to one can only > actually get smaller compared to the previous situation. I think the old behavior is wrong in some cases. I'm fine for addressing separately. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_delegate.cc (right): https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_delegate.cc:93: SessionRestoreStatsCollector::DeferTab(tab_controller); On 2015/05/14 21:27:02, chrisha wrote: > On 2015/05/14 15:47:04, sky wrote: > > I would rather not expose a bunch of static functions on > > SessionRestoreStatsController. It starts getting harder to reason and know > about > > state. Could you pass in the set of deferred tabs to TrackTabs? Just one > > function that way. > > This is an edge case. When memory pressure fires the tabs are already being > tracked (already having been submitted to the collector via TrackTabs). We need > a mechanism to inform the stats collector that they are deliberately *not* being > loaded, and an additional static function seems the easiest way to do this. > > The above code is only for the experiment, and simply reuses that function. This > is an edge case in that TrackTabs and DeferTab are being called back-to-back, > but in intended real usage of this, DeferTab will be called with a time delay > after TrackTabs. > > We *could* augment RestoredTab with an 'is_deferred' state, and simply resubmit > the list to TrackTabs. Any preference? I don't think deferred belongs in RestoredTab as it makes no sense at the call site creating it. I prefer having TrackTabs take a separate vector of deferred tabs.
On 2015/05/14 21:51:50, sky wrote: > > > I would rather not expose a bunch of static functions on > > > SessionRestoreStatsController. It starts getting harder to reason and know > > about > > > state. Could you pass in the set of deferred tabs to TrackTabs? Just one > > > function that way. > > > > This is an edge case. When memory pressure fires the tabs are already being > > tracked (already having been submitted to the collector via TrackTabs). We > need > > a mechanism to inform the stats collector that they are deliberately *not* > being > > loaded, and an additional static function seems the easiest way to do this. > > > > The above code is only for the experiment, and simply reuses that function. > This > > is an edge case in that TrackTabs and DeferTab are being called back-to-back, > > but in intended real usage of this, DeferTab will be called with a time delay > > after TrackTabs. > > > > We *could* augment RestoredTab with an 'is_deferred' state, and simply > resubmit > > the list to TrackTabs. Any preference? > > I don't think deferred belongs in RestoredTab as it makes no sense at the call > site creating it. I prefer having TrackTabs take a separate vector of deferred > tabs. TrackTabs takes a vector of RestoredTab objects, but by the time memory-pressure is seen and tabs may be deferred, we are well into the session restore and the tab-loader no longer has a vector RestoredTab ojects, but rather a vector of NavigationControllers. TrackTabs also gets passed a restore_started time, which doesn't make much sense in the context of deferring a tab. I prefer a separate function because they are doing 2 separate things: TrackTabs is a factory of a sort, and kicks off a new round of session restore tracking. All the tabs are new, and never yet seen by the collector. Deferring a tab is something that happens to a session restore in progress, to a tab that is already being tracked, but at some arbitrary point in time after the beginning of the session restore. Maybe rename it MarkTrackedTabsAsDeferred (make its semantics more clear) and making it accept a vector of NavigationControllers?
Okay, I've made some sweeping changes to this for now, more surely to come. In this iteration I've made the relationship between TabLoader and SessionRestoreStatsCollector explicit again, as the TabLoader needs to know *which* stats collector to talk to in order to defer tabs. We can break this relationship, but that will require a global structure for mapping tabs to the stats collector instance that owns them. I've also tried to make very explicit things that are gathered during an active session restore, and those things that are gathered afterwards (ie: wrt deferred tabs). I've had the thought that those events could also be tracked by a singleton global stats collector that only observes deferred tabs... your opinion? Other issues I've tried to address in my various inline comments. No unittests included yet as I've only been doing manual testing... don't want to invest the time until the architecture and API are largely stable. Sorry for the long delay here, but please take another look? https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:179: UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstLoaded", On 2015/05/14 21:51:50, sky wrote: > On 2015/05/14 21:27:01, chrisha wrote: > > On 2015/05/14 00:12:40, sky wrote: > > > On 2015/05/13 21:18:02, chrisha wrote: > > > > On 2015/05/12 15:27:56, sky wrote: > > > > > Seems like we should never log this if any tab was deferred. > > > > > > > > I'm not sure I agree... there will always be at least one tab loaded > (there > > > will > > > > be at least one visible tab), so why wouldn't we emit this? > > > > > > Two cases to consider: > > > . what happens when you close tabs that are loading before they stop? > > > . what happens if the network is unavailable? > > > > These are concerns regardless of whether or not there are deferred tabs. The > > stats collector doesn't do anything different now than it did for deferred > tabs. > > If we didn't change the stats collector to understand them and emit a few more > > events, a deferred tab is indistinguishable from a tab that was loaded by the > > tab loader arbitrarily far after the session restore initiated. > > > > So if you want to address these specific issues, let's deal with them > > separately. > > Fair enough. As long as they get dealt with. Added a TODO capturing this. https://codereview.chromium.org/1136523004/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:370: NavigationController* tab) { On 2015/05/14 21:51:50, sky wrote: > On 2015/05/14 21:27:02, chrisha wrote: > > On 2015/05/14 00:12:40, sky wrote: > > > On 2015/05/13 21:18:02, chrisha wrote: > > > > On 2015/05/12 15:27:56, sky wrote: > > > > > can you make this take session_restore_id_ so that it's clearer the > > callsite > > > > > should be supplying the id. This way if someone wants to call > > > > > RegisterForNotifications() at some random later date they have to think > > > about > > > > > what the id is. > > > > > > > > After hashing this out with Georges, we really don't need the whole > session > > > > restore ID. I'm now philosophically on board with the whole "multiple > > session > > > > restores happening at once is seen as a single session restore event". > > > > > > I have to think about that more. I'm concerned that if the two don't start > > close > > > enough some of these metrics are going to be misleading. > > > > > > > However, the deferred tabs now create a longer lifespan for the stats > > > collector, > > > > which could cause it to start collecting stats again for subsequent > session > > > > restores that *aren't* coincident with the first ones. To deal with this > > I've > > > > moved to a 2 stage end-of-life management: > > > > > > > > 1. When only deferred tabs remain to be tracked, the stats collector > > detaches > > > > itself from the shared_collector_. This allows a new collector to be > created > > > for > > > > new session restores. > > > > 2. When no tabs at all remain to be tracked, the collector deletes itself > as > > > > before. > > > > > > > This is effectively preserving the existing behavior. A stats collector will > now > > detach from being the shared collector at the same time as before (all tabs > > loaded, first paint seen), and actually even *sooner* if some tabs are > deferred. > > Thus, the window of time when multiple session restores collapse to one can > only > > actually get smaller compared to the previous situation. > > I think the old behavior is wrong in some cases. I'm fine for addressing > separately. Added a TODO capturing this. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:5: // Declares a utility class for tracking tabs involved in a session restore. On 2015/05/14 15:47:04, sky wrote: > Move this description above class description. Done. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:18: // collectors may exist simultaneously, however at most one may be the active On 2015/05/14 15:47:04, sky wrote: > It's not clear what 'active' means here. Acknowledged. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:22: // closed, the session restore will destroy itself. On 2015/05/14 15:47:04, sky wrote: > 'session restore' -> stats collector. But it appears you aren't consistent in > your comments here, eg "session restore stats tracker" on 7, "collector" on > 10... I recommend using SessionRestoreStatsCollector everywhere. Done. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:45: public base::RefCounted<SessionRestoreStatsCollector> { On 2015/05/14 15:47:04, sky wrote: > There isn't a need for this class to be ref counted anymore. Done. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:62: using RenderWidgetHostMap = std::map<content::RenderWidgetHost*, bool>; On 2015/05/14 15:47:04, sky wrote: > Bools can be a bit mysterious, where as enums are generally less confusing. > Could you use an enum here? Done. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:113: // The following 3 maps are used to track tabs at different points in their On 2015/05/14 15:47:04, sky wrote: > Seems like you're tracking a bunch of state per tab in numerous maps that would > be better served by a dedicated structure with a map from tab to state, eg: > struct TabState { > bool was_deferred; A bool is fine here as you always reference via > .was_deferred > // I'm not positive you need both of these, an enum may better capture the > state. > bool waiting_for_paint; > bool is_loading; > }; Unfortunately we need efficient lookup by 2 indices: NavigationControllers and RenderWidgetHosts (hence the 2 separate maps for now). We also need to know how many tabs are in each state, which is tracked for us automatically via the size of the corresponding set. (We would be tracking this explicitly ourselves.) I thought this sounded like a reasonable suggestion and started refactoring the code, but I quickly found it less readable. We could still go this route, but it there's significantly more bookkeeping code and to be readable it would require a bunch of helpers functions for performing state transitions, and a secondary index. https://codereview.chromium.org/1136523004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:147: // tabs_tracked_ map. On 2015/05/14 15:47:04, sky wrote: > Generally we use | around variable and parameter names in comments, eg > |tabs_tracked_|. Done.
https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:79: // The collector should only be destroyed when tracking is entirely Is this true if shutdown happens before everything is loaded? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:100: // Active tabs have already started loading, kicked by the TabLoader. Actually, don't they start loading because they are active? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:124: --waiting_for_load_tab_count_; Might waiting_for_load_tab_count_ be 0 now? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:146: WebContents* web_contents = Source<WebContents>(source).ptr(); Code seems to assume we'll get a LOAD_STOP before a DESTROYED. Is that guaranteed? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:164: // Update statistics. nit: this comment isn't really useful, I would nuke it. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:184: // If first paint has already been observed then there's no need to keep This comment explains what the code does not why. The 'why' is the interesting part to document. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:217: got_first_paint_ = true; AFAICT both paths set got_first_paint_ to true. Set it outside of conditional and remove 220 as it's always true. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:252: // Mark the point at which all tabs have loaded and log the UMA metrics. Again, not a useful comment. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:273: // Get the tab state. Again, this comment isn't useful. It just documents what the code is doing, and the code isn't doing anything tricky here. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:326: // This lookup can fail as events are received for tabs that aren't You can figure out the NavigationController the RWH is associated with by iterating over all the NavigationControllers. As this is only called once, and there aren't a ton of tabs I would rather see you do that then maintain a second map that we have to worry about getting out of sync. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:335: CHECK(tab_state->loading_state == TAB_IS_NOT_LOADING); Why the CHECK? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:56: // The number of tabs involved in the session restore. This corresponds to Using 'the session restore' is confusing here as this may actual span multiple session restores. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:61: // The number of tabs automatically loaded by the TabLoader. This It's not clear what automatically loaded means here. Please make it clearer. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:65: // The time taken to load the first foreground tab. If this is zero it is Clarify what this is relative to and what 'load' constitutes in this context. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:71: // The time taken to paint the first foreground tab. If this is zero it is Same comment about what this is relative to. It's probably worth describing that all times are relative to XXX. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:80: base::TimeDelta all_tabs_loaded; all is too overloaded here. Maybe time_to_non_deferred_tabs_loaded. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:102: virtual void ReportTabLoaderStats(); I would prefer a delegate rather than subclassing. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:118: TabState(content::NavigationController* controller); explicit https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:195: base::TimeTicks restore_started_; const
Okay, another stab at this. PTAL? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:79: // The collector should only be destroyed when tracking is entirely On 2015/05/27 15:18:39, sky wrote: > Is this true if shutdown happens before everything is loaded? Very good point. Removed. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:100: // Active tabs have already started loading, kicked by the TabLoader. On 2015/05/27 15:18:39, sky wrote: > Actually, don't they start loading because they are active? Acknowledged. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:124: --waiting_for_load_tab_count_; On 2015/05/27 15:18:39, sky wrote: > Might waiting_for_load_tab_count_ be 0 now? Yeah, it might. I thought it simpler to keep all of the state transition logic in Observe (the 15 lines or so at the end of that function). This can only cause the collector to wait a little bit longer before it invokes ReportTabLoaderStats. I can also move that logic to a helper function, and call it both here and from Observe. Thoughts? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:146: WebContents* web_contents = Source<WebContents>(source).ptr(); On 2015/05/27 15:18:39, sky wrote: > Code seems to assume we'll get a LOAD_STOP before a DESTROYED. Is that > guaranteed? Another great catch. I'm not sure if that's guaranteed or not, but it's easier to make this tolerate the case where that *doesn't* happen. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:164: // Update statistics. On 2015/05/27 15:18:39, sky wrote: > nit: this comment isn't really useful, I would nuke it. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:184: // If first paint has already been observed then there's no need to keep On 2015/05/27 15:18:39, sky wrote: > This comment explains what the code does not why. The 'why' is the interesting > part to document. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:217: got_first_paint_ = true; On 2015/05/27 15:18:39, sky wrote: > AFAICT both paths set got_first_paint_ to true. Set it outside of conditional > and remove 220 as it's always true. Indeed... remnant of the previous more tortured logic. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:252: // Mark the point at which all tabs have loaded and log the UMA metrics. On 2015/05/27 15:18:39, sky wrote: > Again, not a useful comment. Acknowledged. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:273: // Get the tab state. On 2015/05/27 15:18:39, sky wrote: > Again, this comment isn't useful. It just documents what the code is doing, and > the code isn't doing anything tricky here. Acknowledged. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:326: // This lookup can fail as events are received for tabs that aren't On 2015/05/27 15:18:39, sky wrote: > You can figure out the NavigationController the RWH is associated with by > iterating over all the NavigationControllers. As this is only called once, and > there aren't a ton of tabs I would rather see you do that then maintain a second > map that we have to worry about getting out of sync. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:335: CHECK(tab_state->loading_state == TAB_IS_NOT_LOADING); On 2015/05/27 15:18:39, sky wrote: > Why the CHECK? Made a DCHECK. This really shouldn't be called twice for a tab, but it's harmless if it is. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:56: // The number of tabs involved in the session restore. This corresponds to On 2015/05/27 15:18:39, sky wrote: > Using 'the session restore' is confusing here as this may actual span multiple > session restores. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:61: // The number of tabs automatically loaded by the TabLoader. This On 2015/05/27 15:18:40, sky wrote: > It's not clear what automatically loaded means here. Please make it clearer. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:65: // The time taken to load the first foreground tab. If this is zero it is On 2015/05/27 15:18:40, sky wrote: > Clarify what this is relative to and what 'load' constitutes in this context. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:71: // The time taken to paint the first foreground tab. If this is zero it is On 2015/05/27 15:18:40, sky wrote: > Same comment about what this is relative to. It's probably worth describing that > all times are relative to XXX. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:80: base::TimeDelta all_tabs_loaded; On 2015/05/27 15:18:40, sky wrote: > all is too overloaded here. Maybe time_to_non_deferred_tabs_loaded. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:102: virtual void ReportTabLoaderStats(); On 2015/05/27 15:18:40, sky wrote: > I would prefer a delegate rather than subclassing. Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:118: TabState(content::NavigationController* controller); On 2015/05/27 15:18:40, sky wrote: > explicit Done. https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:195: base::TimeTicks restore_started_; On 2015/05/27 15:18:39, sky wrote: > const Done.
https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:124: --waiting_for_load_tab_count_; On 2015/06/03 21:16:36, chrisha wrote: > On 2015/05/27 15:18:39, sky wrote: > > Might waiting_for_load_tab_count_ be 0 now? > > Yeah, it might. I thought it simpler to keep all of the state transition logic > in Observe (the 15 lines or so at the end of that function). This can only cause > the collector to wait a little bit longer before it invokes > ReportTabLoaderStats. > > I can also move that logic to a helper function, and call it both here and from > Observe. Thoughts? Helper function SGTM. https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:342: DCHECK(tab_state->loading_state == TAB_IS_NOT_LOADING); DCHECK_EQ https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:56: // The number of tabs involved all overlapping session restores being 'in' all? https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:230: : public base::RefCounted<StatsReportingDelegate> { Do you really need this ref counted?
Will now start working on unittest. You're largely happy with the direction of the refactor? https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/60001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:124: --waiting_for_load_tab_count_; On 2015/06/03 22:00:07, sky wrote: > On 2015/06/03 21:16:36, chrisha wrote: > > On 2015/05/27 15:18:39, sky wrote: > > > Might waiting_for_load_tab_count_ be 0 now? > > > > Yeah, it might. I thought it simpler to keep all of the state transition logic > > in Observe (the 15 lines or so at the end of that function). This can only > cause > > the collector to wait a little bit longer before it invokes > > ReportTabLoaderStats. > > > > I can also move that logic to a helper function, and call it both here and > from > > Observe. Thoughts? > > Helper function SGTM. Done. https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.cc:342: DCHECK(tab_state->loading_state == TAB_IS_NOT_LOADING); On 2015/06/03 22:00:07, sky wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:56: // The number of tabs involved all overlapping session restores being On 2015/06/03 22:00:08, sky wrote: > 'in' all? Done. https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:230: : public base::RefCounted<StatsReportingDelegate> { On 2015/06/03 22:00:08, sky wrote: > Do you really need this ref counted? There's a lifetime management issue. Three options: (1) The delegate must outlive the stats collector. That places the burden on the user (the SessionRestoreDelegate or the TabLoader), and since their lifetimes are decoupled from that of the SessionRestoreStatsCollector, this is not easy to do. (2) Ownership is transferred to the SessionRestoreStatsCollector. This works fine in the general case, but makes unittesting slightly harder, as the delegate will be reaped with the stats collector. This means the delegate needs to communicate to another class with a longer lifetime, or the unittest has to arrange for the collector not to be reaped. Both of these approaches are relatively simple. (3) Reference counted. Maybe a little overkill. Does (2) sound reasonable to you?
And yes, I'm happy with direction. https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_stats_collector.h:230: : public base::RefCounted<StatsReportingDelegate> { On 2015/06/04 14:04:47, chrisha wrote: > On 2015/06/03 22:00:08, sky wrote: > > Do you really need this ref counted? > > There's a lifetime management issue. Three options: > > (1) The delegate must outlive the stats collector. That places the burden on the > user (the SessionRestoreDelegate or the TabLoader), and since their lifetimes > are decoupled from that of the SessionRestoreStatsCollector, this is not easy to > do. > > (2) Ownership is transferred to the SessionRestoreStatsCollector. This works > fine in the general case, but makes unittesting slightly harder, as the delegate > will be reaped with the stats collector. This means the delegate needs to > communicate to another class with a longer lifetime, or the unittest has to > arrange for the collector not to be reaped. Both of these approaches are > relatively simple. > > (3) Reference counted. Maybe a little overkill. > > Does (2) sound reasonable to you? 2 sgtm
Okay, rejigged the delegate as discussed. Got the unittests working as standalone tests with mocks (required adding a few more test seams, notably an injected TickClock). Bonus: unittesting found 2 more edge cases where calculations were incorrect. Let me know if you think of other edge case tests to explore. PTAL?
https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:103: // Constructs a SessionRestoreStatsCollector. Ownership of I would nuke the second sentence. It is implied by virtue of reporting_delegate being passed via a scoped_ptr. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:119: protected: Style guide says no protected members. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:188: void set_tick_clock(scoped_ptr<base::TickClock> tick_clock); Generally we inline functions in unix_hacker_style. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:238: StatsReportingDelegate() { } nit: { } -> {} here, next line and 258. Make sure you run git cl format. I'm pretty sure it would have fixed these up. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:258: virtual ~UmaStatsReportingDelegate() { } This should be override, no virtual. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector_unittest.cc (right): https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:38: MOCK_METHOD1(ReportTabLoaderStats, void(const TabLoaderStats&)); Don't use gmock. See threads in chromium-dev.
Removed use of gmock, and addressed other comments. Another look? https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:103: // Constructs a SessionRestoreStatsCollector. Ownership of On 2015/06/17 15:33:54, sky wrote: > I would nuke the second sentence. It is implied by virtue of reporting_delegate > being passed via a scoped_ptr. Done. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:119: protected: On 2015/06/17 15:33:54, sky wrote: > Style guide says no protected members. Was just to make them unittest accessible. Will use a friend declaration instead. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:188: void set_tick_clock(scoped_ptr<base::TickClock> tick_clock); On 2015/06/17 15:33:54, sky wrote: > Generally we inline functions in unix_hacker_style. Done. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:238: StatsReportingDelegate() { } On 2015/06/17 15:33:54, sky wrote: > nit: { } -> {} here, next line and 258. Make sure you run git cl format. I'm > pretty sure it would have fixed these up. Yeah, neglected to run format. Done now. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:258: virtual ~UmaStatsReportingDelegate() { } On 2015/06/17 15:33:54, sky wrote: > This should be override, no virtual. Done. https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector_unittest.cc (right): https://codereview.chromium.org/1136523004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:38: MOCK_METHOD1(ReportTabLoaderStats, void(const TabLoaderStats&)); On 2015/06/17 15:33:54, sky wrote: > Don't use gmock. See threads in chromium-dev. Although I'm clearly on the other side in this holy war, done.
LGTM https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:122: // Indicates whether the loading state of a tab. nit: fix this comment. Maybe you can just nuke it entirely as the names are good. https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:180: void CleanupIfDoneTracking(); By cleanup isn't this a delete? Maybe DeleteIfDoneTracking? Or perhaps ReleaseIfDoneTracking? https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector_unittest.cc (right): https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:31: : report_tab_loader_stats_call_count_(0), 0u on all of these. https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:111: }; DISALLOW... https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:142: }; DISALLOW... (on all the classes you have here). https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:183: // The unittest must take not to access the clock only while the not->care not?
chrisha@chromium.org changed reviewers: + asvitkine@chromium.org
Thanks sky@. asvtikine@, care to take a look at histograms.xml? https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:122: // Indicates whether the loading state of a tab. On 2015/06/17 21:12:04, sky wrote: > nit: fix this comment. Maybe you can just nuke it entirely as the names are > good. Done. https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:180: void CleanupIfDoneTracking(); On 2015/06/17 21:12:04, sky wrote: > By cleanup isn't this a delete? Maybe DeleteIfDoneTracking? Or perhaps > ReleaseIfDoneTracking? Done. https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector_unittest.cc (right): https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:31: : report_tab_loader_stats_call_count_(0), On 2015/06/17 21:12:04, sky wrote: > 0u on all of these. Done. https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:111: }; On 2015/06/17 21:12:04, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:142: }; On 2015/06/17 21:12:04, sky wrote: > DISALLOW... (on all the classes you have here). Done. https://codereview.chromium.org/1136523004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:183: // The unittest must take not to access the clock only while the On 2015/06/17 21:12:04, sky wrote: > not->care not? Done.
https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_delegate.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_delegate.cc:75: for (auto& restored_tab : tabs) { Nit: const auto& https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_delegate.cc:99: for (auto& restored_tab : tabs) { Nit: const auto& https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:24: const char kSessionRestoreActions[] = "SessionRestore.Actions"; Can this be inlined in the macro? (After you make a function wrapping it.) https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:97: for (auto& tab : tabs) { Nit: const auto& https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:404: SESSION_RESTORE_TAB_ACTIONS_UMA_MAX); Please make a helper function for this histogram invocation that takes the enum as a param. Each macro expands to a lot of code, so avoid repeating it for the same histogram name. Same thing for the others. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:238: virtual void ReportTabLoaderStats(const TabLoaderStats& tab_loader_stats) = 0; Nit: Add empty lines between methods. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:255: // StatsReportingDelegate implementation. Nit: I think preference for new code is just: // StatsReportingDelegate: https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector_unittest.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:303: DISALLOW_COPY_AND_ASSIGN(SessionRestoreStatsCollectorTest); This should actually be in the private: section. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/tab_loader.cc:84: new SessionRestoreStatsCollector::UmaStatsReportingDelegate()); Nit: Maybe just pass make_scoped_ptr(new SessionRestoreStatsCollector::UmaStatsReportingDelegate) to the constructor param below directly? https://codereview.chromium.org/1136523004/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136523004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:37630: + restore events. Nit: Expand description to clarify what "treated" means.
Thanks for the feedback. Comments addressed inline. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_delegate.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_delegate.cc:75: for (auto& restored_tab : tabs) { On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Nit: const auto& Done. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_delegate.cc:99: for (auto& restored_tab : tabs) { On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Nit: const auto& Done. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:24: const char kSessionRestoreActions[] = "SessionRestore.Actions"; On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Can this be inlined in the macro? (After you make a function wrapping it.) Done. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:97: for (auto& tab : tabs) { On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Nit: const auto& Done. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:404: SESSION_RESTORE_TAB_ACTIONS_UMA_MAX); On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Please make a helper function for this histogram invocation that takes the enum > as a param. Each macro expands to a lot of code, so avoid repeating it for the > same histogram name. > > Same thing for the others. Done for the two enumeration histograms. The others are all only called once. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.h (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:238: virtual void ReportTabLoaderStats(const TabLoaderStats& tab_loader_stats) = 0; On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Nit: Add empty lines between methods. Done. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.h:255: // StatsReportingDelegate implementation. On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Nit: I think preference for new code is just: > > // StatsReportingDelegate: Done. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector_unittest.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector_unittest.cc:303: DISALLOW_COPY_AND_ASSIGN(SessionRestoreStatsCollectorTest); On 2015/06/18 17:48:48, Alexei Svitkine wrote: > This should actually be in the private: section. Done. https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/1136523004/diff/220001/chrome/browser/session... chrome/browser/sessions/tab_loader.cc:84: new SessionRestoreStatsCollector::UmaStatsReportingDelegate()); On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Nit: Maybe just pass make_scoped_ptr(new > SessionRestoreStatsCollector::UmaStatsReportingDelegate) to the constructor > param below directly? Done. https://codereview.chromium.org/1136523004/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136523004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:37630: + restore events. On 2015/06/18 17:48:48, Alexei Svitkine wrote: > Nit: Expand description to clarify what "treated" means. I've rewritten this to be a little more clear, but the real documentation is in the enumeration text. Is that ok? Done.
lgtm https://codereview.chromium.org/1136523004/diff/240001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:262: for (auto& map_entry : tabs_tracked_) { Nit: const auto&
Thanks, committing. https://codereview.chromium.org/1136523004/diff/240001/chrome/browser/session... File chrome/browser/sessions/session_restore_stats_collector.cc (right): https://codereview.chromium.org/1136523004/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_stats_collector.cc:262: for (auto& map_entry : tabs_tracked_) { On 2015/06/18 20:16:09, Alexei Svitkine wrote: > Nit: const auto& Needs to be non-const, because going into loaded_tabs vector, which is then being fed into RemoveTab (also required non-const).
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1136523004/#ps240001 (title: "Address asvitkine's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136523004/240001
The CQ bit was unchecked by chrisha@chromium.org
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1136523004/#ps260001 (title: "Typo fix that breaks compile.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136523004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1136523004/#ps280001 (title: "Fix clang warnings.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136523004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1136523004/#ps300001 (title: "Fix for unittests involving back/forward navigation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136523004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sky@: Gah, another review required please. There turned out to be two problems: - back/forward navigations of a tab in the process of being restored (loaded but not yet painted) would cause a DCHECK to fire. Added logic to handle this and a new unittest. - The changes to TestRenderViewHost were causing an unrelated ChromeOS test to fail. Needed to initialize a fake gfx::Screen.
LGTM
Patchset #18 (id:340001) has been deleted
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1136523004/#ps380001 (title: "Fix new unittest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136523004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1136523004/#ps400001 (title: "Fix typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136523004/400001
Message was sent while issue was closed.
Committed patchset #20 (id:400001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/f3c87f3238e503d995f3690d592d454715ff9cea Cr-Commit-Position: refs/heads/master@{#335413} |
