313249 - implement nsIStyledContent::FromContent

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Looking the attachment in bug 313207 we QI to nsIStyledContent a lot. These QIs
should be replacable with something like:

 static nsIStyledContent* FromContent(nsIContent *aContent)
 {
   if (aContent->IsContentOfType(eELEMENT))
     return NS_STATIC_CAST(nsIStyledContent*, aContent);
   return nsnull;
 }

Comment 1

[Boris Zbarsky [:bzbarsky]]

Without having any hard data to back this up but based on an LXR search, I would
guess that most of these QIs are from the AttributeChanged impl in the CSS frame
constructor (since I was instrumenting in bug 313207 in a build that already has
bryner's CSSStyleSheet patch to do exactly the sort of casting sicking
proposes).  So I was actually thinking that perhaps AttributeChanged should take
an nsIStyledContent, not an nsIContent.  The callers are basically all concrete
classes passing |this|.  And we only have attributes on elements, right?

Comment 2

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

That sounds like a great idea to do in addition to comment 0. Because yes, we
only have attributes on elements.

Comment 3

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

Really, we could do the same with ContentStatesChanged ContentAppended
ContentInserted and ContentRemoved

Comment 4

[Boris Zbarsky [:bzbarsky]]

For content states changed, I believe we might call it on textnodes right now
(for hover, eg).

For those others, we could do it for the container, maybe.  But that would
involve some caller-side QIs since some places (content sink, eg) currently have
an nsIContent.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Is it time to rename nsIStyledContent to nsIElement yet, btw?  ;)

Comment 6

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

good point about the QIs. Though if we do rename nsIStyledContent to nsIElement
it would make sense to let the sinks deal with those rather then nsIContent.
That might be a big change with doubtfull win though, especially with the
"fast-cast"

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah.  More to the point, as things stand we use nsIStyledContent in the
following places (per lxr):

nsHTMLTableCellElement::WalkContentStyleRules (needs to walk style rules on the
   table)
nsXMLEventsListener::HandleEvent (calls GetID()).
nsCSSFrameConstructor::AttributeChanged (needs to call GetAttributeChangeHint).
style resolution (already uses an IsContentOfType check + cast).

That's it.  So changing the sinks would have no benefit -- no consumer of
ContentAppended/Inserted/Removed actually wants the nsIStyledContent interface.

Comment 8

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

GetID() should move to nsIContent anyway, that's where we have all the other
attribute stuff.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'd rather GetID() went away, at least if we plan to do xml:id.

Comment 10

[Boris Zbarsky [:bzbarsky]]

nsIStyledContent is no more.

Comment 11

[Jason Bassford]

"Fixed" by bug 313968.