837917 - Implement DOMCursor and DOMRequestService::fireDone()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

See bug 836519 comment 7. This is a lower risk fix for bug 836519. We can worry about bug 837865 later.

Comment 1

[Reuben Morais [:reuben]]

Attachment: Implement DOMCursor and DOMRequestService::fireDone

Comment 2

[Reuben Morais [:reuben]]

Comment on attachment 710972 [details] [diff] [review]
Implement DOMCursor and DOMRequestService::fireDone

Sorry for the trailing whitespace noise in nsDOMClassINfo.cpp, I can remove it from the patch if it's a problem.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 710972 [details] [diff] [review]
Implement DOMCursor and DOMRequestService::fireDone

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

This doesn't really do quite the right thing.

nsIDOMDOMCursor should inherit nsIDOMDOMRequest and just add a continue() function. I.e. it should look something like:

interface nsIDOMDOMCursor : nsIDOMDOMRequest {
  void continue();
}

To work around some XPCOM suckyness you can make the interface be something like

interface nsIDOMDOMCursor : nsISupports {
  void continue();
}


This way it's pretty easy to subclass DOMRequest to create a DOMCursor class which inherits nsIDOMDOMCursor and just implements the continue() function.

There should be no need for a nsICursorContinueHandler. Instead when we have a new value we simply fire a "success" event. So the javascript code will look something like:


cursor = someFunctionReturningACursor();
cursor.onsuccess = function(e) {
  ...
  cursor.continue();
}

Comment 4

[Reuben Morais [:reuben]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> This way it's pretty easy to subclass DOMRequest to create a DOMCursor class
> which inherits nsIDOMDOMCursor and just implements the continue() function.
> 
> There should be no need for a nsICursorContinueHandler. Instead when we have
> a new value we simply fire a "success" event. So the javascript code will
> look something like:

I did it like this to avoid requiring every consumer of the API to create a cursor class. Instead all you have to do is implement handleContinue, and then pass yourself as the continue handler when creating the cursor.

> cursor = someFunctionReturningACursor();
> cursor.onsuccess = function(e) {
>   ...
>   cursor.continue();
> }

This is better!

Comment 5

[Reuben Morais [:reuben]]

Attachment: Patch, v2 (as discussed on IRC)

"Easy to subclass", he said, "simply fire a success event", he said.

The API in this patch is more straightforward, just like you described in the original review. We need nsICursorContinueCallback to use the cursors in JavaScript, where we can't inherit from DOMCursor.

Comment 6

[Reuben Morais [:reuben]]

Please ignore the whitespace changes in DOMRequestHelper, I'll remove that in the next version or when checking in.

Comment 7

[Reuben Morais [:reuben]]

Attachment: Patch, v2 (as discussed on IRC), no whitespace changes

Sorry for the bug spam, my editor is configured to trim whitespace on save and apparently the style guide isn't as enforced as I imagined it would be, lots of files have those.

Comment 8

[Gregor Wagner [:gwagner]]

It's blocking the contacts performance.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

We will consider a low risk uplift nomination this week to get this in v1.0.1 but this isn't blocking any blockers and we're being very discriminating about what is a blockers at this point.

Comment 10

[Reuben Morais [:reuben]]

<mounir> I do not understand when .continue is called()
<mounir> I mean, you don't test it

continue() is called by content when it wants the next result to be fetched and sent, see the code in comment 3. This test uses it extensively: https://bugzilla.mozilla.org/attachment.cgi?id=713043&action=diff&headers=1&context=file#a/dom/contacts/tests/test_contacts_getall.html_sec1

<mounir> and what fireDone() is used for

DOMRequest::FireSuccess asserts on its state to make sure it's only called once. FireDone resets the internal state and unroots mResult so we can call FireSuccess again.

<mounir> also what's this callback for?

The callback is called in DOMCursor::Continue() to inform the consumer that content wants the next result in the list. See the handleContinue() function in https://bugzilla.mozilla.org/attachment.cgi?id=713043&action=diff&headers=1#a/dom/contacts/ContactManager.js_sec8

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 711942 [details] [diff] [review]
Patch, v2 (as discussed on IRC), no whitespace changes

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

It's not really clear what ::FireDone() was expected to do. In other words, I'm not certain how we can know we are at the end of the cursor. We could send a success with .result=null or maybe set a |done| attribute to true (that's what Jonas proposed a long time ago). We could also simply ignore the following .continue() call. I haven't seen any discussion regarding this.

Ideally, we should have something like this:
interface DOMCursor {
  DOMRequest forEach(Callback aCb);
};

So we could write:
var cursor = getThatCursor();
cursor.forEach(function(v) {
  array.push(v);
}).then(function() {
  // Do something with that array.
});

With the current DOMRequest interface, .then() should be .onsucces.

Actually, if we do not *need* to block on this, I would prefer if we could implement something like that because it would be a more Future-proof design ;)

::: dom/base/DOMCursor.cpp
@@ +45,5 @@
> +  // No result means we are waiting for a previous call.
> +  if (mResult == JSVAL_VOID) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> +  }
> +  mCallback->HandleContinue();

I'm not sure what this is trying to do. Shouldn't you reset the DOMRequest and be ready to get another success event sent?

::: dom/base/nsIDOMDOMRequest.idl
@@ +27,5 @@
>  interface nsIDOMRequestService : nsISupports
>  {
>    nsIDOMDOMRequest createRequest(in nsIDOMWindow window);
> +  nsIDOMDOMCursor createCursor(in nsIDOMWindow window,
> +                               in nsICursorContinueCallback aCallback);

What is this callback for?

@@ +33,5 @@
>    void fireSuccess(in nsIDOMDOMRequest request, in jsval result);
>    void fireError(in nsIDOMDOMRequest request, in DOMString error);
>    void fireSuccessAsync(in nsIDOMDOMRequest request, in jsval result);
>    void fireErrorAsync(in nsIDOMDOMRequest request, in DOMString error);
> +  void fireDone(in nsIDOMDOMRequest request, in jsval result);

What fireDone do? Seems like behaving like continue() should.

::: dom/base/test/test_domrequest.html
@@ +45,5 @@
>  
> +// fire again
> +ev = null;
> +req.onsuccess = function(e) {
> +  ev = e;

Being able to send two success event on the same DOMRequest seems evil. We shouldn't allow that. Unless I'm missing something and that object is a DOMCursor?
Actually, I would like to have a test that checks that *can't* happen.

And please, test the .continue() behaviour. Having a separate file (test_domcursor.html) would be nice actually.

::: dom/interfaces/base/nsIDOMDOMCursor.idl
@@ +4,5 @@
> +
> +#include "nsIDOMDOMRequest.idl"
> +
> +[scriptable, function, uuid(3a75d80a-9258-4ab8-95fd-ec0b5f634df1)]
> +interface nsICursorContinueCallback : nsISupports

I do not understand what's that for and it doesn't seem to be tested.

Comment 12

[Reuben Morais [:reuben]]

(In reply to Mounir Lamouri (:mounir) from comment #11)
> Comment on attachment 711942 [details] [diff] [review]
> Patch, v2 (as discussed on IRC), no whitespace changes
> 
> Review of attachment 711942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's not really clear what ::FireDone() was expected to do. In other words,
> I'm not certain how we can know we are at the end of the cursor. We could
> send a success with .result=null or maybe set a |done| attribute to true
> (that's what Jonas proposed a long time ago). We could also simply ignore
> the following .continue() call. I haven't seen any discussion regarding this.

The cursor doesn't know it's position in the result list, it serves only as a tunnel for the data. APIs exposing cursors can decide what to do to signal the last result (send null, stop sending success, etc). 

> ::: dom/base/DOMCursor.cpp
> @@ +45,5 @@
> > +  // No result means we are waiting for a previous call.
> > +  if (mResult == JSVAL_VOID) {
> > +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> > +  }
> > +  mCallback->HandleContinue();
> 
> I'm not sure what this is trying to do. Shouldn't you reset the DOMRequest
> and be ready to get another success event sent?

I do that in FireDone, mostly because in the previous version DOMCursor did not inherit from DOMRequest. Doing it in continue is better and makes the code simpler. I'll change that.

> ::: dom/base/nsIDOMDOMRequest.idl
> @@ +27,5 @@
> >  interface nsIDOMRequestService : nsISupports
> >  {
> >    nsIDOMDOMRequest createRequest(in nsIDOMWindow window);
> > +  nsIDOMDOMCursor createCursor(in nsIDOMWindow window,
> > +                               in nsICursorContinueCallback aCallback);
> 
> What is this callback for?

It's called whenever content calls continue(). I'll document it in a comment.
 
> And please, test the .continue() behaviour. Having a separate file
> (test_domcursor.html) would be nice actually.

Will do.

> ::: dom/interfaces/base/nsIDOMDOMCursor.idl
> @@ +4,5 @@
> > +
> > +#include "nsIDOMDOMRequest.idl"
> > +
> > +[scriptable, function, uuid(3a75d80a-9258-4ab8-95fd-ec0b5f634df1)]
> > +interface nsICursorContinueCallback : nsISupports
> 
> I do not understand what's that for and it doesn't seem to be tested.

See above.

Comment 13

[Reuben Morais [:reuben]]

Attachment: Patch, v3

Comments addressed. I got rid of FireDone() and moved the reset code to Continue(). Also added DOMCursor tests.

Comment 14

[Mounir Lamouri (:mounir)]

Comment on attachment 713151 [details] [diff] [review]
Patch, v3

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

Sorry Reuben but I think this will need one last iteration :(
The code is generally fine (there are a few comments but no big deals) but we need to have a better behaviour when we reach the end of the cursor. What we decided to do with Jonas is to add a boolean attribute named 'done' to DOMCursor. That boolean attribute would tell the consumer if the cursor is done. It goes without saying that calling .continue() if that boolean is true would fail.

So, what you should do I think is to have a fireDone() method that would set the result to JSVAL_VOID and set that boolean to true and fire a success event. You shouldn't have to change the behaviour of ::Continue() because mResult being JSVAL_VOID should make the call fail.
Two things to know:
- fireDone() should take a nsIDOMDOMCursor, not a nsIDOMDOMRequest;
- mDone already exist in DOMRequest but it is something completely different. You will have to find another name.

Also, to speed up things, feel free to ping Jonas and I on IRC today before sending a new patch. Hopefully, the next iteration will be a r+.

::: dom/base/DOMCursor.cpp
@@ +21,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED_1(DOMCursor, DOMRequest, mCallback)
> +
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(DOMCursor, DOMRequest)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END

I think you should take care of the callback in the cycle collection but we should probably ask someone who knows about CC.

@@ +26,5 @@
> +
> +DOMCursor::DOMCursor()
> +  : DOMRequest()
> +{
> +}

That should disappear.

@@ +36,5 @@
> +}
> +
> +DOMCursor::~DOMCursor()
> +{
> +}

That too.

@@ +41,5 @@
> +
> +NS_IMETHODIMP
> +DOMCursor::Continue()
> +{
> +  // No result means we are waiting for a previous call.

I think this comment should be:
// We need to have a result here because we must be in a 'success' state.

@@ +46,5 @@
> +  if (mResult == JSVAL_VOID) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> +  }
> +
> +  // Reset the request state so we can FireSuccess() again

nit: you need a "." at the end of the sentence ;)

::: dom/base/DOMCursor.h
@@ +24,5 @@
> +  NS_FORWARD_NSIDOMDOMREQUEST(DOMRequest::)
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(DOMCursor,
> +                                                         DOMRequest)
> +
> +  DOMCursor();

We do not need that default ctor for the moment so instead of declaring it, you should use MOZ_DELETE:
DOMCursor() MOZ_DELETE;

and don't implement the ctor.

@@ +27,5 @@
> +
> +  DOMCursor();
> +  DOMCursor(nsIDOMWindow* aWindow, nsICursorContinueCallback *aCallback);
> +
> +  virtual ~DOMCursor();

You do not need to declare and define the destructor if it does nothing.

::: dom/base/nsIDOMDOMCursor.idl
@@ +4,5 @@
> +
> +#include "nsIDOMDOMRequest.idl"
> +
> +[scriptable, function, uuid(3a75d80a-9258-4ab8-95fd-ec0b5f634df1)]
> +interface nsICursorContinueCallback : nsISupports

It's a bit weird to have this declared here but used in nsIDOMDOMRequest.idl. I guess with WebIDL, we could have a partial interface nsIDOMRequestService that adds the createCursor() method.

::: dom/base/nsIDOMDOMRequest.idl
@@ +27,1 @@
>  interface nsIDOMRequestService : nsISupports

Gasp, we should have name this nsIRequestService. It's not related to your bug though.

::: dom/base/test/test_domcursor.html
@@ +35,5 @@
> +}
> +
> +var tests = [
> +  function() {
> +    // create a cursor

Please, give a better description of the test: you are actually checking the interface of the DOMCursor object and the initial state.

@@ +91,5 @@
> +      is(e.target, req, "correct target during error");
> +      is(req.readyState, "done", "correct readyState after error");
> +      is(req.error.name, "error msg", "correct error after error");
> +      is(req.result, undefined, "correct result after error");
> +      next();

I would add the following tests:
- calling .continue() when the cursor is in a an 'error' state should throw;
- calling .continue() twice in a row (in a 'success' state before the first call) should throw;
- calling .continue() when the cursor is at the end should throw.

::: dom/base/test/test_domrequest.html
@@ +35,5 @@
>  req.onsuccess = function(e) {
>    ev = e;
> +  req.onsuccess = function() {
> +    ok(false, "onsuccess should only be called once");
> +  }

That's not what I meant. I meant that calling .fireSuccess() twice on the same request should not work. Your test is still testing that this is working.

What I want is something like:
var ev = null;
req.onsuccess = function(e) {
  ok(false, "we should not fire twice a success event on the same DOMRequest");
};
reqserv.fireSuccess(req, "my result");
is(ev, null, ...);
is(req.readyState, "done", ...);

Comment 15

[Reuben Morais [:reuben]]

Attachment: Patch, v4

(In reply to Mounir Lamouri (:mounir) from comment #14)> 
> ::: dom/base/test/test_domrequest.html
> @@ +35,5 @@
> >  req.onsuccess = function(e) {
> >    ev = e;
> > +  req.onsuccess = function() {
> > +    ok(false, "onsuccess should only be called once");
> > +  }
> 
> That's not what I meant. I meant that calling .fireSuccess() twice on the
> same request should not work. Your test is still testing that this is
> working.
> 
> What I want is something like:
> var ev = null;
> req.onsuccess = function(e) {
>   ok(false, "we should not fire twice a success event on the same
> DOMRequest");
> };
> reqserv.fireSuccess(req, "my result");

FireSuccess checks this with a (non-fatal) NS_ASSERTION. I'm not sure how I can test that from JS. Do you want me to change FireSuccess so it throws instead of asserting?

Comment 16

[Reuben Morais [:reuben]]

Comment on attachment 714088 [details] [diff] [review]
Patch, v4

Trying Sicking since he's still around.

Comment 17

[Reuben Morais [:reuben]]

Comment on attachment 714088 [details] [diff] [review]
Patch, v4

Sorry for the bugspam.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 714088 [details] [diff] [review]
Patch, v4

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

r=me with those things fixed.

Mounir, feel free to also do a review pass. Stealing this since we want to get this landed very soon.

::: dom/base/DOMCursor.cpp
@@ +37,5 @@
> +DOMCursor::DOMCursor(nsIDOMWindow* aWindow, nsICursorContinueCallback* aCallback)
> +  : DOMRequest(aWindow)
> +  , mCallback(aCallback)
> +  , mFinished(false)
> +{

Add a MOZ_ASSERT here to make sure that aCallback isn't null. That'll serve as documentation that a handler needs to be provided.

::: dom/base/DOMCursor.h
@@ +22,5 @@
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_NSIDOMDOMCURSOR
> +  NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)
> +  NS_FORWARD_NSIDOMDOMREQUEST(DOMRequest::)

If you make nsIDOMDOMCursor not inherit nsIDOMDOMRequest you can remove these FORWARD macros.

This stuff can be done better once we use WebIDL here.

::: dom/base/nsIDOMDOMCursor.idl
@@ +12,5 @@
> +  void handleContinue();
> +};
> +
> +[scriptable, builtinclass, uuid(062ea35a-5158-425a-b7bc-3ae9daa84398)]
> +interface nsIDOMDOMCursor : nsIDOMDOMRequest

Actually, make this not inherit nsIDOMDOMRequest. Otherwise you have to deal with ambiguities of having nsIDOMDOMRequest and nsIDOMEventTarget inherited multiple times.

Comment 19

[Reuben Morais [:reuben]]

Attachment: Patch, v4

Comments addressed. r=sicking

Comment 20

[Gregor Wagner [:gwagner]]

\o/

https://hg.mozilla.org/integration/mozilla-inbound/rev/8db40a38aeb1

Comment 21

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/c75dd4eaa371 for everybody's favorite test failure, https://tbpl.mozilla.org/php/getParsedLog.php?id=19761231&tree=Mozilla-Inbound

Comment 22

[Reuben Morais [:reuben]]

Attachment: Patch, v4

Aw I was about to attach a follow up patch :((((

Comment 23

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ec1e5aade7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3ec1e5aade7

Comment 25

[Reuben Morais [:reuben]]

Attachment: Patch, rebased to mozilla-b2g18

Comment 26

[Reuben Morais [:reuben]]

This blocks bug 836519 which blocks leo. Please land attachment 715652 [details] [diff] [review] in mozilla-b2g18.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1eafa1f01743

Comment 28

[Alex Keybl [:akeybl]]

Blocking tef+, therefore this is tef+.

Comment 29

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/33d8125f59ec

Comment 30

[Jeremie Patonnier :Jeremie]

Documentation is done here:

https://developer.mozilla.org/en-US/docs/Web/API/DOMCursor
https://developer.mozilla.org/en-US/docs/Web/API/DOMCursor.done
https://developer.mozilla.org/en-US/docs/Web/API/DOMCursor.continue

Review welcome :)