Implemented rest of webkit api/glue code needed for HTML5 media canPlayType().

Implemented rest of webkit api/glue code needed for HTML5 media canPlayType().

BUG=16636
TEST=we should respect the codecs= parameter when provided as a media mime type

[scherkus (not reviewing), 2009-07-24 00:31:59]

Continuation of http://codereview.chromium.org/159293/

I'm unsure about having a bit of logic in the glue code, unless we move it all
over to net:: and have another tri-state enum

[darin (slow to review), 2009-07-24 16:10:19]

http://codereview.chromium.org/160073/diff/17/19
File net/base/mime_util.h (right):

http://codereview.chromium.org/160073/diff/17/19#newcode49
Line 49: // Returns true if and only if all codecs are supported by this
browser.
nit: please don't mention browser here.  net/base is trying to not be browser
specific.  (this file probably belongs in chrome/ somewhere, but that's a change
for another day!)

http://codereview.chromium.org/160073/diff/17/20
File webkit/api/public/WebMimeRegistry.h (right):

http://codereview.chromium.org/160073/diff/17/20#newcode45
Line 45: virtual SupportsType supportsMediaMIMEType(const WebString& mimeType,
I'm very tempted to say that we should have each of these functions return
SupportsType just for API consistency.  WDYT?

Also, why do we call it ProbablySupported and not Supported?  It seems like we
only need the states: yes, no, and maybe.

How about we go with enum names that exactly match the ones defined in WebCore: 
IsNotSupported, IsSupported, and MayBeSupported (in that order)?

[darin (slow to review), 2009-07-24 16:10:36]

> I'm unsure about having a bit of logic in the glue code, unless we move it all
> over to net:: and have another tri-state enum

What you've done seems good to me.

[scherkus (not reviewing), 2009-07-24 17:05:04]

http://codereview.chromium.org/160073/diff/17/19
File net/base/mime_util.h (right):

http://codereview.chromium.org/160073/diff/17/19#newcode49
Line 49: // Returns true if and only if all codecs are supported by this
browser.
On 2009/07/24 16:10:19, darin wrote:
> nit: please don't mention browser here.  net/base is trying to not be browser
> specific.  (this file probably belongs in chrome/ somewhere, but that's a
change
> for another day!)

Done.

http://codereview.chromium.org/160073/diff/17/20
File webkit/api/public/WebMimeRegistry.h (right):

http://codereview.chromium.org/160073/diff/17/20#newcode45
Line 45: virtual SupportsType supportsMediaMIMEType(const WebString& mimeType,
On 2009/07/24 16:10:19, darin wrote:
> I'm very tempted to say that we should have each of these functions return
> SupportsType just for API consistency.  WDYT?

canPlayType() is actually a three-state variable that gets passed all the way
back to javascript

I'm not sure how many callers there are, but my biggest worry would be different
callers treating MayBeSupported differently or flat-out ignoring it.

So we'd end up with:
  - SimpleWebMimeRegistryImpl::supportsImageMIMEType() converts
net::IsSupportedImageMimeType() from true/false to IsSupported/IsNotSupported
  - ChromiumBridge::isSupportedImageMIMEType() converts
IsSupported/IsNotSupported back to true/false

Would that be ok?

> 
> Also, why do we call it ProbablySupported and not Supported?  It seems like we
> only need the states: yes, no, and maybe.
> 
> How about we go with enum names that exactly match the ones defined in
WebCore: 
> IsNotSupported, IsSupported, and MayBeSupported (in that order)?

we named ours after the spec:
http://www.whatwg.org/specs/web-apps/current-work/#dom-navigator-canplaytype

but keeping the same as webkit should make it easier to merge should they ever
rename them

[darin (slow to review), 2009-07-24 17:08:58]

On Fri, Jul 24, 2009 at 10:05 AM, <scherkus@chromium.org> wrote:

>
> http://codereview.chromium.org/160073/diff/17/19
> File net/base/mime_util.h (right):
>
> http://codereview.chromium.org/160073/diff/17/19#newcode49
> Line 49: // Returns true if and only if all codecs are supported by this
> browser.
> On 2009/07/24 16:10:19, darin wrote:
>
>> nit: please don't mention browser here.  net/base is trying to not be
>>
> browser
>
>> specific.  (this file probably belongs in chrome/ somewhere, but
>>
> that's a change
>
>> for another day!)
>>
>
> Done.
>
> http://codereview.chromium.org/160073/diff/17/20
> File webkit/api/public/WebMimeRegistry.h (right):
>
> http://codereview.chromium.org/160073/diff/17/20#newcode45
> Line 45: virtual SupportsType supportsMediaMIMEType(const WebString&
> mimeType,
> On 2009/07/24 16:10:19, darin wrote:
>
>> I'm very tempted to say that we should have each of these functions
>>
> return
>
>> SupportsType just for API consistency.  WDYT?
>>
>
> canPlayType() is actually a three-state variable that gets passed all
> the way back to javascript
>
> I'm not sure how many callers there are, but my biggest worry would be
> different callers treating MayBeSupported differently or flat-out
> ignoring it.
>
> So we'd end up with:
>  - SimpleWebMimeRegistryImpl::supportsImageMIMEType() converts
> net::IsSupportedImageMimeType() from true/false to
> IsSupported/IsNotSupported
>  - ChromiumBridge::isSupportedImageMIMEType() converts
> IsSupported/IsNotSupported back to true/false
>
> Would that be ok?


Yes, I think so.  I care more about making the stable WebKit API interface
clean.



>
>
>
>  Also, why do we call it ProbablySupported and not Supported?  It seems
>>
> like we
>
>> only need the states: yes, no, and maybe.
>>
>
>  How about we go with enum names that exactly match the ones defined in
>>
> WebCore:
>
>> IsNotSupported, IsSupported, and MayBeSupported (in that order)?
>>
>
> we named ours after the spec:
>
> http://www.whatwg.org/specs/web-apps/current-work/#dom-navigator-canplaytype
>
> but keeping the same as webkit should make it easier to merge should
> they ever rename them
>


I see... yeah, let's stay consistent with the enum values defined in
WebCore.

-Darin

[kylep, 2009-07-24 17:31:06]

I changed the order because WebCore's makes no sense to me. It's like they
had IsNot- and Is-, then later decided crap we need MayBe-. It makes more
sense to me to order them in increasing or decreasing certitude, i.e.
IsNot-, Maybe-, Is- Supported.

On Fri, Jul 24, 2009 at 10:08 AM, Darin Fisher <darin@chromium.org> wrote:

> On Fri, Jul 24, 2009 at 10:05 AM, <scherkus@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/160073/diff/17/19
>> File net/base/mime_util.h (right):
>>
>> http://codereview.chromium.org/160073/diff/17/19#newcode49
>> Line 49: // Returns true if and only if all codecs are supported by this
>> browser.
>> On 2009/07/24 16:10:19, darin wrote:
>>
>>> nit: please don't mention browser here.  net/base is trying to not be
>>>
>> browser
>>
>>> specific.  (this file probably belongs in chrome/ somewhere, but
>>>
>> that's a change
>>
>>> for another day!)
>>>
>>
>> Done.
>>
>> http://codereview.chromium.org/160073/diff/17/20
>> File webkit/api/public/WebMimeRegistry.h (right):
>>
>> http://codereview.chromium.org/160073/diff/17/20#newcode45
>> Line 45: virtual SupportsType supportsMediaMIMEType(const WebString&
>> mimeType,
>> On 2009/07/24 16:10:19, darin wrote:
>>
>>> I'm very tempted to say that we should have each of these functions
>>>
>> return
>>
>>> SupportsType just for API consistency.  WDYT?
>>>
>>
>> canPlayType() is actually a three-state variable that gets passed all
>> the way back to javascript
>>
>> I'm not sure how many callers there are, but my biggest worry would be
>> different callers treating MayBeSupported differently or flat-out
>> ignoring it.
>>
>> So we'd end up with:
>>  - SimpleWebMimeRegistryImpl::supportsImageMIMEType() converts
>> net::IsSupportedImageMimeType() from true/false to
>> IsSupported/IsNotSupported
>>  - ChromiumBridge::isSupportedImageMIMEType() converts
>> IsSupported/IsNotSupported back to true/false
>>
>> Would that be ok?
>
>
> Yes, I think so.  I care more about making the stable WebKit API interface
> clean.
>
>
>
>>
>>
>>
>>  Also, why do we call it ProbablySupported and not Supported?  It seems
>>>
>> like we
>>
>>> only need the states: yes, no, and maybe.
>>>
>>
>>  How about we go with enum names that exactly match the ones defined in
>>>
>> WebCore:
>>
>>> IsNotSupported, IsSupported, and MayBeSupported (in that order)?
>>>
>>
>> we named ours after the spec:
>>
>> http://www.whatwg.org/specs/web-apps/current-work/#dom-navigator-canplaytype
>>
>> but keeping the same as webkit should make it easier to merge should
>> they ever rename them
>>
>
>
> I see... yeah, let's stay consistent with the enum values defined in
> WebCore.
>
> -Darin
>

[fbarchard, 2009-07-24 18:51:14]

LGTM with just notes/nits that we can look into later.

http://codereview.chromium.org/160073/diff/1027/1028
File net/base/mime_util.cc (right):

http://codereview.chromium.org/160073/diff/1027/1028#newcode216
Line 216: "mp4a",
note that the ftyp atom of mp4 contains a list of types.  I'm not sure if they
are used to match up with mimetypes.
for example mobile.mp4 starts with
00000000- 00 00 00 18  66 74 79 70  61 76 63 31  00 00 00 00 [....ftypavc1....]
but it seems like this list should be longer?  like include mp3, and other
codecs from this list:
http://wiki.whatwg.org/wiki/Video_type_parameters#Browser_Support

00000001- 69 73 6F 6D  61 76 63 31  00 22 4F 44  6D 64 61 74 [isomavc1."ODmdat]

http://codereview.chromium.org/160073/diff/1027/1028#newcode367
Line 367: for (size_t i = 0; i < codecs.size(); ++i) {
performance nit:
size_t is great for 64 bit apps where you want huge memory buffers but its less
efficient/overkill than int, and int lends itself to openmp, whereas size_t
doesnt.

[scherkus (not reviewing), 2009-07-24 19:12:12]

we've got some seriously long function names now, but c++ guide says "return
type + function name are always on the same line" so I guess we'll deal with it
:(

[scherkus (not reviewing), 2009-07-24 19:17:25]

On 2009/07/24 18:51:14, fbarchard wrote:
> LGTM with just notes/nits that we can look into later.
> 
> http://codereview.chromium.org/160073/diff/1027/1028
> File net/base/mime_util.cc (right):
> 
> http://codereview.chromium.org/160073/diff/1027/1028#newcode216
> Line 216: "mp4a",
> note that the ftyp atom of mp4 contains a list of types.  I'm not sure if they
> are used to match up with mimetypes.
> for example mobile.mp4 starts with
> 00000000- 00 00 00 18  66 74 79 70  61 76 63 31  00 00 00 00
[....ftypavc1....]
> but it seems like this list should be longer?  like include mp3, and other
> codecs from this list:
> http://wiki.whatwg.org/wiki/Video_type_parameters#Browser_Support

I thought so too, but according to that list mp3 doesn't have an official MIME
codec type registered.  We can't include the other ones (3gpp, dirac, speex,
etc...) because then we'd be lying :)

> http://codereview.chromium.org/160073/diff/1027/1028#newcode367
> Line 367: for (size_t i = 0; i < codecs.size(); ++i) {
> performance nit:
> size_t is great for 64 bit apps where you want huge memory buffers but its
less
> efficient/overkill than int, and int lends itself to openmp, whereas size_t
> doesnt.

[darin (slow to review), 2009-07-24 19:25:53]

Maybe it would be good to send a patch to WebKit to fix that?
-Darin


On Fri, Jul 24, 2009 at 10:30 AM, Kyle Prete <kylep@chromium.org> wrote:

> I changed the order because WebCore's makes no sense to me. It's like they
> had IsNot- and Is-, then later decided crap we need MayBe-. It makes more
> sense to me to order them in increasing or decreasing certitude, i.e.
> IsNot-, Maybe-, Is- Supported.
>
>
> On Fri, Jul 24, 2009 at 10:08 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> On Fri, Jul 24, 2009 at 10:05 AM, <scherkus@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.org/160073/diff/17/19
>>> File net/base/mime_util.h (right):
>>>
>>> http://codereview.chromium.org/160073/diff/17/19#newcode49
>>> Line 49: // Returns true if and only if all codecs are supported by this
>>> browser.
>>> On 2009/07/24 16:10:19, darin wrote:
>>>
>>>> nit: please don't mention browser here.  net/base is trying to not be
>>>>
>>> browser
>>>
>>>> specific.  (this file probably belongs in chrome/ somewhere, but
>>>>
>>> that's a change
>>>
>>>> for another day!)
>>>>
>>>
>>> Done.
>>>
>>> http://codereview.chromium.org/160073/diff/17/20
>>> File webkit/api/public/WebMimeRegistry.h (right):
>>>
>>> http://codereview.chromium.org/160073/diff/17/20#newcode45
>>> Line 45: virtual SupportsType supportsMediaMIMEType(const WebString&
>>> mimeType,
>>> On 2009/07/24 16:10:19, darin wrote:
>>>
>>>> I'm very tempted to say that we should have each of these functions
>>>>
>>> return
>>>
>>>> SupportsType just for API consistency.  WDYT?
>>>>
>>>
>>> canPlayType() is actually a three-state variable that gets passed all
>>> the way back to javascript
>>>
>>> I'm not sure how many callers there are, but my biggest worry would be
>>> different callers treating MayBeSupported differently or flat-out
>>> ignoring it.
>>>
>>> So we'd end up with:
>>>  - SimpleWebMimeRegistryImpl::supportsImageMIMEType() converts
>>> net::IsSupportedImageMimeType() from true/false to
>>> IsSupported/IsNotSupported
>>>  - ChromiumBridge::isSupportedImageMIMEType() converts
>>> IsSupported/IsNotSupported back to true/false
>>>
>>> Would that be ok?
>>
>>
>> Yes, I think so.  I care more about making the stable WebKit API interface
>> clean.
>>
>>
>>
>>>
>>>
>>>
>>>  Also, why do we call it ProbablySupported and not Supported?  It seems
>>>>
>>> like we
>>>
>>>> only need the states: yes, no, and maybe.
>>>>
>>>
>>>  How about we go with enum names that exactly match the ones defined in
>>>>
>>> WebCore:
>>>
>>>> IsNotSupported, IsSupported, and MayBeSupported (in that order)?
>>>>
>>>
>>> we named ours after the spec:
>>>
>>> http://www.whatwg.org/specs/web-apps/current-work/#dom-navigator-canplaytype
>>>
>>> but keeping the same as webkit should make it easier to merge should
>>> they ever rename them
>>>
>>
>>
>> I see... yeah, let's stay consistent with the enum values defined in
>> WebCore.
>>
>> -Darin
>>
>
>

[scherkus (not reviewing), 2009-07-24 20:44:03]

On 2009/07/24 19:25:53, darin wrote:
> Maybe it would be good to send a patch to WebKit to fix that?
> -Darin

Yeah it would be nice to use HTML5 keywords throughout the entire chain of
calls.  MayBe Kyle can investigate :P

> On Fri, Jul 24, 2009 at 10:30 AM, Kyle Prete <kylep@chromium.org> wrote:
> 
> > I changed the order because WebCore's makes no sense to me. It's like they
> > had IsNot- and Is-, then later decided crap we need MayBe-. It makes more
> > sense to me to order them in increasing or decreasing certitude, i.e.
> > IsNot-, Maybe-, Is- Supported.
> >
> >
> > On Fri, Jul 24, 2009 at 10:08 AM, Darin Fisher <darin@chromium.org> wrote:
> >
> >> On Fri, Jul 24, 2009 at 10:05 AM, <scherkus@chromium.org> wrote:
> >>
> >>>
> >>> http://codereview.chromium.org/160073/diff/17/19
> >>> File net/base/mime_util.h (right):
> >>>
> >>> http://codereview.chromium.org/160073/diff/17/19#newcode49
> >>> Line 49: // Returns true if and only if all codecs are supported by this
> >>> browser.
> >>> On 2009/07/24 16:10:19, darin wrote:
> >>>
> >>>> nit: please don't mention browser here.  net/base is trying to not be
> >>>>
> >>> browser
> >>>
> >>>> specific.  (this file probably belongs in chrome/ somewhere, but
> >>>>
> >>> that's a change
> >>>
> >>>> for another day!)
> >>>>
> >>>
> >>> Done.
> >>>
> >>> http://codereview.chromium.org/160073/diff/17/20
> >>> File webkit/api/public/WebMimeRegistry.h (right):
> >>>
> >>> http://codereview.chromium.org/160073/diff/17/20#newcode45
> >>> Line 45: virtual SupportsType supportsMediaMIMEType(const WebString&
> >>> mimeType,
> >>> On 2009/07/24 16:10:19, darin wrote:
> >>>
> >>>> I'm very tempted to say that we should have each of these functions
> >>>>
> >>> return
> >>>
> >>>> SupportsType just for API consistency.  WDYT?
> >>>>
> >>>
> >>> canPlayType() is actually a three-state variable that gets passed all
> >>> the way back to javascript
> >>>
> >>> I'm not sure how many callers there are, but my biggest worry would be
> >>> different callers treating MayBeSupported differently or flat-out
> >>> ignoring it.
> >>>
> >>> So we'd end up with:
> >>>  - SimpleWebMimeRegistryImpl::supportsImageMIMEType() converts
> >>> net::IsSupportedImageMimeType() from true/false to
> >>> IsSupported/IsNotSupported
> >>>  - ChromiumBridge::isSupportedImageMIMEType() converts
> >>> IsSupported/IsNotSupported back to true/false
> >>>
> >>> Would that be ok?
> >>
> >>
> >> Yes, I think so.  I care more about making the stable WebKit API interface
> >> clean.
> >>
> >>
> >>
> >>>
> >>>
> >>>
> >>>  Also, why do we call it ProbablySupported and not Supported?  It seems
> >>>>
> >>> like we
> >>>
> >>>> only need the states: yes, no, and maybe.
> >>>>
> >>>
> >>>  How about we go with enum names that exactly match the ones defined in
> >>>>
> >>> WebCore:
> >>>
> >>>> IsNotSupported, IsSupported, and MayBeSupported (in that order)?
> >>>>
> >>>
> >>> we named ours after the spec:
> >>>
> >>>
http://www.whatwg.org/specs/web-apps/current-work/#dom-navigator-canplaytype
> >>>
> >>> but keeping the same as webkit should make it easier to merge should
> >>> they ever rename them
> >>>
> >>
> >>
> >> I see... yeah, let's stay consistent with the enum values defined in
> >> WebCore.
> >>
> >> -Darin
> >>
> >
> >
>

[darin (slow to review), 2009-07-25 00:26:24]

LGTM