Closed Bug 1446533 Opened 2 years ago Closed 2 years ago

Get rid of nsIDOMCharacterData

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 3 obsolete files)

74.99 KB, patch
Nika
: review+
Details | Diff | Splinter Review
13.42 KB, patch
Nika
: review+
Details | Diff | Splinter Review
19.03 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.09 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.32 KB, patch
Nika
: review+
Details | Diff | Splinter Review
8.55 KB, patch
Nika
: review+
Details | Diff | Splinter Review
13.35 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.25 KB, patch
Nika
: review+
Details | Diff | Splinter Review
17.88 KB, patch
Nika
: review+
Details | Diff | Splinter Review
2.47 KB, patch
Nika
: review+
Details | Diff | Splinter Review
22.20 KB, patch
Nika
: review+
Details | Diff | Splinter Review
15.17 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.16 KB, patch
Nika
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 1446598
Depends on: 1446599
Blocks: 1132934
MozReview-Commit-ID: IBVkQJOi6O7
Attachment #8959766 - Flags: review?(nika)
Attached patch part 2. Get rid of nsIDOMComment (obsolete) — Splinter Review
MozReview-Commit-ID: GGXPQnlwuUM
Attachment #8959767 - Flags: review?(nika)
Attachment #8959766 - Attachment is obsolete: true
Attachment #8959766 - Flags: review?(nika)
Attachment #8959767 - Attachment is obsolete: true
Attachment #8959767 - Flags: review?(nika)
Patches for this will fix bug 826414 too.
Blocks: 826414
This is not renaming the files yet; that will be a separate changeset.

MozReview-Commit-ID: 5TxkEiQlaKF
Attachment #8959908 - Flags: review?(nika)
The DOMMatrix.cpp changes are because it was sneaking in headers via another
unified file.

MozReview-Commit-ID: GPp9WOywI5D
Attachment #8959909 - Flags: review?(nika)
MozReview-Commit-ID: 5YeaCPwvIJH
Attachment #8959910 - Flags: review?(nika)
MozReview-Commit-ID: DhiN6mCOb34
Attachment #8959911 - Flags: review?(nika)
MozReview-Commit-ID: JP809oJeQiX
Attachment #8959912 - Flags: review?(nika)
MozReview-Commit-ID: FCBGyqPfC4B
Attachment #8959913 - Flags: review?(nika)
MozReview-Commit-ID: 48XZ2J9ewHP
Attachment #8959914 - Flags: review?(nika)
MozReview-Commit-ID: 7100YyU5jOG
Attachment #8959915 - Flags: review?(nika)
MozReview-Commit-ID: Lei6xZ2rw2K
Attachment #8959916 - Flags: review?(nika)
MozReview-Commit-ID: CfZNQRiDs43
Attachment #8959917 - Flags: review?(nika)
MozReview-Commit-ID: 8YLea3SmQQU
Attachment #8959918 - Flags: review?(nika)
MozReview-Commit-ID: KXex3Rjcire
Attachment #8959919 - Flags: review?(nika)
Priority: -- → P2
Blocks: 1447098
FromContent will be renamed to FromNode in bug 1447098.

MozReview-Commit-ID: DhiN6mCOb34
Attachment #8960297 - Flags: review?(nika)
Attachment #8959911 - Attachment is obsolete: true
Attachment #8959911 - Flags: review?(nika)
Comment on attachment 8959908 [details] [diff] [review]
part 1.  Rename nsGenericDOMDataNode to CharacterData

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

I did not read through this entire thing - I'm just gonna rubberstamp it :p
Attachment #8959908 - Flags: review?(nika) → review+
Comment on attachment 8959909 [details] [diff] [review]
part 2.  Rename nsGenericDOMDataNode.{h,cpp} to CharacterData

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

::: dom/base/DOMMatrix.cpp
@@ +11,5 @@
>  #include "mozilla/dom/DOMPoint.h"
>  #include "mozilla/dom/DOMPointBinding.h"
>  #include "mozilla/dom/BindingDeclarations.h"
>  #include "mozilla/dom/ToJSValue.h"
> +#include "mozilla/RuleNodeCacheConditions.h"

Oh the joys of unified builds
Attachment #8959909 - Flags: review?(nika) → review+
Attachment #8959910 - Flags: review?(nika) → review+
Comment on attachment 8959912 [details] [diff] [review]
part 5.  Remove nsIDOMCharacterData::AppendData

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

::: dom/base/CharacterData.cpp
@@ +227,5 @@
> +  nsresult rv = SetTextInternal(mText.GetLength(), 0, aData.BeginReading(),
> +                                aData.Length(), true);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv);
> +  }  

trailing whitespace.
Attachment #8959912 - Flags: review?(nika) → review+
Comment on attachment 8959913 [details] [diff] [review]
part 6.  Remove nsIDOMCharacterData::GetLength

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

::: dom/base/nsRange.cpp
@@ +2731,1 @@
>      {

I just love how consistent our C++ style is in gecko.
Attachment #8959913 - Flags: review?(nika) → review+
Comment on attachment 8959914 [details] [diff] [review]
part 7.  Remove nsIDOMCharacterData::InsertData

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

::: dom/base/CharacterData.cpp
@@ +219,3 @@
>  {
>    nsresult rv = SetTextInternal(mText.GetLength(), 0, aData.BeginReading(),
>                                  aData.Length(), true);

Should this method be made to forward to InsertData?

@@ +232,5 @@
> +  nsresult rv = SetTextInternal(aOffset, 0, aData.BeginReading(),
> +                                aData.Length(), true);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv);
> +  }  

trailing whitespace.
Attachment #8959914 - Flags: review?(nika) → review+
Comment on attachment 8959915 [details] [diff] [review]
part 8.  Remove nsIDOMCharacterData::DeleteData

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

::: dom/base/CharacterData.cpp
@@ +241,5 @@
>  {
> +  nsresult rv = SetTextInternal(aOffset, aCount, nullptr, 0, true);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv);
> +  }  

I have a feeling you copy-pasted the same function a bunch of times :p

::: editor/libeditor/CompositionTransaction.cpp
@@ +131,5 @@
>        while (node && node->IsNodeOfType(nsINode::eTEXT) &&
>               remainLength > 0) {
>          Text* text = static_cast<Text*>(node.get());
>          uint32_t textLength = text->TextLength();
> +        text->DeleteData(0, remainLength, IgnoreErrors());

Random side comment: I really like how the IgnoreErrors() reads in this code - it's so much more clear than the old way of doing it.
Attachment #8959915 - Flags: review?(nika) → review+
Attachment #8959916 - Flags: review?(nika) → review+
MozReview-Commit-ID: KTEgqc4Wzjd
Attachment #8960306 - Flags: review?(nika)
Attachment #8959917 - Flags: review?(nika) → review+
Attachment #8959918 - Flags: review?(nika) → review+
Comment on attachment 8959919 [details] [diff] [review]
part 12.  Remove nsIDOMCharacterData

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

::: dom/base/DocumentType.cpp
@@ +73,2 @@
>    // data nodes (they have a null .nodeValue and don't implement
> +  // CharacterData)

This reads really confusingly
Attachment #8959919 - Flags: review?(nika) → review+
Attachment #8960297 - Flags: review?(nika) → review+
Comment on attachment 8960306 [details] [diff] [review]
part 6.5.  Fix some old whitespace bits in range code

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

thanks
Attachment #8960306 - Flags: review?(nika) → review+
> trailing whitespace.

Fixed.

> I just love how consistent our C++ style is in gecko.

Mmm... This is kin code.  Very old.  I will just fix this.  ;)

> Should this method be made to forward to InsertData?

Good point.  Done.

> trailing whitespace.

Fixed.

> I have a feeling you copy-pasted the same function a bunch of times :p

Well, the same failure check...  Fixed.

> I really like how the IgnoreErrors() reads in this code

Yeah, that part felt nice.  ;)

> This reads really confusingly

I will make it clearer that this refers to the DOM interface.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/899edac390aa
part 1.  Rename nsGenericDOMDataNode to CharacterData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/77630b78e554
part 2.  Rename nsGenericDOMDataNode.{h,cpp} to CharacterData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/e43988b4ba26
part 3.  Remove nsIDOMCharacterData::Get/SetData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd517334cd5
part 4.  Remove nsIDOMCharacterData::SubstringData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/87162a82c062
part 5.  Remove nsIDOMCharacterData::AppendData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1182d32f823e
part 6.  Remove nsIDOMCharacterData::GetLength  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4984d2a063a
part 6.5.  Fix some old whitespace bits in range code.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3ac3c3b989
part 7.  Remove nsIDOMCharacterData::InsertData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/e429f3715268
part 8.  Remove nsIDOMCharacterData::DeleteData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10a7fadaee8
part 9.  Remove nsIDOMCharacterData::ReplaceData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/6525e7ba0b1d
part 10.  Remove remaining nsIDOMCharacterData uses in editor.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/18e3eb86e416
part 11.  Remove remaining nsIDOMCharacterData uses in range code.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f71b21eb19
part 12.  Remove nsIDOMCharacterData.  r=mystor
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b72f16720e5
part 7 typo fix to reopen the CLOSED TREE.  r=bzbarsky
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.