Bug 39941 - Deploy adoptRef in more places, including all HTML and MathML elements
Summary: Deploy adoptRef in more places, including all HTML and MathML elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-30 22:16 PDT by Darin Adler
Modified: 2010-06-16 17:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (82.45 KB, patch)
2010-05-30 23:08 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (137.62 KB, patch)
2010-05-31 11:31 PDT, Darin Adler
levin: review-
Details | Formatted Diff | Diff
Patch (89.44 KB, patch)
2010-06-13 12:02 PDT, Darin Adler
levin: review+
darin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2010-05-30 22:16:44 PDT
Deploy adoptRef in more places, including all HTML and MathML elements
Comment 1 Darin Adler 2010-05-30 23:08:12 PDT
Created attachment 57436 [details]
Patch
Comment 2 WebKit Review Bot 2010-05-31 00:28:21 PDT
Attachment 57436 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2699075
Comment 3 Darin Adler 2010-05-31 11:31:02 PDT
Created attachment 57486 [details]
Patch
Comment 4 WebKit Review Bot 2010-05-31 12:43:35 PDT
Attachment 57486 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2763019
Comment 5 David Levin 2010-05-31 13:58:15 PDT
Comment on attachment 57486 [details]
Patch

This patch has the fix for 39941 and the fix for 39692, which seems like a mistake.
Comment 6 Darin Adler 2010-05-31 18:22:03 PDT
Darn, I guess I have much to learn about using webkit-patch. Now for some hand editing :-(
Comment 7 Darin Adler 2010-06-13 12:02:46 PDT
Created attachment 58599 [details]
Patch
Comment 8 David Levin 2010-06-13 14:41:04 PDT
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).
Comment 9 Darin Adler 2010-06-16 10:26:50 PDT
(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.
Comment 10 David Levin 2010-06-16 13:19:55 PDT
(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.
Comment 11 Darin Adler 2010-06-16 16:07:50 PDT
Committed r61293: <http://trac.webkit.org/changeset/61293>
Comment 12 Eric Seidel (no email) 2010-06-16 16:34:19 PDT
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.
Comment 13 Darin Adler 2010-06-16 16:38:57 PDT
(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.
Comment 14 Eric Seidel (no email) 2010-06-16 17:46:48 PDT
I must have read the diff wrong.  Thanks.