Closed Bug 1418868 Opened 2 years ago Closed 2 years ago

Remove IndiceWrapper abstract class

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file)

Since stagefright is removed, we don't need an abstract layer anymore.
Comment on attachment 8929957 [details]
Bug 1418868 - remove IndiceWrapper abstract layer.

https://reviewboard.mozilla.org/r/201116/#review206244

::: dom/media/mp4/MP4Metadata.h:20
(Diff revision 1)
>  #include "ByteStream.h"
>  #include "mp4parse.h"
>  
>  namespace mozilla {
>  
> -class IndiceWrapper {
> +// the owner of mIndice is rust mp4 paser, so lifetime of this class

the -> The

paser -> parser

::: dom/media/mp4/MP4Metadata.h:21
(Diff revision 1)
>  #include "mp4parse.h"
>  
>  namespace mozilla {
>  
> -class IndiceWrapper {
> +// the owner of mIndice is rust mp4 paser, so lifetime of this class
> +// SHOULD NOT longer than rust parser.

This comment doesn't seem right.  mUnique is a UniquePtr, which is owning, and it's allocated inside IndiceWrapper's ctor and the members of aRustIndice are copied over.  So there's no connection between an IndiceWrapper and mp4 parser's lifetime, right?

It seems like now we could make mIndice be a direct member rather than allocating it, too.
Attachment #8929957 - Flags: review?(kinetik) → review-
(In reply to Matthew Gregan [:kinetik] from comment #2)
> > -class IndiceWrapper {
> > +// the owner of mIndice is rust mp4 paser, so lifetime of this class
> > +// SHOULD NOT longer than rust parser.
> 
> This comment doesn't seem right.  mUnique is a UniquePtr, which is owning,
> and it's allocated inside IndiceWrapper's ctor and the members of
> aRustIndice are copied over.  So there's no connection between an
> IndiceWrapper and mp4 parser's lifetime, right?

Yes, the memory owned by mp4 parser is the raw data from rust Vec. I'll update it.

> 
> It seems like now we could make mIndice be a direct member rather than
> allocating it, too.

Will do.
Comment on attachment 8929957 [details]
Bug 1418868 - remove IndiceWrapper abstract layer.

https://reviewboard.mozilla.org/r/201116/#review206824
Attachment #8929957 - Flags: review?(kinetik) → review+
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05e3e4ff97a5
remove IndiceWrapper abstract layer. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/05e3e4ff97a5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.