1153629 - Clean up nsHTMLEditRules::GetListActionNodes, GetParagraphFormatNodes, GeNodesFromPoint, ListIsEmptyLine, GetChildNodesForOperation, MakeBlockquote, RemoveBlockStyle, RemovePartOfBlock, ApplyBlockStyle, MakeTransitionList, AlignInnerBlocks, etc.

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This patchset will remove all nsCOMArray from nsHTMLEditRules.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Clean up nsHTMLEditRules:GetListActionNodes

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 - Clean up nsHTMLEditRules::GetParagraphFormatNodes

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 - Clean up nsHTMLEditRules::GetNodesFromPoint

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4 - Clean up nsHTMLEditRules::ListIsEmptyLine

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 5 - Clean up nsHTMLEditRules::GetChildNodesForOperation

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 6 - Clean up nsHTMLEditRules::MakeBlockquote

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 7 - Clean up nsHTMLEditRules::RemoveBlockStyle, RemovePartOfBlock

I removed the out params from RemovePartOfBlock since nothing was using them.

I've been replacing (Content)IsDescendantOf calls with Contains(), because it's nicer to read.  Recently I became a bit unsure when I realized that Contains() silently returns false if passed a null pointer, instead of asserting.  Does it make sense to use it?

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 8 - Clean up nsHTMLEditRules::ApplyBlockStyle

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 9 - Clean up nsHTMLEditRules::MakeTransitionList

There was an incorrect comment: "Different parents, or separated by <br>: transition point".  The function doesn't check for <br>, but maybe it was supposed to?  I fixed the comment to match the function.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 10 - Clean up nsHTMLEditRules::AlignInnerBlocks

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 11 - Clean up nsHTMLEditRules::AdjustSpecialBreaks

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 12 - Clean up nsHTMLEditRules::RemoveEmptyNodes

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8bc8694cf2

Comment 14

[(no longer active)]

Comment on attachment 8592780 [details] [diff] [review]
Part 2 - Clean up nsHTMLEditRules::GetParagraphFormatNodes

Review of attachment 8592780 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +1067,5 @@
>    // post process list.  We need to replace any block nodes that are not format
>    // nodes with their content.  This is so we only have to look "up" the hierarchy
>    // to find format nodes, instead of both up and down.
> +  for (int32_t i = arrayOfNodes.Length() - 1; i >= 0; i--) {
> +    auto& curNode = arrayOfNodes[i];

Couldn't you do:

  for (auto& curNode : Reversed(arrayOfNodes)) {

?

Comment 15

[(no longer active)]

Comment on attachment 8592783 [details] [diff] [review]
Part 4 - Clean up nsHTMLEditRules::ListIsEmptyLine

Review of attachment 8592783 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +3515,5 @@
>        NS_ENSURE_STATE(theBlock);
>        // remember our new block for postprocessing
>        mNewBlock = theBlock;
>        // delete anything that was in the list of nodes
> +      for (int32_t j = arrayOfNodes.Length() - 1; j >= 0; j--) {

Nit: Can you please rewrite this as:

  while (!arrayOfNodes.IsEmpty()) {

The current structure of the loop kind of suggests that there's a bug in it.

@@ +3659,5 @@
>      // remember our new block for postprocessing
>      mNewBlock = theBlock->AsDOMNode();
>      RelativeChangeIndentationOfElementNode(theBlock->AsDOMNode(), +1);
>      // delete anything that was in the list of nodes
> +    for (int32_t j = arrayOfNodes.Length() - 1; j >= 0; j--) {

Here too.

@@ +3845,5 @@
>      // remember our new block for postprocessing
>      mNewBlock = theBlock->AsDOMNode();
>      // delete anything that was in the list of nodes
> +    for (int32_t j = arrayOfNodes.Length() - 1; j >= 0; j--) {
> +      nsCOMPtr<nsINode> curNode = arrayOfNodes[0];

And here.

@@ +8929,5 @@
>      // remember our new block for postprocessing
>      mNewBlock = thePositionedDiv->AsDOMNode();
>      // delete anything that was in the list of nodes
> +    for (int32_t j = arrayOfNodes.Length() - 1; j >= 0; j--) {
> +      nsCOMPtr<nsINode> curNode = arrayOfNodes[0];

Ditto.

Comment 16

[(no longer active)]

Comment on attachment 8592784 [details] [diff] [review]
Part 5 - Clean up nsHTMLEditRules::GetChildNodesForOperation

Review of attachment 8592784 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.h
@@ +274,5 @@
>    nsresult GetNodesForOperation(nsTArray<nsRefPtr<nsRange>>& aArrayOfRanges,
>                                  nsTArray<nsCOMPtr<nsINode>>& outArrayOfNodes,
>                                  EditAction aOperationType,
>                                  TouchContent aTouchContent = TouchContent::yes);
> +  void GetChildNodesForOperation(nsINode& aNode, 

Nit: trailing whitespace!

Comment 17

[(no longer active)]

Comment on attachment 8592789 [details] [diff] [review]
Part 7 - Clean up nsHTMLEditRules::RemoveBlockStyle, RemovePartOfBlock

Review of attachment 8592789 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below fixed.

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +6907,5 @@
>        NS_ENSURE_SUCCESS(res, res);
> +    } else if (IsInlineNode(GetAsDOMNode(curNode))) {
> +      if (curBlock) {
> +        // If so, is this node a descendant?
> +        if (curBlock != curNode && curBlock->Contains(curNode)) {

nsINode::Contains <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp#2333> and nsEditorUtils::IsDescendantOf do vastly different things.  Please do not use Contains() instead here and elsewhere in other patches.

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> Couldn't you do:
> 
>   for (auto& curNode : Reversed(arrayOfNodes)) {
> 
> ?

I don't think so, because AppendInnerFormatNodes appends nodes to the array, and range-based for isn't guaranteed to work if the loop body modifies the array.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> nsINode::Contains
> <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp#2333>
> and nsEditorUtils::IsDescendantOf do vastly different things.  Please do not
> use Contains() instead here and elsewhere in other patches.

Okay -- what do they do differently in practice, other than Contains() returning true if the nodes are the same?  And Contains silently returning false on a null input (ack!).  And not having the extra index argument.

Comment 19

[(no longer active)]

(In reply to :Aryeh Gregor from comment #18)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #14)
> > Couldn't you do:
> > 
> >   for (auto& curNode : Reversed(arrayOfNodes)) {
> > 
> > ?
> 
> I don't think so, because AppendInnerFormatNodes appends nodes to the array,
> and range-based for isn't guaranteed to work if the loop body modifies the
> array.

Ah, yes...  This is so confusing.  :(

> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #17)
> > nsINode::Contains
> > <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp#2333>
> > and nsEditorUtils::IsDescendantOf do vastly different things.  Please do not
> > use Contains() instead here and elsewhere in other patches.
> 
> Okay -- what do they do differently in practice, other than Contains()
> returning true if the nodes are the same?  And Contains silently returning
> false on a null input (ack!).  And not having the extra index argument.

Well, nsContentUtils::ContentIsDescendantOf and nsEditorUtils::IsDescendantOf have the same behavior, but nsINode::Contains does a whole bunch of checks before calling nsContentUtils::ContentIsDescendantOf which would not have been performed by nsEditorUtils::IsDescendantOf, such as whether the nodes are in a document, the special case of the parent being a document, etc.

Now, it may very well be the case that none of those checks matter for our needs, but that's not obvious to me without investigation, and I don't really think that switching to nsINode::Contains is worth that investigation.

If anything, I would like us to nuke nsEditorUtils::IsDescendantOf in favor of nsContentUtils::ContentIsDescendantOf.  :-)

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #19)
> Well, nsContentUtils::ContentIsDescendantOf and
> nsEditorUtils::IsDescendantOf have the same behavior, but nsINode::Contains
> does a whole bunch of checks before calling
> nsContentUtils::ContentIsDescendantOf which would not have been performed by
> nsEditorUtils::IsDescendantOf, such as whether the nodes are in a document,
> the special case of the parent being a document, etc.
> 
> Now, it may very well be the case that none of those checks matter for our
> needs, but that's not obvious to me without investigation, and I don't
> really think that switching to nsINode::Contains is worth that investigation.

It's specced: https://dom.spec.whatwg.org/#dom-node-containsother

> If anything, I would like us to nuke nsEditorUtils::IsDescendantOf in favor
> of nsContentUtils::ContentIsDescendantOf.  :-)

We can't do so entirely because it doesn't have the aIndex argument.

Comment 21

[(no longer active)]

(In reply to :Aryeh Gregor from comment #20)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #19)
> > Well, nsContentUtils::ContentIsDescendantOf and
> > nsEditorUtils::IsDescendantOf have the same behavior, but nsINode::Contains
> > does a whole bunch of checks before calling
> > nsContentUtils::ContentIsDescendantOf which would not have been performed by
> > nsEditorUtils::IsDescendantOf, such as whether the nodes are in a document,
> > the special case of the parent being a document, etc.
> > 
> > Now, it may very well be the case that none of those checks matter for our
> > needs, but that's not obvious to me without investigation, and I don't
> > really think that switching to nsINode::Contains is worth that investigation.
> 
> It's specced: https://dom.spec.whatwg.org/#dom-node-containsother

Sure.  But I'm not sure what you mean, are you arguing that switching to nsINode::Contains is worth it?  If yes, I don't think the fact that it is specced is a good enough reason, I care more about not breaking stuff that we don't have tests for...

> > If anything, I would like us to nuke nsEditorUtils::IsDescendantOf in favor
> > of nsContentUtils::ContentIsDescendantOf.  :-)
> 
> We can't do so entirely because it doesn't have the aIndex argument.

That's true.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/394ba1703c08
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdfdce695b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee256b6f62ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb266456a948
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ff1d4bb056
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a62d4ec4542
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b609c3da83
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f5d3fc71b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/af47b386bea7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0fdf67005d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d071f07242
https://hg.mozilla.org/integration/mozilla-inbound/rev/25bc426c8c0e

Comment 23

[Carsten Book [:Tomcat]]

Hi, this changes got in backout in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=329dd852c06b since one of this changes caused memory leaks in linux/OS X m2,m4, M-e10s m4 and Crashtests like:

https://treeherder.mozilla.org/logviewer.html#?job_id=9178717&repo=mozilla-inbound

06:21:14 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | default process: 1483211 bytes leaked (AsyncLatencyLogger, AsyncStatement, AtomImpl, BodyRule, CSSStyleSheet, ...)
06:21:14 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, plugin process 1044
06:21:14 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
06:21:14 INFO - Per-Inst Leaked Total Rem
06:21:14 INFO - 0 TOTAL 57 0 301 0
06:21:14 INFO - nsTraceRefcnt::DumpStatistics: 26 entries

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Almost certain this is caused by bug 1157848.  Will reland with that if it passes the try run:

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

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/152ade0072cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/b028d352877d
https://hg.mozilla.org/integration/mozilla-inbound/rev/21318cef2ac4
https://hg.mozilla.org/integration/mozilla-inbound/rev/77f01af28007
https://hg.mozilla.org/integration/mozilla-inbound/rev/33634e2476cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/55060c30180e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c27dad8a22d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e50601e79b5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cc8d9fa845
https://hg.mozilla.org/integration/mozilla-inbound/rev/af542a26cc53
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1aa3a03afe
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce645a33477a

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/152ade0072cc
https://hg.mozilla.org/mozilla-central/rev/b028d352877d
https://hg.mozilla.org/mozilla-central/rev/21318cef2ac4
https://hg.mozilla.org/mozilla-central/rev/77f01af28007
https://hg.mozilla.org/mozilla-central/rev/33634e2476cd
https://hg.mozilla.org/mozilla-central/rev/55060c30180e
https://hg.mozilla.org/mozilla-central/rev/9c27dad8a22d
https://hg.mozilla.org/mozilla-central/rev/e50601e79b5e
https://hg.mozilla.org/mozilla-central/rev/37cc8d9fa845
https://hg.mozilla.org/mozilla-central/rev/af542a26cc53
https://hg.mozilla.org/mozilla-central/rev/6c1aa3a03afe
https://hg.mozilla.org/mozilla-central/rev/ce645a33477a