Bug 30343 – [v8] typeof(HTMLMediaElement) should return undefined if media engine is not available

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Hin-Chung Lam]

In Chromium:

1. Remove all ffmpeg library files
2. Run Chromium
3. Open a page and does typeof(HTMLMediaElement), it should return undefined, but it returns "function" instead.

In the v8 binding we should check if the media engine and intercept such calls and returns undefined.

[Hin-Chung Lam]

Created attachment 41138 [details]
Patch

This patch passes try servers on chromium side.

[Eric Seidel (no email)]

Comment on attachment 41138 [details]
Patch

Every change requires a test:
"No new test because this is V8 specific." makes no sense.

[Hin-Chung Lam]

Created attachment 41240 [details]
patch

Providing a manual test.

[Eric Seidel (no email)]

OK.  I misunderstood how one would reproduce this.  I guess I didn't read the bug close enough.  Yes, I think it's better to have the manual test, but I now understand why you didn't have a test initially.

It would also of course be possible to override MediaPlayer::isAvailable() via some layoutTestController method, but a manual test is OK in this case.

I think a v8-enabled reviewer should review this though.

[Dimitri Glazkov (Google)]

Didn't atwilson just do this?

[Andrew Wilson]

I changed window.Audio, and did nothing for HTMLMediaElement or any other constructors, as I was not aware that this was the desired behavior.

Just returning undefined for the constructor is incorrect - you should instead follow the example for window.Audio, which actually removes the associated attribute from the window object.

Case in point: with the patch attached to this bug, "typeof HTMLMediaElement === 'undefined'", but "'HTMLMediaElement' in window" would still return true. If you follow the approach I used for window.Audio, then you'd get the correct behavior in both cases.

Take a look here:

https://bugs.webkit.org/show_bug.cgi?id=30240

[Hin-Chung Lam]

Great, I'll also change the tests accordingly.

[Hin-Chung Lam]

Created attachment 41254 [details]
Patch + manual test case

[Hin-Chung Lam]

I've uploaded a new patch does similar things according to atwilson. Please take a look. Thanks.

[Andrew Wilson]

This looks fine to me (although someone with reviewer bit needs to r+ it) - only comment is that ideally the test would include testing that ('HTMLMediaElement' in window) == false.

[Dimitri Glazkov (Google)]

Comment on attachment 41254 [details]
Patch + manual test case

> +        attribute [Conditional=VIDEO,EnabledAtRuntime] HTMLAudioElementConstructor HTMLAudioElement;
> +        attribute [Conditional=VIDEO,EnabledAtRuntime] HTMLMediaElementConstructor HTMLMediaElement;
> +        attribute [Conditional=VIDEO,EnabledAtRuntime] HTMLVideoElementConstructor HTMLVideoElement;
> +        attribute [Conditional=VIDEO,EnabledAtRuntime] MediaErrorConstructor MediaError;

Space between IDL attributes would be nice for readability. You can fix upon landing.

[Hin-Chung Lam]

Landed as http://trac.webkit.org/changeset/49803.