Summary: | Deploy adoptRef in more places, including all HTML and MathML elements | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
Component: | DOM | Assignee: | Darin Adler <darin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, eric, levin, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Darin Adler
2010-05-30 22:16:44 PDT
Created attachment 57436 [details]
Patch
Attachment 57436 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2699075 Created attachment 57486 [details]
Patch
Attachment 57486 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2763019 Comment on attachment 57486 [details]
Patch
This patch has the fix for 39941 and the fix for 39692, which seems like a mistake.
Darn, I guess I have much to learn about using webkit-patch. Now for some hand editing :-( Created attachment 58599 [details]
Patch
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).
(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. (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. Committed r61293: <http://trac.webkit.org/changeset/61293> 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. (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. I must have read the diff wrong. Thanks. |