| Summary: | Coalesce text nodes when foster parenting | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | darin, eric | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 41123 | ||||||
| Attachments: |
|
||||||
|
Description
Adam Barth
2010-07-08 17:36:19 PDT
Created attachment 60989 [details]
Patch
This feels wrong. Why is the if inside attach at site instead of Just having two separate methods? 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. 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. > 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. 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.
Committed r62924: <http://trac.webkit.org/changeset/62924> (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*. Comment on attachment 60989 [details]
Patch
Clearing commit-queue flag to get this out of the commit-queue.
|