Bug 41921 – Coalesce text nodes when foster parenting

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Coalesce text nodes when foster parenting

[Adam Barth]

Created attachment 60989 [details]
Patch

[Eric Seidel (no email)]

This feels wrong. Why is the if inside attach at site instead of Just having two separate methods?

[Adam Barth]

Yeah, there's something wrong in the layering.  I'm inclined to land this patch and then think the whole class through in another patch once we see the foreign content part.  The class isn't that complicated.

[Darin Adler]

Comment on attachment 60989 [details]
Patch

Drive-by comments:

> +        // FIXME: We need an insertElement which does not send mutation events.

One way to deal with this is to create a function that promises it won't send mutation events, even if it can't yet deliver on that promise. Then you only need the FIXME in one place.

Generally speaking we should probably have parents in the HTML parser be Element* instead of Node*. We should change things around to make that happen because Element has more efficient versions of some of the functions that you can call on Node in a less efficient fashion.

I might get a chance to review this later, although I assume Eric will beat me to it.

[Adam Barth]

> One way to deal with this is to create a function that promises it won't send mutation events, even if it can't yet deliver on that promise. Then you only need the FIXME in one place.

That's a good idea.

> Generally speaking we should probably have parents in the HTML parser be Element* instead of Node*. We should change things around to make that happen because Element has more efficient versions of some of the functions that you can call on Node in a less efficient fashion.

We can work towards that.  We need to use nodes sometimes because we need to attach things to the document occasionally.

> I might get a chance to review this later, although I assume Eric will beat me to it.

You're welcome to review it, but this patch is ugly.  We're learning more about what this class needs to do, and it's internal structure hasn't quite caught up yet.  I'll try to make the class beautiful before we're done.

[Eric Seidel (no email)]

Comment on attachment 60989 [details]
Patch

Sigh.  We need to fix this.  But I agree, it is clearer to land it first and then work from the fixed version towards a saner wholistic design.

[Adam Barth]

Committed r62924: <http://trac.webkit.org/changeset/62924>

[Darin Adler]

(In reply to comment #5)
> > Generally speaking we should probably have parents in the HTML parser be Element* instead of Node*. We should change things around to make that happen because Element has more efficient versions of some of the functions that you can call on Node in a less efficient fashion.
> 
> We can work towards that.  We need to use nodes sometimes because we need to attach things to the document occasionally.

If it  can be a document, then we can still at least use ContainerNode*.

[Daniel Bates]

Comment on attachment 60989 [details]
Patch

Clearing commit-queue flag to get this out of the commit-queue.