Closed Bug 1314526 Opened 3 years ago Closed 3 years ago

Return void for some StateObject::HandleXXX() functions

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(6 files)

Their return values are never used.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8807006 - Flags: review?(kikuo)
Attachment #8807007 - Flags: review?(kikuo)
Attachment #8807008 - Flags: review?(kikuo)
Attachment #8807009 - Flags: review?(kikuo)
Attachment #8807010 - Flags: review?(kikuo)
Attachment #8807011 - Flags: review?(kikuo)
Comment on attachment 8807006 [details]
Bug 1314526. Part 1 - fix HandleCDMProxyReady().

https://reviewboard.mozilla.org/r/90256/#review89976
Attachment #8807006 - Flags: review?(kikuo) → review+
Comment on attachment 8807007 [details]
Bug 1314526. Part 2 - fix HandleAudioDecoded().

https://reviewboard.mozilla.org/r/90258/#review89982

::: dom/media/MediaDecoderStateMachine.cpp:853
(Diff revision 1)
> -  bool HandleAudioDecoded(MediaData* aAudio) override
> +  void HandleAudioDecoded(MediaData* aAudio) override
>    {
>      MOZ_ASSERT(false);
> -    return true;
>    }

A thought just came up to my mind.
Maybe we could assert(false) for these functions by default in the base StateObject, so that it would become very clear that what should/should not happen in each derived state.

Not sure if this is practical, kinda aggressive.
Attachment #8807007 - Flags: review?(kikuo) → review+
Comment on attachment 8807008 [details]
Bug 1314526. Part 3 - fix HandleVideoDecoded()

https://reviewboard.mozilla.org/r/90260/#review89984
Attachment #8807008 - Flags: review?(kikuo) → review+
Comment on attachment 8807007 [details]
Bug 1314526. Part 2 - fix HandleAudioDecoded().

https://reviewboard.mozilla.org/r/90258/#review89982

> A thought just came up to my mind.
> Maybe we could assert(false) for these functions by default in the base StateObject, so that it would become very clear that what should/should not happen in each derived state.
> 
> Not sure if this is practical, kinda aggressive.

SGTM. Such assertions will improve the invariants like we should never receive audio/video samples during shutdown or decoding metadata. We can do that in follow-ups.
Comment on attachment 8807009 [details]
Bug 1314526. Part 4 - fix HandleEndOfStream().

https://reviewboard.mozilla.org/r/90262/#review90016
Attachment #8807009 - Flags: review?(kikuo) → review+
Comment on attachment 8807010 [details]
Bug 1314526. Part 5 - fix HandleWaitingForData().

https://reviewboard.mozilla.org/r/90264/#review90018
Attachment #8807010 - Flags: review?(kikuo) → review+
Comment on attachment 8807011 [details]
Bug 1314526. Part 6 - fix HandleAudioCaptured().

https://reviewboard.mozilla.org/r/90266/#review90020
Attachment #8807011 - Flags: review?(kikuo) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d5799848acf
Part 1 - fix HandleCDMProxyReady(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/30ab9b36441b
Part 2 - fix HandleAudioDecoded(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/214d895597a0
Part 3 - fix HandleVideoDecoded() r=kikuo
https://hg.mozilla.org/integration/autoland/rev/5b6b44bd0495
Part 4 - fix HandleEndOfStream(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/8bda14acdc22
Part 5 - fix HandleWaitingForData(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/dbd323ea0320
Part 6 - fix HandleAudioCaptured(). r=kikuo
You need to log in before you can comment on or make changes to this bug.