Closed Bug 1328130 Opened 7 years ago Closed 7 years ago

Alternative syntax to MozPromiseRequestHolder::Begin()

Categories

(Core :: XPCOM, defect, P3)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(3 files)

Bug 1328130. Part 1 - add ->Track().
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1328130. Part 2 - remove MozPromiseRequestHolder::Begin().
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1328130. Part 3 - remove unused functions and fix comments.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details 
For

MozPromise<> p;
MozPromiseRequestHolder<> holder;

we want to allow

p->Then()->Then()->Then()->End(holder);

The syntax is better than |holder.Begin(p->Then()->Then()->Then())| in several ways:

1. it is more indentation friendly.
2. it is more obvious the holder is associated with the request returned by last Then().
3. it is more consistent with the syntax of Then().
Assignee: nobody → jwwang
Priority: -- → P3
This seems like a nice idea, though I'm not wild about the name.

Maybe ->Hold(holder)?
Adding End() or Hold() feels like re-introducing something like CompletionPromise() that you worked very hard to remove! :-)

Instead, would it be possible to just have `MozPromiseRequestHolder<...> holder = p->...->Then();` automagically work?
(Possibly by having MozPromiseHolder's constructor and operator= taking a ThenCommand and doing a Begin() with it?)
(In reply to Bobby Holley (PTO) (busy with Stylo) from comment #1)
> This seems like a nice idea, though I'm not wild about the name.
> 
> Maybe ->Hold(holder)?

The word "End" is chosen:
1. to pair with the Begin of MozPromiseRequestHolder.
2. to signify the end of promise chaining and is semantically related to "Then"
(In reply to Gerald Squelart [:gerald] from comment #2)
> Adding End() or Hold() feels like re-introducing something like
> CompletionPromise() that you worked very hard to remove! :-)

I don't get it. Can you elaborate more details?
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #4)
> (In reply to Gerald Squelart [:gerald] from comment #2)
> > Adding End() or Hold() feels like re-introducing something like
> > CompletionPromise() that you worked very hard to remove! :-)
> 
> I don't get it. Can you elaborate more details?

Bad joke I guess...
I meant that previously we had to do `p->Then()->CompletionPromise()->Then()->etc...`, and you removed CompletionPromise (thank you!).
Now you are introducing a new step in the chain, where we have to do `->End()`. I'm hoping we could just do without it altogether, but I don't know if that's possible.
No. ->End() is not necessary.

|p->Then()->End(holder)| is equivalent to |holder.Begin(p->Then())|.

We have some code in gtests where the result of |p->Then()| is not used at all.

Note the result of |p->Then()| must either be disconnected or completed otherwise you will fail the assertion at http://searchfox.org/mozilla-central/rev/22be34bcc4d5c56b62482a537bba77a6cdce117b/xpcom/threads/MozPromise.h#347.
In case I still was not clear:

* Currently we have to do:
|holder.Begin(p->Then()->Then()->Then())|

* You are proposing that we could more easily do:
|p->Then()->Then()->Then()->End(holder)|

* And I'm hoping that we could even more easily do:
|holder = p->Then()->Then()->Then()|

So assigning the last ThenCommand to the MozPromiseRequestHolder would magically do the same thing as what `Begin()` does now, and the same thing as your proposed `End(holder)` -- so hopefully this would take care of "consuming" the last Then(), to avoid the assertion you point at in comment 6.
Exactly. The 3 examples should do the same thing under the hood. I thought about |holder = p->Then()->Then()->Then()| before. But it turns out I prefer the syntax of |p->Then()->Then()->Then()->End(holder)| somehow.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> (In reply to Bobby Holley (PTO) (busy with Stylo) from comment #1)
> > This seems like a nice idea, though I'm not wild about the name.
> > 
> > Maybe ->Hold(holder)?
> 
> The word "End" is chosen:
> 1. to pair with the Begin of MozPromiseRequestHolder.

Maybe I don't understand, but my impression is that this is _replacing_ MozPromiseRequestHolder::Begin, not being paired with it. That's why "End" seems like a strange name - it's doing the same thing that "Begin" did previously (i.e. "Begin tracking the promise in the holder"), but it's now called "End".
It is like the 2 sides of the same coin. When you "begin" tracking a request to the promise, you also "end" the promise chaining. We put focus on different sides though.

For |p->Then()->Then()->Then()->Then()->End()|, the End() just looks like the end of a train spatially. It also looks like the end of the Then() chaining temporally.
I see that, but think it's more important to indicate what precisely is happening to the promise, rather than just emphasizing that the chain is ending - especially since most chains are just one |Then()| call.

So I think I like ->Hold or ->Track or the operator= thing.
Good. There are 2 votes for operator=. Let's go for that. :)
If we allow operator=, we probably also need an implicit constructor to allow:
MozPromiseRequestHolder holder = p->Then();

Otherwise, it is awkward that you can do:
MozPromiseRequestHolder holder; holder = p->Then();

but not:
MozPromiseRequestHolder holder = p->Then();

On second thought, I will go for ->Track() and remove MozPromiseRequestHolder::Begin() so we have only one syntax to track a promise and avoid the confusion caused by too many ways in doing one thing.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #13)
> If we allow operator=, we probably also need an implicit constructor to allow:
> MozPromiseRequestHolder holder = p->Then();

No need to be implicit, I think.

If you add `template<typename PromiseType> friend class MozPromiseRequestHolder;` just above the definition of ThenCommand, you can have an explicit constructor:
  explicit MozPromiseRequestHolder(typename PromiseType::ThenCommand&& aThen) : mRequest(Move(aThen)) {}

(It can be done without 'friend', but it's harder and uglier.)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #13)
> If we allow operator=, we probably also need an implicit constructor to
> allow:
> MozPromiseRequestHolder holder = p->Then();
> 
> Otherwise, it is awkward that you can do:
> MozPromiseRequestHolder holder; holder = p->Then();
> 
> but not:
> MozPromiseRequestHolder holder = p->Then();

When do we ever stack-declare holders though?
 
> On second thought, I will go for ->Track() and remove
> MozPromiseRequestHolder::Begin() so we have only one syntax to track a
> promise and avoid the confusion caused by too many ways in doing one thing.

Sounds good to me in any case. Thinking about it more, I think the operator= stuff is kind of confusing in this use-case.
I agree operator= is confusing in this case because the assignment happens between 2 different classes.
Attachment #8826484 - Flags: review?(gsquelart)
Attachment #8826485 - Flags: review?(gsquelart)
Attachment #8826486 - Flags: review?(gsquelart)
Comment on attachment 8826485 [details]
Bug 1328130. Part 2 - remove MozPromiseRequestHolder::Begin().

https://reviewboard.mozilla.org/r/104448/#review105120

p->Then->...->Then->Track does look best! Good choice, Bobby and JW.
Attachment #8826485 - Flags: review?(gsquelart) → review+
Comment on attachment 8826486 [details]
Bug 1328130. Part 3 - remove unused functions and fix comments.

https://reviewboard.mozilla.org/r/104450/#review105122
Attachment #8826486 - Flags: review?(gsquelart) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a58564c456a
Part 1 - add ->Track(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/cae8a8f65c45
Part 2 - remove MozPromiseRequestHolder::Begin(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/cfe45fea824d
Part 3 - remove unused functions and fix comments. r=gerald
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: