1784192 - Change style property in editor from nsAtom to nsStaticAtom (and clean up TypeInState and PropItem)

Status: RESOLVED FIXED

(Core :: DOM: Editor, task, P3)

Description

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

nsStaticAtom instances are alive until shutting down and our editor do not use dynamic atoms to handle style. Therefore, we can make all of them use nsStaticAtom instead.

Comment 1

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

And I'll refactor TypeInState and PropItem for making what they do clearer because I always try to understand again and again what they do due to unclear names.

Comment 2

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

Attachment: Bug 1784192 - part 1: Change `PropItem::tag` to `nsStaticAtom*` r=m_kato!

It's always a pointer to nsStaticAtom instance or nullptr. Therefore,
it can be nsStaticAtom* and we can make its users treat nsStaticAtom
instead of nsAtom.

Additionally, this patch changes some pointer parameters to references if
they are never nullptr.

Comment 3

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

Attachment: Bug 1784192 - part 2: Change `PropItem::attr` to a strong pointer r=m_kato!

It's currently no problem to manage it with a raw pointer because it may be
set to a dynamic atom only when nsIHTMLEditor.insertLinkAroundSelection is
called with unknown attribute, but comm-central uses it only with href
attribute.

I think that we should change the API just to take href value in the future,
but for now, it should be RefPtr<nsAtom>.

Depends on D155310

Comment 4

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

Attachment: Bug 1784192 - part 3: Change other members of `PropItem` r=m_kato!

According to the debug, its value can be CSS property value if in the CSS mode.
For making the value meaning easier to understand, this renames it to
mAttributeValueOrCSSValue.

Depends on D155311

Comment 5

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

Attachment: Bug 1784192 - part 4: Change `PropItem::mSpecifiedStyle` to a constant r=m_kato!

Only the value member needs to be updated when setting the prop multiple times.
Therefore, we cannot change all members to constants.

Depends on D155312

Comment 6

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

Attachment: Bug 1784192 - part 5: Make `TypeInState` manage `PropItem` instances with `UniquePtr` r=m_kato!

It's ugly to manage them as raw pointer especially when deleting the instances.
We should make it use UniquePtr.

Depends on D155313

Comment 7

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

Attachment: Bug 1784192 - part 6: Rename `TypeInState::SetProp` and `TypeInState::TakeSetProperty` r=m_kato!

I usually retry to understand what they mean. Therefore, I'd like to give new
names for them (and rename TypeInState class in a following patch).

Depends on D155314

Comment 8

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

Attachment: Bug 1784192 - part 7: Rename `TypeInState::ClearProp` and related methods r=m_kato!

For consistency with the previous patch, we should rename them too. Then, we're
getting rid of unclear word "Prop" from the public methods.

Depends on D155315

Comment 9

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

Attachment: Bug 1784192 - part 8: Make scanner methods of `TypeInState` return `Maybe<size_t>` r=m_kato!

Depends on D155316

Comment 10

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

Attachment: Bug 1784192 - part 9: Rename `TypeInState` to `PendingStyles` r=m_kato!

Additionally,

• PropItem -> PendingStyle
• StyleCache -> PendingStyleCache
• AutoStyleCacheArray -> AutoPendingStyleCacheArray

And finally, PendingStyle (formally PropItem) is changed to class and
its members are encapsuled.

Depends on D155317

Comment 11

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

Attachment: Bug 1784192 - part 10: Make `PendingStyles::GetTypeInState()` return an `enum class` instead of taking 2 bool out parameters r=m_kato!

Depends on D155318

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61b5b71feb21
part 1: Change `PropItem::tag` to `nsStaticAtom*` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/8b0d988cfcdb
part 2: Change `PropItem::attr` to a strong pointer r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c60a519079f1
part 3: Change other members of `PropItem` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1bb446d79840
part 4: Change `PropItem::mSpecifiedStyle` to a constant r=m_kato

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16ef13574d67
part 5: Make `TypeInState` manage `PropItem` instances with `UniquePtr` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7f2a4bf264e7
part 6: Rename `TypeInState::SetProp` and `TypeInState::TakeSetProperty` r=m_kato

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6be885417efd
part 7: Rename `TypeInState::ClearProp` and related methods r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bf35b554c9e6
part 8: Make scanner methods of `TypeInState` return `Maybe<size_t>` r=m_kato

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dace560a5e6d
part 9: Rename `TypeInState` to `PendingStyles` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0dacbbae333e
part 10: Make `PendingStyles::GetTypeInState()` return an `enum class` instead of taking 2 bool out parameters r=m_kato

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/61b5b71feb21
https://hg.mozilla.org/mozilla-central/rev/8b0d988cfcdb
https://hg.mozilla.org/mozilla-central/rev/c60a519079f1
https://hg.mozilla.org/mozilla-central/rev/1bb446d79840
https://hg.mozilla.org/mozilla-central/rev/16ef13574d67
https://hg.mozilla.org/mozilla-central/rev/7f2a4bf264e7
https://hg.mozilla.org/mozilla-central/rev/6be885417efd
https://hg.mozilla.org/mozilla-central/rev/bf35b554c9e6
https://hg.mozilla.org/mozilla-central/rev/dace560a5e6d
https://hg.mozilla.org/mozilla-central/rev/0dacbbae333e