Closed Bug 1588688 Opened 1 year ago Closed 7 months ago

Change using array of nsINode to array of nsIContent in editor

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(17 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

https://searchfox.org/mozilla-central/search?q=nsTArray%3C(OwningNonNull%3C%7CnsCOMPtr%3C)%3FnsINode&case=true&regexp=true&path=libeditor

Currently, a lot of methods handle nodes with nsTArray<OwningNonNull<nsINode>>, nsTArray<nsCOMPtr<nsINode>> or nsTArray<nsINode*> in editor module. However, editor does not handle non-nsIContent nodes actually. So, we can make them use nsTArray<OwningNonNull<nsIContent>> etc and make them be free from IsContent() check.

Priority: -- → P3
Assignee: nobody → masayuki
Status: NEW → ASSIGNED

TrivialFunctor returns always true for DOMIterator::AppendList(). That
means that DOMIterator appends all nodes which are listed up by
ContentIterator. So, it's reasonable to make DOMIterator have
AppendAllNodesToArray() because it's more self-documented and faster.

Additionally, BRNodeFunctor always returns true when nodes with which
HTMLBRElement::FromNode() returns non-nullptr. So, we can make
AppendAllNodesToArray() a template class which just checks whether
every nodes are specific node type.

This patch creates DOMIterator::AppendNodesToArray() which takes pointer to
a functor and removes DOMIterator::AppendList(). This allows callers to use
lambda rather than the remaining functors, UniqueFunctor and
EmptyEditableFunctor, which are used only once. Therefore, writing them as
lambda makes the callers easier to understand. Finally, we can remove those
functor classes' base class, BoolDomIterFunctor too.

Note that this patch avoids using std::function even though if we'd use it
as functor, we could make each lambda can capture variables and each lambda
does not need to convert to point of a function with + operator explicitly.
The reason is, std::function is too slow if AppendNodesToArray() is called
in a hot path.

Depends on D61968

Unless editor needs to treat document node, all nodes are sub-class of
nsIContent. Therefore, we can make HTMLEditor::CollectEditTargetNodes()
return array of nsIContent instead of array of nsINode.

Depends on D61969

The content iterators always return nsIContent even though the result of
GetCurrentNode() is nsINode*. Therefore, we can make
HTMLEditor::RemoveEmptyNodesIn() use array of nsIContent instead of
array of nsINode.

Depends on D61970

Same as the previous patch, the content iterator won't return non-nsIContent
node. Therefore, HTMLEditor::HandleDeleteNonCollapsedSelection() can treat
it with array of nsIContent.

Depends on D61971

HTMLEditor::CollapseAdjacentTextNodes() collects editable text nodes first so
that the array can be array of dom::Text. Additionally, using
DOMSubtreeIterator instead of ContentSubtreeIterator makes the code easier
to understand.

Depends on D61972

Helper methods of HTMLEditor::DoInsertHTMLWithContext() which uses array of
nsINode is really messy and we can make them simpler. This is first
preparation of a series of refactoring patches of them.

Depends on D61973

HTMLEditor::ScanForListAndTableStructure() is complicated because it has
2 modes, one is for handling list element, the other is for handling table
element. This patch splits them as 2 methods.

Depends on D61974

The name does not explain what it does. Additionally, it should be same style
as HTMLEditor::IsReplaceableListElement() for making it easier to understand.

Depends on D61975

It can be a static method and does not make sense taking array of nodes as
input argument because it refers only first or last node of the array.

Depends on D61976

HTMLEditor::CreateListOfNodesToPaste() can be static and it does not need to
take DocumentFragment as an argument if the range is always specified.
For avoiding 2 input path to specify a range, we should make it take only
range boundaries and array of nodes to return the collected nodes.

Depends on D61977

HTMLEditor::DiscoverPartialListsAndTables() can be static and we can make
its definition simpler. However, due to the odd logic, I'm still not sure
what it does. I'll update it after cleaning up its result user,
HTMLEditor::ReplaceOrphanedStructure().

Depends on D61978

Unfortunately, even with this cleaning up, I don't understand what it does
because existing comment and what actually it does seem different.

Depends on D61979

In these patches, we know that there are a lot of helper methods to fix node
list which is created by SubtreeContentIterator and will be inserted into
the editor's DOM tree, and they are not used by other places. So, we can
encapsulate them into a stack only class for making their usage clearer.

Depends on D61980

For guaranteeing the meaning of list or <table> element in
ReplaceOrphanedStructure(), it should call DiscoverPartialListsAndTables().
Then, their meaning becomes clearer than coming from its parameter.

Depends on D61981

For guaranteeing the meaning of aArrayOfListAndTableRelatedElements,
ReplaceOrphanedStructure() should call
CollectListAndTableRelatedElementsAt() by itself rather than taking as
a parameter.

Then, what it does becomes a little bit clearer. Therefore, this patch renames
ReplaceOrphanedStructure() to EnsureBeginsOrEndsWithValidContent() and
DiscoverPartialListsAndTables() to GetMostAncestorListOrTableElement(),
and adds a lot of comments.

As far as I've tested by my hand, the behavior is not changed even though
it's really buggy in some cases. We should fix them after writing automated
tests in another bug.

Depends on D61982

Finally, this patch makes HTMLEditor::DoInsertHTMLWithContext() stop using
array of nsINode.

Depends on D61983

I'll land each patch separately after the merge. So, feel free to put off to review them.

Keywords: leave-open

And also feel free to write any patches for editor because each patch does really simple things so that it's easy to rebase.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c8024f8147e6
part 1: Remove `TrivialFunctor` and `BRNodeFunctor` for `DOMIterator` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2a2b0b1b3be6
part 2: Remove `UniqueFunctor` and `EmptyEditableFunctor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/283e729181e1
part 3: Make `HTMLEditor::CollectEditTargetNodes()` and its users use array of `nsIContent` rather than `nsINode` r=m_kato
Attachment #9124973 - Attachment description: Bug 1588688 - part7-1: Sort out `HTMLEditor::ScanForListAndTableStructure()` r=m_kato! → Bug 1588688 - part 7-1: Sort out `HTMLEditor::ScanForListAndTableStructure()` r=m_kato!
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/283906e4aee2
part 4: Make `HTMLEditor::RemoveEmptyNodesIn()` use array of `nsIContent` rather than `nsINode` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/335b4d422b7d
part 5: Make `HTMLEditor::HandleDeleteNonCollapsedSelection()` use array of `nsIContent` instead of `nsINode` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/db882455dab3
part 6: Make `HTMLEditor::CollapseAdjacentTextNodes()` use array of `dom::Text` rather than `nsINode` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a60e490f4948
part 7-1: Sort out `HTMLEditor::ScanForListAndTableStructure()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/931d03e3922d
part 7-2: Pick list element handling part of `HTMLEditor::ScanForListAndTableStructure()` up r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/102521a4f26d
part 7-3: Clean up `HTMLEditor::ScanForTableStructure()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f286c12597d3
part 7-4: Clean up `HTMLEditor::GetListAndTableParents()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6c0289fdd24b
part 7-5: Clean up `HTMLEditor::CreateListOfNodesToPaste()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0809a778c694
part 7-6: Clean up `HTMLEditor::DiscoverPartialListsAndTables()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/69ee7198e366
part 7-7: Clean up `HTMLEditor::ReplaceOrphanedStructure()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2737da7a5827
part 7-8: Encapsulate the first and last node fixer into a stack only class r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c3af04a575dc
part 7-9: Make `ReplaceOrphanedStructure()` call `DiscoverPartialListsAndTables()` by itself r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/45a6cdb1ddce
part 7-10: Make `ReplaceOrphanedStructure()` call `CollectListAndTableRelatedElementsAt()` by itself r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/dbddba0b7322
part 8: Make `HTMLEditor::DoInsertHTMLWithContext()` and its helper methods use array of `nsIContent` rather than `nsINode` r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.