Handling |SessionCommand|s as scoped_ptr's

Handling |SessionCommand|s as scoped_ptr's

BUG=424679
TEST=covered by existing tests

Committed: https://crrev.com/57b5b03f18152d605ee13ad1f54422e7d83dd7f5
Cr-Commit-Position: refs/heads/master@{#302216}

[Mr4D (OOO till 08-26), 2014-10-30 22:25:32]

skuhne@chromium.org changed reviewers:
+ sky@chromium.org

[Mr4D (OOO till 08-26), 2014-10-30 22:25:33]

Please have a look!

[sky, 2014-10-30 22:51:07]

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.cc:96:
ScopedVector<SessionCommand>* passed = new ScopedVector<SessionCommand>();
ScopedVector supports move semantics. Can't you supply pending_commands_ to the
constructor and have it effectively inline what you have here? Additionally,
because it supports move semantics there should be no need for a pointer
anymore. ScopedVector should work.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.h:110: ScopedVector<SessionCommand>
 pending_commands_;
You've got two spaces here.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_backend_unittest.cc (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_backend_unittest.cc:31: // Create a new
SessionCommands structure and swap existing commands into them.
Same comment about move semantics.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_backend_unittest.cc:67:
DCHECK(commands.empty());
ASSERT_TRUE.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_service_commands.h (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service_commands.h:75:
scoped_ptr<SessionCommand>* command,
out type params should be last.

[Mr4D (OOO till 08-26), 2014-10-30 23:31:21]

Please have another look!

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.cc:96:
ScopedVector<SessionCommand>* passed = new ScopedVector<SessionCommand>();
Aha. ScopedVector.passed() works. Interesting. Done.

For the base::Bind however... I tried various things to use a scoped_ptr and it
did not work. Bind is apparently doing some assigns which would need to use
.Pass which they do not.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.h:110: ScopedVector<SessionCommand>
 pending_commands_;
On 2014/10/30 22:51:07, sky wrote:
> You've got two spaces here.

Done.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_backend_unittest.cc (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_backend_unittest.cc:31: // Create a new
SessionCommands structure and swap existing commands into them.
On 2014/10/30 22:51:07, sky wrote:
> Same comment about move semantics.

Done.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_backend_unittest.cc:67:
DCHECK(commands.empty());
On 2014/10/30 22:51:07, sky wrote:
> ASSERT_TRUE.

Done.

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_service_commands.h (right):

https://codereview.chromium.org/685133004/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service_commands.h:75:
scoped_ptr<SessionCommand>* command,
On 2014/10/30 22:51:07, sky wrote:
> out type params should be last.

Done.

[sky, 2014-10-31 02:34:36]

LGTM

[Mr4D (OOO till 08-26), 2014-10-31 03:17:54]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-31 03:18:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685133004/20001

[commit-bot: I haz the power, 2014-10-31 04:00:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-31 04:00:34]

Patchset 2 (id:??) landed as
https://crrev.com/57b5b03f18152d605ee13ad1f54422e7d83dd7f5
Cr-Commit-Position: refs/heads/master@{#302216}