Implement DOMCursor and DOMRequestService::fireDone()

RESOLVED FIXED in Firefox 21

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

({dev-doc-complete})

Trunk
mozilla21
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

6 years ago
See bug 836519 comment 7. This is a lower risk fix for bug 836519. We can worry about bug 837865 later.
(Assignee)

Updated

6 years ago
Summary: Implement DOMRequestService::fireDone() → Implement DOMCursor and DOMRequestService::fireDone()
(Assignee)

Comment 1

6 years ago
Created attachment 710972 [details] [diff] [review]
Implement DOMCursor and DOMRequestService::fireDone
Attachment #710972 - Flags: feedback?(anygregor)
(Assignee)

Comment 2

6 years ago
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.
Attachment #710972 - Flags: feedback?(anygregor) → review?(jonas)
(Assignee)

Updated

6 years ago
Blocks: 837865
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();
}
Attachment #710972 - Flags: review?(jonas) → review-
(Assignee)

Comment 4

6 years ago
(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!
(Assignee)

Comment 5

6 years ago
Created attachment 711937 [details] [diff] [review]
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.
Attachment #710972 - Attachment is obsolete: true
Attachment #711937 - Flags: review?(jonas)
(Assignee)

Comment 6

6 years ago
Please ignore the whitespace changes in DOMRequestHelper, I'll remove that in the next version or when checking in.
(Assignee)

Comment 7

6 years ago
Created attachment 711942 [details] [diff] [review]
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.
Attachment #711937 - Attachment is obsolete: true
Attachment #711937 - Flags: review?(jonas)
Attachment #711942 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Attachment #711942 - Flags: review?(mounir)
It's blocking the contacts performance.
blocking-b2g: --- → tef?
OS: Mac OS X → All
Hardware: x86 → All
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.
blocking-b2g: tef? → -
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
tracking-b2g18: --- → +
(Assignee)

Updated

6 years ago
Blocks: 838467
(Assignee)

Comment 10

6 years ago
<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 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.
Attachment #711942 - Flags: review?(mounir)
Attachment #711942 - Flags: review?(jonas)
Attachment #711942 - Flags: review-
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
Created attachment 713151 [details] [diff] [review]
Patch, v3

Comments addressed. I got rid of FireDone() and moved the reset code to Continue(). Also added DOMCursor tests.
Attachment #711942 - Attachment is obsolete: true
Attachment #713151 - Flags: review?(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", ...);
Attachment #713151 - Flags: review?(mounir) → review-
(Assignee)

Comment 15

6 years ago
Created attachment 714088 [details] [diff] [review]
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?
Attachment #713151 - Attachment is obsolete: true
Attachment #714088 - Flags: review?(mounir)
(Assignee)

Comment 16

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

Trying Sicking since he's still around.
Attachment #714088 - Flags: review?(mounir) → review?(jonas)
(Assignee)

Comment 17

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

Sorry for the bugspam.
Attachment #714088 - Flags: review?(jonas) → review?(mounir)
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.
Attachment #714088 - Flags: review?(mounir) → review+
(Assignee)

Comment 19

6 years ago
Created attachment 714244 [details] [diff] [review]
Patch, v4

Comments addressed. r=sicking
Attachment #714088 - Attachment is obsolete: true
Attachment #714244 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Keywords: checkin-needed
(Assignee)

Comment 22

6 years ago
Created attachment 714263 [details] [diff] [review]
Patch, v4

Aw I was about to attach a follow up patch :((((
Attachment #714244 - Attachment is obsolete: true
Attachment #714263 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a3ec1e5aade7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Comment 25

6 years ago
Created attachment 715652 [details] [diff] [review]
Patch, rebased to mozilla-b2g18
(Assignee)

Comment 26

6 years ago
This blocks bug 836519 which blocks leo. Please land attachment 715652 [details] [diff] [review] in mozilla-b2g18.
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
blocking-b2g: - → leo?
Keywords: checkin-needed
blocking-b2g: leo? → leo+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1eafa1f01743
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
(Assignee)

Updated

5 years ago
Blocks: 846895
Blocking tef+, therefore this is tef+.
blocking-b2g: leo+ → tef+
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/33d8125f59ec
status-b2g18-v1.0.1: wontfix → fixed
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.