Closed Bug 1451672 Opened 2 years ago Closed Last year

Rename methods of editor which creates transactions

Categories

(Core :: Editor, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(24 files)

59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
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.
Priority: -- → P3
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
Attachment #8968778 - Flags: review?(m_kato) → review+
Comment on attachment 8968779 [details]
Bug 1451672 - part 2: Rename EditorBase::InsertNode() to EditorBase::InsertNodeWithTransaction()

https://reviewboard.mozilla.org/r/237474/#review243636
Attachment #8968779 - Flags: review?(m_kato) → review+
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
Attachment #8968780 - Flags: review?(m_kato) → review+
Comment on attachment 8968781 [details]
Bug 1451672 - part 4: Rename EditorBase::SplitNodeImpl() to EditorBase::DoSplitNode()

https://reviewboard.mozilla.org/r/237478/#review243656
Attachment #8968781 - Flags: review?(m_kato) → review+
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
Attachment #8968782 - Flags: review?(m_kato) → review+
Comment on attachment 8968783 [details]
Bug 1451672 - part 6: Rename EditorBase::JoinNodesImpl() to EditorBase::DoJoinNodes()

https://reviewboard.mozilla.org/r/237482/#review243666
Attachment #8968783 - Flags: review?(m_kato) → review+
Comment on attachment 8968784 [details]
Bug 1451672 - part 7: Rename EditorBase::DeleteNode() to EditorBase::DeleteNodeWithTransaction()

https://reviewboard.mozilla.org/r/237484/#review243668
Attachment #8968784 - Flags: review?(m_kato) → review+
Comment on attachment 8968785 [details]
Bug 1451672 - part 8: Rename EditorBase::DeleteText() to EditorBase::DeleteTextWithTransaction()

https://reviewboard.mozilla.org/r/237486/#review244030
Attachment #8968785 - Flags: review?(m_kato) → review+
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
Attachment #8968787 - Flags: review?(m_kato) → review+
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);
Attachment #8968788 - Flags: review?(m_kato) → review+
Comment on attachment 8968789 [details]
Bug 1451672 - part 12: Create HTMLEditor::RemoveStyleSheetWithTransaction() as implementation of nsIEditorStyleSheets::RemoveStyleSheet()

https://reviewboard.mozilla.org/r/237494/#review244052
Attachment #8968789 - Flags: review?(m_kato) → review+
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
Attachment #8968790 - Flags: review?(m_kato) → review+
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
Attachment #8968791 - Flags: review?(m_kato) → review+
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
Attachment #8968792 - Flags: review?(m_kato) → review+
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
Attachment #8968793 - Flags: review?(m_kato) → review+
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
Attachment #8968794 - Flags: review?(m_kato) → review+
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.
Attachment #8968795 - Flags: review?(m_kato) → review+
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
Attachment #8968796 - Flags: review?(m_kato) → review+
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
Attachment #8968797 - Flags: review?(m_kato) → review+
Comment on attachment 8968798 [details]
Bug 1451672 - part 21: Refine TextEditor::TypedText()

https://reviewboard.mozilla.org/r/237512/#review244100
Attachment #8968798 - Flags: review?(m_kato) → review+
Comment on attachment 8968799 [details]
Bug 1451672 - part 22: Rename HTMLEditor::InsertNodeIntoProperAncestor() to HTMLEditor::InsertNodeIntoProperAncestorWithTransaction()

https://reviewboard.mozilla.org/r/237514/#review244104
Attachment #8968799 - Flags: review?(m_kato) → review+
Comment on attachment 8968800 [details]
Bug 1451672 - part 23: Rename HTMLEditor::Set*BackgroundColor() to HTMLEditor::Set*BackgroundColorWithTransaction()

https://reviewboard.mozilla.org/r/237516/#review244106
Attachment #8968800 - Flags: review?(m_kato) → review+
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
Attachment #8968801 - Flags: review?(m_kato) → review+
Comment on attachment 8968786 [details]
Bug 1451672 - part 9: Create TextEditor::DeleteSelectionAsAction() as implementation of nsIEditor::DeleteSelection()

https://reviewboard.mozilla.org/r/237488/#review244116
Attachment #8968786 - Flags: review?(m_kato) → review+
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
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.