Closed Bug 1345690 Opened 3 years ago Closed 3 years ago

Get rid of DeleteNodeTransaction::Init(), DeleteRangeTransaction::Init() and DeleteTextTransaction::Init()

Categories

(Core :: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

Arguments of those methods should be set all arguments at constructing.

Additionally, these methods check if all necessary nodes are editable. I think that it should be renamed to "IsAvailable()" or something.
JoinNodeTransaction has CheckValidity() like IsAvailable(),  If we use "IsAvailable" name method, we should replace "CheckValidity" name with it.
Comment on attachment 8845777 [details]
Bug 1345690 part.1 Make the constructor of DeleteNodeTransaction initialize all necessary members instead of Init()

https://reviewboard.mozilla.org/r/118516/#review120916

::: editor/libeditor/EditorBase.cpp:4292
(Diff revision 1)
>    return transaction.forget();
>  }
>  
>  nsresult
>  EditorBase::CreateTxnForDeleteNode(nsINode* aNode,
>                                     DeleteNodeTransaction** aTransaction)

Although this isn't this issue, we should change to already_AddRefed<DeleteNodeTransaction> CreateTxtForDeleteNode.  We should file a new bug.
Attachment #8845777 - Flags: review?(m_kato) → review+
Comment on attachment 8845778 [details]
Bug 1345690 part.2 Make the constructor of DeleteRangeTransaction initialize all members instead of Init()

https://reviewboard.mozilla.org/r/118518/#review120920

::: editor/libeditor/EditorBase.cpp:4347
(Diff revision 1)
>  nsresult
>  EditorBase::CreateTxnForDeleteSelection(EDirection aAction,
>                                          EditAggregateTransaction** aTransaction,
>                                          nsINode** aNode,
>                                          int32_t* aOffset,
>                                          int32_t* aLength)

Although this is another issue, aTransaction should be return value as already_AddRefed<EditAggregateTransaction>.
Attachment #8845778 - Flags: review?(m_kato) → review+
Comment on attachment 8845779 [details]
Bug 1345690 part.3 Rename DeleteTextTransaction::Init() to DeleteTextTransaction::CanDoIt() since it does not initialize anything and just checking if the text node is editable

https://reviewboard.mozilla.org/r/118520/#review120922
Attachment #8845779 - Flags: review?(m_kato) → review+
Comment on attachment 8845780 [details]
Bug 1345690 part.4 Rename JoinNodeTransaction::CheckValidity() to JoinNodeTransaction::CanDoIt() for consistency with other transaction classes

https://reviewboard.mozilla.org/r/118522/#review120924
Attachment #8845780 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5f4e819e9d53
part.1 Make the constructor of DeleteNodeTransaction initialize all necessary members instead of Init() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c845707f9d1c
part.2 Make the constructor of DeleteRangeTransaction initialize all members instead of Init() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/98894712d99f
part.3 Rename DeleteTextTransaction::Init() to DeleteTextTransaction::CanDoIt() since it does not initialize anything and just checking if the text node is editable r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e6423ffe849e
part.4 Rename JoinNodeTransaction::CheckValidity() to JoinNodeTransaction::CanDoIt() for consistency with other transaction classes r=m_kato
You need to log in before you can comment on or make changes to this bug.