Closed Bug 1321471 Opened 3 years ago Closed 3 years ago

Use of MozPromise::ThenPromise() in place of CompletionPromise()

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

A follow-up of bug 1321250.

The client code will call ThenPromise() for more promise chaining or Then() to stop.
Assignee: nobody → jwwang
Depends on: 1321250
Priority: -- → P3
Attachment #8816046 - Flags: review?(jyavenard)
Comment on attachment 8816046 [details]
Bug 1321471. Part 1 - Use of MozPromise::ThenPromise() in place of CompletionPromise(). .

https://reviewboard.mozilla.org/r/96830/#review97086

Please split the patch in two.

One removing the use of CompletionPromise

and one remove CompletionPromise() method altogether.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
>  
> -  return Reader()->Shutdown()
> -    ->Then(OwnerThread(), __func__, master,
> +  return Reader()->Shutdown()->ThenPromise(
> +    OwnerThread(), __func__, master,
> -           &MediaDecoderStateMachine::FinishShutdown,
> +    &MediaDecoderStateMachine::FinishShutdown,
> -           &MediaDecoderStateMachine::FinishShutdown)
> +    &MediaDecoderStateMachine::FinishShutdown);
> -    ->CompletionPromise();

clang-format's mozilla make that:

  return Reader()->Shutdown()->ThenPromise(
    OwnerThread(), __func__, master, &MediaDecoderStateMachine::FinishShutdown,
    &MediaDecoderStateMachine::FinishShutdown);

::: xpcom/threads/MozPromise.h
(Diff revision 1)
>  
>      // MSVC complains when an inner class (ThenValueBase::{Resolve,Reject}Runnable)
>      // tries to access an inherited protected member.
>      bool IsDisconnected() const { return mDisconnected; }
>  
> -    virtual MozPromise* CompletionPromise() = 0;

this should be split in another patch. That's quite out of scope from the commit description.
Attachment #8816046 - Flags: review?(jyavenard) → review+
Comment on attachment 8816046 [details]
Bug 1321471. Part 1 - Use of MozPromise::ThenPromise() in place of CompletionPromise(). .

https://reviewboard.mozilla.org/r/96830/#review97086

> clang-format's mozilla make that:
> 
>   return Reader()->Shutdown()->ThenPromise(
>     OwnerThread(), __func__, master, &MediaDecoderStateMachine::FinishShutdown,
>     &MediaDecoderStateMachine::FinishShutdown);

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation
2 spaces indention.

> this should be split in another patch. That's quite out of scope from the commit description.

Will do.
Comment on attachment 8816350 [details]
Bug 1321471. Part 2 - remove CompletionPromise() to enforce use of ThenPromise().

https://reviewboard.mozilla.org/r/97112/#review97348

Split the patch per discussion and carry r+.
Attachment #8816350 - Flags: review+
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31c6008a6773
Part 1 - Use of MozPromise::ThenPromise() in place of CompletionPromise(). r=jya.
https://hg.mozilla.org/integration/autoland/rev/24424daa3526
Part 2 - remove CompletionPromise() to enforce use of ThenPromise(). r=jwwang
Blocks: 1321744
https://hg.mozilla.org/mozilla-central/rev/31c6008a6773
https://hg.mozilla.org/mozilla-central/rev/24424daa3526
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.