313343 - Pass nsIStyledContent to nsIDocumentObserver::AttributeChanged

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This will help us avoid a QI in nsCSSFrameConstructor and not affect any other
callers.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so.

Comment 2

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

Comment on attachment 200419 [details] [diff] [review]
Like so.

The added include shouldn't be needed in:

>Index: extensions/transformiix/source/xpath/nsXPathResult.cpp
>Index: extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp
>Index: docshell/shistory/src/nsSHEntry.cpp

with that r=me

Comment 3

[Johnny Stenback (:jst)]

I'm not strongly against this, but wouldn't it make more sense to simply get rid of nsIStyledContent all together and move all its methods to nsIContent? Now we essentially require that all elements implement nsIStyledContent to play nice in the document observer world, why not simply say that all nsIContent classes need to expose what's currently in nsIStyledContent? 

Comment 4

[Boris Zbarsky [:bzbarsky]]

We could do that, sure.  sicking, dbaron, what do you think?

Comment 5

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

I was considering that solution too. I don't really have a strong oppinion either way, but I also don't think we should add more stuff to nsIContent unless we actually gain something from it. I.e. if it would be a performance of code-cleanness benefit then it sounds like a good idea, if not I don't mind keeping nsIStyledContent and renamed it nsIElement

Comment 6

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

That said, I've been pondering merging nsITextContent into nsIContent :)

And looking at the interface it's almost all about various attribute things and we have most attribute stuff on nsIContent anyways...

Comment 7

[Boris Zbarsky [:bzbarsky]]

So I really don't care either way.  Let's just pick something and then I'll implement it...

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 200419 [details] [diff] [review]
Like so.

I do still think that this would be cleaner if we'd simply eliminated nsIStyledContent since now we're making impossible for nsIContent implementations that are not nsIStyledContent from sending AttributeChanged notifications. Not that such classes exist (where it matters), but still, it seems a bit odd to require implementing this interface to be able to tell observers about an attribute change.

Wanna file a followup bug to change that?

sr=jst for this change in any case though...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Filed bug 313968 on eliminating nsIStyledContent, and landed this.