1449670 - Remove nsINode::eTEXT

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Consumers can use IsText() and GetAsText() instead.

Comment 1

[Andrew McCreight [:mccr8]]

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 2

[Andrew McCreight [:mccr8]]

Attachment: simple script to do the replacement

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1449670, part 1 - Convert IsNodeOfType(nsINode::eTEXT) to IsText().

This method is not a virtual call, and also looks nicer.

This patch was mostly generated by a Python script, but I manually
cleaned up the code in a few places where statements didn't need to be
split across multiple lines any more.

Review commit: https://reviewboard.mozilla.org/r/234610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234610/

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1449670, part 2 - Remove nsINode::eTEXT.

Review commit: https://reviewboard.mozilla.org/r/234612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234612/

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8965784 [details]
Bug 1449670, part 2 - Remove nsINode::eTEXT.

https://reviewboard.mozilla.org/r/234612/#review240336

Comment 7

[Andrew McCreight [:mccr8]]

(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.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8965783 [details]
Bug 1449670, part 1 - Convert IsNodeOfType(nsINode::eTEXT) to IsText().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234610/diff/1-2/

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8965784 [details]
Bug 1449670, part 2 - Remove nsINode::eTEXT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234612/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c5683308f483
https://hg.mozilla.org/mozilla-central/rev/1344ca26ef0a