Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Description • 11 years ago

#ifdef MOZ_SYS_MSG makes it painful to debug in the browser. Let's replace it with a preference like other webapis.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 1 • 11 years ago

This is a really dirty ugly poc. But it show the global idea.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 2 • 11 years ago

Comment on attachment 737121 [details] [diff] [review]
dirty ugly poc

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

::: dom/activities/src/Activity.h
@@ +26,5 @@
>    virtual JSObject*
>    WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;
>  
>    static bool PrefEnabled()
>    {

This should look at the pref

::: dom/base/nsDOMClassInfo.cpp
@@ +1804,1 @@
>      DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorSystemMessages)

Should use DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY

	[:fabrice] Fabrice Desré
	Comment 4 • 11 years ago

Comment on attachment 737959 [details] [diff] [review]
activities2.patch

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

::: dom/activities/src/Activity.h
@@ +29,4 @@
>  
>    static bool PrefEnabled()
>    {
> +    return Preferences::GetBool("dom.sysmsg.enabled");

I would add the default "false" value explicitly, especially since you don't set the pref in all.js

::: dom/apps/src/Webapps.jsm
@@ -228,4 @@
>  
>    // Installs a 3rd party app.
>    installPreinstalledApp: function installPreinstalledApp(aId) {
> -#ifdef MOZ_WIDGET_GONK

You probably don't want to change that for now, unless you provide something sensible for coreAppsDir. That may "work" just because we return early in the first try {} block.

@@ -1284,4 @@
>    updateAppHandlers: function(aOldManifest, aNewManifest, aApp) {
>      debug("updateAppHandlers: old=" + aOldManifest + " new=" + aNewManifest);
>      this.notifyAppsRegistryStart();
> -#ifdef MOZ_SYS_MSG

Why isn't this replaced by a preference check?

::: dom/base/Navigator.cpp
@@ +1450,5 @@
>  NS_IMETHODIMP
>  Navigator::MozHasPendingMessage(const nsAString& aType, bool *aResult)
>  {
> +  if (!Preferences::GetBool("dom.sysmsg.enabled", false)) {
> +    return NS_OK;

why not NS_ERROR_NOT_IMPLEMENTED?

@@ +1465,5 @@
>  Navigator::MozSetMessageHandler(const nsAString& aType,
>                                  nsIDOMSystemMessageCallback *aCallback)
>  {
> +  if (!Preferences::GetBool("dom.sysmsg.enabled", false)) {
> +    return NS_OK;

ditto

	Kevin Grandon :kgrandon Assignee
	Comment 6 • 11 years ago

Comment on attachment 738009 [details] [diff] [review]
activities3.patch

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

Thanks for the review Fabrice! I've updated the patch to capture your comments, and would appreciate your formal review stamp if you have the time.

	[:fabrice] Fabrice Desré
	Comment 7 • 11 years ago

Comment on attachment 738009 [details] [diff] [review]
activities3.patch

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

We're almost there!

Some things to check: Services.prefs.getBoolPref("dom.sysmsg.enabled") used in Webapps.jsm will throw on platforms that don't have the preference set.
You should do at least one of:
- turn that into a function supportSystemMessages() (bonus points for caching the result).
- add the pref with a default "false" value to modules/libpref/src/init/all.js

I'd like to make sure that when the pref is turned off we have "mozSetMessageHandler in navigator" return false, and "MozActivity in window" also.

::: dom/apps/src/Webapps.jsm
@@ -439,4 @@
>      }).bind(this));
>    },
>  
> -#ifdef MOZ_SYS_MSG

Didn't you forget to use the pref there instead of removing the #ifdef?

	Kevin Grandon :kgrandon Assignee
	Comment 8 • 11 years ago

Comment on attachment 738009 [details] [diff] [review]
activities3.patch

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

::: dom/apps/src/Webapps.jsm
@@ -439,4 @@
>      }).bind(this));
>    },
>  
> -#ifdef MOZ_SYS_MSG

This is defining a function within an object literal - I don't think we want to use a preference here to remove it. The calling functions check the preference before calling _registerSystemMessages so I believe this should be ok.

	Kevin Grandon :kgrandon Assignee
	Comment 9 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 738009 [details] [diff] [review]
> activities3.patch
> 
> Review of attachment 738009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're almost there!
> 
> Some things to check: Services.prefs.getBoolPref("dom.sysmsg.enabled") used
> in Webapps.jsm will throw on platforms that don't have the preference set.

Sounds good. I will create a function which caches this and default it to false.


> I'd like to make sure that when the pref is turned off we have
> "mozSetMessageHandler in navigator" return false, and "MozActivity in
> window" also.

I verified that window.MozActivity and navigator.mozSetMessageHandler are undefined when the pref is false. Is it important to differentiate between undefined and false responses for disabled APIs?

	Mounir Lamouri (:mounir)
	Comment 10 • 11 years ago

(In reply to Kevin Grandon :kgrandon from comment #9)
> > I'd like to make sure that when the pref is turned off we have
> > "mozSetMessageHandler in navigator" return false, and "MozActivity in
> > window" also.
> 
> I verified that window.MozActivity and navigator.mozSetMessageHandler are
> undefined when the pref is false. Is it important to differentiate between
> undefined and false responses for disabled APIs?

That's good, we want window.mozActivity and navigator.mozSetMessageHandler to return undefined.

	Kevin Grandon :kgrandon Assignee
	Comment 11 • 11 years ago

Hi Fabrice - Please take a look at this patch when you have a moment. It's the same as before just with the default preference, and wrapping supportSystemMessages() call. I think your other concerns are already handled (see my previous comments). Thanks!

	Kevin Grandon :kgrandon Assignee
	Comment 12 • 11 years ago

Thank you for the awesome reviews on this Fabrice.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 13 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/556eb3bb6298

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 14 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb5eeb57318

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/cbbd94e6ab32

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

Yeah, this is what blew up the tests on inbound earlier. Backed out.
https://hg.mozilla.org/projects/birch/rev/d4e57a4a2fcd

Logs available below:
https://tbpl.mozilla.org/?tree=Birch&rev=d8c4ca787e39

	Kevin Grandon :kgrandon Assignee
	Comment 17 • 11 years ago

Sorry about that guys =( There was a typo in the patch which I didn't run into during my testing. Fixed now, will push this to the try server.

	Kevin Grandon :kgrandon Assignee
	Comment 18 • 11 years ago

Comment on attachment 742412 [details] [diff] [review]
Fixed patch

Hi Fabrice,

Here is a slightly change patch, with the typo fixed =/ Not sure if I need a new review+ from you or not, requesting in case.

This has been pushed to the try server: https://tbpl.mozilla.org/?tree=Try&rev=b281862b30bd

A few tests are failing with android, but it doesn't appear that they are related to this change.. Could you take a quick look to see if it's safe? Thanks!

	Kevin Grandon :kgrandon Assignee
	Comment 19 • 11 years ago

Try server results above, trying this again. Adding checkin-needed keyword again. Thanks all.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/57db8a22ec9b

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/57db8a22ec9b