Summary: | Implement new plug-in API for muting plug-ins | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ada Chan <adachan> | ||||||||||
Component: | Plug-ins | Assignee: | Ada Chan <adachan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | adachan, andersca, ap, buildbot, cdumez, commit-queue, dbates, rniwa, sam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=139231 | ||||||||||||
Attachments: |
|
Description
Ada Chan
2014-10-27 14:53:10 PDT
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. Created attachment 241088 [details]
Patch
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.
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.
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.
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. 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. (Sorry about the duplicate post, Bugzilla got stuck for some reason) (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. Created attachment 241148 [details]
Revised patch
Incorporated Chris' feedback.
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.
Created attachment 241320 [details]
Revised patch
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.
Committed: http://trac.webkit.org/changeset/176039 |