[Sync] Componentize synced_tab_delegate

[Sync] Componentize synced_tab_delegate

SyncedTabDelegate now just returns the data used by Sessions directly, and
moves all content interaction into its implementation.

As part of this, synced_tab_delegate_desktop becomes obsolete, as the
logic to decide which type of synced tab delegate is now just inlined in the
NotificationServiceSessionsRouter. In addition, SyncedSessionsUtil has
been distributed throughout where it was actually used, and ShouldSyncURL
is now a member off the new SyncSessionsClient interface.

BUG=512468, 512471
Committed: https://crrev.com/f09345c6dffa2e7f4a7b3ec3a197861fc7b71d24
Cr-Commit-Position: refs/heads/master@{#356240}

[Nicolas Zea, 2015-10-15 21:11:15]

Patchset #3 (id:40001) has been deleted

[Nicolas Zea, 2015-10-15 21:21:38]

zea@chromium.org changed reviewers:
+ bauerb@chromium.org, pavely@chromium.org

[Nicolas Zea, 2015-10-15 21:21:39]

+Bernhard for Supervised user changes (it's now emitting
SerializedNavigationEntries, rather than NavigationEntries)

+Pavel for everything else

+FYI Max and Sky

[Bernhard Bauer, 2015-10-16 10:37:33]

supervised users LGTM.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_observer.h
(right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_observer.h:67:
ScopedVector<const sessions::SerializedNavigationEntry> blocked_navigations_;
If SerializedNavigationEntry is copyable (and we are okay with copying it
around), we could also make this a plain std::vector.

[blundell, 2015-10-16 10:41:25]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-10-16 10:41:26]

This looks great at a high level :).

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/synced_tab_delegate.h (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.h:58: virtual bool HasWebContents()
const = 0;
drive-by nit: This method should be renamed, assuming it's actually still needed
in this interface.

[skym, 2015-10-16 16:55:46]

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

[skym, 2015-10-16 16:55:46]

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

[skym, 2015-10-16 16:55:47]

Sorry for the obnoxious nits.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_tab_delegate_android.h (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:30: SessionID::id_type
GetWindowId() const override;
While you're in here, should probably #include for id_type.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:33: std::string
GetExtensionAppId() const override;
While you're in here, #include <string>. Although, I thought, the PRESUBMIT lint
check should have caught this one. /shrug

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:37: GURL
GetVirtualURLAtIndex(int i) const override;
Forward declare GURL

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:39: ui::PageTransition
GetTransitionAtIndex(int i) const override;
#include for ui::PageTransition

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:42:
sessions::SerializedNavigationEntry* serialized_entry) const override;
Forward declare SerializedNavigationEntry

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:49:
GetBlockedNavigations() const override;
Indentation looks odd here, might be the cr tool.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:421: if
(!ShouldSyncTab(sessions_client_, modified_tab))
Won't this miss things like if you have a tab

www.google.com -> chrome://1 -> chrome://2 -> chrome://3 -> chrome://4 ->
chrome://5

As you transition to the next 'uninteresting' entry, say chrome://6, and the
last entry rolls off, foreign clients should remove this tab, but instead the
behavior will be it kind of freezes/gets stuck. Admittedly, this case probably
isn't very important.

I think the existing code had a similar problem, it wasn't flaring in this case.
But at least it was still processing the change.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:447: // Note, we always
associate windows because it's possible a tab became
It seems AssociateWindows gets called every time to solve a similar problem as
my comment above. We don't seem to have a way to tell if it was previously
synced. I would think though, this kind of logic is exactly what the session
tracker is supposed to do. I wonder if it already does this.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/sync_sessions_client.h (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/sync_sessions_client.h:11: class SyncedTabDelegate;
Are these forward declarations in preparation for something? Or did you change
your mind somewhere and never remove them?

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/sync_sessions_client.h:15: struct SessionTab;
Same.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/synced_tab_delegate.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.cc:22: const SyncedWindowDelegate*
SyncedTabDelegate::GetSyncedWindowDelegate() const {
Needs #include for SyncedWindowDelegate I think.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/synced_tab_delegate.h (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.h:15: #include
"ui/base/page_transition_types.h"
Errr, what? I thought ui/ dependencies in components/ was forbidden. I'm
confused.

[skym, 2015-10-16 16:55:47]

Sorry for the obnoxious nits.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_tab_delegate_android.h (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:33: std::string
GetExtensionAppId() const override;
While you're in here, #include <string>. Although, I thought, the PRESUBMIT lint
check should have caught this one. /shrug

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:421: if
(!ShouldSyncTab(sessions_client_, modified_tab))
Won't this miss things like if you have a tab

www.google.com -> chrome://1 -> chrome://2 -> chrome://3 -> chrome://4 ->
chrome://5

As you transition to the next 'uninteresting' entry, say chrome://6, and the
last entry rolls off, foreign clients should remove this tab, but instead the
behavior will be it kind of freezes/gets stuck. Admittedly, this case probably
isn't very important.

I think the existing code had a similar problem, it wasn't flaring in this case.
But at least it was still processing the change.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:447: // Note, we always
associate windows because it's possible a tab became
It seems AssociateWindows gets called every time to solve a similar problem as
my comment above. We don't seem to have a way to tell if it was previously
synced. I would think though, this kind of logic is exactly what the session
tracker is supposed to do. I wonder if it already does this.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/sync_sessions_client.h (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/sync_sessions_client.h:11: class SyncedTabDelegate;
Are these forward declarations in preparation for something? Or did you change
your mind somewhere and never remove them?

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/sync_sessions_client.h:15: struct SessionTab;
Same.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/synced_tab_delegate.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.cc:22: const SyncedWindowDelegate*
SyncedTabDelegate::GetSyncedWindowDelegate() const {
Needs #include for SyncedWindowDelegate I think.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/synced_tab_delegate.h (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.h:15: #include
"ui/base/page_transition_types.h"
Errr, what? I thought ui/ dependencies in components/ was forbidden. I'm
confused.

[pavely, 2015-10-16 21:55:20]

lgtm with minor comments.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.h (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:59: scoped_ptr<SyncSessionsClientImpl>
sync_sessions_client_;
You can hold pointer to SyncSessionClient, don't have to hold
SyncSessionClientImpl. In this case you won't need to forward declare Impl.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_session_tracker.cc:21: for (auto* tab :
window.tabs) {
Optional comment:
I think in both "for" loops you could use "const auto&"
Also to understand this code I needed to look up types of tabs and navigations.
Maybe it would make sense to spell out types inside "for" loops (I'm not sure).

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:1050:
tab_delegate.GetSerializedNavigationAtIndex(i, &serialized_entry);
Nit: It would make sense to move obtaining serialized_entry 4 lines down to
where it is pushed into navigations.

[maxbogue, 2015-10-17 21:53:57]

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

[maxbogue, 2015-10-17 21:53:58]

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:75: // Chrome implementation of
SyncSesssionsClient. Needs to be in a separate class
s/sss/ss/

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:83: // SyncSesssionsClient
implementation.
s/sss/ss/

[Bernhard Bauer, 2015-10-19 08:20:57]

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_session_tracker.cc:21: for (auto* tab :
window.tabs) {
On 2015/10/16 21:55:20, pavely wrote:
> Optional comment:
> I think in both "for" loops you could use "const auto&"
> Also to understand this code I needed to look up types of tabs and
navigations.
> Maybe it would make sense to spell out types inside "for" loops (I'm not
sure).

+1 for spelling out types.

[Nicolas Zea, 2015-10-20 20:13:16]

Patchset #7 (id:140001) has been deleted

[Nicolas Zea, 2015-10-20 23:14:42]

Comments addressed. Pavel/Sky, PTAL, as I had to refactor how the ShouldSync is
checked and moved it into the SyncedTabDelegate.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_observer.h
(right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_observer.h:67:
ScopedVector<const sessions::SerializedNavigationEntry> blocked_navigations_;
On 2015/10/16 10:37:33, Bernhard Bauer wrote:
> If SerializedNavigationEntry is copyable (and we are okay with copying it
> around), we could also make this a plain std::vector.

It is copyable, but that could be fairly expensive, so I think I'd prefer to
leave it like this.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:75: // Chrome implementation of
SyncSesssionsClient. Needs to be in a separate class
On 2015/10/17 21:53:58, maxbogue wrote:
> s/sss/ss/

Done.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:83: // SyncSesssionsClient
implementation.
On 2015/10/17 21:53:58, maxbogue wrote:
> s/sss/ss/

Done.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.h (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:59: scoped_ptr<SyncSessionsClientImpl>
sync_sessions_client_;
On 2015/10/16 21:55:20, pavely wrote:
> You can hold pointer to SyncSessionClient, don't have to hold
> SyncSessionClientImpl. In this case you won't need to forward declare Impl.

Good call, done.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_session_tracker.cc:21: for (auto* tab :
window.tabs) {
On 2015/10/16 21:55:20, pavely wrote:
> Optional comment:
> I think in both "for" loops you could use "const auto&"
> Also to understand this code I needed to look up types of tabs and
navigations.
> Maybe it would make sense to spell out types inside "for" loops (I'm not
sure).

Done.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_session_tracker.cc:21: for (auto* tab :
window.tabs) {
On 2015/10/19 08:20:57, Bernhard Bauer wrote:
> On 2015/10/16 21:55:20, pavely wrote:
> > Optional comment:
> > I think in both "for" loops you could use "const auto&"
> > Also to understand this code I needed to look up types of tabs and
> navigations.
> > Maybe it would make sense to spell out types inside "for" loops (I'm not
> sure).
> 
> +1 for spelling out types.

Acknowledged.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_tab_delegate_android.h (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:30: SessionID::id_type
GetWindowId() const override;
On 2015/10/16 16:55:46, skym wrote:
> While you're in here, should probably #include for id_type.

So, my interpretation of IWYU is that it's not necessary when it's a type you're
getting from the class you implement (with the exception of std libs). As such,
all the methods of SyncedTabDelegate are fine (and we therefore don't need to
duplicate all the includes rom synced tab delegate).

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/i_5qAOxe0xY
seems to confirm this

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:33: std::string
GetExtensionAppId() const override;
On 2015/10/16 16:55:47, skym wrote:
> While you're in here, #include <string>. Although, I thought, the PRESUBMIT
lint
> check should have caught this one. /shrug

Done.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:37: GURL
GetVirtualURLAtIndex(int i) const override;
On 2015/10/16 16:55:46, skym wrote:
> Forward declare GURL

See above

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:39: ui::PageTransition
GetTransitionAtIndex(int i) const override;
On 2015/10/16 16:55:46, skym wrote:
> #include for ui::PageTransition

see above

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:42:
sessions::SerializedNavigationEntry* serialized_entry) const override;
On 2015/10/16 16:55:46, skym wrote:
> Forward declare SerializedNavigationEntry

See above.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.h:49:
GetBlockedNavigations() const override;
On 2015/10/16 16:55:46, skym wrote:
> Indentation looks odd here, might be the cr tool.

Yeah, this is git cl format's work.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:421: if
(!ShouldSyncTab(sessions_client_, modified_tab))
On 2015/10/16 16:55:47, skym wrote:
> Won't this miss things like if you have a tab
> 
> http://www.google.com -> chrome://1 -> chrome://2 -> chrome://3 -> chrome://4
->
> chrome://5
> 
> As you transition to the next 'uninteresting' entry, say chrome://6, and the
> last entry rolls off, foreign clients should remove this tab, but instead the
> behavior will be it kind of freezes/gets stuck. Admittedly, this case probably
> isn't very important.
> 
> I think the existing code had a similar problem, it wasn't flaring in this
case.
> But at least it was still processing the change.

Good catch! You're right, previously we'd have removed the tab from the header
node, but this bypasses the AssociateWindows call. Fixed, and added a test case
for it.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:447: // Note, we always
associate windows because it's possible a tab became
On 2015/10/16 16:55:47, skym wrote:
> It seems AssociateWindows gets called every time to solve a similar problem as
> my comment above. We don't seem to have a way to tell if it was previously
> synced. I would think though, this kind of logic is exactly what the session
> tracker is supposed to do. I wonder if it already does this.

I'm not sure I understand your comment. What do you mean by previously synced?

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/sync_sessions_client.h (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/sync_sessions_client.h:11: class SyncedTabDelegate;
On 2015/10/16 16:55:47, skym wrote:
> Are these forward declarations in preparation for something? Or did you change
> your mind somewhere and never remove them?

Woops, these were leftovers from previous iterations. Removed.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/sync_sessions_client.h:15: struct SessionTab;
On 2015/10/16 16:55:47, skym wrote:
> Same.

Done.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/synced_tab_delegate.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.cc:22: const SyncedWindowDelegate*
SyncedTabDelegate::GetSyncedWindowDelegate() const {
On 2015/10/16 16:55:47, skym wrote:
> Needs #include for SyncedWindowDelegate I think.

As part of cleaning this up, I realized this code isn't needed at all.

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
File components/sync_sessions/synced_tab_delegate.h (right):

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.h:15: #include
"ui/base/page_transition_types.h"
On 2015/10/16 16:55:47, skym wrote:
> Errr, what? I thought ui/ dependencies in components/ was forbidden. I'm
> confused.

We can include ui/, just not content/ or chrome/

https://codereview.chromium.org/1408643002/diff/120001/components/sync_sessio...
components/sync_sessions/synced_tab_delegate.h:58: virtual bool HasWebContents()
const = 0;
On 2015/10/16 10:41:26, blundell wrote:
> drive-by nit: This method should be renamed, assuming it's actually still
needed
> in this interface.

It is in fact still used (although this made me realized IsPinned could be
removed). Renamed to IsFullyLoaded, and added comment about what its for.

[skym, 2015-10-21 00:38:36]

lgtm

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:421: if
(!ShouldSyncTab(sessions_client_, modified_tab))
On 2015/10/20 23:14:42, Nicolas Zea wrote:
> On 2015/10/16 16:55:47, skym wrote:
> > Won't this miss things like if you have a tab
> > 
> > http://www.google.com -> chrome://1 -> chrome://2 -> chrome://3 ->
chrome://4
> ->
> > chrome://5
> > 
> > As you transition to the next 'uninteresting' entry, say chrome://6, and the
> > last entry rolls off, foreign clients should remove this tab, but instead
the
> > behavior will be it kind of freezes/gets stuck. Admittedly, this case
probably
> > isn't very important.
> > 
> > I think the existing code had a similar problem, it wasn't flaring in this
> case.
> > But at least it was still processing the change.
> 
> Good catch! You're right, previously we'd have removed the tab from the header
> node, but this bypasses the AssociateWindows call. Fixed, and added a test
case
> for it.

I'm a little sad to see this optimization go away, but it does keep things
easier/simpler. I think when we convert this to USS we're going to have to solve
this problem, since the directory won't be doing bit-wise diffs for us anymore.

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:447: // Note, we always
associate windows because it's possible a tab became
On 2015/10/20 23:14:42, Nicolas Zea wrote:
> On 2015/10/16 16:55:47, skym wrote:
> > It seems AssociateWindows gets called every time to solve a similar problem
as
> > my comment above. We don't seem to have a way to tell if it was previously
> > synced. I would think though, this kind of logic is exactly what the session
> > tracker is supposed to do. I wonder if it already does this.
> 
> I'm not sure I understand your comment. What do you mean by previously synced?

Oooh, good point, this is more complex than I realized. I guess, did the last
version that was sent to the change processor of the containing window for this
tab, refer to this tab or not... Definitely out of scope of your changes, I was
just thinking.

https://codereview.chromium.org/1408643002/diff/220001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/1408643002/diff/220001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1656: // Only the
header node should be updated, and it should no longer have any
I was really startled that we don't delete tabs when I read this comment... But
I guess, if no windows ref it, it's essentially deleted, and sending the delete
would only cost bandwidth. Odd.

[Bernhard Bauer, 2015-10-21 10:48:34]

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_observer.h
(right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_observer.h:67:
ScopedVector<const sessions::SerializedNavigationEntry> blocked_navigations_;
On 2015/10/20 23:14:41, Nicolas Zea wrote:
> On 2015/10/16 10:37:33, Bernhard Bauer wrote:
> > If SerializedNavigationEntry is copyable (and we are okay with copying it
> > around), we could also make this a plain std::vector.
> 
> It is copyable, but that could be fairly expensive, so I think I'd prefer to
> leave it like this.

Hm, in that case...
1. Wouldn't it make sense to move the newly allocated SerializedNavigationEntry
in there instead of copying it?
2. In general, I find that if it is possible to use something in the wrong way,
people will do it. So if copying is indeed expensive, should we disallow
implicit copying and assignment, and maybe add an explicit Copy() method?

[pavely, 2015-10-21 18:49:32]

lgtm

[Nicolas Zea, 2015-10-22 00:29:10]

zea@chromium.org changed reviewers:
+ avi@chromium.org, sky@chromium.org

[Nicolas Zea, 2015-10-22 00:29:11]

+Avi for ui/base dependency in sync_sessions (SyncedTabDelegate now returns
transition types and is moving into components/sync_sessions)

+Sky for chrome/browser/DEPS, chrome/test/BUILD.gn, and the chrome/ui test
changes (a callsite was updated).

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_observer.h
(right):

https://codereview.chromium.org/1408643002/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_observer.h:67:
ScopedVector<const sessions::SerializedNavigationEntry> blocked_navigations_;
On 2015/10/21 10:48:34, Bernhard Bauer wrote:
> On 2015/10/20 23:14:41, Nicolas Zea wrote:
> > On 2015/10/16 10:37:33, Bernhard Bauer wrote:
> > > If SerializedNavigationEntry is copyable (and we are okay with copying it
> > > around), we could also make this a plain std::vector.
> > 
> > It is copyable, but that could be fairly expensive, so I think I'd prefer to
> > leave it like this.
> 
> Hm, in that case...
> 1. Wouldn't it make sense to move the newly allocated
SerializedNavigationEntry
> in there instead of copying it?
> 2. In general, I find that if it is possible to use something in the wrong
way,
> people will do it. So if copying is indeed expensive, should we disallow
> implicit copying and assignment, and maybe add an explicit Copy() method?

It's not so much that making copies is bad, as that Session sync will be
accessing this theoretically quite often.

Keep in mind, we only make a new copy when we create the serialized navigation
entry, which in this case is whenever a new navigation is blocked, so should be
fairly rare.

On the other hand, Sessions sync will be accessing the set of blocked
navigations at least as often as on every navigation within that tab. That's
much more common, and is the scenario I'd prefer to avoid doing unnecessary
copying with.

[Avi (use Gerrit), 2015-10-22 01:08:10]

Sigh.

page_transition_types doesn't really belong in ui/base, but kinda ended up there
because no one had anywhere else to put it.

I need to bring this up for discussion.

[Nicolas Zea, 2015-10-22 03:23:48]

Description was changed from

==========
[Sync] Componentize synced_tab_delegate

SyncedTabDelegate now just returns the data used by Sessions directly, and
moves all content interaction into its implementation.

As part of this, synced_tab_delegate_desktop becomes obsolete, as the
logic to decide which type of synced tab delegate is now just inlined in the
NotificationServiceSessionsRouter. In addition, SyncedSessionsUtil has
been distributed throughout where it was actually used, and ShouldSyncURL
is now a member off the new SyncSessionsClient interface.

Note that to make this work, a change was made to SessionsSyncManager
to filter out tab changes with only pending navigations. As such, only after
the first navigation is committed will sessions sync actually update
the sync data. Tests were updated to reflect this change.

BUG=512468, 512471
==========

to

==========
[Sync] Componentize synced_tab_delegate

SyncedTabDelegate now just returns the data used by Sessions directly, and
moves all content interaction into its implementation.

As part of this, synced_tab_delegate_desktop becomes obsolete, as the
logic to decide which type of synced tab delegate is now just inlined in the
NotificationServiceSessionsRouter. In addition, SyncedSessionsUtil has
been distributed throughout where it was actually used, and ShouldSyncURL
is now a member off the new SyncSessionsClient interface.

BUG=512468, 512471
==========

[blundell, 2015-10-22 06:21:55]

On 2015/10/22 01:08:10, Avi wrote:
> Sigh.
> 
> page_transition_types doesn't really belong in ui/base, but kinda ended up
there
> because no one had anywhere else to put it.
> 
> I need to bring this up for discussion.

This is fairly off-topic at this point, but we actually had a *lengthy*
discussion that resulted in it going into //ui/base (me, sdefresne@, jam@,
brettw@, beng@). I agree with your sentiment (everyone acknowledged that it was
the least-bad of a set of not-great choices), but I'm not sure that reopening
the discussion will result in a different conclusion.

[blundell, 2015-10-22 06:24:39]

On 2015/10/22 06:21:55, blundell wrote:
> On 2015/10/22 01:08:10, Avi wrote:
> > Sigh.
> > 
> > page_transition_types doesn't really belong in ui/base, but kinda ended up
> there
> > because no one had anywhere else to put it.
> > 
> > I need to bring this up for discussion.
> 
> This is fairly off-topic at this point, but we actually had a *lengthy*
> discussion that resulted in it going into //ui/base (me, sdefresne@, jam@,
> brettw@, beng@). I agree with your sentiment (everyone acknowledged that it
was
> the least-bad of a set of not-great choices), but I'm not sure that reopening
> the discussion will result in a different conclusion.

I hadn't yet gone through chromium-dev@ when I wrote this email :). It's true
that the fact that //content can now depend on //components is a changed factor
in the discussion.

[blundell, 2015-10-22 16:58:49]

On 2015/10/22 06:24:39, blundell wrote:
> On 2015/10/22 06:21:55, blundell wrote:
> > On 2015/10/22 01:08:10, Avi wrote:
> > > Sigh.
> > > 
> > > page_transition_types doesn't really belong in ui/base, but kinda ended up
> > there
> > > because no one had anywhere else to put it.
> > > 
> > > I need to bring this up for discussion.
> > 
> > This is fairly off-topic at this point, but we actually had a *lengthy*
> > discussion that resulted in it going into //ui/base (me, sdefresne@, jam@,
> > brettw@, beng@). I agree with your sentiment (everyone acknowledged that it
> was
> > the least-bad of a set of not-great choices), but I'm not sure that
reopening
> > the discussion will result in a different conclusion.
> 
> I hadn't yet gone through chromium-dev@ when I wrote this email :). It's true
> that the fact that //content can now depend on //components is a changed
factor
> in the discussion.

Avi, is it possible to decouple the discussion on page_transition_type.h from
progress on this CL? AFAIK, the dependency from //components/sync_driver to
//ui/base does not cause any concrete problems in the codebase; it's no
different than any other dependency to //ui/base currently existing that is only
for page_transition_types.h.

[Avi (use Gerrit), 2015-10-22 18:10:03]

On 2015/10/22 16:58:49, blundell wrote:
> On 2015/10/22 06:24:39, blundell wrote:
> > On 2015/10/22 06:21:55, blundell wrote:
> > > On 2015/10/22 01:08:10, Avi wrote:
> > > > Sigh.
> > > > 
> > > > page_transition_types doesn't really belong in ui/base, but kinda ended
up
> > > there
> > > > because no one had anywhere else to put it.
> > > > 
> > > > I need to bring this up for discussion.
> > > 
> > > This is fairly off-topic at this point, but we actually had a *lengthy*
> > > discussion that resulted in it going into //ui/base (me, sdefresne@, jam@,
> > > brettw@, beng@). I agree with your sentiment (everyone acknowledged that
it
> > was
> > > the least-bad of a set of not-great choices), but I'm not sure that
> reopening
> > > the discussion will result in a different conclusion.
> > 
> > I hadn't yet gone through chromium-dev@ when I wrote this email :). It's
true
> > that the fact that //content can now depend on //components is a changed
> factor
> > in the discussion.
> 
> Avi, is it possible to decouple the discussion on page_transition_type.h from
> progress on this CL? AFAIK, the dependency from //components/sync_driver to
> //ui/base does not cause any concrete problems in the codebase; it's no
> different than any other dependency to //ui/base currently existing that is
only
> for page_transition_types.h.

Can you make the DEPS file point to that one exact file only?

[Nicolas Zea, 2015-10-22 18:17:26]

On 2015/10/22 18:10:03, Avi wrote:
> 
> Can you make the DEPS file point to that one exact file only?

Sure, done.

[Avi (use Gerrit), 2015-10-22 21:10:48]

I can approve that.

LGTM

[Nicolas Zea, 2015-10-22 21:21:29]

On 2015/10/22 21:10:48, Avi wrote:
> I can approve that.
> 
> LGTM

Scott, ping on top level build changes (the new sync sessions test support
target dep)?

[sky, 2015-10-22 23:09:20]

On 2015/10/22 21:21:29, Nicolas Zea wrote:
> On 2015/10/22 21:10:48, Avi wrote:
> > I can approve that.
> > 
> > LGTM
> 
> Scott, ping on top level build changes (the new sync sessions test support
> target dep)?

Sorry, could you clarify which files you want me to look at? Thanks!

[Nicolas Zea, 2015-10-22 23:10:40]

On 2015/10/22 23:09:20, sky wrote:
> On 2015/10/22 21:21:29, Nicolas Zea wrote:
> > On 2015/10/22 21:10:48, Avi wrote:
> > > I can approve that.
> > > 
> > > LGTM
> > 
> > Scott, ping on top level build changes (the new sync sessions test support
> > target dep)?
> 
> Sorry, could you clarify which files you want me to look at? Thanks!

Yep, just chrome/browser/DEPS and chrome/test/BUILD.gn, components/BUILD.gn, and
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (for a callsite
change).

[sky, 2015-10-22 23:19:07]

LGTM - I'm not an owner of components/BUILD.gn.

[blundell, 2015-10-23 06:13:04]

lgtm once the issues I've outlined are fixed

https://codereview.chromium.org/1408643002/diff/300001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1408643002/diff/300001/components/BUILD.gn#ne...
components/BUILD.gn:322: "//components/sessions:test_support",
(a) I think you mean to add //components/sync_sessions:test_support
(b) Presumably you need to make a corresponding change in components_tests.gyp?
(c) It's more in the spirit of how this target works to add this dependency on
the individual foo:unit_tests targets that need it (see tho comment at the top
of this deps list). Line 321 looks like a mistake.

[Nicolas Zea, 2015-10-23 22:16:36]

Patchset #15 (id:320001) has been deleted

[Nicolas Zea, 2015-10-23 22:18:15]

https://codereview.chromium.org/1408643002/diff/300001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1408643002/diff/300001/components/BUILD.gn#ne...
components/BUILD.gn:322: "//components/sessions:test_support",
On 2015/10/23 06:13:04, blundell wrote:
> (a) I think you mean to add //components/sync_sessions:test_support
> (b) Presumably you need to make a corresponding change in
components_tests.gyp?
> (c) It's more in the spirit of how this target works to add this dependency on
> the individual foo:unit_tests targets that need it (see tho comment at the top
> of this deps list). Line 321 looks like a mistake.

You're right. Fixed.

[Nicolas Zea, 2015-10-23 22:18:20]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-10-23 22:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
skym@chromium.org, pavely@chromium.org, avi@chromium.org, sky@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1408643002/#ps340001
(title: "Rebase and address Colin's comments")

[commit-bot: I haz the power, 2015-10-23 22:19:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/340001

[commit-bot: I haz the power, 2015-10-23 23:42:48]

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

[commit-bot: I haz the power, 2015-10-23 23:42:49]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nicolas Zea, 2015-10-23 23:50:20]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2015-10-23 23:50:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/340001

[commit-bot: I haz the power, 2015-10-24 02:21:47]

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

[commit-bot: I haz the power, 2015-10-24 02:21:49]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Nicolas Zea, 2015-10-24 04:06:27]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2015-10-24 04:06:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/340001

[commit-bot: I haz the power, 2015-10-24 08:07:43]

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

[commit-bot: I haz the power, 2015-10-24 08:07:44]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[blundell, 2015-10-26 07:07:36]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2015-10-26 07:08:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/340001

[commit-bot: I haz the power, 2015-10-26 09:16:32]

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

[commit-bot: I haz the power, 2015-10-26 09:16:33]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Nicolas Zea, 2015-10-26 18:31:12]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-10-26 18:31:13]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
avi@chromium.org, sky@chromium.org, pavely@chromium.org, skym@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1408643002/#ps360001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-26 18:33:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/360001

[commit-bot: I haz the power, 2015-10-26 19:18:26]

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

[commit-bot: I haz the power, 2015-10-26 19:18:27]

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

[Nicolas Zea, 2015-10-26 19:25:57]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2015-10-26 19:27:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/360001

[commit-bot: I haz the power, 2015-10-26 22:43:13]

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

[commit-bot: I haz the power, 2015-10-26 22:43:14]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Nicolas Zea, 2015-10-27 00:07:47]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-10-27 00:07:47]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
avi@chromium.org, sky@chromium.org, pavely@chromium.org, skym@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1408643002/#ps380001
(title: "Rebase and fix android")

[commit-bot: I haz the power, 2015-10-27 00:09:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/380001

[commit-bot: I haz the power, 2015-10-27 00:39:36]

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

[commit-bot: I haz the power, 2015-10-27 00:39:37]

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

[Nicolas Zea, 2015-10-27 04:37:23]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-10-27 04:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
avi@chromium.org, sky@chromium.org, pavely@chromium.org, skym@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1408643002/#ps400001
(title: "Fix test broken by rebase")

[commit-bot: I haz the power, 2015-10-27 04:37:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408643002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408643002/400001

[commit-bot: I haz the power, 2015-10-27 05:29:59]

Committed patchset #18 (id:400001)

[commit-bot: I haz the power, 2015-10-27 05:30:54]

Patchset 18 (id:??) landed as
https://crrev.com/f09345c6dffa2e7f4a7b3ec3a197861fc7b71d24
Cr-Commit-Position: refs/heads/master@{#356240}

[sdefresne, 2015-10-28 10:45:45]

sdefresne@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2015-10-28 10:45:46]

https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager.h (right):

https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.h:140: static GURL
GetCurrentVirtualURL(const SyncedTabDelegate& tab_delegate);
I think you forgot to remove this method declaration when removing the
implementation.

https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.h:143: static GURL
GetCurrentFaviconURL(const SyncedTabDelegate& tab_delegate);
Same here.

[sdefresne, 2015-10-28 16:36:31]

https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (left):

https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager.cc:407: entry &&
I think the crash in http://crbug.com/548572  is due to this change.

It is possible that entry is null, but SyncedTabDelegate::GetVirtualURLAtIndex()
never checks that the entry returned by GetPossiblyPendingEntryAtIndex() is non
null.

[sky, 2015-10-28 17:17:16]

On 2015/10/28 16:36:31, sdefresne wrote:
>
https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
> File chrome/browser/sync/sessions/sessions_sync_manager.cc (left):
> 
>
https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
> chrome/browser/sync/sessions/sessions_sync_manager.cc:407: entry &&
> I think the crash in http://crbug.com/548572  is due to this change.
> 
> It is possible that entry is null, but
SyncedTabDelegate::GetVirtualURLAtIndex()
> never checks that the entry returned by GetPossiblyPendingEntryAtIndex() is
non
> null.

I can readily hit this crash on windows if I middle click trybot result. For
example, middle clicking the red linux_chromium_rel_ng link on
https://codereview.chromium.org/1414943003/#ps20001 triggers a crash in this
code (crash id d749086e34e2a894).

[Nicolas Zea, 2015-10-28 17:22:34]

On 2015/10/28 17:17:16, sky wrote:
> On 2015/10/28 16:36:31, sdefresne wrote:
> >
>
https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
> > File chrome/browser/sync/sessions/sessions_sync_manager.cc (left):
> > 
> >
>
https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
> > chrome/browser/sync/sessions/sessions_sync_manager.cc:407: entry &&
> > I think the crash in http://crbug.com/548572  is due to this change.
> > 
> > It is possible that entry is null, but
> SyncedTabDelegate::GetVirtualURLAtIndex()
> > never checks that the entry returned by GetPossiblyPendingEntryAtIndex() is
> non
> > null.
> 
> I can readily hit this crash on windows if I middle click trybot result. For
> example, middle clicking the red linux_chromium_rel_ng link on
> https://codereview.chromium.org/1414943003/#ps20001 triggers a crash in this
> code (crash id d749086e34e2a894).

Fix for crash is currently landing via
https://codereview.chromium.org/1422393002/

[sky, 2015-10-28 17:37:10]

Thanks!

On Wed, Oct 28, 2015 at 10:22 AM, <zea@chromium.org> wrote:

> On 2015/10/28 17:17:16, sky wrote:
>
>> On 2015/10/28 16:36:31, sdefresne wrote:
>> >
>>
>
>
>
https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
>
>> > File chrome/browser/sync/sessions/sessions_sync_manager.cc (left):
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1408643002/diff/400001/chrome/browser/sync/se...
>
>> > chrome/browser/sync/sessions/sessions_sync_manager.cc:407: entry &&
>> > I think the crash in http://crbug.com/548572  is due to this change.
>> >
>> > It is possible that entry is null, but
>> SyncedTabDelegate::GetVirtualURLAtIndex()
>> > never checks that the entry returned by
>> GetPossiblyPendingEntryAtIndex() is
>> non
>> > null.
>>
>
> I can readily hit this crash on windows if I middle click trybot result.
>> For
>> example, middle clicking the red linux_chromium_rel_ng link on
>> https://codereview.chromium.org/1414943003/#ps20001 triggers a crash in
>> this
>> code (crash id d749086e34e2a894).
>>
>
> Fix for crash is currently landing via
> https://codereview.chromium.org/1422393002/
>
> https://codereview.chromium.org/1408643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.