572688 - Add nsIDOMNSDocument::mozSetImageElement

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Attachment: v1

This is for background-image: -moz-element(), bug 506826.

Spec:
You have a <div style="background-image: -moz-element(#elem)"></div>.
Normally, this will reference an element with the ID "elem". However, when you call document.mozSetImageElement("elem", someOtherElement), someOtherElement will be referenced instead. Calling document.mozSetImageElement("elem", null) will stop the override.
This should only happen for -moz-element references; things like <rect fill="url(#elem)"/> shouldn't notice the override.

Implementation:
The entries of the identifier map in nsDocument get a new field mImageElement which is set by mozSetImageElement. The identifier map knows when to consider the image override because AddIDTargetObserver has a new parameter aForImage which is remembered in the ChangeCallback struct. The original source of this flag is in nsReferencedElement::Reset, from which it is passed everywhere.

At the moment the flag is always false. I'll put a patch that calls nsReferencedElement::Reset with aReferenceImage == PR_TRUE in a different bug.

The one thing I wasn't sure about at all is the change to the ChangeCallbackEntry's HashKey method. I'm adding a boolean that needs to be considered, so I'm just XORing it on a bit that's pretty far to the left. Does that make sense?

Comment 1

[Markus Stange [:mstange]]

Attachment: v2

And this one actually compiles.

Comment 2

[Markus Stange [:mstange]]

(In reply to comment #0)
> At the moment the flag is always false. I'll put a patch that calls
> nsReferencedElement::Reset with aReferenceImage == PR_TRUE in a different bug.

That patch is in bug 572689, part 3.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

What problems does this solve that aren't solved by someOtherElement.id = "elem", and why does this need to be exposed to content?

Comment 4

[Markus Stange [:mstange]]

For example, you can do this:

var img = new Image();
img.src = "someimage.png";
document.mozSetImageElement("elem", img);

and you don't have to add that image to the DOM. But I agree that doing

img.id = "elem"
img.style.display = "none"
document.body.appendChild(img)

is not much more work.

In
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/d58cbafa83471800/b4b5ab1849ffe3af
roc says:

> It's [...] for working with elements in specific
> subdocuments, working with elements that aren't in a document, and
> writing script library APIs that work with a passed-in element reference
> where you may not want to change the ID of that element.

Comment 5

[Markus Stange [:mstange]]

I guess I need to update this on top of bug 572614.

Comment 6

[Markus Stange [:mstange]]

Attachment: v3

Updated after the landing of bug 572614.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> img.id = "elem"
> img.style.display = "none"
> document.body.appendChild(img)

That's not too bad, but it does have slightly higher performance overhead. More importantly, it doesn't handle the case where instead of creating a new anonymous element, you want to refer to an existing element in the document which doesn't already have an ID (and you don't want to modify the document to give it one), especially if you want to refer to an existing element in *another* document, in which case trying to refer to it by ID won't work at all.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  virtual mozilla::dom::Element* LookupImageElement(const nsAString& aElementId) = 0;

Can we typedef mozilla::dom::Element Element; in the class so we don't have to repeat the fully qualified name?

Can we use a flags parameter instead of a boolean parameter, so it's more extensible and it's more obvious in callers what the parameter is doing?

Comment 9

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

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

>+ if ((args->mImageOnly && !aEntry->mKey.mForImage) ||
>+     (args->mHaveImageOverride && !args->mImageOnly && aEntry->mKey.mForImage))

would be simpler/faster written as

if (aEntry->mKey.mForImage ? (args->mHaveImageOverride && !args->mImageOnly) :
                             args->mImageOnly)

(or even faster but much less clear

if ((aEntry->mKey.mForImage != args->mImageOnly) &&
    (args->mImageOnly || args->mHaveImageOverride))

)

 Element*
-nsIdentifierMapEntry::GetIdElement()
-{
+nsIdentifierMapEntry::GetIdElement(PRBool aAllowImageOverride)
+{

Mind reverting this change and instead adding a separate nsIdentifierMapEntry::GetImageIdElement or some such? That will avoid using default arguments which are always an annoyance, and also saves some branches in pretty heavily used code paths.

>+NS_IMETHODIMP
>+nsDocument::MozSetImageElement(const nsAString& aImageElementId,
>+                               nsIDOMElement* aImageElement)
>+{
>+  if (!CheckMozSetImageElementArg(aImageElementId))
>+    return NS_OK; // XXXmstange is NS_OK correct?

I would say it is yes. Keeps things in sync with getElementById which also doesn't throw for invalid ids.


>+  static inline PRBool CheckMozSetImageElementArg(const nsAString& aId)
>+  {
>+    if (aId.IsEmpty()) {
>+      ReportEmptyMozSetImageElementArg();
>+      return PR_FALSE;
>+    }
>+    return PR_TRUE;
>+  }
>+
>+  static void ReportEmptyMozSetImageElementArg();

I really don't think we need to report someone calling mozSetImageElement with an empty ID to the error console. I doubt that that will be a common enough mistake. So I say get rid of these functions and just inline the empty-string check.

>+    // XXXbz we should really have a sane GetElementById on nsIDocument.
>+    nsCOMPtr<nsIDOMElement> element;
>+    domDoc->GetElementById(aRef, getter_AddRefs(element));
>+    if (element) {
>+      content = do_QueryInterface(element);
>+    }

Nowadays we do have a sane one! I know you just moved this code, but it'd be great of you changed this to use nsIDocument::GetElementById and remove the QI above this code. Alternatively move the QI to inside the |if| along with the other code that you're moving.

r=me with those fixed

However I would like to have roc sign off on that this is how we want to do this type of thing.

Comment 10

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

Though really, we could rename mozSetImageElement to mozOverrideId and make it override *all* places where IDs are used. I.e. SVG and getElementById would all be affected by the new API.

It does seem somewhat more sensible than the possibility of -moz-element and getElementById returning different things.

I don't feel very strongly either way.

Comment 11

[Markus Stange [:mstange]]

(In reply to comment #8)
> +  virtual mozilla::dom::Element* LookupImageElement(const nsAString&
> aElementId) = 0;
> 
> Can we typedef mozilla::dom::Element Element; in the class so we don't have to
> repeat the fully qualified name?

Sure.

> Can we use a flags parameter instead of a boolean parameter, so it's more
> extensible and it's more obvious in callers what the parameter is doing?

Which functions should I convert to using this flag? All the way from nsReferencedElement::Reset through nsIDocument::AddIDTargetObserver through nsIdentifierMapEntry::FireChangeCallbacks?
And what should I call the flag? NS_ALLOW_IMAGE_OVERRIDE?

(In reply to comment #9)
> (From update of attachment 453074 [details] [diff] [review])
> >+ if ((args->mImageOnly && !aEntry->mKey.mForImage) ||
> >+     (args->mHaveImageOverride && !args->mImageOnly && aEntry->mKey.mForImage))
> 
> would be simpler/faster written as
> 
> if (aEntry->mKey.mForImage ? (args->mHaveImageOverride && !args->mImageOnly) :
>                              args->mImageOnly)

This is good!

>  Element*
> -nsIdentifierMapEntry::GetIdElement()
> -{
> +nsIdentifierMapEntry::GetIdElement(PRBool aAllowImageOverride)
> +{
> 
> Mind reverting this change and instead adding a separate
> nsIdentifierMapEntry::GetImageIdElement or some such? That will avoid using
> default arguments which are always an annoyance, and also saves some branches
> in pretty heavily used code paths.

Good idea, I'll do that.

> >+NS_IMETHODIMP
> >+nsDocument::MozSetImageElement(const nsAString& aImageElementId,
> >+                               nsIDOMElement* aImageElement)
> >+{
> >+  if (!CheckMozSetImageElementArg(aImageElementId))
> >+    return NS_OK; // XXXmstange is NS_OK correct?
> 
> I would say it is yes. Keeps things in sync with getElementById which also
> doesn't throw for invalid ids.

OK.

> >+  static inline PRBool CheckMozSetImageElementArg(const nsAString& aId)
> >+  {
> >+    if (aId.IsEmpty()) {
> >+      ReportEmptyMozSetImageElementArg();
> >+      return PR_FALSE;
> >+    }
> >+    return PR_TRUE;
> >+  }
> >+
> >+  static void ReportEmptyMozSetImageElementArg();
> 
> I really don't think we need to report someone calling mozSetImageElement with
> an empty ID to the error console.

Thanks :)

> Nowadays we do have a sane one! I know you just moved this code

In fact this code has already been corrected in bug 572609, I just haven't rebased the patch yet.

Could you also answer my "XXXmstange does this make sense?" question?

(In reply to comment #10)
> Though really, we could rename mozSetImageElement to mozOverrideId and make it
> override *all* places where IDs are used. I.e. SVG and getElementById would all
> be affected by the new API.

Offhand I can't think of any strong arguments against that API. Though having document.getElementById() return an element that's from another document might be a little strange...

Roc, your call.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

I originally designed this the way it is, so enthusiastic r=me!

I would prefer not to mess with getElementById. There would be performance impact.

Just one thing, please please please can we have "typedef mozilla::dom::Element Element;" in nsIDocument and wherever else we use Element?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, one reason not to mess with getElementById is that then trusted scripts couldn't trust getElementById. That would be very bad.

Comment 14

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

(In reply to comment #12)
> (From update of attachment 453074 [details] [diff] [review])
> I originally designed this the way it is, so enthusiastic r=me!
> 
> I would prefer not to mess with getElementById. There would be performance
> impact.

Really? As far as I can see it would only be one extra, well predicted, branch for each getElementById, so hardly noticable.

> Just one thing, please please please can we have "typedef mozilla::dom::Element
> Element;" in nsIDocument and wherever else we use Element?

Generally mozilla::dom::Element should be used in /content, where we should be doing "using mozilla::dom" anyway, so I'm not convinced that is needed?

(In reply to comment #13)
> Actually, one reason not to mess with getElementById is that then trusted
> scripts couldn't trust getElementById. That would be very bad.

Since pages can simply call setAttribute("id",...) on any element, I don't think we generally can "trust" that getElementById returns what we expect anyway.

Comment 15

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

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

>     static PLDHashNumber HashKey(KeyTypePointer aKey)
>     {
>       return (NS_PTR_TO_INT32(aKey->mCallback) >> 2) ^
>-             (NS_PTR_TO_INT32(aKey->mData));
>+             (NS_PTR_TO_INT32(aKey->mData)) ^
>+             (aKey->mForImage << 30); // XXXmstange does this make sense?
>     }

Given that it's unlikely to use the same callback function for both image and non-image consumers, I'm not even sure that we need to add mForImage to the hash calculation.

But if you really do want to have it, I suspect the above implementation will generally lose it. A better strategy is something like:

      return (NS_PTR_TO_INT32(aKey->mCallback) >> 1) ^
             (NS_PTR_TO_INT32(aKey->mData) << 1) ^
             (aKey->mForImage); // XXXmstange does this make sense?

I don't care strongly either way.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #14)
> (In reply to comment #12)
> > Just one thing, please please please can we have "typedef mozilla::dom::Element
> > Element;" in nsIDocument and wherever else we use Element?
> 
> Generally mozilla::dom::Element should be used in /content, where we should be
> doing "using mozilla::dom" anyway, so I'm not convinced that is needed?

We're repeating mozilla::dom::Element in nsIDocument more than once, so it's needed.

> (In reply to comment #13)
> > Actually, one reason not to mess with getElementById is that then trusted
> > scripts couldn't trust getElementById. That would be very bad.
> 
> Since pages can simply call setAttribute("id",...) on any element, I don't
> think we generally can "trust" that getElementById returns what we expect
> anyway.

That's true, but I still think allowing getElementToId to behave strangely this way is a bad idea, especially since we don't have a compelling use-case.

Comment 17

[Markus Stange [:mstange]]

Attachment: v4

Updated to trunk, with typedef, Jonas' comments addressed, still using mozSetImageElement, but still using a boolean instead of a flag.

(In reply to comment #15)
> Given that it's unlikely to use the same callback function for both image and
> non-image consumers, I'm not even sure that we need to add mForImage to the
> hash calculation.

Oh, I hadn't really thought about that. You're right.

Comment 18

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

I actually prefer a boolean to a flag. If we end up needing to pass more information in the future, it's easy enough to change things then.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

AddIDTargetObserver(id, observer, target, PR_TRUE) just isn't as useful to read as AddIDTargetObserver(id, observer, target, ADD_FOR_IMAGE) (or something like that).

Comment 20

[Markus Stange [:mstange]]

(In reply to comment #19)
> AddIDTargetObserver(id, observer, target, PR_TRUE) just isn't as useful to read

I agree, but the only consumer currently looks like this:

mElement = mWatchDocument->AddIDTargetObserver(mWatchID, Observe, this,
                                               mReferencingImage);

so it's pretty useful already.
Converting to a flag would make it look like this:

PRUint32 flags = mReferencingImage ? nsIDocument::OBSERVE_IMAGE : 0;
mElement = mWatchDocument->AddIDTargetObserver(mWatchID, Observe, this,
                                               flags);

I'm not sure that's much clearer.

Comment 21

[Markus Stange [:mstange]]

Attachment: convert boolean to flag

Here's a patch that does the conversion.

roc, jonas: please decide if you want it :)

Comment 22

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

I'm still a fan of the bool option, but I don't feel strongly either way.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Neither do I. Go ahead with the boolean if that's what you want.

Comment 24

[Markus Stange [:mstange]]

I kept the boolean.
http://hg.mozilla.org/mozilla-central/rev/6876b5b6d83c

Comment 25

[Eric Shepherd [:sheppy]]

Documented here:

https://developer.mozilla.org/en/DOM/document.mozSetImageElement

And linked from the DOM.document reference page and the Firefox 4 for developers pages.