[Oilpan]: Move CSSPropertySourceData, InspectorStyleProperty, CSSStyleSourceData, and CSSRuleSource…

[Oilpan]: Move CSSPropertySourceData, InspectorStyleProperty, CSSStyleSourceData, and CSSRuleSourceData to the oilpan heap using transition types.

Also removed unsused emptyCSSPropertySourceData (adding apavlov as reviewer to verify).

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=341815
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170550

[haraken, 2014-03-28 14:15:00]

https://codereview.chromium.org/216553004/diff/1/Source/core/css/CSSPropertyS...
File Source/core/css/CSSPropertySourceData.h (right):

https://codereview.chromium.org/216553004/diff/1/Source/core/css/CSSPropertyS...
Source/core/css/CSSPropertySourceData.h:92: struct CSSRuleSourceData : public
RefCountedWillBeGarbageCollectedFinalized<CSSRuleSourceData> {

Can this be RefCountedWillBeGarbageCollected?

https://codereview.chromium.org/216553004/diff/1/Source/core/css/CSSPropertyS...
Source/core/css/CSSPropertySourceData.h:154: static const bool
canInitializeWithMemset = true;

Why don't you need:

  static const bool canMoveWithMemcpy = true;

?

https://codereview.chromium.org/216553004/diff/1/Source/core/css/parser/Bison...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/216553004/diff/1/Source/core/css/parser/Bison...
Source/core/css/parser/BisonCSSParser.h:85: BisonCSSParser(Document*);

Add explicit to these constructors.

https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorStyleSheet.h (right):

https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorStyleSheet.h:104: struct InspectorStyleProperty {

Again, I want to understand why we don't need to add a VectorTrait for this. In
my understanding, a VectorTrait is not for optimization but is needed to avoid
InspectorStyleProperty's destructors from being called multiple times.

Erik would know better.

[wibling-chromium, 2014-03-31 09:38:27]

Thanks for the review!

https://codereview.chromium.org/216553004/diff/1/Source/core/css/CSSPropertyS...
File Source/core/css/CSSPropertySourceData.h (right):

https://codereview.chromium.org/216553004/diff/1/Source/core/css/CSSPropertyS...
Source/core/css/CSSPropertySourceData.h:92: struct CSSRuleSourceData : public
RefCountedWillBeGarbageCollectedFinalized<CSSRuleSourceData> {
On 2014/03/28 14:15:00, haraken wrote:
> 
> Can this be RefCountedWillBeGarbageCollected?

Yes, done.

https://codereview.chromium.org/216553004/diff/1/Source/core/css/CSSPropertyS...
Source/core/css/CSSPropertySourceData.h:154: static const bool
canInitializeWithMemset = true;
On 2014/03/28 14:15:00, haraken wrote:
> 
> Why don't you need:
> 
>   static const bool canMoveWithMemcpy = true;
> 
> ?

We cannot use memcpy to copy the CSSPropertySourceData class as it contains two
string. When canMoveWithMemcpy is false the move/copy will be done by using the
copy constructor, so it is purely an optimization.

https://codereview.chromium.org/216553004/diff/1/Source/core/css/parser/Bison...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/216553004/diff/1/Source/core/css/parser/Bison...
Source/core/css/parser/BisonCSSParser.h:85: BisonCSSParser(Document*);
On 2014/03/28 14:15:00, haraken wrote:
> 
> Add explicit to these constructors.

Done.

https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorStyleSheet.h (right):

https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorStyleSheet.h:104: struct InspectorStyleProperty {
On 2014/03/28 14:15:00, haraken wrote:
> 
> Again, I want to understand why we don't need to add a VectorTrait for this.
In
> my understanding, a VectorTrait is not for optimization but is needed to avoid
> InspectorStyleProperty's destructors from being called multiple times.
> 
> Erik would know better.

I will let Erik answer this, but it should be a pure optimization AFAIK.

[Erik Corry, 2014-03-31 09:55:53]

https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorStyleSheet.h (right):

https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorStyleSheet.h:104: struct InspectorStyleProperty {
On 2014/03/28 14:15:00, haraken wrote:
> 
> Again, I want to understand why we don't need to add a VectorTrait for this.
In
> my understanding, a VectorTrait is not for optimization but is needed to avoid
> InspectorStyleProperty's destructors from being called multiple times.
> 
> Erik would know better.

It's not really a correctness issue (not having the trait will not cause
incorrect code), it's a compilability issue (not having the trait should cause a
compile time assert).  I can't work out why we don't need the trait here, we may
have accidentally disabled a compile time assert (we will look into it after
lunch).

In this case the struct is initializable with memset and calling a destructor on
memsetted memory is safe (the strings have refptrs in them and it's OK to
destruct a null refptr).

[haraken, 2014-03-31 10:35:57]

With that VectorTrait issue addressed, LGTM.

I want to understand:

- What happens if we forget to specify {canInitializeWithMemset,
canMoveWithMemcpy} on objects which can be {initialized with memset, be moved
with memcpy}, and then use the objects in an on-heap vector?

- What happens if we don't specify {canInitializeWithMemset, canMoveWithMemcpy}
on objects which cannot be {initialized with memset, be moved with memcpy}, and
then use the objects in an on-heap vector?

[wibling-chromium, 2014-03-31 10:38:45]

On 2014/03/31 09:55:53, Erik Corry wrote:
>
https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
> File Source/core/inspector/InspectorStyleSheet.h (right):
> 
>
https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
> Source/core/inspector/InspectorStyleSheet.h:104: struct InspectorStyleProperty
{
> On 2014/03/28 14:15:00, haraken wrote:
> > 
> > Again, I want to understand why we don't need to add a VectorTrait for this.
> In
> > my understanding, a VectorTrait is not for optimization but is needed to
avoid
> > InspectorStyleProperty's destructors from being called multiple times.
> > 
> > Erik would know better.
> 
> It's not really a correctness issue (not having the trait will not cause
> incorrect code), it's a compilability issue (not having the trait should cause
a
> compile time assert).  I can't work out why we don't need the trait here, we
may
> have accidentally disabled a compile time assert (we will look into it after
> lunch).
> 
> In this case the struct is initializable with memset and calling a destructor
on
> memsetted memory is safe (the strings have refptrs in them and it's OK to
> destruct a null refptr).

It turned out that when we have a vector where elements are only added we never
hit the compile time assert. I have now moved the assert into the vector's
constructors and also added the trait to InspectorStyleProperty.

[wibling-chromium, 2014-03-31 10:40:43]

On 2014/03/31 10:38:45, wibling-chromium wrote:
> On 2014/03/31 09:55:53, Erik Corry wrote:
> >
>
https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
> > File Source/core/inspector/InspectorStyleSheet.h (right):
> > 
> >
>
https://codereview.chromium.org/216553004/diff/1/Source/core/inspector/Inspec...
> > Source/core/inspector/InspectorStyleSheet.h:104: struct
InspectorStyleProperty
> {
> > On 2014/03/28 14:15:00, haraken wrote:
> > > 
> > > Again, I want to understand why we don't need to add a VectorTrait for
this.
> > In
> > > my understanding, a VectorTrait is not for optimization but is needed to
> avoid
> > > InspectorStyleProperty's destructors from being called multiple times.
> > > 
> > > Erik would know better.
> > 
> > It's not really a correctness issue (not having the trait will not cause
> > incorrect code), it's a compilability issue (not having the trait should
cause
> a
> > compile time assert).  I can't work out why we don't need the trait here, we
> may
> > have accidentally disabled a compile time assert (we will look into it after
> > lunch).
> > 
> > In this case the struct is initializable with memset and calling a
destructor
> on
> > memsetted memory is safe (the strings have refptrs in them and it's OK to
> > destruct a null refptr).
> 
> It turned out that when we have a vector where elements are only added we
never
> hit the compile time assert. I have now moved the assert into the vector's
> constructors and also added the trait to InspectorStyleProperty.

I will not submit the change to Vector with this change as it seems to require
fixing quite a few other places where we did not have the VectorTraits
overriden.

[Erik Corry, 2014-03-31 10:50:09]

On 2014/03/31 10:35:57, haraken wrote:
> With that VectorTrait issue addressed, LGTM.
> 
> I want to understand:
> 
> - What happens if we forget to specify {canInitializeWithMemset,
> canMoveWithMemcpy} on objects which can be {initialized with memset, be moved
> with memcpy}, and then use the objects in an on-heap vector?

You have to either set canInitializeWithMemset or unset needsDestruction. 
Otherwise you get a compile time failure for heap vectors (Gustav is fixing this
so it's always true)
If you forget to specify canMoveWithMemcpy you get slower performance.

> - What happens if we don't specify {canInitializeWithMemset,
canMoveWithMemcpy}
> on objects which cannot be {initialized with memset, be moved with memcpy},
and
> then use the objects in an on-heap vector?

Same thing really. The traits don't change the behaviour of the generated code,
only control whether you can generate the code or you get a compiler failure.

[Erik Corry, 2014-03-31 10:51:05]

On 2014/03/31 10:50:09, Erik Corry wrote:
> On 2014/03/31 10:35:57, haraken wrote:
> > With that VectorTrait issue addressed, LGTM.
> > 
> > I want to understand:
> > 
> > - What happens if we forget to specify {canInitializeWithMemset,
> > canMoveWithMemcpy} on objects which can be {initialized with memset, be
moved
> > with memcpy}, and then use the objects in an on-heap vector?
> 
> You have to either set canInitializeWithMemset or unset needsDestruction. 
> Otherwise you get a compile time failure for heap vectors (Gustav is fixing
this
> so it's always true)
> If you forget to specify canMoveWithMemcpy you get slower performance.
> 
> > - What happens if we don't specify {canInitializeWithMemset,
> canMoveWithMemcpy}
> > on objects which cannot be {initialized with memset, be moved with memcpy},
> and
> > then use the objects in an on-heap vector?
> 
> Same thing really. The traits don't change the behaviour of the generated
code,
> only control whether you can generate the code or you get a compiler failure.

Sorry, that last sentence applied to canInitializeWithMemset, not
canMoveWithMemcpy.

[wibling-chromium, 2014-03-31 10:55:39]

PTAL

[haraken, 2014-03-31 11:24:36]

LGTM

https://codereview.chromium.org/216553004/diff/20001/Source/core/css/CSSPrope...
File Source/core/css/CSSPropertySourceData.h (right):

https://codereview.chromium.org/216553004/diff/20001/Source/core/css/CSSPrope...
Source/core/css/CSSPropertySourceData.h:154: static const bool
canInitializeWithMemset = true;

Would you add a comment about why canMoveWithMemcpy cannot be set?

https://codereview.chromium.org/216553004/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorStyleSheet.h (right):

https://codereview.chromium.org/216553004/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorStyleSheet.h:318: static const bool
canInitializeWithMemset = true;

Ditto.

[haraken, 2014-03-31 11:24:54]

On 2014/03/31 10:50:09, Erik Corry wrote:
> On 2014/03/31 10:35:57, haraken wrote:
> > With that VectorTrait issue addressed, LGTM.
> > 
> > I want to understand:
> > 
> > - What happens if we forget to specify {canInitializeWithMemset,
> > canMoveWithMemcpy} on objects which can be {initialized with memset, be
moved
> > with memcpy}, and then use the objects in an on-heap vector?
> 
> You have to either set canInitializeWithMemset or unset needsDestruction. 
> Otherwise you get a compile time failure for heap vectors (Gustav is fixing
this
> so it's always true)
> If you forget to specify canMoveWithMemcpy you get slower performance.
> 
> > - What happens if we don't specify {canInitializeWithMemset,
> canMoveWithMemcpy}
> > on objects which cannot be {initialized with memset, be moved with memcpy},
> and
> > then use the objects in an on-heap vector?
> 
> Same thing really. The traits don't change the behaviour of the generated
code,
> only control whether you can generate the code or you get a compiler failure.

Makes sense, thanks for the detailed clarification!

[Erik Corry, 2014-03-31 11:34:31]

lgtm

[zerny-chromium, 2014-03-31 12:08:04]

lgtm

[wibling-chromium, 2014-04-01 10:30:13]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-04-01 10:30:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/216553004/40001

[commit-bot: I haz the power, 2014-04-01 11:38:50]

Change committed as 170550