Refactor SyncedWindowDelegateAndroid into TabModel

Refactor SyncedWindowDelegateAndroid into TabModel

Pull out a generic TabModel class for Android.  It owns SyncedWindowDelegate
like Browser owns a corresponding instance.  This will allow us to eventually
implement TabModelAndroid.

We no longer need to keep SyncedWindowDelegateRegistry, as we were using that
in non-sync related places to reach the TabModel methods.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148798

[sky, 2012-07-16 23:07:29]

I didn't look at the android or sync changes. I'm assuming one of the other
reviewers is doing that.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1861: return
!TabModelListAndroid::IsOffTheRecordSessionActive();
I would rather see a function in the browser namespace that does this. The
android implementation would call off to TabModelListAndroid, and the
non-android implementation would use BrowserList::IsOffTheRecordSessionActive.
Maybe put these in browser_otr_state.h, browser_otr_state.cc and
browser_otr_state_android.cc

[sky, 2012-07-16 23:17:55]

So, even though I didn't look at the android files I see they all end in
_android. This seems redundant since they are in the android directory. I would
nuke the _android from them.

[sky, 2012-07-17 00:32:12]

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/tab_model/tab_model_android.cc (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.cc:13:
TabModelAndroid::TabModelAndroid()
The order of definitions should match declarations.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.cc:15: this)) { }
{ } -> {} every where.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/tab_model/tab_model_android.h (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:7: #pragma once
no more #pragma once (see style guide).

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:24: class
TabModelAndroid {
Add a description.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:27: virtual
SessionID::id_type GetSessionId() const = 0;
nit: newline between 26/27.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:40: virtual Profile*
profile() const = 0;
virtual methods should not be unix_hacker_style.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:42:
browser_sync::SyncedWindowDelegate* synced_window_delegate() const;
Its generally dangerous to have a const method return a non-const object.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:47: virtual
~TabModelAndroid();
destructor before other methods.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:50:
scoped_ptr<browser_sync::SyncedWindowDelegateAndroid> synced_window_delegate_;
Try to keep your interface pure virtual. Can this be pulled else where?

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:52: #endif  //
CHROME_BROWSER_UI_ANDROID_TAB_MODEL_TAB_MODEL_ANDROID_H_
newline between 52/52.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/tab_model/tab_model_list_android.h (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_list_android.h:16: class
TabModelListAndroid {
Add a description.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_list_android.h:33: private:
nit: newline between 32/33.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_list_android.h:36:
DISALLOW_COPY_AND_ASSIGN(TabModelListAndroid);
Since you don't want anyone creating an instance of this use
DISALLOW_IMPLICIT_CONSTRUCTORS.

[David Trainor- moved to gerrit, 2012-07-18 23:59:04]

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1861: return
!TabModelListAndroid::IsOffTheRecordSessionActive();
On 2012/07/16 23:07:30, sky wrote:
> I would rather see a function in the browser namespace that does this. The
> android implementation would call off to TabModelListAndroid, and the
> non-android implementation would use BrowserList::IsOffTheRecordSessionActive.
> Maybe put these in browser_otr_state.h, browser_otr_state.cc and
> browser_otr_state_android.cc

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/tab_model/tab_model_android.cc (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.cc:13:
TabModelAndroid::TabModelAndroid()
On 2012/07/17 00:32:12, sky wrote:
> The order of definitions should match declarations.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.cc:15: this)) { }
On 2012/07/17 00:32:12, sky wrote:
> { } -> {} every where.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/tab_model/tab_model_android.h (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:7: #pragma once
On 2012/07/17 00:32:12, sky wrote:
> no more #pragma once (see style guide).

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:24: class
TabModelAndroid {
On 2012/07/17 00:32:12, sky wrote:
> Add a description.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:27: virtual
SessionID::id_type GetSessionId() const = 0;
On 2012/07/17 00:32:12, sky wrote:
> nit: newline between 26/27.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:40: virtual Profile*
profile() const = 0;
On 2012/07/17 00:32:12, sky wrote:
> virtual methods should not be unix_hacker_style.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:42:
browser_sync::SyncedWindowDelegate* synced_window_delegate() const;
On 2012/07/17 00:32:12, sky wrote:
> Its generally dangerous to have a const method return a non-const object.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:47: virtual
~TabModelAndroid();
On 2012/07/17 00:32:12, sky wrote:
> destructor before other methods.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:50:
scoped_ptr<browser_sync::SyncedWindowDelegateAndroid> synced_window_delegate_;
On 2012/07/17 00:32:12, sky wrote:
> Try to keep your interface pure virtual. Can this be pulled else where?

I can pull out synced_window_delegate_ and BroadcastAllTabsLoaded.  I'll have to
duplicate the logic if we need two implementations of AndroidTabModel, but if
that's preferable to having the interface not be pure virtual that's fine with
me :).

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_android.h:52: #endif  //
CHROME_BROWSER_UI_ANDROID_TAB_MODEL_TAB_MODEL_ANDROID_H_
On 2012/07/17 00:32:12, sky wrote:
> newline between 52/52.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/tab_model/tab_model_list_android.h (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_list_android.h:16: class
TabModelListAndroid {
On 2012/07/17 00:32:12, sky wrote:
> Add a description.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_list_android.h:33: private:
On 2012/07/17 00:32:12, sky wrote:
> nit: newline between 32/33.

Done.

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_model/tab_model_list_android.h:36:
DISALLOW_COPY_AND_ASSIGN(TabModelListAndroid);
On 2012/07/17 00:32:12, sky wrote:
> Since you don't want anyone creating an instance of this use
> DISALLOW_IMPLICIT_CONSTRUCTORS.

Done.

[jar (doing other things), 2012-07-19 00:11:34]

Please upload changes.
thx!

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10701030/diff/7001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1861: return
!TabModelListAndroid::IsOffTheRecordSessionActive();
I see the comment... and your "done," but don't see the change.

On 2012/07/18 23:59:04, dtrainor wrote:
> On 2012/07/16 23:07:30, sky wrote:
> > I would rather see a function in the browser namespace that does this. The
> > android implementation would call off to TabModelListAndroid, and the
> > non-android implementation would use
BrowserList::IsOffTheRecordSessionActive.
> > Maybe put these in browser_otr_state.h, browser_otr_state.cc and
> > browser_otr_state_android.cc
> 
> Done.

[David Trainor- moved to gerrit, 2012-07-19 16:50:27]

Keep getting side tracked, will be up sometime soon today.

[David Trainor- moved to gerrit, 2012-07-23 20:46:03]

ping :)

[jar (doing other things), 2012-07-23 20:49:31]

You should clarify who you are pinging for what part of the review.

You have a bunch of reviewers listed.

Thanks,

Jim

[David Trainor- moved to gerrit, 2012-07-23 20:52:37]

On 2012/07/23 20:49:31, jar wrote:
> You should clarify who you are pinging for what part of the review.
> 
> You have a bunch of reviewers listed.
> 
> Thanks,
> 
> Jim

Ah sorry!  Thanks for letting me know :).
-Scott, addressed your comments.
-Tommy, I think Yaron wanted you to fill in for him, can you give this a quick
look over to make sure the final version is okay with sync?

[sky, 2012-07-23 22:59:23]

Additionally your last comment indicated you hadn't updated the patch.
Is it up to date?

  -Scott

On Mon, Jul 23, 2012 at 1:52 PM,  <dtrainor@chromium.org> wrote:
> On 2012/07/23 20:49:31, jar wrote:
>>
>> You should clarify who you are pinging for what part of the review.
>
>
>> You have a bunch of reviewers listed.
>
>
>> Thanks,
>
>
>> Jim
>
>
> Ah sorry!  Thanks for letting me know :).
> -Scott, addressed your comments.
> -Tommy, I think Yaron wanted you to fill in for him, can you give this a
> quick
> look over to make sure the final version is okay with sync?
>
> https://chromiumcodereview.appspot.com/10701030/

[David Trainor- moved to gerrit, 2012-07-23 23:03:56]

Yeah I uploaded patch set 3, which should have all of the requested fixes.  I'm
new to the Chromium code review tool/process my bad.

On 2012/07/23 22:59:23, sky wrote:
> Additionally your last comment indicated you hadn't updated the patch.
> Is it up to date?
> 
>   -Scott
> 
> On Mon, Jul 23, 2012 at 1:52 PM,  <mailto:dtrainor@chromium.org> wrote:
> > On 2012/07/23 20:49:31, jar wrote:
> >>
> >> You should clarify who you are pinging for what part of the review.
> >
> >
> >> You have a bunch of reviewers listed.
> >
> >
> >> Thanks,
> >
> >
> >> Jim
> >
> >
> > Ah sorry!  Thanks for letting me know :).
> > -Scott, addressed your comments.
> > -Tommy, I think Yaron wanted you to fill in for him, can you give this a
> > quick
> > look over to make sure the final version is okay with sync?
> >
> > https://chromiumcodereview.appspot.com/10701030/

[David Trainor- moved to gerrit, 2012-07-23 23:05:57]

Aah, I assumed uploading a new patch would automatically notify everyone on the
list.  That explains some of the confusion.

On 2012/07/23 23:03:56, dtrainor wrote:
> Yeah I uploaded patch set 3, which should have all of the requested fixes. 
I'm
> new to the Chromium code review tool/process my bad.
> 
> On 2012/07/23 22:59:23, sky wrote:
> > Additionally your last comment indicated you hadn't updated the patch.
> > Is it up to date?
> > 
> >   -Scott
> > 
> > On Mon, Jul 23, 2012 at 1:52 PM,  <mailto:dtrainor@chromium.org> wrote:
> > > On 2012/07/23 20:49:31, jar wrote:
> > >>
> > >> You should clarify who you are pinging for what part of the review.
> > >
> > >
> > >> You have a bunch of reviewers listed.
> > >
> > >
> > >> Thanks,
> > >
> > >
> > >> Jim
> > >
> > >
> > > Ah sorry!  Thanks for letting me know :).
> > > -Scott, addressed your comments.
> > > -Tommy, I think Yaron wanted you to fill in for him, can you give this a
> > > quick
> > > look over to make sure the final version is okay with sync?
> > >
> > > https://chromiumcodereview.appspot.com/10701030/

[sky, 2012-07-24 04:03:57]

I didn't look at TabModel/TabModelList. I'm assuming your other reviews are
doing that.

https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/android/browser_otr_state_android.cc (right):

https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
chrome/browser/ui/android/browser_otr_state_android.cc:5: #include
"chrome/browser/ui/browser_otr_state.h"
We generally have implementation specific files live side-by-side with
non-implementation specific. See session_restore_android.cc in this patch. Any
reason you're not doing that here?

[David Trainor- moved to gerrit, 2012-07-24 05:31:10]

I saw most of the android implementations for this directory were under the
android folder (which I thought was odd, but I wanted to stay somewhat
consistent).  I'll pull the file out though.

On 2012/07/24 04:03:57, sky wrote:
> I didn't look at TabModel/TabModelList. I'm assuming your other reviews are
> doing that.
> 
>
https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
> File chrome/browser/ui/android/browser_otr_state_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
> chrome/browser/ui/android/browser_otr_state_android.cc:5: #include
> "chrome/browser/ui/browser_otr_state.h"
> We generally have implementation specific files live side-by-side with
> non-implementation specific. See session_restore_android.cc in this patch. Any
> reason you're not doing that here?

[nyquist, 2012-07-24 18:32:34]

http://codereview.chromium.org/10701030/diff/17001/chrome/browser/ui/android/...
File chrome/browser/ui/android/tab_model/tab_model.h (right):

http://codereview.chromium.org/10701030/diff/17001/chrome/browser/ui/android/...
chrome/browser/ui/android/tab_model/tab_model.h:38: // Returns true when all
tabs have been restored from disk.
This comment seems wrong. I believe it should be something like:
// Return true if we are currently restoring sessions asynchronously.

[David Trainor- moved to gerrit, 2012-07-24 20:48:52]

Fixed comments, moved browser_otr_state_android out of android folder.  Rebased
for git try.

https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/android/browser_otr_state_android.cc (right):

https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
chrome/browser/ui/android/browser_otr_state_android.cc:5: #include
"chrome/browser/ui/browser_otr_state.h"
On 2012/07/24 04:03:57, sky wrote:
> We generally have implementation specific files live side-by-side with
> non-implementation specific. See session_restore_android.cc in this patch. Any
> reason you're not doing that here?

Done.

https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/android/tab_model/tab_model.h (right):

https://chromiumcodereview.appspot.com/10701030/diff/17001/chrome/browser/ui/...
chrome/browser/ui/android/tab_model/tab_model.h:38: // Returns true when all
tabs have been restored from disk.
On 2012/07/24 18:32:34, nyquist wrote:
> This comment seems wrong. I believe it should be something like:
> // Return true if we are currently restoring sessions asynchronously.

Done.

[sky, 2012-07-24 21:41:00]

LGTM

[commit-bot: I haz the power, 2012-07-25 16:48:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/10701030/23004

[commit-bot: I haz the power, 2012-07-25 16:48:25]

Presubmit check for 10701030-23004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/sync

Presubmit checks took 1.5s to calculate.

[David Trainor- moved to gerrit, 2012-07-25 17:09:40]

Hey akalin, can you ptal?  I didn't add someone from chrome/browser/sync on this
change.  I've been working with nyquist and yfriedman on this change downstream,
but should have put one of you guys on this for upstream.  Thanks!

[David Trainor- moved to gerrit, 2012-07-26 20:39:54]

Hi Nicolas.  Can you please take a look at this?  Been working with Yaron and
Tommy but need someone from the sync OWNERS file to approve the sync components.
 Thanks!

[akalin, 2012-07-27 17:17:27]

LGTM

http://codereview.chromium.org/10701030/diff/23004/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_window_delegate_android.cc (left):

http://codereview.chromium.org/10701030/diff/23004/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_window_delegate_android.cc:5: #include
"chrome/browser/sync/glue/synced_window_delegate.h"
no need for this include (just delegate_android.h should suffice)

http://codereview.chromium.org/10701030/diff/23004/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_window_delegate_android.h (right):

http://codereview.chromium.org/10701030/diff/23004/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_window_delegate_android.h:23: virtual bool
HasWindow() const OVERRIDE;
Add:

 // browser_sync::SyncedWindowDelegate implementation.

above HasWindow()

[David Trainor- moved to gerrit, 2012-07-27 17:53:13]

https://chromiumcodereview.appspot.com/10701030/diff/23004/chrome/browser/syn...
File chrome/browser/sync/glue/synced_window_delegate_android.cc (left):

https://chromiumcodereview.appspot.com/10701030/diff/23004/chrome/browser/syn...
chrome/browser/sync/glue/synced_window_delegate_android.cc:5: #include
"chrome/browser/sync/glue/synced_window_delegate.h"
On 2012/07/27 17:17:28, akalin wrote:
> no need for this include (just delegate_android.h should suffice)

Done.

https://chromiumcodereview.appspot.com/10701030/diff/23004/chrome/browser/syn...
File chrome/browser/sync/glue/synced_window_delegate_android.h (right):

https://chromiumcodereview.appspot.com/10701030/diff/23004/chrome/browser/syn...
chrome/browser/sync/glue/synced_window_delegate_android.h:23: virtual bool
HasWindow() const OVERRIDE;
On 2012/07/27 17:17:28, akalin wrote:
> Add:
> 
>  // browser_sync::SyncedWindowDelegate implementation.
> 
> above HasWindow()

Done.

[commit-bot: I haz the power, 2012-07-27 17:57:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/10701030/34001

[commit-bot: I haz the power, 2012-07-27 20:46:20]

Change committed as 148798