Fix a bug that hasVideo() always returns true even after we have the size info

Fix a bug that hasVideo() always returns true even after we have the size info

We should use the size info for hasVideo if we already have that information

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

[qinmin, 2012-11-29 20:19:28]

PTAL

[Miguel Garcia, 2012-11-29 20:28:02]

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:327: has_size_info_ = true;
shouldn't you check that width or height is > 0?

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.h:229: bool has_size_info_;
can you call it is_video_available_ or has video_size_ ? something that says
video on the name would be good.

[qinmin, 2012-11-29 20:39:33]

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:327: has_size_info_ = true;
No, we don't. This is called by the android media player, so if the size is (0,
0), then that means there is no video frames. And subsequent calls to hasVideo()
will return false. 
On the otherhand, if the size is > 0, than subsequence calls to hasVideo() will
return true.
If this is never called, we will use the previous MIME check scheme.

On 2012/11/29 20:28:02, Miguel Garcia wrote:
> shouldn't you check that width or height is > 0?

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.h:229: bool has_size_info_;
using is_video_available_ does not reflect the correct information before
OnVideoSizeChanged() is called. Before onVideoSizeChanged() gets first called,
we have to go through the whole hasVideo() code even if is_video_available is
true. Then this variable does not really reflect the correct information we
have.

On 2012/11/29 20:28:02, Miguel Garcia wrote:
> can you call it is_video_available_ or has video_size_ ? something that says
> video on the name would be good.

[Miguel Garcia, 2012-11-29 20:43:36]

lgtm

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:327: has_size_info_ = true;
OK, makes sense


On 2012/11/29 20:39:33, qinmin wrote:
> No, we don't. This is called by the android media player, so if the size is
(0,
> 0), then that means there is no video frames. And subsequent calls to
hasVideo()
> will return false. 
> On the otherhand, if the size is > 0, than subsequence calls to hasVideo()
will
> return true.
> If this is never called, we will use the previous MIME check scheme.
> 
> On 2012/11/29 20:28:02, Miguel Garcia wrote:
> > shouldn't you check that width or height is > 0?
>

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/11412251/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.h:229: bool has_size_info_;
ok

On 2012/11/29 20:39:33, qinmin wrote:
> using is_video_available_ does not reflect the correct information before
> OnVideoSizeChanged() is called. Before onVideoSizeChanged() gets first called,
> we have to go through the whole hasVideo() code even if is_video_available is
> true. Then this variable does not really reflect the correct information we
> have.
> 
> On 2012/11/29 20:28:02, Miguel Garcia wrote:
> > can you call it is_video_available_ or has video_size_ ? something that says
> > video on the name would be good.
>

[Miguel Garcia, 2012-11-29 20:43:38]

lgtm

[commit-bot: I haz the power, 2012-11-29 20:45:27]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[scherkus (not reviewing), 2012-11-29 20:49:28]

lgtm

[commit-bot: I haz the power, 2012-11-30 09:58:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/11412251/1

[Miguel Garcia, 2012-11-30 12:10:00]

On 2012/11/30 09:58:21, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/qinmin%40chromium.org/11412251/1

I sent this to the CQ so it can be gardened during the MV shift with some luck.
Hope it's ok.

[commit-bot: I haz the power, 2012-11-30 12:37:32]

Change committed as 170440