Scripts do not find document.cookies and document.location, because we now get XHTML instead of HTML. There are other bugs tracking the same problem, but I'm glad to file a P1 regression :)
Created attachment 14536 [details] proposed fix Of earlier bugs, I only found bug 12628, which this patch also fixes. I think I also saw a complaint about missing Document.cookie, but maybe it was on a mailing list.
Comment on attachment 14536 [details] proposed fix Looks great. + * dom/Document.idl: Additions mostly match HTML5 HTMLDocument interface, except for + of location, which is read-only in HTML5, and lastModified, which is not present there. "for of" here. I'd like to see more of JSDocument::setLocation in a method named Document::setLocation. We should do everything possible to have the language bindings be only language issues. So I'd have the setLocation function take parameters for the Frame* and the wasRunByUserGesture boolean, but have the call to FrameLoader be in the document. Not new code, but it's bad long term for us to use the idiom frame->loader()->documentLoader() to get to the loader for this document, because that could be the loader for the previous or next document in this frame rather than the current one. I'd like that to be encapsulated in a documentLoader() function so we can fix it in one place. Why did the type of the return value for getElementsByName change? Internally it can be useful for the types to be more specific than those in the IDL files. Was there a problem compiling the JavaScript binding? If so, I guess that's the best solution. Too bad. -#if !defined(LANGUAGE_OBJECTIVE_C) +#if defined(LANGUAGE_JAVASCRIPT) I think we want these APIs in other languages. Why make them JavaScript-specific?
Created attachment 14611 [details] proposed fix (In reply to comment #2) > I'd like to see more of JSDocument::setLocation in a method named > Document::setLocation. AFAICT, all that can be moved there is a single frame->loader()->scheduleLocationChange() call. I would think that would only increase confusion, since there's already a bunch of loosely similar functions for changing location or URL scattered throughout the code. Or am I missing something? > Not new code, but it's bad long term for us to use the idiom > frame->loader()->documentLoader() to get to the loader for this document, > because that could be the loader for the previous or next document in this > frame rather than the current one. I'd like that to be encapsulated in a > documentLoader() function so we can fix it in one place. I've added such a function to Frame, but I'm not sure if all instances of "frame->loader()->documentLoader()" should be replaced with it. > Why did the type of the return value for getElementsByName change? Internally > it can be useful for the types to be more specific than those in the IDL files. > Was there a problem compiling the JavaScript binding? If so, I guess that's the > best solution. Too bad. Yes, the binding didn't compile otherwise. > I think we want these APIs in other languages. Why make them > JavaScript-specific? I guess I still thought of them as obsolete, despite them being first-class citizens in HTML5. Fixed.
Comment on attachment 14611 [details] proposed fix Document::setBody should take a PassRefPtr<HTMLElement> rather than an HTMLElement*. Why not have getElementsByName return NameNodeList? Does it create a real problem. I ask because sometimes we can generate more efficient code if the specific class is known to the caller, so sometimes we might want our internal function to have a more-specific type than what's exported to JS and other public API. + (WebCore::Frame::documentLoader): Encapsulated "frame->loader()->documentLoader()" idiom, + which is dangerous because that could be the loader for the previous or next document in this + frame rather than the current one. Having it in one place will make fixing it easier. I agree with your point about encapsulating it in one place. But that place should be in Document, not Frame!
This will need to be rewritten after a patch from bug 13830 lands, and I suppose Sam may get to it faster than myself; unassigning.
Created attachment 14707 [details] updated patch Updates the patch to deal with code changes. Some slight changes, I did not make the FrameLoader related changes because I felt that it was outside the scope of the bug. I moved the applets collection along with the other collection accessors that were moved down. I also updated the ObjC bindings expected results to correspond to the new results.
Is it the best approach to put all HTMLDocument methods and properties directly in the Document interface? Another possible approach would be to retain separate SVGDocument and HTMLDocument interfaces, but to make both those interfaces available on all documents. (I think there may be a few conflicts between SVGDocument and HTMLDocument which would have to be appropriately resolved.) Additional comments: +#if defined(LANGUAGE_JAVASCRIPT) + attribute [Custom] Window location; #endif This is surely wrong, the location property returns a Location object, not a Window. I realize this is a pre-existing bug but changing the IDL seems like a good time to fix it. r- for now to consider the general design issue and this one detail. Otherwise looks fine.
<rdar://problem/5335895>
Created attachment 15586 [details] updated patch Taking this back :) It seems that splitting the interfaces into separate IDLs is out of the scope of this bug (and maybe even inappropriate during stabilization). The Window vs. Location issue has been already fixed on TOT. So, I've just updated the patch to cleanly apply. Interestingly, there's now one more DOM test passing than before (dom/xhtml/level2/html/HTMLDocument08 is the new success).
Comment on attachment 15586 [details] updated patch + // When assigning location, IE and Mozilla both resolve the URL + // relative not the target frame. Typo: "not" => "to" Do we have a test for this behavior? The code looks good to me, but I'd like to hear back from Maciej once more before giving r+.
(In reply to comment #10) > Typo: "not" => "to" > Do we have a test for this behavior? I don't know - I just moved this code from JSHTMLDocumentCustom. Actually, I'm not even sure if changing "not" to "to" would make this statement correct, that's why I didn't change it.
+ // When assigning location, IE and Mozilla both resolve the URL + // relative not the target frame. I think the correct statement here is "IE and Mozilla both resolve the URL relative to the source frame, not the target frame."
Created attachment 15786 [details] updated patch Updated the wording per the above comment.
*** Bug 14887 has been marked as a duplicate of this bug. ***
Comment on attachment 15786 [details] updated patch I think the parameter to setBody should be a PassRefPtr<HTMLElement>. r=me for feature branch
Committed revision 26078 (feature branch).