Oilpan: cleanup based on review comments after removal of TreeShared.

Oilpan: cleanup based on review comments after removal of TreeShared.

There are a couple of comments (in particular SVG stuff) that I
have not addressed. I have filed bug reports for those to make
sure that I get around to revisiting them.

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

[Mads Ager (chromium), 2014-05-07 12:15:05]

Thanks for your comments guys. For reference, the comments that I'm addressing
here are from: https://codereview.chromium.org/262093006

Kouhei, I'll reach out to you when I start looking into the SVG part of this
again. I need to understand the SVG animations better in order to get this
right. :)

[wibling-chromium, 2014-05-07 12:22:11]

lgtm

[zerny-chromium, 2014-05-07 12:31:09]

lgtm

[Mikhail, 2014-05-07 12:34:27]

https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
File Source/wtf/RefPtr.h (right):

https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
operator==(const RefPtr<T>& a, const U& b)
I would not like us to implicitly compare RefPtr with any other object, this
might cause to various bugs (e.g. how would "if (refPtr == 0)" or "if (refPtr ==
nullptr)" work now?)

[Mikhail, 2014-05-07 12:36:29]

On 2014/05/07 12:34:27, mikhail.pozdnyakov wrote:
> https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
> File Source/wtf/RefPtr.h (right):
> 
>
https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
> Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
> operator==(const RefPtr<T>& a, const U& b)
> I would not like us to implicitly compare RefPtr with any other object, this
> might cause to various bugs (e.g. how would "if (refPtr == 0)" or "if (refPtr
==
> nullptr)" work now?)

instead we should do "if (m_refPtr == RefPtr(objectToCompare)) // cast
explicitly"

[Mads Ager (chromium), 2014-05-07 12:40:08]

https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
File Source/wtf/RefPtr.h (right):

https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
operator==(const RefPtr<T>& a, const U& b)
On 2014/05/07 12:34:28, mikhail.pozdnyakov wrote:
> I would not like us to implicitly compare RefPtr with any other object, this
> might cause to various bugs (e.g. how would "if (refPtr == 0)" or "if (refPtr
==
> nullptr)" work now?)

It will not (compile-time error because of ambiguity) and you shouldn't do it
according to the code style anyway. You should do "if (!refPtr)".

There operators were explicitly added in Node.h for (Pass)RefPtr<Node> which
seems strange and therefore I am putting it down here where it belongs if we
want it.

I would honestly prefer that people wrote the explicit '&' when using a
reference:

void foo(Document& doc) {
   if (m_refPtr == &doc)
       doStuff();
}

However, that is not the approach currently taken in the Blink code base.
Instead these comparison operators have been defined for the rather large Node
subset of types.

[haraken, 2014-05-07 12:50:19]

LGTM with questions.

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

https://codereview.chromium.org/267303004/diff/1/Source/core/dom/shadow/Eleme...
Source/core/dom/shadow/ElementShadow.cpp:174:
InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get());

Shall we add a FIXME to this?

https://codereview.chromium.org/267303004/diff/1/Source/core/html/HTMLFormCon...
File Source/core/html/HTMLFormControlElement.cpp (left):

https://codereview.chromium.org/267303004/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.cpp:68: hideVisibleValidationMessage();

tkent-san: I'm not sure if we can remove this. See
https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.cpp
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:549:
WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& instances =
svgRareData()->elementInstances();

Shall we typedef this? We have a lot of 
WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> > in this CL.

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:570: return svgRareData()->elementInstances();

Probably not related to your CL, but I'm not quite sure how this works.

If we don't have SVGRareData, we return a hash set from emptyInstances(). Then
we add things to the hash set. At some point, SVGRareData is created, then we
return a hash set from the SVGRareData. Now we have two hash sets involved.
Won't it cause an issue?

[Mikhail, 2014-05-07 12:57:37]

On 2014/05/07 12:40:08, Mads Ager (chromium) wrote:
> https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
> File Source/wtf/RefPtr.h (right):
> 
>
https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
> Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
> operator==(const RefPtr<T>& a, const U& b)
> On 2014/05/07 12:34:28, mikhail.pozdnyakov wrote:
> > I would not like us to implicitly compare RefPtr with any other object, this
> > might cause to various bugs (e.g. how would "if (refPtr == 0)" or "if
(refPtr
> ==
> > nullptr)" work now?)
> 
> It will not (compile-time error because of ambiguity) and you shouldn't do it
> according to the code style anyway. You should do "if (!refPtr)".
> 
> There operators were explicitly added in Node.h for (Pass)RefPtr<Node> which
> seems strange and therefore I am putting it down here where it belongs if we
> want it.
> 
> I would honestly prefer that people wrote the explicit '&' when using a
> reference:
> 
> void foo(Document& doc) {
>    if (m_refPtr == &doc)
>        doStuff();
> }
> 
> However, that is not the approach currently taken in the Blink code base.
> Instead these comparison operators have been defined for the rather large Node
> subset of types.

IMO implicit address taking is not what people would anticipate comparing
something to a pointer (pointer semantics is broken!)this also brings an
inconsistency: construction from ref is explicit <-> comparison is implicit and
I still believe there is a likelihood of unexpected behavior for example
comparing RefPtr with RawPtr works fine now, and will stop working after the
change..
And finally, what's wrong with "if (m_refPtr == &doc)" (or "if (m_refPtr ==
RefPtr(doc))")? this is so much better..

[Mads Ager (chromium), 2014-05-07 13:35:50]

On 2014/05/07 12:57:37, mikhail.pozdnyakov wrote:
> On 2014/05/07 12:40:08, Mads Ager (chromium) wrote:
> > https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
> > File Source/wtf/RefPtr.h (right):
> > 
> >
>
https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
> > Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
> > operator==(const RefPtr<T>& a, const U& b)
> > On 2014/05/07 12:34:28, mikhail.pozdnyakov wrote:
> > > I would not like us to implicitly compare RefPtr with any other object,
this
> > > might cause to various bugs (e.g. how would "if (refPtr == 0)" or "if
> (refPtr
> > ==
> > > nullptr)" work now?)
> > 
> > It will not (compile-time error because of ambiguity) and you shouldn't do
it
> > according to the code style anyway. You should do "if (!refPtr)".
> > 
> > There operators were explicitly added in Node.h for (Pass)RefPtr<Node> which
> > seems strange and therefore I am putting it down here where it belongs if we
> > want it.
> > 
> > I would honestly prefer that people wrote the explicit '&' when using a
> > reference:
> > 
> > void foo(Document& doc) {
> >    if (m_refPtr == &doc)
> >        doStuff();
> > }
> > 
> > However, that is not the approach currently taken in the Blink code base.
> > Instead these comparison operators have been defined for the rather large
Node
> > subset of types.
> 
> IMO implicit address taking is not what people would anticipate comparing
> something to a pointer (pointer semantics is broken!)this also brings an
> inconsistency: construction from ref is explicit <-> comparison is implicit
and
> I still believe there is a likelihood of unexpected behavior for example
> comparing RefPtr with RawPtr works fine now, and will stop working after the
> change..
> And finally, what's wrong with "if (m_refPtr == &doc)" (or "if (m_refPtr ==
> RefPtr(doc))")? this is so much better..

I agree with you. However, you are wrong that construction is explicit:

ALWAYS_INLINE explicit RefPtr(T& ref) : m_ptr(&ref) { m_ptr->ref(); }

So, references being treated as pointers for RefPtr is already a reality. I'm
not introducing this, it was already there. I totally agree with you: I prefer
both construction and comparison to use '&doc' if 'doc' is a reference. However,
this decision was done a while ago and I was not involved in making it. I'm just
making it work the way it is now. I would be happy if that decision was
reverted, but I find it unrelated to this changelist. :)

[Mads Ager (chromium), 2014-05-07 13:37:38]

On 2014/05/07 13:35:50, Mads Ager (chromium) wrote:
> On 2014/05/07 12:57:37, mikhail.pozdnyakov wrote:
> > On 2014/05/07 12:40:08, Mads Ager (chromium) wrote:
> > > https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
> > > File Source/wtf/RefPtr.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
> > > Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
> > > operator==(const RefPtr<T>& a, const U& b)
> > > On 2014/05/07 12:34:28, mikhail.pozdnyakov wrote:
> > > > I would not like us to implicitly compare RefPtr with any other object,
> this
> > > > might cause to various bugs (e.g. how would "if (refPtr == 0)" or "if
> > (refPtr
> > > ==
> > > > nullptr)" work now?)
> > > 
> > > It will not (compile-time error because of ambiguity) and you shouldn't do
> it
> > > according to the code style anyway. You should do "if (!refPtr)".
> > > 
> > > There operators were explicitly added in Node.h for (Pass)RefPtr<Node>
which
> > > seems strange and therefore I am putting it down here where it belongs if
we
> > > want it.
> > > 
> > > I would honestly prefer that people wrote the explicit '&' when using a
> > > reference:
> > > 
> > > void foo(Document& doc) {
> > >    if (m_refPtr == &doc)
> > >        doStuff();
> > > }
> > > 
> > > However, that is not the approach currently taken in the Blink code base.
> > > Instead these comparison operators have been defined for the rather large
> Node
> > > subset of types.
> > 
> > IMO implicit address taking is not what people would anticipate comparing
> > something to a pointer (pointer semantics is broken!)this also brings an
> > inconsistency: construction from ref is explicit <-> comparison is implicit
> and
> > I still believe there is a likelihood of unexpected behavior for example
> > comparing RefPtr with RawPtr works fine now, and will stop working after the
> > change..
> > And finally, what's wrong with "if (m_refPtr == &doc)" (or "if (m_refPtr ==
> > RefPtr(doc))")? this is so much better..
> 
> I agree with you. However, you are wrong that construction is explicit:
> 
> ALWAYS_INLINE explicit RefPtr(T& ref) : m_ptr(&ref) { m_ptr->ref(); }
> 
> So, references being treated as pointers for RefPtr is already a reality. I'm
> not introducing this, it was already there. I totally agree with you: I prefer
> both construction and comparison to use '&doc' if 'doc' is a reference.
However,
> this decision was done a while ago and I was not involved in making it. I'm
just
> making it work the way it is now. I would be happy if that decision was
> reverted, but I find it unrelated to this changelist. :)

What I mean to say is that the constructor is explicit, but that still means
that you can do:

class A {
public:
   A(B& b) : m_b(b) {}
private:
   RefPtr<B> m_b;
};

So these conversion from B& to B* happen already and the comparison operators
are already there for Nodes.

[Mikhail, 2014-05-07 13:59:01]

On 2014/05/07 13:37:38, Mads Ager (chromium) wrote:
> On 2014/05/07 13:35:50, Mads Ager (chromium) wrote:
> > On 2014/05/07 12:57:37, mikhail.pozdnyakov wrote:
> > > On 2014/05/07 12:40:08, Mads Ager (chromium) wrote:
> > > > https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
> > > > File Source/wtf/RefPtr.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
> > > > Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
> > > > operator==(const RefPtr<T>& a, const U& b)
> > > > On 2014/05/07 12:34:28, mikhail.pozdnyakov wrote:
> > > > > I would not like us to implicitly compare RefPtr with any other
object,
> > this
> > > > > might cause to various bugs (e.g. how would "if (refPtr == 0)" or "if
> > > (refPtr
> > > > ==
> > > > > nullptr)" work now?)
> > > > 
> > > > It will not (compile-time error because of ambiguity) and you shouldn't
do
> > it
> > > > according to the code style anyway. You should do "if (!refPtr)".
> > > > 
> > > > There operators were explicitly added in Node.h for (Pass)RefPtr<Node>
> which
> > > > seems strange and therefore I am putting it down here where it belongs
if
> we
> > > > want it.
> > > > 
> > > > I would honestly prefer that people wrote the explicit '&' when using a
> > > > reference:
> > > > 
> > > > void foo(Document& doc) {
> > > >    if (m_refPtr == &doc)
> > > >        doStuff();
> > > > }
> > > > 
> > > > However, that is not the approach currently taken in the Blink code
base.
> > > > Instead these comparison operators have been defined for the rather
large
> > Node
> > > > subset of types.
> > > 
> > > IMO implicit address taking is not what people would anticipate comparing
> > > something to a pointer (pointer semantics is broken!)this also brings an
> > > inconsistency: construction from ref is explicit <-> comparison is
implicit
> > and
> > > I still believe there is a likelihood of unexpected behavior for example
> > > comparing RefPtr with RawPtr works fine now, and will stop working after
the
> > > change..
> > > And finally, what's wrong with "if (m_refPtr == &doc)" (or "if (m_refPtr
==
> > > RefPtr(doc))")? this is so much better..
> > 
> > I agree with you. However, you are wrong that construction is explicit:
> > 
> > ALWAYS_INLINE explicit RefPtr(T& ref) : m_ptr(&ref) { m_ptr->ref(); }
> > 
> > So, references being treated as pointers for RefPtr is already a reality.
I'm
> > not introducing this, it was already there. I totally agree with you: I
prefer
> > both construction and comparison to use '&doc' if 'doc' is a reference.
> However,
> > this decision was done a while ago and I was not involved in making it. I'm
> just
> > making it work the way it is now. I would be happy if that decision was
> > reverted, but I find it unrelated to this changelist. :)
> 
> What I mean to say is that the constructor is explicit, but that still means
> that you can do:
> 
> class A {
> public:
>    A(B& b) : m_b(b) {}
> private:
>    RefPtr<B> m_b;
> };
> 
> So these conversion from B& to B* happen already and the comparison operators
> are already there for Nodes.

I see. So, think it is better to discuss the RefPtr semantics separately and
solve the weirdness further..
Anyway this should not prevent from this CL landing, lgtm

[tkent, 2014-05-07 14:11:04]

https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
File Source/wtf/RefPtr.h (right):

https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
operator==(const RefPtr<T>& a, const U& b)
I also have a concern about adding the generic version of operator==.
Node is exceptional because
 - WTF_MAKE_NONCOPYABLE
 - Node-comparison is always address-comparison.

We might have a RefCounted class of which operator== should compare its content,
not address.  I don't know if we have such class at this moment.

[Mads Ager (chromium), 2014-05-08 08:02:28]

On 2014/05/07 14:11:04, tkent wrote:
> https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h
> File Source/wtf/RefPtr.h (right):
> 
>
https://codereview.chromium.org/267303004/diff/1/Source/wtf/RefPtr.h#newcode166
> Source/wtf/RefPtr.h:166: template<typename T, typename U> inline bool
> operator==(const RefPtr<T>& a, const U& b)
> I also have a concern about adding the generic version of operator==.
> Node is exceptional because
>  - WTF_MAKE_NONCOPYABLE
>  - Node-comparison is always address-comparison.
> 
> We might have a RefCounted class of which operator== should compare its
content,
> not address.  I don't know if we have such class at this moment.

Everything that is RefCounted is also NONCOPYABLE. It has to be, otherwise you
are copying a ref count but you don't have ref pointers that will decrement the
count for the copy.

Having non-pointer comparison for something that is ref counted is probably
going to be a mess anyway. If you have two RefCounted objects in RefPtrs you can
compare them and that will be pointer comparison. So in order to make sure that
you get the overwritten comparison operator you will have to use the * operator
in any case:

class A() { bool operator==(const A& other) { return true; } };
RefPtr<A> a1 = something;
RefPtr<A> a2 = somethingElse;
a == a2; // false because it uses pointer comparison
*a == *a2; // true

I think there are two options here:

1) remove the RefPtr<A>(A& a) constructor and the operator==(RefPtr<A>, const
A&) operators. When you are dealing with references you should be explicit about
what you want.

2) consistently do what is done for Node now which is to say that if you are
comparing with a RefPtr you *always* do pointer comparisons and therefore the
reference is implicitly pointer converted.

I am strongly in favor of option 1). It seems to me that the constructor and the
equality operators for Node were added in order to not have to write '&' all
over the code. I don't think we should 'hide' references like that.

Anyway, it is not really relevant for this change. I'll do an update to this
changelist where I do not mess with the comparison operators. :-)

[Mads Ager (chromium), 2014-05-08 09:08:58]

Thanks for the comments. I have removed the comparison operator part of this
change. That should be done (or undone) separately from the rest of these
changes.

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

https://codereview.chromium.org/267303004/diff/1/Source/core/dom/shadow/Eleme...
Source/core/dom/shadow/ElementShadow.cpp:174:
InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get());
On 2014/05/07 12:50:20, haraken wrote:
> 
> Shall we add a FIXME to this?

I filed a bug report to track this and will coordinate with the devtools team on
this. I don't think we need a FIXME here as well (an oilpan fixme in code that
is not run in oilpan builds is a little funky.)

https://codereview.chromium.org/267303004/diff/1/Source/core/html/HTMLFormCon...
File Source/core/html/HTMLFormControlElement.cpp (left):

https://codereview.chromium.org/267303004/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.cpp:68: hideVisibleValidationMessage();
On 2014/05/07 12:50:20, haraken wrote:
> 
> tkent-san: I'm not sure if we can remove this. See
>
https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...

Kent confirmed that this can be removed on the other code review where I asked a
similar question.

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.cpp
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:549:
WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& instances =
svgRareData()->elementInstances();
On 2014/05/07 12:50:20, haraken wrote:
> 
> Shall we typedef this? We have a lot of 
> WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> > in this CL.

The issue is that this type is used all over the place and there seems to be no
good place for the typedef...

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:570: return svgRareData()->elementInstances();
On 2014/05/07 12:50:20, haraken wrote:
> 
> Probably not related to your CL, but I'm not quite sure how this works.
> 
> If we don't have SVGRareData, we return a hash set from emptyInstances(). Then
> we add things to the hash set. At some point, SVGRareData is created, then we
> return a hash set from the SVGRareData. Now we have two hash sets involved.
> Won't it cause an issue?

Yeah, unrelated to my change. When you collect element instance you always
create a new set, so the empty set here should never be updated. In any case, if
there is an issue here it is unrelated to this change.

[haraken, 2014-05-08 09:15:40]

LGTM

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.cpp
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/267303004/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:549:
WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& instances =
svgRareData()->elementInstances();
On 2014/05/08 09:08:59, Mads Ager (chromium) wrote:
> On 2014/05/07 12:50:20, haraken wrote:
> > 
> > Shall we typedef this? We have a lot of 
> > WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> > in this CL.
> 
> The issue is that this type is used all over the place and there seems to be
no
> good place for the typedef...

Probably you can define it in SVGElement.h, since the call sites of
WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> > should include
SVGElement.h. If no, I'm OK with not having the typedef.

[Mads Ager (chromium), 2014-05-08 09:17:19]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-05-08 09:17:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/267303004/20001

[commit-bot: I haz the power, 2014-05-08 10:52:38]

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:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/6916)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6660)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6159)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6626)

[commit-bot: I haz the power, 2014-05-08 11:20:34]

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

[commit-bot: I haz the power, 2014-05-08 11:20:35]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[Mads Ager (chromium), 2014-05-08 11:46:57]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-05-08 11:47:45]

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

[commit-bot: I haz the power, 2014-05-08 11:48:01]

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

[commit-bot: I haz the power, 2014-05-08 11:48:02]

Failed to apply patch for Source/core/svg/SVGElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/svg/SVGElement.cpp
  Hunk #1 FAILED at 532.
  Hunk #2 FAILED at 546.
  Hunk #3 succeeded at 783 (offset -2 lines).
  Hunk #4 succeeded at 803 (offset -2 lines).
  Hunk #5 succeeded at 816 (offset -2 lines).
  Hunk #6 succeeded at 833 (offset -2 lines).
  Hunk #7 succeeded at 1039 (offset -2 lines).
  2 out of 7 hunks FAILED -- saving rejects to file
Source/core/svg/SVGElement.cpp.rej

Patch:       Source/core/svg/SVGElement.cpp
Index: Source/core/svg/SVGElement.cpp
diff --git a/Source/core/svg/SVGElement.cpp b/Source/core/svg/SVGElement.cpp
index
254df4ec29e49a72ccb6c87193e4a04448401e6a..ca65b9030feea5cedc54dbe65d0b6b386d9af307
100644
--- a/Source/core/svg/SVGElement.cpp
+++ b/Source/core/svg/SVGElement.cpp
@@ -532,7 +532,7 @@ void SVGElement::mapInstanceToElement(SVGElementInstance*
instance)
 {
     ASSERT(instance);
 
-    HashSet<SVGElement*>& instances = ensureSVGRareData()->elementInstances();
+    WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& instances =
ensureSVGRareData()->elementInstances();
     ASSERT(!instances.contains(instance->shadowTreeElement()));
 
     instances.add(instance->shadowTreeElement());
@@ -546,18 +546,27 @@ void SVGElement::removeInstanceMapping(SVGElementInstance*
instance)
     if (!instance->shadowTreeElement())
         return;
 
-    HashSet<SVGElement*>& instances = svgRareData()->elementInstances();
+    WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& instances =
svgRareData()->elementInstances();
     ASSERT(instances.contains(instance->shadowTreeElement()));
 
     instances.remove(instance->shadowTreeElement());
 }
 
-const HashSet<SVGElement*>& SVGElement::instancesForElement() const
+static WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& emptyInstances()
 {
-    if (!hasSVGRareData()) {
-        DEFINE_STATIC_LOCAL(HashSet<SVGElement*>, emptyInstances, ());
-        return emptyInstances;
-    }
+#if ENABLE(OILPAN)
+    DEFINE_STATIC_LOCAL(Persistent<HeapHashSet<WeakMember<SVGElement> > >,
emptyInstances, (new HeapHashSet<WeakMember<SVGElement> >));
+    return *emptyInstances;
+#else
+    DEFINE_STATIC_LOCAL(HashSet<RawPtr<SVGElement> >, emptyInstances, ());
+    return emptyInstances;
+#endif
+}
+
+const WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >&
SVGElement::instancesForElement() const
+{
+    if (!hasSVGRareData())
+        return emptyInstances();
     return svgRareData()->elementInstances();
 }
 
@@ -785,7 +794,7 @@ bool SVGElement::haveLoadedRequiredResources()
     return true;
 }
 
-static inline void collectInstancesForSVGElement(SVGElement* element,
HashSet<SVGElement*>& instances)
+static inline void collectInstancesForSVGElement(SVGElement* element,
WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& instances)
 {
     ASSERT(element);
     if (element->containingShadowRoot())
@@ -805,10 +814,10 @@ bool SVGElement::addEventListener(const AtomicString&
eventType, PassRefPtr<Even
         return false;
 
     // Add event listener to all shadow tree DOM element instances
-    HashSet<SVGElement*> instances;
+    WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> > instances;
     collectInstancesForSVGElement(this, instances);
-    const HashSet<SVGElement*>::const_iterator end = instances.end();
-    for (HashSet<SVGElement*>::const_iterator it = instances.begin(); it !=
end; ++it) {
+    const WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement>
>::const_iterator end = instances.end();
+    for (WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >::const_iterator
it = instances.begin(); it != end; ++it) {
         bool result = (*it)->Node::addEventListener(eventType, listener,
useCapture);
         ASSERT_UNUSED(result, result);
     }
@@ -818,7 +827,7 @@ bool SVGElement::addEventListener(const AtomicString&
eventType, PassRefPtr<Even
 
 bool SVGElement::removeEventListener(const AtomicString& eventType,
EventListener* listener, bool useCapture)
 {
-    HashSet<SVGElement*> instances;
+    WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> > instances;
     collectInstancesForSVGElement(this, instances);
     if (instances.isEmpty())
         return Node::removeEventListener(eventType, listener, useCapture);
@@ -835,8 +844,8 @@ bool SVGElement::removeEventListener(const AtomicString&
eventType, EventListene
         return false;
 
     // Remove event listener from all shadow tree DOM element instances
-    const HashSet<SVGElement*>::const_iterator end = instances.end();
-    for (HashSet<SVGElement*>::const_iterator it = instances.begin(); it !=
end; ++it) {
+    const WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement>
>::const_iterator end = instances.end();
+    for (WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >::const_iterator
it = instances.begin(); it != end; ++it) {
         SVGElement* shadowTreeElement = *it;
         ASSERT(shadowTreeElement);
 
@@ -1041,13 +1050,13 @@ void SVGElement::invalidateInstances()
     if (instanceUpdatesBlocked())
         return;
 
-    const HashSet<SVGElement*>& set = instancesForElement();
+    const WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >& set =
instancesForElement();
     if (set.isEmpty())
         return;
 
     // Mark all use elements referencing 'element' for rebuilding
-    const HashSet<SVGElement*>::const_iterator end = set.end();
-    for (HashSet<SVGElement*>::const_iterator it = set.begin(); it != end;
++it) {
+    const WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement>
>::const_iterator end = set.end();
+    for (WillBeHeapHashSet<RawPtrWillBeWeakMember<SVGElement> >::const_iterator
it = set.begin(); it != end; ++it) {
         (*it)->setCorrespondingElement(0);
 
         if (SVGUseElement* element = (*it)->correspondingUseElement()) {

[Mads Ager (chromium), 2014-05-08 12:03:38]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-05-08 12:04:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/267303004/80001

[commit-bot: I haz the power, 2014-05-08 13:18:28]

Change committed as 173645