799905 - .URL and .compatMode should be defined on Document, not HTMLDocument

Status: RESOLVED FIXED

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

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

DOM says .URL and .compatMode should both exist on all Document objects, not just HTMLDocument:

http://dom.spec.whatwg.org/#interface-document

This matches IE/Chrome.  Opera and Gecko both make them defined only on HTMLDocument.  The spec/IE/Chrome behavior is better because it makes things more consistent between HTML and XML.

I'm writing tests for this right now as part of the DOM test suite, which should be imported the next time we sync, since it will be in DOMCore/tests/approved/.  So I'll set in-testsuite+ right now, and no separate test should be needed.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

No separate test, because as noted, I'm writing one for the W3C's DOM test repo and it will get imported later.  Try, Linux64 debug only: https://tbpl.mozilla.org/?tree=Try&rev=4849d8b4a9a0

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(Also already tested by dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html -- there are two expected passes to fix before pushing.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 669934 [details] [diff] [review]
Patch

You need to update the IIDs involved here.

This changes the behavior of GetURL() when mDocumentURI is null.  Is that on purpose?  Is that situation even web-visible?

r=me with those addressed.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> You need to update the IIDs involved here.

Bah, I always forget that.

> This changes the behavior of GetURL() when mDocumentURI is null.  Is that on
> purpose?  Is that situation even web-visible?

Old behavior of nsHTMLDocument::GetURL if mDocumentURI is null: create nsAutoCString, copy it to out param.

New behavior: SetDOMStringToNull on out param.

How is that different?  nsAutoCString initializes itself to empty, right?

> r=me with those addressed.

Will wait for feedback on the latter point before pushing.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> How is that different?

The former will look like "" in JS, while the second will look like null in JS.  So it matters for code that does things like doc.URL.indexOf("foo") because it will throw with the new code.

But again, only if we can expose to web content a document with a null mDocumentURI.  Which I'm not quite sure we can, but it's worth checking.

But also, if the spec does not say this is a nullable string, then we should just nix the SetDOMStringToNull bit and return an empty string, since the other would not work in a WebIDL world anyway (in WebIDL we make assumptions about return values based on the IDL, with some fatal asserts to back them up).

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Oh!  I thought "Null" there just meant "empty string".  Per spec, these are not nullable, and documents' default URI is "about:blank".  So if we want to default to anything, it should probably be "about:blank".  I wasn't able to tell from code inspection whether we ever expose null mDocumentURI to web content.

Does about:blank sound good to you, or should I just make it an empty string?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Empty string would probably be better, for now...

Longer-term it might be better to ensure we never have a null mDocumentURI.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Okay, I'll change SetDOMStringToNull(aDocumentURI); to aDocumentURI.Truncate(); in GetDocumentURI() in the patch.  I did a new try run in case anything depended on GetDocumentURI() returning null instead of empty string:

https://tbpl.mozilla.org/?tree=Try&rev=5e79aaa8fa42

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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