Convert SessionService to use new CancelableTaskTracker

Convert SessionService to use new CancelableTaskTracker

BUG=155883
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172418

[marja, 2012-12-06 08:21:58]

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
chrome/browser/sessions/session_restore.cc:716: void
OnGotSession(ScopedVector<SessionWindow> windows,
Is it possible to avoid copying the ScopedVector when passing the param?

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
File chrome/browser/sessions/session_service.h (right):

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
chrome/browser/sessions/session_service.h:277: ScopedVector<SessionCommand>
commands);
(Here too)

[kaiwang, 2012-12-06 18:07:52]

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
chrome/browser/sessions/session_restore.cc:716: void
OnGotSession(ScopedVector<SessionWindow> windows,
no.. ScopedVector is a non-copyable type like scoped_ptr. So the only way to put
it in Callback is declare like this. I think this is a good thing, the callback
writer knows clearly all the SessionWindows are owned here.

The only member of ScopedVector is an std::vector, and Pass() uses
vector::swap(). So the construction is very cheap.

[kaiwang, 2012-12-06 18:29:31]

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
chrome/browser/sessions/session_restore.cc:716: void
OnGotSession(ScopedVector<SessionWindow> windows,
Oh, I think I was wrong. It's possible to Bind a 
"const ScopedVector<...>&"
Although I don't feel this makes much sense...

But the case of this CL, SessionCallbacks can own SessionWindows (see old
comment in session_service.h), so the ScopedVector must be modifiable.

[kaiwang, 2012-12-10 18:14:57]

ping sky@ and marja@

[marja, 2012-12-10 18:22:56]

(I deliberately didn't add myself as a reviewer :)

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
chrome/browser/sessions/session_restore.cc:716: void
OnGotSession(ScopedVector<SessionWindow> windows,
On 2012/12/06 18:29:31, kaiwang wrote:
> Oh, I think I was wrong. It's possible to Bind a 
> "const ScopedVector<...>&"
> Although I don't feel this makes much sense...
> 
> But the case of this CL, SessionCallbacks can own SessionWindows (see old
> comment in session_service.h), so the ScopedVector must be modifiable.

Hmm, so this copy probably gets ScopedVector(RValue other)
called, and it does the swap, so the original ScopedVector is emptied, and
nothing disastrous happens. Ok.

[sky, 2012-12-10 22:11:58]

Marja, you started to look at this, any chance you could finish it?

  -Scott

On Mon, Dec 10, 2012 at 10:22 AM,  <marja@chromium.org> wrote:
> (I deliberately didn't add myself as a reviewer :)
>
>
>
>
https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
> File chrome/browser/sessions/session_restore.cc (right):
>
>
https://codereview.chromium.org/11446033/diff/1/chrome/browser/sessions/sessi...
> chrome/browser/sessions/session_restore.cc:716: void
> OnGotSession(ScopedVector<SessionWindow> windows,
> On 2012/12/06 18:29:31, kaiwang wrote:
>>
>> Oh, I think I was wrong. It's possible to Bind a
>> "const ScopedVector<...>&"
>> Although I don't feel this makes much sense...
>
>
>> But the case of this CL, SessionCallbacks can own SessionWindows (see
>
> old
>>
>> comment in session_service.h), so the ScopedVector must be modifiable.
>
>
> Hmm, so this copy probably gets ScopedVector(RValue other)
> called, and it does the swap, so the original ScopedVector is emptied,
> and nothing disastrous happens. Ok.
>
> https://codereview.chromium.org/11446033/

[marja, 2012-12-10 22:16:38]

Hi,

Ok, I can have a look at this, but not today (it's getting late here in UTC+1).
Sorry for the extra delay!

[marja, 2012-12-11 10:31:07]

lgtm

[marja, 2012-12-11 10:34:18]

(Removing sky from reviewer list, I think his intention was to delegate the
reviewing to me, so no need to wait for his review.)

[commit-bot: I haz the power, 2012-12-11 18:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11446033/1

[commit-bot: I haz the power, 2012-12-11 21:51:32]

Change committed as 172418