Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes do not need a name slot for web compat #110

Closed
foolip opened this issue Nov 19, 2015 · 8 comments
Closed

Attributes do not need a name slot for web compat #110

foolip opened this issue Nov 19, 2015 · 8 comments

Comments

@foolip
Copy link
Member

foolip commented Nov 19, 2015

https://dom.spec.whatwg.org/#concept-attribute-name

This is an issue I noticed while investigating https://code.google.com/p/chromium/issues/detail?id=502301, trying to replace all case-insensitive attribute lookups in Blink with lowercasing of the input.

Blink doesn't store a separate attribute name, but instead just constructs a string from the prefix (if any) and localName where needed.

The get an attribute by name steps use the name concept in a way that does not match Blink, which can be described as:

  1. Return the first attribute with no prefix and whose localName is a case-sensitive match for name, if any.
  2. Return the first attribute where one of the following is true, if any:
    1. The attribute has no prefix, is on an HTML element in an HTML document, and its localName is a case-insensitive match for name.
    2. The attribute has a prefix and its prefix+":"+localName is a maybe-case-insenstive match for name

Where maybe-case-insensitive is case-insensitive for attributes on HTML elements in HTML documents and otherwise case-sensitive.

Rewriting this to match the new model it would be:

  1. If the attribute is on an HTML element in an HTML document, lowercase name
  2. Return the first attribute with no prefix and whose localName is a case-sensitive match for name, if any.
  3. Return the first attribute with a prefix where its prefix+":"+localName is case-senstive match for name, if any.

I'm not sure if spec'ing it like this would make sense.

@bzbarsky, how does this work in Gecko, do you actually store the name separately?

@ArkadiuszMichalski
Copy link
Contributor

Will be one common convention comparing to Element, where we don't have slot for qualified name, so for example Element.tagName performs "similar operations". Other specifications don't use Attr's name from DOM as references?

@bzbarsky
Copy link

@foolip So first of all, it's worth separating what's stored and what's compared. That is, it's quite possible to implement the spec algorithm, and efficiently in the common case, even if you just store prefix and localname separately and don't store what the spec calls "name".

Now in terms of what Gecko actually implements, each attribute has an AttrName which is either a string or a struct. The string is used for the case when the attribute has no prefix and is in the null namespace. The struct is used for the other cases (i.e. when there is a prefix and/or a namespace). The struct stores the prefix, localName, namespace, and a "qualified name" which is basically used to optimize access to things that need a qualified name. The structs are shared across things with the same (prefix, localName, namespace) triple, so in practice the space used by the "qualified name" is negligible, especially given how rare attributes that need the struct are to start with.

The "get an attribute by name" steps in Gecko ASCII-lowercase the name, then return the first attribute for which either the AttrName is a string matching the given one or the AttrName is a struct for which the qualified name matches the given string.

Note that the latter compare could be done even if you don't store the full "name" string, by e.g. checking that the given string starts with the prefix, ends with the localName, has only one more char, and that char is ':'. This can be done without allocations and string copies, if that's what's worrying you.

But yes, conceptually I don't think attributes need a "name" slot, since it can be recreated from the prefix and localName. I can't speak to how convenient or not it would be to specify things that way.

I do think specifying the two-pass thing you propose is not a good idea, especially since there is no particular implementation need for it afaict.

@foolip
Copy link
Member Author

foolip commented Nov 23, 2015

Thanks @bzbarsky, that is very helpful!

My concern was that the spec says "Unless explicitly given when an attribute is created, its name is set to its local name" and that perhaps there might be some edge case in HTML or XML parsing where the Gecko's "qualified name" is explicitly given, and wouldn't be the same as the computed value in Blink.

If that is not the case, then it would be nice to clarify the spec so that it's clear that the name slot doesn't need to be stored.

I agree that there's no need to spec a two-pass lookup, as steps 2 and 3 in the "new model" above are mutually exclusive.

Thoughts, @annevk?

In any event, this issue won't need to block https://code.google.com/p/chromium/issues/detail?id=502301, I'll just assume that a computed prefix+":"+localName string is and will continue to be equivalent to what the spec says. (Possible to optimize, of course.)

@annevk
Copy link
Member

annevk commented Nov 23, 2015

Well, what the specification says currently is more like name = prefix === null ? localName : prefix + ":" + localName or some such. I'm happy to drop this slot though. I might keep the concept, but turn it into a getter-like algorithm.

@foolip
Copy link
Member Author

foolip commented Nov 23, 2015

Yes, that is exactly how QualifiedName::toString() is implemented in Blink, and having that as an algorithm instead of a slot might be nice.

It looks like the concept is only used in attribute lookup, so another model could be to try eliminating it entirely, instead having qualified name variables in the few places where it's needed. That's entirely editorial, though.

@annevk annevk closed this as completed in aaa22ab Nov 23, 2015
@foolip
Copy link
Member Author

foolip commented Nov 23, 2015

Thanks! I commented on a few mistakes in aaa22ab

@annevk
Copy link
Member

annevk commented Nov 23, 2015

Thank you, fixed in 144f011.

@foolip
Copy link
Member Author

foolip commented Nov 23, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants