Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(268)

Issue 2935183002: [TabMetrics] Add signals that mark the start and end of session restore. (Closed)

Created:
3 years, 6 months ago by ducbui
Modified:
3 years, 5 months ago
CC:
chromium-reviews, chrome-grc-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+278 lines, -0 lines) Patch
M chrome/browser/resource_coordinator/tab_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/resource_coordinator/tab_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +36 lines, -0 lines 0 comments Download
M chrome/browser/resource_coordinator/tab_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +23 lines, -0 lines 0 comments Download
M chrome/browser/sessions/session_restore.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +17 lines, -0 lines 1 comment Download
M chrome/browser/sessions/session_restore.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +14 lines, -0 lines 0 comments Download
A chrome/browser/sessions/session_restore_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +21 lines, -0 lines 0 comments Download
A chrome/browser/sessions/session_restore_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +148 lines, -0 lines 0 comments Download
M chrome/browser/sessions/tab_loader.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 71 (30 generated)
ducbui
PTAL
3 years, 6 months ago (2017-06-13 23:18:23 UTC) #3
fmeawad
+zhenw Overall looks good. https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc#newcode324 chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) Can this be ...
3 years, 6 months ago (2017-06-14 15:34:58 UTC) #5
ducbui
On 2017/06/14 15:34:58, fmeawad wrote: > +zhenw > > Overall looks good. > > https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc ...
3 years, 6 months ago (2017-06-14 16:14:44 UTC) #6
fmeawad
https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc#newcode324 chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) On 2017/06/14 15:34:58, fmeawad wrote: > Can ...
3 years, 6 months ago (2017-06-14 16:20:57 UTC) #7
ducbui
https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc#newcode324 chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) On 2017/06/14 16:20:56, fmeawad wrote: > On ...
3 years, 6 months ago (2017-06-14 16:24:18 UTC) #8
ducbui
https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/1/chrome/browser/sessions/tab_loader.cc#newcode324 chrome/browser/sessions/tab_loader.cc:324: if (tab_manager) On 2017/06/14 16:24:18, ducbui wrote: > On ...
3 years, 6 months ago (2017-06-14 16:47:39 UTC) #9
Zhen Wang
Can you also add some test?
3 years, 6 months ago (2017-06-14 17:04:29 UTC) #10
lpy
overall LG to me, some tiny nits. https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource_coordinator/tab_manager.h File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource_coordinator/tab_manager.h#newcode157 chrome/browser/resource_coordinator/tab_manager.h:157: void MarkSessionRestoreStarted() ...
3 years, 6 months ago (2017-06-14 17:23:20 UTC) #11
ducbui
https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource_coordinator/tab_manager.h File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/20001/chrome/browser/resource_coordinator/tab_manager.h#newcode157 chrome/browser/resource_coordinator/tab_manager.h:157: void MarkSessionRestoreStarted() { is_during_session_restore_ = true; } On 2017/06/14 ...
3 years, 6 months ago (2017-06-14 17:37:58 UTC) #12
ducbui
Hi Oystein oystein@, PTAL. Thanks.
3 years, 6 months ago (2017-06-15 16:47:59 UTC) #14
zhenw
lgtm
3 years, 6 months ago (2017-06-15 16:59:12 UTC) #16
fmeawad
lgtm https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions/tab_loader.cc File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions/tab_loader.cc#newcode324 chrome/browser/sessions/tab_loader.cc:324: DCHECK(tab_manager); I would just DCHECK(g_browser_process->GetTabManager()) and avoid creating ...
3 years, 6 months ago (2017-06-15 17:00:43 UTC) #17
ducbui
https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions/tab_loader.cc File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/80001/chrome/browser/sessions/tab_loader.cc#newcode324 chrome/browser/sessions/tab_loader.cc:324: DCHECK(tab_manager); On 2017/06/15 17:00:43, fmeawad wrote: > I would ...
3 years, 6 months ago (2017-06-15 17:38:09 UTC) #18
fmeawad
https://codereview.chromium.org/2935183002/diff/100001/chrome/browser/sessions/tab_loader.cc File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2935183002/diff/100001/chrome/browser/sessions/tab_loader.cc#newcode108 chrome/browser/sessions/tab_loader.cc:108: OnSessionRestoreStarted(); lpy and I thinks that these calls should ...
3 years, 6 months ago (2017-06-15 17:55:19 UTC) #19
Zhen Wang
Hi Duc, Just a general comment on the workflow. In general, when you want the ...
3 years, 6 months ago (2017-06-15 17:56:55 UTC) #20
Zhen Wang
+chrisha
3 years, 6 months ago (2017-06-15 19:09:38 UTC) #22
chrisha
There's other code in-flight doing something very similar (talk to zhenw), as it is needed ...
3 years, 6 months ago (2017-06-15 20:23:42 UTC) #27
Zhen Wang
On 2017/06/15 20:23:42, chrisha wrote: > There's other code in-flight doing something very similar (talk ...
3 years, 6 months ago (2017-06-15 21:56:57 UTC) #28
ducbui
On 2017/06/15 21:56:57, Zhen Wang wrote: > On 2017/06/15 20:23:42, chrisha wrote: > > There's ...
3 years, 6 months ago (2017-06-19 20:38:52 UTC) #29
chrisha
lgtm Note that we want to move TabLoader's logic into TabManager once the background tab ...
3 years, 6 months ago (2017-06-20 18:35:01 UTC) #34
ducbui
Hi sky@, Could PTAL changes in tab_loader.cc. Thanks.
3 years, 6 months ago (2017-06-20 23:05:41 UTC) #38
sky
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 ...
3 years, 6 months ago (2017-06-21 00:40:37 UTC) #39
chrisha
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 > ...
3 years, 6 months ago (2017-06-21 14:43:51 UTC) #40
chrisha
> TabManager would be the only place that is aware of a group of background-opened ...
3 years, 6 months ago (2017-06-21 14:57:38 UTC) #41
sky
On Wed, Jun 21, 2017 at 7:43 AM, <chrisha@chromium.org> wrote: > On 2017/06/21 00:40:37, sky ...
3 years, 6 months ago (2017-06-21 19:32:08 UTC) #42
ducbui
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 ...
3 years, 6 months ago (2017-06-22 16:20:59 UTC) #43
ducbui
On 2017/06/22 16:20:59, ducbui wrote: > On 2017/06/21 14:43:51, chrisha wrote: > > On 2017/06/21 ...
3 years, 6 months ago (2017-06-23 00:54:36 UTC) #44
sky
On Thu, Jun 22, 2017 at 5:54 PM, <ducbui@google.com> wrote: > On 2017/06/22 16:20:59, ducbui ...
3 years, 6 months ago (2017-06-23 03:40:08 UTC) #45
lpy
I left some comments https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resource_coordinator/tab_manager.cc File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resource_coordinator/tab_manager.cc#newcode140 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/resource_coordinator/tab_manager.h File chrome/browser/resource_coordinator/tab_manager.h ...
3 years, 6 months ago (2017-06-23 22:35:58 UTC) #47
ducbui
https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resource_coordinator/tab_manager.cc File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2935183002/diff/220001/chrome/browser/resource_coordinator/tab_manager.cc#newcode140 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. ...
3 years, 6 months ago (2017-06-23 23:30:15 UTC) #48
lpy
lgtm sky@ could you please take a look?
3 years, 6 months ago (2017-06-23 23:37:15 UTC) #49
sky
https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/sessions/session_restore_observer.cc File chrome/browser/sessions/session_restore_observer.cc (right): https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/sessions/session_restore_observer.cc#newcode21 chrome/browser/sessions/session_restore_observer.cc:21: DCHECK(!observers().HasObserver(observer)) << "Duplicate registration"; There is no need for ...
3 years, 5 months ago (2017-06-26 15:34:11 UTC) #50
ducbui
I have just uploaded a new patchset to address new comments. sky@: Could you PTAL? ...
3 years, 5 months ago (2017-06-26 19:59:26 UTC) #51
sky
https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/sessions/session_restore_observer.h File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/240001/chrome/browser/sessions/session_restore_observer.h#newcode20 chrome/browser/sessions/session_restore_observer.h:20: static void AddObserver(SessionRestoreObserver* observer); On 2017/06/26 19:59:26, ducbui wrote: ...
3 years, 5 months ago (2017-06-26 21:38:09 UTC) #52
ducbui
I have just addressed the new comments. sky@: Could you PTAL? Thanks. https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/sessions/session_restore_observer_unittest.cc File chrome/browser/sessions/session_restore_observer_unittest.cc ...
3 years, 5 months ago (2017-06-26 22:30:59 UTC) #53
sky
LGTM https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/sessions/session_restore_observer.h File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/sessions/session_restore_observer.h#newcode16 chrome/browser/sessions/session_restore_observer.h:16: // loading tabs in session restore. Please also ...
3 years, 5 months ago (2017-06-27 00:11:53 UTC) #54
ducbui
https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/resource_coordinator/tab_manager.h File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2935183002/diff/260001/chrome/browser/resource_coordinator/tab_manager.h#newcode161 chrome/browser/resource_coordinator/tab_manager.h:161: bool IsSessionRestoreLoadingTabs() const { I changed this function name ...
3 years, 5 months ago (2017-06-27 00:12:49 UTC) #55
ducbui
https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/sessions/session_restore_observer.h File chrome/browser/sessions/session_restore_observer.h (right): https://codereview.chromium.org/2935183002/diff/280001/chrome/browser/sessions/session_restore_observer.h#newcode16 chrome/browser/sessions/session_restore_observer.h:16: // loading tabs in session restore. On 2017/06/27 00:11:53, ...
3 years, 5 months ago (2017-06-27 00:45:03 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2935183002/320001
3 years, 5 months ago (2017-06-27 17:02:10 UTC) #67
commit-bot: I haz the power
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/1b8597354a9400f5708d6e1476a51aff1b79ef15
3 years, 5 months ago (2017-06-27 17:07:03 UTC) #70
chrisha
3 years, 5 months ago (2017-06-27 17:09:07 UTC) #71
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...

Powered by Google App Engine
This is Rietveld 408576698