Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
79973002:
sync: Route local sessions events to SessionsSyncManager (Closed)
|
Created:
7 years, 1 month ago by tim (not reviewing) Modified:
7 years ago CC:
chromium-reviews, tim+watch_chromium.org, rsimha+watch_chromium.org, haitaol+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionsync: Route local sessions events to SessionsSyncManager
Builds on https://codereview.chromium.org/74653002/
What's left after this:
- Full reassociation on ProcessSyncChanges errors.
- Integration Tests
- Stale Session pruning.
Since GetActiveEntry is now deprecated, switched to use GetVisibleEntry instead per bug 273710.
BUG=98892
R=rlarocque@chromium.org, zea@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238317
Patch Set 1 : #
Total comments: 27
Patch Set 2 : zea's review #Patch Set 3 : remove NOTIMPLEMENTED #Patch Set 4 : review + tweaks #
Total comments: 15
Patch Set 5 : Richard's review #
Total comments: 6
Patch Set 6 : zea's review #Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/notification_service_sessions_router.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:21: nit: remove extra newline https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/notification_service_sessions_router.h (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/notification_service_sessions_router.h:41: const syncer::SyncableService::StartSyncFlare& flare); nit: looks like this can fit on one line? also, newline after https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/notification_service_sessions_router.h:43: void OnNavigationBlocked(content::WebContents* web_contents); nit: newline after. Also, comment what this function is for? https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:328: if (error.IsSet()) { not necessarily for this patch, but I suspect triggering the re-association shouldn't be based on the SyncChangeProcessor returning an error (currently, if it does that, it'll also disable the datatype). https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:340: sync_processor_->ProcessSyncChanges(FROM_HERE, changes); nit: fits on one line? https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:341: if (error.IsSet()) { nit: remove curly brace https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.h:61: virtual void OnBrowserOpened() = 0; Is this necessary? We always reassociate windows anyways; providing this kind of implies that OnLocalTabModified isn't itself looking at windows. Perhaps just have OnLocalTabModified support a NULL tab delegate (and comment that that only looks at the windows)? https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.h:81: virtual ~SyncInternalApiDelegate() {} nit: newline after https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:122: : public BrowserWithTestWindowTest, now that you're abstracting away all the real interactions, do you still need this (which is what allows for the NavigateAndCommitActiveTab)? It'd be nice to instead trigger calls directly via the local event router. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:165: test_processor_), nit: fits on previous line? https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:177: FROM_HERE, syncer::SyncError::PERSISTENCE_ERROR, "Error", nit: prefer something other than persistence error, which doesn't really apply to sessions (e.g. maybe DATATYPE_ERROR.
https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/notification_service_sessions_router.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:21: On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: remove extra newline Done. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/notification_service_sessions_router.h (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/notification_service_sessions_router.h:41: const syncer::SyncableService::StartSyncFlare& flare); On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: looks like this can fit on one line? also, newline after Done. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/notification_service_sessions_router.h:43: void OnNavigationBlocked(content::WebContents* web_contents); On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: newline after. Also, comment what this function is for? Done. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:328: if (error.IsSet()) { On 2013/11/21 02:34:52, Nicolas Zea wrote: > not necessarily for this patch, but I suspect triggering the re-association > shouldn't be based on the SyncChangeProcessor returning an error (currently, if > it does that, it'll also disable the datatype). Hm. Right. Well, in the old code, we'd re-associate if any of the Read/WriteNode operations failed to look things up. I'm not sure what we want to do here in this case, if anything, then. For the delete case in 440, we need to reset TabNodePool, but at the same time I don't want to thrash with another client as the comment suggests.. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:340: sync_processor_->ProcessSyncChanges(FROM_HERE, changes); On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: fits on one line? nope https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:341: if (error.IsSet()) { On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: remove curly brace I'm with you, although as a rule of thumb we try to keep curly braces around macros to quarantine the damage they might do. In this case though and the one above, maybe I should just remove the NOTIMPLEMENTED()? I'm not sure what I'd do in this case. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.h:61: virtual void OnBrowserOpened() = 0; On 2013/11/21 02:34:52, Nicolas Zea wrote: > Is this necessary? We always reassociate windows anyways; providing this kind of > implies that OnLocalTabModified isn't itself looking at windows. > > Perhaps just have OnLocalTabModified support a NULL tab delegate (and comment > that that only looks at the windows)? I could maybe change it to OnBrowserModified(SyncedTabDelegate* modified_tab). On the other hand, I would argue that this interface is meant to only define "what sorts of events are interesting to tab sync" versus what it does in response to different events. There are some similarities between the two implementations but OLTM does additional stuff and it's kind of nice to not have an extra state to handle. I also think the current name is nice because it makes it clear we only handle one tab at a time and the browser opened event is separate (null |modified_tab| doesn't imply a "multiple tab" browser opened event). A comment something like "A Browser was opened [x]or a single tab was modified" might be what we'd want. If we forsee more events in the future we want to react to where the implementation will always be identical and we won't be tempted to add yet-another state (like handling the NULL parameter), I could be swayed. If we reduced the 3 to one single callback I might agree more, but we're likely going to have separate functions with different arguments anyway (favicons). https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.h:81: virtual ~SyncInternalApiDelegate() {} On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: newline after Done. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:122: : public BrowserWithTestWindowTest, On 2013/11/21 02:34:52, Nicolas Zea wrote: > now that you're abstracting away all the real interactions, do you still need > this (which is what allows for the NavigateAndCommitActiveTab)? It'd be nice to > instead trigger calls directly via the local event router. I was thinking the same when creating the LocalEventRouter and I think you're absolutely correct. I think we should do that. I decided against it as part of this patch because - There's non-trivial fake code to write to splice in at and use GetSyncedWindowDelegates - It affects all tests in here (including the ones that only do MergeDataAndStartSyncing), slightly more than I was going for with this patch alone - I didn't see any other examples of code creating test implementations of NavigationEntry, which we'd need - There is a dependency on SyncedTabDelegate::GetWebContents (for NetworkTimeHelper) and I didn't want to get into the business of trying to return a fake WebContents because I think that's what BrowserWithTestWindowTest is for. However, I could try to break that dependency (sent a thread offline about that) and use SyncedTabDelegateFake (and a SyncedWindowDelegate thing I create) in place of BrowserWithTestWindowTest. It would be pretty awesome, more targeted, faster, and likely produce less of that crappy verbose output (and be runnable without xvfb-run over ssh :p). I also think it'd be easier to do once SessionModelAssociator is gone and there's one less place to reconcile, but I get how that may sound like an excuse :) My next set of patches will be test-focused anyway (integration tests). Rather than re-write this whole suite as part of this parity-goal CL, why don't I leave it as is to get coverage in the meantime and see if I can dedicate a future patch to revamping it? https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:165: test_processor_), On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: fits on previous line? Done. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:177: FROM_HERE, syncer::SyncError::PERSISTENCE_ERROR, "Error", On 2013/11/21 02:34:52, Nicolas Zea wrote: > nit: prefer something other than persistence error, which doesn't really apply > to sessions (e.g. maybe DATATYPE_ERROR. Ah. I actually wasn't clear on the difference from the comments in sync_error.h. Changed.
https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:328: if (error.IsSet()) { On 2013/11/21 21:38:59, timsteele wrote: > On 2013/11/21 02:34:52, Nicolas Zea wrote: > > not necessarily for this patch, but I suspect triggering the re-association > > shouldn't be based on the SyncChangeProcessor returning an error (currently, > if > > it does that, it'll also disable the datatype). > > Hm. Right. Well, in the old code, we'd re-associate if any of the Read/WriteNode > operations failed to look things up. > > I'm not sure what we want to do here in this case, if anything, then. > > For the delete case in 440, we need to reset TabNodePool, but at the same time I > don't want to thrash with another client as the comment suggests.. Yeah, the old version was kind of violating some layering in order to get that information. The right approach is probably to, on delete, set a flag so that on the next local change we do a GetAllSyncData + Reassociate. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.cc:341: if (error.IsSet()) { On 2013/11/21 21:38:59, timsteele wrote: > On 2013/11/21 02:34:52, Nicolas Zea wrote: > > nit: remove curly brace > > I'm with you, although as a rule of thumb we try to keep curly braces around > macros to quarantine the damage they might do. > > In this case though and the one above, maybe I should just remove the > NOTIMPLEMENTED()? I'm not sure what I'd do in this case. I think we can probably ignore the error result. Sync's going to disable us anyways. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.h:61: virtual void OnBrowserOpened() = 0; On 2013/11/21 21:38:59, timsteele wrote: > On 2013/11/21 02:34:52, Nicolas Zea wrote: > > Is this necessary? We always reassociate windows anyways; providing this kind > of > > implies that OnLocalTabModified isn't itself looking at windows. > > > > Perhaps just have OnLocalTabModified support a NULL tab delegate (and comment > > that that only looks at the windows)? > > I could maybe change it to OnBrowserModified(SyncedTabDelegate* modified_tab). > On the other hand, I would argue that this interface is meant to only define > "what sorts of events are interesting to tab sync" versus what it does in > response to different events. > > There are some similarities between the two implementations but OLTM does > additional stuff and it's kind of nice to not have an extra state to handle. I > also think the current name is nice because it makes it clear we only handle one > tab at a time and the browser opened event is separate (null |modified_tab| > doesn't imply a "multiple tab" browser opened event). A comment something like > "A Browser was opened [x]or a single tab was modified" might be what we'd want. > > If we forsee more events in the future we want to react to where the > implementation will always be identical and we won't be tempted to add > yet-another state (like handling the NULL parameter), I could be swayed. If we > reduced the 3 to one single callback I might agree more, but we're likely going > to have separate functions with different arguments anyway (favicons). > > How about two methods, with the second method being named OnBrowserModified, and a comment that only one of these methods should be called for a single event. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:122: : public BrowserWithTestWindowTest, On 2013/11/21 21:38:59, timsteele wrote: > On 2013/11/21 02:34:52, Nicolas Zea wrote: > > now that you're abstracting away all the real interactions, do you still need > > this (which is what allows for the NavigateAndCommitActiveTab)? It'd be nice > to > > instead trigger calls directly via the local event router. > > I was thinking the same when creating the LocalEventRouter and I think you're > absolutely correct. I think we should do that. I decided against it as part of > this patch because > > - There's non-trivial fake code to write to splice in at and use > GetSyncedWindowDelegates > - It affects all tests in here (including the ones that only do > MergeDataAndStartSyncing), slightly more than I was going for with this patch > alone > - I didn't see any other examples of code creating test implementations of > NavigationEntry, which we'd need > - There is a dependency on SyncedTabDelegate::GetWebContents (for > NetworkTimeHelper) and I didn't want to get into the business of trying to > return a fake WebContents because I think that's what BrowserWithTestWindowTest > is for. > > However, I could try to break that dependency (sent a thread offline about that) > and use SyncedTabDelegateFake (and a SyncedWindowDelegate thing I create) in > place of BrowserWithTestWindowTest. It would be pretty awesome, more targeted, > faster, and likely produce less of that crappy verbose output (and be runnable > without xvfb-run over ssh :p). > > I also think it'd be easier to do once SessionModelAssociator is gone and > there's one less place to reconcile, but I get how that may sound like an excuse > :) My next set of patches will be test-focused anyway (integration tests). > Rather than re-write this whole suite as part of this parity-goal CL, why don't > I leave it as is to get coverage in the meantime and see if I can dedicate a > future patch to revamping it? sgtm
SessionsSyncManagerTest.NotifiedOfRefresh only applies to mobile platforms now due to search / instant NTP revamps, so I changed my patch to only enable it on those platforms. Also switched to use GetVisibleEntry in the SyncedTabDelegate as GetActiveEntry is now deprecated, per bug 273710. https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/80001/chrome/browser/sync/sessi... chrome/browser/sync/sessions2/sessions_sync_manager.h:61: virtual void OnBrowserOpened() = 0; On 2013/11/22 23:34:51, Nicolas Zea wrote: > On 2013/11/21 21:38:59, timsteele wrote: > > On 2013/11/21 02:34:52, Nicolas Zea wrote: > > > Is this necessary? We always reassociate windows anyways; providing this > kind > > of > > > implies that OnLocalTabModified isn't itself looking at windows. > > > > > > Perhaps just have OnLocalTabModified support a NULL tab delegate (and > comment > > > that that only looks at the windows)? > > > > I could maybe change it to OnBrowserModified(SyncedTabDelegate* modified_tab). > > On the other hand, I would argue that this interface is meant to only define > > "what sorts of events are interesting to tab sync" versus what it does in > > response to different events. > > > > There are some similarities between the two implementations but OLTM does > > additional stuff and it's kind of nice to not have an extra state to handle. I > > also think the current name is nice because it makes it clear we only handle > one > > tab at a time and the browser opened event is separate (null |modified_tab| > > doesn't imply a "multiple tab" browser opened event). A comment something like > > "A Browser was opened [x]or a single tab was modified" might be what we'd > want. > > > > If we forsee more events in the future we want to react to where the > > implementation will always be identical and we won't be tempted to add > > yet-another state (like handling the NULL parameter), I could be swayed. If we > > reduced the 3 to one single callback I might agree more, but we're likely > going > > to have separate functions with different arguments anyway (favicons). > > > > > > How about two methods, with the second method being named OnBrowserModified, and > a comment that only one of these methods should be called for a single event. As discussed, went ahead and removed OnBrowserOpened.
+Richard in case he has time to give this a look + approval while Nicolas is out. If not no worries, I can wait until next week.
Added some comments. I think it's still a good idea to wait until Nicolas gets back to finish the review, though. I may not have understood it all. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service.cc:201: scoped_ptr<SessionsSyncManager::LocalEventRouter> router( It looks like there's a circular dependency here. The NotificationServiceSessionsRouter needs a SessionsSyncManager, and the SessionsSyncManager needs a LocalEventRouter. You've resolved it by writing the NotificationServiceSessionsRouter so that it doesn't require a LocalEventHandler (SessionsSyncManager) right away, and is prepared to operate without one. If this is the only reason why you've made NotificationServiceSessionsRouter support a mode of operation where its handler_ == NULL, I suggest you reconsider. There are other solutions. I would prefer to not see the design of the NotificationServiceSessionsRouter class compromised for this reason alone. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/notification_service_sessions_router.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:36: set_flare(sync_start_util::GetFlareForSyncableService( Could this be injected as a constructor parameter? That might make it easier to test. It might also let you remove the set_flare method. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:49: registrar_.Add(this, The session_change_processor.cc variant of this function also listens to NOTIFICATION_BROWSER_OPENED. Why is this code different? https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager.cc:312: content::NotificationService::current()->Notify( I've always thought that this would be better implemented as a WebUI Javascript callback. That way we could: - Clean up the sessions code. - Not require that sessions sync be enabled for this to work. - Allow more complicated fetch logic (eg. Refresh every time the user activates an "other tabs" UI element, rather than on every NTP open). No need to change it now, though. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager.h:89: class LocalEventRouter { Unless there's some C++ trivia regarding inner classes that I don't know about, I think this is in many ways worse than a top level class definition and in no way better. It makes forward declaration impossible. It makes it impossible to define a class that uses this interface without including sync_sessions_manager.h. Despite your work to reduce dependencies, profile_sync_service.cc will still end up including this file through notification_service_sessions_router.h. It's more verbose than even a top-level "class SessionsSyncManager_LocalEventRouter" would have been (:: vs _). That's just a silly example to emphasize the difference between the two options; I'm sure you could come up with a better and shorter name than that. In summary, I oppose all public inner classes in C++, including this one. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:1023: TEST_F(SessionsSyncManagerTest, OnLocalTabModified) { nit: Add a test comment.
https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service.cc:201: scoped_ptr<SessionsSyncManager::LocalEventRouter> router( On 2013/11/27 22:47:51, rlarocque wrote: > It looks like there's a circular dependency here. The > NotificationServiceSessionsRouter needs a SessionsSyncManager, and the > SessionsSyncManager needs a LocalEventRouter. You've resolved it by writing the > NotificationServiceSessionsRouter so that it doesn't require a LocalEventHandler > (SessionsSyncManager) right away, and is prepared to operate without one. > > If this is the only reason why you've made NotificationServiceSessionsRouter > support a mode of operation where its handler_ == NULL, I suggest you > reconsider. There are other solutions. I would prefer to not see the design of > the NotificationServiceSessionsRouter class compromised for this reason alone. That's not quite what's happening here. The router always a) exists (with ProfileSyncService) and b) observes NotificationService. It won't route things to SessionsSyncManager until SessionsSyncManager is told to MergeDataAndStartSyncing. At that point SSM tells the router to start routing to it. In the interim, if there is no handler, the router may decide to instead send up a sync flare to start syncing becuase local sessions activity is happening. Once we create a TabStripModel based LocalEventRouter, things will keep working the same way from SSM's perspective and it shouldn't need to change. What will change, however, is what the different router implementation relies on to trigger a flare. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/notification_service_sessions_router.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:36: set_flare(sync_start_util::GetFlareForSyncableService( On 2013/11/27 22:47:51, rlarocque wrote: > Could this be injected as a constructor parameter? That might make it easier to > test. It might also let you remove the set_flare method. Good suggestion. Done. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:49: registrar_.Add(this, On 2013/11/27 22:47:51, rlarocque wrote: > The session_change_processor.cc variant of this function also listens to > NOTIFICATION_BROWSER_OPENED. Why is this code different? Ah. I mentioned I removed this in my last response on the CR but didn't point out why :) When a browser is opened and that "window" is interesting to tab sync, we always get a TAB_PARENTED notification. There may have been cases in the past where BROWSER_OPENED was uniquely interesting, but neither zea@ or I see it any longer. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager.cc:312: content::NotificationService::current()->Notify( On 2013/11/27 22:47:51, rlarocque wrote: > I've always thought that this would be better implemented as a WebUI Javascript > callback. That way we could: > - Clean up the sessions code. > - Not require that sessions sync be enabled for this to work. > - Allow more complicated fetch logic (eg. Refresh every time the user activates > an "other tabs" UI element, rather than on every NTP open). > > No need to change it now, though. That does seem nice. I actually wrestled around with this code a fair bit because as it turns out, it's no longer reachable on desktop (hence the decorations around my test in the _unittest.cc file). It is reachable on Android though. To your point about not requiring sessions to be enabled, can you explain? It occurs to me that we could do this filtering entirely within the LocalEventRouter now, which exists unless sessions sync is explicitly disabled, and probably makes a lot more sense; That could also be an action that explicitly sends a flare to sync to start. For now I'd like to maintain as much parity as I can though. But interested in changing this down the road. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager.h:89: class LocalEventRouter { On 2013/11/27 22:47:51, rlarocque wrote: > Unless there's some C++ trivia regarding inner classes that I don't know about, > I think this is in many ways worse than a top level class definition and in no > way better. > > It makes forward declaration impossible. > > It makes it impossible to define a class that uses this interface without > including sync_sessions_manager.h. Despite your work to reduce dependencies, > profile_sync_service.cc will still end up including this file through > notification_service_sessions_router.h. > > It's more verbose than even a top-level "class > SessionsSyncManager_LocalEventRouter" would have been (:: vs _). That's just a > silly example to emphasize the difference between the two options; I'm sure you > could come up with a better and shorter name than that. > > In summary, I oppose all public inner classes in C++, including this one. OK. Made it a top level interface so that it's forward declarable and less verbose. My goal here actually wasn't about reducing dependencies fwiw, ProfileSyncService still needs to include SessionsSyncManager by design. The Handler+Router are basically how SessionsSyncManager declares what it needs to work properly. They aren't intended to be used separately and changes to one likely means changes to the other, so I'd prefer to keep them close. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:1023: TEST_F(SessionsSyncManagerTest, OnLocalTabModified) { On 2013/11/27 22:47:51, rlarocque wrote: > nit: Add a test comment. Done.
Have you uploaded a new patch? The most recent one I see is #4, which the site claims was uploaded one week ago. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service.cc:201: scoped_ptr<SessionsSyncManager::LocalEventRouter> router( On 2013/12/02 18:29:49, timsteele wrote: > On 2013/11/27 22:47:51, rlarocque wrote: > > It looks like there's a circular dependency here. The > > NotificationServiceSessionsRouter needs a SessionsSyncManager, and the > > SessionsSyncManager needs a LocalEventRouter. You've resolved it by writing > the > > NotificationServiceSessionsRouter so that it doesn't require a > LocalEventHandler > > (SessionsSyncManager) right away, and is prepared to operate without one. > > > > If this is the only reason why you've made NotificationServiceSessionsRouter > > support a mode of operation where its handler_ == NULL, I suggest you > > reconsider. There are other solutions. I would prefer to not see the design > of > > the NotificationServiceSessionsRouter class compromised for this reason alone. > > That's not quite what's happening here. The router always a) exists (with > ProfileSyncService) and b) observes NotificationService. It won't route things > to SessionsSyncManager until SessionsSyncManager is told to > MergeDataAndStartSyncing. At that point SSM tells the router to start routing to > it. In the interim, if there is no handler, the router may decide to instead > send up a sync flare to start syncing becuase local sessions activity is > happening. > > Once we create a TabStripModel based LocalEventRouter, things will keep working > the same way from SSM's perspective and it shouldn't need to change. What will > change, however, is what the different router implementation relies on to > trigger a flare. OK, I think I understand now. Would it be a viable alternative to have the SessionsSyncManager always be registered, but keep a boolean 'is_watching_changes_' to ignore events that occur prior to the MergeDataAndStartSyncing() class? (That's not necessarily a recommendation. I'm just trying to make sure that I understand how it works.) https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager.cc:312: content::NotificationService::current()->Notify( On 2013/12/02 18:29:49, timsteele wrote: > On 2013/11/27 22:47:51, rlarocque wrote: > > I've always thought that this would be better implemented as a WebUI > Javascript > > callback. That way we could: > > - Clean up the sessions code. > > - Not require that sessions sync be enabled for this to work. > > - Allow more complicated fetch logic (eg. Refresh every time the user > activates > > an "other tabs" UI element, rather than on every NTP open). > > > > No need to change it now, though. > > That does seem nice. I actually wrestled around with this code a fair bit > because as it turns out, it's no longer reachable on desktop (hence the > decorations around my test in the _unittest.cc file). It is reachable on > Android though. > > To your point about not requiring sessions to be enabled, can you explain? It > occurs to me that we could do this filtering entirely within the > LocalEventRouter now, which exists unless sessions sync is explicitly disabled, > and probably makes a lot more sense; That could also be an action that > explicitly sends a flare to sync to start. > > For now I'd like to maintain as much parity as I can though. But interested in > changing this down the road. By "not requiring sessions to be enabled", I mean that the JavaScript callback would work regardless of whether or not sessions sync has been explicitly disabled by the user. I'm not sure that we care about that, since (I think) this was implemented to help update the "other devices" UI, so we'd probably want to disable it if sessions sync were disabled anyway. Still, though, a solution that works even when sessions sync is disabled is more flexible and could be nice to have in the future.
Oops, uploaded to the wrong review! Fixed now. https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/79973002/diff/480001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service.cc:201: scoped_ptr<SessionsSyncManager::LocalEventRouter> router( On 2013/12/02 18:53:53, rlarocque wrote: > On 2013/12/02 18:29:49, timsteele wrote: > > On 2013/11/27 22:47:51, rlarocque wrote: > > > It looks like there's a circular dependency here. The > > > NotificationServiceSessionsRouter needs a SessionsSyncManager, and the > > > SessionsSyncManager needs a LocalEventRouter. You've resolved it by writing > > the > > > NotificationServiceSessionsRouter so that it doesn't require a > > LocalEventHandler > > > (SessionsSyncManager) right away, and is prepared to operate without one. > > > > > > If this is the only reason why you've made NotificationServiceSessionsRouter > > > support a mode of operation where its handler_ == NULL, I suggest you > > > reconsider. There are other solutions. I would prefer to not see the > design > > of > > > the NotificationServiceSessionsRouter class compromised for this reason > alone. > > > > That's not quite what's happening here. The router always a) exists (with > > ProfileSyncService) and b) observes NotificationService. It won't route things > > to SessionsSyncManager until SessionsSyncManager is told to > > MergeDataAndStartSyncing. At that point SSM tells the router to start routing > to > > it. In the interim, if there is no handler, the router may decide to instead > > send up a sync flare to start syncing becuase local sessions activity is > > happening. > > > > Once we create a TabStripModel based LocalEventRouter, things will keep > working > > the same way from SSM's perspective and it shouldn't need to change. What will > > change, however, is what the different router implementation relies on to > > trigger a flare. > > OK, I think I understand now. > > Would it be a viable alternative to have the SessionsSyncManager always be > registered, but keep a boolean 'is_watching_changes_' to ignore events that > occur prior to the MergeDataAndStartSyncing() class? > > (That's not necessarily a recommendation. I'm just trying to make sure that I > understand how it works.) Yes, except that the goal with the Router approach was to de-couple SessionsSyncManager from the observer mechanism and make it easy to swap in a different module for that. The current one is based on NotificationService, but as bug 98892 outlines we'd like to try moving to TabStripModelObserver in the future. With this patch, SessionsSyncManager won't need to change because the LocalSessionsEventHandler contract remains the same. It also allows for more targeted tests in the future, since we just have to satisfy LocalSessionEventHandler vs the full navigation stack.
LGTM with some nits https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/notification_service_sessions_router.cc (right): https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:125: << type; nit: fix indent (or see if it fits on one line) https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager.h:87: virtual ~SyncInternalApiDelegate() {} nit: newline after https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:65: } nit: newline after
LGTM.
Thanks! https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/notification_service_sessions_router.cc (right): https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/notification_service_sessions_router.cc:125: << type; On 2013/12/02 21:41:33, Nicolas Zea wrote: > nit: fix indent (or see if it fits on one line) Done. https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager.h (right): https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager.h:87: virtual ~SyncInternalApiDelegate() {} On 2013/12/02 21:41:33, Nicolas Zea wrote: > nit: newline after Done. https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... File chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/79973002/diff/490001/chrome/browser/sync/sess... chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc:65: } On 2013/12/02 21:41:33, Nicolas Zea wrote: > nit: newline after Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tim@chromium.org/79973002/500001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Message was sent while issue was closed.
Committed patchset #6 manually as r238317 (presubmit successful). |
