Add support for defaultMuted attribute

RESOLVED FIXED in mozilla11

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: drexler, Assigned: drexler)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm] [lang=c++], URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

This is a follow up from Bug 689834
Component: General → Video/Audio
Product: Firefox → Core
QA Contact: general → video.audio
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

6 years ago
Whiteboard: [mentor=jdm] [lang=c++]
(Assignee)

Comment 1

6 years ago
Created attachment 578362 [details] [diff] [review]
Support for defaultMuted attribute v1
Attachment #578362 - Flags: feedback?(josh)
(Assignee)

Updated

6 years ago
Depends on: 689834
(Assignee)

Updated

6 years ago
No longer depends on: 689834
(Assignee)

Comment 2

6 years ago
Unsure whether i could say it fully depended on the 689834 as NS_IMPL_BOOL_ATTR needed it for resolving the muted attribute passed to it.

Updated

6 years ago
Attachment #578362 - Flags: review?(kinetik)
Attachment #578362 - Flags: feedback?(josh)
Attachment #578362 - Flags: feedback+
(In reply to andrew (:drexler) from comment #2)
> Unsure whether i could say it fully depended on the 689834 as
> NS_IMPL_BOOL_ATTR needed it for resolving the muted attribute passed to it.

Right, this patch depends on that patch because you need the muted atom.  I've readded it.  I thought it'd be simpler to split this out from bug 689834, but I was probably wrong as the trickier part is actually that bug and splitting things may have added to the confusion, sorry.

+ Support for defaultMuted attribute

Please include the bug number in the commit message.

+[scriptable, uuid(dcf812f5-373b-4Fc1-9a60-59078af33bf7)] 

Why's 'F' in caps?
Depends on: 689834
(Assignee)

Comment 4

6 years ago
(In reply to Matthew Gregan [:kinetik] from comment #3)
> +[scriptable, uuid(dcf812f5-373b-4Fc1-9a60-59078af33bf7)] 
> 
> Why's 'F' in caps?

Oh missed that. Visual Studio's GUID generator has it in caps so manually changed it. I will resubmit the patch with the changes.
(Assignee)

Comment 5

6 years ago
Created attachment 578980 [details] [diff] [review]
Support for defaultMuted attribute v2
Attachment #578362 - Attachment is obsolete: true
Attachment #578362 - Flags: review?(kinetik)
Attachment #578980 - Flags: review?(kinetik)
Can you please write a mochitest and add it to content/media/test?  If you need a base, test_paused.html is a simple one to start from.  The main things to test is that <audio> and <audio muted> start in the expected states, and that toggling defaultMuted does not affect muted (and vice versa).
(Assignee)

Comment 7

6 years ago
Sure will do that.
(Assignee)

Updated

6 years ago
Assignee: nobody → andrew.quartey
(Assignee)

Comment 8

6 years ago
Created attachment 579823 [details] [diff] [review]
Patch v3

defaultMuted attribute implementation and associated test.
Attachment #578980 - Attachment is obsolete: true
Attachment #578980 - Flags: review?(kinetik)
Attachment #579823 - Flags: review?(kinetik)
Looks good, thanks.  I'll hang off on r+ing until we've got bug 689834 sorted out.
+    v1.defaultMuted = true;
+    a1.defaultMuted = true;
+    ok(v1.defaultMuted, "v1.defaultMuted should be true");
+    ok(a1.defaultMuted, "a1.defaultMuted should be true");

Can you also add a hasAttribute("muted") test here.  That'll prove that the defaultMuted DOM attribute is correctly mapped to the muted content attribute.  Also, check that the muted DOM attribute remains false.

+    v2.defaultMuted = false;
+    a2.defaultMuted = false;
+    ok(!v2.defaultMuted, "v2.defaultMuted should be false");
+    ok(v2.muted, "v2.muted should be true");
+    ok(!a2.defaultMuted, "a2.defaultMuted should be false");
+    ok(a2.muted, "a2.muted should be true");

And for this, a !hasAttribute("muted").
(Assignee)

Comment 11

5 years ago
Created attachment 581535 [details] [diff] [review]
Patch v4

Added hasAttribute() checks to test_defaultMuted.html
Attachment #579823 - Attachment is obsolete: true
Attachment #579823 - Flags: review?(kinetik)
Attachment #581535 - Flags: review?(kinetik)
Attachment #581535 - Flags: review?(kinetik) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 581844 [details] [diff] [review]
Patch v5

Added extra checks for state of muted attribute in test_defaultMuted.html per bz's suggestion in comment 35 of bug 689834.
Attachment #581535 - Attachment is obsolete: true
Attachment #581844 - Flags: review?(bzbarsky)
Comment on attachment 581844 [details] [diff] [review]
Patch v5

r=me
Attachment #581844 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/20dcfdaaacec
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/20dcfdaaacec
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 711446

Updated

5 years ago
Keywords: dev-doc-needed
OS: Windows Vista → All
Hardware: x86 → All
https://developer.mozilla.org/en/DOM/HTMLMediaElement
https://developer.mozilla.org/en/Firefox_11_for_developers

Also added the "muted" attribute on:
https://developer.mozilla.org/en/HTML/Element/video
https://developer.mozilla.org/en/HTML/Element/audio
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.