Summary: | [v8] typeof(HTMLMediaElement) should return undefined if media engine is not available | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hin-Chung Lam <hclam> | ||||||||
Component: | WebCore JavaScript | Assignee: | Hin-Chung Lam <hclam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, atwilson, dglazkov, eric.carlson, eric, levin | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Hin-Chung Lam
2009-10-13 15:06:26 PDT
Created attachment 41138 [details]
Patch
This patch passes try servers on chromium side.
Comment on attachment 41138 [details]
Patch
Every change requires a test:
"No new test because this is V8 specific." makes no sense.
Created attachment 41240 [details]
patch
Providing a manual test.
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. Didn't atwilson just do this? 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 Great, I'll also change the tests accordingly. Created attachment 41254 [details]
Patch + manual test case
I've uploaded a new patch does similar things according to atwilson. Please take a look. Thanks. 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 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. Landed as http://trac.webkit.org/changeset/49803. |