861496 - Replace #ifdef MOZ_SYS_MSG by a preference

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dirty ugly poc

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

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 3

[Kevin Grandon :kgrandon]

Attachment: activities2.patch

Comment 4

[[:fabrice] Fabrice Desré]

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

Comment 5

[Kevin Grandon :kgrandon]

Attachment: activities3.patch

Comment 6

[Kevin Grandon :kgrandon]

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.

Comment 7

[[:fabrice] Fabrice Desré]

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?

Comment 8

[Kevin Grandon :kgrandon]

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.

Comment 9

[Kevin Grandon :kgrandon]

(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?

Comment 10

[Mounir Lamouri (:mounir)]

(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.

Comment 11

[Kevin Grandon :kgrandon]

Attachment: activities4.patch

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!

Comment 12

[Kevin Grandon :kgrandon]

Thank you for the awesome reviews on this Fabrice.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Kevin Grandon :kgrandon]

Attachment: Fixed patch

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.

Comment 18

[Kevin Grandon :kgrandon]

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!

Comment 19

[Kevin Grandon :kgrandon]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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