806523 - Provide a way for plugins to detect which Windows environment they are running in

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Provide for plugins a way of knowing which Windows environment the browser is running in. We're thinking this can be through a new NPN_GetValue identifier. (PNVmetroBrowser?)

Comment 1

[Jim Mathies [:jimm]]

Attachment: plugins v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: metro browser tests

Comment 3

[Jim Mathies [:jimm]]

Attachment: metro browser tests

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 699320 [details] [diff] [review]
plugins v.1

This adds support for a metro/desktop flag that can be queried via NPN_GetValue.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 699325 [details] [diff] [review]
metro browser tests

Adding a mochitest-metro-chrome test for the same. I'll be adding to these tests as I work through other windowless flash support bugs.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 699320 [details] [diff] [review]
plugins v.1

Review of attachment 699320 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +271,5 @@
> +                                                                NPError* result)
> +{
> +    *result = NPERR_INVALID_PARAM;
> +#ifdef XP_WIN
> +    NPWindowsEnvironment flag;

I didn't need this extra variable, I can just assign 'value' directly. Updated in my local copy.

Comment 7

[Benjamin Smedberg]

Did this API get proposed to plugin-futures? npapi.h is imported from the npapi-sdk which is shared code.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Did this API get proposed to plugin-futures? npapi.h is imported from the
> npapi-sdk which is shared code.

No it did not, but but obviously can be. Who listens to plugin-futures? Before I do that though I'd like to get approval on the approach, including whatever I'll put together for bug 806513.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #8)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> > Did this API get proposed to plugin-futures? npapi.h is imported from the
> > npapi-sdk which is shared code.
> 
> No it did not, but but obviously can be. Who listens to plugin-futures?
> Before I do that though I'd like to get approval on the approach, including
> whatever I'll put together for bug 806513.

https://mail.mozilla.org/pipermail/plugin-futures/

Not a very busy list. I'll put something together to post at some point here.

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 699325 [details] [diff] [review]
metro browser tests

Review of attachment 699325 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/tests/Makefile.in
@@ +17,5 @@
>    browser_context_ui.js \
>    browser_onscreen_keyboard.js \
>    browser_onscreen_keyboard.html \
>    browser_downloads.js \
> +  browser_downloads.js \

Looks like a duplicate line snuck in by accident.

::: browser/metro/base/tests/browser_plugin_values.js
@@ +28,5 @@
> +
> +    let utils = Cc["@mozilla.org/windows-metroutils;1"].createInstance(Ci.nsIWinMetroUtils);
> +    is(plugin.getNPNVWindowsEnvironment(),
> +        utils.immersive ? XRE_WindowsEnvironmentType_Metro : XRE_WindowsEnvironmentType_Desktop,
> +        "Checking for expected windows environment");

Since these metro tests run only in the immersive environment, only one of these code paths will actually be tested.  Should we add a corresponding desktop browser-chrome mochitest too?

::: browser/metro/profile/metro.js
@@ +395,5 @@
> +// test plugin so we can write tests for future plugin support.
> +pref("plugin.disable", false);
> +pref("dom.ipc.plugins.enabled", false);
> +pref("dom.ipc.plugins.java.enabled", false);
> +pref("dom.ipc.plugins.enabled.nptest.dll", true);

If possible, we should just set these prefs at the start of the plugin tests and reset them after the tests finish, rather than change our default config.

If I understand correctly, pref("dom.ipc.plugins.enabled", false) prevents plugins like Flash from running out-of-process but still lets them run in-process.

Comment 11

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> ::: browser/metro/base/tests/browser_plugin_values.js
> @@ +28,5 @@
> > +
> > +    let utils = Cc["@mozilla.org/windows-metroutils;1"].createInstance(Ci.nsIWinMetroUtils);
> > +    is(plugin.getNPNVWindowsEnvironment(),
> > +        utils.immersive ? XRE_WindowsEnvironmentType_Metro : XRE_WindowsEnvironmentType_Desktop,
> > +        "Checking for expected windows environment");
> 
> Since these metro tests run only in the immersive environment, only one of
> these code paths will actually be tested.  Should we add a corresponding
> desktop browser-chrome mochitest too?

Yes I've got a duplicate mochitest-plain test I can add to the test plugin tests that checks the value as well. I'll add it to the patch.

> ::: browser/metro/profile/metro.js
> @@ +395,5 @@
> > +// test plugin so we can write tests for future plugin support.
> > +pref("plugin.disable", false);
> > +pref("dom.ipc.plugins.enabled", false);
> > +pref("dom.ipc.plugins.java.enabled", false);
> > +pref("dom.ipc.plugins.enabled.nptest.dll", true);
> 
> If possible, we should just set these prefs at the start of the plugin tests
> and reset them after the tests finish, rather than change our default config.
> 
> If I understand correctly, pref("dom.ipc.plugins.enabled", false) prevents
> plugins like Flash from running out-of-process but still lets them run
> in-process.

I'll look. Maybe I can add something specific to metro tests somewhere. We set general pref in the test profile but I don't think I want to muck with those - 
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#372

Comment 12

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #11)
> I'll look. Maybe I can add something specific to metro tests somewhere. We
> set general pref in the test profile but I don't think I want to muck with
> those - 
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#372

I just mean that you should use Services.prefs.setBoolPref("plugin.disable", false) at the start of your tests, and then clearUserPref("plugin.disable") in your test's cleanup code (or use SpecialPowers.pushPrefEnv which is a convenience function that does the same thing).

Comment 13

[Jim Mathies [:jimm]]

Attachment: metro browser tests

Comment 14

[Jim Mathies [:jimm]]

Attachment: metro browser tests

- pref cleanup
- moved update to PluginInstanceParent down to the plugins patch

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 699447 [details] [diff] [review]
metro browser tests

Review of attachment 699447 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck but someone else should review the PluginInstanceParent.cpp change if they haven't already.

::: dom/plugins/test/mochitest/test_win8_values.html
@@ +1,3 @@
> +<html>
> +<head>
> +  <title>Test New Windows 8 Related Values</title>

I think you need to add this to the Makefile.in (probably in a WINNT-only section).

Comment 16

[Jim Mathies [:jimm]]

Attachment: metro browser tests

final rev with the makefile addition.

Comment 17

[Josh Aas]

Comment on attachment 699320 [details] [diff] [review]
plugins v.1

Review of attachment 699320 [details] [diff] [review]:
-----------------------------------------------------------------

The API should be proposed on plugin-futures, we can move on it quickly. Once we have a solution we can land in npapi-sdk and then sync NPAPI headers.

::: dom/plugins/ipc/PluginMessageUtils.h
@@ +892,5 @@
>    }
>  };
>  
> +template <>
> +struct ParamTraits<NPWindowsEnvironment>

Is this type really necessary? Can we just use an int and cast?