[oilpan] Completely move HTMLFormControlElement's hierarchy to the managed heap

[oilpan] Completely move HTMLFormControlElement's hierarchy to the managed heap

R=vegorov@google.com

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

[haraken, 2013-07-17 11:01:51]

Several tests are still crashing. Not yet for review.

[haraken, 2013-07-17 14:10:23]

PTAL.

Now only one test is failing, which I will fix tomorrow:
accessibility/accessibility-node-memory-management.html

https://codereview.chromium.org/19509003/diff/5001/Source/core/accessibility/...
File Source/core/accessibility/AccessibilitySlider.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/accessibility/...
Source/core/accessibility/AccessibilitySlider.cpp:143: return
Handle<HTMLInputElement>(static_cast<HTMLInputElement*>(m_renderer->node()));

I didn't add a FIXME for Handle<X>(static_cast<X*>(foo)) patterns, because we
will be able to easily get rid of these unnecessary Handle<X>()'s when we move
'foo' to the heap.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLInput...
File Source/core/html/HTMLInputElement.h (left):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLInput...
Source/core/html/HTMLInputElement.h:208: String accept() const;

This conflicts with our accept(). However, fortunately, this accept() is unused,
so we can just remove it.

[Vyacheslav Egorov (Google), 2013-07-18 16:50:08]

LGTM

this change is immense and contains mostly mechanical changes so I am afraid
that I missed something truly important due to its size.

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:341:
Handle<HTMLInputElement>
selectedElement(static_cast<HTMLInputElement*>(r.innerNonSharedNode()));
I wonder if we should be using 

 Handle<HTMLInputElement> selectedElement =
adoptRawResult(static_cast<HTMLInputElemen
     t*>(r.innerNonSharedNode()));

instead.

It will be easier to find later.

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebFormControlElement.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebFormControlElement.cpp:97:
WebFormControlElement::operator Handle<HTMLFormControlElement>() const
I wonder if we want to cast to Handle<T> not to Result<T>?

It seems more natural to cast to Result<T> as this is what we return (similar
how references counting returns PassRefPtr).

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebInputElement.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebInputElement.cpp:273: WebInputElement::operator
Handle<HTMLInputElement>() const
Consider instead:

WebInputElement::operator Result<HTMLInputElement>() const
{
  return adoptRawResult(static_cast<HTMLInputElement*>(m_private.get()));
}

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebSearchableFormData.cpp (left):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebSearchableFormData.cpp:243: HTMLInputElement*
inputElement = selectedInputElement.operator
PassRefPtr<HTMLInputElement>().get();
Why there is an operator to begin with if we are using it in the non operator
syntax?

https://codereview.chromium.org/19509003/diff/5001/Source/core/accessibility/...
File Source/core/accessibility/AccessibilitySlider.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/accessibility/...
Source/core/accessibility/AccessibilitySlider.cpp:143: return
Handle<HTMLInputElement>(static_cast<HTMLInputElement*>(m_renderer->node()));
On 2013/07/17 14:10:23, haraken wrote:
> 
> I didn't add a FIXME for Handle<X>(static_cast<X*>(foo)) patterns, because we
> will be able to easily get rid of these unnecessary Handle<X>()'s when we move
> 'foo' to the heap.

I would use adoptRawResult instead of Handle<>() for such occasions though.

It is an intended use of such function AFAIK

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/BaseButto...
File Source/core/html/BaseButtonInputType.h (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/BaseButto...
Source/core/html/BaseButtonInputType.h:41: BaseButtonInputType(const
Handle<HTMLInputElement>&);
Interesting that you decided to use const reference here while we use pass by
value in other places.

I agree that we should start using const references in more places.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormC...
File Source/core/html/HTMLFormControlElementWithState.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormC...
Source/core/html/HTMLFormControlElementWithState.cpp:50:
oldDocument->formController()->unregisterFormElementWithState(Handle<HTMLFormControlElementWithState>(this));
I wonder if every class should have two methods like this declared:

class T {
private:
  Result<T> self() { return adoptRawResult(this); }
  Result<const T> self() const { return adoptRawResult(this); }
}

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormE...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormE...
Source/core/html/HTMLFormElement.cpp:683: if
(namedItems.find(aliasElement.passRefPtr()) == notFound) {
Any specific reason why you decided to use passRefPtr here instead of .raw()?

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLInput...
File Source/core/html/HTMLInputElement.h (left):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLInput...
Source/core/html/HTMLInputElement.h:208: String accept() const;
On 2013/07/17 14:10:23, haraken wrote:
> 
> This conflicts with our accept(). However, fortunately, this accept() is
unused,
> so we can just remove it.

Nice! With this removed we might be even able to rename acceptHeapVisitor into
accept later.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/InputType.h
File Source/core/html/InputType.h (left):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/InputType...
Source/core/html/InputType.h:70: WTF_MAKE_FAST_ALLOCATED;
Any particular reason why you had to remove this?

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioInpu...
File Source/core/html/RadioInputType.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioInpu...
Source/core/html/RadioInputType.cpp:95: Handle<HTMLInputElement>
inputElement(static_cast<HTMLInputElement*>(node));
Do we want to have HandleScope in this loop?

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioNode...
File Source/core/html/RadioNodeList.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioNode...
Source/core/html/RadioNodeList.cpp:65: Handle<const HTMLInputElement>
inputElement = toRadioButtonInputElement(node);
HandleScope?

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioNode...
Source/core/html/RadioNodeList.cpp:77: Handle<HTMLInputElement> inputElement =
toRadioButtonInputElement(node);
HandleScope? (ditto in other loops).

https://codereview.chromium.org/19509003/diff/5001/Source/core/page/DragContr...
File Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/page/DragContr...
Source/core/page/DragController.cpp:453: m_fileInputElementUnderMouse.clear();
I think we should kill clear() method and just use nullptr assignment.

[haraken, 2013-07-19 02:56:31]

Committed patchset #3 manually as r154555.

[haraken, 2013-07-19 02:57:09]

Thanks for reviewing, Slava!

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:341:
Handle<HTMLInputElement>
selectedElement(static_cast<HTMLInputElement*>(r.innerNonSharedNode()));

Done. I fixed all places.

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebFormControlElement.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebFormControlElement.cpp:97:
WebFormControlElement::operator Handle<HTMLFormControlElement>() const

I replaced the operator overloading with a normal method getValue().

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebInputElement.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebInputElement.cpp:273: WebInputElement::operator
Handle<HTMLInputElement>() const
On 2013/07/18 16:50:08, Vyacheslav Egorov (Google) wrote:
> Consider instead:
> 
> WebInputElement::operator Result<HTMLInputElement>() const
> {
>   return adoptRawResult(static_cast<HTMLInputElement*>(m_private.get()));
> }

Done.

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebSearchableFormData.cpp (left):

https://codereview.chromium.org/19509003/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebSearchableFormData.cpp:243: HTMLInputElement*
inputElement = selectedInputElement.operator
PassRefPtr<HTMLInputElement>().get();
On 2013/07/18 16:50:08, Vyacheslav Egorov (Google) wrote:
> Why there is an operator to begin with if we are using it in the non operator
> syntax?

Done.

https://codereview.chromium.org/19509003/diff/5001/Source/core/accessibility/...
File Source/core/accessibility/AccessibilitySlider.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/accessibility/...
Source/core/accessibility/AccessibilitySlider.cpp:143: return
Handle<HTMLInputElement>(static_cast<HTMLInputElement*>(m_renderer->node()));
On 2013/07/18 16:50:08, Vyacheslav Egorov (Google) wrote:
> On 2013/07/17 14:10:23, haraken wrote:
> > 
> > I didn't add a FIXME for Handle<X>(static_cast<X*>(foo)) patterns, because
we
> > will be able to easily get rid of these unnecessary Handle<X>()'s when we
move
> > 'foo' to the heap.
> 
> I would use adoptRawResult instead of Handle<>() for such occasions though.
> 
> It is an intended use of such function AFAIK

Done.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/BaseButto...
File Source/core/html/BaseButtonInputType.h (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/BaseButto...
Source/core/html/BaseButtonInputType.h:41: BaseButtonInputType(const
Handle<HTMLInputElement>&);

If I use Handle<HTMLInputElement>, it doesn't compile due to circular #include
dependency.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormC...
File Source/core/html/HTMLFormControlElementWithState.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormC...
Source/core/html/HTMLFormControlElementWithState.cpp:50:
oldDocument->formController()->unregisterFormElementWithState(Handle<HTMLFormControlElementWithState>(this));

Agreed. Let me do that in a separate CL.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormE...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/HTMLFormE...
Source/core/html/HTMLFormElement.cpp:683: if
(namedItems.find(aliasElement.passRefPtr()) == notFound) {
On 2013/07/18 16:50:08, Vyacheslav Egorov (Google) wrote:
> Any specific reason why you decided to use passRefPtr here instead of .raw()?

Done.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioInpu...
File Source/core/html/RadioInputType.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioInpu...
Source/core/html/RadioInputType.cpp:95: Handle<HTMLInputElement>
inputElement(static_cast<HTMLInputElement*>(node));
On 2013/07/18 16:50:08, Vyacheslav Egorov (Google) wrote:
> Do we want to have HandleScope in this loop?

Done.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioNode...
File Source/core/html/RadioNodeList.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioNode...
Source/core/html/RadioNodeList.cpp:65: Handle<const HTMLInputElement>
inputElement = toRadioButtonInputElement(node);
On 2013/07/18 16:50:08, Vyacheslav Egorov (Google) wrote:
> HandleScope?

Done.

https://codereview.chromium.org/19509003/diff/5001/Source/core/html/RadioNode...
Source/core/html/RadioNodeList.cpp:77: Handle<HTMLInputElement> inputElement =
toRadioButtonInputElement(node);
On 2013/07/18 16:50:08, Vyacheslav Egorov (Google) wrote:
> HandleScope? (ditto in other loops).

Done.

https://codereview.chromium.org/19509003/diff/5001/Source/core/page/DragContr...
File Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/19509003/diff/5001/Source/core/page/DragContr...
Source/core/page/DragController.cpp:453: m_fileInputElementUnderMouse.clear();

I'm neutral to this. Sometimes we need to return an original value, like this:

  return m_somePersistentHandle.clear();

For consistency, I might want to always use .clear() when clearing persistent
handles.