886260 - SpecialPowers.setComplexValue and tests

Status: NEW

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

Attachment: wip patch

This is a follow-up from bug 880310, where I already mentioned in comment 0 that SpecialPowers.setComplexValue wasn't working.
This patch makes it working.

However, there are 2 problems:
- The message system only allows passing strings, and setComplexValue needs to have info on nsIFile, nsISupportsString, nsILocalizedPrefString. I've made the patch so that it's converted back to those objects. This works well, except when there would be a testcase that would write 2 nsIFile prefs, for instance and then is using getComplexValue to get the nsIFile instance. In that case, you don't get the same exact object back (although, the path is still the same).
This could perhaps be solved by storing all cases where setComplexValue/getComplexValue is used inside the specialPowers object.
Or perhaps not bother and just use strings for the setComplexValue/getComplexValue methods on the specialPowers object?

- The other problem is that when the pref of a complexValue is changed, no notifyObserver is called at all. The pushPrefEnv method relies on that (inside _applyPrefs):
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#868
That means that complexValue can't be the last or only one in a pushPrefEnv call, because then the callback wouldn't be executed.
Ideally, this would have to be solved in the pref backend code (which doesn't seem to handle complexValue at all that well), which I don't know how to.
A workaround perhaps would be to artificially set a dummy bool pref (and unset it), when complexValue is the last or only one in a pushPrefEnv call.

What do you think, Joel?

Comment 1

[(not currently active) Ted Mielczarek]

If the complexValue stuff is broken because of platform bugs we should probably remove or disable the API until those platform bugs are fixed. (Maybe it doesn't matter, and nobody cares about using complexValue prefs?)

Comment 2

[Martijn Wargers (dead)]

It's not being used by mochitests, afaict.

Comment 3

[Martijn Wargers (dead)]

I guess removing this code might be the best option.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 766607 [details] [diff] [review]
wip patch

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

I sort of agree with ted on this.  Overall, this patch isn't too bad although I don't like the addition of a single variables.  If we don't get a notification whe nthe pref changes, I would like to wait until that is fixed before tackling these tests.

Comment 5

[Martijn Wargers (dead)]

I filed bug 886553 for better support of complex values in prefbranch.

In the meantime, should the complex value code in specialPowers be removed? Since that code makes it seem that it is supported, while it isn't.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

I believe so.

Comment 7

[Martijn Wargers (dead)]

Ok, bug 886553 is invalid, observers are being fired when chaning complex values.
It turns out that complex values are internally stored as char prefs.
That also makes me wonder about the first problem in comment 0, that is probably not a big deal, then, also.

Comment 8

[Martijn Wargers (dead)]

Ok, the 1st problem I mentioned in comment 0 is really moot, because this:
var pb = SpecialPowers.Cc["@mozilla.org/preferences-service;1"].getService(SpecialPowers.Ci.nsIPrefBranch);
pb.setComplexValue("test.complex", SpecialPowers.Ci.nsIFile, testFile);
var t1 = pb.getComplexValue("test.complex", SpecialPowers.Ci.nsIFile);
var t2 = pb.getComplexValue("test.complex", SpecialPowers.Ci.nsIFile);
alert(t1==t2);
..is returning false.
So I guess one shouldn't expect to get the same object returned when doing a getComplexValue.

Comment 9

[Martijn Wargers (dead)]

Attachment: 886260_setprefcomplex.diff

Ok, so the issue in comment 8 should ideally be the opposite, actually. You should never get returned the same object with getComplexValue.
This is not what this patch is doing, but I don't think it's an issue that has practical consequences.
I tried to fix it by creating nsIFile, nsISupportsString, etc dynamically in the getPref function, but then I got this js error:
 3:53.45 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Error: Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass at http://mochi.test:8888/tests/Harness_Sanity/test_SpecialPowersPushPrefEnv.html?autorun=1&closeWhenDone=1&consoleLevel=INFO&failureFile=/Users/mwargers/mozilla-central/obj-x86_64-apple-darwin11.4.2/.mozbuild/mochitest_failures.json:141
mwargers:mozilla-central mwargers$ ./mach mochitest-plain Harness_Sanity/test_SpecialPowersPushPrefEnv.html
So then I gave up. Like I said, I don't think this is a real issue in practice.

I still left the dump() commands in this patch for now, if you don't mind.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 780558 [details] [diff] [review]
886260_setprefcomplex.diff

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

overall, remove the dump statements and for a few which are truly beneficial, lets fix the use of \n.

::: testing/mochitest/tests/test_SpecialPowersPushPrefEnv.html
@@ +88,5 @@
> +  try {
> +    SpecialPowers.setComplexValue("test.complexfile", SpecialPowers.Ci.nsIFile, 'bogus');
> +  } catch(e) {
> +    SpecialPowers.setComplexValue("test.complexfile", SpecialPowers.Ci.nsIFile, testFile);
> +  }

I have no idea what is supposed to work here.  There is no ok() or is() call.  I assume we want to hit the catch condition.

@@ +137,5 @@
>      setTimeout(test1, 0, ++aCount);
>      return;
>    }
>  
> +SpecialPowers.getComplexValue('test.complexfile', SpecialPowers.Ci.nsIFile)

where does this call put the returned value?  This seems like an unnecessary call.

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +193,5 @@
>                return(prefs.getCharPref(prefName));
>              else
>                return(prefs.setCharPref(prefName, prefValue));
>            case "COMPLEX":
> +            dump("case complex\n\nprefname:"+prefName+"\n\n\n");

I don't like having extra dump calls in our main libraries.  If these are very beneficial, please remove the extra \n and do your best to minimize these.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +31,5 @@
>    this._fm = null;
>    this._cb = null;
> +  this._prefFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +  this._prefSupportsString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
> +  this._prefLocalizedString = Cc["@mozilla.org/pref-localizedstring;1"].createInstance(Ci.nsIPrefLocalizedString);

please add a comment here to indicate the intended use of these.  It is not ideal to have a single value for an api that can be called over and over again, but I understand the need for these.

@@ +828,5 @@
>        }
>        this._prefEnvUndoStack.push(cleanupActions);
>        this._pendingPrefs.push([pendingActions, delayedCallback]);
> +      dump("this._applyPrefs();"+pendingActions.length+"\n\n\n\n\n");
> +            dump("pendingActions.length:"+pendingActions.length+"\n\n\n\n\n");

please put some indication in the dump that this is specialpowers and transaction 'x'.  Also less \n would be nice.

@@ +833,3 @@
>        this._applyPrefs();
>      } else {
> +      dump("tcallbak;\n\n\n\n\n");

I am not sure this dump is useful here.

@@ +1060,4 @@
>      } else {
>        msg = {'op':'set', 'prefName': aPrefName, 'prefType': aPrefType, 'prefValue': aValue};
>      }
> +    dump('sendsyncmessage\n\n\n\n');

remove this dump!

Comment 11

[Martijn Wargers (dead)]

Sorry, I was not clear, I was planning on removing all the dumps that I added before final review. Thanks for the review!

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.