Bug 36465

Created attachment 51368 [details] Patch

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".

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.

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

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

Comment on attachment 51371 [details] Patch updated with Darin's feedback (will land later) Typo in changelog (spotted by evmar)

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

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

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

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

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.

> 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.

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.

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.

(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.

> 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.)

(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

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.

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

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

(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).

> 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.

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.

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.

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.

> > 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. :(

> 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

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.

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>.

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