Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499083004:
Reland of [Sync] Put session tracker in charge of maintaining local state. (Closed)
|
Created:
4 years, 1 month ago by Nicolas Zea Modified:
4 years ago Reviewers:
skym CC:
chromium-reviews, sync-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland of [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 user visible 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. To make this work, the TabNodePool has been moved into
the SyncedSessionTracker, and in general data ownership has been
simplified (with quite a few new tests added).
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/159246f269b561bf931fa56a35b2704fab7dcc96
Cr-Commit-Position: refs/heads/master@{#436786}
Patch Set 1 #Patch Set 2 : Compiling and passing tests #Patch Set 3 : More working #Patch Set 4 : Self review #
Total comments: 57
Patch Set 5 : Address comments #
Total comments: 8
Patch Set 6 : Address comments #Messages
Total messages: 26 (15 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: This issue passed the CQ dry run.
Description was changed from ========== Reland of [Sync] Put session tracker in charge of maintaining local state. Original codereview: https://codereview.chromium.org/2494533002 Fixes handling of placeholder tabs (which don't have a tab contents delegate) and free of tab nodes. Tests added to prevent future regressions. BUG=639009 ========== to ========== Reland of [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 user visible 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. To make this work, the TabNodePool has been moved into the SyncedSessionTracker, and in general data ownership has been simplified (with quite a few new tests added). 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 ==========
zea@chromium.org changed reviewers: + skym@chromium.org
Sky, PTAL. This is pretty significantly different from the original patch I landed, as it turned out some assumptions I had in that patch were false. In addition, there were several test cases missing, and I've gone ahead and streamlined/simplified things as much as I could to make things easier to test.
I'm still trying to grok everything here, but here's some minor nits so far. At a high level, I like the idea of moving the pool inside the tracker. And the removal of the un-associated state is great. But trying to get into the details, it gets really confusing. It seems like the tracker treats the tab node id as authoritative while the pool treats the tab id as authoritative. Part of me wonders, maybe it'd be more elegant if you had 3 components here. A tracker that was linking sessions/windows/tabs, a pool that managed tab node ids, and an associator that linked tabs ids and tab node ids, and checks/deals with conflicts and linking through non-association aware API calls to the other two components. It seems like the association logic is everywhere. Maybe that's necessary though. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:14: #include "chrome/browser/sessions/session_tab_helper.h" :/ would be nice if most of these unit tests could live in components/. Maybe there should be a separate test that is performing more of an integration test instead. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:513: class SyncedTabDelegateFake : public SyncedTabDelegate { How that we have two implementations in here, can you add some comments about how it is different? Also, does this thing actually get a WebContents? https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:529: void AppendEntry(std::unique_ptr<content::NavigationEntry> entry) { While you're in here, can you organize these so that all the SyncedTabDelegate override methods are together and labeled, and this one isn't sneaking in the middle of them all? https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:610: // An Android placeholder delegate. These delegates have no WebContents Can you elaborate in this comment what's android specific about this? If you didn't know what was going on, you might infer that all SyncedTabDelegates on android lack WebContents. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:611: // or TabContentsDelegate. I'm confused what exactly a TabContentsDelegate is. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:618: // Overrides. Is there a reason you didn't do // SyncedTabDelegate implementation. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:625: // or a TabContentsDelegate. Again, what is a TabContentsDelegate? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:69: std::string TabNodeIdToTag(const std::string& machine_tag, int tab_node_id) { Is duplicating in anonymous namespaces between .cc and unittest.cc preferred over making it private static and friending? (test is already a friend) https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:668: session_tracker_.ReassociateLocalTab(specifics.tab_node_id(), Err, is this really correct? What if you've already received a OnLocalTabModified for a SyncedTabDelegate with this tab node id, but they've swapped the tab id on you? This would overwrite correct data with stale data, right? Or am I missing something? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:734: DVLOG(1) << "Associating tab " << tab_id << " with node " Nit: We only actually associate the tab_id with tab_node_id if the tab_id is already present in session_tracker_, right? This would be more accurate to say we're "Attempting to associate ..." I guess maybe that's implied. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:245: // SessionWindow information of a SessionHeader node for a foreign session, This #1 case now included local data, right? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:338: // Ensure a placeholder SessionTab is in place, if not already. This sounds like GetTab(...) is going to create a SyncedTabDelegate object that returns true when IsPlaceholderTab() is called. That's not what going on though, right? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/tab_node_pool.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.cc:29: if (max_used_tab_node_id_ < tab_node_id) do you think std::max would be more clear? max_used_tab_node_id_ = std::max(max_used_tab_node_id_, tab_node_id); https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.cc:63: free_nodes_pool_.insert(*tab_node_id); This is kind of odd that you put it in free_nodes_pool_ and then immediately erase it in the functional call below. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/tab_node_pool.h (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.h:25: // tab_tag = StringPrintf("%s_%ui", local_session_tag, tab_node_id); I'm not very familiar with printf functionality, or what %ui does, but this is slightly different from elsewhere. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.h:63: // Removes association for |tab_id| and returns it to the free node pool. ... and returns its tab node id to the free pool. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/tab_node_pool_unittest.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool_unittest.cc:34: const int kTabNodeId1 = 10; Is there a reason these are not constants that are the same for each test case? Also, might be a bit more clear also if kTabNodeId1000 = 1000, etc.
Some more questions as I try to understand things. Thinking about this today, I'm not convinced pushing the pool into the tracker is where we want to end up. But it's okay for now. Someone has to be aware of if we're dealing with a local or a foreign session, and I think that has to be the manager. This change isn't able to fully abstract that away from the manager, as is evident from all the xLocalX() methods the manager calls on the tracker. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:734: DVLOG(1) << "Associating tab " << tab_id << " with node " On 2016/12/03 01:20:46, skym wrote: > Nit: We only actually associate the tab_id with tab_node_id if the tab_id is > already present in session_tracker_, right? This would be more accurate to say > we're "Attempting to associate ..." I guess maybe that's implied. Errm, now I'm confused by my comment from last week. And the code. What's going on here. Which call does the actual "associating"? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:736: session_tracker_.AddTabNode(session_tag, specifics.tab_node_id()); I think AddX(...) is a much better method name than GetX(...) being used to ensure we give data to the tracker when it needs it. But can you add a comment explaining why this call is needed? (Is it both so we can garbage collect orphaned foreign tabs, and so we can re-use existing tab ids for local content? I don't see the tab node id getting passed to the pool though.) I'm really having a hard time understanding the API that the tracker and pool classes have, when they need to get called, and what those methods need to do. All this call is doing is making sure the tracker remembers this tab node id. But from the method name, that's super not obvious. ... Actually the more I understand here, the old approach was awkwardly and clunkily used here, but tried to enforced consistency. If you wanted the sessions::SessionTab to be auto created, then you had to pass a tab node id. Now the tracker just trusts that the manager is going to AddTabNode as well here. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:746: sessions::SessionTab* tab = session_tracker_.GetTab(session_tag, tab_id); Without passing tab node id to this call, why do we have a separate GetTab and LookupSessionTab call? They both take the same arguments and return a sessions::SessionTab object. GetTab(...) does a bunch of creation, but if that's going to happen, then you will never enter the if clause above to break out early. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (left): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:290: // TabIDs are not stable across restarts of a client. Consider this This comment block, at least the part about tab ids not being consistent across restarts, was super helpful to me to understand some of the requirements for the logic around sessions. Would be nice to have this live on somewhere prominent. Similarly, would be nice for the concept of placeholders and not trusting the locally loaded data unless sometimes would be good. Maybe all of these tricky scenarios could be documented in sessions_sync_manager.h https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:355: local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id); Three case here, right? It returns kInvalidTabID, it returns the same value as new_tab_id, or it returns a different value? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:366: // It's possible a placeholder is already in place for the new tab. If Placeholder? Like as in SyncedTabDelegate::IsPlaceholderTab()? It seems like you could have a different tab node id sitting here previously using the |new_tab_id|, and you're stealing their SessionTab. They can always create a new one I suppose, but the one you stole now contains someone else's data, which is wrong for us, right? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:375: tab_ptr = GetTab(local_session_tag_, new_tab_id); ...So there are three cases that GetTab could run into, right? new_tab_id already exists and is mapped, it already exists and is unmapped, and it doesn't exist and is created here and thrown into the unmapped_tabs_ w/ the new id? Except you special case the first (already in synced_tab_map_) in the above logic, and the outcome is the same? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:381: if (old_tab_id != TabNodePool::kInvalidTabID && At this point, I'm wondering, would it have been easier to just erase everything and then re-add everything?
Thanks for the comments, PTAL! https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:14: #include "chrome/browser/sessions/session_tab_helper.h" On 2016/12/03 01:20:46, skym wrote: > :/ would be nice if most of these unit tests could live in components/. Maybe > there should be a separate test that is performing more of an integration test > instead. Yeah, that's valid. I filed crbug.com/671283 for that. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:513: class SyncedTabDelegateFake : public SyncedTabDelegate { On 2016/12/03 01:20:46, skym wrote: > How that we have two implementations in here, can you add some comments about > how it is different? Also, does this thing actually get a WebContents? Done. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:529: void AppendEntry(std::unique_ptr<content::NavigationEntry> entry) { On 2016/12/03 01:20:46, skym wrote: > While you're in here, can you organize these so that all the SyncedTabDelegate > override methods are together and labeled, and this one isn't sneaking in the > middle of them all? Done. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:610: // An Android placeholder delegate. These delegates have no WebContents On 2016/12/03 01:20:46, skym wrote: > Can you elaborate in this comment what's android specific about this? If you > didn't know what was going on, you might infer that all SyncedTabDelegates on > android lack WebContents. Done. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:611: // or TabContentsDelegate. On 2016/12/03 01:20:46, skym wrote: > I'm confused what exactly a TabContentsDelegate is. Looks like they don't exist anymore. Basically they were a wrapper around a WebContents from what I recall. Removed the reference. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:618: // Overrides. On 2016/12/03 01:20:46, skym wrote: > Is there a reason you didn't do > > // SyncedTabDelegate implementation. Nope. Done. https://codereview.chromium.org/2499083004/diff/60001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:625: // or a TabContentsDelegate. On 2016/12/03 01:20:46, skym wrote: > Again, what is a TabContentsDelegate? Removed. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:69: std::string TabNodeIdToTag(const std::string& machine_tag, int tab_node_id) { On 2016/12/03 01:20:46, skym wrote: > Is duplicating in anonymous namespaces between .cc and unittest.cc preferred > over making it private static and friending? (test is already a friend) Given it's just a one-liner with no real logic, it seems simpler this way. I can change if you prefer. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:668: session_tracker_.ReassociateLocalTab(specifics.tab_node_id(), On 2016/12/03 01:20:46, skym wrote: > Err, is this really correct? What if you've already received a > OnLocalTabModified for a SyncedTabDelegate with this tab node id, but they've > swapped the tab id on you? This would overwrite correct data with stale data, > right? Or am I missing something? InitFromSyncModel is called before OnLocalTabModified can be called (InitFromSyncModel is part of the initial associated). https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:734: DVLOG(1) << "Associating tab " << tab_id << " with node " On 2016/12/03 01:20:46, skym wrote: > Nit: We only actually associate the tab_id with tab_node_id if the tab_id is > already present in session_tracker_, right? This would be more accurate to say > we're "Attempting to associate ..." I guess maybe that's implied. Are you referring to the if conditional just below? That's just there to catch cases where two different tab node ids are attempting to refer to the same tab id (which is invalid). Otherwise, we'll create the tab id in the session tracker via the GetTab below. The actual "associating" happens in the caller of this method (InitFromSyncModel), which invokes ReassociateLocalTab. All we're doing here is populating the SessionTab with the appropriate data. You're right that the debug log message isn't quite accurate here. Updated it to better reflect what's happening. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:736: session_tracker_.AddTabNode(session_tag, specifics.tab_node_id()); On 2016/12/05 19:16:57, skym wrote: > I think AddX(...) is a much better method name than GetX(...) being used to > ensure we give data to the tracker when it needs it. But can you add a comment > explaining why this call is needed? (Is it both so we can garbage collect > orphaned foreign tabs, and so we can re-use existing tab ids for local content? > I don't see the tab node id getting passed to the pool though.) > > I'm really having a hard time understanding the API that the tracker and pool > classes have, when they need to get called, and what those methods need to do. > > All this call is doing is making sure the tracker remembers this tab node id. > But from the method name, that's super not obvious. > > ... > > Actually the more I understand here, the old approach was awkwardly and clunkily > used here, but tried to enforced consistency. If you wanted the > sessions::SessionTab to be auto created, then you had to pass a tab node id. Now > the tracker just trusts that the manager is going to AddTabNode as well here. Cleaned up the comments here. PTAL! Also re: AddX vs GetX, the issue is that the SessionTracker doesn't understand specifics, and that's something I kind of want to preserve. It's useful to think of it as only dealing with the bookkeeping, but not the contents. The manager is doing a fair amount of work in the processing of the actual sync entities that I wanted to keep separate, and I think the best way is to provide it direct access to the SessionTabs themselves. And yeah, the old API was I think more clunky, so I pulled it into two methods (since many of the times you call GetTab you don't actually know the tab node associated with it). I hoped that AddTabNode would be clear that it's just adding bookkeeping for the node (as opposed to AddTab, which sounds like it's adding the tab's contents. Maybe this should be OnTabNodeSeen, or AddTabNodeID? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:746: sessions::SessionTab* tab = session_tracker_.GetTab(session_tag, tab_id); On 2016/12/05 19:16:57, skym wrote: > Without passing tab node id to this call, why do we have a separate GetTab and > LookupSessionTab call? They both take the same arguments and return a > sessions::SessionTab object. GetTab(...) does a bunch of creation, but if that's > going to happen, then you will never enter the if clause above to break out > early. Good point, the calls can be combined. Done. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (left): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:290: // TabIDs are not stable across restarts of a client. Consider this On 2016/12/05 19:16:57, skym wrote: > This comment block, at least the part about tab ids not being consistent across > restarts, was super helpful to me to understand some of the requirements for the > logic around sessions. Would be nice to have this live on somewhere prominent. > > Similarly, would be nice for the concept of placeholders and not trusting the > locally loaded data unless sometimes would be good. Maybe all of these tricky > scenarios could be documented in sessions_sync_manager.h Good call. Moved to the SessionSyncManager's UpdateTrackerWithSpecifics method, which is really where this issue matters. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:245: // SessionWindow information of a SessionHeader node for a foreign session, On 2016/12/03 01:20:46, skym wrote: > This #1 case now included local data, right? Correct. Updated. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:338: // Ensure a placeholder SessionTab is in place, if not already. On 2016/12/03 01:20:46, skym wrote: > This sounds like GetTab(...) is going to create a SyncedTabDelegate object that > returns true when IsPlaceholderTab() is called. That's not what going on though, > right? I'm using placeholder like the colloquial term, unrelated to the SyncedTabDelegate (which the tracker knows nothing about). This is just to have consistency by ensuring there's a SessionTab object for each tab node (which helps with testing). https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:355: local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id); On 2016/12/05 19:16:57, skym wrote: > Three case here, right? It returns kInvalidTabID, it returns the same value as > new_tab_id, or it returns a different value? Correct. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:366: // It's possible a placeholder is already in place for the new tab. If On 2016/12/05 19:16:57, skym wrote: > Placeholder? Like as in SyncedTabDelegate::IsPlaceholderTab()? It seems like you > could have a different tab node id sitting here previously using the > |new_tab_id|, and you're stealing their SessionTab. They can always create a new > one I suppose, but the one you stole now contains someone else's data, which is > wrong for us, right? Placeholder like a dummy SessionTab in place to show that the tab exists (which get's created anytime GetTab is called). It's unrelated to the SyncedTabDelegate logic. I'm not sure thinking about it as "someone else's data" is correct. It's still this device's data for that tab id. It's just that it's possibly stale. Also note that that old tab node is automatically freed as part of ReassociateTabNode, so things remain consistent. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:375: tab_ptr = GetTab(local_session_tag_, new_tab_id); On 2016/12/05 19:16:57, skym wrote: > ...So there are three cases that GetTab could run into, right? new_tab_id > already exists and is mapped, it already exists and is unmapped, and it doesn't > exist and is created here and thrown into the unmapped_tabs_ w/ the new id? > > Except you special case the first (already in synced_tab_map_) in the above > logic, and the outcome is the same? Ah, you're right, this conditional is pointless. GetTab is already looking for the new_tab_id, and will reuse the tab if it exists. Fixed! https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:381: if (old_tab_id != TabNodePool::kInvalidTabID && On 2016/12/05 19:16:57, skym wrote: > At this point, I'm wondering, would it have been easier to just erase everything > and then re-add everything? I'm not sure I follow. Erase all tabs for this session, just to reassociate one? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/tab_node_pool.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.cc:29: if (max_used_tab_node_id_ < tab_node_id) On 2016/12/03 01:20:46, skym wrote: > do you think std::max would be more clear? > > max_used_tab_node_id_ = std::max(max_used_tab_node_id_, tab_node_id); Done. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.cc:63: free_nodes_pool_.insert(*tab_node_id); On 2016/12/03 01:20:46, skym wrote: > This is kind of odd that you put it in free_nodes_pool_ and then immediately > erase it in the functional call below. It allows for consistency. AssociateTabNode assumes the node is free. We're basically creating a tab node here, so it makes sense to create it as free first. Changed this to just call AddTabNode instead for clarity. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/tab_node_pool.h (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.h:25: // tab_tag = StringPrintf("%s_%ui", local_session_tag, tab_node_id); On 2016/12/03 01:20:46, skym wrote: > I'm not very familiar with printf functionality, or what %ui does, but this is > slightly different from elsewhere. Huh, this comment is out of date (we don't have an underscore). Updated. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool.h:63: // Removes association for |tab_id| and returns it to the free node pool. On 2016/12/03 01:20:46, skym wrote: > ... and returns its tab node id to the free pool. Done. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/tab_node_pool_unittest.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/tab_node_pool_unittest.cc:34: const int kTabNodeId1 = 10; On 2016/12/03 01:20:46, skym wrote: > Is there a reason these are not constants that are the same for each test case? > Also, might be a bit more clear also if kTabNodeId1000 = 1000, etc. I'm not sure I agree on the naming. It's clearer to me that kTabNodeId3 is associated with kTabId3 (and it's useful to have their values be different for debugger purposes). I went ahead and pulled them all out and shared them across tests though.
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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:736: session_tracker_.AddTabNode(session_tag, specifics.tab_node_id()); On 2016/12/06 01:32:55, Nicolas Zea wrote: > On 2016/12/05 19:16:57, skym wrote: > > I think AddX(...) is a much better method name than GetX(...) being used to > > ensure we give data to the tracker when it needs it. But can you add a comment > > explaining why this call is needed? (Is it both so we can garbage collect > > orphaned foreign tabs, and so we can re-use existing tab ids for local > content? > > I don't see the tab node id getting passed to the pool though.) > > > > I'm really having a hard time understanding the API that the tracker and pool > > classes have, when they need to get called, and what those methods need to do. > > > > All this call is doing is making sure the tracker remembers this tab node id. > > But from the method name, that's super not obvious. > > > > ... > > > > Actually the more I understand here, the old approach was awkwardly and > clunkily > > used here, but tried to enforced consistency. If you wanted the > > sessions::SessionTab to be auto created, then you had to pass a tab node id. > Now > > the tracker just trusts that the manager is going to AddTabNode as well here. > > Cleaned up the comments here. PTAL! > > Also re: AddX vs GetX, the issue is that the SessionTracker doesn't understand > specifics, and that's something I kind of want to preserve. It's useful to think > of it as only dealing with the bookkeeping, but not the contents. The manager is > doing a fair amount of work in the processing of the actual sync entities that I > wanted to keep separate, and I think the best way is to provide it direct access > to the SessionTabs themselves. > > And yeah, the old API was I think more clunky, so I pulled it into two methods > (since many of the times you call GetTab you don't actually know the tab node > associated with it). > > I hoped that AddTabNode would be clear that it's just adding bookkeeping for the > node (as opposed to AddTab, which sounds like it's adding the tab's contents. > Maybe this should be OnTabNodeSeen, or AddTabNodeID? Both options sound good to me. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:338: // Ensure a placeholder SessionTab is in place, if not already. On 2016/12/06 01:32:55, Nicolas Zea wrote: > On 2016/12/03 01:20:46, skym wrote: > > This sounds like GetTab(...) is going to create a SyncedTabDelegate object > that > > returns true when IsPlaceholderTab() is called. That's not what going on > though, > > right? > > I'm using placeholder like the colloquial term, unrelated to the > SyncedTabDelegate (which the tracker knows nothing about). This is just to have > consistency by ensuring there's a SessionTab object for each tab node (which > helps with testing). Ahh, okay, so you're saying that adding one here if it doesn't exist helps with tests? It looks like the only time GetTabNodeForLocalTab is called in non-test code is right before the SyncChange is built, which means there had better be a real SyncTab here. And very not placeholder-y. I also worry that using the colloquial definition of placeholder might be confusing. I'd prefer to see a comment that talks about this from a consistency perspective. // Although we don't need a SessionTab to fulfill this request, this forces the creation of one if it doesn't already exist. This helps to make sure we're tracking this |tab_id| if |local_tab_pool_| is, and everyone's data structures are kept in sync and as consistent as possible. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:366: // It's possible a placeholder is already in place for the new tab. If On 2016/12/06 01:32:55, Nicolas Zea wrote: > On 2016/12/05 19:16:57, skym wrote: > > Placeholder? Like as in SyncedTabDelegate::IsPlaceholderTab()? It seems like > you > > could have a different tab node id sitting here previously using the > > |new_tab_id|, and you're stealing their SessionTab. They can always create a > new > > one I suppose, but the one you stole now contains someone else's data, which > is > > wrong for us, right? > > Placeholder like a dummy SessionTab in place to show that the tab exists (which > get's created anytime GetTab is called). It's unrelated to the SyncedTabDelegate > logic. > > I'm not sure thinking about it as "someone else's data" is correct. It's still > this device's data for that tab id. It's just that it's possibly stale. Also > note that that old tab node is automatically freed as part of > ReassociateTabNode, so things remain consistent. Similar to above, I'm concerned that using placeholder is confusing (it was to me). I'd prefer this comment to explain when/why a SyncTab would already exist. I tried to write down my understanding of what's going on here, but I realized I don't understand when this method should be called. It seems like ReassociateLocalTab isn't being called for non-placeholder tabs. Why not? Why do placeholders need this while non-place holders don't? Shouldn't they both get set up with initial data from sync, and then at some point in the future (the length of gap is the only difference) we get data from delegates and need to worry about correcting tab ids? https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:381: if (old_tab_id != TabNodePool::kInvalidTabID && On 2016/12/06 01:32:55, Nicolas Zea wrote: > On 2016/12/05 19:16:57, skym wrote: > > At this point, I'm wondering, would it have been easier to just erase > everything > > and then re-add everything? > > I'm not sure I follow. Erase all tabs for this session, just to reassociate one? I meant remove everything at tab_node_id, new_tab_id, and old_tab_id. Then create a new SessionTab every time. But looking at this again today, I'm liking your approach more. So never mind! https://codereview.chromium.org/2499083004/diff/80001/chrome/browser/sync/ses... File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2499083004/diff/80001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:519: : current_entry_index_(0), is_supervised_(false), sync_id_(-1) {} It's a bit odd that half of these members fields have their initial values set here, and half are set where the variables are defined. https://codereview.chromium.org/2499083004/diff/80001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1808: TEST_F(SessionsSyncManagerTest, SaveUnassociatedNodesForReassociation) { Should the test case be renamed to not use the term unassociated? Maybe AssociationReusesNodes or something. https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:731: session_tracker_.CleanupForeignSession(session_tag); This kind of feels like a trap for whoever tries to fix the TODO on line 655. Though I guess SyncedSessionTracker::CleanupForeignSession DCHECKs so it'll fail fast. https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:344: GetSession(local_session_tag_)->tab_node_ids.insert(*tab_node_id); This SyncedSession::tab_node_ids book keeping doesn't currently have any affect on anything, right? When we create new sessions objects we should try to fix this discrepancy.
Comments addressed, thanks! https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:736: session_tracker_.AddTabNode(session_tag, specifics.tab_node_id()); On 2016/12/06 19:37:21, skym wrote: > On 2016/12/06 01:32:55, Nicolas Zea wrote: > > On 2016/12/05 19:16:57, skym wrote: > > > I think AddX(...) is a much better method name than GetX(...) being used to > > > ensure we give data to the tracker when it needs it. But can you add a > comment > > > explaining why this call is needed? (Is it both so we can garbage collect > > > orphaned foreign tabs, and so we can re-use existing tab ids for local > > content? > > > I don't see the tab node id getting passed to the pool though.) > > > > > > I'm really having a hard time understanding the API that the tracker and > pool > > > classes have, when they need to get called, and what those methods need to > do. > > > > > > All this call is doing is making sure the tracker remembers this tab node > id. > > > But from the method name, that's super not obvious. > > > > > > ... > > > > > > Actually the more I understand here, the old approach was awkwardly and > > clunkily > > > used here, but tried to enforced consistency. If you wanted the > > > sessions::SessionTab to be auto created, then you had to pass a tab node id. > > Now > > > the tracker just trusts that the manager is going to AddTabNode as well > here. > > > > Cleaned up the comments here. PTAL! > > > > Also re: AddX vs GetX, the issue is that the SessionTracker doesn't understand > > specifics, and that's something I kind of want to preserve. It's useful to > think > > of it as only dealing with the bookkeeping, but not the contents. The manager > is > > doing a fair amount of work in the processing of the actual sync entities that > I > > wanted to keep separate, and I think the best way is to provide it direct > access > > to the SessionTabs themselves. > > > > And yeah, the old API was I think more clunky, so I pulled it into two methods > > (since many of the times you call GetTab you don't actually know the tab node > > associated with it). > > > > I hoped that AddTabNode would be clear that it's just adding bookkeeping for > the > > node (as opposed to AddTab, which sounds like it's adding the tab's contents. > > Maybe this should be OnTabNodeSeen, or AddTabNodeID? > > Both options sound good to me. Switched to OnTabNodeSeen. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:338: // Ensure a placeholder SessionTab is in place, if not already. On 2016/12/06 19:37:21, skym wrote: > On 2016/12/06 01:32:55, Nicolas Zea wrote: > > On 2016/12/03 01:20:46, skym wrote: > > > This sounds like GetTab(...) is going to create a SyncedTabDelegate object > > that > > > returns true when IsPlaceholderTab() is called. That's not what going on > > though, > > > right? > > > > I'm using placeholder like the colloquial term, unrelated to the > > SyncedTabDelegate (which the tracker knows nothing about). This is just to > have > > consistency by ensuring there's a SessionTab object for each tab node (which > > helps with testing). > > Ahh, okay, so you're saying that adding one here if it doesn't exist helps with > tests? It looks like the only time GetTabNodeForLocalTab is called in non-test > code is right before the SyncChange is built, which means there had better be a > real SyncTab here. And very not placeholder-y. > > I also worry that using the colloquial definition of placeholder might be > confusing. > > I'd prefer to see a comment that talks about this from a consistency > perspective. > > // Although we don't need a SessionTab to fulfill this request, this forces the > creation of one if it doesn't already exist. This helps to make sure we're > tracking this |tab_id| if |local_tab_pool_| is, and everyone's data structures > are kept in sync and as consistent as possible. Well, it's called right before we fill the SessionTab from the SyncedTabDelegate (in AssociateTab). As such, at the time we call it, it very well could be a placeholder (if it's the first time we're filling the tab). That said, agreed that a comment would be good here. Added. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:366: // It's possible a placeholder is already in place for the new tab. If On 2016/12/06 19:37:21, skym wrote: > On 2016/12/06 01:32:55, Nicolas Zea wrote: > > On 2016/12/05 19:16:57, skym wrote: > > > Placeholder? Like as in SyncedTabDelegate::IsPlaceholderTab()? It seems like > > you > > > could have a different tab node id sitting here previously using the > > > |new_tab_id|, and you're stealing their SessionTab. They can always create a > > new > > > one I suppose, but the one you stole now contains someone else's data, which > > is > > > wrong for us, right? > > > > Placeholder like a dummy SessionTab in place to show that the tab exists > (which > > get's created anytime GetTab is called). It's unrelated to the > SyncedTabDelegate > > logic. > > > > I'm not sure thinking about it as "someone else's data" is correct. It's still > > this device's data for that tab id. It's just that it's possibly stale. Also > > note that that old tab node is automatically freed as part of > > ReassociateTabNode, so things remain consistent. > > Similar to above, I'm concerned that using placeholder is confusing (it was to > me). I'd prefer this comment to explain when/why a SyncTab would already exist. > > I tried to write down my understanding of what's going on here, but I realized I > don't understand when this method should be called. It seems like > ReassociateLocalTab isn't being called for non-placeholder tabs. Why not? Why do > placeholders need this while non-place holders don't? Shouldn't they both get > set up with initial data from sync, and then at some point in the future (the > length of gap is the only difference) we get data from delegates and need to > worry about correcting tab ids? Chatted offline. I've added a comment to SessionsSyncManager to explain when and where android placeholders are used, and when we reassociate them using their sync id. https://codereview.chromium.org/2499083004/diff/60001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:381: if (old_tab_id != TabNodePool::kInvalidTabID && On 2016/12/06 19:37:21, skym wrote: > On 2016/12/06 01:32:55, Nicolas Zea wrote: > > On 2016/12/05 19:16:57, skym wrote: > > > At this point, I'm wondering, would it have been easier to just erase > > everything > > > and then re-add everything? > > > > I'm not sure I follow. Erase all tabs for this session, just to reassociate > one? > > I meant remove everything at tab_node_id, new_tab_id, and old_tab_id. Then > create a new SessionTab every time. But looking at this again today, I'm liking > your approach more. So never mind! Acknowledged. https://codereview.chromium.org/2499083004/diff/80001/chrome/browser/sync/ses... File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2499083004/diff/80001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:519: : current_entry_index_(0), is_supervised_(false), sync_id_(-1) {} On 2016/12/06 19:37:21, skym wrote: > It's a bit odd that half of these members fields have their initial values set > here, and half are set where the variables are defined. Done. https://codereview.chromium.org/2499083004/diff/80001/chrome/browser/sync/ses... chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1808: TEST_F(SessionsSyncManagerTest, SaveUnassociatedNodesForReassociation) { On 2016/12/06 19:37:21, skym wrote: > Should the test case be renamed to not use the term unassociated? Maybe > AssociationReusesNodes or something. Done. https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:731: session_tracker_.CleanupForeignSession(session_tag); On 2016/12/06 19:37:21, skym wrote: > This kind of feels like a trap for whoever tries to fix the TODO on line 655. > Though I guess SyncedSessionTracker::CleanupForeignSession DCHECKs so it'll fail > fast. Yeah, this is changing in the followup patch. https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... File components/sync_sessions/synced_session_tracker.cc (right): https://codereview.chromium.org/2499083004/diff/80001/components/sync_session... components/sync_sessions/synced_session_tracker.cc:344: GetSession(local_session_tag_)->tab_node_ids.insert(*tab_node_id); On 2016/12/06 19:37:21, skym wrote: > This SyncedSession::tab_node_ids book keeping doesn't currently have any affect > on anything, right? When we create new sessions objects we should try to fix > this discrepancy. It does get consumed actually, but only when deleting a foreign session (via LookupTabNodeIds).
The CQ bit was checked by zea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2499083004/#ps100001 (title: "Address comments")
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": 100001, "attempt_start_ts": 1481063503722520,
"parent_rev": "83d135bb40a124df507a81546be37673d6f9ef2f", "commit_rev":
"1d6e7a98b49eeef3b2ce821f28c3b910fe33739a"}
Message was sent while issue was closed.
Description was changed from ========== Reland of [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 user visible 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. To make this work, the TabNodePool has been moved into the SyncedSessionTracker, and in general data ownership has been simplified (with quite a few new tests added). 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 ========== Reland of [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 user visible 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. To make this work, the TabNodePool has been moved into the SyncedSessionTracker, and in general data ownership has been simplified (with quite a few new tests added). 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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Reland of [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 user visible 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. To make this work, the TabNodePool has been moved into the SyncedSessionTracker, and in general data ownership has been simplified (with quite a few new tests added). 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 ========== Reland of [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 user visible 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. To make this work, the TabNodePool has been moved into the SyncedSessionTracker, and in general data ownership has been simplified (with quite a few new tests added). 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/159246f269b561bf931fa56a35b2704fab7dcc96 Cr-Commit-Position: refs/heads/master@{#436786} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96 Cr-Commit-Position: refs/heads/master@{#436786}
Message was sent while issue was closed.
On 2016/12/07 00:43:20, commit-bot: I haz the power wrote: > Patchset 6 (id:??) landed as > https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96 > Cr-Commit-Position: refs/heads/master@{#436786} Hello, This is Chan Li from Findit team and I'm looking for possible cause for flaky test RecentTabsSubMenuModelTest.MaxWidth. Could you help me and check if this change could cause this flakiness? Thank you so much!
Message was sent while issue was closed.
On 2016/12/10 20:26:37, chanli wrote: > On 2016/12/07 00:43:20, commit-bot: I haz the power wrote: > > Patchset 6 (id:??) landed as > > https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96 > > Cr-Commit-Position: refs/heads/master@{#436786} > > Hello, > > This is Chan Li from Findit team and I'm looking for possible cause for flaky > test RecentTabsSubMenuModelTest.MaxWidth. Could you help me and check if this > change could cause this flakiness? Thank you so much! Yes, this did cause RecentTabsSubMenuModelTest.MaxWidth to flake, and it has been fixed in https://codereview.chromium.org/2562853002/ already. |
