Bug 13707 – REGRESSION: JavaScript exceptions on quotes.burntelectrons.org

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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.

[Darin Adler]

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?

[Alexey Proskuryakov]

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.

[Darin Adler]

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!

[Alexey Proskuryakov]

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.

[Sam Weinig]

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.

[Maciej Stachowiak]

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.

[Oliver Hunt]

<rdar://problem/5335895>

[Alexey Proskuryakov]

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

[Adam Roben (:aroben)]

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

[Alexey Proskuryakov]

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

[Maciej Stachowiak]

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

[Alexey Proskuryakov]

Created attachment 15786 [details]
updated patch

Updated the wording per the above comment.

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

[Darin Adler]

Comment on attachment 15786 [details]
updated patch

I think the parameter to setBody should be a PassRefPtr<HTMLElement>.

r=me for feature branch

[Alexey Proskuryakov]

Committed revision 26078 (feature branch).