879319 - implement past names map in HTMLFormElement

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

http://www.whatwg.org/specs/web-apps/current-work/#dom-form-nameditem

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

The patch seems bigger just because I moved AddElementToTableInternal().

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758040 [details] [diff] [review]
patch

1) The past names map is a simple mapping of name to element, not of name to "element or list of elements".  So I'm not sure it's worth trying to reuse AddElementToTableInternal for it; the addition to the past names map should be as simple as a single Put() call.

2) Please document that we're using RemoveElementFromTableInternal because we do not want to do the remove if aElement does not match the entry in the past names map.  And maybe change RemoveElementFromTableInternal to not QI before doing the == compare for the single element case, since that should not be needed and for the past names map is pure unnecessary overhead?

3) Things that come from mImageNameLookupTable should also end up in the past names map according to the spec, right?

4) Instead of a boolean argument, we should use an enum to make it clearer what's going on.

r-, but this is looking pretty good!

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758445 [details] [diff] [review]
patch

I think I'd prefer s/ElementDeleted/ElementRemoved/

>+  // If the element is fully deleted, we have to remove it from the

"If the element is being removed from the form, we have to ..."?

>+nsHTMLFormElement::AddToPastNamesMap(const nsAString& aName,
>+  nsCOMPtr<nsIContent> node = do_QueryInterface(aChild);

This makes me sad, but I guess it's the best we can do with the APIs we have here so far.  Maybe a followup to clean this up?

>+    mPastNameLookupTable.Get(aName, getter_AddRefs(supports));

No need for that.  Just:

  if (node) {
    mPastNameLookupTable.Put(aName, aChild);
  }

since that's not going to be much more expensive than the lookup if we're already in the hashtable and will be strictly less work if we're not.

> +++ b/content/html/content/src/nsHTMLFormElement.h

>+   *        is removed because it's deleted, it will be removed from the past

Again, "removed from the form" instead of "deleted"?

>+// A map from an ID or NAME attribute to the past Form Control elements, this
>+// hash holds strong references either to the named Form control elements.

How about:

  // A map from names to elements that were gotten by those names from this
  // form in that past.  See "past names map" in the HTML5 specification.

>+++ b/content/html/content/test/test_bug879319.html

>+input0.setAttribute("form", "");
>+ok(!("foo0" in form.elements), "foo0 is not in form.elements");
>+is(form.foo0, input0, "Form.foo0 is still here");

Oh, this is _very_ interesting.  I think the spec is wrong here and changing @form like this so that the object is no longer associated with this form should remove it from the past names map...  Please file a spec bug?

>+input0.parentNode.removeChild(input0);
>+ok(!("foo0" in form.elements), "foo0 is not in form.elements");
>+is(form.foo0, input0, "Form.foo0 is still here");

Because the spec definitely says that at this point form.foo0 should stop being input0.  But there's no way to make that work, since it no longer has a reference to the form!

>+is(form.foo0, input1, "Form.foo0 is not input1");

How about "form.foo0 should be input1"?

>+is(form.foo1, input1, "Form.foo1 is not input1");

Likewise.

There should be a test that removing input1 from the DOM makes form.foo1 not point to it anymore.  Might have to be a todo for now until we webidlize the form.

Please add a test like that for removing img0 from the DOM as well... and I suspect that this patch would fail that test: we need to remove the image from the past names map when it stops being associated with the form, right?

r=me with those fixed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758445 [details] [diff] [review]
patch

Actually, I just realized the removal parts of this patch are just wrong.  You need to remove all the entries for the element, not just the ones for its current name...

Please add a test for it, though it's possible it fails in current Gecko because we just cache all this state directly on the JSObject right now.

In any case, the removal story here needs to happen eventually, unless the spec gets changed.

That said, I'm not sure the removal bits of the spec make sense.  It's worth testing how other UAs, and especially Trident and WebKit, handle this past names stuff, both in the "removed from document" case and the "@form got changed" case....

Comment 6

[Andrea Marchesini [:baku]]

I fixed the removal part but, if the inputs are cached on the JSObject, what's the purpose of the past names table?
 
> Please add a test for it, though it's possible it fails in current Gecko
> because we just cache all this state directly on the JSObject right now.

I'm testing the other UAs.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I fixed the removal part but, if the inputs are cached on the JSObject, what's the
> purpose of the past names table?

When you switch to WebIDL bindings, the caching on the JSObject will go away; at that point the past names table is what's supposed to provide behavior that looks like that caching.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 760892 [details] [diff] [review]
patch

I'm worried about that hashtable enumeration: it makes teardown of a form O(N^2) in the number of form elements that were accessed via form.foo.

Maybe a followup to look into having a faster mapping from an element to its past names?

>+++ b/content/html/content/src/nsHTMLFormElement.h

>+   *        from the past names map too, otherwise it will be keep into it.

  "otherwise it will stay in the past names map"

perhaps?

>+++ b/content/html/content/test/test_bug879319.html
>+is(form.foo0, input0, "Form.foo0 is still here");

This should be a todo_isnot with a "Form.foo0 should not still be here" message, because that's the sane behavior and the one you will get when switching to WebIDL here.  At last after the setAttribute("form", "") call.

Also, you should test here that form.tmp0 and form.tmp1 went away as well.  Again, with a todo_isnot or todo_is with "undefined" as one of the args, until we switch to WebIDL here.

>+input0.parentNode.removeChild(input0);

This test is not doing anything useful, since at this point the input is not form-associated anymore.  Please do this on a different node that's still associated with the form.

>+is(form.foo0, input1, "Form.foo0 is not input1");
>+is(form.foo1, input1, "Form.foo1 is not input1");

How about "should be input1" instead of "is not input1"?

Still need to test that removing the <img> from the DOM makes form.bar0 become undefined.

r=me with the above addressed.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 11

[Andrea Marchesini [:baku]]

Attachment: interdiff

Here the interdiff.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 761943 [details] [diff] [review]
interdiff

>+ok(input2, "input0 exists");
>+is(form.foo2, input2, "Form.foo0 should exist");
>+ok(!("foo2" in form.elements), "foo0 is not in form.elements");

s/0/2/ in all of those?

>+is(form.foo2, input2, "Form.foo0 is still here");

  todo_is(form.foo2, undefined, "Form.foo2 should not longer be there");

r=me with those fixed

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

That still needs the messages fixed in:

>+ok(input2, "input0 exists");
>+is(form.foo2, input2, "Form.foo0 should exist");

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 16

[Andrea Marchesini [:baku]]

Attachment: interdiff

Debugging HTMLFormElement I found that I forgot this step: any image element has to be removed from the past names table when unbind from the tree.
This is just the interdiff.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch

the full patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 763148 [details] [diff] [review]
interdiff

r=me

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch

rebased

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in as https://hg.mozilla.org/integration/mozilla-inbound/rev/25126626dd43 and then backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/26696aad9b5b because I had to back out the bug this depends on.

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d02f89580ad9

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d02f89580ad9