window.Document !== window.XMLDocument

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Adam Barth]

window.Document !== window.XMLDocument

[Adam Barth]

Created attachment 51368 [details]
Patch

[Darin Adler]

Comment on attachment 51368 [details]
Patch

> -    static PassRefPtr<Document> createDocument(Frame*);
> +    static PassRefPtr<XMLDocument> createDocument(Frame*);

I think we should get rid of DOMImplementation::createDocument and DOMImplementation::createHTMLDocument. I can't find any callers of either function.

> +    static PassRefPtr<Document> createXHTML(Frame* frame)
> +    {
> +        return adoptRef(new XMLDocument(frame, true));
> +    }

Return type here should be PassRefPtr<XMLDocument>.

> +    XMLDocument(Frame* frame, bool isXHTML);

This should be private; it is only called from the static member functions above.

> +    virtual ~XMLDocument();

This isn't needed at all. We should leave it out.

> --- a/WebCore/xml/XMLHttpRequest.cpp
> +++ b/WebCore/xml/XMLHttpRequest.cpp
> @@ -41,6 +41,7 @@
>  #include "Settings.h"
>  #include "TextResourceDecoder.h"
>  #include "ThreadableLoader.h"
> +#include "XMLDocument.h"

We should also remove the include of "Document.h".

[Adam Barth]

Thanks for the quick review.

> I think we should get rid of DOMImplementation::createDocument and
> DOMImplementation::createHTMLDocument. I can't find any callers of either
> function.

DOMImplementation::createDocument has at least one client: XMLHttpRequest.  I've adopted the rest of your comments (including removing createHTMLDocument).  I'll land this sometime at night because I'm sure I screwed up editing at least one of the build systems.

[Adam Barth]

Created attachment 51371 [details]
Patch updated with Darin's feedback (will land later)

[Adam Barth]

Created attachment 51372 [details]
Patch updated with Darin's feedback (will land later)

[Adam Barth]

Comment on attachment 51371 [details]
Patch updated with Darin's feedback (will land later)

Typo in changelog (spotted by evmar)

[Adam Barth]

Created attachment 51387 [details]
For EWS bots (no actual review needed)

[WebKit Review Bot]

Attachment 51387 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1080005

[Adam Barth]

Created attachment 51388 [details]
For EWS bots (no actual review needed)

[WebKit Review Bot]

Attachment 51388 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1051119

[Alexey Proskuryakov]

I'm completely confused. Why is this a good change? There is no such thing as XMLDocument in HTML5, it's just a legacy alias for Document that we support.

[Adam Barth]

> I'm completely confused. Why is this a good change? There is no such thing as
> XMLDocument in HTML5

Then that's a bug in HTML5.

> It's just a legacy alias for Document that we support.

The problem is that we need to hang some APIs off XMLDocument that don't show up in HTMLDocument.  We can't put them on Document because HTMLDocument inherits from Document.

We used to implement XMLDocument as an alias for Document, but that's an error, at least according to Firefox, which has a separate concept of an XMLDocument.

[Sam Weinig]

I also don't understand the merit of this change? Is there some real world bug we are trying to fix? The XMLDocument constructor has been a hack that has worked well as far as I can tell.

[Adam Barth]

The real-world bug we're trying to fix is Bug 9063.  The load API is supposed to be present on XMLDocument but not on HTMLDocument.  Currently, that's not possible because XMLDocument is aliased to base class of HTMLDocument.  After this patch, it is possible.

[Sam Weinig]

(In reply to comment #14)
> The real-world bug we're trying to fix is Bug 9063.  The load API is supposed
> to be present on XMLDocument but not on HTMLDocument.  Currently, that's not
> possible because XMLDocument is aliased to base class of HTMLDocument.  After
> this patch, it is possible.

Ah, I stick by 2006 Sam in thinking we probably shouldn't implement document.load() and instead evangelize to sites that use it.

[Adam Barth]

> Ah, I stick by 2006 Sam in thinking we probably shouldn't implement
> document.load() and instead evangelize to sites that use it.

I got annoyed by the lack of document.load because I couldn't report bugs in IE9 to Microsoft because their site relies upon document.  (The site worked fine in Firefox.)

[Ian 'Hixie' Hickson]

(In reply to comment #12)
> The problem is that we need to hang some APIs off XMLDocument that don't show
> up in HTMLDocument.  We can't put them on Document because HTMLDocument
> inherits from Document.

In HTML5 HTMLDocument does not inherit from Document. See:

   http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#documents-in-the-dom

[Adam Barth]

Hum...  That would require some major reworking of the implementation, but it could be done.  There's still the problem of SVGDocument inheriting from Document:

http://www.w3.org/TR/SVGTiny12/svgudomidl.html

The load API isn't supposed to appear on SVGDocument.

[Adam Barth]

See also Bug 36480 for more tests related to document.load.

[Adam Barth]

See also this related Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=100795

[Alexey Proskuryakov]

(In reply to comment #18)
> Hum...  That would require some major reworking of the implementation, but it
> could be done.

Yes, I think that we should kill our C++ document hierarchy, and match HTML5. Having separate classes for different document kinds also causes issues like bug 25397.

> The load API isn't supposed to appear on SVGDocument.

What is the problem with it being present on all documents? By the way, we should consider making document.load hidden, like document.all (but that's not a discussion for this bug).

[Adam Barth]

> Yes, I think that we should kill our C++ document hierarchy, and match HTML5.
> Having separate classes for different document kinds also causes issues like
> bug 25397.

That could be valuable, but is moderately independent of how the web platform API looks to web content.

> > The load API isn't supposed to appear on SVGDocument.
> 
> What is the problem with it being present on all documents?

Please see the Mozilla bug cited in Comment 20:
https://bugzilla.mozilla.org/show_bug.cgi?id=100795

Basically, if an inline event handler tries to call a function in the global scoped named "load" the call will fail because document is in the scope chain for inline event handlers.

> By the way, we
> should consider making document.load hidden, like document.all (but that's not
> a discussion for this bug).

That doesn't seem optimal because (1) it doesn't address the compatibility issue caused by putting load on every document and (2) it's a novel magical element of the web platform that isn't present in other browsers.  Adopting the Firefox solution has the virtues of (A) matching Firefox and thereby reduce the entropy in the web platform and (B) proven compatibility with web content for the better part of a decade.

[Alexey Proskuryakov]

The Firefox document hierarchy looks and feels obsolete since HTML elements became the same as XHTML ones in DOM. I don't think XMLDocument a reasonable thing to expose now.

> (B) proven compatibility with web content for the better part of a decade.

This is true to some extent, but matching other browsers has been also known to introduce unexpected compatibility problems due to enabling different code paths. E.g., document.all had an even better historical compatibility story, but there was no way we could enable it without making it undetectable.

[Adam Barth]

Do you have an alternative proposal for resolving Bug 9063?

> The Firefox document hierarchy looks and feels obsolete since HTML elements
> became the same as XHTML ones in DOM. I don't think XMLDocument a reasonable
> thing to expose now.

This sounds more like an aesthetic argument.  I agree that this solution is rather unaesthetic, but I don't know of another workable solution.

> > (B) proven compatibility with web content for the better part of a decade.
> 
> This is true to some extent, but matching other browsers has been also known to
> introduce unexpected compatibility problems due to enabling different code
> paths. E.g., document.all had an even better historical compatibility story,
> but there was no way we could enable it without making it undetectable.

I would be surprised if that were the case here.  These perils exist more often when matching IE.  In this case, I'm suggesting we match the behavior that Firefox has had since 2001.

In particular, Bug 9063 is the most common WebKit compatibility issue I run into personally.  I've run across it several times in the past few months and have had to use Firefox to access some web sites.  I don't really understand the resistance to fixing this bug.

[Alexey Proskuryakov]

I don't think we'll want to introduce XMLDocument interface unless it's defined in HTML5. We made much more drastic departures from current Firefox behavior to match the agreed-upon behavior in HTML5 (e.g. the aforementioned change to give HTML elements XHMTL namespace), and Firefox is changing in the same direction.

This is not really an aesthetic argument. The differences are very much detectable from JS code. For example, suppose that one calls document.open() on an SVG document. Per HTML5, this changes it into an HTML one - but it's still the same object, and thus implements the same interfaces.

In any case, this is a discussion for WHATWG mailing list, if you want to bring it up there.

> Do you have an alternative proposal for resolving Bug 9063?

One way to implement document.load without regressing behavior on sites that call load() from attribute event listeners is to only let it be found in specific cases. E.g., only via dot notation, and only on the object's prototype itself. This could be done at property lookup time, or even in the JS parser. I'm sure someone familiar with JSC would have more and better ideas.

[Adam Barth]

> > Do you have an alternative proposal for resolving Bug 9063?
> 
> One way to implement document.load without regressing behavior on sites that
> call load() from attribute event listeners is to only let it be found in
> specific cases. E.g., only via dot notation, and only on the object's prototype
> itself. This could be done at property lookup time, or even in the JS parser.
> I'm sure someone familiar with JSC would have more and better ideas.

That's even more magical than document.all.  :(

[Adam Barth]

> In any case, this is a discussion for WHATWG mailing list, if you want to bring
> it up there.

Posted: http://lists.w3.org/Archives/Public/public-html/2010Mar/0706.html

[Alexey Proskuryakov]

Current HTML5's solution is for XMLDocumentLoader (containing "load" and "async") to only be present on document object if it has its XML Document bit set. So, I'm marking this WONTFIX.

> suppose that one calls document.open() on an SVG document. 
> Per HTML5, this changes it into an HTML one

Not true (anymore?) - open() raises an exception if it's called on a document that isn't already an HTML one. So, it seems that for any given document object, "load" will be present or not, there won't be any dynamic changes of its presence.

[Alexey Proskuryakov]

HTML5 has specified XMLDocument now (in a way that's very different from Firefox). Document.load now lives in this XMLDocument.

See <http://www.w3.org/Bugs/Public/show_bug.cgi?id=14037>.

[Alexey Proskuryakov]

*** Bug 25664 has been marked as a duplicate of this bug. ***