Closed Bug 604592 Opened 9 years ago Closed 9 years ago

Make Node.prefix readonly

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [wdc])

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
Web DOM Core makes it so. This patch still needs tests.
Flags: in-testsuite?
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.)
content/base/test/test_bug564863.xhtml would need to be fixed.
Blocks: 393222
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #483415 - Attachment is obsolete: true
Attachment #498520 - Flags: review?(Olli.Pettay)
(Post-2.0, of course.)
Whiteboard: [wdc]
I'm still not sure whether I want this change in DOM (Web) Core.
Sure, Anne is the person to convince.
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 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?
(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.
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.
Littlemutt says IE9 does support setting prefix (<http://software.hixie.ch/utilities/js/live-dom-viewer/saved/891>).
I still think we should try this and see if it breaks any high-profile sites. My guess is that it won't.
I agree. Do we want it for Fx5?
Yes. The longer we wait the bigger risk some site comes to depend on current behavior.
OK. In that case I'll need a r+.
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.
Attached patch Patch v2Splinter Review
Attachment #523671 - Flags: review?(Olli.Pettay)
Attachment #498520 - Attachment is obsolete: true
Attachment #498520 - Flags: review?(Olli.Pettay)
Comment on attachment 523671 [details] [diff] [review]
Patch v2

r=me after updating those IIDs
Attachment #523671 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/4d2da0c23ae2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Flags: in-testsuite? → in-testsuite+
Keywords: dev-doc-needed
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.
You need to log in before you can comment on or make changes to this bug.