Foreign tab suggestions should update when disabling sync or signing out.

Foreign tab suggestions should update when disabling sync or signing out.

Previously we were responding to OnSyncConfigurationCompleted by
invoking our callback to regenerate suggestions. This covered initial
sync setup, along with enabling or disabling any types from sync, in
case Open Tabs was one of the types that was changing. However, when
sync is completely disabled or the user signs out, we don't perform an
configuration, but session data is no longer there. We want to clear
our suggestions, but the hook we were using isn't called.

The approach this CL takes is to switch to using OnStateChanged for
this tracking. This event is called extremely often, for a variety of
things. Because generating suggestions when there are actual sessions
data is not computationally trivial, we only want to invoke the
callback in TabDelegateSyncAdapter when it is reasonable to do so, so
we've added a bool state variable to track the previous state, and only
invoke the callback when we're transitioning between states.

Note that this change does not mean that the current or any existing
NTPs are going to remove their foreign sessions data. By having the
ForeignSessionsSuggestionsProvider provide new suggestions, this
current only seems to affect NTPs that are opened afterwards.

BUG=669041
Committed: https://crrev.com/2fc56e330bdaef8b163ef126bb2a81c4d946ac43
Cr-Commit-Position: refs/heads/master@{#439578}

[skym, 2016-12-15 22:35:30]

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

[commit-bot: I haz the power, 2016-12-15 22:36:17]

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

[skym, 2016-12-15 22:41:12]

Description was changed from

==========
Foreign tab suggestions should update when disabling sync or signing out.

BUG=669041
==========

to

==========
Foreign tab suggestions should update when disabling sync or signing out.

Previously we were responding to OnSyncConfigurationCompleted by
invoking our callback to regenerate suggestions. This covered initial
sync setup, along with enabling or disabling any types from sync, in
case Open Tabs was one of the types that was changing. However, when
sync is completely disabled or the user signs out, we don't perform an
configuration, but session data is no longer there. We want to clear
our suggestions, but the hook we were using isn't called.

The approach this CL takes is to switch to using OnStateChanged for
this tracking. This event is called extremely often, for a variety of
things. Because generating suggestions when there are actual sessions
data is not computationally trivial, we only want to invoke the
callback in TabDelegateSyncAdapter when it is reasonable to do so, so
we've added a bool state variable to track the previous state, and only
invoke the callback when we're transitioning between states.

Note that this change does not mean that the current or any existing
NTPs are going to remove their foreign sessions data. By having the
ForeignSessionsSuggestionsProvider provide new suggestions, this
current only seems to affect NTPs that are opened afterwards.

BUG=669041
==========

[skym, 2016-12-15 22:41:12]

skym@chromium.org changed reviewers:
+ treib@chromium.org

[commit-bot: I haz the power, 2016-12-15 23:10:35]

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

[commit-bot: I haz the power, 2016-12-15 23:10:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[skym, 2016-12-15 23:30:35]

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

[commit-bot: I haz the power, 2016-12-15 23:33:15]

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, 2016-12-16 00:26:58]

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

[commit-bot: I haz the power, 2016-12-16 00:26:59]

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

[skym, 2016-12-16 15:25:28]

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

[commit-bot: I haz the power, 2016-12-16 15:25:45]

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, 2016-12-16 16:25:11]

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

[commit-bot: I haz the power, 2016-12-16 16:25:12]

Dry run: This issue passed the CQ dry run.

[skym, 2016-12-16 16:44:15]

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

[skym, 2016-12-16 16:44:38]

skym@chromium.org changed reviewers:
+ maxbogue@chromium.org

[commit-bot: I haz the power, 2016-12-16 16:44:42]

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

[skym, 2016-12-16 16:45:20]

PTAL.

treib@ - ntp_snippets/
maxbogue@ - sync_sessions/

[commit-bot: I haz the power, 2016-12-16 17:57:21]

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

[commit-bot: I haz the power, 2016-12-16 17:57:21]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-12-16 22:21:34]

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc (right):

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:54: // realize
that this approach requires the data returned by
Parse error, plz fix this sentence.

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:57: // nullptr.
This is currently the case, as GetOpenTabsUIDelegate() is gaurded
guarded*

https://codereview.chromium.org/2578293002/diff/80001/components/sync_session...
File components/sync_sessions/open_tabs_ui_delegate.h (left):

https://codereview.chromium.org/2578293002/diff/80001/components/sync_session...
components/sync_sessions/open_tabs_ui_delegate.h:37: const SessionID::id_type
tab_id,
Why's this const being removed? Is this left over from another patch or is it
necessary for the mock?

[Marc Treib, 2016-12-19 11:47:53]

Sorry for the delay!
ntp_snippets LGTM after maxbogue's comments are addressed; some more nits below.

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc (right):

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:79:
had_session_data_ = HasSessionsData();
I think updating had_sessions_data_ should happen inside InvokeCallback - WDYT?

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/sessions/tab_delegate_sync_adapter_unittest.cc
(right):

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter_unittest.cc:87:
EXPECT_EQ(0, CallbackCount());
nit: ASSERT_EQ since this is a precondition rather than part of the test itself;
similar in the other tests.

[skym, 2016-12-19 19:51:02]

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

[commit-bot: I haz the power, 2016-12-19 19:51:33]

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

[skym, 2016-12-19 20:31:43]

Updated for comments.

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc (right):

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:54: // realize
that this approach requires the data returned by
On 2016/12/16 22:21:34, maxbogue wrote:
> Parse error, plz fix this sentence.

Done.

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:57: // nullptr.
This is currently the case, as GetOpenTabsUIDelegate() is gaurded
On 2016/12/16 22:21:34, maxbogue wrote:
> guarded*

Done.

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:79:
had_session_data_ = HasSessionsData();
On 2016/12/19 11:47:53, Marc Treib (OOO until Jan 12) wrote:
> I think updating had_sessions_data_ should happen inside InvokeCallback -
WDYT?

Done.

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/sessions/tab_delegate_sync_adapter_unittest.cc
(right):

https://codereview.chromium.org/2578293002/diff/80001/components/ntp_snippets...
components/ntp_snippets/sessions/tab_delegate_sync_adapter_unittest.cc:87:
EXPECT_EQ(0, CallbackCount());
On 2016/12/19 11:47:53, Marc Treib (OOO until Jan 12) wrote:
> nit: ASSERT_EQ since this is a precondition rather than part of the test
itself;
> similar in the other tests.

Done.

https://codereview.chromium.org/2578293002/diff/80001/components/sync_session...
File components/sync_sessions/open_tabs_ui_delegate.h (left):

https://codereview.chromium.org/2578293002/diff/80001/components/sync_session...
components/sync_sessions/open_tabs_ui_delegate.h:37: const SessionID::id_type
tab_id,
On 2016/12/16 22:21:34, maxbogue wrote:
> Why's this const being removed? Is this left over from another patch or is it
> necessary for the mock?

Necessary for the mock. So my theory, is that someone (maybe GMOCK) was dropping
the const on the primitive (SessionID::id_type), because it doesn't do anything
on a pure virtual method. However, not everyone dropped it, and MSVC always
complained about the mismatch. I tried dropping just the const from my gmock
usage (see PS#3) but that didn't help.

So, instead, I dropped the const from everything (except for
sessions_sync_manager.cc), and compilation was happy. I didn't think it warrants
a comment because

1) const on primitives on virtual methods is pointless
2) if you try to change this, you'll get a compile error

[maxbogue, 2016-12-19 20:35:16]

lgtm

https://codereview.chromium.org/2578293002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc (right):

https://codereview.chromium.org/2578293002/diff/120001/components/ntp_snippet...
components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:57: // results.
Fourtunately,this isn'ta problem. GetOpenTabsUIDelegate() is
"Fourtunately,this isn'ta" -> "Fortunately, this isn't a"

[skym, 2016-12-19 20:41:57]

The CQ bit was checked by skym@chromium.org

[skym, 2016-12-19 20:41:57]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2578293002/#ps140001
(title: "Updating comment.")

[commit-bot: I haz the power, 2016-12-19 20:42:38]

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

[skym, 2016-12-19 20:44:54]

The CQ bit was unchecked by skym@chromium.org

[skym, 2016-12-19 20:45:42]

The CQ bit was checked by skym@chromium.org

[skym, 2016-12-19 20:45:43]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2578293002/#ps160001
(title: "Fixed more spelling errors.")

[commit-bot: I haz the power, 2016-12-19 20:46:21]

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, 2016-12-19 21:44:57]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482180342897990,
"parent_rev": "d03b9d8f0cf37f68dfc38ae09f90fcbbf52fdbcd", "commit_rev":
"085fdb6972806055105e4527de8b792a0f8e9628"}

[commit-bot: I haz the power, 2016-12-19 21:45:23]

Description was changed from

==========
Foreign tab suggestions should update when disabling sync or signing out.

Previously we were responding to OnSyncConfigurationCompleted by
invoking our callback to regenerate suggestions. This covered initial
sync setup, along with enabling or disabling any types from sync, in
case Open Tabs was one of the types that was changing. However, when
sync is completely disabled or the user signs out, we don't perform an
configuration, but session data is no longer there. We want to clear
our suggestions, but the hook we were using isn't called.

The approach this CL takes is to switch to using OnStateChanged for
this tracking. This event is called extremely often, for a variety of
things. Because generating suggestions when there are actual sessions
data is not computationally trivial, we only want to invoke the
callback in TabDelegateSyncAdapter when it is reasonable to do so, so
we've added a bool state variable to track the previous state, and only
invoke the callback when we're transitioning between states.

Note that this change does not mean that the current or any existing
NTPs are going to remove their foreign sessions data. By having the
ForeignSessionsSuggestionsProvider provide new suggestions, this
current only seems to affect NTPs that are opened afterwards.

BUG=669041
==========

to

==========
Foreign tab suggestions should update when disabling sync or signing out.

Previously we were responding to OnSyncConfigurationCompleted by
invoking our callback to regenerate suggestions. This covered initial
sync setup, along with enabling or disabling any types from sync, in
case Open Tabs was one of the types that was changing. However, when
sync is completely disabled or the user signs out, we don't perform an
configuration, but session data is no longer there. We want to clear
our suggestions, but the hook we were using isn't called.

The approach this CL takes is to switch to using OnStateChanged for
this tracking. This event is called extremely often, for a variety of
things. Because generating suggestions when there are actual sessions
data is not computationally trivial, we only want to invoke the
callback in TabDelegateSyncAdapter when it is reasonable to do so, so
we've added a bool state variable to track the previous state, and only
invoke the callback when we're transitioning between states.

Note that this change does not mean that the current or any existing
NTPs are going to remove their foreign sessions data. By having the
ForeignSessionsSuggestionsProvider provide new suggestions, this
current only seems to affect NTPs that are opened afterwards.

BUG=669041

Review-Url: https://codereview.chromium.org/2578293002
==========

[commit-bot: I haz the power, 2016-12-19 21:45:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-19 21:47:21]

Description was changed from

==========
Foreign tab suggestions should update when disabling sync or signing out.

Previously we were responding to OnSyncConfigurationCompleted by
invoking our callback to regenerate suggestions. This covered initial
sync setup, along with enabling or disabling any types from sync, in
case Open Tabs was one of the types that was changing. However, when
sync is completely disabled or the user signs out, we don't perform an
configuration, but session data is no longer there. We want to clear
our suggestions, but the hook we were using isn't called.

The approach this CL takes is to switch to using OnStateChanged for
this tracking. This event is called extremely often, for a variety of
things. Because generating suggestions when there are actual sessions
data is not computationally trivial, we only want to invoke the
callback in TabDelegateSyncAdapter when it is reasonable to do so, so
we've added a bool state variable to track the previous state, and only
invoke the callback when we're transitioning between states.

Note that this change does not mean that the current or any existing
NTPs are going to remove their foreign sessions data. By having the
ForeignSessionsSuggestionsProvider provide new suggestions, this
current only seems to affect NTPs that are opened afterwards.

BUG=669041

Review-Url: https://codereview.chromium.org/2578293002
==========

to

==========
Foreign tab suggestions should update when disabling sync or signing out.

Previously we were responding to OnSyncConfigurationCompleted by
invoking our callback to regenerate suggestions. This covered initial
sync setup, along with enabling or disabling any types from sync, in
case Open Tabs was one of the types that was changing. However, when
sync is completely disabled or the user signs out, we don't perform an
configuration, but session data is no longer there. We want to clear
our suggestions, but the hook we were using isn't called.

The approach this CL takes is to switch to using OnStateChanged for
this tracking. This event is called extremely often, for a variety of
things. Because generating suggestions when there are actual sessions
data is not computationally trivial, we only want to invoke the
callback in TabDelegateSyncAdapter when it is reasonable to do so, so
we've added a bool state variable to track the previous state, and only
invoke the callback when we're transitioning between states.

Note that this change does not mean that the current or any existing
NTPs are going to remove their foreign sessions data. By having the
ForeignSessionsSuggestionsProvider provide new suggestions, this
current only seems to affect NTPs that are opened afterwards.

BUG=669041

Committed: https://crrev.com/2fc56e330bdaef8b163ef126bb2a81c4d946ac43
Cr-Commit-Position: refs/heads/master@{#439578}
==========

[commit-bot: I haz the power, 2016-12-19 21:47:22]

Patchset 9 (id:??) landed as
https://crrev.com/2fc56e330bdaef8b163ef126bb2a81c4d946ac43
Cr-Commit-Position: refs/heads/master@{#439578}