Last Comment Bug 706731 - Add support for defaultMuted attribute
: Add support for defaultMuted attribute
[mentor=jdm] [lang=c++]
: dev-doc-complete
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Andrew Quartey [:drexler]
Depends on: 689834 711446
  Show dependency treegraph
Reported: 2011-11-30 18:09 PST by Andrew Quartey [:drexler]
Modified: 2012-01-21 10:36 PST (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Support for defaultMuted attribute v1 (2.56 KB, patch)
2011-12-01 13:00 PST, Andrew Quartey [:drexler]
josh: feedback+
Details | Diff | Review
Support for defaultMuted attribute v2 (2.57 KB, patch)
2011-12-04 21:31 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Review
Patch v3 (5.07 KB, patch)
2011-12-07 13:48 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Review
Patch v4 (5.39 KB, patch)
2011-12-13 21:44 PST, Andrew Quartey [:drexler]
kinetik: review+
Details | Diff | Review
Patch v5 (5.72 KB, patch)
2011-12-14 17:22 PST, Andrew Quartey [:drexler]
bzbarsky: review+
Details | Diff | Review

Description Andrew Quartey [:drexler] 2011-11-30 18:09:56 PST
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
Comment 1 Andrew Quartey [:drexler] 2011-12-01 13:00:08 PST
Created attachment 578362 [details] [diff] [review]
Support for defaultMuted attribute v1
Comment 2 Andrew Quartey [:drexler] 2011-12-01 13:19:52 PST
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.
Comment 3 Matthew Gregan [:kinetik] 2011-12-04 21:01:52 PST
(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?
Comment 4 Andrew Quartey [:drexler] 2011-12-04 21:10:00 PST
(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.
Comment 5 Andrew Quartey [:drexler] 2011-12-04 21:31:13 PST
Created attachment 578980 [details] [diff] [review]
Support for defaultMuted attribute v2
Comment 6 Matthew Gregan [:kinetik] 2011-12-04 21:32:05 PST
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).
Comment 7 Andrew Quartey [:drexler] 2011-12-04 21:34:03 PST
Sure will do that.
Comment 8 Andrew Quartey [:drexler] 2011-12-07 13:48:34 PST
Created attachment 579823 [details] [diff] [review]
Patch v3

defaultMuted attribute implementation and associated test.
Comment 9 Matthew Gregan [:kinetik] 2011-12-12 01:25:15 PST
Looks good, thanks.  I'll hang off on r+ing until we've got bug 689834 sorted out.
Comment 10 Matthew Gregan [:kinetik] 2011-12-12 17:58:42 PST
+    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").
Comment 11 Andrew Quartey [:drexler] 2011-12-13 21:44:24 PST
Created attachment 581535 [details] [diff] [review]
Patch v4

Added hasAttribute() checks to test_defaultMuted.html
Comment 12 Andrew Quartey [:drexler] 2011-12-14 17:22:24 PST
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.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-14 21:57:33 PST
Comment on attachment 581844 [details] [diff] [review]
Patch v5

Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-15 11:53:21 PST
Comment 15 Ed Morley [:emorley] 2011-12-16 06:09:22 PST

Note You need to log in before you can comment on or make changes to this bug.