Closed
Bug 454683
Opened 16 years ago
Closed 16 years ago
Media element setVolume should fail on invalid values
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b1
People
(Reporter: cpearce, Assigned: cpearce)
References
()
Details
Attachments
(1 file, 4 obsolete files)
1.53 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
According to the spec (see url), media element should only accept volume values in the range [0.0, 1.0], if a value outside of that range is entered, an INDEX_SIZE_ERR exception should be raised. Currently the nsHTMLMediaElement::SetAttr() doesn't check the values to see if they're in the specified range, it should, and if they're invalid, it should fail without attempting to change the volume.
Assignee | ||
Comment 1•16 years ago
|
||
I mean nsHTMLMediaElement::SetVolume() should validate the volume, not nsHTMLMediaElement::SetAttr().
Assignee | ||
Comment 2•16 years ago
|
||
Assignee: nobody → chris
Attachment #337986 -
Flags: superreview?(roc)
Attachment #337986 -
Flags: review?(chris.double)
Can you test the actual exception value raised?
Assignee | ||
Comment 4•16 years ago
|
||
Mochitest checks for NS_ERROR_DOM_INDEX_SIZE_ERR specifically when setting an invalid volume.
Attachment #337986 -
Attachment is obsolete: true
Attachment #340447 -
Flags: superreview?(roc)
Attachment #340447 -
Flags: review?(chris.double)
Attachment #337986 -
Flags: superreview?(roc)
Attachment #337986 -
Flags: review?(chris.double)
Assignee | ||
Updated•16 years ago
|
Attachment #340447 -
Attachment is obsolete: true
Attachment #340447 -
Flags: superreview?(roc)
Attachment #340447 -
Flags: review?(chris.double)
Assignee | ||
Comment 5•16 years ago
|
||
Comment on attachment 340447 [details] [diff] [review] Patch v2 - with check for INDEX_SIZE_ERR in mochitest Oops, didn't add mochitest before qrefresh...
Assignee | ||
Comment 6•16 years ago
|
||
Mochitest checks for NS_ERROR_DOM_INDEX_SIZE_ERR specifically when setting an invalid volume. Now with mochitest!
Attachment #340449 -
Flags: superreview?(roc)
Attachment #340449 -
Flags: review?(chris.double)
Comment on attachment 340449 [details] [diff] [review] Patch v2.1 - with check for INDEX_SIZE_ERR in mochitest [Backout: Comment 11] Wow, I never knew about that funky catch syntax!
Attachment #340449 -
Flags: superreview?(roc)
Attachment #340449 -
Flags: superreview+
Attachment #340449 -
Flags: review?(chris.double)
Attachment #340449 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 8•16 years ago
|
||
Comment on attachment 340449 [details] [diff] [review] Patch v2.1 - with check for INDEX_SIZE_ERR in mochitest [Backout: Comment 11] http://hg.mozilla.org/mozilla-central/rev/62b670248ab7
Attachment #340449 -
Attachment description: Patch v2.1 - with check for INDEX_SIZE_ERR in mochitest → Patch v2.1 - with check for INDEX_SIZE_ERR in mochitest
[Checkin: Comment 8]
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Windows Vista → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Version: unspecified → Trunk
Comment 9•16 years ago
|
||
Pushed (untested) http://hg.mozilla.org/mozilla-central/rev/e55a141d8dd8 to fix { *** 24277 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_volume.html | Error thrown during test: uncaught exception: [Exception... "Index or size is negative or greater than the allowed amount" code: "1" nsresult: "0x80530001 (NS_ERROR_DOM_INDEX_SIZE_ERR)" location: "http://localhost:8888/tests/content/media/video/test/test_volume.html Line: 19"] - got 0, expected }
Comment 10•16 years ago
|
||
(In reply to comment #9) Pushed [while tree closed :-|] (untested) http://hg.mozilla.org/mozilla-central/rev/2a022ad8a3a0 to fix (my bad) { *** 24152 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_volume.html | Error thrown during test: missing ) after catch - got 0, expected 1 }
Backed out both fix attempts and the original patch, since neither of the fix attempts worked.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 12•16 years ago
|
||
Comment on attachment 340449 [details] [diff] [review] Patch v2.1 - with check for INDEX_SIZE_ERR in mochitest [Backout: Comment 11] (In reply to comment #11) > Backed out both fix attempts and the original patch, since neither of the fix > attempts worked. Yep :-< New error was { *** 24220 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_volume.html | Error thrown during test: Permission denied for <http://localhost:8888> to create wrapper for object of class XPCComponents_Interfaces - got 0, expected 1 } Chris, where/how did you get that test to work ?
Attachment #340449 -
Attachment description: Patch v2.1 - with check for INDEX_SIZE_ERR in mochitest
[Checkin: Comment 8] → Patch v2.1 - with check for INDEX_SIZE_ERR in mochitest
[Backout: Comment 11]
Assignee | ||
Comment 13•16 years ago
|
||
(In reply to comment #12) > Chris, where/how did you get that test to work ? My original patch worked for me on my Vista laptop, it's a mystery why it didn't work elsewhere! I'll try it on my Linux box now...
Assignee | ||
Comment 14•16 years ago
|
||
This is pretty much just patch v1, but it checks exception.name == "name" rather than exception == "name". I've tested this on Linux and Windows Vista, it works on them.
Attachment #340449 -
Attachment is obsolete: true
Attachment #341036 -
Flags: review?(roc)
Assignee | ||
Comment 15•16 years ago
|
||
Comment on attachment 341036 [details] [diff] [review] Patch v3 - Uses exeception.name to ID exception Oops, wrong file again.
Attachment #341036 -
Attachment is obsolete: true
Attachment #341036 -
Flags: review?(roc)
Assignee | ||
Comment 16•16 years ago
|
||
Correct file this time...
Attachment #341037 -
Flags: review?(roc)
Attachment #341037 -
Flags: superreview+
Attachment #341037 -
Flags: review?(roc)
Attachment #341037 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 17•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bdadf2e546f5
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•