Remove ScopedVector from prioritized_dispatcher_unittest

Remove ScopedVector from prioritized_dispatcher_unittest

ScopedVector is deprecated.

BUG=554289
Committed: https://crrev.com/da99b3860fd79989d0e1a82bbe503ce422a202f3
Cr-Commit-Position: refs/heads/master@{#365788}

[Olli Raula, 2015-12-08 08:04:19]

Description was changed from

==========
Simplify prioritized_dispatcher_unittest and remove ScopedVector

Remove ScopedVector as it is unused and store TestJob in stack
as we do not need pointers anywhere.

BUG=554289
==========

to

==========
Simplify prioritized_dispatcher_unittest and remove ScopedVector

Remove ScopedVector as it is unused and we can store TestJob in stack
because we do not need pointers anywhere.

BUG=554289
==========

[Olli Raula, 2015-12-08 08:06:53]

olli.raula@intel.com changed reviewers:
+ ttuttle@chromium.org

[Olli Raula, 2015-12-08 08:07:12]

Could you review?

On 2015/12/08 08:06:53, Olli Raula wrote:
> mailto:olli.raula@intel.com changed reviewers:
> + mailto:ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-12-08 16:21:19]

On 2015/12/08 08:07:12, Olli Raula wrote:
> Could you review?

Hi Olli,

I don't think your change works. You create a TestJob inside AddJob (allocated
on the stack), call Add on it (which stores a pointer to it in the dispatcher),
then return from AddJob, destroying the TestJob you created and returning a
*copy*. You now have an invalid pointer inside the dispatcher.

If you'd like to get rid of the ScopedVector, you could use individual
scoped_ptrs for each TestJob instead, but that would be harder to read.

What is your motivation for changing this test?

Thanks,

ttuttle

[Olli Raula, 2015-12-09 07:32:26]

Description was changed from

==========
Simplify prioritized_dispatcher_unittest and remove ScopedVector

Remove ScopedVector as it is unused and we can store TestJob in stack
because we do not need pointers anywhere.

BUG=554289
==========

to

==========
ScopedVector from prioritized_dispatcher_unittest

BUG=554289
==========

[Olli Raula, 2015-12-10 07:36:11]

Hi, my motivation was remove ScopedVector and then I tried to simplify it more.
Now it only changes ScopedVector to std::vector<scoped_ptr>>.

Thanks

[Deprecated (see juliatuttle), 2015-12-10 17:54:24]

On 2015/12/10 07:36:11, Olli Raula wrote:
> Hi, my motivation was remove ScopedVector and then I tried to simplify it
more.
> Now it only changes ScopedVector to std::vector<scoped_ptr>>.
> 
> Thanks

But why do you want to remove ScopedVector?

Also, a std::vector<scoped_ptr> is worse than a ScopedVector; it does the same
thing, but requires the awkward std::move every time you add elements. I meant
using scoped_ptr inside each of the test cases, so they're deallocated when the
test case finishes instead of when the test fixture object is destroyed.

[Olli Raula, 2015-12-11 07:41:48]

> But why do you want to remove ScopedVector?

You did not check the issue what I linked? ScopedVector is Deprecated,
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
You can also see my commit history, I have bunch of these already committed.

[Olli Raula, 2015-12-11 14:31:42]

Description was changed from

==========
ScopedVector from prioritized_dispatcher_unittest

BUG=554289
==========

to

==========
Remove ScopedVector from prioritized_dispatcher_unittest

BUG=554289
==========

[Olli Raula, 2015-12-15 07:04:51]

Could you please review this?

[Olli Raula, 2015-12-15 14:03:33]

On 2015/12/15 07:04:51, Olli Raula wrote:
> Could you please review this?

And apologize my too direct words

[Deprecated (see juliatuttle), 2015-12-15 15:15:10]

On 2015/12/15 14:03:33, Olli Raula wrote:
> On 2015/12/15 07:04:51, Olli Raula wrote:
> > Could you please review this?
> 
> And apologize my too direct words

No worries -- sorry it dropped off my radar!

I'd like a couple changes:

1. Please note that ScopedVector is deprecated in the change description, so
it's obvious to anyone who sees it why you made the change.

2. Please move the scoped_ptr storage of the TestJobs into the test cases
themselves. Right now, it appears that the test cases leak the TestJobs without
a careful reading of the underlying class. (That is, have AddJob return a
scoped_ptr<TestJob>, and declare the jobs as scoped_ptr<TestJob> in each of the
test cases.)

[Olli Raula, 2015-12-16 08:17:25]

Description was changed from

==========
Remove ScopedVector from prioritized_dispatcher_unittest

BUG=554289
==========

to

==========
Remove ScopedVector from prioritized_dispatcher_unittest

ScopedVector is deprecated.

BUG=554289
==========

[Deprecated (see juliatuttle), 2015-12-16 20:28:24]

lgtm if you fix the remaining nits.

https://codereview.chromium.org/1510513002/diff/60001/net/base/prioritized_di...
File net/base/prioritized_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1510513002/diff/60001/net/base/prioritized_di...
net/base/prioritized_dispatcher_unittest.cc:6: #include <string>
This is no longer needed, since you're not storing a vector in the test fixture
at all.

https://codereview.chromium.org/1510513002/diff/60001/net/base/prioritized_di...
net/base/prioritized_dispatcher_unittest.cc:144: make_scoped_ptr(new
TestJob(dispatcher_.get(), data, priority, &log_)));
You don't need make_scoped_ptr here; scoped_ptr has a constructor that accepts
(and takes ownership of) a raw pointer.

https://codereview.chromium.org/1510513002/diff/60001/net/base/prioritized_di...
net/base/prioritized_dispatcher_unittest.cc:151: make_scoped_ptr(new
TestJob(dispatcher_.get(), data, priority, &log_)));
Ditto.

[Olli Raula, 2015-12-17 08:27:25]

The CQ bit was checked by olli.raula@intel.com

[Olli Raula, 2015-12-17 08:27:25]

The patchset sent to the CQ was uploaded after l-g-t-m from ttuttle@chromium.org
Link to the patchset: https://codereview.chromium.org/1510513002/#ps80001
(title: "fixes")

[commit-bot: I haz the power, 2015-12-17 08:27:36]

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

[commit-bot: I haz the power, 2015-12-17 08:55:18]

Description was changed from

==========
Remove ScopedVector from prioritized_dispatcher_unittest

ScopedVector is deprecated.

BUG=554289
==========

to

==========
Remove ScopedVector from prioritized_dispatcher_unittest

ScopedVector is deprecated.

BUG=554289
==========

[commit-bot: I haz the power, 2015-12-17 08:55:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-17 08:56:03]

Description was changed from

==========
Remove ScopedVector from prioritized_dispatcher_unittest

ScopedVector is deprecated.

BUG=554289
==========

to

==========
Remove ScopedVector from prioritized_dispatcher_unittest

ScopedVector is deprecated.

BUG=554289

Committed: https://crrev.com/da99b3860fd79989d0e1a82bbe503ce422a202f3
Cr-Commit-Position: refs/heads/master@{#365788}
==========

[commit-bot: I haz the power, 2015-12-17 08:56:04]

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