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

RESOLVED FIXED in Firefox 40

Status

()

--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(12 attachments)

16.79 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
11.57 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
7.08 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
18.50 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
6.87 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
7.64 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
14.28 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
14.34 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
8.57 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
6.78 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
5.01 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
11.24 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
This patchset will remove all nsCOMArray from nsHTMLEditRules.
Flags: in-testsuite-
Created attachment 8592776 [details] [diff] [review]
Part 1 - Clean up nsHTMLEditRules:GetListActionNodes
Attachment #8592776 - Flags: review?(ehsan)
Created attachment 8592780 [details] [diff] [review]
Part 2 - Clean up nsHTMLEditRules::GetParagraphFormatNodes
Attachment #8592780 - Flags: review?(ehsan)
Created attachment 8592781 [details] [diff] [review]
Part 3 - Clean up nsHTMLEditRules::GetNodesFromPoint
Attachment #8592781 - Flags: review?(ehsan)
Created attachment 8592783 [details] [diff] [review]
Part 4 - Clean up nsHTMLEditRules::ListIsEmptyLine
Attachment #8592783 - Flags: review?(ehsan)
Created attachment 8592784 [details] [diff] [review]
Part 5 - Clean up nsHTMLEditRules::GetChildNodesForOperation
Attachment #8592784 - Flags: review?(ehsan)
Created attachment 8592786 [details] [diff] [review]
Part 6 - Clean up nsHTMLEditRules::MakeBlockquote
Attachment #8592786 - Flags: review?(ehsan)
Created attachment 8592789 [details] [diff] [review]
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?
Attachment #8592789 - Flags: review?(ehsan)
Created attachment 8592790 [details] [diff] [review]
Part 8 - Clean up nsHTMLEditRules::ApplyBlockStyle
Attachment #8592790 - Flags: review?(ehsan)
Created attachment 8592791 [details] [diff] [review]
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.
Attachment #8592791 - Flags: review?(ehsan)
Created attachment 8592792 [details] [diff] [review]
Part 10 - Clean up nsHTMLEditRules::AlignInnerBlocks
Attachment #8592792 - Flags: review?(ehsan)
Created attachment 8592795 [details] [diff] [review]
Part 11 - Clean up nsHTMLEditRules::AdjustSpecialBreaks
Attachment #8592795 - Flags: review?(ehsan)
Created attachment 8592798 [details] [diff] [review]
Part 12 - Clean up nsHTMLEditRules::RemoveEmptyNodes
Attachment #8592798 - Flags: review?(ehsan)

Updated

4 years ago
Attachment #8592776 - Flags: review?(ehsan) → review+

Comment 14

4 years ago
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)) {

?
Attachment #8592780 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8592781 - Flags: review?(ehsan) → review+

Comment 15

4 years ago
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.
Attachment #8592783 - Flags: review?(ehsan) → review+

Comment 16

4 years ago
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!
Attachment #8592784 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8592786 - Flags: review?(ehsan) → review+

Comment 17

4 years ago
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.
Attachment #8592789 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8592790 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8592791 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8592792 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8592795 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8592798 - Flags: review?(ehsan) → review+
(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.
Flags: needinfo?(ehsan)

Comment 19

4 years ago
(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.  :-)
Flags: needinfo?(ehsan)
(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

4 years ago
(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.
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
Flags: needinfo?(ayg)
Depends on: 1157848
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
Flags: needinfo?(ayg)

Updated

4 years ago
Depends on: 1158651
You need to log in before you can comment on or make changes to this bug.