Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2494533002:
[Sync] Put session tracker in charge of maintaining local state. (Closed)
|
Created:
4 years, 1 month ago by Nicolas Zea Modified:
4 years, 1 month ago CC:
chromium-reviews, sync-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Put session tracker in charge of maintaining local state.
Previously, the session tracker was only updated when local tabs were
available in the TabModel. It was not updated with sync data from a previous
session (and as a result, we had to pass around the restored_data list
at association time in order to handle restoring placeholder tabs).
The session tracker now gets updated at startup of previous local state,
which is then used when reassociating restored tabs. This is a purely
refactoring change that should not introduce external behavioral changes
(although internally the data is managed in a different way).
To make this happen, InitFromSyncModel now treats local tabs and remote
tabs the same. In addition, the SyncedSessionTracker now has a
ReassociateTab method that can be called to update the tab id of a
SessionTab.
This is all necessary in order to enable preserving tabs from a previous
session when they're not present in the TabModel (which can happen on
Android when a custom tab is opened, but the main tabbed activity is not
loaded).
BUG=639009
Committed: https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f
Cr-Commit-Position: refs/heads/master@{#432006}
Patch Set 1 #Patch Set 2 : Self review #Patch Set 3 : Rebase #
Total comments: 15
Patch Set 4 : Address feedback #
Total comments: 2
Patch Set 5 : Rebase and update comment #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 34 (21 generated)
The CQ bit was checked by zea@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
zea@chromium.org changed reviewers: + maxbogue@chromium.org
PTAL
maxbogue@chromium.org changed reviewers: + skym@chromium.org
Mostly LG with nits and a question, but I want to do another review pass tomorrow and make sure I understand. I'd also like +Sky to do a quick review pass, as he is far more familiar with the sessions code than I am. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:357: // Note: on some platforms the tab object may have changed, so we ensure This note is now obsolete. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:966: sessions::SessionTab* session_tab = nullptr; Is there any particular reason this is a separate line before the GetTab line? Seems unnecessary. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:997: LOG(ERROR) << "Failed to restore tab " << new_tab_id; Maybe "Failed to reassociate tab "? https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:1008: LOG(ERROR) << "Failed to restore tab " << new_tab_id; Maybe "Failed to update local tab map for "? https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/sessions_sync_manager.h (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.h:265: // Returns true on success, false on failure. Still looks void to me. This from an earlier revision? https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:342: bool SyncedSessionTracker::ReassociateTab(const std::string& session_tag, So was this entire operation just never done previously? I'm slightly confused by that. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/synced_session_tracker.h (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/synced_session_tracker.h:140: // Returns true on success, false on failure. nit: I'd prefer "Returns whether the reassociation succeeded." Apparently I like the word "whether" a lot.
PTAL https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:357: // Note: on some platforms the tab object may have changed, so we ensure On 2016/11/10 06:43:25, maxbogue wrote: > This note is now obsolete. Done. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:966: sessions::SessionTab* session_tab = nullptr; On 2016/11/10 06:43:25, maxbogue wrote: > Is there any particular reason this is a separate line before the GetTab line? > Seems unnecessary. Done. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:997: LOG(ERROR) << "Failed to restore tab " << new_tab_id; On 2016/11/10 06:43:26, maxbogue wrote: > Maybe "Failed to reassociate tab "? Done. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:1008: LOG(ERROR) << "Failed to restore tab " << new_tab_id; On 2016/11/10 06:43:25, maxbogue wrote: > Maybe "Failed to update local tab map for "? Done. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/sessions_sync_manager.h (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/sessions_sync_manager.h:265: // Returns true on success, false on failure. On 2016/11/10 06:43:26, maxbogue wrote: > Still looks void to me. This from an earlier revision? Woops, yes, removed. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:342: bool SyncedSessionTracker::ReassociateTab(const std::string& session_tag, On 2016/11/10 06:43:26, maxbogue wrote: > So was this entire operation just never done previously? I'm slightly confused > by that. Yeah, previously at startup we would put the tab entities in the tab node pool, but not add their data into the tracker. We'd only add a tab into the tracker when it was created locally. And at that point the tab id would never change. Now that we're loading tabs from old sessions into the tracker, we need this functionality. https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/synced_session_tracker.h (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/synced_session_tracker.h:140: // Returns true on success, false on failure. On 2016/11/10 06:43:26, maxbogue wrote: > nit: I'd prefer "Returns whether the reassociation succeeded." Apparently I like > the word "whether" a lot. Done.
https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2494533002/diff/40001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:342: bool SyncedSessionTracker::ReassociateTab(const std::string& session_tag, On 2016/11/10 18:25:36, Nicolas Zea wrote: > On 2016/11/10 06:43:26, maxbogue wrote: > > So was this entire operation just never done previously? I'm slightly confused > > by that. > > Yeah, previously at startup we would put the tab entities in the tab node pool, > but not add their data into the tracker. We'd only add a tab into the tracker > when it was created locally. And at that point the tab id would never change. > > Now that we're loading tabs from old sessions into the tracker, we need this > functionality. So this CL is changing functionality? Could you update the description in that case? https://codereview.chromium.org/2494533002/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.h (right): https://codereview.chromium.org/2494533002/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.h:122: // Keep all the links to local tab data in one place. A tab_node_id and tab Update comment.
Description was changed from ========== [Sync] Put session tracker in charge of maintaining local state. The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce functional changes. BUG=639009 ========== to ========== [Sync] Put session tracker in charge of maintaining local state. The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). BUG=639009 ==========
The CQ bit was checked by zea@chromium.org to run a CQ dry run
Patchset #5 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by zea@chromium.org to run a CQ dry run
PTAL
https://codereview.chromium.org/2494533002/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.h (right): https://codereview.chromium.org/2494533002/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.h:122: // Keep all the links to local tab data in one place. A tab_node_id and tab On 2016/11/10 22:27:14, maxbogue wrote: > Update comment. Done.
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.
lgtm, but please wait for Sky to review (sorry about yesterday; he missed that I actually added him as a reviewer). Also, please expand on the CL description so it has three paragraphs: how it worked before, how it works now, and the justification for the change. Even after reading through this CL ~4 times I still only see the "how it works now" part of that. https://codereview.chromium.org/2494533002/diff/120001/components/sync_sessio... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2494533002/diff/120001/components/sync_sessio... components/sync_sessions/sessions_sync_manager.cc:666: make_linked_ptr<TabLink>(new TabLink(specifics.tab_node_id())); Seems like we should probably move this class to unique_ptrs in a future CL? Or am I missing something different about linked_ptr?
Description was changed from ========== [Sync] Put session tracker in charge of maintaining local state. The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). BUG=639009 ========== to ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were loaded from memory. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. BUG=639009 ==========
Description was changed from ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were loaded from memory. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. BUG=639009 ========== to ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were loaded from memory. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. This is all necessary in order to enable preserving tabs from a previous session when they're not present in the TabModel (which can happen on Android when a custom tab is opened, but the main tabbed activity is not loaded). BUG=639009 ==========
Description was changed from ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were loaded from memory. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. This is all necessary in order to enable preserving tabs from a previous session when they're not present in the TabModel (which can happen on Android when a custom tab is opened, but the main tabbed activity is not loaded). BUG=639009 ========== to ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were available in the TabModel. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. This is all necessary in order to enable preserving tabs from a previous session when they're not present in the TabModel (which can happen on Android when a custom tab is opened, but the main tabbed activity is not loaded). BUG=639009 ==========
Done. https://codereview.chromium.org/2494533002/diff/120001/components/sync_sessio... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2494533002/diff/120001/components/sync_sessio... components/sync_sessions/sessions_sync_manager.cc:666: make_linked_ptr<TabLink>(new TabLink(specifics.tab_node_id())); On 2016/11/11 17:05:31, maxbogue wrote: > Seems like we should probably move this class to unique_ptrs in a future CL? Or > am I missing something different about linked_ptr? Yeah, the main reason to use linked ptr here was we didn't have move semantics before within STL containers. Now that we do unique ptr should be enough.
Ahhh that description is very helpful, thank you! I'm much more comfortable with the change now. I think it's fine to go ahead and land this now; I don't want to block you any longer since Sky is out sick. He can glance over it next week.
The CQ bit was checked by zea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were available in the TabModel. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. This is all necessary in order to enable preserving tabs from a previous session when they're not present in the TabModel (which can happen on Android when a custom tab is opened, but the main tabbed activity is not loaded). BUG=639009 ========== to ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were available in the TabModel. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. This is all necessary in order to enable preserving tabs from a previous session when they're not present in the TabModel (which can happen on Android when a custom tab is opened, but the main tabbed activity is not loaded). BUG=639009 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were available in the TabModel. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. This is all necessary in order to enable preserving tabs from a previous session when they're not present in the TabModel (which can happen on Android when a custom tab is opened, but the main tabbed activity is not loaded). BUG=639009 ========== to ========== [Sync] Put session tracker in charge of maintaining local state. Previously, the session tracker was only updated when local tabs were available in the TabModel. It was not updated with sync data from a previous session (and as a result, we had to pass around the restored_data list at association time in order to handle restoring placeholder tabs). The session tracker now gets updated at startup of previous local state, which is then used when reassociating restored tabs. This is a purely refactoring change that should not introduce external behavioral changes (although internally the data is managed in a different way). To make this happen, InitFromSyncModel now treats local tabs and remote tabs the same. In addition, the SyncedSessionTracker now has a ReassociateTab method that can be called to update the tab id of a SessionTab. This is all necessary in order to enable preserving tabs from a previous session when they're not present in the TabModel (which can happen on Android when a custom tab is opened, but the main tabbed activity is not loaded). BUG=639009 Committed: https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f Cr-Commit-Position: refs/heads/master@{#432006} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f Cr-Commit-Position: refs/heads/master@{#432006}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/2507543002/ by zea@chromium.org. The reason for reverting is: crbug.com/665314 crbug.com/665524. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
