344050 - Kill nsITextContent

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There's no real reason to have separate interfaces for nsITextContent and nsIContent. We should just move the text-access methods to nsIContent, all attribute related methods live there anyway, so why not text-related?

I have a working patch, but I need to rename nsIContent::Text() to nsIContent::GetText() since it can return null for elements.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP

This only misses the Text() -> GetText() renaming.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This one's good. The patch does change behaviour in a few case. An old QI to nsITextContent would succeed for comment nodes and text nodes, whereas i've replaced the QIs with IsNodeOfType tests for either nsINode::eTEXT or nsINode::eDATA_NODE. eDATA_NODE is set for comments, text and PIs.

I suspect that pretty much all eDATA_NODE tests should be replaced with eTEXT tests, but i'm worried about modifying code I don't know.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Same as above with -w

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Aaron: Could you take a look at the changes to nsHyperTextAccessible.cpp and let me know if the code should only check for text-nodes, or if it actually wants to find comments and/or processing instructions as well.

Comment 5

[Aaron Leventhal]

r=aaronlev on the accessibility changes, although you can assume we only care about eTEXT. Don't need to check it if the role == ROLE_TEXT_LEAF, because then we know it's eTEXT. You can assert that if you want.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 228732 [details] [diff] [review]
Same as above with -w

+PRBool
+nsGenericElement::TextIsOnlyWhitespace()
+{
+  return PR_FALSE;
+}
+
+void
+nsGenericElement::AppendTextTo(nsAString& aResult)
+{
+}

Would we want to make those two methods warn if they're called? I don't see any reason for anyone to ever get into a situation where those would be called w/o a developer doing something unintended.

r+sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

We should probably assert, not just warn.  Imo...

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

In this patch there are actually places where I do use the fact that those methods return what they return.

For example in nsHTMLCopyEncoder::IsEmptyTextContent and nsHTMLEditor::StripFormattingNodes (and probably nsCSSFrameConstructor, TextIsOnlyWhitespace) we want to skip textnodes that are whitespace-only, but not other types of nodes. Here we won't have to first check for text-ness and then check only-whitespace. But rather we can check only-whitespace directly.

Same thing almost applies to AppendTextTo. However it turns out that most, if not all, places need to check for eTEXT (to avoid getting comments and PIs) anyway, so maybe we could warn or assert. But it feels a bit strange that all other text-related methods should be allowed with AppendTextTo being the only exception.

Let me know what you think.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So where do we use SetText() on elements?  Or for that matter GetText() or TextLength()?  I'll buy that TextIsOnlyWhitespace() is useful for elements if we're using it, but all the other methods don't seem that useful.

Oh, and a comment nit: "concatenation", not "concatination".

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

SetText() should not be used, i agree that that should assert no matter what.

GetText() is used as an alternative way of doing IsNodeOfType(eDATA_NODE) which is more convinient when you want to get the fragment first thing you do anyway.

TextLength() is currently used in nsAccessible::AppendFlatStringFromContentNode and nsCSSFrameConstructor,IsFirstLetterContent to for lengths greater than 0.

Comment 11

[Boris Zbarsky [:bzbarsky]]

nsAccessible::AppendFlatStringFromContentNode already checks for IsNodeOfType(nsINode::eTEXT), so that' ok.  In the frame constructor, the only caller is passing in the GetContent of an nsTextFrame.  So I'd be happy to just assert IsNodeOfType(nsINode::eTEXT) in IsFirstLetterContent() and make TextLength() assert...

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch with comments addressed

This patch addresses the comment from jst and bz by adding the assertion, but also adding a comment saying that we can remove the assertion if it turns out that it'd be useful to be able to call those methods.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in, there was an inverted test in nsComboboxControlFrame.cpp (== should be !=).

It was about a 1% win in Tp on btek.

Comment 14

[Martijn Wargers (dead)]

The patch is checked in, so should it be marked fixed now?

Comment 15

[Ian Neal]

Would the following TB be related to this checkin: TB21266540E ?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Make metrics compile too

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ian, that does look like a regression from this bug.  Please file a bug on it and mark it blocking this one?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 230348 [details] [diff] [review]
Make metrics compile too

Why change the hash to hold nsINodes instead of nsIDocuments?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

...I'd rather that you cast in NodeWillBeDestroyed instead since you'll have to make an unsafe cast somewhere no matter what.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: With the other cast

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 230532 [details] [diff] [review]
With the other cast

Though I just realized, this isn't related to this bug, is it. It's bug 342062 :)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er.... yes, it is.  One sicking bug is much like another, or something.  ;)

Checked that in.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This was checked in some time ago