323793 - Expose .ping attribute for <a> and <area> elements

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Darin Fisher]

Expose .ping attribute for <a> and <area> elements if support for sending pings is enabled in the user agent.  This allows websites to detect the existance of the <a ping> feature.  Without this (or some alternative), there is no reliable way for a website to make use of <a ping>.

Comment 1

[Darin Fisher]

According to Ian, the ping DOM property should be undefined when support for <a ping> is disabled in the browser.  What is the best way to expose such a property via our DOM implementation?  Simply having nsHTMLAnchorElement QI to some new interface defining the ping attribute would seem to be insufficient.  It seems like I need to affect the classinfo for the anchor element at runtime (or at least at browser startup, but preferrable in response to pref changes).  Thoughts?

Comment 2

[Benjamin Smedberg]

> According to Ian, the ping DOM property should be undefined when support for <a
> ping> is disabled in the browser.  What is the best way to expose such a

This sounds like a bad specification that should be rethought. Can't we just have the .ping attribute exist but return null?

Comment 3

[Hixie (not reading bugmail)]

Um. I didn't say it should be undefined when disabled. I said it should be undefined when the property is not supported. There should not be a way to tell when the user has disabled support for this; if there was, it would negate most of the benefit of the privacy improvements of the property.

Comment 4

[Darin Fisher]

OK, I misunderstood Ian's comments here:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2005-October/004898.html

Given this new information, the QI issue is moot.  We will always expose and implement ".ping" properly regardless of how the user's preferences have been set.  In that way, enabling/disabling <a ping> works more like referrers.  Websites have no way of knowing whether or not a referrer will be sent unless they go check server logs on the linked-to site ;-)

This seems like a decent solution from a user privacy point-of-view.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Sounds like a plan to me.  So just add this to nsIDOMNSHTMLAnchorElement (and same for nsIDOMNSHTMLAreaElement) and impl the prop in those files?

Comment 6

[Darin Fisher]

Yup, will do.  Thanks for confirming the approach!

Comment 7

[Darin Fisher]

Boris: The getter for ".ping" needs to return a list of absolute URIs according to the spec, and that code is probably best shared between nsHTMLAnchorElement and nsHTMLAreaElement.  Should I just define a method on nsGenericHTMLElement that both may call?  Is that the best way to share method implementations amongst different HTML elements?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

doesn't AnchorElement and AreaElement use NS_IMPL_URI_ATTR, whereas this can't be used here as it's a list of URIs?

Comment 9

[Darin Fisher]

Ah, perhaps it makes sense to provide a GetURIListAttr method that is similar, or to add a second parameter to GetURIAttr that controls how it treats whitespace.

Comment 10

[Boris Zbarsky [:bzbarsky]]

A new method on nsGenericHTMLElement makes the most sense, probably.

Comment 11

[Darin Fisher]

Attachment: v1 patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209172 [details] [diff] [review]
v1 patch

>Index: dom/public/idl/html/nsIDOMNSHTMLAnchorElement.idl
>-[scriptable, uuid(a6cf911c-15b3-11d2-932e-00805f8add32)]
>+[scriptable, uuid(a756a043-6222-4d00-a015-b5c167e219c7)]

It just occurred to me.... We're considering putting this on the 1.8 branch for Firefox 2.0, right?  In that case we should consider nsIDOMNSHTMLAnchorElement2 instead.

In fact, I think we should consider it in general -- nsIDOMNSHTMLAnchorElement has been stable for a while, and I bet people are using it as "just another DOM API"...  That is, I think we should consider freezing it (with documentation), and putting new things on nsIDOMNSHTMLAnchorElement2.

Similar for nsIDOMNSHTMLAreaElement.

All of this subject to what jst says, of course.

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLElement::GetURIListAttr(nsIAtom* aAttr, nsAString& aResult)

Hmm...  I guess we're seriously relying on the fact that our iterators are actually PRUnichar* and not string iterators, eh?  Took me a bit to notice that.  ;)

Could you perhaps comment that?  Most times with string code advancing an actual string iterator past the end of the string (as we do here with our PRUnichar*s) is bad...

>Index: content/html/content/src/nsHTMLAtomList.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/Attic/nsHTMLAtomList.h,v

That file doesn't exist anymore (note the Attic part).  You want to patch content/base/src/nsGkAtomList.h

sr=bzbarsky with those nits picked.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 209172 [details] [diff] [review]
v1 patch

r=jst

Comment 14

[Darin Fisher]

> >+nsGenericHTMLElement::GetURIListAttr(nsIAtom* aAttr, nsAString& aResult)
> 
> Hmm...  I guess we're seriously relying on the fact that our iterators are
> actually PRUnichar* and not string iterators, eh?  Took me a bit to notice
> that.  ;)
> 
> Could you perhaps comment that?  Most times with string code advancing an
> actual string iterator past the end of the string (as we do here with our
> PRUnichar*s) is bad...

String iterators are a thing of the past.  The string API already requires that strings are contiguous runs of PRUnichar (or char) elements.  The only variable is whether they are null terminated or not.  We still have nsAString:: const_iterator and such for backwards compat, but using those doesn't really add anything.  I think we should remove them in favor of raw character pointers at some point ;-)

I made the nsIDOMNS{Anchor|Area}Element2 change as you suggested.

Comment 15

[Darin Fisher]

Attachment: v1.1 patch

revised patch for check-in.

Comment 16

[Darin Fisher]

fixed-on-trunk

Comment 17

[Boris Zbarsky [:bzbarsky]]

Actually, the new interfaces also need to be added to the right places in nsDOMClassInfo.cpp, no?

Comment 18

[Darin Fisher]

> Actually, the new interfaces also need to be added to the right places in
> nsDOMClassInfo.cpp, no?

Whoops.  Thanks.

Comment 19

[Darin Fisher]

Attachment: fixup DOMClassInfo for new interfaces [fixed-on-trunk]

Like this...

Comment 20

[Darin Fisher]

Comment on attachment 209426 [details] [diff] [review]
fixup DOMClassInfo for new interfaces [fixed-on-trunk]

This patch is on the trunk now.