Refactor GMP info file parsing into helper class

RESOLVED FIXED in Firefox 47

Status

()

Core
Audio/Video: GMP
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Dealing with GMP info files would be easier if we factored the parsing of the info file and storing parsed results into a helper class. Then we'd not need to re-parse the entire file again when we want to read one field from the .info file.
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Comment 1

2 years ago
Created attachment 8717134 [details] [diff] [review]
Refactor GMP info file parsing into helper class
Attachment #8717134 - Flags: review?(gsquelart)
Comment on attachment 8717134 [details] [diff] [review]
Refactor GMP info file parsing into helper class

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

r+ with question:

::: dom/media/gmp/GMPChild.cpp
@@ +325,5 @@
>      return false;
>    }
>  
> +  nsTArray<nsCString> libs;
> +  SplitAt(", ", mInfoParser.Get(NS_LITERAL_CSTRING("libraries")), libs);

You're now splitting the "libraries" list at spaces (not just commas) -- Is that really what you intend?
Attachment #8717134 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 3

2 years ago
(In reply to Gerald Squelart [:gerald] from comment #2)
> Comment on attachment 8717134 [details] [diff] [review]
> Refactor GMP info file parsing into helper class
> 
> Review of attachment 8717134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with question:
> 
> ::: dom/media/gmp/GMPChild.cpp
> @@ +325,5 @@
> >      return false;
> >    }
> >  
> > +  nsTArray<nsCString> libs;
> > +  SplitAt(", ", mInfoParser.Get(NS_LITERAL_CSTRING("libraries")), libs);
> 
> You're now splitting the "libraries" list at spaces (not just commas) -- Is
> that really what you intend?

Yes, that's intentional. This enabled me to remove the lib.Trim(" ") call.
(In reply to Chris Pearce (:cpearce) from comment #3)
> (In reply to Gerald Squelart [:gerald] from comment #2)
> > > +  SplitAt(", ", mInfoParser.Get(NS_LITERAL_CSTRING("libraries")), libs);
> > You're now splitting the "libraries" list at spaces (not just commas) -- Is
> > that really what you intend?
> 
> Yes, that's intentional. This enabled me to remove the lib.Trim(" ") call.

I see. Hopefully there are no libraries with spaces in their name!(?)

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f00cf14ee294
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.