Move all clip display item parameters to the constructor

Move all clip display item parameters to the constructor

This patch changes ClipDisplayItem to take all rounded rects in through
the constructor. Because rounded rects are rarer than regular clips,
the rounded rects are now heap allocated. If this becomes a problem
in the future we may want to factor out a RoundedClipsDisplayItem.

No new tests as there should be no change in behavior.

BUG=484943
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197515

[pdr., 2015-06-19 22:07:16]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[pdr., 2015-06-19 22:07:39]

PTAL

The diffs are a little hard to read because this patch depends on
https://codereview.chromium.org/1198583003 which is in the queue.

[chrishtr, 2015-06-19 22:10:17]

https://codereview.chromium.org/1192363005/diff/1/Source/core/paint/BoxClippe...
File Source/core/paint/BoxClipper.cpp (right):

https://codereview.chromium.org/1192363005/diff/1/Source/core/paint/BoxClippe...
Source/core/paint/BoxClipper.cpp:57: roundedRects = adoptPtr(new
Vector<FloatRoundedRect>());
How about putting the vector on the stack, and requiring the argument? Then pass
by value to copy the vector elements.

[pdr., 2015-06-19 23:23:37]

On 2015/06/19 at 22:10:17, chrishtr wrote:
>
https://codereview.chromium.org/1192363005/diff/1/Source/core/paint/BoxClippe...
> File Source/core/paint/BoxClipper.cpp (right):
> 
>
https://codereview.chromium.org/1192363005/diff/1/Source/core/paint/BoxClippe...
> Source/core/paint/BoxClipper.cpp:57: roundedRects = adoptPtr(new
Vector<FloatRoundedRect>());
> How about putting the vector on the stack, and requiring the argument? Then
pass by value to copy the vector elements.

Chris and I discussed this in person. It's hard to do this because Vectors
themselves have a heap allocated arena so we end up doing two mallocs either
way. Because rounded rects aren't the common case, we think this may be the best
approach after all.

PTAL

[chrishtr, 2015-06-19 23:24:34]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-19 23:24:35]

lgtm

[commit-bot: I haz the power, 2015-06-19 23:25:07]

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

[commit-bot: I haz the power, 2015-06-19 23:28:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-19 23:28:25]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59891)

[pdr., 2015-06-19 23:50:21]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-06-19 23:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1192363005/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-19 23:50:39]

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

[commit-bot: I haz the power, 2015-06-20 01:18:42]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197515

[jbroman, 2015-06-29 11:51:40]

On 2015/06/19 23:23:37, pdr wrote:
> On 2015/06/19 at 22:10:17, chrishtr wrote:
> >
>
https://codereview.chromium.org/1192363005/diff/1/Source/core/paint/BoxClippe...
> > File Source/core/paint/BoxClipper.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1192363005/diff/1/Source/core/paint/BoxClippe...
> > Source/core/paint/BoxClipper.cpp:57: roundedRects = adoptPtr(new
> Vector<FloatRoundedRect>());
> > How about putting the vector on the stack, and requiring the argument? Then
> pass by value to copy the vector elements.
> 
> Chris and I discussed this in person. It's hard to do this because Vectors
> themselves have a heap allocated arena so we end up doing two mallocs either
> way. Because rounded rects aren't the common case, we think this may be the
best
> approach after all.
> 
> PTAL

At the risk of retroactive review (sorry): mind clarifying what you mean by
this?
Vector<Element> is analogous to OwnPtr<Element[]>, so having OwnPtr<Vector<Foo>>
seems weirdly indirect to me.

Vector shouldn't heap-allocate a buffer when it is default-constructed and empty
(all it should do is set three fields to zero, IIRC), and Vector::swap lets you
swap out the underlying buffer pointer, size and capacity. The usual (pre-move)
pattern is:

Vector<Foo> foos;
// add foos
new FooConsumer(foos);

fooConsumer::FooConsumer(Vector<Foo>& foos)
{
    m_foos.swap(foos);
}

When there are no foos, this should require only the heap allocation used to
construct FooConsumer.