Bug 39941

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.