Closed Bug 1367701 Opened 7 years ago Closed 7 years ago

Remove SeekJob::mTransition

Categories

(Core :: Audio/Video: Playback, enhancement)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kaku, Assigned: kaku)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1367701 P1 - add a virtual Transit() method and remove SeekJob:mTransition;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1367701 P2 - transit to NextFrameSeekingState in Transit();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1367701 P3 - modify comments;
59 bytes, text/x-review-board-request
jwwang
: review+
Details 
Spawn from bug 1366362 comment 32.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Comment on attachment 8871215 [details]
Bug 1367701 P1 - add a virtual Transit() method;

https://reviewboard.mozilla.org/r/142704/#review146366

::: dom/media/MediaDecoderStateMachine.cpp:1028
(Diff revision 1)
>  
>  protected:
>    SeekJob mSeekJob;
>  
>    virtual void DoSeek() = 0;
> +  virtual void Transit() { SetState<DecodingState>(); }

NextState sounds more appropriate to me
Comment on attachment 8871216 [details]
Bug 1367701 P1 - add a virtual Transit() method and remove SeekJob:mTransition;

https://reviewboard.mozilla.org/r/142706/#review146370

P2 and P1 should be merged together. As with just P1 NextFrameSeek is broken once again.

::: dom/media/MediaDecoderStateMachine.cpp:1763
(Diff revision 1)
>  private:
>    MozPromiseRequestHolder<MediaDecoder::SeekPromise> mAccurateSeekRequest;
>    SeekJob mFutureSeekJob;
>  
> -  void Transit() override { // No transition after seek completed. }
> +  // We don't want to transition to DecodingState once this seek completes.
> +  void Transit() override { }

this member shouldn't be made private. it's a bad habit I've seen ised in this code
Comment on attachment 8871215 [details]
Bug 1367701 P1 - add a virtual Transit() method;

https://reviewboard.mozilla.org/r/142704/#review146372

::: dom/media/MediaDecoderStateMachine.cpp:1028
(Diff revision 1)
>  
>  protected:
>    SeekJob mSeekJob;
>  
>    virtual void DoSeek() = 0;
> +  virtual void Transit() { SetState<DecodingState>(); }

I will say:

// Transition to the next state (defined by the subclass) when seek is completed.
virtual void GoToNextState();
Attachment #8871215 - Flags: review?(jwwang) → review+
Comment on attachment 8871217 [details]
Bug 1367701 P2 - transit to NextFrameSeekingState in Transit();

https://reviewboard.mozilla.org/r/142708/#review146374

::: dom/media/MediaDecoderStateMachine.cpp:1739
(Diff revision 1)
>    {
>      mFutureSeekJob = Move(aFutureSeekJob);
>  
>      AccurateSeekingState::Enter(Move(aCurrentSeekJob),
>                                  EventVisibility::Suppressed)
>        ->Then(OwnerThread(),

i think the entire MozPromiseRequestHolder can be removed, we no longer need thr Then.

either the promise is resolved and the it will go to the next state, or Exit will be called early which will reject the promise.

we no longer need to care about interrupting the workflow
Comment on attachment 8871216 [details]
Bug 1367701 P1 - add a virtual Transit() method and remove SeekJob:mTransition;

https://reviewboard.mozilla.org/r/142706/#review146376

::: dom/media/MediaDecoderStateMachine.cpp:1763
(Diff revision 1)
>  private:
>    MozPromiseRequestHolder<MediaDecoder::SeekPromise> mAccurateSeekRequest;
>    SeekJob mFutureSeekJob;
>  
> -  void Transit() override { // No transition after seek completed. }
> +  // We don't want to transition to DecodingState once this seek completes.
> +  void Transit() override { }

It can be when it is not part of the public interface which is commonly used in Template Method pattern.

We can discuss that somewhere else. Let's just keep the visibility consistent in this change.
Attachment #8871216 - Flags: review?(jwwang) → review+
Comment on attachment 8871217 [details]
Bug 1367701 P2 - transit to NextFrameSeekingState in Transit();

https://reviewboard.mozilla.org/r/142708/#review146380

::: dom/media/MediaDecoderStateMachine.cpp:1739
(Diff revision 1)
>    {
>      mFutureSeekJob = Move(aFutureSeekJob);
>  
>      AccurateSeekingState::Enter(Move(aCurrentSeekJob),
>                                  EventVisibility::Suppressed)
>        ->Then(OwnerThread(),

SGTM.
Attachment #8871217 - Flags: review?(jwwang) → review+
Comment on attachment 8871218 [details]
Bug 1367701 P3 - modify comments;

https://reviewboard.mozilla.org/r/142710/#review146382
Attachment #8871218 - Flags: review?(jwwang) → review+
Attachment #8871215 - Attachment is obsolete: true
Try looks good, thanks for the reveiw!
Pushed by tkuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b504b3373572
P1 - add a virtual Transit() method and remove SeekJob:mTransition; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c057839d5db2
P2 - transit to NextFrameSeekingState in Transit(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5850e5d245fb
P3 - modify comments; r=jwwang
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: