Last Comment Bug 722626 - Implement DOMRequest
: Implement DOMRequest
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
:
Mentors:
Depends on:
Blocks: 674720 678695
  Show dependency treegraph
 
Reported: 2012-01-30 23:11 PST by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2012-05-23 08:07 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (16.74 KB, text/plain)
2012-01-30 23:11 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details
WIP 2 (18.08 KB, patch)
2012-01-31 14:49 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Patch to fix (18.08 KB, patch)
2012-01-31 16:57 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review-
Details | Diff | Splinter Review
Patch v2 (22.50 KB, patch)
2012-02-22 08:53 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-30 23:11:05 PST
Created attachment 592998 [details]
WIP

This compiles and should mostly work, but probably has some issues.

The nsIDOMDOMRequest.idl file should explain how to use this from script.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-31 00:12:41 PST
Comment on attachment 592998 [details]
WIP

Tiny request, can you split Request and RequestService into different files?
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-31 00:26:53 PST
The .idl, or the .h or the .cpp, or all three? The .cpp for the service could probably go away completely and be inlined.

Oh, but I do need to quickstub nsIDOMDOMRequest. The service won't be used in a perf critical path so there's no need to quickstub it.
Comment 3 Olli Pettay [:smaug] (reviewing overload) 2012-01-31 02:18:47 PST
May I ask what is DOMRequest?
Comment 4 Mounir Lamouri (:mounir) 2012-01-31 06:31:17 PST
Jonas, do you plan to merge SmsRequest and IDBRequest with DOMRequest?

(In reply to Olli Pettay [:smaug] from comment #3)
> May I ask what is DOMRequest?

It's a way for new APIs to return some kind a promise. If you call a foo() function, it will return a DOMRequest object which will have a fire or success event fired on it at some point letting you know when things are done.

For exemple:
var request = doSomething(foo, bar);
request.onsuccess = function() { alert('cool!'); }
request.onerror = function() { alert('sad!'); }
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-31 09:21:46 PST
(In reply to Jonas Sicking (:sicking) from comment #2)
> The .idl, or the .h or the .cpp, or all three?

/me votes for all three.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-31 11:24:08 PST
I do want to get rid of SmsRequest in favor of DOMRequest.

I'd like to do the same with IDBRequest, but it'll require spec changes so it depends on if I can get those through.
Comment 7 Gregor Wagner [:gwagner] 2012-01-31 13:43:58 PST
We currently have permission issues. Something like
request.onsuccess = function() { alert('cool!'); } gives me:
3 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/contacts/tests/test_contacts_basics.html | an unexpected uncaught JS exception reported through window.onerror - Permission denied to access property 'onsuccess'
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-31 14:49:21 PST
Created attachment 593226 [details] [diff] [review]
WIP 2
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-31 15:08:50 PST
Initial tests shows that this appears to be working!
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-31 16:57:06 PST
Created attachment 593268 [details] [diff] [review]
Patch to fix

The only real issue that I ran into here is dealing with the various wrappers that are created for the DOMRequest object.

The problem is that the DOMRequest object is created in a chrome scope and then handed to the page. Initially this meant that the we got a COW wrapper (or whatever it's called these days) around the DOMRequest which prevented the page from using it.

To fix this I made createRequest instead return a jsval, and the implementation of the createRequest function explicitly creates a js object using the webpage's global object as scope.

This way XPConnect will create a X-Ray wrapper before handing the object to the calling chrome code, and then remove that X-Ray wrapper when the object is handed to the webpage.

At least that's what I think is happening :-)
Comment 11 Gregor Wagner [:gwagner] 2012-01-31 17:49:56 PST
I see now a compartment mismatch error when I want to access request.result:

-*- ContactManager: result: []
*** Compartment mismatch 0x115d12000 vs. 0x10a4d1000
Assertion failure: compartment mismatched, at /Volumes/mac/moz/ws3/js/src/jscntxtinlines.h:153

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
CrashInJS () at /Volumes/mac/moz/ws3/js/src/jsutil.cpp:94
94	    *((volatile int *) NULL) = 123;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  CrashInJS () at /Volumes/mac/moz/ws3/js/src/jsutil.cpp:94
#1  0x0000000103bce1dd in JS_Assert (s=0x103ffa64a "compartment mismatched", file=0x103ffa661 "/Volumes/mac/moz/ws3/js/src/jscntxtinlines.h", ln=153) at /Volumes/mac/moz/ws3/js/src/jsutil.cpp:114
#2  0x00000001039d7e75 in js::CompartmentChecker::fail (c1=0x115d12000, c2=0x10a4d1000) at jscntxtinlines.h:153
#3  0x00000001039d8d65 in js::CompartmentChecker::check (this=0x7fff5fbf2b98, c=0x10a4d1000) at jscntxtinlines.h:169
#4  0x00000001039d8e2e in js::CompartmentChecker::check (this=0x7fff5fbf2b98, obj=0x10aaabd00) at jscntxtinlines.h:177
#5  0x00000001039d8e83 in js::CompartmentChecker::check (this=0x7fff5fbf2b98, v=@0x7fff5fbf2ba8) at jscntxtinlines.h:187
#6  0x00000001039d5fd7 in js::assertSameCompartment<JSObject*, JS::Value> (cx=0x10d198560, t1=0x10ab97ee0, t2={data = {asBits = 18445477440788282624, debugView = {payload47 = 4473928960, tag = JSVAL_TAG_OBJECT}, s = {payload = {i32 = 178961664, u32 = 178961664, why = 178961664}}, asDouble = -nan(0xb80010aaabd00), asPtr = 0xfffb80010aaabd00, asWord = 18445477440788282624}}) at jscntxtinlines.h:261
#7  0x0000000103b4f3b4 in js::CallJSPropertyOp (cx=0x10d198560, op=0x1029d4600 <nsIDOMDOMRequest_GetResult>, receiver=0x10ab97ee0, id={asBits = 4473725248}, vp=0x7fff5fbf5968) at jscntxtinlines.h:363
#8  0x0000000103b0af1a in js::Shape::get (this=0x116342f38, cx=0x10d198560, receiver=0x10ab97ee0, obj=0x10ab97ee0, pobj=0x10ab97d90, vp=0x7fff5fbf5968) at jsscopeinlines.h:301
#9  0x0000000103afa5fa in js_NativeGetInline (cx=0x10d198560, receiver=0x10ab97ee0, obj=0x10ab97ee0, pobj=0x10ab97d90, shape=0x116342f38, getHow=2, vp=0x7fff5fbf5968) at /Volumes/mac/moz/ws3/js/src/jsobj.cpp:5230
#10 0x0000000103afafd3 in js_GetPropertyHelperInline (cx=0x10d198560, obj=0x10ab97ee0, receiver=0x10ab97ee0, id={asBits = 4473725248}, getHow=2, vp=0x7fff5fbf5968) at /Volumes/mac/moz/ws3/js/src/jsobj.cpp:5387
#11 0x0000000103afa970 in js::GetPropertyHelper (cx=0x10d198560, obj=0x10ab97ee0, id={asBits = 4473725248}, getHow=2, vp=0x7fff5fbf5968) at /Volumes/mac/moz/ws3/js/src/jsobj.cpp:5396
#12 0x0000000103ace297 in js::GetPropertyOperation (cx=0x10d198560, pc=0x10cff6be3 "5", lval=@0x10a500240, vp=0x7fff5fbf5968) at jsinterpinlines.h:288
#13 0x0000000103ab6a97 in js::Interpret (cx=0x10d198560, entryFrame=0x10a5001c0, interpMode=js::JSINTERP_NORMAL) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:2674
#14 0x0000000103aab600 in js::RunScript (cx=0x10d198560, script=0x11633af18, fp=0x10a5001c0) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:474
#15 0x0000000103ac5631 in js::InvokeKernel (cx=0x10d198560, args={<js::CallReceiver> = {usedRval_ = false, argv_ = 0x10a5001a8}, argc_ = 1}, construct=js::NO_CONSTRUCT) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:537
#16 0x0000000103a26e4b in js::Invoke (cx=0x10d198560, args=@0x7fff5fbf61a0, construct=js::NO_CONSTRUCT) at jsinterp.h:157
#17 0x0000000103ac5d4a in js::Invoke (cx=0x10d198560, thisv=@0x7fff5fbf6258, fval=@0x7fff5fbf62c0, argc=1, argv=0x7fff5fbf6cb0, rval=0x7fff5fbf68c8) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:569
Comment 12 Gregor Wagner [:gwagner] 2012-02-03 14:07:54 PST
Jonas fixed the compartment mismatch with a workaround.
Comment 13 Blake Kaplan (:mrbkap) 2012-02-04 03:18:40 PST
What was the workaround? I was wondering where aResult actually comes from when I was reading the patch and haven't had a chance to go figure it out.
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-04 23:26:41 PST
The way things work is something like this:

1. chrome script call createRequest and passes in a nsIDOMWindow object
2. createRequest creates a DOMRequest object and passes the nsIDOMWindow to the ctor (this is needed for event firing)
3. createRequest explicitly creates a js-object wrapper for the DOMRequest. The wrapper is created using the nsIDOMWindow as parent
4. createRequest returns the js-object wrapper to the chrome code
5. XPConnect wraps a xray wrapper around the js-object wrapper (at least this is my guess)
6. chrome code returns js-object to content code.
7. XPConnect unwraps the xray wrapper and returns the "normal" js-object wrapper to the content code
8. We wait
9. chrome code calls fireSuccess and passes in a jsval and the xray wrapper as arguments
10. the fireSuccess function calls an internal function on the DOMRequest and passes along the jsval
11. the internal function sets the jsval as mResult member on the DOMRequest
12. the internal function fires a "success" DOM event on the DOMRequest
13. a content event handler is called in response to the event firing.
14. the content code calls the .result getter on the DOMRequest
15. the getter returns the jsval in mResult

What happens with the patch as-is is that we return the jsval, which is still whatever chrome object was passed in step 9, to the content code. Since the getter is quick stubbed XPConnect doesn't wrap a COW (or whatever they are called these days) around the jsval, and so we end up with a compartment missmatch (as well as security problems).

The workaround was to remove quickstubs. This way XPConnect wraps a COW around the jsval and things work fine.

Johnny suggested that we should make quickstubs always check if wrapping is needed. Alternatively we could annotate this function such that quickstubs knows that this check is needed in this case.

It's hard to wrap the jsval on the way "to" mResult. This because as far as XPConnect sees, step 9 is calling a function on a chrome object and passes in a chrome object. No wrapping is needed.

Another way to fix this would be to make the fireSuccess function wrap as needed since the API is explicitly designed to pass jsvals from chrome code to content code.
Comment 15 Mounir Lamouri (:mounir) 2012-02-06 09:30:15 PST
Comment on attachment 593268 [details] [diff] [review]
Patch to fix

Review of attachment 593268 [details] [diff] [review]:
-----------------------------------------------------------------

(taking this review from mrbkap because I think he is on vacation)

r-, mostly because of mDone never set to true (thus useless). In addition, I would like to see a new version of this patch.

::: dom/base/DOMRequest.cpp
@@ +12,5 @@
> +#include "nsEventDispatcher.h"
> +#include "nsIPrivateDOMEvent.h"
> +
> +using mozilla::dom::DOMRequest;
> +using mozilla::dom::DOMRequestService;

Our code seems to usually do:
namespace mozilla {
namespace dom {
namespace class {

// Implementation

} // namespace class
} // namespace dom
} // namespace mozilla

Though, I think bent doesn't want to use that coding style too...

@@ +17,5 @@
> +
> +DOMRequest::DOMRequest(nsIDOMWindow* aWindow)
> +  : mDone(false),
> +    mResult(JSVAL_VOID),
> +    mRooted(false)

nit: comma at the beginning of the line is better because anyone can add a line without modifying the previous one

@@ +21,5 @@
> +    mRooted(false)
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  if (window) {
> +    mOwner = window->GetCurrentInnerWindow();

You should take into account that aWindow could be a inner window or are you sure the callers will always respect that assumption? In that case, an assert seems appropriate.

@@ +43,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(DOMRequest,
> +                                                nsDOMEventTargetWrapperCache)
> +  tmp->mResult = JSVAL_VOID;
> +  tmp->UnrootResultVal();

Do that only if the value is currently rooted.

@@ +54,5 @@
> +  // Don't need NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER because
> +  // nsDOMEventTargetHelper does it for us.
> +  if (JSVAL_IS_GCTHING(tmp->mResult)) {
> +    void *gcThing = JSVAL_TO_GCTHING(tmp->mResult);
> +    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(gcThing, "mResultVal")

nit: s/"mResultVal"/"mResult"/

@@ +73,5 @@
> +NS_IMETHODIMP
> +DOMRequest::GetReadyState(nsAString& aReadyState)
> +{
> +  mDone ? aReadyState.AssignLiteral("done") :
> +          aReadyState.AssignLiteral("pending");

nit: IMO, it's more readable with : below the ?.

@@ +79,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +DOMRequest::GetResult(jsval *aResult)

nit: |jsval *aResult| seems inconsistent with the rest of the file, you probably want |jsval* aResult|.

@@ +81,5 @@
> +
> +NS_IMETHODIMP
> +DOMRequest::GetResult(jsval *aResult)
> +{
> +  *aResult = mResult;

I think it might be interesting to add something like:
if (!mDone) {
  NS_ASSERTION(mResult == JSVAL_VOID, "...");
  *aResult = JSVAL_VOID;
  return NS_OK;
}

So we are sure we are respecting what is required per spec and we get an assert if things go wrong.

@@ +89,5 @@
> +
> +NS_IMETHODIMP
> +DOMRequest::GetError(nsIDOMDOMError** aError)
> +{
> +  NS_IF_ADDREF(*aError = mError);

Ditto but using nsnull.

@@ +97,5 @@
> +
> +void
> +DOMRequest::FireSuccess(jsval aResult)
> +{
> +  NS_ABORT_IF_FALSE(!mDone, "Already fired success/error");

Seems like mDone is never set to true...

@@ +108,5 @@
> +
> +void
> +DOMRequest::FireError(const nsAString& aError)
> +{
> +  NS_ABORT_IF_FALSE(!mDone, "Already fired success/error");

Seems like mDone is never set to true...

@@ +118,5 @@
> +
> +void
> +DOMRequest::FireEvent(const nsAString& aType)
> +{
> +  if (NS_FAILED(CheckInnerWindowCorrectness())) {

Can't you just set the inner window as mOwner in the ctor?

@@ +138,5 @@
> +  privevent->SetTrusted(true);
> +
> +  event->InitEvent(aType, false, false);
> +
> +  DispatchDOMEvent(nsnull, event, nsnull, nsnull);

Could you tell me why this code wouldn't work?

  nsRefPtr<nsDOMEvent> event = new nsDOMEvent(nsnull, nsnull);
  nsresult rv = event->InitEvent(aType, false, false);
  if (NS_FAILED(rv)) {
    return;
  }

  rv = event->SetTrusted(PR_TRUE);
  if (NS_FAILED(rv)) {
    return;
  }

  bool dummy;
  aRequest->DispatchEvent(event, &dummy);

Seems more common. In addition, there is a bug that aims to have a nsContentUtils::DispatchToSelf() method doing exactly this.

@@ +171,5 @@
> +DOMRequestService::FireSuccess(nsIDOMDOMRequest* aRequest,
> +                               const jsval& aResult)
> +{
> +  DOMRequest* req = static_cast<DOMRequest*>(aRequest);
> +  req->FireSuccess(aResult);

nit: you could also simply do:
static_cast<DOMRequest*>(aRequest)->FireSuccess(aResult);

@@ +181,5 @@
> +DOMRequestService::FireError(nsIDOMDOMRequest* aRequest,
> +                             const nsAString& aError)
> +{
> +  DOMRequest* req = static_cast<DOMRequest*>(aRequest);
> +  req->FireError(aError);

nit: static_cast<DOMRequest*>(aRequest)->FireError(aError);

::: dom/base/DOMRequest.h
@@ +50,5 @@
> +  void FireEvent(const nsAString& aType);
> +
> +  void RootResultVal()
> +  {
> +    if (!mRooted) {

I would ASSERT instead of checking. That should not happen.

@@ +58,5 @@
> +  }
> +
> +  void UnrootResultVal()
> +  {
> +    if (mRooted) {

ditto (assert)

@@ +65,5 @@
> +    }
> +  }
> +};
> +
> +class DOMRequestService : public nsIDOMRequestService

I would prefer a specific header for that.

@@ +83,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#define DOMREQUEST_SERVICE_CONTRACTID "@mozilla.org/dom/dom-request-service;1"

You could move this to the IDL too.

::: dom/base/nsDOMClassInfo.cpp
@@ +4365,5 @@
>      DOM_CLASSINFO_MAP_ENTRY(nsIDOMDOMError)
>    DOM_CLASSINFO_MAP_END
>  
> +  DOM_CLASSINFO_MAP_BEGIN(DOMRequest, nsIDOMDOMRequest)
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMDOMRequest)

I think you are missing:
DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget)

::: dom/base/nsIDOMDOMRequest.idl
@@ +17,5 @@
> +  readonly attribute jsval result;
> +  readonly attribute nsIDOMDOMError error;
> +
> +  attribute nsIDOMEventListener onsuccess;
> +  attribute nsIDOMEventListener onerror;

Could you try to keep the alignment/identation we usually see in idl files:
  readonly attribute Type  foo;
              attribute Type2 foobar;

@@ +21,5 @@
> +  attribute nsIDOMEventListener onerror;
> +};
> +
> +[scriptable, builtinclass, uuid(eebcdf29-f8fa-4c36-bbc7-2146b1cbaf7b)]
> +interface nsIDOMRequestService : nsISupports

I would prefer to have this moved to a specific idl.

::: layout/build/nsLayoutCID.h
@@ +124,5 @@
>  { 0x1a26a7b7, 0xd06e, 0x4f45, { 0x8b, 0x45, 0xd7, 0xad, 0x60, 0xf7, 0xa9, 0xab } }
>  
> +// {3160e271-138d-4cc7-9d63-6429f16957c7}
> +#define DOMREQUEST_SERVICE_CID \
> +{ 0x3160e271, 0x138d, 0x4cc7, { 0x9d, 0x63, 0x64, 0x29, 0xf1, 0x69, 0x57, 0xc7 } }

Could you put that in nsIDOMDOMRequest.idl?
Comment 16 Mounir Lamouri (:mounir) 2012-02-08 03:40:19 PST
Two other things about this patch:
- we need tests (and I believe that should be fairly easy to do);
- we need a solution to handle IPC, this implementation seems too much focused on JS modules.
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-22 07:36:17 PST
> ::: dom/base/DOMRequest.cpp
> @@ +12,5 @@
> > +#include "nsEventDispatcher.h"
> > +#include "nsIPrivateDOMEvent.h"
> > +
> > +using mozilla::dom::DOMRequest;
> > +using mozilla::dom::DOMRequestService;
> 
> Our code seems to usually do:
> namespace mozilla {
> namespace dom {
> namespace class {
> 
> // Implementation
> 
> } // namespace class
> } // namespace dom
> } // namespace mozilla
> 
> Though, I think bent doesn't want to use that coding style too...

I prefer using.

> @@ +21,5 @@
> > +    mRooted(false)
> > +{
> > +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> > +  if (window) {
> > +    mOwner = window->GetCurrentInnerWindow();
> 
> You should take into account that aWindow could be a inner window or are you
> sure the callers will always respect that assumption? In that case, an
> assert seems appropriate.

In fact, this totally fails because we always get an inner window. Fixed the code to handle both inner and outer windows.

> @@ +43,5 @@
> > +
> > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(DOMRequest,
> > +                                                nsDOMEventTargetWrapperCache)
> > +  tmp->mResult = JSVAL_VOID;
> > +  tmp->UnrootResultVal();
> 
> Do that only if the value is currently rooted.

Easier to move that check to the UnrootResultVal function.

> @@ +73,5 @@
> > +NS_IMETHODIMP
> > +DOMRequest::GetReadyState(nsAString& aReadyState)
> > +{
> > +  mDone ? aReadyState.AssignLiteral("done") :
> > +          aReadyState.AssignLiteral("pending");
> 
> nit: IMO, it's more readable with : below the ?.

I disagree

> @@ +118,5 @@
> > +
> > +void
> > +DOMRequest::FireEvent(const nsAString& aType)
> > +{
> > +  if (NS_FAILED(CheckInnerWindowCorrectness())) {
> 
> Can't you just set the inner window as mOwner in the ctor?

The idea is that if the user navigates we should not fire the event. See the implementation of CheckInnerWindowCorrectness

> @@ +65,5 @@
> > +    }
> > +  }
> > +};
> > +
> > +class DOMRequestService : public nsIDOMRequestService
> 
> I would prefer a specific header for that.

I don't :)

> @@ +83,5 @@
> > +
> > +} // namespace dom
> > +} // namespace mozilla
> > +
> > +#define DOMREQUEST_SERVICE_CONTRACTID "@mozilla.org/dom/dom-request-service;1"
> 
> You could move this to the IDL too.

I think I prefer as-is

> @@ +21,5 @@
> > +  attribute nsIDOMEventListener onerror;
> > +};
> > +
> > +[scriptable, builtinclass, uuid(eebcdf29-f8fa-4c36-bbc7-2146b1cbaf7b)]
> > +interface nsIDOMRequestService : nsISupports
> 
> I would prefer to have this moved to a specific idl.

I prefer as-is

> ::: layout/build/nsLayoutCID.h
> @@ +124,5 @@
> >  { 0x1a26a7b7, 0xd06e, 0x4f45, { 0x8b, 0x45, 0xd7, 0xad, 0x60, 0xf7, 0xa9, 0xab } }
> >  
> > +// {3160e271-138d-4cc7-9d63-6429f16957c7}
> > +#define DOMREQUEST_SERVICE_CID \
> > +{ 0x3160e271, 0x138d, 0x4cc7, { 0x9d, 0x63, 0x64, 0x29, 0xf1, 0x69, 0x57, 0xc7 } }
> 
> Could you put that in nsIDOMDOMRequest.idl?

I prefer as-is
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-22 08:53:58 PST
Created attachment 599636 [details] [diff] [review]
Patch v2
Comment 19 Blake Kaplan (:mrbkap) 2012-02-23 09:19:47 PST
Comment on attachment 599636 [details] [diff] [review]
Patch v2

Review of attachment 599636 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMRequest.cpp
@@ +21,5 @@
> +  , mResult(JSVAL_VOID)
> +  , mRooted(false)
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  mOwner = window->IsInnerWindow() ? window.get() :

I realized that in the previous patch, you null checked window. Can that still happen?

@@ +92,5 @@
> +NS_IMETHODIMP
> +DOMRequest::GetError(nsIDOMDOMError** aError)
> +{
> +  NS_ASSERTION(mDone || !mError,
> +               "Result should be undefined when pending");

Copy/paste error: the message here should refer to error, I think.

::: dom/base/DOMRequest.h
@@ +72,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIDOMREQUESTSERVICE
> +
> +  // Returns an owning reference! No one should call this but the factory.
> +  static DOMRequestService* FactoryCreate()

Maybe a stupid question, but can this return already_AddRefed<DOMRequestService>?
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-27 00:23:53 PST
Checked in a few days ago:

http://hg.mozilla.org/mozilla-central/rev/afc1125f9c14


(In reply to Blake Kaplan (:mrbkap) from comment #19)
> I realized that in the previous patch, you null checked window. Can that
> still happen?

No, that should never happen. I talked with blake about this and since it can never be null I also removed the nullcheck for the sgo.

> Copy/paste error: the message here should refer to error, I think.

Fixed

> Maybe a stupid question, but can this return
> already_AddRefed<DOMRequestService>?

This signature is forced by the macros used to create XPCOM factories :(
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-15 17:16:25 PDT
You forgot to hook mError up to cycle collection.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-16 00:16:10 PDT
Hmm.. mError is just a DOMError object, so it shouldn't be able to participate in cycles. mResult does seem like we need to cc though!
Comment 24 Eric Shepherd [:sheppy] 2012-05-23 07:51:56 PDT
It looks like at present DOMRequest is not part of a specification. True? Either way, is there a spec or draft spec I can link to? Is it planned to be submitted for spec?
Comment 25 Mounir Lamouri (:mounir) 2012-05-23 08:07:23 PDT
DOM Core would have been the best specification to host that but I asked around and it can't be added because of the onfoo methods which need a dependency to HTML specification (and for sanity, DOM Core doesn't want that). The solution is to have that part moved to DOM Core but it's not going to happen very soon I believe.

In the mean time, I don't really know we could do.

Note You need to log in before you can comment on or make changes to this bug.