Closed Bug 235342 Opened 21 years ago Closed 3 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: 3 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: