Split TabRestoreService into InMemoryTRS and PersistentTRS

Split TabRestoreService into InMemoryTRS and PersistentTRS

This is a refactoring needed as part of Chrome for Android upstreaming.

This refactors TabRestoreService by making it an interface and splitting the 
initial TabRestoreService class into two classes implementing that interface:

- InMemoryTabRestoreService: A tab restore service that doesn't persist its 
  state to disk. This is used on Android where tab persistence is implemented
  on the application side in Java.

- PersistentTabRestoreService: This is equivalent in terms of
  features/behaviors to the initial TabRestoreService. All non-Android clients
  should (indirectly) use this. Note that clients should still use 
  TabRestoreServiceFactory to get a TabRestoreService instance.

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

[sky, 2012-09-26 20:54:21]

https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
File chrome/browser/sessions/session_backend_android.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
chrome/browser/sessions/session_backend_android.cc:10: // Dummy session backend
for Android. Tab and session persistence is handled
If tab and session persistence is handled else where, why is the sessions code
even compiled?

[Philippe, 2012-09-26 21:28:09]

https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
File chrome/browser/sessions/session_backend_android.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
chrome/browser/sessions/session_backend_android.cc:10: // Dummy session backend
for Android. Tab and session persistence is handled
On 2012/09/26 20:54:21, sky wrote:
> If tab and session persistence is handled else where, why is the sessions code
> even compiled?

That's a good question. We do use TabRestoreService::CreateHistoricalTab()
internally on the app side. Although we use here a dummy backend I suspect that
we rely on TabRestoreService's observers notification in some way which would be
the only no-op on Android (see NotifyTabsChanged() in
TabRestoreService::AddEntry()).
Unfortunately I'm not really familiar with this code. Do you  know who is
subscribing to these notifications and what would be the impact of not receiving
them?
I didn't see any Android-specific code subscribing to these notifications and
have really no idea why we do call TabRestoreService::CreateHistoricalTab(). You
might have an idea.
It's hard to track the file history internally on the app side with all the
merges and moves.

[Philippe, 2012-09-26 21:38:10]

https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
File chrome/browser/sessions/session_backend_android.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
chrome/browser/sessions/session_backend_android.cc:10: // Dummy session backend
for Android. Tab and session persistence is handled
On 2012/09/26 21:28:09, Philippe wrote:
> On 2012/09/26 20:54:21, sky wrote:
> > If tab and session persistence is handled else where, why is the sessions
code
> > even compiled?
> 
> That's a good question. We do use TabRestoreService::CreateHistoricalTab()
> internally on the app side. Although we use here a dummy backend I suspect
that
> we rely on TabRestoreService's observers notification in some way which would
be
> the only no-op on Android (see NotifyTabsChanged() in
> TabRestoreService::AddEntry()).
> Unfortunately I'm not really familiar with this code. Do you  know who is
> subscribing to these notifications and what would be the impact of not
receiving
> them?
> I didn't see any Android-specific code subscribing to these notifications and
> have really no idea why we do call TabRestoreService::CreateHistoricalTab().
You
> might have an idea.
> It's hard to track the file history internally on the app side with all the
> merges and moves. 

s/no-op/op. I meant notifications seem to be the only operation we do on Android
since we are using this dummy backend which doesn't do any actual read/write.

[sky, 2012-09-26 23:00:59]

On Wed, Sep 26, 2012 at 2:28 PM,  <pliard@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> File chrome/browser/sessions/session_backend_android.cc (right):
>
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> chrome/browser/sessions/session_backend_android.cc:10: // Dummy session
> backend for Android. Tab and session persistence is handled
> On 2012/09/26 20:54:21, sky wrote:
>>
>> If tab and session persistence is handled else where, why is the
>
> sessions code
>>
>> even compiled?
>
>
> That's a good question. We do use
> TabRestoreService::CreateHistoricalTab() internally on the app side.

If that's all you need it should be factored into a standalone function.

> Although we use here a dummy backend I suspect that we rely on
> TabRestoreService's observers notification in some way which would be
> the only no-op on Android (see NotifyTabsChanged() in
> TabRestoreService::AddEntry()).
> Unfortunately I'm not really familiar with this code. Do you  know who
> is subscribing to these notifications and what would be the impact of
> not receiving them?

I don't know who relies on these for android code. But presumably you
only care about TabRestoreServiceChanged, which would only happen if
you're modifying TabRestoreService. So, if you're not using
TabRestoreService then it should never be modified and
TabRestoreServiceChanged never sent, right?

  -Scott

> I didn't see any Android-specific code subscribing to these
> notifications and have really no idea why we do call
> TabRestoreService::CreateHistoricalTab(). You might have an idea.
> It's hard to track the file history internally on the app side with all
> the merges and moves.
>
> https://chromiumcodereview.appspot.com/10989027/

[Philippe, 2012-09-26 23:29:35]

On 2012/09/26 23:00:59, sky wrote:
> On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > File chrome/browser/sessions/session_backend_android.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > chrome/browser/sessions/session_backend_android.cc:10: // Dummy session
> > backend for Android. Tab and session persistence is handled
> > On 2012/09/26 20:54:21, sky wrote:
> >>
> >> If tab and session persistence is handled else where, why is the
> >
> > sessions code
> >>
> >> even compiled?
> >
> >
> > That's a good question. We do use
> > TabRestoreService::CreateHistoricalTab() internally on the app side.
> 
> If that's all you need it should be factored into a standalone function.
> 
> > Although we use here a dummy backend I suspect that we rely on
> > TabRestoreService's observers notification in some way which would be
> > the only no-op on Android (see NotifyTabsChanged() in
> > TabRestoreService::AddEntry()).
> > Unfortunately I'm not really familiar with this code. Do you  know who
> > is subscribing to these notifications and what would be the impact of
> > not receiving them?
> 
> I don't know who relies on these for android code. But presumably you
> only care about TabRestoreServiceChanged, which would only happen if
> you're modifying TabRestoreService. So, if you're not using
> TabRestoreService then it should never be modified and
> TabRestoreServiceChanged never sent, right?
> 
>   -Scott
> 
> > I didn't see any Android-specific code subscribing to these
> > notifications and have really no idea why we do call
> > TabRestoreService::CreateHistoricalTab(). You might have an idea.
> > It's hard to track the file history internally on the app side with all
> > the merges and moves.
> >
> > https://chromiumcodereview.appspot.com/10989027/

My understanding is that we do modify TabRestoreService's in-memory state by
calling CreateHistoricalTab(). Therefore TabRestoreService's clients might be
interested in receiving notifications. The only thing we don't do in
TabRestoreService is the disk persistence. I'm wondering by the way why we don't
have a non-dummy session backend implementation on the Chromium side forwarding
calls to Java to implement the actual persistence.

I'm adding Yaron who probably knows more about this. Yaron do you know why we
are calling TabRestoreService::CreateHistoricalTab() on the app side given that
we handle tab and session persistence on the Java side? Is that only to notify
observers (which don't seem to include Android-specific classes)? And also do
you know why we don't implement (at least a part of) tab/session persistence on
the Chromium side? I guess that this heavily depends on classes implemented on
the application side.

[Philippe, 2012-09-27 23:51:35]

On 2012/09/26 23:29:35, Philippe wrote:
> On 2012/09/26 23:00:59, sky wrote:
> > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org> wrote:
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > > File chrome/browser/sessions/session_backend_android.cc (right):
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > > chrome/browser/sessions/session_backend_android.cc:10: // Dummy session
> > > backend for Android. Tab and session persistence is handled
> > > On 2012/09/26 20:54:21, sky wrote:
> > >>
> > >> If tab and session persistence is handled else where, why is the
> > >
> > > sessions code
> > >>
> > >> even compiled?
> > >
> > >
> > > That's a good question. We do use
> > > TabRestoreService::CreateHistoricalTab() internally on the app side.
> > 
> > If that's all you need it should be factored into a standalone function.
> > 
> > > Although we use here a dummy backend I suspect that we rely on
> > > TabRestoreService's observers notification in some way which would be
> > > the only no-op on Android (see NotifyTabsChanged() in
> > > TabRestoreService::AddEntry()).
> > > Unfortunately I'm not really familiar with this code. Do you  know who
> > > is subscribing to these notifications and what would be the impact of
> > > not receiving them?
> > 
> > I don't know who relies on these for android code. But presumably you
> > only care about TabRestoreServiceChanged, which would only happen if
> > you're modifying TabRestoreService. So, if you're not using
> > TabRestoreService then it should never be modified and
> > TabRestoreServiceChanged never sent, right?
> > 
> >   -Scott
> > 
> > > I didn't see any Android-specific code subscribing to these
> > > notifications and have really no idea why we do call
> > > TabRestoreService::CreateHistoricalTab(). You might have an idea.
> > > It's hard to track the file history internally on the app side with all
> > > the merges and moves.
> > >
> > > https://chromiumcodereview.appspot.com/10989027/
> 
> My understanding is that we do modify TabRestoreService's in-memory state by
> calling CreateHistoricalTab(). Therefore TabRestoreService's clients might be
> interested in receiving notifications. The only thing we don't do in
> TabRestoreService is the disk persistence. I'm wondering by the way why we
don't
> have a non-dummy session backend implementation on the Chromium side
forwarding
> calls to Java to implement the actual persistence.
> 
> I'm adding Yaron who probably knows more about this. Yaron do you know why we
> are calling TabRestoreService::CreateHistoricalTab() on the app side given
that
> we handle tab and session persistence on the Java side? Is that only to notify
> observers (which don't seem to include Android-specific classes)? And also do
> you know why we don't implement (at least a part of) tab/session persistence
on
> the Chromium side? I guess that this heavily depends on classes implemented on
> the application side.

I had a quick chat with Yaron and this will need a little more work. I will see
how I can get rid of the call to TabRestoreService::CreateHistoricalTab() on the
application side so that we can totally compile it out.

I will give you an update once I have something working.

[Philippe, 2012-09-28 18:14:56]

On 2012/09/27 23:51:35, Philippe wrote:
> On 2012/09/26 23:29:35, Philippe wrote:
> > On 2012/09/26 23:00:59, sky wrote:
> > > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org> wrote:
> > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > > > File chrome/browser/sessions/session_backend_android.cc (right):
> > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > > > chrome/browser/sessions/session_backend_android.cc:10: // Dummy session
> > > > backend for Android. Tab and session persistence is handled
> > > > On 2012/09/26 20:54:21, sky wrote:
> > > >>
> > > >> If tab and session persistence is handled else where, why is the
> > > >
> > > > sessions code
> > > >>
> > > >> even compiled?
> > > >
> > > >
> > > > That's a good question. We do use
> > > > TabRestoreService::CreateHistoricalTab() internally on the app side.
> > > 
> > > If that's all you need it should be factored into a standalone function.
> > > 
> > > > Although we use here a dummy backend I suspect that we rely on
> > > > TabRestoreService's observers notification in some way which would be
> > > > the only no-op on Android (see NotifyTabsChanged() in
> > > > TabRestoreService::AddEntry()).
> > > > Unfortunately I'm not really familiar with this code. Do you  know who
> > > > is subscribing to these notifications and what would be the impact of
> > > > not receiving them?
> > > 
> > > I don't know who relies on these for android code. But presumably you
> > > only care about TabRestoreServiceChanged, which would only happen if
> > > you're modifying TabRestoreService. So, if you're not using
> > > TabRestoreService then it should never be modified and
> > > TabRestoreServiceChanged never sent, right?
> > > 
> > >   -Scott
> > > 
> > > > I didn't see any Android-specific code subscribing to these
> > > > notifications and have really no idea why we do call
> > > > TabRestoreService::CreateHistoricalTab(). You might have an idea.
> > > > It's hard to track the file history internally on the app side with all
> > > > the merges and moves.
> > > >
> > > > https://chromiumcodereview.appspot.com/10989027/
> > 
> > My understanding is that we do modify TabRestoreService's in-memory state by
> > calling CreateHistoricalTab(). Therefore TabRestoreService's clients might
be
> > interested in receiving notifications. The only thing we don't do in
> > TabRestoreService is the disk persistence. I'm wondering by the way why we
> don't
> > have a non-dummy session backend implementation on the Chromium side
> forwarding
> > calls to Java to implement the actual persistence.
> > 
> > I'm adding Yaron who probably knows more about this. Yaron do you know why
we
> > are calling TabRestoreService::CreateHistoricalTab() on the app side given
> that
> > we handle tab and session persistence on the Java side? Is that only to
notify
> > observers (which don't seem to include Android-specific classes)? And also
do
> > you know why we don't implement (at least a part of) tab/session persistence
> on
> > the Chromium side? I guess that this heavily depends on classes implemented
on
> > the application side.
> 
> I had a quick chat with Yaron and this will need a little more work. I will
see
> how I can get rid of the call to TabRestoreService::CreateHistoricalTab() on
the
> application side so that we can totally compile it out.
> 
> I will give you an update once I have something working.

I think we can hardly get rid of TabRestoreService. It's tightly coupled to the
RecentlyClosedTabsHandler class which is used to display the list of recently
closed tabs on the welcome page (and which is one of the notifications
subscribers).
I might be able to abstract TabRestoreService and have a specific Android
implementation for it. I'm not sure it is the way to go though.
This would add a significant amount of code and abstractions without adding much
value.

One possible way of doing would be not to have the Android dummy session backend
and set the session backend instance to NULL instead on Android. The downside is
that we would have to check that the pointer is not NULL every time we access
the backend.
More generally we could also support an in-memory mode in the TabRestoreService
as we use in Android.

What do you think?

[sky, 2012-09-28 20:26:51]

I think it makes more sense to introduce an interface that
RecentlyClosedTabsHandler talks to. TabRestoreService would implement
this as would the android side.

  -Scott

On Fri, Sep 28, 2012 at 11:14 AM,  <pliard@chromium.org> wrote:
> On 2012/09/27 23:51:35, Philippe wrote:
>>
>> On 2012/09/26 23:29:35, Philippe wrote:
>> > On 2012/09/26 23:00:59, sky wrote:
>> > > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org> wrote:
>> > > >
>> > > >
>> > >
>> >
>
>
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
>>
>> > > > File chrome/browser/sessions/session_backend_android.cc (right):
>> > > >
>> > > >
>> > >
>> >
>
>
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
>>
>> > > > chrome/browser/sessions/session_backend_android.cc:10: // Dummy
>> > > > session
>> > > > backend for Android. Tab and session persistence is handled
>> > > > On 2012/09/26 20:54:21, sky wrote:
>> > > >>
>> > > >> If tab and session persistence is handled else where, why is the
>> > > >
>> > > > sessions code
>> > > >>
>> > > >> even compiled?
>> > > >
>> > > >
>> > > > That's a good question. We do use
>> > > > TabRestoreService::CreateHistoricalTab() internally on the app side.
>> > >
>> > > If that's all you need it should be factored into a standalone
>> > > function.
>> > >
>> > > > Although we use here a dummy backend I suspect that we rely on
>> > > > TabRestoreService's observers notification in some way which would
>> > > > be
>> > > > the only no-op on Android (see NotifyTabsChanged() in
>> > > > TabRestoreService::AddEntry()).
>> > > > Unfortunately I'm not really familiar with this code. Do you  know
>> > > > who
>> > > > is subscribing to these notifications and what would be the impact
>> > > > of
>> > > > not receiving them?
>> > >
>> > > I don't know who relies on these for android code. But presumably you
>> > > only care about TabRestoreServiceChanged, which would only happen if
>> > > you're modifying TabRestoreService. So, if you're not using
>> > > TabRestoreService then it should never be modified and
>> > > TabRestoreServiceChanged never sent, right?
>> > >
>> > >   -Scott
>> > >
>> > > > I didn't see any Android-specific code subscribing to these
>> > > > notifications and have really no idea why we do call
>> > > > TabRestoreService::CreateHistoricalTab(). You might have an idea.
>> > > > It's hard to track the file history internally on the app side with
>> > > > all
>> > > > the merges and moves.
>> > > >
>> > > > https://chromiumcodereview.appspot.com/10989027/
>> >
>> > My understanding is that we do modify TabRestoreService's in-memory
>> > state by
>> > calling CreateHistoricalTab(). Therefore TabRestoreService's clients
>> > might
>
> be
>>
>> > interested in receiving notifications. The only thing we don't do in
>> > TabRestoreService is the disk persistence. I'm wondering by the way why
>> > we
>> don't
>> > have a non-dummy session backend implementation on the Chromium side
>> forwarding
>> > calls to Java to implement the actual persistence.
>> >
>> > I'm adding Yaron who probably knows more about this. Yaron do you know
>> > why
>
> we
>>
>> > are calling TabRestoreService::CreateHistoricalTab() on the app side
>> > given
>> that
>> > we handle tab and session persistence on the Java side? Is that only to
>
> notify
>>
>> > observers (which don't seem to include Android-specific classes)? And
>> > also
>
> do
>>
>> > you know why we don't implement (at least a part of) tab/session
>> > persistence
>> on
>> > the Chromium side? I guess that this heavily depends on classes
>> > implemented
>
> on
>>
>> > the application side.
>
>
>> I had a quick chat with Yaron and this will need a little more work. I
>> will
>
> see
>>
>> how I can get rid of the call to TabRestoreService::CreateHistoricalTab()
>> on
>
> the
>>
>> application side so that we can totally compile it out.
>
>
>> I will give you an update once I have something working.
>
>
> I think we can hardly get rid of TabRestoreService. It's tightly coupled to
> the
> RecentlyClosedTabsHandler class which is used to display the list of
> recently
> closed tabs on the welcome page (and which is one of the notifications
> subscribers).
> I might be able to abstract TabRestoreService and have a specific Android
> implementation for it. I'm not sure it is the way to go though.
> This would add a significant amount of code and abstractions without adding
> much
> value.
>
> One possible way of doing would be not to have the Android dummy session
> backend
> and set the session backend instance to NULL instead on Android. The
> downside is
> that we would have to check that the pointer is not NULL every time we
> access
> the backend.
> More generally we could also support an in-memory mode in the
> TabRestoreService
> as we use in Android.
>
> What do you think?
>
> http://codereview.chromium.org/10989027/

[Philippe, 2012-09-28 23:00:34]

On 2012/09/28 20:26:51, sky wrote:
> I think it makes more sense to introduce an interface that
> RecentlyClosedTabsHandler talks to. TabRestoreService would implement
> this as would the android side.
> 
>   -Scott
> 
> On Fri, Sep 28, 2012 at 11:14 AM,  <mailto:pliard@chromium.org> wrote:
> > On 2012/09/27 23:51:35, Philippe wrote:
> >>
> >> On 2012/09/26 23:29:35, Philippe wrote:
> >> > On 2012/09/26 23:00:59, sky wrote:
> >> > > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org> wrote:
> >> > > >
> >> > > >
> >> > >
> >> >
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> >>
> >> > > > File chrome/browser/sessions/session_backend_android.cc (right):
> >> > > >
> >> > > >
> >> > >
> >> >
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> >>
> >> > > > chrome/browser/sessions/session_backend_android.cc:10: // Dummy
> >> > > > session
> >> > > > backend for Android. Tab and session persistence is handled
> >> > > > On 2012/09/26 20:54:21, sky wrote:
> >> > > >>
> >> > > >> If tab and session persistence is handled else where, why is the
> >> > > >
> >> > > > sessions code
> >> > > >>
> >> > > >> even compiled?
> >> > > >
> >> > > >
> >> > > > That's a good question. We do use
> >> > > > TabRestoreService::CreateHistoricalTab() internally on the app side.
> >> > >
> >> > > If that's all you need it should be factored into a standalone
> >> > > function.
> >> > >
> >> > > > Although we use here a dummy backend I suspect that we rely on
> >> > > > TabRestoreService's observers notification in some way which would
> >> > > > be
> >> > > > the only no-op on Android (see NotifyTabsChanged() in
> >> > > > TabRestoreService::AddEntry()).
> >> > > > Unfortunately I'm not really familiar with this code. Do you  know
> >> > > > who
> >> > > > is subscribing to these notifications and what would be the impact
> >> > > > of
> >> > > > not receiving them?
> >> > >
> >> > > I don't know who relies on these for android code. But presumably you
> >> > > only care about TabRestoreServiceChanged, which would only happen if
> >> > > you're modifying TabRestoreService. So, if you're not using
> >> > > TabRestoreService then it should never be modified and
> >> > > TabRestoreServiceChanged never sent, right?
> >> > >
> >> > >   -Scott
> >> > >
> >> > > > I didn't see any Android-specific code subscribing to these
> >> > > > notifications and have really no idea why we do call
> >> > > > TabRestoreService::CreateHistoricalTab(). You might have an idea.
> >> > > > It's hard to track the file history internally on the app side with
> >> > > > all
> >> > > > the merges and moves.
> >> > > >
> >> > > > https://chromiumcodereview.appspot.com/10989027/
> >> >
> >> > My understanding is that we do modify TabRestoreService's in-memory
> >> > state by
> >> > calling CreateHistoricalTab(). Therefore TabRestoreService's clients
> >> > might
> >
> > be
> >>
> >> > interested in receiving notifications. The only thing we don't do in
> >> > TabRestoreService is the disk persistence. I'm wondering by the way why
> >> > we
> >> don't
> >> > have a non-dummy session backend implementation on the Chromium side
> >> forwarding
> >> > calls to Java to implement the actual persistence.
> >> >
> >> > I'm adding Yaron who probably knows more about this. Yaron do you know
> >> > why
> >
> > we
> >>
> >> > are calling TabRestoreService::CreateHistoricalTab() on the app side
> >> > given
> >> that
> >> > we handle tab and session persistence on the Java side? Is that only to
> >
> > notify
> >>
> >> > observers (which don't seem to include Android-specific classes)? And
> >> > also
> >
> > do
> >>
> >> > you know why we don't implement (at least a part of) tab/session
> >> > persistence
> >> on
> >> > the Chromium side? I guess that this heavily depends on classes
> >> > implemented
> >
> > on
> >>
> >> > the application side.
> >
> >
> >> I had a quick chat with Yaron and this will need a little more work. I
> >> will
> >
> > see
> >>
> >> how I can get rid of the call to TabRestoreService::CreateHistoricalTab()
> >> on
> >
> > the
> >>
> >> application side so that we can totally compile it out.
> >
> >
> >> I will give you an update once I have something working.
> >
> >
> > I think we can hardly get rid of TabRestoreService. It's tightly coupled to
> > the
> > RecentlyClosedTabsHandler class which is used to display the list of
> > recently
> > closed tabs on the welcome page (and which is one of the notifications
> > subscribers).
> > I might be able to abstract TabRestoreService and have a specific Android
> > implementation for it. I'm not sure it is the way to go though.
> > This would add a significant amount of code and abstractions without adding
> > much
> > value.
> >
> > One possible way of doing would be not to have the Android dummy session
> > backend
> > and set the session backend instance to NULL instead on Android. The
> > downside is
> > that we would have to check that the pointer is not NULL every time we
> > access
> > the backend.
> > More generally we could also support an in-memory mode in the
> > TabRestoreService
> > as we use in Android.
> >
> > What do you think?
> >
> > http://codereview.chromium.org/10989027/

Then I'm fine with doing that. I just wanted to make sure you were ready to
accept such design changes. Please expect a quite large change :)

[sky, 2012-09-28 23:07:18]

Why would this be that big? RecentlyClosedTabsHandler doesn't use that
much of TabRestoreService, which should mean a simple interface.
Presumably you'll need to promote the observer too, but that won't be
that large either. Am I missing something?

  -Scott

On Fri, Sep 28, 2012 at 4:00 PM,  <pliard@chromium.org> wrote:
> On 2012/09/28 20:26:51, sky wrote:
>>
>> I think it makes more sense to introduce an interface that
>> RecentlyClosedTabsHandler talks to. TabRestoreService would implement
>> this as would the android side.
>
>
>>    -Scott
>
>
>> On Fri, Sep 28, 2012 at 11:14 AM,  <mailto:pliard@chromium.org> wrote:
>> > On 2012/09/27 23:51:35, Philippe wrote:
>> >>
>> >> On 2012/09/26 23:29:35, Philippe wrote:
>> >> > On 2012/09/26 23:00:59, sky wrote:
>> >> > > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org>
>> >> > > wrote:
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >
>> >
>> >
>
>
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
>>
>> >>
>> >> > > > File chrome/browser/sessions/session_backend_android.cc (right):
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >
>> >
>> >
>
>
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
>>
>> >>
>> >> > > > chrome/browser/sessions/session_backend_android.cc:10: // Dummy
>> >> > > > session
>> >> > > > backend for Android. Tab and session persistence is handled
>> >> > > > On 2012/09/26 20:54:21, sky wrote:
>> >> > > >>
>> >> > > >> If tab and session persistence is handled else where, why is the
>> >> > > >
>> >> > > > sessions code
>> >> > > >>
>> >> > > >> even compiled?
>> >> > > >
>> >> > > >
>> >> > > > That's a good question. We do use
>> >> > > > TabRestoreService::CreateHistoricalTab() internally on the app
>> >> > > > side.
>> >> > >
>> >> > > If that's all you need it should be factored into a standalone
>> >> > > function.
>> >> > >
>> >> > > > Although we use here a dummy backend I suspect that we rely on
>> >> > > > TabRestoreService's observers notification in some way which
>> >> > > > would
>> >> > > > be
>> >> > > > the only no-op on Android (see NotifyTabsChanged() in
>> >> > > > TabRestoreService::AddEntry()).
>> >> > > > Unfortunately I'm not really familiar with this code. Do you
>> >> > > > know
>> >> > > > who
>> >> > > > is subscribing to these notifications and what would be the
>> >> > > > impact
>> >> > > > of
>> >> > > > not receiving them?
>> >> > >
>> >> > > I don't know who relies on these for android code. But presumably
>> >> > > you
>> >> > > only care about TabRestoreServiceChanged, which would only happen
>> >> > > if
>> >> > > you're modifying TabRestoreService. So, if you're not using
>> >> > > TabRestoreService then it should never be modified and
>> >> > > TabRestoreServiceChanged never sent, right?
>> >> > >
>> >> > >   -Scott
>> >> > >
>> >> > > > I didn't see any Android-specific code subscribing to these
>> >> > > > notifications and have really no idea why we do call
>> >> > > > TabRestoreService::CreateHistoricalTab(). You might have an idea.
>> >> > > > It's hard to track the file history internally on the app side
>> >> > > > with
>> >> > > > all
>> >> > > > the merges and moves.
>> >> > > >
>> >> > > > https://chromiumcodereview.appspot.com/10989027/
>> >> >
>> >> > My understanding is that we do modify TabRestoreService's in-memory
>> >> > state by
>> >> > calling CreateHistoricalTab(). Therefore TabRestoreService's clients
>> >> > might
>> >
>> > be
>> >>
>> >> > interested in receiving notifications. The only thing we don't do in
>> >> > TabRestoreService is the disk persistence. I'm wondering by the way
>> >> > why
>> >> > we
>> >> don't
>> >> > have a non-dummy session backend implementation on the Chromium side
>> >> forwarding
>> >> > calls to Java to implement the actual persistence.
>> >> >
>> >> > I'm adding Yaron who probably knows more about this. Yaron do you
>> >> > know
>> >> > why
>> >
>> > we
>> >>
>> >> > are calling TabRestoreService::CreateHistoricalTab() on the app side
>> >> > given
>> >> that
>> >> > we handle tab and session persistence on the Java side? Is that only
>> >> > to
>> >
>> > notify
>> >>
>> >> > observers (which don't seem to include Android-specific classes)? And
>> >> > also
>> >
>> > do
>> >>
>> >> > you know why we don't implement (at least a part of) tab/session
>> >> > persistence
>> >> on
>> >> > the Chromium side? I guess that this heavily depends on classes
>> >> > implemented
>> >
>> > on
>> >>
>> >> > the application side.
>> >
>> >
>> >> I had a quick chat with Yaron and this will need a little more work. I
>> >> will
>> >
>> > see
>> >>
>> >> how I can get rid of the call to
>> >> TabRestoreService::CreateHistoricalTab()
>> >> on
>> >
>> > the
>> >>
>> >> application side so that we can totally compile it out.
>> >
>> >
>> >> I will give you an update once I have something working.
>> >
>> >
>> > I think we can hardly get rid of TabRestoreService. It's tightly coupled
>> > to
>> > the
>> > RecentlyClosedTabsHandler class which is used to display the list of
>> > recently
>> > closed tabs on the welcome page (and which is one of the notifications
>> > subscribers).
>> > I might be able to abstract TabRestoreService and have a specific
>> > Android
>> > implementation for it. I'm not sure it is the way to go though.
>> > This would add a significant amount of code and abstractions without
>> > adding
>> > much
>> > value.
>> >
>> > One possible way of doing would be not to have the Android dummy session
>> > backend
>> > and set the session backend instance to NULL instead on Android. The
>> > downside is
>> > that we would have to check that the pointer is not NULL every time we
>> > access
>> > the backend.
>> > More generally we could also support an in-memory mode in the
>> > TabRestoreService
>> > as we use in Android.
>> >
>> > What do you think?
>> >
>> > http://codereview.chromium.org/10989027/
>
>
> Then I'm fine with doing that. I just wanted to make sure you were ready to
> accept such design changes. Please expect a quite large change :)
>
> http://codereview.chromium.org/10989027/

[Philippe, 2012-09-28 23:23:41]

On 2012/09/28 23:07:18, sky wrote:
> Why would this be that big? RecentlyClosedTabsHandler doesn't use that
> much of TabRestoreService, which should mean a simple interface.
> Presumably you'll need to promote the observer too, but that won't be
> that large either. Am I missing something?
> 
>   -Scott
> 
> On Fri, Sep 28, 2012 at 4:00 PM,  <mailto:pliard@chromium.org> wrote:
> > On 2012/09/28 20:26:51, sky wrote:
> >>
> >> I think it makes more sense to introduce an interface that
> >> RecentlyClosedTabsHandler talks to. TabRestoreService would implement
> >> this as would the android side.
> >
> >
> >>    -Scott
> >
> >
> >> On Fri, Sep 28, 2012 at 11:14 AM,  <mailto:pliard@chromium.org> wrote:
> >> > On 2012/09/27 23:51:35, Philippe wrote:
> >> >>
> >> >> On 2012/09/26 23:29:35, Philippe wrote:
> >> >> > On 2012/09/26 23:00:59, sky wrote:
> >> >> > > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org>
> >> >> > > wrote:
> >> >> > > >
> >> >> > > >
> >> >> > >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> >>
> >> >>
> >> >> > > > File chrome/browser/sessions/session_backend_android.cc (right):
> >> >> > > >
> >> >> > > >
> >> >> > >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> >>
> >> >>
> >> >> > > > chrome/browser/sessions/session_backend_android.cc:10: // Dummy
> >> >> > > > session
> >> >> > > > backend for Android. Tab and session persistence is handled
> >> >> > > > On 2012/09/26 20:54:21, sky wrote:
> >> >> > > >>
> >> >> > > >> If tab and session persistence is handled else where, why is the
> >> >> > > >
> >> >> > > > sessions code
> >> >> > > >>
> >> >> > > >> even compiled?
> >> >> > > >
> >> >> > > >
> >> >> > > > That's a good question. We do use
> >> >> > > > TabRestoreService::CreateHistoricalTab() internally on the app
> >> >> > > > side.
> >> >> > >
> >> >> > > If that's all you need it should be factored into a standalone
> >> >> > > function.
> >> >> > >
> >> >> > > > Although we use here a dummy backend I suspect that we rely on
> >> >> > > > TabRestoreService's observers notification in some way which
> >> >> > > > would
> >> >> > > > be
> >> >> > > > the only no-op on Android (see NotifyTabsChanged() in
> >> >> > > > TabRestoreService::AddEntry()).
> >> >> > > > Unfortunately I'm not really familiar with this code. Do you
> >> >> > > > know
> >> >> > > > who
> >> >> > > > is subscribing to these notifications and what would be the
> >> >> > > > impact
> >> >> > > > of
> >> >> > > > not receiving them?
> >> >> > >
> >> >> > > I don't know who relies on these for android code. But presumably
> >> >> > > you
> >> >> > > only care about TabRestoreServiceChanged, which would only happen
> >> >> > > if
> >> >> > > you're modifying TabRestoreService. So, if you're not using
> >> >> > > TabRestoreService then it should never be modified and
> >> >> > > TabRestoreServiceChanged never sent, right?
> >> >> > >
> >> >> > >   -Scott
> >> >> > >
> >> >> > > > I didn't see any Android-specific code subscribing to these
> >> >> > > > notifications and have really no idea why we do call
> >> >> > > > TabRestoreService::CreateHistoricalTab(). You might have an idea.
> >> >> > > > It's hard to track the file history internally on the app side
> >> >> > > > with
> >> >> > > > all
> >> >> > > > the merges and moves.
> >> >> > > >
> >> >> > > > https://chromiumcodereview.appspot.com/10989027/
> >> >> >
> >> >> > My understanding is that we do modify TabRestoreService's in-memory
> >> >> > state by
> >> >> > calling CreateHistoricalTab(). Therefore TabRestoreService's clients
> >> >> > might
> >> >
> >> > be
> >> >>
> >> >> > interested in receiving notifications. The only thing we don't do in
> >> >> > TabRestoreService is the disk persistence. I'm wondering by the way
> >> >> > why
> >> >> > we
> >> >> don't
> >> >> > have a non-dummy session backend implementation on the Chromium side
> >> >> forwarding
> >> >> > calls to Java to implement the actual persistence.
> >> >> >
> >> >> > I'm adding Yaron who probably knows more about this. Yaron do you
> >> >> > know
> >> >> > why
> >> >
> >> > we
> >> >>
> >> >> > are calling TabRestoreService::CreateHistoricalTab() on the app side
> >> >> > given
> >> >> that
> >> >> > we handle tab and session persistence on the Java side? Is that only
> >> >> > to
> >> >
> >> > notify
> >> >>
> >> >> > observers (which don't seem to include Android-specific classes)? And
> >> >> > also
> >> >
> >> > do
> >> >>
> >> >> > you know why we don't implement (at least a part of) tab/session
> >> >> > persistence
> >> >> on
> >> >> > the Chromium side? I guess that this heavily depends on classes
> >> >> > implemented
> >> >
> >> > on
> >> >>
> >> >> > the application side.
> >> >
> >> >
> >> >> I had a quick chat with Yaron and this will need a little more work. I
> >> >> will
> >> >
> >> > see
> >> >>
> >> >> how I can get rid of the call to
> >> >> TabRestoreService::CreateHistoricalTab()
> >> >> on
> >> >
> >> > the
> >> >>
> >> >> application side so that we can totally compile it out.
> >> >
> >> >
> >> >> I will give you an update once I have something working.
> >> >
> >> >
> >> > I think we can hardly get rid of TabRestoreService. It's tightly coupled
> >> > to
> >> > the
> >> > RecentlyClosedTabsHandler class which is used to display the list of
> >> > recently
> >> > closed tabs on the welcome page (and which is one of the notifications
> >> > subscribers).
> >> > I might be able to abstract TabRestoreService and have a specific
> >> > Android
> >> > implementation for it. I'm not sure it is the way to go though.
> >> > This would add a significant amount of code and abstractions without
> >> > adding
> >> > much
> >> > value.
> >> >
> >> > One possible way of doing would be not to have the Android dummy session
> >> > backend
> >> > and set the session backend instance to NULL instead on Android. The
> >> > downside is
> >> > that we would have to check that the pointer is not NULL every time we
> >> > access
> >> > the backend.
> >> > More generally we could also support an in-memory mode in the
> >> > TabRestoreService
> >> > as we use in Android.
> >> >
> >> > What do you think?
> >> >
> >> > http://codereview.chromium.org/10989027/
> >
> >
> > Then I'm fine with doing that. I just wanted to make sure you were ready to
> > accept such design changes. Please expect a quite large change :)
> >
> > http://codereview.chromium.org/10989027/

I don't have a very well defined plan yet but I can already see that
RecentlyClosedTabsHandler uses:
- TabRestoreService::Entries/Entry
- TabRestoreService::Tab
- TabRestoreService::Window
- TabRestoreServiceFactory::GetForProfile()
- TabRestoreServiceDelegate::FindDelegateForWebContents() (I haven't looked at
that in detail though)
- TabRestoreService::AddObserver()
- TabRestoreService::RemoveObserver()
- TabRestoreService::RemoveTabEntryById()
- TabRestoreService::RestoreEntryById()
- TabRestoreService::ClearEntries()
- TabRestoreService::LoadTabsFromLastSession()

So I think it does use a significant part of TabRestoreService.

I'm concerned about duplicating some code. I think it's easy to end up with an
awkward design.

What do you think?

[Philippe, 2012-09-28 23:25:35]

On 2012/09/28 23:23:41, Philippe wrote:
> On 2012/09/28 23:07:18, sky wrote:
> > Why would this be that big? RecentlyClosedTabsHandler doesn't use that
> > much of TabRestoreService, which should mean a simple interface.
> > Presumably you'll need to promote the observer too, but that won't be
> > that large either. Am I missing something?
> > 
> >   -Scott
> > 
> > On Fri, Sep 28, 2012 at 4:00 PM,  <mailto:pliard@chromium.org> wrote:
> > > On 2012/09/28 20:26:51, sky wrote:
> > >>
> > >> I think it makes more sense to introduce an interface that
> > >> RecentlyClosedTabsHandler talks to. TabRestoreService would implement
> > >> this as would the android side.
> > >
> > >
> > >>    -Scott
> > >
> > >
> > >> On Fri, Sep 28, 2012 at 11:14 AM,  <mailto:pliard@chromium.org> wrote:
> > >> > On 2012/09/27 23:51:35, Philippe wrote:
> > >> >>
> > >> >> On 2012/09/26 23:29:35, Philippe wrote:
> > >> >> > On 2012/09/26 23:00:59, sky wrote:
> > >> >> > > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org>
> > >> >> > > wrote:
> > >> >> > > >
> > >> >> > > >
> > >> >> > >
> > >> >> >
> > >> >
> > >> >
> > >> >
> > >
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > >>
> > >> >>
> > >> >> > > > File chrome/browser/sessions/session_backend_android.cc (right):
> > >> >> > > >
> > >> >> > > >
> > >> >> > >
> > >> >> >
> > >> >
> > >> >
> > >> >
> > >
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sess...
> > >>
> > >> >>
> > >> >> > > > chrome/browser/sessions/session_backend_android.cc:10: // Dummy
> > >> >> > > > session
> > >> >> > > > backend for Android. Tab and session persistence is handled
> > >> >> > > > On 2012/09/26 20:54:21, sky wrote:
> > >> >> > > >>
> > >> >> > > >> If tab and session persistence is handled else where, why is
the
> > >> >> > > >
> > >> >> > > > sessions code
> > >> >> > > >>
> > >> >> > > >> even compiled?
> > >> >> > > >
> > >> >> > > >
> > >> >> > > > That's a good question. We do use
> > >> >> > > > TabRestoreService::CreateHistoricalTab() internally on the app
> > >> >> > > > side.
> > >> >> > >
> > >> >> > > If that's all you need it should be factored into a standalone
> > >> >> > > function.
> > >> >> > >
> > >> >> > > > Although we use here a dummy backend I suspect that we rely on
> > >> >> > > > TabRestoreService's observers notification in some way which
> > >> >> > > > would
> > >> >> > > > be
> > >> >> > > > the only no-op on Android (see NotifyTabsChanged() in
> > >> >> > > > TabRestoreService::AddEntry()).
> > >> >> > > > Unfortunately I'm not really familiar with this code. Do you
> > >> >> > > > know
> > >> >> > > > who
> > >> >> > > > is subscribing to these notifications and what would be the
> > >> >> > > > impact
> > >> >> > > > of
> > >> >> > > > not receiving them?
> > >> >> > >
> > >> >> > > I don't know who relies on these for android code. But presumably
> > >> >> > > you
> > >> >> > > only care about TabRestoreServiceChanged, which would only happen
> > >> >> > > if
> > >> >> > > you're modifying TabRestoreService. So, if you're not using
> > >> >> > > TabRestoreService then it should never be modified and
> > >> >> > > TabRestoreServiceChanged never sent, right?
> > >> >> > >
> > >> >> > >   -Scott
> > >> >> > >
> > >> >> > > > I didn't see any Android-specific code subscribing to these
> > >> >> > > > notifications and have really no idea why we do call
> > >> >> > > > TabRestoreService::CreateHistoricalTab(). You might have an
idea.
> > >> >> > > > It's hard to track the file history internally on the app side
> > >> >> > > > with
> > >> >> > > > all
> > >> >> > > > the merges and moves.
> > >> >> > > >
> > >> >> > > > https://chromiumcodereview.appspot.com/10989027/
> > >> >> >
> > >> >> > My understanding is that we do modify TabRestoreService's in-memory
> > >> >> > state by
> > >> >> > calling CreateHistoricalTab(). Therefore TabRestoreService's clients
> > >> >> > might
> > >> >
> > >> > be
> > >> >>
> > >> >> > interested in receiving notifications. The only thing we don't do in
> > >> >> > TabRestoreService is the disk persistence. I'm wondering by the way
> > >> >> > why
> > >> >> > we
> > >> >> don't
> > >> >> > have a non-dummy session backend implementation on the Chromium side
> > >> >> forwarding
> > >> >> > calls to Java to implement the actual persistence.
> > >> >> >
> > >> >> > I'm adding Yaron who probably knows more about this. Yaron do you
> > >> >> > know
> > >> >> > why
> > >> >
> > >> > we
> > >> >>
> > >> >> > are calling TabRestoreService::CreateHistoricalTab() on the app side
> > >> >> > given
> > >> >> that
> > >> >> > we handle tab and session persistence on the Java side? Is that only
> > >> >> > to
> > >> >
> > >> > notify
> > >> >>
> > >> >> > observers (which don't seem to include Android-specific classes)?
And
> > >> >> > also
> > >> >
> > >> > do
> > >> >>
> > >> >> > you know why we don't implement (at least a part of) tab/session
> > >> >> > persistence
> > >> >> on
> > >> >> > the Chromium side? I guess that this heavily depends on classes
> > >> >> > implemented
> > >> >
> > >> > on
> > >> >>
> > >> >> > the application side.
> > >> >
> > >> >
> > >> >> I had a quick chat with Yaron and this will need a little more work. I
> > >> >> will
> > >> >
> > >> > see
> > >> >>
> > >> >> how I can get rid of the call to
> > >> >> TabRestoreService::CreateHistoricalTab()
> > >> >> on
> > >> >
> > >> > the
> > >> >>
> > >> >> application side so that we can totally compile it out.
> > >> >
> > >> >
> > >> >> I will give you an update once I have something working.
> > >> >
> > >> >
> > >> > I think we can hardly get rid of TabRestoreService. It's tightly
coupled
> > >> > to
> > >> > the
> > >> > RecentlyClosedTabsHandler class which is used to display the list of
> > >> > recently
> > >> > closed tabs on the welcome page (and which is one of the notifications
> > >> > subscribers).
> > >> > I might be able to abstract TabRestoreService and have a specific
> > >> > Android
> > >> > implementation for it. I'm not sure it is the way to go though.
> > >> > This would add a significant amount of code and abstractions without
> > >> > adding
> > >> > much
> > >> > value.
> > >> >
> > >> > One possible way of doing would be not to have the Android dummy
session
> > >> > backend
> > >> > and set the session backend instance to NULL instead on Android. The
> > >> > downside is
> > >> > that we would have to check that the pointer is not NULL every time we
> > >> > access
> > >> > the backend.
> > >> > More generally we could also support an in-memory mode in the
> > >> > TabRestoreService
> > >> > as we use in Android.
> > >> >
> > >> > What do you think?
> > >> >
> > >> > http://codereview.chromium.org/10989027/
> > >
> > >
> > > Then I'm fine with doing that. I just wanted to make sure you were ready
to
> > > accept such design changes. Please expect a quite large change :)
> > >
> > > http://codereview.chromium.org/10989027/
> 
> I don't have a very well defined plan yet but I can already see that
> RecentlyClosedTabsHandler uses:
> - TabRestoreService::Entries/Entry
> - TabRestoreService::Tab
> - TabRestoreService::Window
> - TabRestoreServiceFactory::GetForProfile()
> - TabRestoreServiceDelegate::FindDelegateForWebContents() (I haven't looked at
> that in detail though)
> - TabRestoreService::AddObserver()
> - TabRestoreService::RemoveObserver()
> - TabRestoreService::RemoveTabEntryById()
> - TabRestoreService::RestoreEntryById()
> - TabRestoreService::ClearEntries()
> - TabRestoreService::LoadTabsFromLastSession()
> 
> So I think it does use a significant part of TabRestoreService.
> 
> I'm concerned about duplicating some code. I think it's easy to end up with an
> awkward design.
> 
> What do you think?

It might be simpler actually to abstract the BaseSessionService
(TabRestoreService's parent class) and use composition rather than inheritance.
What do you think?

[pliard, 2012-09-29 00:06:50]

I will try to abstract TabRestoreService and see how it looks.


On Sat, Sep 29, 2012 at 12:25 AM, <pliard@chromium.org> wrote:

> On 2012/09/28 23:23:41, Philippe wrote:
>
>> On 2012/09/28 23:07:18, sky wrote:
>> > Why would this be that big? RecentlyClosedTabsHandler doesn't use that
>> > much of TabRestoreService, which should mean a simple interface.
>> > Presumably you'll need to promote the observer too, but that won't be
>> > that large either. Am I missing something?
>> >
>> >   -Scott
>> >
>> > On Fri, Sep 28, 2012 at 4:00 PM,  <mailto:pliard@chromium.org> wrote:
>> > > On 2012/09/28 20:26:51, sky wrote:
>> > >>
>> > >> I think it makes more sense to introduce an interface that
>> > >> RecentlyClosedTabsHandler talks to. TabRestoreService would implement
>> > >> this as would the android side.
>> > >
>> > >
>> > >>    -Scott
>> > >
>> > >
>> > >> On Fri, Sep 28, 2012 at 11:14 AM,  <mailto:pliard@chromium.org>
>> wrote:
>> > >> > On 2012/09/27 23:51:35, Philippe wrote:
>> > >> >>
>> > >> >> On 2012/09/26 23:29:35, Philippe wrote:
>> > >> >> > On 2012/09/26 23:00:59, sky wrote:
>> > >> >> > > On Wed, Sep 26, 2012 at 2:28 PM,  <mailto:pliard@chromium.org
>> >
>> > >> >> > > wrote:
>> > >> >> > > >
>> > >> >> > > >
>> > >> >> > >
>> > >> >> >
>> > >> >
>> > >> >
>> > >> >
>> > >
>> > >
>> > >
>> >
>>
>
> https://chromiumcodereview.**appspot.com/10989027/diff/**
>
8002/chrome/browser/sessions/**session_backend_android.cc<https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sessions/session_backend_android.cc>
>
>> > >>
>> > >> >>
>> > >> >> > > > File chrome/browser/sessions/**session_backend_android.cc
>> (right):
>> > >> >> > > >
>> > >> >> > > >
>> > >> >> > >
>> > >> >> >
>> > >> >
>> > >> >
>> > >> >
>> > >
>> > >
>> > >
>> >
>>
>
> https://chromiumcodereview.**appspot.com/10989027/diff/**
>
8002/chrome/browser/sessions/**session_backend_android.cc#**newcode10<https://chromiumcodereview.appspot.com/10989027/diff/8002/chrome/browser/sessions/session_backend_android.cc#newcode10>
>
>> > >>
>> > >> >>
>> > >> >> > > > chrome/browser/sessions/**session_backend_android.cc:10:
>> // Dummy
>> > >> >> > > > session
>> > >> >> > > > backend for Android. Tab and session persistence is handled
>> > >> >> > > > On 2012/09/26 20:54:21, sky wrote:
>> > >> >> > > >>
>> > >> >> > > >> If tab and session persistence is handled else where, why
>> is
>>
> the
>
>> > >> >> > > >
>> > >> >> > > > sessions code
>> > >> >> > > >>
>> > >> >> > > >> even compiled?
>> > >> >> > > >
>> > >> >> > > >
>> > >> >> > > > That's a good question. We do use
>> > >> >> > > > TabRestoreService::**CreateHistoricalTab() internally on
>> the app
>> > >> >> > > > side.
>> > >> >> > >
>> > >> >> > > If that's all you need it should be factored into a standalone
>> > >> >> > > function.
>> > >> >> > >
>> > >> >> > > > Although we use here a dummy backend I suspect that we rely
>> on
>> > >> >> > > > TabRestoreService's observers notification in some way which
>> > >> >> > > > would
>> > >> >> > > > be
>> > >> >> > > > the only no-op on Android (see NotifyTabsChanged() in
>> > >> >> > > > TabRestoreService::AddEntry())**.
>> > >> >> > > > Unfortunately I'm not really familiar with this code. Do you
>> > >> >> > > > know
>> > >> >> > > > who
>> > >> >> > > > is subscribing to these notifications and what would be the
>> > >> >> > > > impact
>> > >> >> > > > of
>> > >> >> > > > not receiving them?
>> > >> >> > >
>> > >> >> > > I don't know who relies on these for android code. But
>> presumably
>> > >> >> > > you
>> > >> >> > > only care about TabRestoreServiceChanged, which would only
>> happen
>> > >> >> > > if
>> > >> >> > > you're modifying TabRestoreService. So, if you're not using
>> > >> >> > > TabRestoreService then it should never be modified and
>> > >> >> > > TabRestoreServiceChanged never sent, right?
>> > >> >> > >
>> > >> >> > >   -Scott
>> > >> >> > >
>> > >> >> > > > I didn't see any Android-specific code subscribing to these
>> > >> >> > > > notifications and have really no idea why we do call
>> > >> >> > > > TabRestoreService::**CreateHistoricalTab(). You might have
>> an
>>
> idea.
>
>> > >> >> > > > It's hard to track the file history internally on the app
>> side
>> > >> >> > > > with
>> > >> >> > > > all
>> > >> >> > > > the merges and moves.
>> > >> >> > > >
>> > >> >> > > >
https://chromiumcodereview.**appspot.com/10989027/<https://chromiumcodereview...
>> > >> >> >
>> > >> >> > My understanding is that we do modify TabRestoreService's
>> in-memory
>> > >> >> > state by
>> > >> >> > calling CreateHistoricalTab(). Therefore TabRestoreService's
>> clients
>> > >> >> > might
>> > >> >
>> > >> > be
>> > >> >>
>> > >> >> > interested in receiving notifications. The only thing we don't
>> do in
>> > >> >> > TabRestoreService is the disk persistence. I'm wondering by the
>> way
>> > >> >> > why
>> > >> >> > we
>> > >> >> don't
>> > >> >> > have a non-dummy session backend implementation on the Chromium
>> side
>> > >> >> forwarding
>> > >> >> > calls to Java to implement the actual persistence.
>> > >> >> >
>> > >> >> > I'm adding Yaron who probably knows more about this. Yaron do
>> you
>> > >> >> > know
>> > >> >> > why
>> > >> >
>> > >> > we
>> > >> >>
>> > >> >> > are calling TabRestoreService::**CreateHistoricalTab() on the
>> app side
>> > >> >> > given
>> > >> >> that
>> > >> >> > we handle tab and session persistence on the Java side? Is that
>> only
>> > >> >> > to
>> > >> >
>> > >> > notify
>> > >> >>
>> > >> >> > observers (which don't seem to include Android-specific
>> classes)?
>>
> And
>
>> > >> >> > also
>> > >> >
>> > >> > do
>> > >> >>
>> > >> >> > you know why we don't implement (at least a part of) tab/session
>> > >> >> > persistence
>> > >> >> on
>> > >> >> > the Chromium side? I guess that this heavily depends on classes
>> > >> >> > implemented
>> > >> >
>> > >> > on
>> > >> >>
>> > >> >> > the application side.
>> > >> >
>> > >> >
>> > >> >> I had a quick chat with Yaron and this will need a little more
>> work. I
>> > >> >> will
>> > >> >
>> > >> > see
>> > >> >>
>> > >> >> how I can get rid of the call to
>> > >> >> TabRestoreService::**CreateHistoricalTab()
>> > >> >> on
>> > >> >
>> > >> > the
>> > >> >>
>> > >> >> application side so that we can totally compile it out.
>> > >> >
>> > >> >
>> > >> >> I will give you an update once I have something working.
>> > >> >
>> > >> >
>> > >> > I think we can hardly get rid of TabRestoreService. It's tightly
>>
> coupled
>
>> > >> > to
>> > >> > the
>> > >> > RecentlyClosedTabsHandler class which is used to display the list
>> of
>> > >> > recently
>> > >> > closed tabs on the welcome page (and which is one of the
>> notifications
>> > >> > subscribers).
>> > >> > I might be able to abstract TabRestoreService and have a specific
>> > >> > Android
>> > >> > implementation for it. I'm not sure it is the way to go though.
>> > >> > This would add a significant amount of code and abstractions
>> without
>> > >> > adding
>> > >> > much
>> > >> > value.
>> > >> >
>> > >> > One possible way of doing would be not to have the Android dummy
>>
> session
>
>> > >> > backend
>> > >> > and set the session backend instance to NULL instead on Android.
>> The
>> > >> > downside is
>> > >> > that we would have to check that the pointer is not NULL every
>> time we
>> > >> > access
>> > >> > the backend.
>> > >> > More generally we could also support an in-memory mode in the
>> > >> > TabRestoreService
>> > >> > as we use in Android.
>> > >> >
>> > >> > What do you think?
>> > >> >
>> > >> >
http://codereview.chromium.**org/10989027/<http://codereview.chromium.org/109...
>> > >
>> > >
>> > > Then I'm fine with doing that. I just wanted to make sure you were
>> ready
>>
> to
>
>> > > accept such design changes. Please expect a quite large change :)
>> > >
>> > >
http://codereview.chromium.**org/10989027/<http://codereview.chromium.org/109...
>>
>
>  I don't have a very well defined plan yet but I can already see that
>> RecentlyClosedTabsHandler uses:
>> - TabRestoreService::Entries/**Entry
>> - TabRestoreService::Tab
>> - TabRestoreService::Window
>> - TabRestoreServiceFactory::**GetForProfile()
>> - TabRestoreServiceDelegate::**FindDelegateForWebContents() (I haven't
>> looked at
>> that in detail though)
>> - TabRestoreService::**AddObserver()
>> - TabRestoreService::**RemoveObserver()
>> - TabRestoreService::**RemoveTabEntryById()
>> - TabRestoreService::**RestoreEntryById()
>> - TabRestoreService::**ClearEntries()
>> - TabRestoreService::**LoadTabsFromLastSession()
>>
>
>  So I think it does use a significant part of TabRestoreService.
>>
>
>  I'm concerned about duplicating some code. I think it's easy to end up
>> with an
>> awkward design.
>>
>
>  What do you think?
>>
>
> It might be simpler actually to abstract the BaseSessionService
> (TabRestoreService's parent class) and use composition rather than
> inheritance.
>
> What do you think?
>
>
http://codereview.chromium.**org/10989027/<http://codereview.chromium.org/109...
>

[Philippe, 2012-09-29 01:51:22]

I came across another issue. I was assuming that TabRestoreService was
inheriting from BaseSessionService only to pull some common implementation code.
The thing is that BaseSessionService itself inherits from ProfileKeyedService
and some other clients do treat TabRestoreService as a ProfileKeyedService. That
means I can't easily make TabRestoreService a top-level interface/abstract
class. The goal would have been to avoid using BaseSessionService on Android.

I don't see any nice design that could work well in our case. I didn't find the
dummy Android backend or the NULL pointer approach that bad.

What do you think?

[sky, 2012-10-01 14:29:38]

Do you need to change all clients, or just RecentlyClosedTabsHandler?
Does android use ProfileKeyedService?

  -Scott

On Fri, Sep 28, 2012 at 6:51 PM,  <pliard@chromium.org> wrote:
> I came across another issue. I was assuming that TabRestoreService was
> inheriting from BaseSessionService only to pull some common implementation
> code.
> The thing is that BaseSessionService itself inherits from
> ProfileKeyedService
> and some other clients do treat TabRestoreService as a ProfileKeyedService.
> That
> means I can't easily make TabRestoreService a top-level interface/abstract
> class. The goal would have been to avoid using BaseSessionService on
> Android.
>
> I don't see any nice design that could work well in our case. I didn't find
> the
> dummy Android backend or the NULL pointer approach that bad.
>
>
> What do you think?
>
> http://codereview.chromium.org/10989027/

[Philippe, 2012-10-01 17:49:39]

On 2012/10/01 14:29:38, sky wrote:
> Do you need to change all clients, or just RecentlyClosedTabsHandler?
> Does android use ProfileKeyedService?
> 
>   -Scott
> 
> On Fri, Sep 28, 2012 at 6:51 PM,  <mailto:pliard@chromium.org> wrote:
> > I came across another issue. I was assuming that TabRestoreService was
> > inheriting from BaseSessionService only to pull some common implementation
> > code.
> > The thing is that BaseSessionService itself inherits from
> > ProfileKeyedService
> > and some other clients do treat TabRestoreService as a ProfileKeyedService.
> > That
> > means I can't easily make TabRestoreService a top-level interface/abstract
> > class. The goal would have been to avoid using BaseSessionService on
> > Android.
> >
> > I don't see any nice design that could work well in our case. I didn't find
> > the
> > dummy Android backend or the NULL pointer approach that bad.
> >
> >
> > What do you think?
> >
> > http://codereview.chromium.org/10989027/

I would need to change all clients since the refactoring affects the public
interface. I only mentioned RecentlyClosedTabsHandler at first since it appeared
to be the one which uses the biggest part of TabRestoreService but there is also
a couple of other clients on Android (not to mention the unit tests) including
browsing_data_remover.cc, browser_command_controller.cc and
browser_tab_strip_model_delegate.cc.

I don't know much about ProfileKeyedService but we do build cc files that use
it.

I'm using fond of refactorings but in this particular case I don't see any clear
design that would be valuable for both desktop and Android, do you?
  
Would it be reasonable to keep the initial approach (or the NULL pointer one)
and add a TODO? We can still improve that later.

What do you think?

[sky, 2012-10-02 20:36:12]

There is only one method in PKS: Shutdown(). You can expose a similar
named method in TabRestoreService that is intended to invoke the
method of the same name on the PKS.

What you need to do aligns nicely with the browser components work (
http://dev.chromium.org/developers/design-documents/browser-components
). You should look into following that pattern. I'm happy to answer
questions. Another resource is Erik Wright who is actively working on
modularizing everything (cc'd).

  -Scott

On Fri, Sep 28, 2012 at 6:51 PM,  <pliard@chromium.org> wrote:
> I came across another issue. I was assuming that TabRestoreService was
> inheriting from BaseSessionService only to pull some common implementation
> code.
> The thing is that BaseSessionService itself inherits from
> ProfileKeyedService
> and some other clients do treat TabRestoreService as a ProfileKeyedService.
> That
> means I can't easily make TabRestoreService a top-level interface/abstract
> class. The goal would have been to avoid using BaseSessionService on
> Android.
>
> I don't see any nice design that could work well in our case. I didn't find
> the
> dummy Android backend or the NULL pointer approach that bad.
>
>
> What do you think?
>
> http://codereview.chromium.org/10989027/

[pliard, 2012-10-05 15:36:31]

I started adding a new PersistentTabRestoreService class inheriting from
TabRestoreService which I'm stripping to fully decouple it from persistence
handling. I don't think it is really close to browser components but it
should address our issue nicely. I was initially mainly concerned about
splitting TabRestoreService into two "random" pieces (determined according
to client needs). It seems that with the default tab restore service
(in-memory only) and the persistent tab restore service we would have two
meaningful and consistent classes.
I might find out new issues later during the refactoring. So far I'm pretty
confident.

How does that sound?


On Tue, Oct 2, 2012 at 10:36 PM, Scott Violet <sky@chromium.org> wrote:

> There is only one method in PKS: Shutdown(). You can expose a similar
> named method in TabRestoreService that is intended to invoke the
> method of the same name on the PKS.
>
> What you need to do aligns nicely with the browser components work (
> http://dev.chromium.org/developers/design-documents/browser-components
> ). You should look into following that pattern. I'm happy to answer
> questions. Another resource is Erik Wright who is actively working on
> modularizing everything (cc'd).
>
>   -Scott
>
> On Fri, Sep 28, 2012 at 6:51 PM,  <pliard@chromium.org> wrote:
> > I came across another issue. I was assuming that TabRestoreService was
> > inheriting from BaseSessionService only to pull some common
> implementation
> > code.
> > The thing is that BaseSessionService itself inherits from
> > ProfileKeyedService
> > and some other clients do treat TabRestoreService as a
> ProfileKeyedService.
> > That
> > means I can't easily make TabRestoreService a top-level
> interface/abstract
> > class. The goal would have been to avoid using BaseSessionService on
> > Android.
> >
> > I don't see any nice design that could work well in our case. I didn't
> find
> > the
> > dummy Android backend or the NULL pointer approach that bad.
> >
> >
> > What do you think?
> >
> > http://codereview.chromium.org/10989027/
>

[sky, 2012-10-10 20:39:33]

As long as TRS is pure virtual, SGTM. If not, and you're close to done
point me at the patch and I'll take a look.

  -Scott

On Fri, Oct 5, 2012 at 8:36 AM, Philippe Liard <pliard@google.com> wrote:
> I started adding a new PersistentTabRestoreService class inheriting from
> TabRestoreService which I'm stripping to fully decouple it from persistence
> handling. I don't think it is really close to browser components but it
> should address our issue nicely. I was initially mainly concerned about
> splitting TabRestoreService into two "random" pieces (determined according
> to client needs). It seems that with the default tab restore service
> (in-memory only) and the persistent tab restore service we would have two
> meaningful and consistent classes.
> I might find out new issues later during the refactoring. So far I'm pretty
> confident.
>
> How does that sound?
>
>
> On Tue, Oct 2, 2012 at 10:36 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> There is only one method in PKS: Shutdown(). You can expose a similar
>> named method in TabRestoreService that is intended to invoke the
>> method of the same name on the PKS.
>>
>> What you need to do aligns nicely with the browser components work (
>> http://dev.chromium.org/developers/design-documents/browser-components
>> ). You should look into following that pattern. I'm happy to answer
>> questions. Another resource is Erik Wright who is actively working on
>> modularizing everything (cc'd).
>>
>>   -Scott
>>
>> On Fri, Sep 28, 2012 at 6:51 PM,  <pliard@chromium.org> wrote:
>> > I came across another issue. I was assuming that TabRestoreService was
>> > inheriting from BaseSessionService only to pull some common
>> > implementation
>> > code.
>> > The thing is that BaseSessionService itself inherits from
>> > ProfileKeyedService
>> > and some other clients do treat TabRestoreService as a
>> > ProfileKeyedService.
>> > That
>> > means I can't easily make TabRestoreService a top-level
>> > interface/abstract
>> > class. The goal would have been to avoid using BaseSessionService on
>> > Android.
>> >
>> > I don't see any nice design that could work well in our case. I didn't
>> > find
>> > the
>> > dummy Android backend or the NULL pointer approach that bad.
>> >
>> >
>> > What do you think?
>> >
>> > http://codereview.chromium.org/10989027/
>
>

[Philippe, 2012-10-17 16:02:08]

On 2012/10/10 20:39:33, sky wrote:
> As long as TRS is pure virtual, SGTM. If not, and you're close to done
> point me at the patch and I'll take a look.
> 
>   -Scott
> 
> On Fri, Oct 5, 2012 at 8:36 AM, Philippe Liard <mailto:pliard@google.com>
wrote:
> > I started adding a new PersistentTabRestoreService class inheriting from
> > TabRestoreService which I'm stripping to fully decouple it from persistence
> > handling. I don't think it is really close to browser components but it
> > should address our issue nicely. I was initially mainly concerned about
> > splitting TabRestoreService into two "random" pieces (determined according
> > to client needs). It seems that with the default tab restore service
> > (in-memory only) and the persistent tab restore service we would have two
> > meaningful and consistent classes.
> > I might find out new issues later during the refactoring. So far I'm pretty
> > confident.
> >
> > How does that sound?
> >
> >
> > On Tue, Oct 2, 2012 at 10:36 PM, Scott Violet <mailto:sky@chromium.org>
wrote:
> >>
> >> There is only one method in PKS: Shutdown(). You can expose a similar
> >> named method in TabRestoreService that is intended to invoke the
> >> method of the same name on the PKS.
> >>
> >> What you need to do aligns nicely with the browser components work (
> >> http://dev.chromium.org/developers/design-documents/browser-components
> >> ). You should look into following that pattern. I'm happy to answer
> >> questions. Another resource is Erik Wright who is actively working on
> >> modularizing everything (cc'd).
> >>
> >>   -Scott
> >>
> >> On Fri, Sep 28, 2012 at 6:51 PM,  <mailto:pliard@chromium.org> wrote:
> >> > I came across another issue. I was assuming that TabRestoreService was
> >> > inheriting from BaseSessionService only to pull some common
> >> > implementation
> >> > code.
> >> > The thing is that BaseSessionService itself inherits from
> >> > ProfileKeyedService
> >> > and some other clients do treat TabRestoreService as a
> >> > ProfileKeyedService.
> >> > That
> >> > means I can't easily make TabRestoreService a top-level
> >> > interface/abstract
> >> > class. The goal would have been to avoid using BaseSessionService on
> >> > Android.
> >> >
> >> > I don't see any nice design that could work well in our case. I didn't
> >> > find
> >> > the
> >> > dummy Android backend or the NULL pointer approach that bad.
> >> >
> >> >
> >> > What do you think?
> >> >
> >> > http://codereview.chromium.org/10989027/
> >
> >

Hi Scott,

I've uploaded a new patch set which is not ready for review yet. However I would
appreciate if you could take a quick look at it to make sure the general
approach looks reasonable to you.

Cheers,
Philippe.

[Philippe, 2012-10-17 16:48:31]

On 2012/10/17 16:02:08, Philippe wrote:
> On 2012/10/10 20:39:33, sky wrote:
> > As long as TRS is pure virtual, SGTM. If not, and you're close to done
> > point me at the patch and I'll take a look.
> > 
> >   -Scott
> > 
> > On Fri, Oct 5, 2012 at 8:36 AM, Philippe Liard <mailto:pliard@google.com>
> wrote:
> > > I started adding a new PersistentTabRestoreService class inheriting from
> > > TabRestoreService which I'm stripping to fully decouple it from
persistence
> > > handling. I don't think it is really close to browser components but it
> > > should address our issue nicely. I was initially mainly concerned about
> > > splitting TabRestoreService into two "random" pieces (determined according
> > > to client needs). It seems that with the default tab restore service
> > > (in-memory only) and the persistent tab restore service we would have two
> > > meaningful and consistent classes.
> > > I might find out new issues later during the refactoring. So far I'm
pretty
> > > confident.
> > >
> > > How does that sound?
> > >
> > >
> > > On Tue, Oct 2, 2012 at 10:36 PM, Scott Violet <mailto:sky@chromium.org>
> wrote:
> > >>
> > >> There is only one method in PKS: Shutdown(). You can expose a similar
> > >> named method in TabRestoreService that is intended to invoke the
> > >> method of the same name on the PKS.
> > >>
> > >> What you need to do aligns nicely with the browser components work (
> > >> http://dev.chromium.org/developers/design-documents/browser-components
> > >> ). You should look into following that pattern. I'm happy to answer
> > >> questions. Another resource is Erik Wright who is actively working on
> > >> modularizing everything (cc'd).
> > >>
> > >>   -Scott
> > >>
> > >> On Fri, Sep 28, 2012 at 6:51 PM,  <mailto:pliard@chromium.org> wrote:
> > >> > I came across another issue. I was assuming that TabRestoreService was
> > >> > inheriting from BaseSessionService only to pull some common
> > >> > implementation
> > >> > code.
> > >> > The thing is that BaseSessionService itself inherits from
> > >> > ProfileKeyedService
> > >> > and some other clients do treat TabRestoreService as a
> > >> > ProfileKeyedService.
> > >> > That
> > >> > means I can't easily make TabRestoreService a top-level
> > >> > interface/abstract
> > >> > class. The goal would have been to avoid using BaseSessionService on
> > >> > Android.
> > >> >
> > >> > I don't see any nice design that could work well in our case. I didn't
> > >> > find
> > >> > the
> > >> > dummy Android backend or the NULL pointer approach that bad.
> > >> >
> > >> >
> > >> > What do you think?
> > >> >
> > >> > http://codereview.chromium.org/10989027/
> > >
> > >
> 
> Hi Scott,
> 
> I've uploaded a new patch set which is not ready for review yet. However I
would
> appreciate if you could take a quick look at it to make sure the general
> approach looks reasonable to you.
> 
> Cheers,
> Philippe.

Also, I tested this change on Android but still have to test it on Desktop
(including the unit tests).

[sky, 2012-10-17 21:49:31]

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:5: #include <vector>
Make sure you svn cp so that history is preserved.

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:16: class
PersistentTabRestoreService : public BaseSessionService,
Doesn't this result in diamond inheritance? PersistantTabRestoreService gets
ProfileKeyedService from both BaseSessionService and InMemoryTabRestoreService.
Also, I don't like having the two inherit from each other. Refactor the common
code into its' own class and have each instance create and delegate to an
instance.

[Philippe, 2012-10-18 09:41:41]

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:5: #include <vector>
On 2012/10/17 21:49:31, sky wrote:
> Make sure you svn cp so that history is preserved.

I'm using git which doesn't provide a way to keep track of copies/moves other
than detecting similarities (AFAICT). This file is unfortunately too different
from the original one.

I see two options:
- Keep a single CL but make the change in multiple patch sets (e.g. the first
one would copy the file(s)), so that you can easily see changes by diffing
between patch sets. The downside is that we would loose the history at the SCM
level as you pointed out.

- Make a first CL/commit that copies the file(s) without adding them to the GYP
file. Then I can create another CL changing these new files.

What do you think?

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:16: class
PersistentTabRestoreService : public BaseSessionService,
On 2012/10/17 21:49:31, sky wrote:
> Doesn't this result in diamond inheritance? PersistantTabRestoreService gets
> ProfileKeyedService from both BaseSessionService and
InMemoryTabRestoreService.
> Also, I don't like having the two inherit from each other. Refactor the common
> code into its' own class and have each instance create and delegate to an
> instance.

Sure.

[Philippe, 2012-10-18 09:54:06]

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:5: #include <vector>
On 2012/10/18 09:41:41, Philippe wrote:
> On 2012/10/17 21:49:31, sky wrote:
> > Make sure you svn cp so that history is preserved.
> 
> I'm using git which doesn't provide a way to keep track of copies/moves other
> than detecting similarities (AFAICT). This file is unfortunately too different
> from the original one.
> 
> I see two options:
> - Keep a single CL but make the change in multiple patch sets (e.g. the first
> one would copy the file(s)), so that you can easily see changes by diffing
> between patch sets. The downside is that we would loose the history at the SCM
> level as you pointed out.
> 
> - Make a first CL/commit that copies the file(s) without adding them to the
GYP
> file. Then I can create another CL changing these new files.
> 
> What do you think?

I think I was wrong on this point.

git cl upload --similarity=10 seems to work fine. So I will keep uploading patch
sets on this CL.

[Philippe, 2012-10-18 15:26:02]

On 2012/10/18 09:54:06, Philippe wrote:
>
http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
> File chrome/browser/sessions/persistent_tab_restore_service.h (right):
> 
>
http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
> chrome/browser/sessions/persistent_tab_restore_service.h:5: #include <vector>
> On 2012/10/18 09:41:41, Philippe wrote:
> > On 2012/10/17 21:49:31, sky wrote:
> > > Make sure you svn cp so that history is preserved.
> > 
> > I'm using git which doesn't provide a way to keep track of copies/moves
other
> > than detecting similarities (AFAICT). This file is unfortunately too
different
> > from the original one.
> > 
> > I see two options:
> > - Keep a single CL but make the change in multiple patch sets (e.g. the
first
> > one would copy the file(s)), so that you can easily see changes by diffing
> > between patch sets. The downside is that we would loose the history at the
SCM
> > level as you pointed out.
> > 
> > - Make a first CL/commit that copies the file(s) without adding them to the
> GYP
> > file. Then I can create another CL changing these new files.
> > 
> > What do you think?
> 
> I think I was wrong on this point.
> 
> git cl upload --similarity=10 seems to work fine. So I will keep uploading
patch
> sets on this CL.

This should be ready for review now. Note that I will exclude
persistent_tab_restore_service.* from the build on Android once you approved the
CL.
It's still useful for me during the code review to be able to build it when
doing an Android build.

We might have to rename the unit tests to
persistent_tab_restore_service_browsertest.cc to better reflect what they are
testing. What do you think?

[sky, 2012-10-18 17:18:20]

The important thing is history is preserved.

  -Scott

On Thu, Oct 18, 2012 at 2:41 AM,  <pliard@chromium.org> wrote:
>
>
http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
> File chrome/browser/sessions/persistent_tab_restore_service.h (right):
>
>
http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
> chrome/browser/sessions/persistent_tab_restore_service.h:5: #include
> <vector>
> On 2012/10/17 21:49:31, sky wrote:
>>
>> Make sure you svn cp so that history is preserved.
>
>
> I'm using git which doesn't provide a way to keep track of copies/moves
> other than detecting similarities (AFAICT). This file is unfortunately
> too different from the original one.
>
> I see two options:
> - Keep a single CL but make the change in multiple patch sets (e.g. the
> first one would copy the file(s)), so that you can easily see changes by
> diffing between patch sets. The downside is that we would loose the
> history at the SCM level as you pointed out.
>
> - Make a first CL/commit that copies the file(s) without adding them to
> the GYP file. Then I can create another CL changing these new files.
>
> What do you think?
>
>
>
http://codereview.chromium.org/10989027/diff/23001/chrome/browser/sessions/pe...
> chrome/browser/sessions/persistent_tab_restore_service.h:16: class
> PersistentTabRestoreService : public BaseSessionService,
> On 2012/10/17 21:49:31, sky wrote:
>>
>> Doesn't this result in diamond inheritance?
>
> PersistantTabRestoreService gets
>>
>> ProfileKeyedService from both BaseSessionService and
>
> InMemoryTabRestoreService.
>>
>> Also, I don't like having the two inherit from each other. Refactor
>
> the common
>>
>> code into its' own class and have each instance create and delegate to
>
> an
>>
>> instance.
>
>
> Sure.
>
> http://codereview.chromium.org/10989027/

[sky, 2012-10-22 13:51:35]

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/ba...
File chrome/browser/sessions/base_session_service.h (left):

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/ba...
chrome/browser/sessions/base_session_service.h:50: // Deletes the last session.
Why remove this comment?

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/in...
File chrome/browser/sessions/in_memory_tab_restore_service.h (right):

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/in...
chrome/browser/sessions/in_memory_tab_restore_service.h:26: // Tab restore
service that doesn't persist tabs on disk. This is mainly used on
removed mainly.

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/in...
chrome/browser/sessions/in_memory_tab_restore_service.h:34: kMaxEntries = 25,
Why is this in an enum and not a static const?

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/pe...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:16: class
PersistentTabRestoreService : public InMemoryTabRestoreService {
Again, there isn't a need for subclassing. Instead move the common bits of
TabRestoreService into something like TabRestoreServiceHelper that both
implementations use. Doing so is less fragile, easier to test and easier to to
understand what each class needs.

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/ta...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/10989027/diff/27015/chrome/browser/sessions/ta...
chrome/browser/sessions/tab_restore_service.h:178: virtual void
DeleteLastSession() = 0;
Add a comment.

[Philippe, 2012-10-23 15:07:36]

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
File chrome/browser/sessions/base_session_service.h (left):

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
chrome/browser/sessions/base_session_service.h:50: // Deletes the last session.
On 2012/10/22 13:51:35, sky wrote:
> Why remove this comment?

It didn't provide much more information that the method's name wasn't already
providing :)
I added it back anyway.

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
File chrome/browser/sessions/in_memory_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
chrome/browser/sessions/in_memory_tab_restore_service.h:26: // Tab restore
service that doesn't persist tabs on disk. This is mainly used on
On 2012/10/22 13:51:35, sky wrote:
> removed mainly.

Done.

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
chrome/browser/sessions/in_memory_tab_restore_service.h:34: kMaxEntries = 25,
On 2012/10/22 13:51:35, sky wrote:
> Why is this in an enum and not a static const?

Done. This moved to TabRestoreServiceHelper (as a static const).

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.h:16: class
PersistentTabRestoreService : public InMemoryTabRestoreService {
On 2012/10/22 13:51:35, sky wrote:
> Again, there isn't a need for subclassing. Instead move the common bits of
> TabRestoreService into something like TabRestoreServiceHelper that both
> implementations use. Doing so is less fragile, easier to test and easier to to
> understand what each class needs.

Done.

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/27015/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service.h:178: virtual void
DeleteLastSession() = 0;
On 2012/10/22 13:51:35, sky wrote:
> Add a comment.

Done. Let me know if you have a more useful comment.

[sky, 2012-10-24 13:51:25]

This is much closer to what I intended. Thank you!

Does android run tab_restore_service_browsertest.cc ? If not, how about some
tests for in_memory_tab_restore_service.cc .

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/in_memory_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/in_memory_tab_restore_service.h:21: TimeFactory*
time_factory_ = NULL);
no default.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.cc:115: : public
BaseSessionService,
Can we remove ProfileKeyedService as a superclass of BaseSessionService now so
that this delegate isn't a ProfileKeyedService?

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.h:20: TimeFactory*
time_factory = NULL);
not default values.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_factory.cc:44: ProfileKeyedService*
TabRestoreServiceFactory::BuildServiceInstanceFor(
Can you move this to the corresponding service .cc files so that we don't need
an ifdef.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_helper.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:41: class Observer {
Why do we need the observer here? Since the two TabResertoreService
implementations invoke methods directly on this won't they know when these
methods are called?

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:61: TimeFactory*
time_factory_ = NULL);
style guide says no parameters with default values.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:63: virtual
~TabRestoreServiceHelper();
Do you really need virtual?

[Philippe, 2012-10-24 15:42:47]

Thanks Scott!

Indeed, tab_restore_service_browsertest.cc is not built/run on Android.
Would you mind if I address this in a next CL?

This CL has now a pretty high priority on my side since this week is an
upstreaming sprint week for us on Android due to the M24 cut.
Ideally this CL should be submitted by the end of the week.

What do you think?

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/in_memory_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/in_memory_tab_restore_service.h:21: TimeFactory*
time_factory_ = NULL);
On 2012/10/24 13:51:25, sky wrote:
> no default.

Done.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.cc:115: : public
BaseSessionService,
On 2012/10/24 13:51:25, sky wrote:
> Can we remove ProfileKeyedService as a superclass of BaseSessionService now so
> that this delegate isn't a ProfileKeyedService?

Good point. Done. I had to make some small changes in SessionService as you will
see.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.h:20: TimeFactory*
time_factory = NULL);
On 2012/10/24 13:51:25, sky wrote:
> not default values.

Done.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_factory.cc:44: ProfileKeyedService*
TabRestoreServiceFactory::BuildServiceInstanceFor(
On 2012/10/24 13:51:25, sky wrote:
> Can you move this to the corresponding service .cc files so that we don't need
> an ifdef.

Good idea.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_helper.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:41: class Observer {
On 2012/10/24 13:51:25, sky wrote:
> Why do we need the observer here? Since the two TabResertoreService
> implementations invoke methods directly on this won't they know when these
> methods are called?

The names of the methods below might be a bit misleading. These methods of the
observer are actually called at very specific moment (e.g. in the middle) of
TabRestoreServiceHelper methods.

Example:
void TabRestoreServiceHelper::RestoreEntryById(...) {
  // Do something.
  if (observer_)
    observer_->OnRestoreEntryById(...);
  // Do something.
}

This observer wasn't needed before when PersistentTRS was extending InMemoryTRS
(although InMemoryTRS had protected virtual methods like OnClearEntries() that
PersistentTRS was overriding which is similar).
I didn't see any other way to avoid duplicating OnRestoreEntryById()'s code.

What do you think?

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:61: TimeFactory*
time_factory_ = NULL);
On 2012/10/24 13:51:25, sky wrote:
> style guide says no parameters with default values.

Done.

https://chromiumcodereview.appspot.com/10989027/diff/45005/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:63: virtual
~TabRestoreServiceHelper();
On 2012/10/24 13:51:25, sky wrote:
> Do you really need virtual?

Done.

[sky, 2012-10-25 00:45:44]

I think you're almost there.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/in_memory_tab_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/in_memory_tab_restore_service.cc:72: NOTIMPLEMENTED();
NOTIMPLEMENTED doesn't seem right here. I think you want a comment as to why
they do nothing.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.cc:115: class
PersistentTabRestoreService::Delegate
Add description.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.h:19:
PersistentTabRestoreService(Profile* profile,
Document ownership of time_factory (you did a nice job of this in
InMemoryTabRestoreService).

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_browsertest.cc:2: // Use of this
source code is governed by a BSD-style license that can be
This file mostly tests persistant_tab_restore_service, right? It should be
renamed to match that (and peristent_tab_restore_service.h should be the first
include, then newline).

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_factory.cc:8: #include
"chrome/browser/sessions/in_memory_tab_restore_service.h"
Remove these includes.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_helper.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:39: // Provides a way to
the client to add some behavior to the tab restore
to -> for
remove some

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:43: virtual ~Observer();
Move this to protected section since you don't delete through this interface.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:45: virtual void
OnClearEntries();
Can you better document when these are invoked. Additionally newline between
48/49.

[Philippe, 2012-10-25 15:37:07]

Thanks Scott.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/in_memory_tab_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/in_memory_tab_restore_service.cc:72: NOTIMPLEMENTED();
On 2012/10/25 00:45:44, sky wrote:
> NOTIMPLEMENTED doesn't seem right here. I think you want a comment as to why
> they do nothing.

Done.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.cc:115: class
PersistentTabRestoreService::Delegate
On 2012/10/25 00:45:44, sky wrote:
> Add description.

Done. Let me know if you think we can add more details.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/persistent_tab_restore_service.h:19:
PersistentTabRestoreService(Profile* profile,
On 2012/10/25 00:45:44, sky wrote:
> Document ownership of time_factory (you did a nice job of this in
> InMemoryTabRestoreService).

Done.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_browsertest.cc:2: // Use of this
source code is governed by a BSD-style license that can be
On 2012/10/25 00:45:44, sky wrote:
> This file mostly tests persistant_tab_restore_service, right? It should be
> renamed to match that (and peristent_tab_restore_service.h should be the first
> include, then newline).

Right, done. We will see in a next CL how to split it.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_factory.cc:8: #include
"chrome/browser/sessions/in_memory_tab_restore_service.h"
On 2012/10/25 00:45:44, sky wrote:
> Remove these includes.

Done.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_helper.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:39: // Provides a way to
the client to add some behavior to the tab restore
On 2012/10/25 00:45:44, sky wrote:
> to -> for
> remove some

Done.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:43: virtual ~Observer();
On 2012/10/25 00:45:44, sky wrote:
> Move this to protected section since you don't delete through this interface.

Good point.

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:45: virtual void
OnClearEntries();
On 2012/10/25 00:45:44, sky wrote:
> Can you better document when these are invoked. Additionally newline between
> 48/49.

Done. These methods are actually not called at so random places as I initially
thought.

[sky, 2012-10-25 16:21:01]

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_helper.h (right):

https://chromiumcodereview.appspot.com/10989027/diff/61002/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_helper.h:45: virtual void
OnClearEntries();
On 2012/10/25 15:37:07, Philippe wrote:
> On 2012/10/25 00:45:44, sky wrote:
> > Can you better document when these are invoked. Additionally newline between
> > 48/49.
> 
> Done. These methods are actually not called at so random places as I initially
> thought.

If that is the case can we nuke this entirely and have PersistentTabRestore call
these after invoking the necessary method on on TabRestoreServiceHelper?

[sky, 2012-10-25 16:21:28]

And note, if doing as I suggested requires a couple more methods so be. Its a
lot clearer than another delegate interface.

[Philippe, 2012-10-25 16:52:08]

On 2012/10/25 16:21:28, sky wrote:
> And note, if doing as I suggested requires a couple more methods so be. Its a
> lot clearer than another delegate interface.

If I understand correctly, this would mean making these TRSHelper's methods
(AddEntry()...) virtual and more importantly making PersistentTRS::Delegate
subclass TRSHelper (which is not the case right now) in order to override them.
Given that you recommended to stay away from inheritance when it is used as a
way to pull implementation code (and I agree with you) I personally think it
would be cleaner to avoid this extra base class.

I agree that an extra Observer interface adds complexity. I'm just not sure we
can avoid it.

What am I missing?

[sky, 2012-10-25 19:15:46]

On Thu, Oct 25, 2012 at 9:52 AM,  <pliard@chromium.org> wrote:
> On 2012/10/25 16:21:28, sky wrote:
>>
>> And note, if doing as I suggested requires a couple more methods so be.
>> Its a
>> lot clearer than another delegate interface.
>
>
> If I understand correctly, this would mean making these TRSHelper's methods
> (AddEntry()...) virtual and more importantly making PersistentTRS::Delegate
> subclass TRSHelper (which is not the case right now) in order to override
> them.

No, I definitely don't want that. I was thinking more that before/afer
PTRS invokes a method on TRSH it could do whatever additional
processing it needs. But I think you communicated earlier its at
specific points and so doesn't really work well like this.


> Given that you recommended to stay away from inheritance when it is used as
> a
> way to pull implementation code (and I agree with you) I personally think it
> would be cleaner to avoid this extra base class.
>
> I agree that an extra Observer interface adds complexity. I'm just not sure
> we
> can avoid it.
>
> What am I missing?
>
> https://chromiumcodereview.appspot.com/10989027/

[sky, 2012-10-25 19:15:50]

LGTM

[Philippe, 2012-10-26 07:49:39]

On 2012/10/25 19:15:50, sky wrote:
> LGTM

Thanks Scott!

[Philippe, 2012-10-26 11:46:29]

FYI, I've uploaded two new (small) patch sets GYPing out the appropriate TRS
files on Android and non-Android platforms.

[commit-bot: I haz the power, 2012-10-26 11:46:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10989027/80001

[commit-bot: I haz the power, 2012-10-26 15:53:51]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2012-10-26 15:54:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10989027/80001

[commit-bot: I haz the power, 2012-10-26 15:56:24]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-10-29 09:08:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10989027/80001

[commit-bot: I haz the power, 2012-10-29 11:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10989027/80001

[commit-bot: I haz the power, 2012-10-29 11:56:55]

Change committed as 164623