Oilpan: add transition types to shadow DOM supporting objects.

Oilpan: add transition types to shadow DOM supporting objects.

R=haraken@chromium.org,ager@chromium.org
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174084

[sof, 2014-05-12 12:07:01]

Please take a look.

[haraken, 2014-05-12 12:21:37]

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Conte...
File Source/core/dom/shadow/ContentDistribution.h (right):

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Conte...
Source/core/dom/shadow/ContentDistribution.h:42: ALLOW_ONLY_INLINE_ALLOCATION();

This should be DISALLOW_ALLOCATION, since ContentDistribution is a part of
InsertionPoint.

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Conte...
Source/core/dom/shadow/ContentDistribution.h:66: void
traceAfterDispatch(Visitor*);

Then you can simply use trace().

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Eleme...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Eleme...
Source/core/dom/shadow/ElementShadow.cpp:53: WillBeHeapVector<bool, 32>
m_distributed;

I'm not sure of this. Do we want to move vectors of primitive types to the heap
now?

IMHO, I'd like to keep vectors of primitive types (especially Vector<String>) as
is. At the moment, our goal is to ship oilpan for the Node & CSS hierarchy as
soon as possible, so we might want to leave other things for later.

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Shado...
File Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Shado...
Source/core/dom/shadow/ShadowRoot.cpp:314: DEFINE_STATIC_LOCAL(const
Vector<RefPtr<InsertionPoint> >, emptyList, ());

I'd prefer:

DEFINE_STATIC_LOCAL(WillBePersistentHeapVector<RefPtrWillBeMember<IntersionPoint>>,
emptyList, ());

and avoid the #if branches.

[sof, 2014-05-12 12:37:48]

Awesome, thanks for the (quick) review.

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Conte...
File Source/core/dom/shadow/ContentDistribution.h (right):

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Conte...
Source/core/dom/shadow/ContentDistribution.h:42: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/12 12:21:38, haraken wrote:
> 
> This should be DISALLOW_ALLOCATION, since ContentDistribution is a part of
> InsertionPoint.

Yes, got my DISALLOW_ALLOCATION() vs ALLOW_ONLY_INLINE_ALLOCATION() mixed up
here also.

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Eleme...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Eleme...
Source/core/dom/shadow/ElementShadow.cpp:53: WillBeHeapVector<bool, 32>
m_distributed;
On 2014/05/12 12:21:38, haraken wrote:
> 
> I'm not sure of this. Do we want to move vectors of primitive types to the
heap
> now?
> 
> IMHO, I'd like to keep vectors of primitive types (especially Vector<String>)
as
> is. At the moment, our goal is to ship oilpan for the Node & CSS hierarchy as
> soon as possible, so we might want to leave other things for later.

Yes, let's not do this.

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Shado...
File Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/277213004/diff/2/Source/core/dom/shadow/Shado...
Source/core/dom/shadow/ShadowRoot.cpp:314: DEFINE_STATIC_LOCAL(const
Vector<RefPtr<InsertionPoint> >, emptyList, ());
On 2014/05/12 12:21:38, haraken wrote:
> 
> I'd prefer:
> 
>
DEFINE_STATIC_LOCAL(WillBePersistentHeapVector<RefPtrWillBeMember<IntersionPoint>>,
> emptyList, ());
> 
> and avoid the #if branches.

Oh, nice - that will work here. Switched.

[haraken, 2014-05-12 13:38:35]

LGTM

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/C...
File Source/core/dom/shadow/ContentDistribution.cpp (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/C...
Source/core/dom/shadow/ContentDistribution.cpp:51:
WillBeHeapHashMap<RawPtrWillBeMember<Node>, size_t>::const_iterator it =
m_indices.find(const_cast<Node*>(node));

It would be great if we don't need const_cast.

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/C...
File Source/core/dom/shadow/ContentDistribution.h (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/C...
Source/core/dom/shadow/ContentDistribution.h:70:
WillBeHeapHashMap<RawPtrWillBeMember<Node>, size_t> m_indices;

Shall we keep const?

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:243:
NodeToDestinationInsertionPoints::const_iterator it =
m_nodeToInsertionPoints.find(const_cast<Node*>(key));

Remove const_cast.

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.h:92: typedef
WillBeHeapHashMap<RawPtrWillBeMember<Node>, DestinationInsertionPoints>
NodeToDestinationInsertionPoints;

Ditto. We want to keep const.

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/I...
File Source/core/dom/shadow/InsertionPoint.cpp (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/I...
Source/core/dom/shadow/InsertionPoint.cpp:250: m_distribution.trace(visitor);

visitor->trace(m_distribution);

should work.

[sof, 2014-05-12 21:57:19]

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:243:
NodeToDestinationInsertionPoints::const_iterator it =
m_nodeToInsertionPoints.find(const_cast<Node*>(key));
On 2014/05/12 13:38:35, haraken wrote:
> 
> Remove const_cast.

It was added for a reason. i.e., you run into resolution trouble without it, not
being able to convert a (const Node*) into a (Node*) when trying to convert the
incoming const pointer to the needed RawPtrWillBeMember<Node>. Trying to address
that by adding more RawPtr<> or Member<> constructors over const pointers is a
road to ruin.

If you instead use "const RawPtrWillBeMember<Node>" as the key type, that const
type cannot be handled by DefaultHash<> nor HashTraits<> readily.

This is the smallest I change I could come up with that worked. If you insist, I
can look into this some more.

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/I...
File Source/core/dom/shadow/InsertionPoint.cpp (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/I...
Source/core/dom/shadow/InsertionPoint.cpp:250: m_distribution.trace(visitor);
On 2014/05/12 13:38:35, haraken wrote:
> 
> visitor->trace(m_distribution);
> 
> should work.

Cute, wasn't aware of that overload.

[haraken, 2014-05-13 05:49:50]

LGTM with a comment about const_cast.

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:243:
NodeToDestinationInsertionPoints::const_iterator it =
m_nodeToInsertionPoints.find(const_cast<Node*>(key));

Probably I'm misunderstanding, but won't the following work?

- Make m_nodeToInsertionPoint WillBeHeapHashMap<RawPtrWillBeMember<const Node>,
DestinationInsertionPoints>.

- Don't use const_cast here.

- Add RawPtr<> and Member<> constructors that does the conversion from const T*
to RawPtr<const T*> and Member<const T*>.

[Mads Ager (chromium), 2014-05-13 15:01:58]

LGTM2

I agree with Haraken's comments on using Member<const Node> where you used to
have a "const Node*".

[sof, 2014-05-15 10:25:42]

On 2014/05/13 05:49:50, haraken wrote:
> LGTM with a comment about const_cast.
> 
>
https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
> File Source/core/dom/shadow/ElementShadow.cpp (right):
> 
>
https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
> Source/core/dom/shadow/ElementShadow.cpp:243:
> NodeToDestinationInsertionPoints::const_iterator it =
> m_nodeToInsertionPoints.find(const_cast<Node*>(key));
> 
> Probably I'm misunderstanding, but won't the following work?
> 
> - Make m_nodeToInsertionPoint WillBeHeapHashMap<RawPtrWillBeMember<const
Node>,
> DestinationInsertionPoints>.
> 
> - Don't use const_cast here.
> 
> - Add RawPtr<> and Member<> constructors that does the conversion from const
T*
> to RawPtr<const T*> and Member<const T*>.

I've pushed the const into the type argument to Member<> (it doesn't read like a
const pointer, but it really is.) Does that address the above?

[Mads Ager (chromium), 2014-05-15 11:00:13]

On 2014/05/15 10:25:42, sof wrote:
> On 2014/05/13 05:49:50, haraken wrote:
> > LGTM with a comment about const_cast.
> > 
> >
>
https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
> > File Source/core/dom/shadow/ElementShadow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/277213004/diff/30001/Source/core/dom/shadow/E...
> > Source/core/dom/shadow/ElementShadow.cpp:243:
> > NodeToDestinationInsertionPoints::const_iterator it =
> > m_nodeToInsertionPoints.find(const_cast<Node*>(key));
> > 
> > Probably I'm misunderstanding, but won't the following work?
> > 
> > - Make m_nodeToInsertionPoint WillBeHeapHashMap<RawPtrWillBeMember<const
> Node>,
> > DestinationInsertionPoints>.
> > 
> > - Don't use const_cast here.
> > 
> > - Add RawPtr<> and Member<> constructors that does the conversion from const
> T*
> > to RawPtr<const T*> and Member<const T*>.
> 
> I've pushed the const into the type argument to Member<> (it doesn't read like
a
> const pointer, but it really is.) Does that address the above?

Yes, it does. LGTM

[sof, 2014-05-15 13:15:07]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-15 13:15:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/277213004/50001

[commit-bot: I haz the power, 2014-05-15 14:01:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6777)

[commit-bot: I haz the power, 2014-05-15 14:39:55]

Change committed as 174084