138105 – Implement new plug-in API for muting plug-ins

Ada Chan

Reported 2014-10-27 14:53:10 PDT

Implement new plug-in API for muting plug-ins.

Ada Chan

Comment 1 2014-11-04 15:40:18 PST

We are proposing a new plugin API to accomplish this: When the browser wants to mute or unmute a plug-in it will call NPP_SetValue with the NPNVmuteAudioBool = 4000 NPNVariable. The value parameter should be a pointer to an NPBool. The plug-in can query the browser for the value using NPN_GetValue. Note that the value of “NPNVmuteAudioBool” does not affect the plug-in’s audio playing state. “NPPVpluginIsPlayingAudio” and "NPNVmuteAudioBool” can both be true at the same time. The plug-in may still be playing audio, but the user won’t be able to hear it if the plug-in is muted.

Ada Chan

Comment 2 2014-11-05 23:35:34 PST

Created attachment 241088 [details] Patch

WebKit Commit Bot

Comment 3 2014-11-05 23:37:30 PST

Attachment 241088 [details] did not pass style-queue: ERROR: Source/WebCore/plugins/npapi.h:436: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/Tests/mac/SetMuted.cpp:95: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ada Chan

Comment 4 2014-11-05 23:58:20 PST

Created attachment 241090 [details] Revised patch Fixed the error from the style checker regarding the switch statement. As for the weird number of spaces in npapi.h, that's intentional to match the other lines in the enum definition.

WebKit Commit Bot

Comment 5 2014-11-06 00:01:06 PST

Attachment 241090 [details] did not pass style-queue: ERROR: Source/WebCore/plugins/npapi.h:436: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 6 2014-11-06 10:26:44 PST

Comment on attachment 241090 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=241090&action=review Just a few drive-by comments. Someone else should review this. > LayoutTests/platform/mac-wk2/plugins/muted-state-expected.txt:12 > + We're missing the TEST COMPLETE at the end. > LayoutTests/platform/mac-wk2/plugins/muted-state.html:4 > +<body onload="runTest()"> Do we really need to run this on load? If you run it on load, you'll likely have to make the test asynchronous and call finishJSTest() at the end of runTest(). (this is likely why TEST COMPLETE does not show currently). But likely, you don't need to run it on load and make the test async. > LayoutTests/platform/mac-wk2/plugins/muted-state.html:5 > +<p id="description"></p> nit: this is not needed. > LayoutTests/platform/mac-wk2/plugins/muted-state.html:6 > +<div id="console"></div> nit: this is not needed. > LayoutTests/platform/mac-wk2/plugins/muted-state.html:27 > + shouldBe("plugin1.isMuted", "false"); shouldBeFalse() > LayoutTests/platform/mac-wk2/plugins/muted-state.html:28 > + shouldBe("plugin1.cachedIsMuted", "false"); ditto > LayoutTests/platform/mac-wk2/plugins/muted-state.html:34 > + shouldBe("plugin1.isMuted", "true"); shouldBeTrue() > LayoutTests/platform/mac-wk2/plugins/muted-state.html:35 > + shouldBe("plugin1.cachedIsMuted", "true"); ditto > LayoutTests/platform/mac-wk2/plugins/muted-state.html:39 > + shouldBe("plugin2.isMuted", "true"); ditto > LayoutTests/platform/mac-wk2/plugins/muted-state.html:40 > + shouldBe("plugin2.cachedIsMuted", "true"); ditto. > LayoutTests/platform/mac-wk2/plugins/muted-state.html:45 > +description("Tests that the mute API implementation works as expected. This test needs to be run through WebKitTestRunner."); nit: I would move this up, it is nice to have the description on top of the file. > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:1092 > + NPBool value = !!muted; Why the !! ? It seems like this may be useful for converting a NPBool into a bool but I don't understand why it is helpful the other way around.

Chris Dumez

Comment 7 2014-11-06 10:29:29 PST

Comment on attachment 241090 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=241090&action=review Just a few drive-by comments. Someone else should review this. >> LayoutTests/platform/mac-wk2/plugins/muted-state-expected.txt:12 >> + > > We're missing the TEST COMPLETE at the end. We're missing the TEST COMPLETE at the end. > LayoutTests/platform/mac-wk2/plugins/muted-state.html:1 > +<head> missing <!DOCTYPE html> to make sure the test is run in strict mode. >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:4 >> +<body onload="runTest()"> > > Do we really need to run this on load? If you run it on load, you'll likely have to make the test asynchronous and call finishJSTest() at the end of runTest(). (this is likely why TEST COMPLETE does not show currently). But likely, you don't need to run it on load and make the test async. Do we really need to run this on load? If you run it on load, you'll likely have to make the test asynchronous and call finishJSTest() at the end of runTest(). (this is likely why TEST COMPLETE does not show currently). But likely, you don't need to run it on load and make the test async. >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:5 >> +<p id="description"></p> > > nit: this is not needed. nit: this is not needed. >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:6 >> +<div id="console"></div> > > nit: this is not needed. nit: this is not needed. >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:27 >> + shouldBe("plugin1.isMuted", "false"); > > shouldBeFalse() shouldBeFalse() >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:28 >> + shouldBe("plugin1.cachedIsMuted", "false"); > > ditto ditto >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:34 >> + shouldBe("plugin1.isMuted", "true"); > > shouldBeTrue() shouldBeTrue() >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:35 >> + shouldBe("plugin1.cachedIsMuted", "true"); > > ditto ditto >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:39 >> + shouldBe("plugin2.isMuted", "true"); > > ditto ditto >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:40 >> + shouldBe("plugin2.cachedIsMuted", "true"); > > ditto. ditto. >> LayoutTests/platform/mac-wk2/plugins/muted-state.html:45 >> +description("Tests that the mute API implementation works as expected. This test needs to be run through WebKitTestRunner."); > > nit: I would move this up, it is nice to have the description on top of the file. nit: I would move this up, it is nice to have the description on top of the file. > LayoutTests/platform/mac-wk2/plugins/muted-state.html:47 > +</script> nit: I think body is not closed >> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:1092 >> + NPBool value = !!muted; > > Why the !! ? It seems like this may be useful for converting a NPBool into a bool but I don't understand why it is helpful the other way around. Why the !! ? It seems like this may be useful for converting a NPBool into a bool but I don't understand why it is helpful the other way around.

Chris Dumez

Comment 8 2014-11-06 10:32:14 PST

(Sorry about the duplicate post, Bugzilla got stuck for some reason)

Ada Chan

Comment 9 2014-11-06 17:01:30 PST

(In reply to comment #6) > Comment on attachment 241090 [details] > Revised patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241090&action=review > > Just a few drive-by comments. Someone else should review this. > > > LayoutTests/platform/mac-wk2/plugins/muted-state-expected.txt:12 > > + > > We're missing the TEST COMPLETE at the end. I have fixed my test so it'll print that at the end. > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:4 > > +<body onload="runTest()"> > > Do we really need to run this on load? If you run it on load, you'll likely > have to make the test asynchronous and call finishJSTest() at the end of > runTest(). (this is likely why TEST COMPLETE does not show currently). But > likely, you don't need to run it on load and make the test async. It needs to be on load because it needs to add the plugin elements into the document's body. I made the test asynchronous. > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:5 > > +<p id="description"></p> > > nit: this is not needed. > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:6 > > +<div id="console"></div> > > nit: this is not needed. > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:27 > > + shouldBe("plugin1.isMuted", "false"); > > shouldBeFalse() > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:28 > > + shouldBe("plugin1.cachedIsMuted", "false"); > > ditto > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:34 > > + shouldBe("plugin1.isMuted", "true"); > > shouldBeTrue() > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:35 > > + shouldBe("plugin1.cachedIsMuted", "true"); > > ditto > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:39 > > + shouldBe("plugin2.isMuted", "true"); > > ditto > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:40 > > + shouldBe("plugin2.cachedIsMuted", "true"); > > ditto. > > > LayoutTests/platform/mac-wk2/plugins/muted-state.html:45 > > +description("Tests that the mute API implementation works as expected. This test needs to be run through WebKitTestRunner."); > > nit: I would move this up, it is nice to have the description on top of the > file. Fixed all of the above. > > > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:1092 > > + NPBool value = !!muted; > > Why the !! ? It seems like this may be useful for converting a NPBool into a > bool but I don't understand why it is helpful the other way around. Yes, I don't need to !! the bool here. Usually I would have done it if I'm trying to set it to a value of a type bigger than an unsigned char. Thanks so much for your feedback, Chris. I'll post an updated patch.

Ada Chan

Comment 10 2014-11-06 17:02:43 PST

Created attachment 241148 [details] Revised patch Incorporated Chris' feedback.

WebKit Commit Bot

Comment 11 2014-11-06 17:05:11 PST

Attachment 241148 [details] did not pass style-queue: ERROR: Source/WebCore/plugins/npapi.h:436: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ada Chan

Comment 12 2014-11-10 17:09:23 PST

Created attachment 241320 [details] Revised patch

WebKit Commit Bot

Comment 13 2014-11-10 17:10:59 PST

Attachment 241320 [details] did not pass style-queue: ERROR: Source/WebCore/plugins/npapi.h:436: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ada Chan

Comment 14 2014-11-12 14:13:47 PST

Committed: http://trac.webkit.org/changeset/176039

Attachments
Patch (31.97 KB, patch) 2014-11-05 23:35 PST, Ada Chan	no flags	Details Formatted Diff Diff
Revised patch (31.95 KB, patch) 2014-11-05 23:58 PST, Ada Chan	no flags	Details Formatted Diff Diff
Revised patch (32.02 KB, patch) 2014-11-06 17:02 PST, Ada Chan	no flags	Details Formatted Diff Diff
Revised patch (31.86 KB, patch) 2014-11-10 17:09 PST, Ada Chan	andersca: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.