Closed Bug 235342 Opened 21 years ago Closed 4 years ago

Make attribute-mapping code more generic

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED INACTIVE

People

(Reporter: sicking, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

clean up comments and function names (checked in to trunk, 2004-02-25 13:04 -0800)
114.89 KB, patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
This is a spinoff from bug 234485 comment 27. Currently the attribute-mapping code is only used by html-elements and very likly contains code that makes it html-specific. This bug is about making the code generic enough that we can use it for other types of content. At least XUL, SVG and MathML could use it. > > > This is because the |MapAttributesInto| business is sooo HTML-specific... > > > > With sicking's changes, this should be pretty simple to fix. > > I hope it is in the plan. I have been waiting for that to happen. What exactly is html-specific about MapAttributesInto and nsMappedAttributes? I don't doubt they are, but I can't fix them unless i know the problem. The only thing i can think of off hand is the fact that GetAttributeMappingFunction lives on nsIHTMLContent. It should be moved to nsIStyledContent. Additionally there might be some stuff in nsHTMLDocument that should live in nsDocument, but I havn't checked if that is the case. > > > Actually, the mapping functions are _constant_ > > > > I'm not sure what this means, exactly... > > C.f. |GetAttributeMappingFunction|, it returns a function based on the tag. It > is currently designed so that different instances of the object could return > different things. But in practice, different instances return the same thing > and this suggests that a static tag-based table-driven approach is > appropriate. Different instances of nsIHTMLContent _do_ return different things. The function is overridden by different elements and returns different things. This is needed because not all html-elements maps the same attribute-names into the same things. For example size=1 means different things for a <font> and for an <input>, so different mappingfunctions are needed. > > >based on the namespace directly > > > >Wouldn't that break for XML nodes in the null namespace vs non-XML HTML > >nodes? > > What do you mean? The behavior won't be any different from what is there at > the moment. Attributes are either mappable or they aren't, based on the > (attrNamespaceID, attrName). Not quite. HTML is in the nullnamespace, as is XML by default. XHTML and HTML have different namespaces. This is how W3C specifies it, and how parts of mozilla works. Don't look at the code for nsGenericHTMLElement::GetNameSpaceID, it is buggy and we're trying to fix it. Though this isn't impossible to overcome, you can easily ask an nsIContent weather it's an (x)html element or not. > > have 10 <font size="3"> tags > > The management is pretty involved at the moment. With the table-driven > approach, there would just be the hard-coded tables as illustrated earlier. > The question is whether that won't work and why. For 10 <font size=3> tags we want to create only one nsIStyleRule. This is a neccesity since otherwise the stylesystem will create a huge ruletree which is bloaty (and maybe slow). In other words, we need to find all elements with the same set of mapped attributes (both same ones set and set to same value) that shares attribute-mapping function. This is what nsMappedAttributes does for us, and the only thing it does. How would that, and the fact that size=1 means different things for different elements, be handled for a table-driven approach?
>What exactly is html-specific about MapAttributesInto and nsMappedAttributes? I >don't doubt they are, but I can't fix them unless i know the problem. Since you are deeply familiar with this code, I think the easiest way for you to appreciate the issues is to attempt the mapping of "xml:lang" which is applicable to every XML/XHTML element. There is a code that already does the mapping of "lang". At first, you will think that it is straightforward to extend it to "xml:lang". But will soon discover that there are ramifications (which, BTW, are much less than before your massive attribute rework, see?). The mapping attempt will give a direct hand-on experience and shed a much greater light than any incomplete ramblings that I may write here. As for your other comments and what bz wrote earlier: > More to the point, what will be the nsIStyleRule objects involved? That's the missing link... Maybe the table-driven suggestion is smoke in the air, maybe it isn't. I will comment to the other remarks as I read the code and gather more information.
With further understanding thanks to your comments, I am seeing that what I had in mind won't achieve the sharing. But is sharing even needed? That's the question. The stylerule can be (implicitly) constructed from the attrs anytime. What I was thinking was along the followong lines: assume, for a start, that each content node has its own stylerule (possibly empty/null) in the internal-attribute-stylesheet, c.f. the earlier link with useful comments: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/public/nsIStyleRule.h&rev=3.22&mark=54-82#54 Then when the style system asks the node to WalkContentStyleRules(), the node would directly map its attributes with its stylerule (i.e., if the node has attributes that matter). Currently, here is WalkContentStyleRules: nsresult nsGenericHTMLElement::WalkContentStyleRules(nsRuleWalker* aRuleWalker) { mAttrsAndChildren.WalkMappedAttributeStyleRules(aRuleWalker); return NS_OK; } It is from here that the attributes get ultimately mapped into the style data, using the the attr stylerule [nsAttrAndChildArray::mImpl->mMappedAttrs] created in SetAttr). Assuming that there is no sharing, i.e., each node has its attr style rule, WalkContentStyleRules() could call a static function with the appropriate arguments (e.g., the attr stylerule of the element). Now... assume that one is bold enough to push the logic this way: nsresult nsGenericHTMLElement::WalkContentStyleRules(nsRuleWalker* aRuleWalker) { // a stack nsIStyleRule init'ed from the attrs (i.e., the relevant // attribute mapping function) nsAutoAttrStyleRule rule(this); if (rule.HasAttributeDependentStyle()) aRuleWalker->Forward(rule); return NS_OK; } Bold, isn't it?!? But that is the kind of scheme that a static table-driven approach would lead to. Its sole purpose is to fill-in the style data. Maybe I am missing something? Won't this work? I didn't see any particular reason to keep the attr stylerules -- other than for the sharing. But mindful of the fact there is a correspondence between the attrs and their stylerule, is saving/sharing the _attribute rules_ even needed? So a direct route eliminates the middle-man, but precludes any sharing of the attr stylerules. As soon as one thinks of sharing, management comes into the picture, and this brings back the kind of genuflexions that |nsMappedAttributes| is doing.
> Maybe I am missing something? Won't this work? It won't. The ruletree keeps a pointer to the rule and fills in the style structs lazily as they are needed. Therefore the rule must not go away until the rulenode in question does (which means it can't be stack-allocated). > But is sharing even needed? Absolutely. It reduces memory consumption a _lot_. For typical pages that use the same attrs over and over on <font> and <td>, we're probably talking an order of magnitude decrease in the size of the ruletree, if not more.
I still think that pretty much all coded needed is there to map xml:lang into style. However there might be more new code involved then should be needed, which would be good to test. What we should do is to move the implementation of SetAttr to nsGenericContainerElement, then all that should be needed is to implement HasAttributeDependantStyle, GetAttributeChangeHint and GetAttributeMappingFunction. Btw, currently the code for GetAttributeChangeHint in html is really strange. Very few elements implement it, and only for very few attributes. Is this a bug? It does look like the cssframeconstructor actually uses the function. An idea would be to merge HasAttributeDependantStyle and GetAttributeChangeHint, so that HasAttributeDependantStyle is just a tiny inline function that calls GetAttributeChangeHint. Though that might be bad for performance. Another issue is that many of these functions only supports attributes in the null namespace. xml:lang would be the first mapped attribute that is namespaced. This might or might not need to be fixed depending on the outcome of bug 234485
GetAttributeChangeHint is for attributes that are *not* mapped into style but whose changes need to force repaint/reflow/reframe anyway. I split those into two functions for a reason.
And that's pretty clearly documented at: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/public/nsIStyledContent.h&rev=1.19&mark=79-87#79 Maybe I should stop commenting my code since nobody reads the comments I write.
>It won't. The ruletree keeps a pointer to the rule and fills in the style >structs lazily as they are needed. Therefore the rule must not go away until >the rulenode in question does (which means it can't be stack-allocated). OK... I take it that the stack way is too bold... [To clarify my earlier comment which caused sicking to say in comment 0 that "different instances of nsIHTMLContent _do_ return different things. The function is overridden by different elements and returns different things." That is right. What I meant was that: all <font> elements always return MapFontAttrInto (generic name, not literally), all <hr> elements return MapHRAttrInto, etc.] Going back to my previous description where each node has its stylerule. Assume now that there is one stylerule per type of element, rather than one stylerule per content instance as I mentioned earlier. That is, assume now that there is one font-stylerule, one hr-stylerule, etc. If this work, it would achieve even a greater sharing than |nsMappedAttributes|. This might answer your reservations about the savings. Anyway, enough tiresome talking, here is patch... (proof-of-concept). It seems to be working -- did some limited testing as it is just a proof-of-concept to get your expert feedback and avoid wasting time in this direction if it is already doomed to lead nowhere. In the code, mRuleMapper is MapFontAttrInto for <font> elements, whereas it is MapHRAttrInto for <hr> elements, etc. The reason why the patch seems to be working comes from the fact that these mappers do not have a state that depends on the stylerule instance as such. They fill the style data dynamically. If the proof-of-concept is acceptable, it would mean that the |nsMappedAttributes| business can be removed/simplified. I will give up if this still doesn't work :-)
Re: comment #6 Keep up the good comments... they proove extremely valuable when need arise, such as at the moment...
Comment on attachment 142164 [details] [diff] [review] share stylerule per type-of-element (proof-of-concept) This doesn't do anything other than cause lots of hash collisions.
Attachment #142164 - Attachment is obsolete: true
Comment on attachment 142164 [details] [diff] [review] share stylerule per type-of-element (proof-of-concept) No, this won't work. The reason is that you want different stylerules when different attributes are set. Otherwise having two elements like: <font color="blue"> <font size="1"> will behave the same, which they of course shouldn't. One should set the fontcolor, one should set the fontsize.
I changed nsMappedAttributes::Equals() to just test for the rule mapper (to address the collision that dbaron mentioned). It doesn't work because the current code keeps the state. What I thought was that the mapper could query the current element on the fly to get the attr values directly rather than storing them itself (this is the no-state I allued to). When we have "font {size: 50%}", it is shared, but it returns different values depending on the context because it is referenced multiple times in the _rulenodes_ (i.e., the rulenodes are not shared). I thought the same could be done for attributes. Conceptually: what I proposed is as if we had "font { &mapExistingFontAttr() }". All is constant here (type-of-element, address of its mapping function). And so could be put in the suggested attr stylerule.
Some of the comments in this bug led me to realize that the current function names were somewhat unclear. The version of HasAttributeDependentStyle on content nodes is really IsAttributeMapped. (That being unclear existed before I did the split mentioned in comment 5 -- but then the lack of clarity caused bugs as well.) I renamed that and AttributeDependenceEntry -> MappedAttributeEntry, and fixed some comments.
There is no such thing as the 'current element' since the same nsMappedAttributes are used for several elements. Though you could *maybe* get the element that is currently being matched from the stylesystem, that violates the contract of nsIStyleRule since it says that the nsIStyleRule should behave the same independently of which node is being matched. So a rule like you're describing is conceptually impossible in our stylesystem. And making changes to the stylesystem to allow such rules would most likly reduce it's efficency enourmously.
Indeed, a given rule should always return the same style. Otherwise caching of style data on the ruletree will break very badly.
sicking wrote: >nsIStyleRule should behave the same independently of which node is being matched bz wrote: >Indeed, a given rule should always return the same style. Maybe I don't get it, sorry. The proposed stylerule, "font { &mapExistingFontAttr() }", is "behaving" the same. It "returns" a static address. Takes the analogy with "font { size: 50% }" which can be written as "font { &ReduceSizeTo50percent() }" [Note that |nsMappedAttributes| won't be there anymore, and the issue of them being used by several elements don't apply.]
no, they are different: "font-size: 50%" is a specific value that can be stored in the stylesystem. I.e. "half of my parent" is a storable value. "the values from this object over there" is not a value storable in the stylesystem.
From my reading, there are _two_ passes. The first pass gathers the stylerules that match, and the second pass goes over the rules to buildup the style data. It is in the first pass that the attr stylerule ("the values from this object over there") apply. In the second pass, the values are effectively filled-in.
Well, not exactly. When we have things like 'font-size: 50%' we disable caching in the rule tree (see the variable called |inherited| in all the nsRuleNode::Compute*Data functions). However, we don't expect to cache structs with inherited properties in the rule tree -- we expect to share them across ancestors/descendants the style context tree to achieve similar gains. However, disabling caching in the rule tree for all mapped attributes would increase memory use and time spent computing style data, especially for the reset structs.
comment 18 was a response to comment 16. I don't understand comment 17, although I recommend reading the comments in nsIStyleRuleProcessor.h and nsIStyleRule.h.
>I don't understand comment 17, Actually, comment 18 answers it too. Comment 17 implied disabling any cache in the rule tree for all mapped attributes, and to let the style data to be recomputed -- which you are saying would be expensive.
(In reply to comment #17) I think you're sort of right (if i understand your comment correctly). But the first pass is only ran once, subsequent reflows/style-reresolution and document-modification won't neccesarily run the first pass again. I recommend that you follow dbarons comments are read up a bit on the stylesystem before suggesting more changes to it.
I did read the code a bit, and wanted something less convoluted, and extensible to other contexts (I have been waiting for years to map MathML attributes, see again?). I think my proposal has some merits, but... I think you understand it now. Once the stylerules are gathered (once), the style data can be build (or rebuild) from them. Hence, the mecanism would seem okay. I am not quite sure about the trade-off, though. Contrary to my earlier comment, maybe the cache doesn't have to be disabled. Also, there will be sharing in the stylecontexts, but as for the recomputation, I confess I can't say if it is going to be much more expensive than what |nsMappedAttributes| is doing. The aapping amounts to calling the various MapAttributesIntoRule() and |nsMappedAttributes| is doing that anyway.
What does this have to do with extending attribute mapping to work for non-HTML documents or with namespaces?
The |nsMappedAttributes| is the class that handles the mapping of attrs to style. Because it (mostly, its predecessors + related, to be fair) have been sooo html-specific, not much has been happening for non-HTML contexts. Should they remain too complex (or require massive changes as the recent rework has shown), I fear that the progress will be hampered.
I really don't think it's hard to extend to work for non-HTML.
I agree, I think pretty much most of the work to make it more generic is already done as part of bug 195350. The only exception is that namespaced mapped attributes arn't supported yet, but that shouldn't affect MathML, and is in general something i'd like to avoid unless absolutly needed.
What do you guys think about my proposed simplification (it took much longer to iron out the details). Is it worth a try or not? I could give it a go, but why bother if your gut-feelings is that it is going to be too slow/bloaty?
I don't really see the benefit. The current scheme is fairly clean (nsMappedAttributes isn't that big) and allows sharing of both stylerules and attributevalues. The actual code for doing the mapping would be more or less the same, right. It's just how we invoke it that would be different.
Comment on attachment 142172 [details] [diff] [review] clean up comments and function names (checked in to trunk, 2004-02-25 13:04 -0800) The only non-search-replace changes in this patch are in the first two files and nsHTMLStylesheet.
Attachment #142172 - Flags: superreview?(jst)
Attachment #142172 - Flags: review?(jst)
Comment on attachment 142172 [details] [diff] [review] clean up comments and function names (checked in to trunk, 2004-02-25 13:04 -0800) r+sr=jst
Attachment #142172 - Flags: superreview?(jst)
Attachment #142172 - Flags: superreview+
Attachment #142172 - Flags: review?(jst)
Attachment #142172 - Flags: review+
Attachment #142172 - Attachment description: clean up comments and function names → clean up comments and function names (checked in to trunk, 2004-02-25 13:04 -0800)
Assignee: general → nobody
QA Contact: ian → general
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046 Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5. If you have questions, please contact :mdaly.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: