[sync] WebContentsObserver based sessions notifications

[sync] WebContentsObserver based sessions notifications

Aggregate individual WebContentsObservers to notify sync of sessions changes

This CL introduces the SyncSessionsWebContentsRouter and the
SyncSessionsRouterTabHelper, which replace the old
NotificationServiceSessionsRouter.  The SyncSessionsWebContentsRouter
aggregates events from the individual TabHelpers, which are scoped to
individual tabs, and pushes them to SessionsSyncManager.
SyncSessionsWebContentsRouters are given a source tab id property which
is set when a tab is created directly from another tab, e.g. by ctrl
clicking.
Some changes are made to SessionsHelper so that it can continue to
correctly wait for tabs to load without using Notifications.

R=zea@chromium.org
BUG=695241
Review-Url: https://codereview.chromium.org/2753753005
Cr-Commit-Position: refs/heads/master@{#459225}
Committed: https://chromium.googlesource.com/chromium/src/+/1901afa5e03bad9ce8a72691f746a685038241b0

[Patrick Noland, 2017-03-15 17:45:52]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 17:46:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 19:18:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 19:18:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patrick Noland, 2017-03-15 21:19:54]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 21:20:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 23:28:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 23:28:54]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-03-16 19:12:56]

Patchset #1 (id:1) has been deleted

[Patrick Noland, 2017-03-16 19:24:53]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 19:25:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nicolas Zea, 2017-03-16 19:57:44]

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc:68:
DLOG(WARNING) << source_tab_id;
forget to remove?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc:71: new_contents
!= web_contents() && source_tab_id != kUnknownTabID) {
looks like kUnknownTabID already exists? Perhaps refer use that instead of -1?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc:71: new_contents
!= web_contents() && source_tab_id != kUnknownTabID) {
Does new_contents != web_contents() prevent source tab id from being the same as
the current tab id?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:17: // global
sessions router. The global router is responsible for forwarding these
it's not really global though right? It's per profile

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:23: 
nit: remove newline?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:35: //
corresponds to. Returns -1 if there is no such tab.
maybe define -1 as an enum? E.g. SyncSessionRouterTabHelper::InvalidTabId?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:40: // router_ is
a KeyedService and is guaranteed to outlive |this|.
newline above

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:41:
SyncSessionsWebContentsRouter* router_;
Member variables like this should be defined below the methods. See
https://google.github.io/styleguide/cppguide.html#Class_Format

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:46: void
SetSourceTabID(const SessionID::id_type id);
if you're using source_tab_id() above, for consistency it might be better to use
set_source_tab_id here.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:48: //
WebContentsObserver implementation.
Any reason these are private?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:66: explicit
SyncSessionsRouterTabHelper(content::WebContents* web_contents,
Constructors should be defined before other methods.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc:19: handler_ =
nullptr;
can remove if you initialize in the header

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc:29: // Android
has no BrowserList or TabStripModel, so we exclude code that refers
nit: newline above

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:13: #if
!defined(OS_ANDROID)
is this pattern of using platform conditional code used elsewhere? If not, I
think it might be better to just have separate files with the platform specific
code as much as possible, otherwise this might become brittle. Thoughts?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:32: //
modified. It does this by aggregating the events pushed to it by individual
it's not actually doing any aggregation right, just plumbing the events through?
Perhaps replace "aggregating" with "routing"?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:76:
LocalSessionEventHandler* handler_;
nit: may as well initialize this to nullptr here.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc
(right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc:12:
namespace sync_sessions {
nit: newline below

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h
(right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h:18: }
nit: // namespace base

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h:20:
namespace sync_sessions {
nit: newline below

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sessions_helper.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.h:121: // Wait for a
session change to propagate to the model associator. Will return
nit: s/model associator/syncableservice, here and elsewhere. There is no model
associator anymore.

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
File components/sync_sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager_unittest.cc:214:
SessionID::id_type GetSourceTabID() const override { return -1; }
Would it be possible to use kUnknownTabId here 
(and below)?

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
File components/sync_sessions/synced_tab_delegate.h (right):

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
components/sync_sessions/synced_tab_delegate.h:37: // Returns -1 if no such tab
exists.
returns kUnknownTabID

[Patrick Noland, 2017-03-16 20:23:27]

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc:71: new_contents
!= web_contents() && source_tab_id != kUnknownTabID) {
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> Does new_contents != web_contents() prevent source tab id from being the same
as
> the current tab id?

Yes.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc:71: new_contents
!= web_contents() && source_tab_id != kUnknownTabID) {
On 2017/03/16 19:57:42, Nicolas Zea wrote:
> looks like kUnknownTabID already exists? Perhaps refer use that instead of -1?

It's defined in a few places(TabNodePool as an enum, the extensions API). I'd
prefer not to rely on the extensions API just because that's where the constant
is, but maybe I could move the TabNodePool enum to SyncedTabDelegate or
SessionTabHelper?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:17: // global
sessions router. The global router is responsible for forwarding these
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> it's not really global though right? It's per profile

Right.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:48: //
WebContentsObserver implementation.
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> Any reason these are private?

Just caution about future use. There's not currently a reason they should be
called by anyone except existing call sites for WebContentsObserver, so making
them private insures against that.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:13: #if
!defined(OS_ANDROID)
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> is this pattern of using platform conditional code used elsewhere? If not, I
> think it might be better to just have separate files with the platform
specific
> code as much as possible, otherwise this might become brittle. Thoughts?

I've seen it done both ways(ifdefs and separate files). My hesitation with using
two separate files is code duplication; any change to the "main" file will have
to be repeated in the android file. This might be avoidable with inheritance
though?

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:32: //
modified. It does this by aggregating the events pushed to it by individual
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> it's not actually doing any aggregation right, just plumbing the events
through?
> Perhaps replace "aggregating" with "routing"?

Right, it's really just routing events from multiple sources, not aggregating.

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
File components/sync_sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager_unittest.cc:214:
SessionID::id_type GetSourceTabID() const override { return -1; }
On 2017/03/16 19:57:44, Nicolas Zea wrote:
> Would it be possible to use kUnknownTabId here 
> (and below)?

Done.

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
File components/sync_sessions/synced_tab_delegate.h (right):

https://codereview.chromium.org/2753753005/diff/20001/components/sync_session...
components/sync_sessions/synced_tab_delegate.h:37: // Returns -1 if no such tab
exists.
On 2017/03/16 19:57:44, Nicolas Zea wrote:
> returns kUnknownTabID

Done.

[commit-bot: I haz the power, 2017-03-16 20:33:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 20:33:30]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-03-16 23:05:33]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 23:06:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 23:43:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 23:43:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Patrick Noland, 2017-03-16 23:49:41]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[Patrick Noland, 2017-03-16 23:49:44]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-16 23:50:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-03-17 00:20:11]

Nicolas, PTAL

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc:68:
DLOG(WARNING) << source_tab_id;
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> forget to remove?

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:17: // global
sessions router. The global router is responsible for forwarding these
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> it's not really global though right? It's per profile

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:23: 
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> nit: remove newline?

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:35: //
corresponds to. Returns -1 if there is no such tab.
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> maybe define -1 as an enum? E.g. SyncSessionRouterTabHelper::InvalidTabId?

Done, but in SyncedTabDelegate. This new definition replaces the definition in
TabNodePool.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:40: // router_ is
a KeyedService and is guaranteed to outlive |this|.
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> newline above

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:41:
SyncSessionsWebContentsRouter* router_;
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> Member variables like this should be defined below the methods. See
> https://google.github.io/styleguide/cppguide.html#Class_Format

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:46: void
SetSourceTabID(const SessionID::id_type id);
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> if you're using source_tab_id() above, for consistency it might be better to
use
> set_source_tab_id here.

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:66: explicit
SyncSessionsRouterTabHelper(content::WebContents* web_contents,
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> Constructors should be defined before other methods.

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc:19: handler_ =
nullptr;
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> can remove if you initialize in the header

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc:29: // Android
has no BrowserList or TabStripModel, so we exclude code that refers
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> nit: newline above

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:13: #if
!defined(OS_ANDROID)
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> is this pattern of using platform conditional code used elsewhere? If not, I
> think it might be better to just have separate files with the platform
specific
> code as much as possible, otherwise this might become brittle. Thoughts?

I've added a subclass that isolates the android incompatible code.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:76:
LocalSessionEventHandler* handler_;
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> nit: may as well initialize this to nullptr here.

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc
(right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc:12:
namespace sync_sessions {
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> nit: newline below

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h
(right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h:18: }
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> nit: // namespace base

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h:20:
namespace sync_sessions {
On 2017/03/16 19:57:43, Nicolas Zea wrote:
> nit: newline below

Done.

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sessions_helper.h (right):

https://codereview.chromium.org/2753753005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.h:121: // Wait for a
session change to propagate to the model associator. Will return
On 2017/03/16 19:57:44, Nicolas Zea wrote:
> nit: s/model associator/syncableservice, here and elsewhere. There is no model
> associator anymore.

Done.

[commit-bot: I haz the power, 2017-03-17 01:39:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 01:39:39]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2017-03-20 18:14:50]

Mostly LG! Mainly some style issues

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File
chrome/browser/sync/sessions/sync_sessions_multi_windowed_web_contents_router.h
(right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_multi_windowed_web_contents_router.h:20:
class SyncSessionsMultiWindowedWebContentsRouter
Multi-windowed is a bit unclear of a name (Android can in fact have multiple
windows). Consider renaming, perhaps we should just call it
BrowserListWebContentsRouter?

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h (right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:65: // router_ is
a KeyedService and is guaranteed to outlive |this|.
nit: "router_" -> "|router_|"

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:67:
SessionID::id_type source_tab_id_;
Might be good to have a comment explaining this a bit more (e.g. what kind of
events might set this)

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router.h (right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:51:
LocalSessionEventHandler* handler_ = nullptr;
style: data members should be private
https://google.github.io/styleguide/cppguide.html#Inheritance Consider adding a
getter method instead. Although it's also worth considering if perhaps
composition might be a better fit here (inheritance in general is discouraged
beyond basic interfaces)

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc
(right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc:40:
#if !defined(OS_ANDROID)
nit: prefer have the positive version of the conditional be the initial (e.g. if
android do this, else do that). Having negation just adds a tiny bit of
cognitive load without really providing any benefit.

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sessions_helper.cc (right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.cc:210: found =
ModelAssociatorHasTabWithUrl(index, url);
nit: SyncableServiceHasTabWithURL (or perhaps SyncSessionsManagerHasTabWithURL
is clearer)

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.cc:226: // controlled, by
the model associator, so we must make our own copy.
nit: model associator -> SyncableService (may as well clean this up while we're
here, since there's not many references left)

https://codereview.chromium.org/2753753005/diff/80001/ios/chrome/browser/sync...
File ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm (right):

https://codereview.chromium.org/2753753005/diff/80001/ios/chrome/browser/sync...
ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm:55: // todo(pnoland):
add logic to store and return the source tab id on ios.
reference crbug?

[Patrick Noland, 2017-03-21 19:17:10]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 19:17:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 19:46:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 19:46:33]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Patrick Noland, 2017-03-21 20:44:04]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[Patrick Noland, 2017-03-21 20:44:10]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-21 20:44:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 21:10:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 21:10:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Patrick Noland, 2017-03-21 21:22:05]

Patchset #4 (id:120001) has been deleted

[Patrick Noland, 2017-03-21 21:42:16]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 21:43:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 22:14:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:14:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patrick Noland, 2017-03-21 22:54:31]

Nicolas, PTAL

I think the trybot failure is an unrelated flake: http://crbug/703830

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File
chrome/browser/sync/sessions/sync_sessions_multi_windowed_web_contents_router.h
(right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_multi_windowed_web_contents_router.h:20:
class SyncSessionsMultiWindowedWebContentsRouter
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> Multi-windowed is a bit unclear of a name (Android can in fact have multiple
> windows). Consider renaming, perhaps we should just call it
> BrowserListWebContentsRouter?

Done.

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h (right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:65: // router_ is
a KeyedService and is guaranteed to outlive |this|.
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> nit: "router_" -> "|router_|"

Done.

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h:67:
SessionID::id_type source_tab_id_;
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> Might be good to have a comment explaining this a bit more (e.g. what kind of
> events might set this)

Done.

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router.h (right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:51:
LocalSessionEventHandler* handler_ = nullptr;
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> style: data members should be private
> https://google.github.io/styleguide/cppguide.html#Inheritance Consider adding
a
> getter method instead. Although it's also worth considering if perhaps
> composition might be a better fit here (inheritance in general is discouraged
> beyond basic interfaces)

Done. I changed the MultiWindowed router to BrowserListRouterHelper, which is a
standalone object owned by SyncSessionsWebContentsRouter.

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc
(right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.cc:40:
#if !defined(OS_ANDROID)
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> nit: prefer have the positive version of the conditional be the initial (e.g.
if
> android do this, else do that). Having negation just adds a tiny bit of
> cognitive load without really providing any benefit.

With composition, this seemed no longer possible without having an empty #ifdef
block followed by an #else, which I figured was even more cognitive load than
negation.

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sessions_helper.cc (right):

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.cc:210: found =
ModelAssociatorHasTabWithUrl(index, url);
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> nit: SyncableServiceHasTabWithURL (or perhaps SyncSessionsManagerHasTabWithURL
> is clearer)

Done.

https://codereview.chromium.org/2753753005/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.cc:226: // controlled, by
the model associator, so we must make our own copy.
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> nit: model associator -> SyncableService (may as well clean this up while
we're
> here, since there's not many references left)

Done.

https://codereview.chromium.org/2753753005/diff/80001/ios/chrome/browser/sync...
File ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm (right):

https://codereview.chromium.org/2753753005/diff/80001/ios/chrome/browser/sync...
ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm:55: // todo(pnoland):
add logic to store and return the source tab id on ios.
On 2017/03/20 18:14:50, Nicolas Zea wrote:
> reference crbug?

Done.

[Nicolas Zea, 2017-03-22 20:47:56]

LGTM!

[Patrick Noland, 2017-03-22 21:37:34]

Description was changed from

==========
[sync] WebContentsObserver based sessions notifications

Aggregate individual WebContentsObservers to notify sync of sessions changes

This CL introduces the SyncSessionsWebContentsRouter and the
SyncSessionsRouterTabHelper, which replace the old
NotificationServiceSessionsRouter.  The SyncSessionsWebContentsRouter
aggregates events from the individual TabHelpers, which are scoped to
individual tabs, and pushes them to SessionsSyncManager.
SyncSessionsWebContentsRouters are given a source tab id property which
is set when a tab is created directly from another tab, e.g. by ctrl
clicking.
Some changes are made to SessionsHelper so that it can continue to
correctly wait for tabs to load without using Notifications.

R=zea@chromium.org
BUG=695241
==========

to

==========
[sync] WebContentsObserver based sessions notifications

Aggregate individual WebContentsObservers to notify sync of sessions changes

This CL introduces the SyncSessionsWebContentsRouter and the
SyncSessionsRouterTabHelper, which replace the old
NotificationServiceSessionsRouter.  The SyncSessionsWebContentsRouter
aggregates events from the individual TabHelpers, which are scoped to
individual tabs, and pushes them to SessionsSyncManager.
SyncSessionsWebContentsRouters are given a source tab id property which
is set when a tab is created directly from another tab, e.g. by ctrl
clicking.
Some changes are made to SessionsHelper so that it can continue to
correctly wait for tabs to load without using Notifications.

R=zea@chromium.org
BUG=695241
==========

[Patrick Noland, 2017-03-22 21:37:35]

pnoland@chromium.org changed reviewers:
+ avi@chromium.org, pkasting@chromium.org

[Patrick Noland, 2017-03-22 21:39:22]

+Avi for tab_helpers and app_menu_controller_unittest
+pkasting for recent_tabs_sub_menu_model_unittest

Avi & Peter, PTAL

[Peter Kasting, 2017-03-22 21:52:15]

LGTM

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right):

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:133:
dummy_router_.reset(new DummyRouter());
Nit: Prefer MakeUnique over bare new:

  dummy_router_ = base::MakeUnique<DummyRouter>();

Optional:

* Fix other places here
* Do some/all of these in the initializer list or in the declaration points,
instead of in code here

[Avi (use Gerrit), 2017-03-23 02:27:08]

lgtm with fix

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:215:
Profile::FromBrowserContext(web_contents->GetBrowserContext())));
Be alphabetical. I'll alphabetize the existing ones when you're committed.

[Patrick Noland, 2017-03-23 18:31:54]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 18:32:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 19:12:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 19:12:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patrick Noland, 2017-03-23 20:15:03]

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:215:
Profile::FromBrowserContext(web_contents->GetBrowserContext())));
On 2017/03/23 02:27:07, Avi wrote:
> Be alphabetical. I'll alphabetize the existing ones when you're committed.

Done.

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right):

https://codereview.chromium.org/2753753005/diff/140001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:133:
dummy_router_.reset(new DummyRouter());
On 2017/03/22 21:52:15, Peter Kasting wrote:
> Nit: Prefer MakeUnique over bare new:
> 
>   dummy_router_ = base::MakeUnique<DummyRouter>();
> 
> Optional:
> 
> * Fix other places here
> * Do some/all of these in the initializer list or in the declaration points,
> instead of in code here

Done.

[Patrick Noland, 2017-03-23 20:15:16]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2017-03-23 20:15:16]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
avi@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2753753005/#ps160001
(title: "use base:MakeUnique, alphabetize")

[commit-bot: I haz the power, 2017-03-23 20:15:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 21:24:05]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490300116016870,
"parent_rev": "5fbf7e618bca38528d65a5f673b030fb0b2cd684", "commit_rev":
"1901afa5e03bad9ce8a72691f746a685038241b0"}

[commit-bot: I haz the power, 2017-03-23 21:24:57]

Description was changed from

==========
[sync] WebContentsObserver based sessions notifications

Aggregate individual WebContentsObservers to notify sync of sessions changes

This CL introduces the SyncSessionsWebContentsRouter and the
SyncSessionsRouterTabHelper, which replace the old
NotificationServiceSessionsRouter.  The SyncSessionsWebContentsRouter
aggregates events from the individual TabHelpers, which are scoped to
individual tabs, and pushes them to SessionsSyncManager.
SyncSessionsWebContentsRouters are given a source tab id property which
is set when a tab is created directly from another tab, e.g. by ctrl
clicking.
Some changes are made to SessionsHelper so that it can continue to
correctly wait for tabs to load without using Notifications.

R=zea@chromium.org
BUG=695241
==========

to

==========
[sync] WebContentsObserver based sessions notifications

Aggregate individual WebContentsObservers to notify sync of sessions changes

This CL introduces the SyncSessionsWebContentsRouter and the
SyncSessionsRouterTabHelper, which replace the old
NotificationServiceSessionsRouter.  The SyncSessionsWebContentsRouter
aggregates events from the individual TabHelpers, which are scoped to
individual tabs, and pushes them to SessionsSyncManager.
SyncSessionsWebContentsRouters are given a source tab id property which
is set when a tab is created directly from another tab, e.g. by ctrl
clicking.
Some changes are made to SessionsHelper so that it can continue to
correctly wait for tabs to load without using Notifications.

R=zea@chromium.org
BUG=695241

Review-Url: https://codereview.chromium.org/2753753005
Cr-Commit-Position: refs/heads/master@{#459225}
Committed:
https://chromium.googlesource.com/chromium/src/+/1901afa5e03bad9ce8a72691f746...
==========

[commit-bot: I haz the power, 2017-03-23 21:24:59]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/1901afa5e03bad9ce8a72691f746...