888803 - Fix stagefright blocklisting to use case insensitive substring match

Status: RESOLVED INVALID

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

The Stagefright blocklist code uses case insensitive exact match in places whereas it should use case insensitive substring matching. Recent additions to the blocklist have done this using the Find method and we should convert all other uses to this.

Comment 1

[cajbir (:cajbir)]

Attachment: Change stagefright blocklist to use substring matching

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 769581 [details] [diff] [review]
Change stagefright blocklist to use substring matching

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

::: widget/android/GfxInfo.cpp
@@ +349,5 @@
>        NS_LossyConvertUTF16toASCII cManufacturer(mManufacturer);
>        NS_LossyConvertUTF16toASCII cModel(mModel);
>        NS_LossyConvertUTF16toASCII cHardware(mHardware);
>  
> +      if (cHardware.Find("antares", true) != -1 ||

This _was_ case-sensitive. The new code is case-insensitive. Is this intentional?

@@ +352,5 @@
>  
> +      if (cHardware.Find("antares", true) != -1 ||
> +          cHardware.Find("harmony", true) != -1 ||
> +          cHardware.Find("picasso", true) != -1 ||
> +          cHardware.Find("picasso_e", true) != -1 ||

picasso being a substring of picasso_e, this is now redundant.

So at this point I need to ask, are you sure that a sweeping change like this, making it match substrings in all blacklist rules, is what you want?

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I'm leaving for a 2-week vacation now, and I don't feel comfortable r+ing this at this point given my questions in comment 2. I suggest switching to Joe as reviewer here.

Comment 4

[cajbir (:cajbir)]

(In reply to Benoit Jacob [:bjacob] (On vacation, be back on july 18) from comment #2)
> So at this point I need to ask, are you sure that a sweeping change like
> this, making it match substrings in all blacklist rules, is what you want?

Thanks, on further thought I think you're right. We'll make changes as needed in future patches. Thanks for the sanity check!