Closed Bug 1240411 Opened 8 years ago Closed 8 years ago

Cleanup dom/media header declarations

Categories

(Core :: Audio/Video, defect)

45 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(10 files)

Lots of redundant virtual keyword or missing override.
See Also: → 1206568
Remove redundant virtual keyword and add missing override if any.
Attachment #8708865 - Flags: review?(karlt)
Remove redundant virtual keyword and add missing override if any.
Attachment #8708866 - Flags: review?(rjesup)
Attachment #8708867 - Flags: review?(cpearce)
Remove redundant virtual keyword and add missing override if any.
Attachment #8708868 - Flags: review?(cpearce)
Remove redundant virtual keywords
Attachment #8708869 - Flags: review?(cpearce)
Remove redundant virtual keyword and add missing override if any.
Remove redundant virtual keyword and add missing override if any.
Attachment #8708873 - Flags: review?(giles)
Remove redundant virtual keyword and add missing override if any.
Attachment #8708874 - Flags: review?(cpearce)
Remove redundant virtual keyword and add missing override if any.
Attachment #8708875 - Flags: review?(jwwang)
Attachment #8708872 - Flags: review?(ayang)
Comment on attachment 8708875 [details] [diff] [review]
P9. Clean up media headers.

Review of attachment 8708875 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaManager.cpp
@@ +794,4 @@
>    AudioConfig(bool aEchoOn, uint32_t aEcho,
>                bool aAgcOn, uint32_t aAgc,
>                bool aNoiseOn, uint32_t aNoise,
>                int32_t aPlayoutDelay)

missing 'override'?

::: dom/media/MediaStreamGraph.cpp
@@ +1796,5 @@
>  
>    class Message : public ControlMessage {
>    public:
>      explicit Message(MediaStream* aStream) : ControlMessage(aStream) {}
> +    void Run()

missing 'override'?

@@ +1803,5 @@
>        auto graph = mStream->GraphImpl();
>        mStream->DestroyImpl();
>        graph->RemoveStreamGraphThread(mStream);
>      }
> +    void RunDuringShutdown()

missing 'override'?
Attachment #8708875 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #10)
> Comment on attachment 8708875 [details] [diff] [review]
> P9. Clean up media headers.
> 
> Review of attachment 8708875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaManager.cpp
> @@ +794,4 @@
> >    AudioConfig(bool aEchoOn, uint32_t aEcho,
> >                bool aAgcOn, uint32_t aAgc,
> >                bool aNoiseOn, uint32_t aNoise,
> >                int32_t aPlayoutDelay)
> 
> missing 'override'?

I can't find an AudioConfig method to override:
this is GetUserMediaCallbackMediaStreamListener that inherits from MediaStreamListener.

I'm not familiar with that part of the code though.

> 
> ::: dom/media/MediaStreamGraph.cpp
> @@ +1796,5 @@
> >  
> >    class Message : public ControlMessage {
> >    public:
> >      explicit Message(MediaStream* aStream) : ControlMessage(aStream) {}
> > +    void Run()
> 
> missing 'override'?

thanks ! I knew I had missed one after I had pressed "next find" too quickly, but couldn't find it after that :)
Attachment #8708872 - Flags: review?(ayang) → review+
Attachment #8708892 - Flags: review?(ayang)
Attachment #8708892 - Flags: review?(ayang) → review+
Attachment #8708867 - Flags: review?(cpearce) → review+
Attachment #8708868 - Flags: review?(cpearce) → review+
Attachment #8708869 - Flags: review?(cpearce) → review+
Attachment #8708874 - Flags: review?(cpearce) → review+
Attachment #8708873 - Flags: review?(giles) → review+
Comment on attachment 8708865 [details] [diff] [review]
P1. Clean up webaudio headers.

If Karl is away this week, could you look into doing it instead?

it's the same stuff as all the other patches.
Attachment #8708865 - Flags: review?(cpearce)
Attachment #8708865 - Flags: review?(cpearce) → review+
Attachment #8708866 - Flags: review?(rjesup) → review+
Attachment #8708865 - Flags: review?(karlt)
You need to log in before you can comment on or make changes to this bug.