[Sync] Remove static methods on SyncedWindowDelegate.

[Sync] Remove static methods on SyncedWindowDelegate.

Before, two static methods (GetAll and FindByID) were declared
on SyncedWindowDelegate but defined in platform specific files.
Now, those methods exist on SyncedWindowDelegatesGetter so
they can use polymorphism for platform specificity.

SyncedTabDelegate was the biggest challenge since it is not
really accessible when constructed to pass the getter in.
Instead, the SyncedWindowDelegatesGetter is set in
SyncedTabDelegate::ImplFromWebContents, before anything
else uses the object as a SyncedTabDelegate.

This CL is a prerequisite to componentizing SyncedWindowDelegate
and SyncedWindowDelegatesGetter.

BUG=512472
Committed: https://crrev.com/a3b1e30c365c1a8c9ca659943349aedd603ef5f4
Cr-Commit-Position: refs/heads/master@{#347840}

[maxbogue, 2015-08-31 19:40:08]

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

[maxbogue, 2015-08-31 19:40:09]

Hey Nicolas, could you review this change?

[Nicolas Zea, 2015-08-31 22:12:48]

Nice job! Mostly nits

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/synced_tab_delegate.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_tab_delegate.h:29: SyncedTabDelegate();
nit: I think it would be better to make this constructor protected to make it
clear that embedders should not be instantiating a SyncedTabDelegate directly,
but instead a derived class.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_tab_delegate.h:75: void
SetSyncedWindowGetter(scoped_ptr<SyncedWindowDelegatesGetter> getter);
Add comment? Would probably be good to also explain where/when this is supposed
to be called.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_tab_delegate.h:85:
scoped_ptr<SyncedWindowDelegatesGetter> synced_window_getter_;
make private?

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/synced_window_delegates_getter_android.cc (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.cc:18: for
(TabModelList::const_iterator i = TabModelList::begin();
nit: use C++11 auto iterators. Something like "const auto& i =
TabModelList::begin()"

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/synced_window_delegates_getter_android.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.h:17: class
SyncedWindowDelegatesGetterAndroid : public SyncedWindowDelegatesGetter {
Add comment describing purpose of class.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.h:19:
SyncedWindowDelegatesGetterAndroid();
Needs a virtual destructor.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.h:20:
std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() override;
nit: newline above. Also, for consistency/clarity sake, add
"SyncedWindowDelegatesGetter implementation"

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:240: return
scoped_ptr<SyncedWindowDelegatesGetter>(
I think you can just return the pointer here? Alternatively, use make_scoped_ptr

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/synced_window_delegates_getter.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/synced_window_delegates_getter.h:21: virtual
std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() = 0;
nit: although it didn't have them before, would be good to add comments here
explaining what these two methods are for (and to add a comment to the class
explaining what it's purpose is)

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/browser_synced_window_delegates_getter.cc (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.cc:28: return
(browser != NULL) ? browser->synced_window_delegate() : NULL;
nit: use nullptr (here and elsewhere in the CL).
(browser != nullptr ? browser->synced_window_delegate() : nullptr)

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/browser_synced_window_delegates_getter.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.h:18: class
BrowserSyncedWindowDelegatesGetter : public SyncedWindowDelegatesGetter {
Comment about class purpose

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.h:20:
BrowserSyncedWindowDelegatesGetter();
explicit virtual destructor

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.h:21:
std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() override;
newline above and comment that this is implementing SyncedWindowDelegatesGetter

[maxbogue, 2015-09-01 17:29:28]

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/synced_tab_delegate.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_tab_delegate.h:29: SyncedTabDelegate();
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> nit: I think it would be better to make this constructor protected to make it
> clear that embedders should not be instantiating a SyncedTabDelegate directly,
> but instead a derived class.

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_tab_delegate.h:75: void
SetSyncedWindowGetter(scoped_ptr<SyncedWindowDelegatesGetter> getter);
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> Add comment? Would probably be good to also explain where/when this is
supposed
> to be called.

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_tab_delegate.h:85:
scoped_ptr<SyncedWindowDelegatesGetter> synced_window_getter_;
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> make private?

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/synced_window_delegates_getter_android.cc (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.cc:18: for
(TabModelList::const_iterator i = TabModelList::begin();
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> nit: use C++11 auto iterators. Something like "const auto& i =
> TabModelList::begin()"

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/synced_window_delegates_getter_android.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.h:17: class
SyncedWindowDelegatesGetterAndroid : public SyncedWindowDelegatesGetter {
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> Add comment describing purpose of class.

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.h:19:
SyncedWindowDelegatesGetterAndroid();
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> Needs a virtual destructor.

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/synced_window_delegates_getter_android.h:20:
std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() override;
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> nit: newline above. Also, for consistency/clarity sake, add
> "SyncedWindowDelegatesGetter implementation"

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:240: return
scoped_ptr<SyncedWindowDelegatesGetter>(
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> I think you can just return the pointer here? Alternatively, use
make_scoped_ptr

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/synced_window_delegates_getter.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/synced_window_delegates_getter.h:21: virtual
std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() = 0;
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> nit: although it didn't have them before, would be good to add comments here
> explaining what these two methods are for (and to add a comment to the class
> explaining what it's purpose is)

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/browser_synced_window_delegates_getter.cc (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.cc:28: return
(browser != NULL) ? browser->synced_window_delegate() : NULL;
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> nit: use nullptr (here and elsewhere in the CL).
> (browser != nullptr ? browser->synced_window_delegate() : nullptr)

I changed most, but not ones in files where NULL is already prevalent.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/browser_synced_window_delegates_getter.h (right):

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.h:18: class
BrowserSyncedWindowDelegatesGetter : public SyncedWindowDelegatesGetter {
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> Comment about class purpose

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.h:20:
BrowserSyncedWindowDelegatesGetter();
On 2015/08/31 22:12:47, Nicolas Zea wrote:
> explicit virtual destructor

Done.

https://codereview.chromium.org/1310373009/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/browser_synced_window_delegates_getter.h:21:
std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() override;
On 2015/08/31 22:12:48, Nicolas Zea wrote:
> newline above and comment that this is implementing
SyncedWindowDelegatesGetter

Done.

[Nicolas Zea, 2015-09-02 21:19:44]

LGTM with a couple nits! Great to see this getting cleaned up!

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_tab_delegate.cc (right):

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate.cc:71: NOTREACHED();
Generally you shouldn't NOTREACH for a case that you're handling. In other
words, if this is invalid, it's better to let it crash below (so we can identify
that it's actually happening). If it's not invalid, then remove the NOTREACHED.

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_tab_delegate_android.cc (right):

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:123: return NULL;
nit: style guide recommends nullptr for all new code, so even if you don't clean
up the rest of the file, I'd say use it here.

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:125: SyncedTabDelegate*
delegate = tab->GetSyncedTabDelegate();
Can delegate be null? Should that be handled?

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

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:119: return NULL;
nit: nullptr here too

[maxbogue, 2015-09-02 23:05:40]

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_tab_delegate.cc (right):

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate.cc:71: NOTREACHED();
On 2015/09/02 21:19:43, Nicolas Zea wrote:
> Generally you shouldn't NOTREACH for a case that you're handling. In other
> words, if this is invalid, it's better to let it crash below (so we can
identify
> that it's actually happening). If it's not invalid, then remove the
NOTREACHED.

Ah, ok. It is invalid, so I've removed the return.

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/synced_tab_delegate_android.cc (right):

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:123: return NULL;
On 2015/09/02 21:19:44, Nicolas Zea wrote:
> nit: style guide recommends nullptr for all new code, so even if you don't
clean
> up the rest of the file, I'd say use it here.

Done.

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/gl...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:125: SyncedTabDelegate*
delegate = tab->GetSyncedTabDelegate();
On 2015/09/02 21:19:43, Nicolas Zea wrote:
> Can delegate be null? Should that be handled?

Yes, thank you. I forgot there were two versions of this function.

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

https://codereview.chromium.org/1310373009/diff/100001/chrome/browser/sync/se...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:119: return NULL;
On 2015/09/02 21:19:44, Nicolas Zea wrote:
> nit: nullptr here too

Done.

[maxbogue, 2015-09-03 00:24:18]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-09-03 00:24:19]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1310373009/#ps120001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-09-03 00:31:29]

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

[commit-bot: I haz the power, 2015-09-03 00:50:00]

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

[commit-bot: I haz the power, 2015-09-03 00:50:00]

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

[maxbogue, 2015-09-03 01:11:21]

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

[maxbogue, 2015-09-03 01:11:22]

+pkasting for: 
chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

[Peter Kasting, 2015-09-03 05:03:59]

LGTM.  That's sure an awkward classname.

[maxbogue, 2015-09-03 16:50:15]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2015-09-03 16:50:32]

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

[maxbogue, 2015-09-03 17:08:31]

On 2015/09/03 05:03:59, Peter Kasting wrote:
> LGTM.  That's sure an awkward classname.

Sure is! I'm open to suggestions for improving it in a followup if you have any.

[commit-bot: I haz the power, 2015-09-03 19:58:55]

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

[commit-bot: I haz the power, 2015-09-03 19:58:56]

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...)

[maxbogue, 2015-09-03 20:01:33]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2015-09-03 20:03:20]

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

[commit-bot: I haz the power, 2015-09-03 23:52:44]

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

[commit-bot: I haz the power, 2015-09-03 23:52: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...)

[maxbogue, 2015-09-08 21:21:52]

Nicolas, please take a look at the new changes to
synced_tab_delegate_android.cc. It wraps a TabContentsSyncedTabDelegate, so I
had to set the window getter on that as well.

[Nicolas Zea, 2015-09-08 23:55:31]

LGTM

[maxbogue, 2015-09-08 23:59:38]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-09-08 23:59:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1310373009/#ps160001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-09-08 23:59:59]

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

[commit-bot: I haz the power, 2015-09-09 00:07:33]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-09 00:08:25]

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