|
|
DescriptionAdd signals that mark the start and end of session restore.
Currently, there is no easy way to know whether we are during session restore
or not, so it is necessary to add some signals to do so.
Since the TabLoader lifetime is equal to the session restore, I made TabLoader
to signal the start/end of session restore to the global TabManager.
BUG=733012
Review-Url: https://codereview.chromium.org/2935183002
Cr-Commit-Position: refs/heads/master@{#482654}
Committed: https://chromium.googlesource.com/chromium/src/+/1b8597354a9400f5708d6e1476a51aff1b79ef15
Patch Set 1 #
Total comments: 4
Patch Set 2 : Change if-check to DCHECK for tab_manager. #
Total comments: 5
Patch Set 3 : Rename IsDuringSessionRestore to IsInSessionRestore #Patch Set 4 : Add a test for MarkSessionRestore[Started,Ended] #Patch Set 5 : Change MarkSessionRestore[Started,Ended] to OnSessionRestore[Started,Ended] #
Total comments: 2
Patch Set 6 : DCHECK() directly on GetTabManager() and eliminate a variable #
Total comments: 1
Patch Set 7 : Add DCHECK for is_in_session_restore and fix a test failure. #Patch Set 8 : Use OnSessionRestore[Started,Ended]() directly in TabLoader. #
Total comments: 1
Patch Set 9 : Add SessionRestoreObserver so TabLoader does not depend on TabManager. #Patch Set 10 : Change to use SessionRestoreTabLoading names and revert SessionRestoreObserver #Patch Set 11 : Add SessionRestoreObserver to remove dependency from TabLoader to TabManager #Patch Set 12 : Store global SessionRestoreObserver list into a separate class. #
Total comments: 12
Patch Set 13 : Address lpy@'s comments #
Total comments: 13
Patch Set 14 : Address various comments. #
Total comments: 9
Patch Set 15 : Address new comments. #
Total comments: 4
Patch Set 16 : Address new comments and revise a variable name. #Patch Set 17 : Enable SessionRestoreObserverTest only when session service available. #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 71 (30 generated)
Description was changed from ========== Add signals that mark the start and end of session restore. Currently, there is no easy way to know whether we are during session restore or not, so it is necessary to add some signals to do so. Since the TabLoader lifetime is equal to the session restore, I made TabLoader to signal the start/end of session restore to the global TabManager. BUG=733012 ========== to ========== Add signals that mark the start and end of session restore. Currently, there is no easy way to know whether we are during session restore or not, so it is necessary to add some signals to do so. Since the TabLoader lifetime is equal to the session restore, I made TabLoader to signal the start/end of session restore to the global TabManager. BUG=733012 ==========
ducbui@google.com changed reviewers: + fmeawad@chromium.org
PTAL
fmeawad@chromium.org changed reviewers: + zhenw@chromium.org
+zhenw Overall looks good. https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) Can this be a DCHECK instead? Similar to here maybe: https://cs.chromium.org/chromium/src/chrome/browser/memory/chrome_memory_coor... #if !defined(OS_ANDROID)
On 2017/06/14 15:34:58, fmeawad wrote: > +zhenw > > Overall looks good. > > https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... > File chrome/browser/sessions/tab_loader.cc (right): > > https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... > chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) > Can this be a DCHECK instead? > Similar to here maybe: > https://cs.chromium.org/chromium/src/chrome/browser/memory/chrome_memory_coor... > #if !defined(OS_ANDROID) I think it's safer to add the "#if defined" while keeping the null check.
https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) On 2017/06/14 15:34:58, fmeawad wrote: > Can this be a DCHECK instead? > Similar to here maybe: > https://cs.chromium.org/chromium/src/chrome/browser/memory/chrome_memory_coor... > #if !defined(OS_ANDROID) >> "I think it's safer to add the "#if defined" while keeping the null check." This is why we put the DCHECK, if the check is always true, it should not be there.
https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) On 2017/06/14 16:20:56, fmeawad wrote: > On 2017/06/14 15:34:58, fmeawad wrote: > > Can this be a DCHECK instead? > > Similar to here maybe: > > > https://cs.chromium.org/chromium/src/chrome/browser/memory/chrome_memory_coor... > > #if !defined(OS_ANDROID) > > >> "I think it's safer to add the "#if defined" while keeping the null check." > > This is why we put the DCHECK, if the check is always true, it should not be > there. Is the usage of TabLoader limited to only tab-manager-supported platforms?
https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) On 2017/06/14 16:24:18, ducbui wrote: > On 2017/06/14 16:20:56, fmeawad wrote: > > On 2017/06/14 15:34:58, fmeawad wrote: > > > Can this be a DCHECK instead? > > > Similar to here maybe: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/memory/chrome_memory_coor... > > > #if !defined(OS_ANDROID) > > > > >> "I think it's safer to add the "#if defined" while keeping the null check." > > > > This is why we put the DCHECK, if the check is always true, it should not be > > there. > > Is the usage of TabLoader limited to only tab-manager-supported platforms? The TabLoader is not compiled on Android and on ChromeCast because session service is not enabled on these platforms. https://cs.chromium.org/chromium/src/chrome/browser/BUILD.gn?type=cs&q=enable... https://cs.chromium.org/chromium/src/chrome/common/features.gni?type=cs&q=ENA... Therefore, I will change the if (tab_manager) to a DCHECK.
Can you also add some test?
overall LG to me, some tiny nits. https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:157: void MarkSessionRestoreStarted() { is_during_session_restore_ = true; } The name is a little bit too verbose, maybe SessionRestoreStarted or OnSessionRestoreStarted? https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:161: bool IsDuringSessionRestore() const { return is_during_session_restore_; } IsInSessionRestore? https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.h (right): https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.h:129: static void MarkSessionRestoreStarted(); ditto
https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:157: void MarkSessionRestoreStarted() { is_during_session_restore_ = true; } On 2017/06/14 17:23:19, lpy wrote: > The name is a little bit too verbose, maybe SessionRestoreStarted or > OnSessionRestoreStarted? I still prefer "MarkSessionRestoreStarted" because the method starts with a verb. Also, although the name is a little verbose, this method is called very occasionally. https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:161: bool IsDuringSessionRestore() const { return is_during_session_restore_; } On 2017/06/14 17:23:19, lpy wrote: > IsInSessionRestore? Yes, this name seems better :)
ducbui@google.com changed reviewers: + oysteine@chromium.org
Hi Oystein oystein@, PTAL. Thanks.
zhenw@google.com changed reviewers: + zhenw@google.com
lgtm
lgtm https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.cc:324: DCHECK(tab_manager); I would just DCHECK(g_browser_process->GetTabManager()) and avoid creating a variable.
https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.cc:324: DCHECK(tab_manager); On 2017/06/15 17:00:43, fmeawad wrote: > I would just DCHECK(g_browser_process->GetTabManager()) and avoid creating a > variable. Done.
https://codereview.chromium.org/2935183002/diff/100001/chrome/browser/session... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/100001/chrome/browser/session... chrome/browser/sessions/tab_loader.cc:108: OnSessionRestoreStarted(); lpy and I thinks that these calls should be inlined since they will not be called from anywhere else.
Hi Duc, Just a general comment on the workflow. In general, when you want the reviewers to review, you will need to send a message with replies to comments, so the reviewers know when to take action. Updating patches to the CL does not do that for you automatically. This time, I happened to notice you have added tests when you send message to Oystein. Worflow: 1. Upload a new CL and ask for review. 2. Get reviews from reviewers. 3. Modify code and upload a patch (or multiple patches). 4. After addressing comments, send a message to reviewers with replies. (Important!) 5. Usually, do not upload new patches until new reviewer feedback to avoid confusion. 6. Get more reviews from reviewers and repeat 2-5 until lg2m. 8. Submit.
zhenw@chromium.org changed reviewers: + chrisha@chromium.org - zhenw@google.com
+chrisha
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
There's other code in-flight doing something very similar (talk to zhenw), as it is needed for some session restore related metrics. The problem with this simplistic approach is that there can be multiple independent session restores going on simultaneously in a single browser (session restores are tied to profiles). In which case you need some type of counter / ID mechanism.
On 2017/06/15 20:23:42, chrisha wrote: > There's other code in-flight doing something very similar (talk to zhenw), as it > is needed for some session restore related metrics. This CL is actually a split from the other CL (https://crrev.com/2930013005/). I will keep the profile discussion here as it is more related to this one. Duc, maybe you can make that CL depending on this CL, so people are not confused. > The problem with this simplistic approach is that there can be multiple > independent session restores going on simultaneously in a single browser > (session restores are tied to profiles). In which case you need some type of > counter / ID mechanism. Do we care about differentiating metrics reported from different session restores? Currently we are using TabLoader's life to check if it is in session restore. Multiple simultaneous session restores share the same TabLoader, which means TabLoader's life covers from the start of the 1st session restore to the end of the last session restore. Given UMA is aggregated data, a counter may be enough?
On 2017/06/15 21:56:57, Zhen Wang wrote: > On 2017/06/15 20:23:42, chrisha wrote: > > There's other code in-flight doing something very similar (talk to zhenw), as > it > > is needed for some session restore related metrics. > > This CL is actually a split from the other CL (https://crrev.com/2930013005/). I > will keep the profile discussion here as it is more related to this one. > > Duc, maybe you can make that CL depending on this CL, so people are not > confused. > > > The problem with this simplistic approach is that there can be multiple > > independent session restores going on simultaneously in a single browser > > (session restores are tied to profiles). In which case you need some type of > > counter / ID mechanism. > > Do we care about differentiating metrics reported from different session > restores? > > Currently we are using TabLoader's life to check if it is in session restore. > Multiple simultaneous session restores share the same TabLoader, which means > TabLoader's life covers from the start of the 1st session restore to the end of > the last session restore. Given UMA is aggregated data, a counter may be enough? I think tracking the loading of foreground tabs from the first to the last session restore might be fine, because session restores of different profiles which share the same browser process can slow down the loading of foreground tabs of different profiles. We want to know how fast foreground tabs are loaded during session restore, so the recorded metrics might be acceptable as long as they are recorded during session restore.
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm Note that we want to move TabLoader's logic into TabManager once the background tab opening works, so this will all change then. (TabManager will know explicitly which tabs are related to session restore, so no callbacks required.)
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ducbui@google.com changed reviewers: + sky@chromium.org
Hi sky@, Could PTAL changes in tab_loader.cc. Thanks.
https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... chrome/browser/sessions/tab_loader.cc:110: g_browser_process->GetTabManager()->OnSessionRestoreStarted(); Putting this here isn't entirely correct as TabLoader is created *after* session restore has waited to read the file. I also think other places may be interested in tracking the lifetime of session restore so that I would prefer a generic observer interface. TabLoader shouldn't have a direct dependency on TabManager, it should only care about notifying observers.
On 2017/06/21 00:40:37, sky wrote: > https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... > File chrome/browser/sessions/tab_loader.cc (right): > > https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... > chrome/browser/sessions/tab_loader.cc:110: > g_browser_process->GetTabManager()->OnSessionRestoreStarted(); > Putting this here isn't entirely correct as TabLoader is created *after* session > restore has waited to read the file. Fair point. > I also think other places may be interested in tracking the lifetime of session > restore so that I would prefer a generic observer interface. TabLoader shouldn't > have a direct dependency on TabManager, it should only care about notifying > observers. Is this a case of building something we *think* might be used, or are there clients that would use this right now? If it's the former I'd prefer the simpler non-invasive thing with a comment saying to lift this to an observer interface in the case of a second client needing this info. Some additional context: we plan on moving TabLoader's logic into TabManager for a couple reasons: - we want to stagger loading of tabs opened in the background (pretty much the same logic) - we want a single "lifecycle" engine to reason about when to evict tabs (discard) and when to load them (session restore, background tab opening, reloading discarded tabs) TabManager would be the only place that is aware of a group of background-opened tabs being loaded, which is similar in spirit to an "ongoing session restore". Would it make sense in your mind to have TabManager expose the observer interface?
> TabManager would be the only place that is aware of a group of background-opened > tabs being loaded, which is similar in spirit to an "ongoing session restore". > Would it make sense in your mind to have TabManager expose the observer > interface? Looking at the follow-up CL using this functionality, the more I like sky's idea of a dedicated observer interface on the SessionRestore class itself. More comments over here: https://codereview.chromium.org/2930013005
On Wed, Jun 21, 2017 at 7:43 AM, <chrisha@chromium.org> wrote: > On 2017/06/21 00:40:37, sky wrote: > > > https://codereview.chromium.org/2935183002/diff/140001/ > chrome/browser/sessions/tab_loader.cc > > File chrome/browser/sessions/tab_loader.cc (right): > > > > > https://codereview.chromium.org/2935183002/diff/140001/ > chrome/browser/sessions/tab_loader.cc#newcode110 > > chrome/browser/sessions/tab_loader.cc:110: > > g_browser_process->GetTabManager()->OnSessionRestoreStarted(); > > Putting this here isn't entirely correct as TabLoader is created *after* > session > > restore has waited to read the file. > > Fair point. > > > I also think other places may be interested in tracking the lifetime of > session > > restore so that I would prefer a generic observer interface. TabLoader > shouldn't > > have a direct dependency on TabManager, it should only care about > notifying > > observers. > > Is this a case of building something we *think* might be used, or are there > clients that would use this right now? > My main concern is trying to minimize dependencies between classes. I don't think there is an immediate need for another implementation. -Scott > If it's the former I'd prefer the simpler > non-invasive thing with a comment saying to lift this to an observer > interface > in the case of a second client needing this info. > > Some additional context: we plan on moving TabLoader's logic into > TabManager for > a couple reasons: > - we want to stagger loading of tabs opened in the background (pretty much > the > same logic) > - we want a single "lifecycle" engine to reason about when to evict tabs > (discard) and when to load them (session restore, background tab opening, > reloading discarded tabs) > > TabManager would be the only place that is aware of a group of > background-opened > tabs being loaded, which is similar in spirit to an "ongoing session > restore". > Would it make sense in your mind to have TabManager expose the observer > interface? > > https://codereview.chromium.org/2935183002/ > -- 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.
On 2017/06/21 14:43:51, chrisha wrote: > On 2017/06/21 00:40:37, sky wrote: > > > https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... > > File chrome/browser/sessions/tab_loader.cc (right): > > > > > https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... > > chrome/browser/sessions/tab_loader.cc:110: > > g_browser_process->GetTabManager()->OnSessionRestoreStarted(); > > Putting this here isn't entirely correct as TabLoader is created *after* > session > > restore has waited to read the file. > > Fair point. Our purpose is to track tabs which are loaded during session restore. Current signals in the constructor and destructor of TabManager mark the start and end of the loading of (non-deferred) tabs in session restore, so I think they would be sufficient approximation. > > > I also think other places may be interested in tracking the lifetime of > session > > restore so that I would prefer a generic observer interface. TabLoader > shouldn't > > have a direct dependency on TabManager, it should only care about notifying > > observers. > > Is this a case of building something we *think* might be used, or are there > clients that would use this right now? If it's the former I'd prefer the simpler > non-invasive thing with a comment saying to lift this to an observer interface > in the case of a second client needing this info. > > Some additional context: we plan on moving TabLoader's logic into TabManager for > a couple reasons: > - we want to stagger loading of tabs opened in the background (pretty much the > same logic) > - we want a single "lifecycle" engine to reason about when to evict tabs > (discard) and when to load them (session restore, background tab opening, > reloading discarded tabs) > > TabManager would be the only place that is aware of a group of background-opened > tabs being loaded, which is similar in spirit to an "ongoing session restore". > Would it make sense in your mind to have TabManager expose the observer > interface?
On 2017/06/22 16:20:59, ducbui wrote: > On 2017/06/21 14:43:51, chrisha wrote: > > On 2017/06/21 00:40:37, sky wrote: > > > > > > https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... > > > File chrome/browser/sessions/tab_loader.cc (right): > > > > > > > > > https://codereview.chromium.org/2935183002/diff/140001/chrome/browser/session... > > > chrome/browser/sessions/tab_loader.cc:110: > > > g_browser_process->GetTabManager()->OnSessionRestoreStarted(); > > > Putting this here isn't entirely correct as TabLoader is created *after* > > session > > > restore has waited to read the file. > > > > Fair point. > > Our purpose is to track tabs which are loaded during session restore. Current > signals in the constructor and destructor of TabManager mark the start and end > of the loading of (non-deferred) tabs in session restore, so I think they would > be sufficient approximation. I uploaded a new CL which changes the function names SessionRestore[Started,Ended] into SessionRestoreTabloading[Started,Ended]. The new names describe more accurately what we really track here: whether tabs are being loaded (by TabLoader) during session restore or not. > > > > > I also think other places may be interested in tracking the lifetime of > > session > > > restore so that I would prefer a generic observer interface. TabLoader > > shouldn't > > > have a direct dependency on TabManager, it should only care about notifying > > > observers. > > > > Is this a case of building something we *think* might be used, or are there > > clients that would use this right now? If it's the former I'd prefer the > simpler > > non-invasive thing with a comment saying to lift this to an observer interface > > in the case of a second client needing this info. > > > > Some additional context: we plan on moving TabLoader's logic into TabManager > for > > a couple reasons: > > - we want to stagger loading of tabs opened in the background (pretty much the > > same logic) > > - we want a single "lifecycle" engine to reason about when to evict tabs > > (discard) and when to load them (session restore, background tab opening, > > reloading discarded tabs) > > > > TabManager would be the only place that is aware of a group of > background-opened > > tabs being loaded, which is similar in spirit to an "ongoing session restore". > > Would it make sense in your mind to have TabManager expose the observer > > interface? After some talks with sky@ and lpy@, the SessionRestoreObserver implementation seems to need bigger changes to the code so we do not include them into this CL. sky@, chrisha@: could you please PTAL this new CL? Thanks.
On Thu, Jun 22, 2017 at 5:54 PM, <ducbui@google.com> wrote: > On 2017/06/22 16:20:59, ducbui wrote: > > On 2017/06/21 14:43:51, chrisha wrote: > > > On 2017/06/21 00:40:37, sky wrote: > > > > > > > > > > https://codereview.chromium.org/2935183002/diff/140001/ > chrome/browser/sessions/tab_loader.cc > > > > File chrome/browser/sessions/tab_loader.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2935183002/diff/140001/ > chrome/browser/sessions/tab_loader.cc#newcode110 > > > > chrome/browser/sessions/tab_loader.cc:110: > > > > g_browser_process->GetTabManager()->OnSessionRestoreStarted(); > > > > Putting this here isn't entirely correct as TabLoader is created > *after* > > > session > > > > restore has waited to read the file. > > > > > > Fair point. > > > > Our purpose is to track tabs which are loaded during session restore. > Current > > signals in the constructor and destructor of TabManager mark the start > and end > > of the loading of (non-deferred) tabs in session restore, so I think they > would > > be sufficient approximation. > > I uploaded a new CL which changes the function names > SessionRestore[Started,Ended] into SessionRestoreTabloading[Started,Ended]. > The > new names describe more accurately what we really track here: whether tabs > are > being loaded (by TabLoader) during session restore or not. > This SGTM. > > > > > > > > > I also think other places may be interested in tracking the lifetime > of > > > session > > > > restore so that I would prefer a generic observer interface. > TabLoader > > > shouldn't > > > > have a direct dependency on TabManager, it should only care about > notifying > > > > observers. > > > > > > Is this a case of building something we *think* might be used, or are > there > > > clients that would use this right now? If it's the former I'd prefer > the > > simpler > > > non-invasive thing with a comment saying to lift this to an observer > interface > > > in the case of a second client needing this info. > > > > > > Some additional context: we plan on moving TabLoader's logic into > TabManager > > for > > > a couple reasons: > > > - we want to stagger loading of tabs opened in the background (pretty > much > the > > > same logic) > > > - we want a single "lifecycle" engine to reason about when to evict > tabs > > > (discard) and when to load them (session restore, background tab > opening, > > > reloading discarded tabs) > > > > > > TabManager would be the only place that is aware of a group of > > background-opened > > > tabs being loaded, which is similar in spirit to an "ongoing session > restore". > > > Would it make sense in your mind to have TabManager expose the observer > > > interface? > > After some talks with sky@ and lpy@, the SessionRestoreObserver > implementation > seems to need bigger changes to the code so we do not include them into > this CL. > Sorry if I conveyed that. I think you should do that change in this patch. It's always better to start with the right architecture rather than having to change things after the fact. -Scott > > sky@, chrisha@: could you please PTAL this new CL? Thanks. > > https://codereview.chromium.org/2935183002/ > -- 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.
lpy@chromium.org changed reviewers: + lpy@chromium.org
I left some comments https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.cc:140: in_session_restore_tab_loading_(false), is_in_session_restore_tab_loading_ https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.h:397: class SessionRestoreObserverInstance : public SessionRestoreObserver { Let's put this class to tab_manager.cc, and use different name, maybe TabManagerSessionRestoreObserver https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.h:410: SessionRestoreObserverInstance session_restore_observer_instance_; just session_restore_observer_ https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:17: static std::unordered_set<SessionRestoreObserver*>* observers(); Let's use static base::ObserverList<SessionRestoreObserver>& observers(); https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:23: static std::unordered_set<SessionRestoreObserver*>* observers_; Let's use base::ObserverList<SessionRestoreObserver>* https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... chrome/browser/sessions/tab_loader.cc:107: for (auto* observer : *SessionRestoreObserver::observers()) Once use base::ObserverList, this should be auto&
https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.cc:140: in_session_restore_tab_loading_(false), On 2017/06/23 22:35:57, lpy wrote: > is_in_session_restore_tab_loading_ Done. https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.h:397: class SessionRestoreObserverInstance : public SessionRestoreObserver { On 2017/06/23 22:35:57, lpy wrote: > Let's put this class to tab_manager.cc, and use different name, maybe > TabManagerSessionRestoreObserver Done. https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.h:410: SessionRestoreObserverInstance session_restore_observer_instance_; On 2017/06/23 22:35:57, lpy wrote: > just session_restore_observer_ Done. https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:17: static std::unordered_set<SessionRestoreObserver*>* observers(); On 2017/06/23 22:35:57, lpy wrote: > Let's use static base::ObserverList<SessionRestoreObserver>& observers(); Done. https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:23: static std::unordered_set<SessionRestoreObserver*>* observers_; On 2017/06/23 22:35:57, lpy wrote: > Let's use base::ObserverList<SessionRestoreObserver>* Done. https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/session... chrome/browser/sessions/tab_loader.cc:107: for (auto* observer : *SessionRestoreObserver::observers()) On 2017/06/23 22:35:57, lpy wrote: > Once use base::ObserverList, this should be auto& Done.
lgtm sky@ could you please take a look?
https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.cc (right): https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.cc:21: DCHECK(!observers().HasObserver(observer)) << "Duplicate registration"; There is no need for this DCHECK, it's in ObserverList. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.cc:27: DCHECK(observers().HasObserver(observer)); Remove DCHECK. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:15: virtual void OnTabLoadingStarted() {} Clearly document at what point these functions are called. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:15: virtual void OnTabLoadingStarted() {} Given these functions are on the SessionRestore interface I think it better to name them something like OnSessionRestoreStartedLoadingTabs and OnSessionRestoreFinishedLoadingTabs. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:15: virtual void OnTabLoadingStarted() {} Please add test coverage that these functions are called. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:20: static void AddObserver(SessionRestoreObserver* observer); Move these functions onto SessionRestore. Also, the callback in SessionRestore should be replaced by your observer, but that can be done separately.
I have just uploaded a new patchset to address new comments. sky@: Could you PTAL? Thanks. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.cc (right): https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.cc:21: DCHECK(!observers().HasObserver(observer)) << "Duplicate registration"; On 2017/06/26 15:34:10, sky wrote: > There is no need for this DCHECK, it's in ObserverList. Done. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.cc:27: DCHECK(observers().HasObserver(observer)); On 2017/06/26 15:34:11, sky wrote: > Remove DCHECK. Done. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:15: virtual void OnTabLoadingStarted() {} On 2017/06/26 15:34:11, sky wrote: > Clearly document at what point these functions are called. Done. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:15: virtual void OnTabLoadingStarted() {} On 2017/06/26 15:34:11, sky wrote: > Given these functions are on the SessionRestore interface I think it better to > name them something like OnSessionRestoreStartedLoadingTabs and > OnSessionRestoreFinishedLoadingTabs. Done. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:15: virtual void OnTabLoadingStarted() {} On 2017/06/26 15:34:11, sky wrote: > Please add test coverage that these functions are called. Done. https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:20: static void AddObserver(SessionRestoreObserver* observer); On 2017/06/26 15:34:11, sky wrote: > Move these functions onto SessionRestore. Done. > Also, the callback in SessionRestore should be replaced by your observer, but > that can be done separately. Do you mean the callback in SessionRestore::RegisterOnSessionRestoredCallback()? Do you want me to include the replacement in this CL?
https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:20: static void AddObserver(SessionRestoreObserver* observer); On 2017/06/26 19:59:26, ducbui wrote: > On 2017/06/26 15:34:11, sky wrote: > > Move these functions onto SessionRestore. > > Done. > > > Also, the callback in SessionRestore should be replaced by your observer, but > > that can be done separately. > > Do you mean the callback in SessionRestore::RegisterOnSessionRestoredCallback()? Exactly. > Do you want me to include the replacement in this CL? That would be great, but I'm ok if you want to push that to a follow on cl. https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer_unittest.cc (right): https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) (see chromium style guide). https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:33: DCHECK(index < session_restore_events_.size()); This is rather awkward. Why don't you just return the vector? e.g. const std::vector<SessionRestoreEvent>& session_restore_events() const { return ... } https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:87: mock_observer_.GetSessionRestoreEventAt(0), You should assert the size of the vector, otherwise you'll trigger a crash and the remaining unit tests won't execute. By that I mean here you want: ASSERT_EQ(1u, mock_observer_.session_restore_events().size(); and below you want to assert the size is 2u. https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:88: MockSessionRestoreObserver::SessionRestoreEvent::STARTED_LOADING_TABS); Generally we use the format "expected, actual", so swap the ordering here and of others.
I have just addressed the new comments. sky@: Could you PTAL? Thanks. https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer_unittest.cc (right): https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/26 21:38:09, sky wrote: > no (c) (see chromium style guide). Done. I got the (c) when I copied the license from another file. https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:33: DCHECK(index < session_restore_events_.size()); On 2017/06/26 21:38:09, sky wrote: > This is rather awkward. Why don't you just return the vector? e.g. > const std::vector<SessionRestoreEvent>& session_restore_events() const { return > ... } I am fine with the change. Your method looks cleaner. I also added a couple of wrappers for frequent uses in the tests. https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:87: mock_observer_.GetSessionRestoreEventAt(0), On 2017/06/26 21:38:09, sky wrote: > You should assert the size of the vector, otherwise you'll trigger a crash and > the remaining unit tests won't execute. By that I mean here you want: > ASSERT_EQ(1u, mock_observer_.session_restore_events().size(); > and below you want to assert the size is 2u. Done. https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:88: MockSessionRestoreObserver::SessionRestoreEvent::STARTED_LOADING_TABS); On 2017/06/26 21:38:09, sky wrote: > Generally we use the format "expected, actual", so swap the ordering here and of > others. Done.
LGTM https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:16: // loading tabs in session restore. Please also document that in the case of memory pressure not all WebContents may have been created. https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer_unittest.cc (right): https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:98: EXPECT_EQ(1u, number_of_session_restore_events()); You need to ASSERT when checking sizes, otherwise the next line will crash (bringing down all other tests).
https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.h:161: bool IsSessionRestoreLoadingTabs() const { I changed this function name from "IsInSessionRestoreTabLoading()" to "IsSessionRestoreLoadingTabs()" so that it become consistent with methods of SessionRestoreObserver.
https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... chrome/browser/sessions/session_restore_observer.h:16: // loading tabs in session restore. On 2017/06/27 00:11:53, sky wrote: > Please also document that in the case of memory pressure not all WebContents may > have been created. Done. https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... File chrome/browser/sessions/session_restore_observer_unittest.cc (right): https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/session... chrome/browser/sessions/session_restore_observer_unittest.cc:98: EXPECT_EQ(1u, number_of_session_restore_events()); On 2017/06/27 00:11:53, sky wrote: > You need to ASSERT when checking sizes, otherwise the next line will crash > (bringing down all other tests). Done.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ducbui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmeawad@chromium.org, zhenw@google.com, chrisha@chromium.org, lpy@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2935183002/#ps320001 (title: "Enable SessionRestoreObserverTest only when session service available.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 320001, "attempt_start_ts": 1498582913559920, "parent_rev": "4305ff0e2b75705c2cc96603e8c202eade920106", "commit_rev": "1b8597354a9400f5708d6e1476a51aff1b79ef15"}
Message was sent while issue was closed.
Description was changed from ========== Add signals that mark the start and end of session restore. Currently, there is no easy way to know whether we are during session restore or not, so it is necessary to add some signals to do so. Since the TabLoader lifetime is equal to the session restore, I made TabLoader to signal the start/end of session restore to the global TabManager. BUG=733012 ========== to ========== Add signals that mark the start and end of session restore. Currently, there is no easy way to know whether we are during session restore or not, so it is necessary to add some signals to do so. Since the TabLoader lifetime is equal to the session restore, I made TabLoader to signal the start/end of session restore to the global TabManager. BUG=733012 Review-Url: https://codereview.chromium.org/2935183002 Cr-Commit-Position: refs/heads/master@{#482654} Committed: https://chromium.googlesource.com/chromium/src/+/1b8597354a9400f5708d6e1476a5... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/1b8597354a9400f5708d6e1476a5...
Message was sent while issue was closed.
https://codereview.chromium.org/2935183002/diff/320001/chrome/browser/session... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2935183002/diff/320001/chrome/browser/session... chrome/browser/sessions/session_restore.h:138: static base::ObserverList<SessionRestoreObserver>* observers_; Shouldn't this explicitly use a leaky lazy instance, or some other annotation that explains that this leaks? Otherwise we could end up with LSAN errors... |