Oilpan: Replace RefPtrs to Node and its subclasses in core/dom/ with Oilpan transtion types.

Oilpan: Replace RefPtrs to Node and its subclasses in core/dom/ with Oilpan transtion types.

- We need to update RefPtrs not in core/dom/ too because argument types of
  insertBefore/replaceChid/appendChild are changed.

- Member<T> should support implicit conversion to
  RawPtr<SuperClassOfT>.  This is necessary for code like:

    Member<HTMLFooElement> m_foo;
    ....
    parent->appendChild(m_foo);


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

[tkent, 2014-06-04 08:58:46]

Please review this.

This depends on https://codereview.chromium.org/311053003/ and
https://codereview.chromium.org/315843004/ .

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:332: RefPtrWillBeRawPtr<Node> removedChild
ALLOW_UNUSED = oldChild;
This was not buildable without ALLOW_UNUSED.

https://codereview.chromium.org/313813002/diff/1/Source/core/html/HTMLLinkEle...
File Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/html/HTMLLinkEle...
Source/core/html/HTMLLinkElement.cpp:476: RefPtrWillBeRawPtr<Node>
protector(m_owner.get());
RawPtr constructors are ambiguous in this case.

[haraken, 2014-06-04 09:04:04]

LGTM

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:332: RefPtrWillBeRawPtr<Node> removedChild
ALLOW_UNUSED = oldChild;
On 2014/06/04 08:58:45, tkent wrote:
> This was not buildable without ALLOW_UNUSED.

Shall we rename the removeChild to something else? It conflicts with
removeChild() at line 333.

[Mads Ager (chromium), 2014-06-04 09:27:26]

LGTM2

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

https://codereview.chromium.org/313813002/diff/1/Source/wtf/Assertions.h#newc...
Source/wtf/Assertions.h:384: inline bool operator==(const RawPtr<thisType>& a,
const thisType& b) { return a.get() == &b; } \
This is in line with what is being done now, so just ignore this comment. ;-)

I can't resist: I think this is a bad idea. We should not implicitly be taking
the address of something for comparison with a RawPtr. When we use references
and want to compare with a pointer we should do that and explicitly use the '&'
to make that clear. Using references and then hiding the places where what you
really want is a pointer seems like a bad idea that is currently spreading in
the source base.

[zerny-chromium, 2014-06-04 09:28:18]

lgtm for most of it, but I'm not sure where we landed on the issue of
comparison. Did we not end up deciding that it is better to take the address
explicitly when comparing for pointer equality?

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:332: RefPtrWillBeRawPtr<Node> removedChild
ALLOW_UNUSED = oldChild;
On 2014/06/04 08:58:45, tkent wrote:
> This was not buildable without ALLOW_UNUSED.

  RefPtrWillBeRawPtr<Node> removedChild(oldChild);

should be buildable, which is what we have used elsewhere.

https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
File Source/core/inspector/DOMEditor.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
Source/core/inspector/DOMEditor.cpp:312: m_parentNode->replaceChild(m_oldNode,
m_newNode.get(), exceptionState);
can't this just be .get() in both builds?

https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
Source/core/inspector/DOMEditor.cpp:323: m_parentNode->replaceChild(m_newNode,
m_oldNode.get(), exceptionState);
ditto

https://codereview.chromium.org/313813002/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/313813002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:556: operator RawPtr<U>() const { return m_raw; }
Generalizing this operator causes ambiguity since we can then convert a
Member/Persistent to a RawPtr<U> or a U*. Is it strictly needed for this change?
I found that too many protect(foo)'s had to have an added .get() for this to
work...

[zerny-chromium, 2014-06-04 09:30:41]

On 2014/06/04 09:28:18, zerny-chromium wrote:
> lgtm for most of it, but I'm not sure where we landed on the issue of
> comparison. Did we not end up deciding that it is better to take the address
> explicitly when comparing for pointer equality?
> 
>
https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
> File Source/core/dom/ContainerNode.cpp (right):
> 
>
https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
> Source/core/dom/ContainerNode.cpp:332: RefPtrWillBeRawPtr<Node> removedChild
> ALLOW_UNUSED = oldChild;
> On 2014/06/04 08:58:45, tkent wrote:
> > This was not buildable without ALLOW_UNUSED.
> 
>   RefPtrWillBeRawPtr<Node> removedChild(oldChild);
> 
> should be buildable, which is what we have used elsewhere.
> 
>
https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
> File Source/core/inspector/DOMEditor.cpp (right):
> 
>
https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
> Source/core/inspector/DOMEditor.cpp:312: m_parentNode->replaceChild(m_oldNode,
> m_newNode.get(), exceptionState);
> can't this just be .get() in both builds?
> 
>
https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
> Source/core/inspector/DOMEditor.cpp:323: m_parentNode->replaceChild(m_newNode,
> m_oldNode.get(), exceptionState);
> ditto
> 
> https://codereview.chromium.org/313813002/diff/1/Source/platform/heap/Handle.h
> File Source/platform/heap/Handle.h (right):
> 
>
https://codereview.chromium.org/313813002/diff/1/Source/platform/heap/Handle....
> Source/platform/heap/Handle.h:556: operator RawPtr<U>() const { return m_raw;
}
> Generalizing this operator causes ambiguity since we can then convert a
> Member/Persistent to a RawPtr<U> or a U*. Is it strictly needed for this
change?
> I found that too many protect(foo)'s had to have an added .get() for this to
> work...

(of course it compiles for you, so there were not as many Member/Persistent
protects as I though there would be :)

[Erik Corry, 2014-06-04 09:35:47]

lgtm

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:332: RefPtrWillBeRawPtr<Node> removedChild
ALLOW_UNUSED = oldChild;
On 2014/06/04 09:04:04, haraken wrote:
> On 2014/06/04 08:58:45, tkent wrote:
> > This was not buildable without ALLOW_UNUSED.
> 
> Shall we rename the removeChild to something else? It conflicts with
> removeChild() at line 333.

I think you are confusing removedChild and removeChild (with and without a 'd').

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

https://codereview.chromium.org/313813002/diff/1/Source/wtf/Assertions.h#newc...
Source/wtf/Assertions.h:384: inline bool operator==(const RawPtr<thisType>& a,
const thisType& b) { return a.get() == &b; } \
I agree that the need for this indicates we are using references as pointers,
instead of just using pointers as pointers, which would be more natural. That
causes a lot of clumsy code, so we 'fix' the issue one type at a time (in this
case for Node*/Node&) with some rather confusing operators that will invisibly
do pointer comparisons on references.

But that's a general tendency in Blink which will not be changed in this CL, so
this LGTM from the point of view of committing this change.

[tkent, 2014-06-05 00:22:26]

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:332: RefPtrWillBeRawPtr<Node> removedChild
ALLOW_UNUSED = oldChild;
On 2014/06/04 09:35:47, Erik Corry wrote:
> On 2014/06/04 09:04:04, haraken wrote:
> > On 2014/06/04 08:58:45, tkent wrote:
> > > This was not buildable without ALLOW_UNUSED.
> > 
> > Shall we rename the removeChild to something else? It conflicts with
> > removeChild() at line 333.
> 
> I think you are confusing removedChild and removeChild (with and without a
'd').

I removed this part due to https://codereview.chromium.org/319453002/.

https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
File Source/core/inspector/DOMEditor.cpp (right):

https://codereview.chromium.org/313813002/diff/1/Source/core/inspector/DOMEdi...
Source/core/inspector/DOMEditor.cpp:312: m_parentNode->replaceChild(m_oldNode,
m_newNode.get(), exceptionState);
On 2014/06/04 09:28:18, zerny-chromium wrote:
> can't this just be .get() in both builds?

ok, I add .get() and "FIXME: Oilpan"  comment.

https://codereview.chromium.org/313813002/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/313813002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:556: operator RawPtr<U>() const { return m_raw; }
On 2014/06/04 09:28:18, zerny-chromium wrote:
> Generalizing this operator causes ambiguity since we can then convert a
> Member/Persistent to a RawPtr<U> or a U*. Is it strictly needed for this
change?
> I found that too many protect(foo)'s had to have an added .get() for this to
> work...

Without this, we need to add dozens of .get() to code like the following:

    RefPtrWillBeMember<HTMLFooElement> m_foo;
    ....
    parent->appendChild(m_foo);

With this, we need to add just one .get() for HTMLLinkElement.cpp in this CL.
Almost all instances of protect(foo) is not for Member<>.

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

https://codereview.chromium.org/313813002/diff/1/Source/wtf/Assertions.h#newc...
Source/wtf/Assertions.h:384: inline bool operator==(const RawPtr<thisType>& a,
const thisType& b) { return a.get() == &b; } \
As you know, it's off-topic.  I don't think we have project-wide consensus about
this, and I recommend to discuss this on blink-dev if you don't like this.

Anyway, I found this was necessary for just one instance,
ContainerNode::parserInsertBefore. So I removed this macro.

[tkent, 2014-06-05 00:47:02]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-05 00:47:09]

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

[tkent, 2014-06-05 02:26:30]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2014-06-05 02:26:49]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-05 02:26:54]

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

[commit-bot: I haz the power, 2014-06-05 03:01:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10701)

[commit-bot: I haz the power, 2014-06-05 03:03:17]

Change committed as 175522