1649980 - Make `WSRunScanner` and `WSRunObject` stop taking DOM points at construction

Status: RESOLVED FIXED

(Core :: DOM: Editor, task)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

WSRunScanner and WSRunObject take one or two EditorDOMPoints and initialize its members for the point. However, all users of the storing data uses new stack only class TextFragmentData to know what fragments there are. So, every such user should take the range instead of using stored range.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

As coming patches, I believe that WSRunObject's job is, it keeps visible of white-spaces before and after modifying the DOM tree. E.g., if invisible leading/trailing white-spaces in block element may become visible, it deletes them. If visible white-space sequence is being split, it may replace collapsible ASCII white-space at the split point with NBSP if needed. So, perhaps, it should be renamed to WhiteSpaceVisibilityKeeper, but the name is too long and it becomes a class whose all methods are static. Therefore, the class names will be in HTMLEditor a lot...

I still don't have better name idea of WSRunScanner, though.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 1: Move `WSRunScanner::mStart` and `WSRunScanner::mEnd` initializers into `BoundaryData` r=m_kato!

For making WSRunScanner::BoundaryData independent from WSRunScanner,
its initializer should be in the class itself as static factory methods.

Depends on D82295

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 2: Move `WSRunScanner::GetEditableBlockParentOrTopmotEditableInlineContent()` to `HTMLEditUtils` r=m_kato!

Depends on D82693

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 3: Make `WSRunScanner` store white-space sequence data with a `TextFragmentData` instance r=m_kato!

This patch makes WSRunScanner have TextFragmentData const mTextFragmentData
instead of 2 BoundaryDatas, a NoBreakingSpaceData and a bool storing
whether it's preformatted or not.

Depends on D82694

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 4: Implement `GetInclusiveNextEditableCharPoint()` and `GetPreviousEditableCharPoint()` in `TextFragmentData` instead of `WSRunScanner` r=m_kato!

They work with a TextFragmentData instance, and the following patches
require to run it without WSRunScanner/WSRunObject instances.
Therefore, this patch moves them into TextFragmentData.

Depends on D82695

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 5: Implement `GetEndOfCollapsibleASCIIWhiteSpaces()` and `GetPreviousCharPointFromPointInText()` in `TextFragmentData` instead of `WSRunScanner` r=m_kato!

Depends on D82697

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 6: Redesign `WSRunObject::MaybeReplacePreviousNBSPWithASCIIWhiteSpace()` r=m_kato!

It's called by WSRunObject::InsertText() and WSRunObject::InsertBreak() and
it has 2 parts. One is, considering whether previous char is an NBSP or not
and whether it should be replaced with an ASCII white-space if it's followed
by visible character. The other is, doing the replacement. The latter code
is enough simple. Therefore, we can copy them into the callers. Then,
we can move the check logic into TextFragmentData.

Depends on D82698

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 7: Redesign `WSRunObject::MaybeReplaceInclusiveNextNBSPWithASCIIWhiteSpace()` r=m_kato!

Same as the previous patch, it can be split to computation part and
modifying the DOM tree part. Then, the former can be in TextFragmentData
and the latter can be done by the caller which is only
WSRunObject::InsertText().

Depends on D82699

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 8: Make `TextFragmentData::CreateVisibleWhiteSpacesData()` cache its result and returns reference instead of copy of the instance r=m_kato!

CreateVisibleWhiteSpacesData() is now called multiple times and maybe called
after the DOM tree is modified even though it's a bug. Therefore, we should
make it store first result and return its reference instead.

Depends on D82700

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 9: Make `WSRunScanner::InsertText()` a static method r=m_kato!

It's simpler to make WSRunScanner::InsertText() take insertion point.
Then, it can do its jobs with TextFragmentData instance(s).

Depends on D82701

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 10: Get rid of `WSRunScanner::mScanEndPoint` r=m_kato!

Now, mScanEndPoint is not used. This patch removes it and clean up the
constructors of WSRunScanner and WSRunObject.

Depends on D82702

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 11: Make `WSRunObject::ReplaceASCIIWhiteSpacesWithOneNBSP()` static r=m_kato!

Depends on D82703

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 12: Make `WSRunObject::InsertBreak()` static r=m_kato!

It's now can work with static helper methods and a TextFragmentData instance.
Therefore, this patch makes it a static method.

Note that it's always called with nsIEditor::eNone so that we can get rid of
the argument.

Depends on D82704

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 13: Make `WSRunObject::PrepareToDeleteRangePriv()` static r=m_kato!

Depends on D82705

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 14: Make `WSRunObject::PrepareToSplitAcrossBlocksPriv()` static r=m_kato!

Depends on D82706

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 15: Make `WSRunObject::DeleteWSBackward()` static r=m_kato!

Depends on D82707

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 16: Make `WSRunObject::DeleteWSForward()` static r=m_kato!

Depends on D82708

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 17: Merge `WSRunObject::DeleteInvisibleASCIIWhiteSpacesInternal()` to `WSRunObject::DeleteInvisibleASCIIWhiteSpaces()` r=m_kato!

Now, it does not need to be a WSRunObject instance so that its wrapper to
create WSRunObject instance is not necessary.

Depends on D82709

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 18: Merge `WSRunObject::NormalizeWhiteSpacesAtEndOf()` to `WSRunObject::NormalizeWhiteSpacesAround()` r=m_kato!

Similar to the previous patch, WSRunObject::NormalizeWhiteSpacesAround() is
a wrapper to create WSRunObject instance for calling
NormalizeWhiteSpacesAtEndOf(), but it does not need to be WSRunObject's
instance. Therefore, we can merge them.

Note that this renames the merged method to NormalizeVisibleWhiteSpacesAt.

Depends on D82710

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 19: Make `WSRunObject` class not instantiated r=m_kato!

Now, all member methods of WSRunObject are static. So, it shouldn't
be able to instantiated.

Depends on D82711

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 20: Rename `WSRunObject` to `WhiteSpaceVisibilityKeeper` r=m_kato!

Although the new name is long, but I have no better idea. The class's purpose
is to keep white-space visibility around modifying DOM position. Therefore,
I use "keeper" for the name.

Depends on D82712

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 21: Move `BoundaryData` and `NoBreakingWhiteSpacesData` into `TextFragmentData` r=m_kato!

Now, these classes are used only by TextFragmentData and they can be not
exposed. Therefore, we should hide them with making them private nested
classes of TextFragmentData.

Depends on D82713

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1649980 - part 22: Make `VisibleWhiteSpacesData` store start point and end point with `EditorDOMPoint` r=m_kato!

Depends on D82714

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e25637fd897
part 1: Move `WSRunScanner::mStart` and `WSRunScanner::mEnd` initializers into `BoundaryData` r=m_kato

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a06b8e95f27
part 2: Move `WSRunScanner::GetEditableBlockParentOrTopmotEditableInlineContent()` to `HTMLEditUtils` r=m_kato

Comment 26

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3e25637fd897
https://hg.mozilla.org/mozilla-central/rev/4a06b8e95f27

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6de01deddd41
part 3: Make `WSRunScanner` store white-space sequence data with a `TextFragmentData` instance r=m_kato

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88c47cd8a620
part 4: Implement `GetInclusiveNextEditableCharPoint()` and `GetPreviousEditableCharPoint()` in `TextFragmentData` instead of `WSRunScanner` r=m_kato

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ade6312bf1c1
part 5: Implement `GetEndOfCollapsibleASCIIWhiteSpaces()` and `GetPreviousCharPointFromPointInText()` in `TextFragmentData` instead of `WSRunScanner` r=m_kato

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fedbc9d3130
part 6: Redesign `WSRunObject::MaybeReplacePreviousNBSPWithASCIIWhiteSpace()` r=m_kato

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5daee02c9dc7
part 7: Redesign `WSRunObject::MaybeReplaceInclusiveNextNBSPWithASCIIWhiteSpace()` r=m_kato

Comment 32

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6de01deddd41

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3309e46b2f22
part 8: Make `TextFragmentData::CreateVisibleWhiteSpacesData()` cache its result and returns reference instead of copy of the instance r=m_kato

Comment 34

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/88c47cd8a620
https://hg.mozilla.org/mozilla-central/rev/ade6312bf1c1
https://hg.mozilla.org/mozilla-central/rev/6fedbc9d3130
https://hg.mozilla.org/mozilla-central/rev/5daee02c9dc7

Comment 35

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3309e46b2f22

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/daf96afe7740
part 9: Make `WSRunScanner::InsertText()` a static method r=m_kato
https://hg.mozilla.org/integration/autoland/rev/935a16d8abe6
part 10: Get rid of `WSRunScanner::mScanEndPoint` r=m_kato

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d359b5c68942
part 11: Make `WSRunObject::ReplaceASCIIWhiteSpacesWithOneNBSP()` static r=m_kato

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e51d9c0ce8f
part 12: Make `WSRunObject::InsertBreak()` static r=m_kato

Comment 39

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/daf96afe7740
https://hg.mozilla.org/mozilla-central/rev/935a16d8abe6

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/906ef2c7cb40
part 13: Make `WSRunObject::PrepareToDeleteRangePriv()` static r=m_kato

Comment 41

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d359b5c68942
https://hg.mozilla.org/mozilla-central/rev/9e51d9c0ce8f

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83f2c4696006
part 14: Make `WSRunObject::PrepareToSplitAcrossBlocksPriv()` static r=m_kato

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b69c904f104
part 15: Make `WSRunObject::DeleteWSBackward()` static r=m_kato

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb66c9898f9d
part 16: Make `WSRunObject::DeleteWSForward()` static r=m_kato

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae9c2245690b
part 17: Merge `WSRunObject::DeleteInvisibleASCIIWhiteSpacesInternal()` to `WSRunObject::DeleteInvisibleASCIIWhiteSpaces()` r=m_kato

Comment 46

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/906ef2c7cb40

Comment 47

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/83f2c4696006
https://hg.mozilla.org/mozilla-central/rev/0b69c904f104
https://hg.mozilla.org/mozilla-central/rev/cb66c9898f9d
https://hg.mozilla.org/mozilla-central/rev/ae9c2245690b

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aaa0c6fdc568
part 18: Merge `WSRunObject::NormalizeWhiteSpacesAtEndOf()` to `WSRunObject::NormalizeWhiteSpacesAround()` r=m_kato

Comment 49

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/aaa0c6fdc568

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2845e156ce45
part 19: Make `WSRunObject` class not instantiated r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bcef72851157
part 20: Rename `WSRunObject` to `WhiteSpaceVisibilityKeeper` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/977da8bc0c21
part 21: Move `BoundaryData` and `NoBreakingWhiteSpacesData` into `TextFragmentData` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e3bfcfb8feb3
part 22: Make `VisibleWhiteSpacesData` store start point and end point with `EditorDOMPoint` r=m_kato

Comment 51

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2845e156ce45
https://hg.mozilla.org/mozilla-central/rev/bcef72851157
https://hg.mozilla.org/mozilla-central/rev/977da8bc0c21
https://hg.mozilla.org/mozilla-central/rev/e3bfcfb8feb3