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

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

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.

Comment 1

[Makoto Kato [:m_kato]]

JoinNodeTransaction has CheckValidity() like IsAvailable(),  If we use "IsAvailable" name method, we should replace "CheckValidity" name with it.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aed6f596168

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c517b2becbe9

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1345690 part.1 Make the constructor of DeleteNodeTransaction initialize all necessary members instead of Init()

I'd like to make mNodeToDelete an owning-non-null member, but it's not cycle collector aware.  Therefore, this patch only changes mEditorBase from pointer to reference.

Review commit: https://reviewboard.mozilla.org/r/118516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118516/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1345690 part.2 Make the constructor of DeleteRangeTransaction initialize all members instead of Init()

Similar to DeleteNodeTransaction, DeleteRangeTransaction should take all necessary information as its arguments.  However, different from DeleteNodeTransaction, this doesn't need to implement CanDoIt() since nobody checks the state.

Review commit: https://reviewboard.mozilla.org/r/118518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118518/

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: 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

Review commit: https://reviewboard.mozilla.org/r/118520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118520/

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1345690 part.4 Rename JoinNodeTransaction::CheckValidity() to JoinNodeTransaction::CanDoIt() for consistency with other transaction classes

Review commit: https://reviewboard.mozilla.org/r/118522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118522/

Comment 8

[Makoto Kato [:m_kato]]

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.

Comment 9

[Makoto Kato [:m_kato]]

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>.

Comment 10

[Makoto Kato [:m_kato]]

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

Comment 11

[Makoto Kato [:m_kato]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5f4e819e9d53
https://hg.mozilla.org/mozilla-central/rev/c845707f9d1c
https://hg.mozilla.org/mozilla-central/rev/98894712d99f
https://hg.mozilla.org/mozilla-central/rev/e6423ffe849e