Bug 138105 - Implement new plug-in API for muting plug-ins
Summary: Implement new plug-in API for muting plug-ins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ada Chan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-27 14:53 PDT by Ada Chan
Modified: 2014-12-03 12:50 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 2014-10-27 14:53:10 PDT
Implement new plug-in API for muting plug-ins.
Comment 1 Ada Chan 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.
Comment 2 Ada Chan 2014-11-05 23:35:34 PST
Created attachment 241088 [details]
Patch
Comment 3 WebKit Commit Bot 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.
Comment 4 Ada Chan 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.
Comment 5 WebKit Commit Bot 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.
Comment 6 Chris Dumez 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.
Comment 7 Chris Dumez 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.
Comment 8 Chris Dumez 2014-11-06 10:32:14 PST
(Sorry about the duplicate post, Bugzilla got stuck for some reason)
Comment 9 Ada Chan 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.
Comment 10 Ada Chan 2014-11-06 17:02:43 PST
Created attachment 241148 [details]
Revised patch

Incorporated Chris' feedback.
Comment 11 WebKit Commit Bot 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.
Comment 12 Ada Chan 2014-11-10 17:09:23 PST
Created attachment 241320 [details]
Revised patch
Comment 13 WebKit Commit Bot 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.
Comment 14 Ada Chan 2014-11-12 14:13:47 PST
Committed:
http://trac.webkit.org/changeset/176039