Bug 30343 - [v8] typeof(HTMLMediaElement) should return undefined if media engine is not available
Summary: [v8] typeof(HTMLMediaElement) should return undefined if media engine is not ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hin-Chung Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-13 15:06 PDT by Hin-Chung Lam
Modified: 2009-10-19 17:00 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.80 KB, patch)
2009-10-13 16:27 PDT, Hin-Chung Lam
eric: review-
Details | Formatted Diff | Diff
patch (3.19 KB, patch)
2009-10-15 12:59 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch + manual test case (6.06 KB, patch)
2009-10-15 17:25 PDT, Hin-Chung Lam
dglazkov: review+
dglazkov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2009-10-13 15:06:26 PDT
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.
Comment 1 Hin-Chung Lam 2009-10-13 16:27:17 PDT
Created attachment 41138 [details]
Patch

This patch passes try servers on chromium side.
Comment 2 Eric Seidel (no email) 2009-10-15 12:02:23 PDT
Comment on attachment 41138 [details]
Patch

Every change requires a test:
"No new test because this is V8 specific." makes no sense.
Comment 3 Hin-Chung Lam 2009-10-15 12:59:01 PDT
Created attachment 41240 [details]
patch

Providing a manual test.
Comment 4 Eric Seidel (no email) 2009-10-15 13:44:30 PDT
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.
Comment 5 Dimitri Glazkov (Google) 2009-10-15 13:47:43 PDT
Didn't atwilson just do this?
Comment 6 Andrew Wilson 2009-10-15 13:59:40 PDT
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
Comment 7 Hin-Chung Lam 2009-10-15 14:02:01 PDT
Great, I'll also change the tests accordingly.
Comment 8 Hin-Chung Lam 2009-10-15 17:25:01 PDT
Created attachment 41254 [details]
Patch + manual test case
Comment 9 Hin-Chung Lam 2009-10-16 14:15:45 PDT
I've uploaded a new patch does similar things according to atwilson. Please take a look. Thanks.
Comment 10 Andrew Wilson 2009-10-16 15:37:13 PDT
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.
Comment 11 Dimitri Glazkov (Google) 2009-10-19 12:16:12 PDT
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.
Comment 12 Hin-Chung Lam 2009-10-19 17:00:03 PDT
Landed as http://trac.webkit.org/changeset/49803.