Bug 36465 - window.Document !== window.XMLDocument
Summary: window.Document !== window.XMLDocument
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 9063
  Show dependency treegraph
 
Reported: 2010-03-22 16:21 PDT by Adam Barth
Modified: 2011-10-20 20:41 PDT (History)
8 users (show)

See Also:


Attachments
Patch (231.07 KB, patch)
2010-03-22 16:27 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch updated with Darin's feedback (will land later) (231.39 KB, patch)
2010-03-22 16:59 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch updated with Darin's feedback (will land later) (231.39 KB, patch)
2010-03-22 17:02 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
For EWS bots (no actual review needed) (231.38 KB, patch)
2010-03-22 19:17 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
For EWS bots (no actual review needed) (231.55 KB, patch)
2010-03-22 19:26 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-03-22 16:21:02 PDT
window.Document !== window.XMLDocument
Comment 1 Adam Barth 2010-03-22 16:27:46 PDT
Created attachment 51368 [details]
Patch
Comment 2 Darin Adler 2010-03-22 16:38:39 PDT
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".
Comment 3 Adam Barth 2010-03-22 16:57:48 PDT
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.
Comment 4 Adam Barth 2010-03-22 16:59:34 PDT
Created attachment 51371 [details]
Patch updated with Darin's feedback (will land later)
Comment 5 Adam Barth 2010-03-22 17:02:49 PDT
Created attachment 51372 [details]
Patch updated with Darin's feedback (will land later)
Comment 6 Adam Barth 2010-03-22 17:03:40 PDT
Comment on attachment 51371 [details]
Patch updated with Darin's feedback (will land later)

Typo in changelog (spotted by evmar)
Comment 7 Adam Barth 2010-03-22 19:17:06 PDT
Created attachment 51387 [details]
For EWS bots (no actual review needed)
Comment 8 WebKit Review Bot 2010-03-22 19:21:37 PDT
Attachment 51387 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1080005
Comment 9 Adam Barth 2010-03-22 19:26:03 PDT
Created attachment 51388 [details]
For EWS bots (no actual review needed)
Comment 10 WebKit Review Bot 2010-03-22 19:52:19 PDT
Attachment 51388 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1051119
Comment 11 Alexey Proskuryakov 2010-03-22 22:31:15 PDT
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.
Comment 12 Adam Barth 2010-03-22 22:37:42 PDT
> 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.
Comment 13 Sam Weinig 2010-03-22 22:40:07 PDT
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.
Comment 14 Adam Barth 2010-03-22 22:43:19 PDT
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.
Comment 15 Sam Weinig 2010-03-22 23:06:47 PDT
(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.
Comment 16 Adam Barth 2010-03-22 23:11:31 PDT
> 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.)
Comment 17 Ian 'Hixie' Hickson 2010-03-22 23:18:18 PDT
(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
Comment 18 Adam Barth 2010-03-22 23:23:03 PDT
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.
Comment 19 Adam Barth 2010-03-22 23:36:10 PDT
See also Bug 36480 for more tests related to document.load.
Comment 20 Adam Barth 2010-03-23 00:24:19 PDT
See also this related Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=100795
Comment 21 Alexey Proskuryakov 2010-03-23 08:29:42 PDT
(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).
Comment 22 Adam Barth 2010-03-23 13:14:51 PDT
> 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.
Comment 23 Alexey Proskuryakov 2010-03-23 14:14:16 PDT
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.
Comment 24 Adam Barth 2010-03-23 16:25:33 PDT
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.
Comment 25 Alexey Proskuryakov 2010-03-23 17:00:19 PDT
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.
Comment 26 Adam Barth 2010-03-23 17:06:39 PDT
> > 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.  :(
Comment 27 Adam Barth 2010-03-26 18:12:20 PDT
> 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
Comment 28 Alexey Proskuryakov 2010-08-09 04:10:37 PDT
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.
Comment 29 Alexey Proskuryakov 2011-10-20 20:41:14 PDT
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>.