Closed Bug 1425440 Opened 2 years ago Closed 1 year ago

Cleanup the use of nsINode::GetChildAt_Deprecated

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: baku, Assigned: baku)

References

(Regressed 1 open bug)

Details

Attachments

(12 files, 47 obsolete files)

1.47 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
4.29 KB, patch
catalinb
: review-
Details | Diff | Splinter Review
2.28 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
1.26 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
1.19 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
3.41 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
3.09 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
8.88 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
5.92 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
2.72 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
2.39 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
4.08 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
This is a follow up of bug 1425321
Attachment #8937021 - Flags: review?(catalin.badea392)
Attachment #8937023 - Flags: review?(catalin.badea392)
Attachment #8937024 - Flags: review?(catalin.badea392)
Attached patch part 4 - nsXULTooltipListener (obsolete) — Splinter Review
Attachment #8937025 - Flags: review?(catalin.badea392)
Attached patch part 4 - nsXULTooltipListener (obsolete) — Splinter Review
Attachment #8937025 - Attachment is obsolete: true
Attachment #8937025 - Flags: review?(catalin.badea392)
Attachment #8937026 - Flags: review?(catalin.badea392)
Attached patch part 4 - nsXULTooltipListener (obsolete) — Splinter Review
Attachment #8937026 - Attachment is obsolete: true
Attachment #8937026 - Flags: review?(catalin.badea392)
Attachment #8937027 - Flags: review?(catalin.badea392)
Attached patch part 5 - nsListBoxBodyFrame (obsolete) — Splinter Review
Attachment #8937028 - Flags: review?(catalin.badea392)
Attached patch part 6 - nsTextControlFrame (obsolete) — Splinter Review
Attachment #8937030 - Flags: review?(catalin.badea392)
Priority: -- → P2
See Also: → 1330970
Attachment #8937021 - Attachment is obsolete: true
Attachment #8937021 - Flags: review?(catalin.badea392)
Attachment #8937043 - Flags: review?(catalin.badea392)
Attachment #8937023 - Attachment is obsolete: true
Attachment #8937023 - Flags: review?(catalin.badea392)
Attachment #8937045 - Flags: review?(catalin.badea392)
Attachment #8937024 - Attachment is obsolete: true
Attachment #8937024 - Flags: review?(catalin.badea392)
Attachment #8937050 - Flags: review?(catalin.badea392)
Attached patch part 4 - nsXULTooltipListener (obsolete) — Splinter Review
Attachment #8937027 - Attachment is obsolete: true
Attachment #8937027 - Flags: review?(catalin.badea392)
Attachment #8937051 - Flags: review?(catalin.badea392)
Attached patch part 5 - nsListBoxBodyFrame (obsolete) — Splinter Review
Attachment #8937028 - Attachment is obsolete: true
Attachment #8937028 - Flags: review?(catalin.badea392)
Attachment #8937053 - Flags: review?(catalin.badea392)
Attachment #8937030 - Flags: review?(catalin.badea392) → review+
Attachment #8937043 - Flags: review?(catalin.badea392) → review+
Attachment #8937045 - Flags: review?(catalin.badea392) → review+
Attachment #8937050 - Flags: review?(catalin.badea392) → review+
Attachment #8937051 - Flags: review?(catalin.badea392) → review+
Attachment #8937053 - Flags: review?(catalin.badea392) → review+
Attached patch part 7 - txMozillaXMLOutput (obsolete) — Splinter Review
Attachment #8937181 - Flags: review?(catalin.badea392)
Attached patch part 8 - nsCSSFrameConstructor (obsolete) — Splinter Review
Attachment #8937182 - Flags: review?(catalin.badea392)
Attachment #8937182 - Attachment is obsolete: true
Attachment #8937182 - Flags: review?(catalin.badea392)
Attachment #8937183 - Flags: review?(catalin.badea392)
Attached patch part 9 - nsPresContext (obsolete) — Splinter Review
Attachment #8937184 - Flags: review?(catalin.badea392)
Attached patch part 10 - IMEContentObserver (obsolete) — Splinter Review
Attachment #8937187 - Flags: review?(catalin.badea392)
Attached patch part 11 - nsHtml5TreeOperation (obsolete) — Splinter Review
Attachment #8937244 - Flags: review?(catalin.badea392)
Attached patch part 11 - nsHtml5TreeOperation (obsolete) — Splinter Review
Attachment #8937244 - Attachment is obsolete: true
Attachment #8937244 - Flags: review?(catalin.badea392)
Attachment #8937246 - Flags: review?(catalin.badea392)
Attached patch part 12 - nsTextControlFrame (obsolete) — Splinter Review
Attachment #8937247 - Flags: review?(catalin.badea392)
Attached patch part 13 - nsXULSortService (obsolete) — Splinter Review
Attachment #8937248 - Flags: review?(catalin.badea392)
Attached patch part 14 - XULDocument (obsolete) — Splinter Review
Attachment #8937249 - Flags: review?(catalin.badea392)
Attached patch part 15 - txMozillaXMLOutput (obsolete) — Splinter Review
Attachment #8937250 - Flags: review?(catalin.badea392)
Attached patch part 16 - nsXBLPrototypeBinding (obsolete) — Splinter Review
Attachment #8937252 - Flags: review?(catalin.badea392)
Attached patch part 17 - nsGenericHTMLElement (obsolete) — Splinter Review
Attachment #8937253 - Flags: review?(catalin.badea392)
Attached patch part 18 - nsContentUtils (obsolete) — Splinter Review
Attachment #8937254 - Flags: review?(catalin.badea392)
Attached patch part 11 - nsHtml5TreeOperation (obsolete) — Splinter Review
Attachment #8937246 - Attachment is obsolete: true
Attachment #8937246 - Flags: review?(catalin.badea392)
Attachment #8937255 - Flags: review?(catalin.badea392)
Attached patch part 19 - nsINode (obsolete) — Splinter Review
Attachment #8937297 - Flags: review?(catalin.badea392)
Attached patch part 20 - nsXULContentBuilder (obsolete) — Splinter Review
Attachment #8937298 - Flags: review?(catalin.badea392)
Attachment #8937247 - Attachment is obsolete: true
Attachment #8937247 - Flags: review?(catalin.badea392)
Attachment #8937248 - Attachment is obsolete: true
Attachment #8937248 - Flags: review?(catalin.badea392)
Attachment #8937249 - Attachment is obsolete: true
Attachment #8937249 - Flags: review?(catalin.badea392)
Attachment #8937250 - Attachment is obsolete: true
Attachment #8937250 - Flags: review?(catalin.badea392)
Attachment #8937252 - Attachment is obsolete: true
Attachment #8937252 - Flags: review?(catalin.badea392)
Attachment #8937253 - Attachment is obsolete: true
Attachment #8937253 - Flags: review?(catalin.badea392)
Attachment #8937254 - Attachment is obsolete: true
Attachment #8937254 - Flags: review?(catalin.badea392)
Attachment #8937255 - Attachment is obsolete: true
Attachment #8937255 - Flags: review?(catalin.badea392)
Attachment #8937297 - Attachment is obsolete: true
Attachment #8937297 - Flags: review?(catalin.badea392)
Attachment #8937298 - Attachment is obsolete: true
Attachment #8937298 - Flags: review?(catalin.badea392)
Attachment #8937311 - Flags: review?(catalin.badea392)
Attached patch part 11 - nsHtml5TreeOperation (obsolete) — Splinter Review
Attachment #8937312 - Flags: review?(catalin.badea392)
Attached patch part 12 - nsTextControlFrame (obsolete) — Splinter Review
Attachment #8937313 - Flags: review?(catalin.badea392)
Attached patch part 13 - nsXULSortService (obsolete) — Splinter Review
Attachment #8937314 - Flags: review?(catalin.badea392)
Attached patch part 14 - XULDocument (obsolete) — Splinter Review
Attachment #8937315 - Flags: review?(catalin.badea392)
Attached patch part 15 - txMozillaXMLOutput (obsolete) — Splinter Review
Attachment #8937316 - Flags: review?(catalin.badea392)
Attached patch part 16 - nsXBLPrototypeBinding (obsolete) — Splinter Review
Attachment #8937317 - Flags: review?(catalin.badea392)
Attached patch part 17 - sGenericHTMLElement (obsolete) — Splinter Review
Attachment #8937318 - Flags: review?(catalin.badea392)
Attached patch part 18 - nsContentUtils (obsolete) — Splinter Review
Attachment #8937319 - Flags: review?(catalin.badea392)
Attached patch part 19 - nsINode (obsolete) — Splinter Review
Attachment #8937320 - Flags: review?(catalin.badea392)
Attached patch part 20 - nsXULContentBuilder (obsolete) — Splinter Review
Attachment #8937321 - Flags: review?(catalin.badea392)
Attached patch part 21 - FragmentOrElement (obsolete) — Splinter Review
Attachment #8937322 - Flags: review?(catalin.badea392)
Attachment #8937323 - Flags: review?(catalin.badea392)
Attached patch part 23 - doRemoveChildAt (obsolete) — Splinter Review
I think now, with this patch, RemoveChild is completed. GetChildAt_Deprecated is the missing part.
Attachment #8937324 - Flags: review?(catalin.badea392)
Attachment #8937181 - Flags: review?(catalin.badea392) → review+
Attachment #8937183 - Flags: review?(catalin.badea392) → review+
Comment on attachment 8937184 [details] [diff] [review]
part 9 - nsPresContext

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

::: layout/base/nsCSSFrameConstructor.cpp
@@ +7229,5 @@
>    // Scan the children of aContent to see what operations (if any) we need to
>    // perform.
>    bool inRun = false;
>    nsIContent* firstChildInRun = nullptr;
> +  for (nsIContent* child = aContent->GetFirstChild();

I don't understand this change, is this based on another patch?
Attachment #8937184 - Flags: review?(catalin.badea392)
Attached patch part 9 - nsPresContext (obsolete) — Splinter Review
Attachment #8937184 - Attachment is obsolete: true
Attachment #8939534 - Flags: review?(catalin.badea392)
I'm planning to land these patches step by step.
Keywords: leave-open
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad03ffad73fe
Get rid of GetChildAt_Deprecated in nsTreeUtils, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8bb5d303fa
Get rid of GetChildAt_Deprecated in nsTreeContentView, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8817148653e9
Get rid of GetChildAt_Deprecated in nsTreeColumns, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e89e8d40312
Get rid of GetChildAt_Deprecated in nsXULTooltipListener, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/75eea134e76a
Get rid of GetChildAt_Deprecated in nsListBoxBodyFrame, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac1716f22f0
Get rid of GetChildAt_Deprecated in nsTextControlFrame, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52d6d209933
Get rid of GetChildAt_Deprecated in txMozillaXMLOutput, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/074809c8e266
Get rid of GetChildAt_Deprecated in nsCSSFrameConstructor, r=catalinb
Attachment #8937311 - Flags: review?(catalin.badea392) → review+
Comment on attachment 8937187 [details] [diff] [review]
part 10 - IMEContentObserver

This patch seems to be empty.
Attachment #8937187 - Flags: review?(catalin.badea392)
Attachment #8937312 - Flags: review?(catalin.badea392) → review+
Attachment #8937313 - Flags: review?(catalin.badea392) → review+
Attachment #8937314 - Flags: review?(catalin.badea392) → review+
Attachment #8937315 - Flags: review?(catalin.badea392) → review+
Attachment #8937316 - Flags: review?(catalin.badea392) → review+
Attachment #8937317 - Flags: review?(catalin.badea392) → review+
Attachment #8937318 - Flags: review?(catalin.badea392) → review+
Comment on attachment 8937319 [details] [diff] [review]
part 18 - nsContentUtils

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

The rewrite makes a lot more sense. Thanks!
Attachment #8937319 - Flags: review?(catalin.badea392) → review+
Attachment #8937320 - Flags: review?(catalin.badea392) → review+
Attachment #8937321 - Flags: review?(catalin.badea392) → review+
Comment on attachment 8937322 [details] [diff] [review]
part 21 - FragmentOrElement

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

::: dom/base/nsDocument.cpp
@@ +4548,5 @@
>               "(maybe somebody called GetRootElement() too early?)");
>  }
>  
>  void
> +

nit: remove empty line.
Attachment #8937322 - Flags: review?(catalin.badea392) → review+
Attachment #8937323 - Flags: review?(catalin.badea392) → review+
Attachment #8937324 - Flags: review?(catalin.badea392) → review+
Attachment #8939534 - Flags: review?(catalin.badea392) → review+
Attached patch part 10 - IMEContentObserver (obsolete) — Splinter Review
Attachment #8937187 - Attachment is obsolete: true
Attachment #8940196 - Flags: review?(catalin.badea392)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d47e642852b1
Get rid of GetChildAt_Deprecated in nsPresContext, r=catalinb
Attachment #8940196 - Flags: review?(catalin.badea392) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/804de8550bbd
Get rid of GetChildAt_Deprecated in IMEContentObserver, r=catalinb
Attached patch part 24 - nsDocumentEncoder (obsolete) — Splinter Review
Attachment #8940230 - Flags: review?(catalin.badea392)
Attached patch part 25 - nsFocusManager (obsolete) — Splinter Review
Attachment #8940231 - Flags: review?(catalin.badea392)
Attachment #8940232 - Flags: review?(catalin.badea392)
Attachment #8940621 - Flags: review?(catalin.badea392)
Attachment #8940623 - Flags: review?(catalin.badea392)
Attachment #8940624 - Flags: review?(catalin.badea392)
Attachment #8940626 - Flags: review?(catalin.badea392)
Attachment #8940644 - Flags: review?(catalin.badea392)
Attachment #8940645 - Flags: review?(catalin.badea392)
Attachment #8940230 - Flags: review?(catalin.badea392) → review+
Attachment #8940231 - Flags: review?(catalin.badea392) → review+
Comment on attachment 8940232 [details] [diff] [review]
part 26 - nsGenericDOMDataNode

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

::: dom/base/nsGenericDOMDataNode.cpp
@@ +784,3 @@
>    }
> +
> +  MOZ_CRASH("Impossible state");

Why not just return null here? Crashing seems too much for a function that just does a lookup.
Attachment #8940232 - Flags: review?(catalin.badea392) → review-
Attachment #8940621 - Flags: review?(catalin.badea392) → review+
Attachment #8940623 - Flags: review?(catalin.badea392) → review+
Attachment #8940624 - Flags: review?(catalin.badea392) → review+
Attachment #8940626 - Flags: review?(catalin.badea392) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30b995a49772
Get rid of GetChildAt_Deprecated in nsMenuX, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/90492852dfc9
Get rid of GetChildAt_Deprecated in nsMenuItemX, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8011797b1542
Get rid of GetChildAt_Deprecated in nsMenuBarX, r=catalinb
Attachment #8941870 - Flags: review?(catalin.badea392)
Attachment #8941871 - Flags: review?(catalin.badea392)
Attachment #8941872 - Flags: review?(catalin.badea392)
Attachment #8941873 - Flags: review?(catalin.badea392)
Attachment #8940644 - Flags: review?(catalin.badea392) → review+
Attachment #8940645 - Flags: review?(catalin.badea392) → review+
Attachment #8941870 - Flags: review?(catalin.badea392) → review+
Attachment #8941871 - Flags: review?(catalin.badea392) → review+
Attachment #8941872 - Flags: review?(catalin.badea392) → review+
Attachment #8941873 - Flags: review?(catalin.badea392) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3046ae63cbf
Get rid of GetChildAt_Deprecated in nsRange, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a5df1b4070
Get rid of GetChildAt_Deprecated in selection, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a9a1c11053a
Get rid of GetChildAt_Deprecated in nsFrameSelection, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3975b636f8d
Get rid of GetChildAt_Deprecated in nsTypeAheadFind, r=catalinb
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa20efa4469
Get rid of GetChildAt_Deprecated in nsCSSRuleProcessor, r=catalinb
Depends on: 1430516
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1035d52904d
Get rid of GetChildAt_Deprecated in nsDocumentEncoder, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1c98a4a3d4
Introduce nsINode::RemoveChildNode, r=catalinb
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a65645b66b6
Replace RemoveChildAt_Deprecated with RemoveChildNode in nsHtml5TreeOperation, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f95e1a8de9
Replace RemoveChildAt_Deprecated with RemoveChildNode in nsTextControlFrame, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/293349fdff10
Replace RemoveChildAt_Deprecated with RemoveChildNode in nsXULSortService, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef23aa75c3d
Replace RemoveChildAt_Deprecated with RemoveChildNode in nsGenericHTMLElement, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc568e52a68
Get rid of GetChildAt_Deprecated in nsGenericDOMDataNode, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a3fe239646
Replace RemoveChildAt_Deprecated with RemoveChildNode in FragmentOrElement and nsIDocument, r=catalinb
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbbc0208dc8
Replace RemoveChildAt_Deprecated with RemoveChildNode in nsXBLPrototypeBinding, r=catalinb
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96be89486722
Replace RemoveChildAt_Deprecated with RemoveChildNode in nsContentUtils, r=catalinb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab5dd16c2ff
Replace RemoveChildAt_Deprecated with RemoveChildNode in txMozillaXMLOutput, r=catalinb
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39c98c0933e1
Replace RemoveChildAt_Deprecated with RemoveChildNode in XULDocument, r=catalinb
Duplicate of this bug: 1414233
:baku, update?
Flags: needinfo?(amarchesini)
catalinb, many of these remaining patches have been merged in your commit. Is it landed? Can we mark this bug as resolved and continue to work on yours?
Flags: needinfo?(amarchesini) → needinfo?(catalin.badea392)
bug 651120 is not landed and I haven't merged all of these patches into the main commit. Can we just mark it as depending on 651120? If I remember correctly, the reason we can't land this now is because they depend on InsertBefore/RemoveChild not rely on an index parameter, otherwise these will add an extra ComputeIndexOf operation.

bug is 651120 is blocked on a nsRange/nsContentIterator regression I'm investigating using the testcase from bug 1441124. Our editor code uses selections to clear textarea elements and other input elements, which becomes noticeably slower with bug 651120.
Flags: needinfo?(catalin.badea392)
We need a final answer on this.
Duplicate of this bug: 1330970
:baku all yours
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
The leave-open keyword is there and there is no activity for 6 months.
:baku, maybe it's time to close this bug?
Flags: needinfo?(amarchesini)
Attachment #8937030 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8937043 - Attachment is obsolete: true
Attachment #8937045 - Attachment is obsolete: true
Attachment #8937050 - Attachment is obsolete: true
Attachment #8937051 - Attachment is obsolete: true
Attachment #8937053 - Attachment is obsolete: true
Attachment #8937181 - Attachment is obsolete: true
Attachment #8937311 - Attachment is obsolete: true
Attachment #8937312 - Attachment is obsolete: true
Attachment #8937313 - Attachment is obsolete: true
Attachment #8937314 - Attachment is obsolete: true
Attachment #8937315 - Attachment is obsolete: true
Attachment #8937316 - Attachment is obsolete: true
Attachment #8937317 - Attachment is obsolete: true
Attachment #8937318 - Attachment is obsolete: true
Attachment #8937319 - Attachment is obsolete: true
Attachment #8937320 - Attachment is obsolete: true
Attachment #8937321 - Attachment is obsolete: true
Attachment #8937322 - Attachment is obsolete: true
Attachment #8937323 - Attachment is obsolete: true
Attachment #8937324 - Attachment is obsolete: true
Attachment #8939534 - Attachment is obsolete: true
Attachment #8940196 - Attachment is obsolete: true
Attachment #8940230 - Attachment is obsolete: true
Attachment #8940231 - Attachment is obsolete: true
There is still something to do. But I guess we can close this bug.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
Regressions: 1542530
You need to log in before you can comment on or make changes to this bug.