Closed Bug 1784192 Opened 2 years ago Closed 2 years ago

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

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
106 Branch
Tracking Flags:
Tracking Status
firefox106 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

Bug 1784192 - part 1: Change `PropItem::tag` to `nsStaticAtom*` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 2: Change `PropItem::attr` to a strong pointer r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 3: Change other members of `PropItem` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 4: Change `PropItem::mSpecifiedStyle` to a constant r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 5: Make `TypeInState` manage `PropItem` instances with `UniquePtr` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 6: Rename `TypeInState::SetProp` and `TypeInState::TakeSetProperty` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 7: Rename `TypeInState::ClearProp` and related methods r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 8: Make scanner methods of `TypeInState` return `Maybe<size_t>` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 9: Rename `TypeInState` to `PendingStyles` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1784192 - part 10: Make `PendingStyles::GetTypeInState()` return an `enum class` instead of taking 2 bool out parameters r=m_kato!
(deleted), text/x-phabricator-request
Details

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.

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.

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.

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

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

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

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

Depends on D155313

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

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

Additionally,

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

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

Depends on D155317

Summary: Change style property in editor from nsAtom to nsStaticAtom → Change style property in editor from nsAtom to nsStaticAtom (and clean up TypeInState and PropItem)
Pushed by masayuki@d-toybox.com: 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
Pushed by masayuki@d-toybox.com: 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
Pushed by masayuki@d-toybox.com: 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
Pushed by masayuki@d-toybox.com: 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
Regressions: 1787398
No longer regressions: 1787398
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: