Fix- WebMediaPlayerAndroid::hasAudio() to query status of audio and return actual value

getting status of audio and returning value, avoiding false negative

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243900

[amogh.bihani, 2013-12-23 10:56:55]

Kindly review.

[vivekg, 2013-12-23 11:51:11]

https://codereview.chromium.org/120693002/diff/30001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/120693002/diff/30001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:449: return
mime.find("audio/") == std::string::npos;
This change looks suspicious to me! We need to check for the audio mime type and
this check is opposite to that.

[vivekg, 2013-12-23 12:00:21]

https://codereview.chromium.org/120693002/diff/30001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/120693002/diff/30001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:439: return
mime.find("audio/") == std::string::npos;
Would it make sense to check for specific video mime-types specified in [1]/[2]
instead of checking for "audio" not being present? 

[1] http://en.wikipedia.org/wiki/Internet_media_type
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util...

Can we have some sort of testing done for these features? Either a unit test or
layout test may be!... I am not sure about these though! :)

[amogh.bihani, 2014-01-06 14:38:34]

Ping!

[qinmin, 2014-01-06 18:23:49]

not lgtm

https://codereview.chromium.org/120693002/diff/80001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/120693002/diff/80001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:449: return
mime.find("audio/") != std::string::npos;
I don't think this is right.
If the mime is video/xxx, this will always return false, which is wrong as the
video might have audio stream in it

[amogh.bihani, 2014-01-07 09:17:41]

Thanks for the review. :)

I have now considered 'video/*' and 'application/ogg' as well for audio.

[qinmin, 2014-01-08 20:09:20]

lgtm

https://codereview.chromium.org/120693002/diff/130001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/120693002/diff/130001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:452:
mime.find("application/ogg") != std::string::npos)
nit: {} is needed for if statement that spans multiple lines

[commit-bot: I haz the power, 2014-01-09 03:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/120693002/180001

[amogh.bihani, 2014-01-09 03:42:36]

Thanks for the review :)

[amogh.bihani, 2014-01-09 03:43:41]

https://codereview.chromium.org/120693002/diff/130001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/120693002/diff/130001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:452:
mime.find("application/ogg") != std::string::npos)
On 2014/01/08 20:09:20, qinmin wrote:
> nit: {} is needed for if statement that spans multiple lines

Done.

[commit-bot: I haz the power, 2014-01-09 05:54:31]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-09 11:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/120693002/180001

[commit-bot: I haz the power, 2014-01-09 14:45:55]

Change committed as 243900