275196 - xml:id support

Status: RESOLVED WONTFIX

(Core :: XML, defect)

Description

[Dominik]

I know that xml:id is now in Working Draft but it is similar to HTML's id
attribute (xml:id ID  #IMPLIED).

Comment 1

[Karl Benson]

Attachment: Addressing embedded CSS in xml-stylesheet PI

Comment 2

[Karl Benson]

Attachment: Same ID used multiple times

An ID used multiple times causes an xml:id error [1], as described in the spec.
This nonfatal error must be reported to the application [2].

[1] http://www.w3.org/TR/xml-id/#terminology
[2] http://www.w3.org/TR/xml-id/#errors

Comment 3

[Boris Zbarsky [:bzbarsky]]

See comments in bug 258238.

Comment 4

[Anne (:annevk)]

It reached CR: <http://www.w3.org/TR/2005/CR-xml-id-20050208/>

Comment 5

[Anne (:annevk)]

Attachment: test case for xml:id and CSS

We do not support xml-stylesheet with a same-document reference. Here is a test
case that does not rely on that.

Comment 6

[Robin Berjon]

(In reply to comment #3)
> See comments in bug 258238.

As others explained there, there is a limitation on DTDs that they cannot specify two elements of type 
ID on a single element (XML Schema has a similar limitation though it's likely to go in a future version, 
Relax NG does not). This limitation is entirely inherited from SGML and has no use whatsoever. 
Furthermore, it does not indicate that an XML document containing two IDs on an element is invalid, 
simply that it cannot be validated with a DTD. In fact, xml:id specifically states that it does not impose 
that constraint (end of section 4).

The addition of xml:id makes the lives of people that are dealing with multiple vocabularies not 
implemented by the UA a lot simpler, which in turn releases some pressure on UAs to implement more 
and more (this is the sort of facility that allows people to design vocabularies that can be entirely 
implemented in XBL for instance, while still providing the sort of facilities expected by end users, such 
as getElementById).

Is there any chance of seeing this support in soon? Given that the CR period is likely to end soon since 
there are interoperable implementations, and given how useful it can turn out to be, I think it would be 
a really good thing to have it in.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is there any chance of seeing this support in soon? 

Smaug's been thinking about it.  The problem is that there is some very
performance-sensitive code involved (CSS selector matching), and any changes to
support this must be made in a way that does not regress performance on existing
(HTML, and especially "DHTML") content.

Comment 8

[Dominik]

Now xml:id is in CR. I think it's time to implement. Even W3C implement xml:id
as predefined attributes (like xml:lang) in XML Schema
http://www.w3.org/2001/xml.xsd

Comment 9

[Dave Hodder]

xml:id is now a W3C Proposed Recommendation, and is also becoming a requirement
for other specifications.  (For example, it appears in the recent SVG Tiny 1.2
LC Working Draft (13 April 2005).)

Comment 10

[Dave Hodder]

Now a Recommendation.

Comment 11

[Doug Schepers]

Please see non-bug https://bugzilla.mozilla.org/show_bug.cgi?id=309182 to show
why we need this now.

Comment 12

[Aurélien Maille]

Opera now support xml:id (Opera 9.0)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

*** Bug 348683 has been marked as a duplicate of this bug. ***

Comment 14

[mozilla]

I have some workaround code on http://wiki.svg.org/Starter ,
look for "getElm("

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

dtomaszuk, any real reason why this should block 1.9 ?
I agree it would be great, but blocking...

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: some tests for xml:id and setIdAttr*

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

xml:id (no error handling) and setIdAttr* support.
Trying to keep the normal id attribute handling as fast as possible,
xml:id and user defined IDs are slower.
Works with CSS style selectors.
Needs still some jproffing.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

The patch seems to work, shouldn't regress performance and so on, 
but the bigger question is that whether we want to support 
xml:id/setIdAttr*. This does after all add some bloat.
xml:id is imo good thing, needed especially for DIY XML languages, 
and it is quite easy to implement xml:id and setIdAttr* in the same 
time. And Opera does support xml:id.

Comment 19

[Brendan Eich [:brendan]]

This is adding a non-trivial amount of code for a case that does not matter on the web (we're not going to support SVG 1.2).  It's bloat.  We should be removing this kind of code, not adding more.

/be

Comment 20

[Robin Berjon]

This is useful on the Web, if browsers supported it I know I would use it instead of having to translate from xml:id to id+DTD just for them.

It's OT but I fail to see how not supporting SVG 1.2 can be considered an idea even remotely good. 90% of the differences between 1.1 and 1.2 are fixes to things that were insufficiently defined or did not fully cater to interoperability in 1.1. Not supporting it is making a deliberate decision to suck, and sounds very much like an ideologue's decision and surprisingly little like a technically informed one.

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #20)
> This is useful on the Web, if browsers supported it I know I would use it
> instead of having to translate from xml:id to id+DTD just for them.

"Useful on the Web" and "useful to you" are not the same things.  "If browsers supported it" is not the same as "if Firefox supports it".  We will be removing code as well as adding code, and code implementing features that other browsers than Opera do not and will not support is ripe for removal.

> It's OT but I fail to see how not supporting SVG 1.2 can be considered an idea
> even remotely good. 90% of the differences between 1.1 and 1.2 are fixes to
> things that were insufficiently defined or did not fully cater to
> interoperability in 1.1. Not supporting it is making a deliberate decision to
> suck, and sounds very much like an ideologue's decision and surprisingly little
> like a technically informed one.

Please leave your ideology out of it.  SVG 1.2 is not for the web.  There is no technical reason to impose it on browsers.  It was neither designed to interop with web content formats, nor is it in demand among web content authors.  Also, the way CR last call was handled makes me want to avoid ever doing business with the parts of the w3c involved in it, but that's icing on the cake.

See numerous posts (I know you've read them) on the w3c list, or just (re-)read http://weblogs.mozillazine.org/roc/archives/2006/08/whats_wrong_with_the_svg_worki.html

BTW, we still don't have nearly all of 1.1 implemented, so talking about "90%" does not help, even if SVG 1.2 were relevant to the web.

/be

Comment 22

[Brendan Eich [:brendan]]

Just in case it's not clear (I've written about this often), adding code for a feature not in other browsers has to be a calculated move, based on a credible strategy for improving the web.  It can't be just because there's a w3c spec.

(Yes, this means MathML is grandfathered.  If we could load it on demand safely across versions, and not bind it into the core of Gecko, I would.)

There is no strategy behind SVG 1.2 that makes sense for the web.  From bug 258238 I do not see any good strategy favoring xml:id.  As Jonas wrote there, it is a thin-ice hazard.

/be

Comment 23

[Bob Clary [:bc] (inactive)]

*** Bug 362484 has been marked as a duplicate of this bug. ***

Comment 24

[Jonathan Watt [:jwatt]]

Let's leave SVG 1.2 discussion for another bug.

I agree that we should reduce feature bloat and code complexity where ever we have no good use cases. However, I do think we have an important use case for xml:id relating to a standard we want to promote.

One of the problems encountered when using XBL is that getElementById can't be used to find widgets or whatever else you might be implementing using XBL. The _bound_ elements are typically in a custom namespace in a multi-namespace compound document unsuitable for a DTD. I had a chat with Jonas and I think we agree that the only realistic option is to work through the issues with xml:id. I'll let him speak for himself on that point though. Olli has agreed to simplify his patch as much as possible by e.g. removing the DOM 3 setIdAttribute stuff. Perhaps we can revisit this bug once we see where that gets us?

When I say this is a problem I'm not speaking in the hypothetical. This is an issue we at Joost are finding problematic in our use of XBL.

Comment 25

[Hixie (not reading bugmail)]

You shouldn't be sending private markup vocabularies over the wire anyway.

Comment 26

[Jonathan Watt [:jwatt]]

Why? One of Mozilla's primary aims is to promote innovation on the Web. To my mind standardization and promotion of XBL would be a huge step towards this end primarily because it would allow Web content authors to create their own markup dialects. This provides them with a tool to help build what they need (as opposed to many W3C specs). I'm hoping to see "markup libraries" along the lines of JavaScript libraries such as MochiKit at some point.

Comment 27

[Hixie (not reading bugmail)]

Because if you send custom (i.e. proprietary) vocabs over the net to users who aren't expecting it (i.e. in any environment that isn't closed), then they can't use it. It's the same reason you shouldn't send data using Word, or Flash, or any other proprietary format.

If adding xml:id would encourage authors to create proprietary formats and send them on the Web, then that's the biggest argument to avoid adding it.

Comment 28

[Jonathan Watt [:jwatt]]

I don't think it's right to compare a custom format implemented using XBL to proprietary formats such as those used by Word. XBL is open, and if the user agent knows how to handle XBL then it knows how to handle the custom format.

Comment 29

[Hixie (not reading bugmail)]

> [...] If the user agent knows how to handle XBL then it knows how to handle the
> custom format.
 
Exactly. User agents shouldn't have to know how to handle XBL. It's like CSS. It's a layer above, for making fancy widgets and stuff out of the boring lower level semantics. It's not about skipping the semantic layer altogether.

Comment 30

[alexander :surkov (:asurkov)]

Web is not unique thing where XBL can be applied. I guess the main area for XBL is creating vocabularies for mozilla based applications like toolkit and here xmlid would be helpful.

Comment 31

[Hixie (not reading bugmail)]

Gecko is primarily aimed at the Web platform, though, and we shouldn't expose the Web platform to xml:id just because non-Web uses might make sense.

Comment 32

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

I'm actually thinking we should do something even more evil here. I wonder if we should simply say that all attributes named 'id' should be treated as ID attributes. In theory we can claim that we are just extending getElementById and CSS Selector matching.

Yes, it's a bit evil, but I think it would make the world a better place.

Comment 33

[Anne (:annevk)]

I think that makes sense. It also keeps everything simple (only one ID per element, etc.).

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

It makes sense, maybe, but essentially it is "non-standard" way to do 
the same thing as xml:id.
I hope w3c would have reserved "id" for IDs from the beginning...

But if we make the change, would other browser vendors do the same.
Opera supports xml:id already, no idea about webkit.
(Actually some version of khtml used to recognize "id" always as ID)

Comment 35

[Henri Sivonen (:hsivonen)]

I agree with Jonas. I think that conceptually, this should be defined analogously to xml:id. That is, there's an "id processor" between the XML processor and the app and the id processor assigns the type ID to any attribute whose local name is id and that is not in a namespace. It wouldn't have to be implemented like this as long as the observable results are indistinguishable (i.e. DOM methods report ids as IDs, XPath id() works, CSS #id works, getElementById() works, etc.).

Comment 36

[Elliotte Rusty Harold]

Hixie, the whole point of stylesheets in the XML world is that you can send custom document formats over the Web to users who aren't expecting them, and still have them display reasonably.

The claim that we must agree on all semantics in advance (or ever) is false. Custom document formats based on a shared text syntax are just fine. xml:id is, like stylesheets, one part of enabling this to work properly. 

Comment 37

[Elliotte Rusty Harold]

The Web platform includes raw XML which can be and is usefully published on the Web today. It mostly just works. ID support is one annoying wart that can and should be shaved off. 

Comment 38

[Elliotte Rusty Harold]

Declaring all attributes named id to be of type ID would be contrary to various specifications. The job here is to implement the spec, not to redefine it. Taking this nonconformant path would simply introduce interoperability problems into the Web that do not currently exist. 

I think Mozilla has full standards conformance as a goal. 

While it might have made sense to reserve the attribute name "id" in the early days of XML 1.0, that ship has long since sailed. At this point it is far more important to conform to the relevant specifications, even where they;re a little ugly, than it is to try to create an ideal language that is not shared with other tools. 

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: few tests without get/setIdAttr

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP2, only xml:id, no setIdAttr*

Jonas, what do you think about this.
I took the similar approach as Opera; element has only 
one ID attribute, but for example if 'id' is removed and 
there is already 'xml:id', 'xml:id' becomes the ID.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP2 + a missing file

Comment 42

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

So there are some inconsistencies in the latest patch, mostly related to the fact that IsIDAttributeName always returns true for xml:id even if id is set. For example I suspect nsXMLEventsManager::AttributeChanged will behave wrong if id is set and then xml:id is changed because of this.

I actually think that it might be simpler and from a user perspective simpler to let elements possibly have two ids. As sad as that makes me.

Or do you think it's possible to fix the above problem without doing that?

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #42)
> So there are some inconsistencies in the latest patch, mostly related to the
> fact that IsIDAttributeName always returns true for xml:id even if id is set.
> For example I suspect nsXMLEventsManager::AttributeChanged will behave wrong if
> id is set and then xml:id is changed because of this.
Ah, that is true. Will fix that. Sort of leftover from previous patch.
Have to remove modtype checks and just 
mListeners.Enumerate(EnumAndSetIncomplete, aContent);
AddListeners(aDocument);
I think other callers of IsIDAttributeName should do the right thing.

The comment about IsIDAttributeName tries to explain that it may 
return true whenever attribute is possibly an ID attribute.
Maybe the method name should be IsPossibl(e|y)IDAttribute

> Or do you think it's possible to fix the above problem without doing that?

See above. I'd still like to make us work the same way as Opera.
That keeps things simpler to implement and IMO it is up to content
author to not to misuse xml:id.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Why is the change to nsFrameManager::HasAttributeDependentStyle needed?  I would think it should not be necessary...

Comment 45

[Boris Zbarsky [:bzbarsky]]

In particular, the nsCSSRuleProcessor should probably check aAttribute against IsIDAttributeName, not GetIDAttributeName.

Comment 46

[Hixie (not reading bugmail)]

Isn't this somewhat overengineered? Note that it has been suggested that the Attr.IsId attribute, and the Element.setIdAttribute(), Element.setIdAttributeNS(), and Element.setIdAttributeNode() methods, should be dropped altogether. I wouldn't expect the other browsers to implement this. Are we sure we want to add this? (Shouldn't we be working towards making Gecko slimmer, removing minor features like this rather than adding them?)

Comment 47

[Alex Vincent [:WeirdAl]]

I'd object to losing Attr.isId, because my xpathgen component requires it.

Comment 48

[Hixie (not reading bugmail)]

That's a good example of why we should avoid adding features without carefully thinking of whether we want them. People start depending on them! :-)

Comment 49

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

Hixie: This patch does not affect any of those DOM properties. IsId is already implemented and the patch does not implement any of the other methods you mention.

Comment 50

[Hixie (not reading bugmail)]

Ah! Ok, fair enough then. My bad!

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #44)
> Why is the change to nsFrameManager::HasAttributeDependentStyle needed?  I
> would think it should not be necessary...
> 

If 'id' is removed, 'xml:id' becomes the ID so restyling is needed...

> In particular, the nsCSSRuleProcessor should probably check aAttribute against
> IsIDAttributeName, not GetIDAttributeName.
> 

yes, this might do the same thing

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Fixed nsXMLEventsManager.cpp, IsIDAttributeName used now in 
nsCSSRuleProcessor::HasAttributeDependentStyle.

Comment 53

[Anne (:annevk)]

I'm not volunteering (yet), but it would be nice to have a detailed document somewhere on how browsers are to support xml:id so that new browsers can simply support it without reverse engineering other browsers. (Like you guys had to do...)

Comment 54

[Boris Zbarsky [:bzbarsky]]

> If 'id' is removed, 'xml:id' becomes the ID so restyling is needed...

Yes, but this is covered by the normal AttributeChanged notification for "id" in the null namespace, no?

Basically, we want to restyle only if there are id rules that might match the node, not whenever someone changes an id.

You'll probably want to have dbaron check over that part of the patch.

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

That nsFrameManager::HasAttributeDependentStyle was really needed, but
now because IsIDAttributeName is used in 
nsCSSRuleProcessor::HasAttributeDependentStyle, that covers also the
removing of an ID attribute (and when another attribute becomes ID).

Comment 56

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

I'm not a fan of the name IsIDAttributeName since it returns true even if the attribute is in fact not currently the id-attribute. Maybe IsPotentialIDAttributeName?

This does in fact seem to cause a bug in nsHTMLDocument::AttributeWillChange/AttributeChanged since you add and remove elements from the id-map based on the xml:id attribute even if that's not the id.

Comment 57

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

Same thing in nsXULDocument::AttributeChanged. I wonder if it'd be better to change the behavior of the function rather than its name?

Comment 58

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

Comment on attachment 267552 [details] [diff] [review]
v3

Hmm.. actually, the nsHTMLDocument and nsXULDocument changes look right. So just change the function name.

nsXMLEventsManager::AttributeChanged. Looks like a change in behavior here? Or is it just a de-optimization?

In nsXULDocument::AddElementToMap, why not call GetID()->ToString() instead of GetIDAttributeName + GetAttr? Same in RemoveElementFromMap.

Looks good otherwise. So please do the naming change, and it'd be great to get dbaron, bz or roc to sr this since it contains a good amount of style changes.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3.1

Changes to nsXMLEventsManager are needed because the ID attribute of the
element may have changed.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Hoping either bz or dbaron has time to look at the patch...

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273817 [details] [diff] [review]
v3.1

>Index: content/base/public/nsIContent.h

>+   * Note this may return true with many attributes, but only one
>+   * is used as an ID at the time.

"at a time", not "at the time"


Do we really want nsGenericElement::GetIDAttributeName to return xml:id
if neither attribute is set and the nodeinfo does have an ID attribute?
What if GetIDAttributeName is used to determine what attribute to set?
Should we really choose to set xml:id over the language's id?


In nsXMLEventsElement::GetIDAttributeName,
nsGenericHTMLElement::GetIDAttributeName,
nsSVGElement::GetIDAttributeName,
nsXTFElementWrapper::GetIDAttributeName, and
nsXULElement::GetIDAttributeName, it seems more logical to use
kNameSpaceID_None as the parameter to HasAttr, and then set aNameSpaceID
only inside the if, if we're going to return early.

In nsXULDocument, you should remove kIdentityAttrs since you're making
it unused.

In nsCSSRuleProcessor.cpp, the comment:
+                              PRInt32 aNameSpaceID, // the namespace of the attribute
should say "namespace of attribute NOT to test".  And you should also find a nice way to wrap that at less than 80 characters :-)

Still in SelectorMatches:
+        if (attr->mAttr == aAttribute && attr->mNameSpace == aNameSpaceID) {
You need the latter condition to be ||ed with
  attr->mNameSpace == kNameSpaceID_Unknown
since there are attribute selectors of the form [*|attr="value"].

You should add a test for this to the mochitests in layout/style/test/.

+    PRInt32 namespaceID;
+    if (aAttribute &&
+        aAttribute == data.mContent->GetIDAttributeName(namespaceID) &&
+        aNameSpaceID == namespaceID) {

This needs to use IsPotentialIDAttributeName, because if you remove an
id attribute on an HTML element, GetIDAttributeName will change to
returning xml:id, and won't match.

You should add a test for this to the mochitests in layout/style/test/.

+  if (aData->mNameSpaceID == kNameSpaceID_None &&
+      aData->mAttribute == nsGkAtoms::href &&

Could you switch the order of these tests so the one most likely to fail
comes first?

nsIStyleRuleProcessor.h:
+  PRInt32  mNameSpaceID;
+  nsIAtom* mAttribute;   // |HasAttributeDependentStyle| for which attribute?
Could you put the comment above both lines, and a blank line below them?


You should add at least some basic tests for the selector matching of
xml:id as an ID selector in layout/style/test/.

sr=dbaron with those changes.

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #61)
> 
> Do we really want nsGenericElement::GetIDAttributeName to return xml:id
> if neither attribute is set and the nodeinfo does have an ID attribute?
Does that really matter?

> What if GetIDAttributeName is used to determine what attribute to set?
That is not what GetIDAttributeName is meant for, at least not after
this patch. I'll add a comment that the method returns the name of the current 
ID attribute.

> Should we really choose to set xml:id over the language's id?
In Opera language IDs have higher priority.

Comment 63

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

I'm sort of with david, though I don't feel that strongly about it.

What is the disadvantage of returning the 'native' id by default?

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #63)
> What is the disadvantage of returning the 'native' id by default?
> 
Would have to check whether there is xml:id attribute or the dtd ID.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3.2

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why do the mochitests have pass conditions in both the case where an exception is thrown and where it isn't?  Shouldn't one of an exception being thrown or not being thrown be wrong?

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Oh, yes, there is still one that kind of catch() {} left.
Will fix that.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3.2.1

Comment 69

[Olli Pettay [:smaug][[e-mail]]]

I checked in the patch, but it may have caused small tp/tp2 regression 
on bl-bldlnx03. Waiting still for few cycles, then backing out if the regression
isn't just noise.

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3.3

Changes are in content/. Adding IsPotentialIDAttributeNameInternal and
GetIDAttributeNameInternal so that non-virtual calls can be used in places
which are called often (for example nsGenericElement::ParseAttribute).
Adding GetPrimaryIDAttributeName() so that implementations of IsPotentialIDAttributeName and GetIDAttributeName and GetID can be kept in
one place in nsGenericElement.
The patch might help a bit with the perf, though can't verify with this 
machine. (Perf numbers vary too much).

Comment 71

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

+nsIAtom *
+nsGenericElement::GetIDAttributeNameInternal(PRInt32& aNameSpaceID) const
+{
+  aNameSpaceID = kNameSpaceID_None;
+  nsIAtom* idAtom = GetPrimaryIDAttributeName();
+  if (idAtom && HasAttr(aNameSpaceID, idAtom)) {
+    return idAtom;
+  }
+
+  idAtom = mNodeInfo->GetIDAttributeAtom();
+  if (idAtom && HasAttr(aNameSpaceID, idAtom)) {
+    return idAtom;
+  }
+
+  aNameSpaceID = kNameSpaceID_XML;
+  return nsGkAtoms::id;
+}

This unfortunately doesn't look any faster since there's two HasAttr calls which are both virtual and slow :(

Comment 72

[Olli Pettay [:smaug][[e-mail]]]

Those were there in the first patch anyway.
IsPotentialIDAttributeName is called more often than GetIDAttributeName.
It may (may not) be a bit faster in the new patch.
It also depends on how compilers manage to optimize the code.
By keeping more code in one place, optimizing is perhaps easier.
yeah yeah, guessing... but anyway, I prefer this new patch.

Comment 73

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Add NODE_MAY_HAVE_ID_ATTRIBUTE flag

I accept the new member to prototype element, because I've reduced the size 
of the xul element during 1.9 :)

Comment 74

[Boris Zbarsky [:bzbarsky]]

If you do want to do that, can't you put the script type id and the one bit into 32 bits?

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Attachment: smaller members in protoelement

Or the scripttypeIdDcould be smaller. GenericElement can't anyway handle too large
scripttypeIDs.

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 274183 [details] [diff] [review]
smaller members in protoelement

Ugh, no, I get some assertions with this.

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 274174 [details] [diff] [review]
Add NODE_MAY_HAVE_ID_ATTRIBUTE flag

Don't have time today for anything better than this.

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 274183 [details] [diff] [review]
smaller members in protoelement

Oops, this works just fine.
I forgot to compile also xblcontentsink :(

I also verified that using the flag lots of method calls are saved.

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

FYI, running mochitest with the patch, there are ~2900000 calls to 
GetIdAttributeName(Internal). ~2300000 cases the flag is not set.

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

I've run tp2 locally, and couldn't see any pref regression.

Comment 81

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

Comment on attachment 274183 [details] [diff] [review]
smaller members in protoelement

>+nsIAtom *
>+nsGenericElement::GetIDAttributeNameInternal(PRInt32& aNameSpaceID) const
>+{
>+  if (HasFlag(NODE_MAY_HAVE_ID_ATTRIBUTE)) {
>+    aNameSpaceID = kNameSpaceID_None;
>+    nsIAtom* idAtom = GetPrimaryIDAttributeName();
>+    if (idAtom && HasAttr(aNameSpaceID, idAtom)) {
>+      return idAtom;
>+    }
>+
>+    idAtom = mNodeInfo->GetIDAttributeAtom();
>+    if (idAtom && HasAttr(aNameSpaceID, idAtom)) {
>+      return idAtom;
>+    }
>+
>+    aNameSpaceID = kNameSpaceID_XML;
>+    if (HasAttr(aNameSpaceID, nsGkAtoms::id)) {
>+      return nsGkAtoms::id;
>+    }
>+  }
>+
>+  aNameSpaceID = kNameSpaceID_Unknown;
>+  return nsnull;
>+}

What i don't like about this is that xhtml element could potentially start looking at other attributes than 'id' if they end up with a nodeinfo with a id-name set. This can happen in documents that inconsistently uses prefixes and has a DTD. IMHO elements for which we know the id should never pay attention to the nodeinfo.

Now that you've moved the HasFlag(NODE_MAY_HAVE_ID_ATTRIBUTE) check into the GetID function, it seems like it would be better to make GetID call GetIDAttributeName and have the default implementation of that check the nodeinfo, but let the various subclasses override to just look directly for 'id'.

This should even be slightly faster.

Comment 82

[Olli Pettay [:smaug][[e-mail]]]

Attachment: if host language specifies ID for the element, don't ever use DTD.

This also optimizes GetID even more. GetIDAttributeInternal returns AttrInfo,
so no need to re-iterate attributes to find ID attribute.

Comment 84

[Anne (:annevk)]

This is not going to happen.