Currently, a lot of changes in DOM tree is done with: 1. WillDoSomething() calls DoSomething(). 2. DoSomething() creates transaction(s) and call DoTransaction(). 3. Transaction::DoTransaction() calls DoSomethingImpl() etc. I think that DoSomething() should be named as "DoSomethingWithTransaction()". Then, DoSomethingImpl() should be named as "DoSomething()". Then, when you reading the callers, it's clearer if it causes changing undo/redo stack.

DoSomethingImpl() should be renamed to DoDoSomething(), perhaps.

These patches: * do not change XPCOM methods for compatibility. * do not append WithTransaction postfix for TextEditRules and HTMLEditRules since their purpose is handling edit actions (i.e., should change the DOM tree with transactions). * do not change some method names which takes |bool aSupporessTransaction| to chose if using transaction or not. * do not change some method names in table editor, absolutely positioned editor, style editor etc, which are not so important for now. * change pointer arguments to reference arguments as far as possible to avoid nullptr check. * replace NS_ENSURE_* with if statement with NS_WARN_IF() as far as possible. * add explanation of each method which touches with each patch. The number of patches is too many. So, it's okay to give priority to Makoto-san's work and land your smaller patches before this especially when you need to uplift them.

Comment on attachment 8968778 [details] Bug 1451672 - part 1: Rename EditorBase::CreateNode() to EditorBase::CreateNodeWithTransaction() https://reviewboard.mozilla.org/r/237472/#review243614

Comment on attachment 8968779 [details] Bug 1451672 - part 2: Rename EditorBase::InsertNode() to EditorBase::InsertNodeWithTransaction() https://reviewboard.mozilla.org/r/237474/#review243636

Comment on attachment 8968780 [details] Bug 1451672 - part 3: Rename EditorBase::SplitNode() and related methods to ending with "WithTransaction" https://reviewboard.mozilla.org/r/237476/#review243650

Comment on attachment 8968781 [details] Bug 1451672 - part 4: Rename EditorBase::SplitNodeImpl() to EditorBase::DoSplitNode() https://reviewboard.mozilla.org/r/237478/#review243656

Comment on attachment 8968782 [details] Bug 1451672 - part 5: Rename EditorBase::JoinNodes() and related methods with "WithTransaction" postfix https://reviewboard.mozilla.org/r/237480/#review243664

Comment on attachment 8968783 [details] Bug 1451672 - part 6: Rename EditorBase::JoinNodesImpl() to EditorBase::DoJoinNodes() https://reviewboard.mozilla.org/r/237482/#review243666

Comment on attachment 8968784 [details] Bug 1451672 - part 7: Rename EditorBase::DeleteNode() to EditorBase::DeleteNodeWithTransaction() https://reviewboard.mozilla.org/r/237484/#review243668

Comment on attachment 8968785 [details] Bug 1451672 - part 8: Rename EditorBase::DeleteText() to EditorBase::DeleteTextWithTransaction() https://reviewboard.mozilla.org/r/237486/#review244030

Comment on attachment 8968786 [details] Bug 1451672 - part 9: Create TextEditor::DeleteSelectionAsAction() as implementation of nsIEditor::DeleteSelection() https://reviewboard.mozilla.org/r/237488/#review244032 ::: editor/libeditor/HTMLEditRules.cpp:1395 (Diff revision 1) > // must faster to do it once here than to track all > // the changes one at a time. > AutoLockListener lockit(&mListenerEnabled); > > // don't change my selection in subtransactions > NS_ENSURE_STATE(mHTMLEditor); This lie is unnecessary since you don't use mHTMLEditor at next line.

Comment on attachment 8968787 [details] Bug 1451672 - part 10: Rename TextEditor::DeleteSelectionImpl() to TextEditor::DeleteSelectionWithTransaction() https://reviewboard.mozilla.org/r/237490/#review244042

Comment on attachment 8968788 [details] Bug 1451672 - part 11: Rename EditorBase::SetAttribute(), EditorBase::RemoveAttribute() and EditorBase::CloneAttribute() with "WithTransaction" postfix https://reviewboard.mozilla.org/r/237492/#review244050 ::: editor/libeditor/HTMLEditor.cpp:2785 (Diff revision 1) > - if (element) { > + RefPtr<nsAtom> bgColorAtom = NS_Atomize("bgcolor"); > + if (domElement) { > if (selectedCount > 0) { > // Traverse all selected cells > - nsCOMPtr<nsIDOMElement> cell; > - rv = GetFirstSelectedCell(nullptr, getter_AddRefs(cell)); > + nsCOMPtr<nsIDOMElement> domCell; > + rv = GetFirstSelectedCell(nullptr, getter_AddRefs(domCell)); Ah, I have to add Element version GetFirstSelection instead of nsIDOMElement. ::: editor/libeditor/HTMLEditor.cpp:2785 (Diff revision 1) > - if (element) { > + RefPtr<nsAtom> bgColorAtom = NS_Atomize("bgcolor"); > + if (domElement) { > if (selectedCount > 0) { > // Traverse all selected cells > - nsCOMPtr<nsIDOMElement> cell; > - rv = GetFirstSelectedCell(nullptr, getter_AddRefs(cell)); > + nsCOMPtr<nsIDOMElement> domCell; > + rv = GetFirstSelectedCell(nullptr, getter_AddRefs(domCell)); Ah, we have to add Element version GetFirstSelection instead of nsIDOMElement. ::: editor/libeditor/HTMLTableEditor.cpp:171 (Diff revision 1) > + return NS_ERROR_INVALID_ARG; > + } > nsAutoString newSpan; > newSpan.AppendInt(aColSpan, 10); > - return SetAttribute(aCell, NS_LITERAL_STRING("colspan"), newSpan); > + RefPtr<nsAtom> colSpanAtom = NS_Atomize("colspan"); > + return SetAttributeWithTransaction(*cell, *colSpanAtom, newSpan); return SetAttributeWithTransaction(*cell, *nsGkAtoms::colspan, newSpan); ::: editor/libeditor/HTMLTableEditor.cpp:188 (Diff revision 1) > + return NS_ERROR_INVALID_ARG; > + } > nsAutoString newSpan; > newSpan.AppendInt(aRowSpan, 10); > - return SetAttribute(aCell, NS_LITERAL_STRING("rowspan"), newSpan); > + RefPtr<nsAtom> rowSpanAtom = NS_Atomize("rowspan"); > + return SetAttributeWithTransaction(*cell, *rowSpanAtom, newSpan); SetAttributeWithTransation(*cell, *nsGkAtoms::rowspan, newSpan); ::: editor/libeditor/HTMLTableEditor.cpp:1795 (Diff revision 1) > + if (NS_WARN_IF(!destCell)) { > + return NS_ERROR_INVALID_ARG; > + } > > - // Copy backgournd color to new cell > - NS_NAMED_LITERAL_STRING(bgcolor, "bgcolor"); > + // Copy backgournd color to new cell. > + RefPtr<nsAtom> bgColorAtom = NS_Atomize("bgcolor"); remove this line ::: editor/libeditor/HTMLTableEditor.cpp:1807 (Diff revision 1) > return rv; > } > if (!isSet) { > return NS_OK; > } > - return SetAttribute(destCell, bgcolor, color); > + return SetAttributeWithTransaction(*destCell, *bgColorAtom, color); SetAttributeWithTransaction(*destCell, *nsGkAtoms::bgcolor, color);

Comment on attachment 8968789 [details] Bug 1451672 - part 12: Create HTMLEditor::RemoveStyleSheetWithTransaction() as implementation of nsIEditorStyleSheets::RemoveStyleSheet() https://reviewboard.mozilla.org/r/237494/#review244052

Comment on attachment 8968790 [details] Bug 1451672 - part 13: Rename EditorBase::InsertTextImpl() and EditorBase::InsertTextIntoTextNodeImpl() to EditorBase::InsertTextWithTransaction() and EditorBase::InsertTextIntoTextNodeWithTransaction() https://reviewboard.mozilla.org/r/237496/#review244054

Comment on attachment 8968791 [details] Bug 1451672 - part 14: Rename EditorBase::ReplaceContainer() to EditorBase::ReplaceContainerWithTransactionInternal() and create some wrappers of it https://reviewboard.mozilla.org/r/237498/#review244064

Comment on attachment 8968792 [details] Bug 1451672 - part 15: Rename EditorBase::RemoveContainer() and HTMLEditor::RemoveBlockContainer() with "WithTransaction" postfix and make their argument |Element&| https://reviewboard.mozilla.org/r/237500/#review244070

Comment on attachment 8968786 [details] Bug 1451672 - part 9: Create TextEditor::DeleteSelectionAsAction() as implementation of nsIEditor::DeleteSelection() https://reviewboard.mozilla.org/r/237488/#review244032 > This lie is unnecessary since you don't use mHTMLEditor at next line. Oh, good catch!

Comment on attachment 8968793 [details] Bug 1451672 - part 16: Rename EditorBase::MoveNode() to EditorBase::MoveNodeWithTransaction() and create EditorBase::MoveNodeToEndWithTransaction() https://reviewboard.mozilla.org/r/237502/#review244078

Comment on attachment 8968794 [details] Bug 1451672 - part 17: Rename EditorBase::InsertContainerAbove() to EditorBase::InsertContainerWithTransactionInternal() and wraps it with new inline methods, EditorBase::InsertContainerWithTransaction() https://reviewboard.mozilla.org/r/237504/#review244080

Comment on attachment 8968795 [details] Bug 1451672 - part 18: Rename EditorBase::CloneAttributes() to EditorBase::CloneAttributesWithTransaction() https://reviewboard.mozilla.org/r/237506/#review244082 ::: editor/libeditor/EditorBase.cpp:2552 (Diff revision 1) > */ > NS_IMETHODIMP > -EditorBase::CloneAttributes(nsIDOMNode* aDest, > - nsIDOMNode* aSource) > +EditorBase::CloneAttributes(nsIDOMNode* aDestDOMElement, > + nsIDOMNode* aSourceDOMElement) > { > - NS_ENSURE_TRUE(aDest && aSource, NS_ERROR_NULL_POINTER); > + if (NS_WARN_IF(!aDestDOMElement) || NS_WARN_IF(!aSourceDOMElement)) { nits: It may be uncessary to cehck aDest and aSource because you check it again after do_QueryInterface(). do_QueryInterface can accept nullptr parameter.

Comment on attachment 8968795 [details] Bug 1451672 - part 18: Rename EditorBase::CloneAttributes() to EditorBase::CloneAttributesWithTransaction() https://reviewboard.mozilla.org/r/237506/#review244082 > nits: It may be uncessary to cehck aDest and aSource because you check it again after do_QueryInterface(). do_QueryInterface can accept nullptr parameter. s/uncessary/unnecessary/

Comment on attachment 8968796 [details] Bug 1451672 - part 19: Remove TextEditor::CreateBR() and rename TextEditor::CreateBRImpl() to TextEditor::InsertBrElementWithTransaction() https://reviewboard.mozilla.org/r/237508/#review244092

Comment on attachment 8968797 [details] Bug 1451672 - part 20: Rename HTMLEditor::MakeDefinitionItem() and HTMLEditor::InsertBasicBlock() with "WithTransaction" postfix https://reviewboard.mozilla.org/r/237510/#review244094 ::: editor/libeditor/HTMLEditor.cpp:2148 (Diff revision 1) > - NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER); > + if (NS_WARN_IF(!selection)) { > + return NS_ERROR_FAILURE; > + } > + nsDependentAtomString tagName(&aTagName); > RulesInfo ruleInfo(EditAction::makeDefListItem); > - ruleInfo.blockType = &aItemType; > + ruleInfo.blockType = &tagName; Ah, blockType isn't nsAtom... I should change it to nsAtom by another bug.

Comment on attachment 8968797 [details] Bug 1451672 - part 20: Rename HTMLEditor::MakeDefinitionItem() and HTMLEditor::InsertBasicBlock() with "WithTransaction" postfix https://reviewboard.mozilla.org/r/237510/#review244096

Comment on attachment 8968798 [details] Bug 1451672 - part 21: Refine TextEditor::TypedText() https://reviewboard.mozilla.org/r/237512/#review244100

Comment on attachment 8968799 [details] Bug 1451672 - part 22: Rename HTMLEditor::InsertNodeIntoProperAncestor() to HTMLEditor::InsertNodeIntoProperAncestorWithTransaction() https://reviewboard.mozilla.org/r/237514/#review244104

Comment on attachment 8968800 [details] Bug 1451672 - part 23: Rename HTMLEditor::Set*BackgroundColor() to HTMLEditor::Set*BackgroundColorWithTransaction() https://reviewboard.mozilla.org/r/237516/#review244106

Comment on attachment 8968795 [details] Bug 1451672 - part 18: Rename EditorBase::CloneAttributes() to EditorBase::CloneAttributesWithTransaction() https://reviewboard.mozilla.org/r/237506/#review244082 > s/uncessary/unnecessary/ Okay.

Comment on attachment 8968801 [details] Bug 1451672 - part 24: Rename HTMLEditor::CopyLastEditableChildStyles() to HTMLEditor::CopyLastEditableChildStylesWithTransaction() https://reviewboard.mozilla.org/r/237518/#review244114

Comment on attachment 8968786 [details] Bug 1451672 - part 9: Create TextEditor::DeleteSelectionAsAction() as implementation of nsIEditor::DeleteSelection() https://reviewboard.mozilla.org/r/237488/#review244116

Thank you for the quick review. I'll land them tomorrow after merging with bug 1455052.

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/16bc145641f9 part 1: Rename EditorBase::CreateNode() to EditorBase::CreateNodeWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/ef997456c777 part 2: Rename EditorBase::InsertNode() to EditorBase::InsertNodeWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/6c75071ac1bb part 3: Rename EditorBase::SplitNode() and related methods to ending with "WithTransaction" r=m_kato https://hg.mozilla.org/integration/autoland/rev/1f538ba260f8 part 4: Rename EditorBase::SplitNodeImpl() to EditorBase::DoSplitNode() r=m_kato https://hg.mozilla.org/integration/autoland/rev/5f61649cdc96 part 5: Rename EditorBase::JoinNodes() and related methods with "WithTransaction" postfix r=m_kato https://hg.mozilla.org/integration/autoland/rev/28422b0eee9a part 6: Rename EditorBase::JoinNodesImpl() to EditorBase::DoJoinNodes() r=m_kato https://hg.mozilla.org/integration/autoland/rev/81a8389b9daa part 7: Rename EditorBase::DeleteNode() to EditorBase::DeleteNodeWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/5e8f25ef1174 part 8: Rename EditorBase::DeleteText() to EditorBase::DeleteTextWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/585bed4748a7 part 9: Create TextEditor::DeleteSelectionAsAction() as implementation of nsIEditor::DeleteSelection() r=m_kato https://hg.mozilla.org/integration/autoland/rev/81798a8610e1 part 10: Rename TextEditor::DeleteSelectionImpl() to TextEditor::DeleteSelectionWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/407654389402 part 11: Rename EditorBase::SetAttribute(), EditorBase::RemoveAttribute() and EditorBase::CloneAttribute() with "WithTransaction" postfix r=m_kato https://hg.mozilla.org/integration/autoland/rev/4b372e4cc463 part 12: Create HTMLEditor::RemoveStyleSheetWithTransaction() as implementation of nsIEditorStyleSheets::RemoveStyleSheet() r=m_kato https://hg.mozilla.org/integration/autoland/rev/979bfcb75157 part 13: Rename EditorBase::InsertTextImpl() and EditorBase::InsertTextIntoTextNodeImpl() to EditorBase::InsertTextWithTransaction() and EditorBase::InsertTextIntoTextNodeWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/24e17127ec48 part 14: Rename EditorBase::ReplaceContainer() to EditorBase::ReplaceContainerWithTransactionInternal() and create some wrappers of it r=m_kato https://hg.mozilla.org/integration/autoland/rev/3bd18c2b9851 part 15: Rename EditorBase::RemoveContainer() and HTMLEditor::RemoveBlockContainer() with "WithTransaction" postfix and make their argument |Element&| r=m_kato https://hg.mozilla.org/integration/autoland/rev/1871811c637d part 16: Rename EditorBase::MoveNode() to EditorBase::MoveNodeWithTransaction() and create EditorBase::MoveNodeToEndWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/a6d341deddb0 part 17: Rename EditorBase::InsertContainerAbove() to EditorBase::InsertContainerWithTransactionInternal() and wraps it with new inline methods, EditorBase::InsertContainerWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/907f07168fed part 18: Rename EditorBase::CloneAttributes() to EditorBase::CloneAttributesWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/9617921cebc3 part 19: Remove TextEditor::CreateBR() and rename TextEditor::CreateBRImpl() to TextEditor::InsertBrElementWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/5726deba8028 part 20: Rename HTMLEditor::MakeDefinitionItem() and HTMLEditor::InsertBasicBlock() with "WithTransaction" postfix r=m_kato https://hg.mozilla.org/integration/autoland/rev/ec93f6db7e4b part 21: Refine TextEditor::TypedText() r=m_kato https://hg.mozilla.org/integration/autoland/rev/7b6165489943 part 22: Rename HTMLEditor::InsertNodeIntoProperAncestor() to HTMLEditor::InsertNodeIntoProperAncestorWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/6ce44677c7eb part 23: Rename HTMLEditor::Set*BackgroundColor() to HTMLEditor::Set*BackgroundColorWithTransaction() r=m_kato https://hg.mozilla.org/integration/autoland/rev/6a2fca281629 part 24: Rename HTMLEditor::CopyLastEditableChildStyles() to HTMLEditor::CopyLastEditableChildStylesWithTransaction() r=m_kato

https://hg.mozilla.org/mozilla-central/rev/16bc145641f9 https://hg.mozilla.org/mozilla-central/rev/ef997456c777 https://hg.mozilla.org/mozilla-central/rev/6c75071ac1bb https://hg.mozilla.org/mozilla-central/rev/1f538ba260f8 https://hg.mozilla.org/mozilla-central/rev/5f61649cdc96 https://hg.mozilla.org/mozilla-central/rev/28422b0eee9a https://hg.mozilla.org/mozilla-central/rev/81a8389b9daa https://hg.mozilla.org/mozilla-central/rev/5e8f25ef1174 https://hg.mozilla.org/mozilla-central/rev/585bed4748a7 https://hg.mozilla.org/mozilla-central/rev/81798a8610e1 https://hg.mozilla.org/mozilla-central/rev/407654389402 https://hg.mozilla.org/mozilla-central/rev/4b372e4cc463 https://hg.mozilla.org/mozilla-central/rev/979bfcb75157 https://hg.mozilla.org/mozilla-central/rev/24e17127ec48 https://hg.mozilla.org/mozilla-central/rev/3bd18c2b9851 https://hg.mozilla.org/mozilla-central/rev/1871811c637d https://hg.mozilla.org/mozilla-central/rev/a6d341deddb0 https://hg.mozilla.org/mozilla-central/rev/907f07168fed https://hg.mozilla.org/mozilla-central/rev/9617921cebc3 https://hg.mozilla.org/mozilla-central/rev/5726deba8028 https://hg.mozilla.org/mozilla-central/rev/ec93f6db7e4b https://hg.mozilla.org/mozilla-central/rev/7b6165489943 https://hg.mozilla.org/mozilla-central/rev/6ce44677c7eb https://hg.mozilla.org/mozilla-central/rev/6a2fca281629