Closed Bug 799905 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(1 file)

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.
Flags: in-testsuite+
Summary: .URL should be defined on Document, not HTMLDocument → .URL and .compatMode should be defined on Document, not HTMLDocument
Attached patch PatchSplinter Review
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
Attachment #669934 - Flags: review?(bzbarsky)
(Also already tested by dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html -- there are two expected passes to fix before pushing.)
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.
Attachment #669934 - Flags: review?(bzbarsky) → review+
(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.
> 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).
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?
Empty string would probably be better, for now...

Longer-term it might be better to ensure we never have a null mDocumentURI.
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
https://hg.mozilla.org/mozilla-central/rev/eaa7aff9f43c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.