Closed Bug 859601 Opened 7 years ago Closed 7 years ago

mozContacts, mozSettings API - return undefined if the API is unsupported on the platform, not null

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jsmith, Assigned: gwagner)

References

Details

Attachments

(2 files, 4 obsolete files)

See the discussion on bug 859554 for context. Any WebAPI that is not supported on a platform should return undefined, not null. null is only returned if the API is supported on the platform, but the web content does not have access to that API.
Assignee: nobody → anygregor
Blocks: 859554
blocking-b2g: --- → tef?
Wait a second. This is WFM. Don't need to update this API.
Assignee: anygregor → nobody
No longer blocks: 859554
Status: NEW → RESOLVED
blocking-b2g: tef? → ---
Closed: 7 years ago
Resolution: --- → WORKSFORME
Wait, I'm wrong again. Disregard. Just confirmed calling navigator.mozContacts returns null on desktop. Sorry for the churn.
Assignee: nobody → anygregor
Blocks: 859554
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Attached patch WiP (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #735010 - Attachment is obsolete: true
Comment on attachment 735016 [details] [diff] [review]
patch

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

Nice! Since this will probably be done in the same way for most of the apis implemented in JS, maybe we could factorize this in a single reusable/configurable startup component?

::: dom/contacts/ContactManager.js
@@ +746,5 @@
> +          cm.addCategoryEntry("JavaScript-navigator-property", "mozContacts", "@mozilla.org/contactManager;1", false, false);
> +        }
> +        var prefService = Components.classes["@mozilla.org/preferences-service;1"]
> +                                    .getService(Components.interfaces.nsIPrefService);
> +        this.branch = prefService.getBranch("dom.mozContacts.");

nit: you can reuse Services.prefs here
Attachment #735016 - Flags: feedback+
Attached patch patch (obsolete) — Splinter Review
Make it more generic and add a new category: JavaScript-navigator-property-maybe
Attachment #735016 - Attachment is obsolete: true
Attachment #735357 - Flags: review?(bent.mozilla)
Attached patch fix settings APISplinter Review
Comment on attachment 735357 [details] [diff] [review]
patch

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

r=me with these fixed

::: dom/base/NavigatorPropertyHelper.js
@@ +12,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const NAVIGATORPROPERTYHELPER_CID        = Components.ID("{f0d03420-a0ce-11e2-9e96-0800200c9a66}");

Nit: You only use this in one place, could just inline it.

@@ +23,5 @@
> +  QueryInterface : XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
> +
> +  observe: function observe(subject, topic, data) {
> +    let cm = Components.classes["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
> +    switch (topic) {

You should add a default case that warns (if DEBUG) about unknown topics I think.

@@ +25,5 @@
> +  observe: function observe(subject, topic, data) {
> +    let cm = Components.classes["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
> +    switch (topic) {
> +      case 'app-startup':
> +        var enumerator = cm.enumerateCategory("JavaScript-navigator-property-maybe");

Nit: var vs. let

@@ +29,5 @@
> +        var enumerator = cm.enumerateCategory("JavaScript-navigator-property-maybe");
> +        while (enumerator.hasMoreElements()) {
> +          let entry = enumerator.getNext().QueryInterface(Ci.nsISupportsCString);
> +          let keyVal = cm.getCategoryEntry("JavaScript-navigator-property-maybe", entry).split(',');
> +          if (Services.prefs.getBoolPref(entry)) {

This throws if the pref doesn't exist, right? I think you want to guard against that and assume not-present is the same as disabled.

@@ +33,5 @@
> +          if (Services.prefs.getBoolPref(entry)) {
> +            if (DEBUG) debug("enable: " + keyVal[0]);
> +            cm.addCategoryEntry("JavaScript-navigator-property", keyVal[0], keyVal[1], false, false);
> +          }
> +          Services.prefs.addObserver(entry, this, false);

The last arg is false, meaning "don't hold weak". If that what you're using then you don't need the nsISupportsWeakReference above.

@@ +37,5 @@
> +          Services.prefs.addObserver(entry, this, false);
> +        }
> +        break;
> +      case 'nsPref:changed':
> +        var keyVal = cm.getCategoryEntry("JavaScript-navigator-property-maybe", data).split(',');

Nit: var vs. let

@@ +40,5 @@
> +      case 'nsPref:changed':
> +        var keyVal = cm.getCategoryEntry("JavaScript-navigator-property-maybe", data).split(',');
> +        let key  = keyVal[0];
> +        let val = keyVal[1];
> +        if (Services.prefs.getBoolPref(data)) {

This throws.

@@ +42,5 @@
> +        let key  = keyVal[0];
> +        let val = keyVal[1];
> +        if (Services.prefs.getBoolPref(data)) {
> +          if (DEBUG) debug("enable: " + key);
> +          cm.addCategoryEntry("JavaScript-navigator-property", key, val, false, true);

It doesn't make sense to me to overwrite here but not above. I think the not overwriting is probably fine, but it would be good to warn if the
Attachment #735357 - Flags: review?(bent.mozilla) → review+
Attached patch patch (obsolete) — Splinter Review
fix comments and add manifest to installer.
Attachment #735357 - Attachment is obsolete: true
Attachment #735459 - Flags: review?(bent.mozilla)
Attachment #735366 - Flags: review?(bent.mozilla)
Attachment #735366 - Flags: review?(bent.mozilla) → review+
Comment on attachment 735459 [details] [diff] [review]
patch

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

r=me with these fixed.

::: dom/base/NavigatorPropertyHelper.js
@@ +41,5 @@
> +        }
> +        break;
> +      case 'nsPref:changed':
> +        let keyVal = cm.getCategoryEntry("JavaScript-navigator-property-maybe", data).split(',');
> +        let key  = keyVal[0];

Nit: extra space

@@ +49,5 @@
> +            if (DEBUG) debug("enable: " + key);
> +            cm.addCategoryEntry("JavaScript-navigator-property", key, val, false, false);
> +          } else {
> +            if (DEBUG) debug("disable: " + key);
> +            cm.deleteCategoryEntry("JavaScript-navigator-property", key, false);

You want to always call this if the getBoolPref throws because the pref doesn't exist, right?
Attachment #735459 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] from comment #10)
> Comment on attachment 735459 [details] [diff] [review]
> patch
> 
> Review of attachment 735459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with these fixed.
> 
> ::: dom/base/NavigatorPropertyHelper.js
> @@ +41,5 @@
> > +        }
> > +        break;
> > +      case 'nsPref:changed':
> > +        let keyVal = cm.getCategoryEntry("JavaScript-navigator-property-maybe", data).split(',');
> > +        let key  = keyVal[0];
> 
> Nit: extra space
> 
> @@ +49,5 @@
> > +            if (DEBUG) debug("enable: " + key);
> > +            cm.addCategoryEntry("JavaScript-navigator-property", key, val, false, false);
> > +          } else {
> > +            if (DEBUG) debug("disable: " + key);
> > +            cm.deleteCategoryEntry("JavaScript-navigator-property", key, false);
> 
> You want to always call this if the getBoolPref throws because the pref
> doesn't exist, right?

Yes. I don't know if this makes sense in the pref-changed case but maybe someone can delete a pref as well?
Summary: mozContacts API - return undefined if the API is unsupported on the platform, not null → mozContacts, mozSettings API - return undefined if the API is unsupported on the platform, not null
(In reply to Gregor Wagner [:gwagner] from comment #11)
> Yes. I don't know if this makes sense in the pref-changed case but maybe
> someone can delete a pref as well?

It looks to me like we will get this notification if the pref is deleted: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/prefapi.cpp#619
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a28728d22f22 - desktop builds failed complaining about the file not existing (for some unobvious reason), like https://tbpl.mozilla.org/php/getParsedLog.php?id=21656232&tree=Mozilla-Inbound, while Android failed in test_contacts_basics.html like https://tbpl.mozilla.org/php/getParsedLog.php?id=21658595&tree=Mozilla-Inbound
Attached patch patchSplinter Review
Attachment #735459 - Attachment is obsolete: true
Attachment #736361 - Flags: review+
Whiteboard: checkin-needed
Blocks: 860918
https://hg.mozilla.org/mozilla-central/rev/d5f5223bc71c
https://hg.mozilla.org/mozilla-central/rev/c0e95d291680
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 862247
Blocks: 862353
You need to log in before you can comment on or make changes to this bug.