604592 - Make Node.prefix readonly

Status: RESOLVED FIXED

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

Description

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

Attachment: Patch v1

Web DOM Core makes it so. This patch still needs tests.

Comment 1

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

This is not a very important case, but I wonder why Web DOM Core makes this
change.
For some serialization cases allowing changing the prefix might even
be useful.

(And note, Web DOM Core isn't stable at all, and certainly not properly reviewed.)

Comment 2

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

content/base/test/test_bug564863.xhtml would need to be fixed.

Comment 3

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

Attachment: Patch v1.1

Comment 4

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

(Post-2.0, of course.)

Comment 5

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

I'm still not sure whether I want this change in DOM (Web) Core.

Comment 6

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

Sure, Anne is the person to convince.

Comment 7

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

For what it's worth, I think this is a good change. It removes a fair amount of complexity and as far as I can think of there aren't any strong use cases.

If you do think this feature should be kept, do speak up on the webapps list and include use cases or other reasons (web compat?) why you think so.

Comment 8

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

Comment on attachment 498520 [details] [diff] [review]
Patch v1.1

The only use case I could come up for setting .prefix is serialization.
But if one really needs to change prefix, one could either just write
own serializer or clone manually elements to another document and when
creating those elements, use different prefix.
So I guess this change is ok.

Why are you just disabling tests in Makefile.in, but removing the files.
Do either one: disable, or remove files and references from Makefile.in

Do IE9, webkit or Opera implement setting prefix?

Comment 9

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

(In reply to comment #8)
> Comment on attachment 498520 [details] [diff] [review]
> Patch v1.1
> 
> The only use case I could come up for setting .prefix is serialization.
> But if one really needs to change prefix, one could either just write
> own serializer or clone manually elements to another document and when
> creating those elements, use different prefix.
> So I guess this change is ok.
> 
> Why are you just disabling tests in Makefile.in, but removing the files.
> Do either one: disable, or remove files and references from Makefile.in

Can't remember. I'll remove the references.

> Do IE9, webkit or Opera implement setting prefix?

Chrome does, Opera doesn't seem to. I don't have IE9 here.

Comment 10

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

I don't particularly care if other browsers support it. If we can get away with not supporting it (which I hope we can), it's a good simplification of our codebase.

Serialization is an interesting use case. Though only in the rare case that you're serializing a document which uses non-html-non-svg-non-mathml-non-nullnamespace'd nodes. And you want to use a different prefix than the node had when the node was originally created.

I think this is rare enough that it's not worth the complexity. Of course, if we have data indicating otherwise I'm all ears. Such data might arise once we release with this turned off.

Comment 11

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

Littlemutt says IE9 does support setting prefix (<http://software.hixie.ch/utilities/js/live-dom-viewer/saved/891>).

Comment 12

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

I still think we should try this and see if it breaks any high-profile sites. My guess is that it won't.

Comment 13

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

I agree. Do we want it for Fx5?

Comment 14

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

Yes. The longer we wait the bigger risk some site comes to depend on current behavior.

Comment 15

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

OK. In that case I'll need a r+.

Comment 16

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

Comment on attachment 498520 [details] [diff] [review]
Patch v1.1

>diff --git a/content/base/test/test_bug564863.xhtml b/content/base/test/test_bug564863.xhtml
Do we really want to remove the tests in this file?
Would it be possible to just change them somehow.


>   // Introduced in DOM Level 2:
>-           attribute DOMString        prefix;
>-                                         // raises(DOMException) on setting
>+  readonly attribute DOMString        prefix;
Update the comment. We're not compatible with
DOM Level 2 anymore, but DOM Web Core (or whatever the new spec is called :) )




> 
>   // Introduced in DOM Level 2:
>   readonly attribute DOMString        localName;
>   // Introduced in DOM Level 2:
>   boolean            hasAttributes();
> };
>diff --git a/dom/tests/mochitest/dom-level2-core/Makefile.in b/dom/tests/mochitest/dom-level2-core/Makefile.in
>--- a/dom/tests/mochitest/dom-level2-core/Makefile.in
>+++ b/dom/tests/mochitest/dom-level2-core/Makefile.in
>@@ -287,40 +287,24 @@ _TEST_FILES_E	= \
> 		test_nodeissupported02.html \
> 		test_nodeissupported03.html \
> 		test_nodeissupported04.html \
> 		test_nodeissupported05.html \
> 		$(NULL)
> 
> _TEST_FILES_F	= \
> 		test_nodenormalize01.html \
>-		test_nodesetprefix01.html \
>-		test_nodesetprefix02.html \
>-		test_nodesetprefix03.html \
>-		test_nodesetprefix04.html \
>-		test_nodesetprefix05.html \
>-		test_nodesetprefix06.html \
>-		test_nodesetprefix07.html \
>-		test_nodesetprefix08.html \
>-		test_nodesetprefix09.html \
> 		test_normalize01.html \
> 		test_ownerDocument01.html \
> 		test_ownerElement01.html \
> 		test_ownerElement02.html \
> 		test_prefix01.html \
> 		test_prefix02.html \
> 		test_prefix03.html \
> 		test_prefix04.html \
>-		test_prefix05.html \
>-		test_prefix06.html \
>-		test_prefix07.html \
>-		test_prefix08.html \
>-		test_prefix09.html \
>-		test_prefix10.html \
>-		test_prefix11.html \
> 		test_publicId01.html \
> 		test_removeAttributeNS01.html \
> 		test_removeAttributeNS02.html \
> 		test_removeNamedItemNS01.html \
> 		test_removeNamedItemNS02.html \
> 		test_removeNamedItemNS03.html \
> 		test_setAttributeNS01.html \
> 		test_setAttributeNS02.html \
>@@ -339,16 +323,36 @@ _TEST_FILES_F	= \
> 		test_setNamedItemNS01.html \
> 		test_setNamedItemNS02.html \
> 		test_setNamedItemNS03.html \
> 		test_setNamedItemNS04.html \
> 		test_setNamedItemNS05.html \
> 		test_systemId01.html \
> 		$(NULL)
> 
>+# See bug 604592
>+_TEST_FILES_DISABLED = \
>+		test_nodesetprefix01.html \
>+		test_nodesetprefix02.html \
>+		test_nodesetprefix03.html \
>+		test_nodesetprefix04.html \
>+		test_nodesetprefix05.html \
>+		test_nodesetprefix06.html \
>+		test_nodesetprefix07.html \
>+		test_nodesetprefix08.html \
>+		test_nodesetprefix09.html \
>+		test_prefix05.html \
>+		test_prefix06.html \
>+		test_prefix07.html \
>+		test_prefix08.html \
>+		test_prefix09.html \
>+		test_prefix10.html \
>+		test_prefix11.html \
>+		$(NULL)
>+

I don't understand this.
You move testfiles in Makefile.in to 
_TEST_FILES_DISABLED, yet you're actually removing those files.
Is there any reason to have _TEST_FILES_DISABLED?

We should have tests which actually check that .prefix is readonly.

Comment 17

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

Attachment: Patch v2

Comment 18

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

I need to update a lot uuids (http://people.mozilla.org/~sfink/uploads/update-uuids)

Comment 19

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

Comment on attachment 523671 [details] [diff] [review]
Patch v2

r=me after updating those IIDs

Comment 20

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

http://hg.mozilla.org/mozilla-central/rev/4d2da0c23ae2

Comment 21

[Eric Shepherd [:sheppy]]

Tricksy hobbitses, slipping in a Firefox 5 doc bug on launch day eve.

Documentation updated:

https://developer.mozilla.org/En/DOM/Node.prefix

And mentioned on Firefox 5 for developers.