Bug 39941 – Deploy adoptRef in more places, including all HTML and MathML elements

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Deploy adoptRef in more places, including all HTML and MathML elements

[Darin Adler]

Created attachment 57436 [details]
Patch

[WebKit Review Bot]

Attachment 57436 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2699075

[Darin Adler]

Created attachment 57486 [details]
Patch

[WebKit Review Bot]

Attachment 57486 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2763019

[David Levin]

Comment on attachment 57486 [details]
Patch

This patch has the fix for 39941 and the fix for 39692, which seems like a mistake.

[Darin Adler]

Darn, I guess I have much to learn about using webkit-patch. Now for some hand editing :-(

[Darin Adler]

Created attachment 58599 [details]
Patch

[David Levin]

Comment on attachment 58599 [details]
Patch

WebCore/editing/ModifySelectionListLevel.cpp:218
 +      return command->m_listElement;
(I know it is the same code as before but) It would be nice to make this "return command->m_listElement.release();"


WebCore/mathml/MathMLMathElement.cpp:47
 +  RenderObject* MathMLMathElement::createRenderer(RenderArena* arena, RenderStyle* style)
I don't really understand why this change was done.

Also, I don't know what this has to do with "Deploy adoptRef in more place". (I suspect it was just some additional clean up. I suspect that if it is worth doing, then most of the MathML*Element classes should have this done and that could be a change by itself.)

In short, feel free to leave this in the change (but it would be nice to have the "Why?" answered).

[Darin Adler]

(In reply to comment #8)
> WebCore/mathml/MathMLMathElement.cpp:47
>  +  RenderObject* MathMLMathElement::createRenderer(RenderArena* arena, RenderStyle* style)
> I don't really understand why this change was done.
> 
> Also, I don't know what this has to do with "Deploy adoptRef in more place". (I suspect it was just some additional clean up. I suspect that if it is worth doing, then most of the MathML*Element classes should have this done and that could be a change by itself.)

The other MathML elements don’t have their own unique element subclass, so they need to be handled in the MathMLInlineContainerElement. But this element does have a subclass, so there’s no need to have the base class createRenderer support it.

This is not critically important to this patch, and you’re right that I could have left it out or could land it separately.

[David Levin]

(In reply to comment #9)
> (In reply to comment #8)
> > WebCore/mathml/MathMLMathElement.cpp:47
> >  +  RenderObject* MathMLMathElement::createRenderer(RenderArena* arena, RenderStyle* style)
> > I don't really understand why this change was done.
> > 
> > Also, I don't know what this has to do with "Deploy adoptRef in more place". (I suspect it was just some additional clean up. I suspect that if it is worth doing, then most of the MathML*Element classes should have this done and that could be a change by itself.)
> 
> The other MathML elements don’t have their own unique element subclass, so they need to be handled in the MathMLInlineContainerElement. But this element does have a subclass, so there’s no need to have the base class createRenderer support it.
> 
> This is not critically important to this patch, and you’re right that I could have left it out or could land it separately.

Thanks for the info! (As I mentioned before) Feel free to leave it in the patch.

[Darin Adler]

Committed r61293: <http://trac.webkit.org/changeset/61293>

[Eric Seidel (no email)]

This must have broken MathML builds:

  virtual createRenderer(RenderArena*, RenderStyle*);
in trunk/WebCore/mathml/MathMLMathElement.h 

and in trunk/WebCore/mathml/MathMLMathElement.h 

Otherwise LGTM too.

[Darin Adler]

(In reply to comment #12)
> in trunk/WebCore/mathml/MathMLMathElement.h 
> and in trunk/WebCore/mathml/MathMLMathElement.h 

Those are both the same file. I fixed it, but couldn't find a second case.

[Eric Seidel (no email)]

I must have read the diff wrong.  Thanks.