Closed Bug 139276 Opened 20 years ago Closed 19 years ago

document.load() return values and synchronous loading

Categories

(Core :: XML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.4alpha

People

(Reporter: hjtoi-bugzilla, Assigned: hjtoi-bugzilla)

References

()

Details

(Whiteboard: [fixinhand])

Attachments

(1 file, 2 obsolete files)

According to Microsoft, document.load() return value is boolean:
http://msdn.microsoft.com/library/en-us/xmlsdk/htm/xml_mth_hn_2uck.asp?frame=true

We should change our load() method to return boolean (instead of void), and
return false in error conditions. Gotta figure out what kind of error
conditions, though...
Priority: -- → P3
Target Milestone: --- → mozilla1.1alpha
document.load can only return an error condition in its initial stages.
After all it is supposed to be asynchronous. It should send a DOM error or abort 
event if it can't complete the job (e.g. URL not available, server died etc.). 
Right. But I have also been thinking about adding support for sync load(). Might
do these at the same time.
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Boris is working on DOM 3 Load & Save so I believe this will be fixed then.
Target Milestone: mozilla1.2alpha → Future
Depends on: 155749
QA Contact: petersen → rakeshmishra
Summary: document.load() return values → document.load() return values and synchronous loading
Target Milestone: Future → mozilla1.4alpha
Attached patch wip2, works (obsolete) — Splinter Review
This patch still has remnants of some other bug fixes in it, but the main thing
is that this now works (at least after you remove the remnants of other
fixes...)! This will still need some heavy testing (files with xml-stylesheets
and without), and comparisons with IE to see that we match it.

This should also be moved to the DOM Level 3 Load & Save DocumentLS interface.
Attachment #111875 - Attachment is obsolete: true
Attached patch Proposed fixSplinter Review
Attachment #112350 - Attachment is obsolete: true
Comment on attachment 117949 [details] [diff] [review]
Proposed fix

This fix is a very close relative to bug 166978, so take a look at that if you
want to get some background for this fix.

This will get us a working synchronous document.load, but there are other bugs
that will later move this stuff into DOM DocumentLS module, and make us use a
single syncloader for all synchronous loading.
Attachment #117949 - Flags: superreview?(darin)
Attachment #117949 - Flags: review?(jst)
Status: NEW → ASSIGNED
Whiteboard: [fixinhand]
Comment on attachment 117949 [details] [diff] [review]
Proposed fix

+nsDocument::GetAsync(PRBool *aAsync)
+{
+  NS_ERROR("nsDocument::GetAsync() should be overriden by subclass!");
+
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+nsDocument::SetAsync(PRBool aAsync)
+{
+  NS_ERROR("nsDocument::SetAsync() should be overriden by subclass!");
+
+  return NS_OK;
+}

I'd say both of those should return NS_ERROR_NOT_IMPLEMENTED, if anyone ever
calls those, they should know about it...

r=jst
Attachment #117949 - Flags: review?(jst) → review+
Comment on attachment 117949 [details] [diff] [review]
Proposed fix

>+  NS_ERROR("nsDocument::GetAsync() should be overriden by subclass!");
>+  return NS_OK;

NS_NOTREACHED?

should these return NS_ERROR_NOT_IMPLEMENTED?  returning NS_OK w/o
assigning all out params spells trouble.


>Index: content/xml/document/src/nsXMLDocument.cpp

>+    if (!mEventQService) {
>+      return NS_ERROR_FAILURE;

NS_ENSURE_TRUE(mEventQService, NS_ERROR_FAILURE);


>+    if (modalEventQueue) {
>+      mEventQService->PopThreadEventQueue(modalEventQueue);
>+    }

>+    if (modalEventQueue) {
>+      mEventQService->PopThreadEventQueue(modalEventQueue);
>+    }

hmm... begs for a scoped stack based event queue popper, like the
one used to implement OpenWindowJS.


nothing but nits... sr=darin
Attachment #117949 - Flags: superreview?(darin) → superreview+
Made us return NS_ERROR_NOT_IMPLEMENTED in nsDocument, switched to the
convenience macro in that one instance in nsXMLDocument. I did not do the stack
popper, because this stuff should be replaced by the syncloader soon.

Checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
*** Bug 40566 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.