Closed Bug 1449670 Opened 2 years ago Closed Last year

Remove nsINode::eTEXT

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Consumers can use IsText() and GetAsText() instead.
Assignee: nobody → continuation
It turns out that eTEXT is only used for IsNodeOfType(nsINode::eTEXT), so a very simple search and replace handles it. (You also need to do IsNodeOfType(eTEXT) because nsINode.h uses that a few times).
Comment on attachment 8965783 [details]
Bug 1449670, part 1 - Convert IsNodeOfType(nsINode::eTEXT) to IsText().

https://reviewboard.mozilla.org/r/234610/#review240328

::: dom/base/Selection.cpp:2395
(Diff revision 1)
> -  if (endNode->IsNodeOfType(nsINode::eTEXT)) {
> +  if (endNode->IsText()) {
>      // Get the length of the text. We can't just use the offset because
>      // another range could be touching this text node but not intersect our
>      // range.
>      beginOffset = 0;
>      endOffset = static_cast<nsIContent*>(endNode)->TextLength();

Could endNode->AsText() here instead of the cast.

::: dom/base/nsINode.h:430
(Diff revision 1)
> -      !(IsNodeOfType(eTEXT) ||
> +      !(IsText() ||
>          IsNodeOfType(ePROCESSING_INSTRUCTION) ||
>          IsNodeOfType(eCOMMENT) ||
>          IsNodeOfType(eDATA_NODE));

Boy, this code is confused.  eDATA_NODE is only true when the node is comment, textnode, cdata, or PI, so that check is redundant.  And this check is making doctypes be treated as IsContainerNode()....

Anyway, if we want to preserve the current sematics we can just do:

    return IsElement() || !IsCharacterData();
    
but I think returning true here for doctypes is wrong and we should fix this to actually do something sane.  Mind filing a followup on this?

::: dom/base/nsRange.cpp:3077
(Diff revision 1)
>    nsresult rv = iter.Init(aRange);
>    if (NS_FAILED(rv)) return;
>  
>    if (iter.IsDone()) {
>      // the range is collapsed, only continue if the cursor is in a text node
>      nsCOMPtr<nsIContent> content = do_QueryInterface(aStartContainer);

You could avoid this QI like so:

    if (aStartContainer->IsText()) {
      nsTextFrame* textFrame = GetTextFrameForContent(aStartContainer->AsText(), aFlushLayout);
      
etc.

::: dom/base/nsRange.cpp:3104
(Diff revision 1)
>    }
>  
>    do {
>      nsCOMPtr<nsINode> node = iter.GetCurrentNode();
>      iter.Next();
>      nsCOMPtr<nsIContent> content = do_QueryInterface(node);

Should be able to avoi this QI too, but that's more work.  Maybe file a followup?

::: toolkit/components/find/nsFind.cpp:497
(Diff revision 1)
>      return;
>    }
>    nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
>    nsString nodeName = node->NodeName();
>    nsCOMPtr<nsIContent> textContent(do_QueryInterface(aNode));
> -  if (textContent && textContent->IsNodeOfType(nsINode::eTEXT)) {
> +  if (textContent && textContent->IsText()) {

OK, I guess, but this code won't build because AppendTextTo is not on nsIContent anymore.  Luckily, this is inside #ifdef DEBUG_FIND.

::: toolkit/components/find/nsFind.cpp:663
(Diff revision 1)
>      NS_ENSURE_SUCCESS(rv, rv);
>      if (!aStartPoint) {
>        aStartPoint = aSearchRange;
>      }
>  
>      content = do_QueryInterface(mIterator->GetCurrentNode());

I wonder whether we can ditch this QI too.  Followup, please.
Attachment #8965783 - Flags: review?(bzbarsky) → review+
Comment on attachment 8965784 [details]
Bug 1449670, part 2 - Remove nsINode::eTEXT.

https://reviewboard.mozilla.org/r/234612/#review240336
Attachment #8965784 - Flags: review?(bzbarsky) → review+
Status: NEW → ASSIGNED
Priority: -- → P2
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> Could endNode->AsText() here instead of the cast.
Fixed.

> Anyway, if we want to preserve the current sematics we can just do:
>     return IsElement() || !IsCharacterData();
Fixed.

> Mind filing a followup on this?
Filed bug 1453828.

> You could avoid this QI like so:
Fixed.

> Should be able to avoi this QI too, but that's more work.  Maybe file a
> followup?
Filed bug 1454054.

> I wonder whether we can ditch this QI too.  Followup, please.

Filed bug 1454055.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5683308f483
part 1 - Convert IsNodeOfType(nsINode::eTEXT) to IsText(). r=bz
https://hg.mozilla.org/integration/autoland/rev/1344ca26ef0a
part 2 - Remove nsINode::eTEXT. r=bz
https://hg.mozilla.org/mozilla-central/rev/c5683308f483
https://hg.mozilla.org/mozilla-central/rev/1344ca26ef0a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Attachment #8965783 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8965783 - Attachment mime type: text/plain → text/x-review-board-request
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.