Closed Bug 1433566 Opened 2 years ago Closed 2 years ago

Remove nsIDOMText

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: ehsan, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 2 obsolete files)

1.94 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.41 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.72 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.19 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.21 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.03 KB, patch
Nika
: review+
Details | Diff | Splinter Review
1.07 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.43 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.44 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.36 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.98 KB, patch
Nika
: review+
Details | Diff | Splinter Review
14.86 KB, patch
Nika
: review+
Details | Diff | Splinter Review
2.85 KB, patch
Nika
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch Remove nsIDOMText (obsolete) — Splinter Review
Attachment #8945950 - Flags: review?(kyle)
Attachment #8945950 - Flags: review?(kyle) → review+
Priority: -- → P2
Ehsan, is there something blocking this landing?
Flags: needinfo?(ehsan)
Duplicate of this bug: 1445354
Yes, when I pushed this to try a long time ago it broke tests.  Unfortunately I haven't had time to look into why yet.  :-/
Flags: needinfo?(ehsan)
OK.  Do you mind if I push to try the patches I wrote in bug 1445354?
Those patches are green on try, fwiw.  Mind if I just steal the bug?
Flags: needinfo?(ehsan)
Not at all, please go ahead.
Assignee: ehsan → bzbarsky
Flags: needinfo?(ehsan)
MozReview-Commit-ID: Ik5hGubUDqX
Attachment #8959416 - Flags: review?(nika)
MozReview-Commit-ID: LtAhn223y0s
Attachment #8959417 - Flags: review?(nika)
MozReview-Commit-ID: 1jghu75LKDw
Attachment #8959418 - Flags: review?(nika)
This is mostly a straightforward copy.  The only change is what happens with
the return value of CloneDataNode: it's cast to Text directly, and the
null-check is replaced by an assert.

MozReview-Commit-ID: B5rnhEOneZn
Attachment #8959421 - Flags: review?(nika)
MozReview-Commit-ID: I8ZB56a82ze
Attachment #8959422 - Flags: review?(nika)
Just code motion, no changes.

MozReview-Commit-ID: 98LkyuU4mya
Attachment #8959424 - Flags: review?(nika)
MozReview-Commit-ID: 4M4q41mEpgb
Attachment #8959425 - Flags: review?(nika)
MozReview-Commit-ID: 2EY6w5YlLCH
Attachment #8959426 - Flags: review?(nika)
MozReview-Commit-ID: KliYtj5U8jK
Attachment #8959427 - Flags: review?(nika)
MozReview-Commit-ID: DvaZ96j5exf
Attachment #8959428 - Flags: review?(nika)
Attachment #8945950 - Attachment is obsolete: true
Attachment #8959416 - Flags: review?(nika) → review+
Attachment #8959417 - Flags: review?(nika) → review+
Comment on attachment 8959418 [details] [diff] [review]
part 3.  Remove nsIDOMText::SplitText

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

::: dom/interfaces/core/nsIDOMText.idl
@@ +11,2 @@
>   *
>   * For more information on this interface please see 

I'd complain about the trailing whitespace, but we all know how this patchset is going to end ^_^
Attachment #8959418 - Flags: review?(nika) → review+
Attachment #8959419 - Flags: review?(nika) → review+
Attachment #8959420 - Flags: review?(nika) → review+
Comment on attachment 8959421 [details] [diff] [review]
part 6.  Inline SplitData into its one caller

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

::: dom/base/Text.cpp
@@ +33,5 @@
> +  mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, true);
> +
> +  // Use Clone for creating the new node so that the new node is of same class
> +  // as this node!
> +  nsGenericDOMDataNode* clone = CloneDataNode(mNodeInfo, false);

Eek - this is returning a fresh pointer to a refcounted object without addrefing it?

Can you file a follow-up to change CloneDataNode to return an already_AddRefed?
Attachment #8959421 - Flags: review?(nika) → review+
Attachment #8959422 - Flags: review?(nika) → review+
Comment on attachment 8959423 [details] [diff] [review]
part 8.  Convert nsGenericDOMDataNode::GetWholeText to webidl-style calling convention

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

::: dom/base/Text.h
@@ +27,5 @@
>    using nsGenericDOMDataNode::GetWholeText;
>  
>    // WebIDL API
>    already_AddRefed<Text> SplitText(uint32_t aOffset, ErrorResult& rv);
>    void GetWholeText(nsAString& aWholeText, ErrorResult& rv)

Why can't we toss this method completely, and just use the `using` statement 4 lines above (or even toss them both?)

::: dom/base/nsGenericDOMDataNode.cpp
@@ +749,5 @@
>    nsIContent* parent = GetParent();
>  
>    // Handle parent-less nodes
> +  if (!parent) {
> +    GetData(aWholeText);

GetData always returns NS_OK - but it might be good to have an ALWAYS_SUCCEEDS here?
Attachment #8959423 - Flags: review?(nika) → review+
Comment on attachment 8959424 [details] [diff] [review]
part 9.  Move GetWholeText over into Text.cpp

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

::: dom/base/Text.h
@@ +27,3 @@
>    // WebIDL API
>    already_AddRefed<Text> SplitText(uint32_t aOffset, ErrorResult& rv);
> +  void GetWholeText(nsAString& aWholeText, ErrorResult& rv);

ah. this is fine then.
Attachment #8959424 - Flags: review?(nika) → review+
Comment on attachment 8959425 [details] [diff] [review]
part 10.  Stop using nsIDOMText in GetWholeText

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

::: dom/base/Text.cpp
@@ +66,1 @@
>  FirstLogicallyAdjacentTextNode(nsIContent* aNode)

I think this would be cleaner if you change the argument type from nsIContent* to Text*.

@@ +66,4 @@
>  FirstLogicallyAdjacentTextNode(nsIContent* aNode)
>  {
>    do {
> +    MOZ_ASSERT(aNode->IsText());

This assert would be unnecessary.

@@ +71,3 @@
>      nsIContent* sibling = aNode->GetPreviousSibling();
>      if (!sibling || !sibling->IsText()) {
> +      return static_cast<Text*>(aNode);

This cast would be unnecessary, and.

@@ +74,2 @@
>      }
>      aNode = sibling;

You could just cast here because you just checked it was a Text on the previous line.

@@ +80,1 @@
>  LastLogicallyAdjacentTextNode(nsIContent* aNode)

Same with this one.

@@ +111,5 @@
>      aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>      return;
>    }
>  
> +  Text* first = FirstLogicallyAdjacentTextNode(this);

We even always pass in a `this`, which we already know is Text :-)
Attachment #8959425 - Flags: review?(nika) → review-
Attachment #8959426 - Flags: review?(nika) → review+
Attachment #8959427 - Flags: review?(nika) → review+
Attachment #8959428 - Flags: review?(nika) → review+
Blocks: 1387169
> Can you file a follow-up to change CloneDataNode to return an already_AddRefed?

Bug 1446530.

> GetData always returns NS_OK - but it might be good to have an ALWAYS_SUCCEEDS here?

I'm just going to work on bug 1446533 instead.  ;)

> I think this would be cleaner if you change the argument type from nsIContent* to Text*.

Good point.  Fixed.
Attachment #8959425 - Attachment is obsolete: true
Attachment #8959697 - Flags: review?(nika) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/708098d3b655
part 1.  Add a non-virtual nsINode::IsText().  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f45612dea4
part 2.  Remove use of nsIDOMText from JS.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/808ac1023bd6
part 3.  Remove nsIDOMText::SplitText.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/de3b0870addd
part 4.  Remove unused aCloneAfterOriginal arg of nsGenericDOMDataNode::SplitData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5933f9858344
part 5.  Convert nsGenericDOMDataNode::SplitData to webidl calling conventions.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/aabc687a81eb
part 6.  Inline SplitData into its one caller.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9b70292b553
part 7.  Remove nsIDOMText::GetWholeText.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/db5a1e68d3b1
part 8.  Convert nsGenericDOMDataNode::GetWholeText to webidl-style calling convention.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6db91d55c9c
part 9.  Move GetWholeText over into Text.cpp.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/e91f19e48e76
part 10.  Stop using nsIDOMText in GetWholeText.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/28033c867851
part 11.  Stop using nsIDOMText in range code.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/f568bf6abac5
part 12.  Stop using nsIDOMText in layout.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/e693589dd8d4
part 13.  Remove nsIDOMText.  r=mystor
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.