Closed Bug 699859 (asyncContentPrefs) Opened 13 years ago Closed 11 years ago

Refactor nsContentPrefService.js to use async Storage

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: mak, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, main-thread-io, Whiteboard: [Snappy:P1])

Attachments

(1 file, 18 obsolete files)

118.86 KB, patch
mak
: review+
mossop
: superreview+
Details | Diff | Splinter Review
nsContentPrefService.js should stop using synchronous Storage APIs.
Alias: asyncContentPrefs
Summary: Refactor nsSearchService.js to use async Storage → Refactor nsContentPrefService.js to use async Storage
Assignee: nobody → felipc
A comment mak posted on bug 711494:

> ah-ha, I was not looking for the right magic word, here we are:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/nsContentPrefService.js#1457
>
>interesting that this component uses the same statement synchronously and asynchronously, it's a good recipe for thread contention
Felipe is tied up in IE migrator work, re-assigning to Drew!
Assignee: felipc → adw
Attached patch messy work-in-progress (obsolete) — Splinter Review
nsIContentPrefService isn't conducive to an async API, so this creates nsIContentPrefAsyncService.  It does not inherit from nsIContentPrefService so that nsIContentPrefService can be easily deprecated and removed in the future.  It's implemented by the same nsContentPrefService.js component though, since I can reuse some parts, and access to the database needs to be funneled through one connection (right?).

There are a couple of mxr search results pages' worth of nsIContentPrefService consumers, but I figure they can be updated piecemeal if necessary.
Attached patch cleaned-up work-in-progress (obsolete) — Splinter Review
Hey Marco, do you think I'm on the right track?  Feel free to comment on the implementation, but what do you think about the architecture described in comment 3?  This is just a straightforward async conversion of nsIContentPrefService's methods (only getPref and setPref so far) rather than a big rethinking of the interface.
Attachment #586655 - Attachment is obsolete: true
Attachment #587149 - Flags: feedback?(mak77)
Comment on attachment 587149 [details] [diff] [review]
cleaned-up work-in-progress

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

I thought (and hoped) there were less consumers of this component, there are quite a few, just searching for nsIContentPrefService and Services.contentPrefs. May not be trivial to convert all of them, but it's surely worth it. Apart Set and Get a see a bunch of calls to RemovePref methods and some to hasPref (that we could just replace with get), btw we can surely proceed in steps.

Considered this, I agree it's better to have a separate async interface and convert consumers to it in steps.
I didn't go too deep in the review, mostly a panoramic check

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +319,5 @@
> +      if (this[n]) {
> +        this[n].finalize();
> +        this[n] = null;
> +      }
> +    }, this);

I think I'd love if you could cleanup this mess and use a caching object for these statements, so we can just walk it. as it is is nonsense. May be a initial cleanup that can land even early in the process and may help you moving in this code.

something like this.getStatement(query) with a _statements[query] hash. I'd also suggest to have a separate asyncStatement version of it, to avoid mixing up the two, we should avoid using a sync statement as async, better creating a separate one.
This would also fix having all the statements getters spread across the code, it's damn confusing.

@@ +788,5 @@
> +      return;
> +    }
> +
> +    doAsyncStatments(function (cps) {
> +      let settingID = yield this.exec(cps._stmtSelectSettingIDAsync, "id",

I'm not sure if this would create bigger issues regarding the implementation, but using dbconn.executeAsync(statements) is better than executing each statement singularly, cause has an implicit transaction (you can also use bindingParamsArray eventually). Usually it may be feasible when you can use subqueries to handle results from the previous statement, but clearly if there are more relations is not feasible.

@@ +792,5 @@
> +      let settingID = yield this.exec(cps._stmtSelectSettingIDAsync, "id",
> +                                      { name: aName });
> +      if (typeof(settingID) != "number") {
> +        yield this.exec(cps._stmtInsertSettingAsync, null, { name: aName });
> +        settingID = cps._dbConnection.lastInsertRowID;

we can't use lastInsertRowID since we have mixed sync/async access, it's not guaranteed to be related to the last query. you'll have to select it or use subqueries in the other queries. So yeah, this may be a problem once we add an asynchronous setter.

@@ +862,5 @@
> +    let runnable = {
> +      run: function CPS_scheduleCallbackRunnableRun() {
> +        func.call(self);
> +      }
> +    }

you can use Services.tm, and nsIRunnable has the function decorator, you may keep the old code and just use .bind()
Attachment #587149 - Flags: feedback?(mak77)
Attached patch part 1: cleanup (obsolete) — Splinter Review
Attachment #587149 - Attachment is obsolete: true
Attachment #590413 - Flags: review?(mak77)
Attached patch part 1: cleanup (obsolete) — Splinter Review
Attachment #590413 - Attachment is obsolete: true
Attachment #590413 - Flags: review?(mak77)
Attachment #590415 - Flags: review?(mak77)
Comment on attachment 590415 [details] [diff] [review]
part 1: cleanup

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

This is so much better that I'm about to cry, though I'd prefer more separation of the sync/async paths in statements creation

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +244,5 @@
>  
> +    ["_statements", "_asyncStatements"].forEach(function (mapName) {
> +      if (this[mapName])
> +        for each (let stmt in this[mapName])
> +          stmt.finalize();

please brace loops, also brace the if since it goes multiple lines (even if it may be onelined)

@@ +700,5 @@
>      let tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
>      tm.mainThread.dispatch(func, Ci.nsIThread.DISPATCH_NORMAL);
>    },
>  
> +  _getStatement: function CPS__getStatement(sql /*, sql2, ... [, isAsync] */) {

I think having a separate getAsyncStatement would be less error prone, we should really do complete separation, to avoid possible contention by misuse, imo. Also in future would be cool to kill the sync part, so separating now is a likely a good approach.

I also think that you may kill _dbCreateStatement and just merge the creation in each of the 2 methods, I don't see what's useful for apart abstracting so much to make me jump up and down in the sources.

I don't have preferences on using multiple arguments or a single string, using multiple args is probably less usual in our codebase, but I see the advantage to not having to deal with whitespaces, so I don't mind :)
Attachment #590415 - Flags: review?(mak77) → review-
Thanks Marco.

The way this component handles private browsing is messed up.  It keeps a separate private-browsing in-memory store in addition to the normal stores (a database and weird in-memory cache), which makes sense.  But during private-browsing mode, the ways these methods work are inconsistent:

getPref
  All stores are searched, starting with the private-browsing store.

hasCachedPref
  Only the private-browsing store is searched.

removePref
  All stores are searched, but it ends up that only the private-browsing store is modified.

removeGroupedPrefs
  All stores are modified.  (BTW, this method doesn't notify observers like the others do.)

removePrefsByName
  All stores are modified.  (Plus, if prefs end up being removed from the normal stores, observers aren't notified about them.)

getPrefs
  Only the private-browsing store is searched.

getPrefsByName
  Only the private-browsing store is searched.

This matters to me because I'm basically reimplementing all of these.  Seems like private-browsing mode should use the private-browsing store only, end of story.
Status: NEW → ASSIGNED
(In reply to Drew Willcoxon :adw from comment #9)
> This matters to me because I'm basically reimplementing all of these.  Seems
> like private-browsing mode should use the private-browsing store only, end
> of story.

Honestly I'm not sure, the usual behavior we use in other components is that in PB mode you can't store things permanently, but you can access data that was saved earlier. What usually changes is either allowing to store things temporarily or disallowing to store completely. But all the data is commonly available, persistent and temporary.
Probably we should concentrate on the use-cases of this component, supposing you use it to make something a-la greasemonkey, even when browsing in pb mode I would like to see my modifications.
Attached patch part 1: cleanup (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #8)
> I think having a separate getAsyncStatement would be less error prone, we
> should really do complete separation, to avoid possible contention by
> misuse, imo.

Yeah, part 2 has _getAsyncStatement, but right now it's easier for _getStatement to take an optional isAsync argument so that _selectPref and _selectGlobalPref (called by getPref, which is already asyncified) can easily get the kind of statements they need based on the presence of aCallback.  And all _getAsyncStatement has to do is return _getStatement(sql, true).
Attachment #590415 - Attachment is obsolete: true
Attachment #592365 - Flags: review?(mak77)
Attached patch part 2 work in progress: async (obsolete) — Splinter Review
Here's a part 2 work in progress just as a status update.  Left to do before review: write tests for private-browsing mode and observers and fix the problems that come up as a result of those tests.
Comment on attachment 592365 [details] [diff] [review]
part 1: cleanup

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

This looks just good.
I trust you correctly copied the statements, didn't check them line by line.
Does this code have decent tests?

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +944,4 @@
>      try {
> +      stmt.params.group = aGroup;
> +      while (stmt.executeStep())
> +        prefs.setProperty(stmt.row.name, stmt.row.value);

brace loop please

@@ +967,3 @@
>      try {
> +      while (stmt.executeStep())
> +        prefs.setProperty(stmt.row.name, stmt.row.value);

brace loop please

@@ +990,4 @@
>      try {
> +      stmt.params.setting = aName;
> +      while (stmt.executeStep())
> +        prefs.setProperty(stmt.row.groupName, stmt.row.value);

brace loop please
Attachment #592365 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #13)
> Does this code have decent tests?

Looks like it, except that after bug 559992 was fixed the tests overwhelmingly exercise the component's in-memory cache as opposed to ensuring that the values returned by the API are the values in the database.  test_contentPrefsCache.js tests the latter but not very rigorously.

So bug 559992 explains the in-memory cache.  I searched mxr for other consumers of nsIContentPrefService, which I should have done first.  Based on all that, I'd like to rewrite the interface to look like this:

  // asynchronously gets values from the database
  void getPrefs(in nsIVariant group,
                in AString name,
                in nsIContentPrefCallback callback,
                [optional] in nsIContentURIGrouper grouper);

  // convenience, like getPrefs but gets only the first value
  void getPref(in nsIVariant group,
               in AString name,
               in nsIContentPrefCallback callback);
               [optional] in nsIContentURIGrouper grouper);

  // synchronously gets values from the cache
  nsIPropertyBag2 getCachedPrefs(in nsIVariant group,
                                 in AString name,
                                 [optional] in nsIContentURIGrouper grouper);

  // convenience, like getCachedPrefs but gets only the first value, returning
  // undefined if not present
  nsIVariant getCachedPref(in nsIVariant group,
                           in AString name,
                           [optional] in nsIContentURIGrouper grouper);

  void setPref(in nsIVariant group,
               in AString name,
               in nsIVariant value,
               [optional] in nsIContentPrefCallback callback);

  void removePrefs(in nsIVariant group,
                   in AString name,
                   [optional] in nsIContentPrefCallback callback,
                   [optional] in nsIContentURIGrouper grouper);

  void removePrefsByName(in AString name,
                         [optional] in nsIContentPrefCallback callback);

  void removePrefsByGroup(in nsVariant group,
                          [optional] in nsIContentPrefCallback callback,
                          [optional] in nsIContentURIGrouper grouper);

  void removeAllGroupedPrefs([optional] in nsIContentPrefCallback callback);

addObserver, removeObserver, and grouper would still be there.

Nobody uses getPrefsByName, so it's gone.

The current getPrefs is only used by nsPrivateBrowsingService.js, which uses it to get the prefs associated with a domain so it can then remove those prefs.  It actually first queries the raw DB connection to get all the groups that are LIKE the domain -- all the groups that are subdomains.  So really what it needs is a removePrefsByGroup in conjunction with a custom grouper.

The reason getPrefs, getCachedPrefs, and removePrefs are plural is that the given group may match multiple groups in the database depending on the given grouper.  Group A matches group B if grouper(A) == grouper(B).  The default grouper remains the "hostname grouper", i.e., grouper(A) == A.host if A is an nsIURI, but consumers like nsPrivateBrowsingService may need to provide custom groupers.

I'm not sure how I'd implement custom groupers at the SQL level, but I'm thinking of registering custom SQLite functions per grouper.  Either that or modify nsIContentURIGrouper so that its instances are essentially defined by regular expressions.

Alternatively the whole notion of these generalized groups could be trashed in favor of domains and subdomains.  Then the methods could take an includeSubdomains bool instead of a grouper.  Or maybe better a flag whose possible values would be DOMAIN and SUBDOMAINS; more values like ENTIRE_URI could be added later if necessary.

What does everybody think?
(In reply to Drew Willcoxon :adw from comment #14)
> Alternatively the whole notion of these generalized groups could be trashed
> in favor of domains and subdomains.  Then the methods could take an
> includeSubdomains bool instead of a grouper.  Or maybe better a flag whose
> possible values would be DOMAIN and SUBDOMAINS; more values like ENTIRE_URI
> could be added later if necessary.

The flags approach sounds much simpler.
Attached patch work-in-progress rewrite (obsolete) — Splinter Review
This implements the idea in comment 14.  The interface is:

interface nsIContentPrefService2 : nsISupports
{
  // asynchronously gets values from the database
  void getPrefs(in nsIVariant group,
                in AString name,
                in nsIContentPrefCallback callback,
                [optional] in boolean includeSubdomains);

  // convenience, like getPrefs but gets only the first value
  void getPref(in nsIVariant group,
               in AString name,
               in nsIContentPrefCallback callback,
               [optional] in boolean includeSubdomains);

  // synchronously gets values from the cache
  nsIPropertyBag2 getCachedPrefs(in nsIVariant group,
                                 in AString name,
                                 [optional] in boolean includeSubdomains);

  // convenience, like getCachedPrefs but gets only the first value, returning
  // undefined if not present
  nsIVariant getCachedPref(in nsIVariant group,
                           in AString name,
                           [optional] in boolean includeSubdomains);

  void setPref(in nsIVariant group,
               in AString name,
               in nsIVariant value,
               [optional] in nsIContentPrefCallback callback);

  void removePrefs(in nsIVariant group,
                   in AString name,
                   [optional] in nsIContentPrefCallback callback,
                   [optional] in boolean includeSubdomains);

  void removePrefsWithName(in AString name,
                           [optional] in nsIContentPrefCallback callback);

  void removePrefsWithGroup(in nsIVariant group,
                            [optional] in nsIContentPrefCallback callback,
                            [optional] in boolean includeSubdomains);

  void removeAllGroupedPrefs([optional] in nsIContentPrefCallback callback);

  void addObserverForName(in AString name,
                          in nsIContentPrefObserver observer);

  void removeObserverForName(in AString name,
                             in nsIContentPrefObserver observer);
};

The includeSubdomains bool forces a simple suffix comparison using LIKE in the SQL, which can be fooled but which is what nsPrivateBrowsingService.js is already doing.  getPrefs("example.com", "foo", fn, true) yields prefs named "foo" in groups example.com, a.example.com, b.a.example.com, etc.

At first I started on a more sophisticated subdomain matcher using a custom SQLite function, but I hit bug 649151.  So then I wrote the same function but in a binary component, but it turns out that ioService.newURI can only be called on the main thread.

Left to do: implement getCachedPrefs, implement private-browsing awareness, improve the tests I've already written, write tests I haven't yet written, polish and refactor.
Attached patch work-in-progress rewrite (obsolete) — Splinter Review
Asking for feedback on the new interface.  I'd ask for review but private-browsing awareness isn't there yet, and the test isn't finished, huge as it is.

(In reply to Drew Willcoxon :adw from comment #16)
>   // convenience, like getCachedPrefs but gets only the first value,
>   // returning undefined if not present
>   nsIVariant getCachedPref(in nsIVariant group,
>                            in AString name,
>                            [optional] in nsIContentURIGrouper grouper);

Reading bug 559992 more closely, I see this isn't right.  Callers need to be able to distinguish between (a) the pref may or may not exist, it's not in the cache, and (b) the pref does not exist.  undefined by itself doesn't do that.  So I changed it to:

  bool getCachedPref(in nsIVariant group,
                     in AString name,
                     out nsIVariant value,
                     [optional] in boolean includeSubdomains);

It returns true if the pref is cached (even if it's undefined) and false otherwise.  This seems better to me than having getCachedPref() return the pref and then a separate isPrefCached(), since if you don't notice isPrefCached you can miss the nuance of what undefined means, or you might just forget to call it.  The single method puts you on the right path.
Attachment #594397 - Attachment is obsolete: true
Attachment #595296 - Flags: feedback?(mak77)
Attached patch work-in-progress rewrite (obsolete) — Splinter Review
Getting closer... private-browsing awareness is done.  I think only some polish and maybe more test additions are left.
Attachment #595296 - Attachment is obsolete: true
Attachment #595296 - Flags: feedback?(mak77)
The api looks great. A dumb question, but I didn't look at all the reasoning behind the cache, if everything is async, the synchronous cache on the first hit won't have a value, how do we manage that? Is it up to the consumer?
(In reply to Marco Bonardo [:mak] from comment #19)
> Is it up to the consumer?

Yeah, for example http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullZoom.js#241
interesting, so the consumer has to handle both synchronous and aynchronous code paths, sounds expensive and more bugs prone, though I suppose we can't make all consumers purely async :(
We have a similar problem for DOMStorage, it has to be synchronous for the specs, but we don't want mainthread IO, in that case we are evaluating 2 solutions, one of which is a ScriptBlocker to block the script while we cache the data asynchronously (but doesn't look feasible here, and it may still not be fine for our chrome usage of it), the other one involves a complex multi-connections handling that has to keep reads on mainthread.
Sounds like a pretty common problem.
Most consumers should only use the async methods, knock on wood.  The reason for providing the sync cache getters isn't to punt on callers that are hard to convert to async.  As I understand it, it's to provide an optimized but fallible fast path in cases where the UX would suffer by waiting until the next turn of the event loop to do something, like changing the zoom level when the page loads.

Actually it looks like the only caller of hasCachedPref is browser-fullZoom.js.
Attached patch patch (obsolete) — Splinter Review
I didn't need to build on the earlier cleanup patch since I've implemented the new iface in a separate file.  If you still want to take it, we can move it to bug 696499.
Attachment #592365 - Attachment is obsolete: true
Attachment #592366 - Attachment is obsolete: true
Attachment #595626 - Attachment is obsolete: true
Attachment #595948 - Flags: review?(mak77)
(In reply to Drew Willcoxon :adw from comment #23)
> I didn't need to build on the earlier cleanup patch since I've implemented
> the new iface in a separate file.  If you still want to take it, we can move
> it to bug 696499.

Yes please, that cleanup really helps reading the code.

I'll do this review tomorrow.
Comment on attachment 595948 [details] [diff] [review]
patch

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

as a general comment on the approach, what I would really like is that js consumers can just import the new module and use a ContentPrefs object, without having to getService the old one and QI it, or even havin to make a new object (should work like a service after all).

Note that I'm not yet gone through ContentPrefService2.js, though since my actual comments may require some change there, may be better to go through it once these are answered/addressed.

::: dom/interfaces/base/nsIContentPrefService.idl
@@ +69,3 @@
>  interface nsIContentPrefCallback : nsISupports
>  {
> +  void onResult([optional] in nsIVariant aResult);

this may want a small javadoc explaining when is it expected to get a result or not.

@@ +72,4 @@
>  };
>  
>  [scriptable, uuid(0014e2b4-1bab-4946-9211-7d28fc8df4d7)]
>  interface nsIContentPrefService : nsISupports

Could we mark this deprecated? at least with a javadoc @deprecated pointing to nsIContentPrefService2.

::: dom/interfaces/base/nsIContentPrefService2.idl
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsIContentPrefService.idl"

count you just declare the interfaces you use instead of including?

@@ +10,5 @@
> +  /**
> +   * Gets a pref.
> +   *
> +   * @param domain            The pref's domain as an nsIURI, a string, or null.
> +   *                          Null gets the pref that applies to all domains.

Looks like internally if we get a string we just convert it to an nsIURI, so why don't we just get an nsIURI instead of complicating our life with nsIVariant?

@@ +17,5 @@
> +   *                          domain and name is retrieved.  It's passed the
> +   *                          value of the pref as an nsIVariant.  If no such
> +   *                          pref exists, the value will be undefined.
> +   * @param includeSubdomains If true and the given domain is an nsIURI or a
> +   *                          string that represents a URI or domain name, this

so basically if a domain has been specified...

@@ +22,5 @@
> +   *                          method retrieves all prefs with the given name
> +   *                          whose domains either match or are subdomains of
> +   *                          the given domain.  Only one value is ever passed
> +   *                          to the callback, however, and if multiple prefs
> +   *                          are retrieved, any of those prefs may be passed.

so I get back a random pref with that name? What's the use-case covered by this?
Maybe the option should not exist here and if the user needs to use it he will have to use getMany.

@@ +26,5 @@
> +   *                          are retrieved, any of those prefs may be passed.
> +   *                          Defaults to false.
> +   */
> +  void get(in nsIVariant domain,
> +           in AString name,

any specific reason this can't be an ACString?

@@ +58,5 @@
> +   */
> +  void getMany(in nsIVariant domain,
> +               in AString name,
> +               in nsIContentPrefCallback callback,
> +               [optional] in boolean includeSubdomains);

basically same comments as above, not going to repeat them later.

Though, if includeSubdomains is false, how many opportunities I have to get multiple prefs? To me looks like includeSubdomains should default to false in get and to true in getMany, and not even be a param.

So
get(domain, name, callback);
and
getSubDomains(domain, name, callback);

would this leave out some existing use-case?
the same comment would be valid for the cached version

@@ +86,5 @@
> +   *                          through the out-parameter, however, and if
> +   *                          multiple prefs are retrieved, any of those prefs
> +   *                          may be returned.  Defaults to false.
> +   *
> +   * @returns True if the pref is cached or known not to exist.  False

global: @return (no final s)

@@ +134,5 @@
> +  /**
> +   * Sets a pref.
> +   *
> +   * The new value will not be reflected in the store until the given callback
> +   * is called.

Though, if I set() and get() everything will work fine since all requests are serialized, right? may be worth a comment, since as it is looks like I have to wait the callback before ever being able to do a get()

@@ +141,5 @@
> +   *                 sets the pref that applies to all domains.
> +   * @param name     The pref's name as a nonempty string.
> +   * @param value    The value to store for the given domain and name.
> +   * @param callback Asynchronously called when the pref has been stored.  It
> +   *                 takes no arguments.

is there a reason to not just return the pref itself to the callback? it may be handy, suppose I want to use a shared handler and differentiate on the input it gets

@@ +152,5 @@
> +  /**
> +   * Removes one or more prefs.
> +   *
> +   * The removals will not be reflected in the store until the given callback is
> +   * called.

as above, worth to specify if I get() after this call it will work fine and I don't need to wait the callback

@@ +174,5 @@
> +  /**
> +   * Removes the prefs with the given domain.
> +   *
> +   * The removals will not be reflected in the store until the given callback is
> +   * called.

ehr, rather than adding this to each single method, I'd suggest to add a note about asynchronous behavior in a comment before the interface.

@@ +191,5 @@
> +                      [optional] in nsIContentPrefCallback callback,
> +                      [optional] in boolean includeSubdomains);
> +
> +  /**
> +   * Removes all prefs that are associated with a nonnull domain.

why do you have to specify nonnull domain? can we have a null domain? then those prefs would be immortal :)

@@ +212,5 @@
> +   * @param callback Asynchronously called when the prefs have been removed.
> +   *                 It takes no arguments.
> +   */
> +  void removeByName(in AString name,
> +                    [optional] in nsIContentPrefCallback callback);

why does this exist, can't I use removeByDomain passing a null domain to achieve the same result?

@@ +216,5 @@
> +                    [optional] in nsIContentPrefCallback callback);
> +
> +  /**
> +   * Registers an observer that will be notified whenever a pref with the given
> +   * name is set or removed.

please specify if this involves strong ownership (and thus any observer should be removed to avoid leaks)

::: toolkit/components/contentprefs/ContentPrefStore.js
@@ +97,5 @@
> +      else if (this._groups.hasOwnProperty(group))
> +        yield group;
> +    }
> +    else if (Object.keys(this._globalNames).length)
> +      yield null;

global: when one side of an if/else(if) is braced please brace all sides

::: toolkit/components/contentprefs/Makefile.in
@@ +44,5 @@
>  MODULE = contentprefs
>  
>  EXTRA_COMPONENTS = nsContentPrefService.js nsContentPrefService.manifest
>  
> +EXTRA_JS_MODULES = ContentPrefService2.js ContentPrefStore.js

please make these properly newlined

per convention I'd prefer if we name modules as .jsm

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +143,5 @@
>    } catch (e) {}
>    this._observerSvc.addObserver(this, "private-browsing", false);
>  
>    // Observe shutdown so we can shut down the database connection.
>    this._observerSvc.addObserver(this, "xpcom-shutdown", false);

sigh, this is plain wrong... could you please file a bug to move this to a proper profile-before-change?

@@ +188,5 @@
> +        let s = {};
> +        Cu.import("resource://gre/modules/ContentPrefService2.js", s);
> +        this._contentPrefService2 = new s.ContentPrefService2(this);
> +      }
> +      return this._contentPrefService2;

Its a bit unfortunate that we have to make CPS2 take CPS1 as argument, means js consumers can't just import the module and use it... could we rather add a lazy getter service reference inside the module object so js consumers don't have to explicitly create CPS1 to use CPS2?
And if so, you may just return this._contentPrefService2; here and in the constructor defineLazyModuleGetter it, to remove some code from here making the QI more readable.

How do you plan future removal of nsIContentPrefService, with a xpcom shim to allow cpp callers to call CPS2?

::: toolkit/components/contentprefs/tests/unit/test_contentPrefs2.js
@@ +1225,5 @@
> +  do_test_pending();
> +
> +  Cc["@mozilla.org/preferences-service;1"].
> +    getService(Ci.nsIPrefBranch).
> +    setBoolPref("browser.privatebrowsing.keep_current_session", true);

please use Services.prefs

@@ +1256,5 @@
> +  tests.forEach(function (test) {
> +    function gen() {
> +      do_print("Running " + test.name);
> +      yield test();
> +      cps.wrappedJSObject.reset();

please, do not expose wrappedJSObject. I'd prefer if you add an nsIObserver interface and pass test notifications (the object doesn't even have to observe them, you can directly call into it)

@@ +1332,5 @@
> +  do_check_true(areEqual);
> +}
> +
> +function dbOK(expectedRows) {
> +  let stmt = cps.wrappedJSObject._cps.DBConnection.createStatement(

couldn't you just use cps.DBConnection here?

@@ +1365,5 @@
> +  let actualRows = [];
> +  while (stmt.executeStep()) {
> +    actualRows.push([stmt.row.grp, stmt.row.name, stmt.row.value]);
> +  }
> +  stmt.reset();

could this be refactored to use async Storage?

@@ +1451,5 @@
> +  //   if (!pb) {
> +  //     yield true;
> +  //     return;
> +  //   }
> +  cps.wrappedJSObject._schedule(function () next(pb));

just enqueue with Services.tm
Attachment #595948 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #25)
> >  interface nsIContentPrefService : nsISupports
> 
> Could we mark this deprecated? at least with a javadoc @deprecated pointing
> to nsIContentPrefService2.

I wondered about that, and I think not until we're confident the new interface meets the needs of all mozilla-central consumers.  What do you think?

Actually I don't want to even land this without having started on all the consumer rewrite patches.  I looked at them and think the new interface is OK, but you never know until you start writing.

> > +  /**
> > +   * Gets a pref.
> > +   *
> > +   * @param domain            The pref's domain as an nsIURI, a string, or null.
> > +   *                          Null gets the pref that applies to all domains.
> 
> Looks like internally if we get a string we just convert it to an nsIURI, so
> why don't we just get an nsIURI instead of complicating our life with
> nsIVariant?

CPS only uses string domains internally, so it converts nsIURIs to host names.  It tries to convert string arguments to nsIURIs but only so it can then convert them to host names, and failing that it just treats the strings as host names.

Originally I planned for nsIURI-only parameters, but the private browsing service passes in string domains when it removes content prefs for a domain.  It would be easy to modify that one caller, but looking at the larger picture, it's not nice to force callers that only have string domains to construct bogus nsIURIs when CPS works entirely off of domains and not full URIs anyway.

I could also see an argument for accepting only strings.  Really the fact that CPS takes nsIURIs is just a convenience for callers, so they don't have to get the host name first.

> > +   * @param includeSubdomains If true and the given domain is an nsIURI or a
> > +   *                          string that represents a URI or domain name, this
> 
> so basically if a domain has been specified...

You could pass in "my crazy extension" as the domain, too.  Should we prohibit that?

> so I get back a random pref with that name? What's the use-case covered by
> this?
> Maybe the option should not exist here and if the user needs to use it he
> will have to use getMany.

OK.

> > +  void get(in nsIVariant domain,
> > +           in AString name,
> 
> any specific reason this can't be an ACString?

I guess not...

> Though, if includeSubdomains is false, how many opportunities I have to get
> multiple prefs? To me looks like includeSubdomains should default to false in
> get and to true in getMany, and not even be a param.

Yeah, I thought about that, but if you don't know in advance whether you need to include subdomains, then you have to write two paths, one for get() and one for getMany().  But that's probably an uncommon case, so I'll make your changes.

> > +  /**
> > +   * Sets a pref.
> > +   *
> > +   * The new value will not be reflected in the store until the given callback
> > +   * is called.
> 
> Though, if I set() and get() everything will work fine since all requests
> are serialized, right? may be worth a comment, since as it is looks like I
> have to wait the callback before ever being able to do a get()

I think I'll just remove this comment and the others like it.

> > +   *                 sets the pref that applies to all domains.
> > +   * @param name     The pref's name as a nonempty string.
> > +   * @param value    The value to store for the given domain and name.
> > +   * @param callback Asynchronously called when the pref has been stored.  It
> > +   *                 takes no arguments.
> 
> is there a reason to not just return the pref itself to the callback? it may
> be handy, suppose I want to use a shared handler and differentiate on the
> input it gets

OK.  Which of these should the callback take: domain, name, value?

What about the callbacks of the other methods?

> > +  void removeByName(in AString name,
> > +                    [optional] in nsIContentPrefCallback callback);
> 
> why does this exist, can't I use removeByDomain passing a null domain to
> achieve the same result?

No, removeByDomain(null) only removes global prefs.  removeByName(name) removes all prefs with the given name, regardless of whether they're global or have a domain.  I'll try to clarify in the comments.

> ::: toolkit/components/contentprefs/nsContentPrefService.js
> @@ +143,5 @@
> >    } catch (e) {}
> >    this._observerSvc.addObserver(this, "private-browsing", false);
> >  
> >    // Observe shutdown so we can shut down the database connection.
> >    this._observerSvc.addObserver(this, "xpcom-shutdown", false);
> 
> sigh, this is plain wrong... could you please file a bug to move this to a
> proper profile-before-change?

bug 727996

> Its a bit unfortunate that we have to make CPS2 take CPS1 as argument, means
> js consumers can't just import the module and use it... could we rather add
> a lazy getter service reference inside the module object so js consumers
> don't have to explicitly create CPS1 to use CPS2?

I'm not following.  I don't see why Cc["@mozilla.org/content-pref/service;1"].getService(Ci.nsIContentPrefService2) isn't OK.

The only reason CPS2 is a JS module is I wanted to implement it in a new file but needed to return it through the @mozilla.org/content-pref/service;1 component.  It's just an implementation detail.

> How do you plan future removal of nsIContentPrefService, with a xpcom shim
> to allow cpp callers to call CPS2?

Are you assuming that CPS2 would be exported through a JS module?  I'm saying it would still be XPCOM, and it would stop being a JS module at all since CPS1 doesn't need it anymore.

The parts of CPS1 that CPS2 relies on would be moved to CPS2 -- the DB connection, the cache, the private-browsing store, observer stuff.

> > +  Cc["@mozilla.org/preferences-service;1"].
> > +    getService(Ci.nsIPrefBranch).
> > +    setBoolPref("browser.privatebrowsing.keep_current_session", true);
> 
> please use Services.prefs

ugh

> please, do not expose wrappedJSObject. I'd prefer if you add an nsIObserver
> interface and pass test notifications (the object doesn't even have to
> observe them, you can directly call into it)

I don't understand what the parenthetical means.  Could you explain more?

And how is communicating through an observer interface better than wrappedJSObject?

> > +function dbOK(expectedRows) {
> > +  let stmt = cps.wrappedJSObject._cps.DBConnection.createStatement(
> 
> couldn't you just use cps.DBConnection here?

I don't think so, since cps is the CPS2 object, which doesn't have a DBConnection property.

> > +  while (stmt.executeStep()) {
> > +    actualRows.push([stmt.row.grp, stmt.row.name, stmt.row.value]);
> > +  }
> > +  stmt.reset();
> 
> could this be refactored to use async Storage?

For I/O reasons?  It's just a test, sync is fine.  Or because the sync API will be deprecated at some point?

> > +  cps.wrappedJSObject._schedule(function () next(pb));
> 
> just enqueue with Services.tm

ugh
(In reply to Drew Willcoxon :adw from comment #26)
> I wondered about that, and I think not until we're confident the new
> interface meets the needs of all mozilla-central consumers.  What do you
> think?

Well, it's fine if we want to wait some internal conversion, just that we should deprecate as soon as we are confident enough. The basic problem is that we don't want new consumers of the old API, so we should not wait months.

> I could also see an argument for accepting only strings.  Really the fact
> that CPS takes nsIURIs is just a convenience for callers, so they don't have
> to get the host name first.

heh, I see your point. ok, after all the complication is internal, we can handle it.

> > > +   *                 sets the pref that applies to all domains.
> > > +   * @param name     The pref's name as a nonempty string.
> > > +   * @param value    The value to store for the given domain and name.
> > > +   * @param callback Asynchronously called when the pref has been stored.  It
> > > +   *                 takes no arguments.
> > 
> > is there a reason to not just return the pref itself to the callback? it may
> > be handy, suppose I want to use a shared handler and differentiate on the
> > input it gets
> 
> OK.  Which of these should the callback take: domain, name, value?

doh! a good callback would get all of them! I forgot this one only gets one. Usually setters return the set value, but that's not really usable to differentiate... maybe you should just ignore my suggestion :(
Or we should make a better callback interface that gets enough data on what is set or get.

> > Its a bit unfortunate that we have to make CPS2 take CPS1 as argument, means
> > js consumers can't just import the module and use it... could we rather add
> > a lazy getter service reference inside the module object so js consumers
> > don't have to explicitly create CPS1 to use CPS2?
> 
> I'm not following.  I don't see why
> Cc["@mozilla.org/content-pref/service;1"].getService(Ci.
> nsIContentPrefService2) isn't OK.
> 
> The only reason CPS2 is a JS module is I wanted to implement it in a new
> file but needed to return it through the @mozilla.org/content-pref/service;1
> component.  It's just an implementation detail.

Ok, I had not clear your plan, I thought you wanted to make it a module in future and just provide a xpcom shim for cpp callers. If the plan is to have the module until we remove the old implementation nvm my comments regarding the module.

> > please, do not expose wrappedJSObject. I'd prefer if you add an nsIObserver
> > interface and pass test notifications (the object doesn't even have to
> > observe them, you can directly call into it)
> 
> I don't understand what the parenthetical means.  Could you explain more?

just that you don't have to addObserver test notifications.

> And how is communicating through an observer interface better than
> wrappedJSObject?

it's exposing internals when wrapped rather than just the interface, if it's avoidable would be better.

> > > +function dbOK(expectedRows) {
> > > +  let stmt = cps.wrappedJSObject._cps.DBConnection.createStatement(
> > 
> > couldn't you just use cps.DBConnection here?
> 
> I don't think so, since cps is the CPS2 object, which doesn't have a
> DBConnection property.

hm, you can getService the old interface though?

> > > +  while (stmt.executeStep()) {
> > > +    actualRows.push([stmt.row.grp, stmt.row.name, stmt.row.value]);
> > > +  }
> > > +  stmt.reset();
> > 
> > could this be refactored to use async Storage?
> 
> For I/O reasons?  It's just a test, sync is fine.  Or because the sync API
> will be deprecated at some point?

No, not for IO reasons, I think that we should try to limit the use of synchronous storage. I don't think it will be deprecated, though I think one day it will become single-threaded, so you'll have a pure-synchronous connection and a pure-asynchronous one. in this case you would have a pure-asynchronous one, that is the service one.
(In reply to Marco Bonardo [:mak] from comment #27)
> doh! a good callback would get all of them! I forgot this one only gets one.
> Usually setters return the set value, but that's not really usable to
> differentiate... maybe you should just ignore my suggestion :(

Hmm, but it might be a good idea:

             get: value, domain, name
         getMany: propertyBag, domain, name
             set: value, domain, name
          remove: propertyBag, domain, name
  removeByDomain: propertyBag, domain
removeAllDomains: propertyBag
    removeByName: propertyBag, name

All the callbacks take the value or property bag first so if you're only interested in values, you don't have to type the other parameters.

For the removal methods, the property bags contain (domain, value) properties, one property per removed pref.  So the removal methods are a combination get-remove.

Overkill?  There are observers too, which serve a similar purpose.

> just that you don't have to addObserver test notifications.

Oh I see, cool.

> > > couldn't you just use cps.DBConnection here?
> > 
> > I don't think so, since cps is the CPS2 object, which doesn't have a
> > DBConnection property.
> 
> hm, you can getService the old interface though?

I ended up returning the connection via your observer trick so the test doesn't have to rely on CPS1 to get it.

> so you'll have a pure-synchronous connection and a
> pure-asynchronous one. in this case you would have a
> pure-asynchronous one, that is the service one.

Interesting, thanks for explaining.
(In reply to Marco Bonardo [:mak] from comment #25)
> > +  void get(in nsIVariant domain,
> > +           in AString name,
> 
> any specific reason this can't be an ACString?

Why ACString? It's less efficient for JS callers, and doesn't seem to matter much to C++ callers (it's not e.g. a hostname, where nsIURI would return an AUTF8String).
(In reply to Drew Willcoxon :adw from comment #26)
> > please use Services.prefs
> > just enqueue with Services.tm
> 
> ugh

Why the "ugh"? :)
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #29)
> (In reply to Marco Bonardo [:mak] from comment #25)
> > > +  void get(in nsIVariant domain,
> > > +           in AString name,
> > 
> > any specific reason this can't be an ACString?
> 
> Why ACString? It's less efficient for JS callers, and doesn't seem to matter
> much to C++ callers (it's not e.g. a hostname, where nsIURI would return an
> AUTF8String).

I thought the rule of thumb was to go for the smallest string type, if the current trend is to go for the more performance js string AString is fine.
Attached patch patch (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #25)
> any specific reason this can't be an ACString?

After the conversation here I kept them all AStrings.

(In reply to Marco Bonardo [:mak] from comment #27)
> doh! a good callback would get all of them! I forgot this one only gets one.

This patch passes all the arguments you passed to the method to the method's callback so you can differentiate invocations.  Of course for the getters, callbacks are also passed the gotten values -- as the first argument, so that in JS you don't have to specify the others if you don't need them.

The giant test maybe should be split into separate files in a new cps2 directory, especially considering that CPS1 and its tests will be removed in the future.  What do you think?
Attachment #595948 - Attachment is obsolete: true
Attachment #600572 - Flags: review?(mak77)
(In reply to Drew Willcoxon :adw from comment #32)
> The giant test maybe should be split into separate files in a new cps2
> directory, especially considering that CPS1 and its tests will be removed in
> the future.  What do you think?

Well, long tests are bad for a couple reasons:
1. maintenance costs
2. timeout on slow platforms and they get disabled, causing loss of coverage

So sounds like a good proposal to me.
Comment on attachment 600572 [details] [diff] [review]
patch

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

::: dom/interfaces/base/nsIContentPrefService2.idl
@@ +20,5 @@
> +   *   value, domain, name
> +   *
> +   * where value is the value of the preference with the given domain and name,
> +   * and domain and name are the given domain and name.  If no such preference
> +   * exists, value will be undefined (nsIDataType::VTYPE_VOID).

nit: At first I had difficulties reading this, due to the newlines causing phrase interruptions (I just read "the callback is passed"... so what?), maybe it's just me though. Would prefer something like
The callback is passed: value, domain, name.
Value is the...

Though, all of this should be moved in the @param callback description, imo.
And I think this is valid for all callbacks descriptions in the interface.
If you want to gain more space you can indent javadocs as
@param name
       description

@@ +236,5 @@
> +   * will be undefined (nsIDataType::VTYPE_VOID).
> +   */
> +  void onResult(in nsIVariant param1,
> +                in nsIVariant param2,
> +                in nsIVariant param3);

Honestly, having random order of params looks error-prone and causes the implementer to check each single method documentation every time, compared to a callback with fixed positions.
things we may alternatively do here:
1. fixed positions, like (value, name, domain, include) and just pass null where they don't make sense
2. pass an nsIContentPref2Result object containing those attributes (eventually we could also make a nsIContentPref2Observer taking the same object)
3. pass the same arguments we give to nsIContentPrefObserver methods, for coherence. If those are enough there, why would not be the same here?

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +11,5 @@
> +const Cr = Components.results;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/ContentPrefStore.jsm");

ContentPrefStore may be a LazyModuleGetter

@@ +84,5 @@
> +    let stmt = this._stmt(joinArgs(Array.slice(arguments, 2)).replace("$",
> +      "SELECT id " +
> +      "FROM groups " +
> +      "WHERE name = :group OR " +
> +            "(:includeSubdomains AND name LIKE :pattern ESCAPE '/')"

ot: the fact this table doesn't use rev_host sucks pretty much. Could be so much faster :(

@@ +150,5 @@
> +      "SELECT id, name FROM settings WHERE name = :name",
> +      "UNION",
> +      "SELECT NULL, :name",
> +      "ORDER BY id DESC",
> +      "LIMIT 1"

could you rewrite this as a simpler

INSERT OR REPLACE INTO settings (id, name)
VALUES((SELECT id FROM settings WHERE name = :name"), :name) ?

@@ +163,5 @@
> +        "SELECT id, name FROM groups WHERE name = :group",
> +        "UNION",
> +        "SELECT NULL, :group",
> +        "ORDER BY id DESC",
> +        "LIMIT 1"

and ditto here

@@ +190,5 @@
> +             "(SELECT id FROM groups WHERE name = :group),",
> +             "(SELECT id FROM settings WHERE name = :name),",
> +             ":value",
> +      "ORDER BY id DESC",
> +      "LIMIT 1"

and here.

@@ +543,5 @@
> +}
> +
> +function checkCPCallback(callback) {
> +  if (!callback)
> +    throw invalidArg("callback must be given.");

rather just check aCallback instanceof nsIContentPref2Callback

@@ +550,5 @@
> +function invalidArg(msg) {
> +  return Components.Exception(msg, Cr.NS_ERROR_INVALID_ARG);
> +}
> +
> +function tryCatch(receiver, fn, args, rethrow) {

I think this hides a bit too much of the code flow. there are just 3 uses that may well just do a common try catch

@@ +561,5 @@
> +      throw err;
> +  }
> +}
> +
> +function tryCatchCPCallback(callback, args) {

we usually prefix these with "safe", like safeCPCallback

::: toolkit/components/contentprefs/tests/unit/test_contentPrefs2.js
@@ +1485,5 @@
> +    },
> +    handleError: function (err) {
> +      do_throw(err);
> +    }
> +  });

please finalize() the statement after executeAsync.

@@ +1563,5 @@
> +  }
> +  catch (err) {
> +    threw = true;
> +  }
> +  do_check_true(threw);

you may just do_throw() after fn(); instead of checking a bool
Attachment #600572 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #34)
> > +  void onResult(in nsIVariant param1,
> > +                in nsIVariant param2,
> > +                in nsIVariant param3);
> 
> Honestly, having random order of params looks error-prone and causes the
> implementer to check each single method documentation every time, compared
> to a callback with fixed positions.
> things we may alternatively do here:
> 1. fixed positions, like (value, name, domain, include) and just pass null
> where they don't make sense
> 2. pass an nsIContentPref2Result object containing those attributes
> (eventually we could also make a nsIContentPref2Observer taking the same
> object)

I don't think these are less error-prone than the way the patch does it.  You would still have to check each method's documentation to see which parameters or result object properties are relevant.

Option 1 is pretty bad because it has to include all possible callback params for every CPS method.  And you left one out, valuesPropertyBag, so it would actually have to be:

  value, valuesPropertyBag, domain, name, includeSubdomains

If we ever added a new method whose callback has different params, we'd have to include them too.  We'd have to update the implementations of all the methods to pass more args, and callers would have to update all their callbacks even if they don't use the new method.

Option 2 basically has the same problem, except it's not as bad since it doesn't break the implementation or callers.  But you'd still end up with a grab bag of values -- properties in this case -- whose connections to each other and to CPS methods are not specified in code.

The one advantage that I see of both options is that the params or properties would be typed instead of nsIVariants, which would be easier on C++ callers.

How about a callback interface per method?  So for get(), the callback would be:

  [function]
  interface nsIContentPref2GetCallback {
    void onResult(in nsIVariant value, nsIVariant domain, in AString name);
  };

This way the connections between methods, callbacks, and callback params are clear, the params are typed, and it mostly documents itself.

> 3. pass the same arguments we give to nsIContentPrefObserver methods, for
> coherence. If those are enough there, why would not be the same here?

Well, the point here is to pass the args you passed to a method to the method's callback in addition to the gotten value(s) for the getters.  The observer callbacks don't cover all that.

> > +function tryCatch(receiver, fn, args, rethrow) {
> 
> I think this hides a bit too much of the code flow. there are just 3 uses
> that may well just do a common try catch

I have been sent from the future to terminate all duplicated code.

> @@ +1563,5 @@
> > +  }
> > +  catch (err) {
> > +    threw = true;
> > +  }
> > +  do_check_true(threw);
> 
> you may just do_throw() after fn(); instead of checking a bool

But then in the success case the test doesn't record that the function correctly threw an exception, and if you'll let me get philosophical, the set of possible assertions differs between passing and failing runs of the test.
(In reply to Drew Willcoxon :adw from comment #35)
> How about a callback interface per method?

Marco, what do you think?
ehr sorry for late, I missed requests in the middle of bugmails and thunderbird mail corruptions, next time, if possible, please feedback? me on a patch, since those are really easy to track, or ping me if I don't answer in a max of a couple days.

(In reply to Drew Willcoxon :adw from comment #35)
> Option 1 is pretty bad because it has to include all possible callback
> params for every CPS method.  And you left one out, valuesPropertyBag, so it
> would actually have to be:
> 
>   value, valuesPropertyBag, domain, name, includeSubdomains

ugh, yeah, that's a lot of stuff.

> Option 2 basically has the same problem, except it's not as bad since it
> doesn't break the implementation or callers.  But you'd still end up with a
> grab bag of values -- properties in this case -- whose connections to each
> other and to CPS methods are not specified in code.

for this option we may even go deeper and, ugh, accept a jsval, that would allow to ignore missing attributes in the object. Though cpp callers would not be too happy about that. It's also true we only have a couple cpp consumers and making a conversion helper would not be crazily hard.

> How about a callback interface per method?  So for get(), the callback would
> be:
> 
>   [function]
>   interface nsIContentPref2GetCallback {
>     void onResult(in nsIVariant value, nsIVariant domain, in AString name);
>   };
> 
> This way the connections between methods, callbacks, and callback params are
> clear, the params are typed, and it mostly documents itself.

May work for me, since we don't have infinite cases, so how many callbacks we'd have? 3?

> I have been sent from the future to terminate all duplicated code.

lol

> But then in the success case the test doesn't record that the function
> correctly threw an exception, and if you'll let me get philosophical, the
> set of possible assertions differs between passing and failing runs of the
> test.

philosophical ok.
Attached patch patch (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #37)
> > How about a callback interface per method?
> 
> May work for me, since we don't have infinite cases, so how many callbacks
> we'd have? 3?

7, one per async method.  (That's what I'm requesting feedback on for this patch.)
Attachment #600572 - Attachment is obsolete: true
Attachment #608091 - Flags: feedback?(mak77)
may we merge some of those?
The first 3 have similar arguments order and meaning
4-5 may both get domain name include, where 5 would have empty name (maybe could be moved as last argument and made optional)
6-7 may have optional name
Personally I care most about arguments order and meaning staying constant, we'd reduce those to 3.
hm though the relation between remove callbacks would be poor...
Right, and if N methods share a callback iface, if we had to change one of those methods but not the others, we'd have to break apart the callback iface and modify the other method signatures, too.
So, my thought was that we may have just 2 callbacks:
nsIContentPrefCallback(value, domain, name)
nsIContentPrefRemoveCallback (name, domain, includeSubdomain)
and just null the unused arguments.
Though you have a point there, about future changes.  It's even true it's unlikely we move really far from those in future.
Since regardless we will need SR on this stuff, could be worth to extend the feedback request to a superreviewer (gavin may want to look at this since he did some work on this in the past?), both to get some fresh ideas and to avoid some useless work in implementing something that may be rejected.
Comment on attachment 608091 [details] [diff] [review]
patch

So, the problem is:
- we need callbacks
- would like callbacks to be reusable (so the same callback being usable for multiple calls)
- would like to avoid having tens of arguments
- would prefer not having moving arguments (so each argument is a variant and may be anything depending on the calling function)
- having 7 different callbacks is a bit over-engineered but it's also the less confusing
- would like pre-SR on an approach to avoid having issues later

There is a bit of previous discussion in the bug about the various approaches.
Attachment #608091 - Flags: feedback?(gavin.sharp)
Comment on attachment 608091 [details] [diff] [review]
patch

Do the remove* methods really need to pass in all of their arguments in the callback? It seems to me like nsIContentPrefRemoveCallback/nsIContentPrefRemoveByDomainCallback/nsIContentPrefRemoveAllDomainsCallback could be merged into:

+interface nsIContentPrefRemoveCallback : nsISupports
+  void onResult([optional] in nsIVariant domain,
+                [optional] in AString name);

with remove() passing domain+name, removeByDomain() passing only domain, and removeAllDomains passing neither. What's the use case for passing includeSubdomains to the callback?

I don't really understand the "sharing callbacks makes changing the methods difficult" argument - it seems quite unlikely that the semantics of any of these methods would change significantly.

Similarly, get/set could use the same callback interface.
Attachment #608091 - Flags: feedback?(gavin.sharp)
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #44)
> removeAllDomains passing neither. What's the use case for passing
> includeSubdomains to the callback?

Marco suggested in comments 25 and 27 that good callbacks receive all the args passed to their calls so they can differentiate which calls triggered them.  Do you agree here?  It sounds like you don't.

> I don't really understand the "sharing callbacks makes changing the methods
> difficult" argument - it seems quite unlikely that the semantics of any of
> these methods would change significantly.

I was wrong in comment 41.  If callbacks are shared and we had to add a new callback interface, the methods that don't need it could go on using the old ones.  In that case we might end up with something like nsIContentPrefRemoveCallback and nsIContentPrefRemoveCallbackWithMoreArguments.
(In reply to Drew Willcoxon :adw from comment #45)
> Marco suggested in comments 25 and 27 that good callbacks receive all the
> args passed to their calls so they can differentiate which calls triggered
> them.  Do you agree here?  It sounds like you don't.

Yeah, I don't think I agree. It seems like in the common case you'll have a single callback per method call (in which case you don't need to do any confirming that it's the right callback), and in other cases getting only the important arguments seems sufficient. I guess this would be better discussed with specific usage examples.
(In reply to Drew Willcoxon :adw from comment #45)
> (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #44)
> > removeAllDomains passing neither. What's the use case for passing
> > includeSubdomains to the callback?
> 
> Marco suggested in comments 25 and 27 that good callbacks receive all the
> args passed to their calls so they can differentiate which calls triggered
> them.

Enough to distinguish the callback is fine, I was too zealous in that phrase! I could be fine with Gavin's suggestions.  Yeah without actual use-cases is a bit hard to make estimates, we should probably start converting callers to make a call.
Comment on attachment 608091 [details] [diff] [review]
patch

I think this feedback was on the callbacks, I support Gavin's view, so it should not be needed for now.  Otherwise feel free to reset it.
Attachment #608091 - Flags: feedback?(mak77)
What's the next step here?
I'm working on modifying the patch to use the simpler callback interfaces that Gavin suggested.  I think m-c consumers will be fine with them.
Attached patch patch (obsolete) — Splinter Review
Attachment #608091 - Attachment is obsolete: true
Attachment #614619 - Flags: review?(mak77)
Attached patch patch (obsolete) — Splinter Review
Posting a patch to Bugzilla somehow makes it real no matter how many times I proofread it first.
Attachment #614619 - Attachment is obsolete: true
Attachment #614619 - Flags: review?(mak77)
Attachment #614631 - Flags: review?(mak77)
Comment on attachment 614631 [details] [diff] [review]
patch

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

Sorry if it took a bit of time... I still have to look at the tests, but that's a minor thing. I'm happy there are so many tests cause it's a bit hard to figure out all of the new code through a review.

::: dom/interfaces/base/nsIContentPrefService2.idl
@@ +21,5 @@
> +   * @param domain    The preference's domain as an nsIURI, a string, or null.
> +   *                  Null gets the preference that applies to all domains.
> +   * @param name      The preference's name as a nonempty string.
> +   * @param callback  Asynchronously called when the preference with the given
> +   *                  domain and name is retrieved.

should state if the callback is notified in case the pref doesn't exist, and what in case of errors fetching it?
The same for most of the other callback params, either we invoke always or only on success. In the former case we may need to add a nsresult argument to them.

Based on async favicons API experience, there will be a time when some code will actually need to know if something went wrong or doesn't exist, and then it may be hard to change all of the callback interfaces to support that.
May you please do a quick sweep through callers and see if that may be a common or useful path? We could even wait to start replacing call points, we should not advertize the new idl to add-ons (for example) until this is clear, imo.

@@ +76,5 @@
> +   * the bag corresponds to either a cached preference or the knowledge that a
> +   * preference does not exist.  In the former case the property's value is the
> +   * preference's value, and in the latter case the property's value is
> +   * undefined.  The property's key is the preference's domain (or null if the
> +   * given domain is null).

I think you may remove parens from here... The whole thing sounds a bit complicated, but maybe we should just add an example to mdn...

@@ +80,5 @@
> +   * given domain is null).
> +   *
> +   * This method returns true if the bag is nonempty and false otherwise, in
> +   * which case getSubdomains() must be called to determine whether the
> +   * preferences exist.

just move the second part of the phrase to a new phrase in the @return, having these separated sounds a bit confusing

@@ +173,5 @@
> +   * When a set or remove method is called, observers are notified after the set
> +   * or removal completes but before method's callback is called.
> +   *
> +   * The service holds a strong reference to the observer, so the observer must
> +   * be removed later to avoid leaking it.

move to a @note

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Sorry but I don't remember if I already asked why you are using modules instead of scripts and loadSubScript.  I know the module approach is temporary, just to separate code, but being a module, you know, add-ons will be tempted to use it as-is, that is far from what we want.

@@ +14,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +["Services", "ContentPrefStore"].forEach(function (name) {
> +  let uri = "resource://gre/modules/" + name + ".jsm";
> +  XPCOMUtils.defineLazyModuleGetter(this, name, uri);
> +}, this);

nit: not sure the point of this, it's taking the same amount of lines and looks a bit more expensive...

@@ +25,5 @@
> +
> +ContentPrefService2.prototype = {
> +
> +  get: function CPS2_get(group, name, callback) {
> +    checkCPCallback(callback, Ci.nsIContentPrefGetCallback);

I think "ensureValid" would be clearer than "check", the latter doesn't seem to enforce anything
Do we really need the "CP" part in this and in safeCPCallback? ensureValidCallback and safeCallback may be clear regardless.

@@ +38,5 @@
> +      safeCPCallback(callback, [bag, group, name]);
> +    });
> +  },
> +
> +  _get: function CPS2__get(group, name, subdoms, fn) {

I actually prefer the names we used in the idl... at least subdoms is completely unclear, I expect it being some sort of array than a bool... includeSubdoms would be better

@@ +40,5 @@
> +  },
> +
> +  _get: function CPS2__get(group, name, subdoms, fn) {
> +    group = this._parseGroup(group);
> +    checkName(name);

ensureValidName()

@@ +54,5 @@
> +        bag.setProperty(grp, val);
> +        this._cache.set(grp, name, val);
> +        return;
> +      }
> +      if (!bag.enumerator.hasMoreElements())

a newline or comment after row handling may better separate the handling of "no-more-rows" case

@@ +58,5 @@
> +      if (!bag.enumerator.hasMoreElements())
> +        this._cache.set(group, name, undefined);
> +      for (let [sgroup, val] in this._pbStore.match(group, name, subdoms)) {
> +        bag.setProperty(sgroup, val);
> +      }

some comment on the steps we follow would be appreciated (globally there's a lack of comments, not going to ask you add javadocs to each method, but at least non-common things should be explained).
Here someone patching this code has to figure out that the second part is handling EOR, why we look into pb storage (may not be immediately clear that we read both previously existing and pb prefs), and how sgroup differs from group (or mostly what the s stands for).

@@ +64,5 @@
> +    });
> +  },
> +
> +  _commonGetStmt: function CPS2__commonGetStmt(group, name, subdoms) {
> +    let stmt = group ?

For how much that may suck, please use a common if/else, for the sake of readability.

@@ +147,5 @@
> +
> +    // Create the setting if it doesn't exist.
> +    let stmt = this._stmt(
> +      "INSERT OR REPLACE INTO settings (id, name)",
> +      "VALUES((SELECT id FROM settings WHERE name = :name), :name)"

why not using OR IGNORE? it's cheaper and doesn't look like we have to replace something

@@ +156,5 @@
> +    // Create the group if it doesn't exist.
> +    if (group) {
> +      stmt = this._stmt(
> +        "INSERT OR REPLACE INTO groups (id, name)",
> +        "VALUES((SELECT id FROM groups WHERE name = :group), :group)"

ditto

@@ +197,5 @@
> +    stmt.params.name = name;
> +    stmt.params.value = value;
> +    stmts.push(stmt);
> +
> +    this._execStmts(stmts, function (row) {

I'm a little worried that we don't handle any way cases where _execStmts may fail (apart reportError)... Probably it's not extremely important for the specifics of this component, and it's a pretty much rare condition... though thinking about that.  Btw, just as a first step we should ensure we are consistent, so eigher we callback always or just on success (obviously when something async happens, if we throw synchronously I don't expect us to callback).

@@ +441,5 @@
> +      stmt.finalize();
> +    }
> +  },
> +
> +  _stmt: function CPS2__stmt(sql /*, sql2, sql3, ... */) {

Add a javadoc to better explain the arguments

@@ +450,5 @@
> +      this._statements[sql] = this._cps._dbConnection.createAsyncStatement(sql);
> +    return this._statements[sql];
> +  },
> +
> +  _execStmts: function CPS2__execStmts(stmts, onRow) {

add a javadoc to better explain the arguments (especially when and how the callback is invoked, the error handling and so on)

@@ +469,5 @@
> +      }
> +    });
> +  },
> +
> +  _parseGroup: function CPS2__parseGroup(group) {

this one needs documentation too, cause parse is really a generic term

@@ +530,5 @@
> +    ];
> +    if (supportedIIDs.some(function (i) iid.equals(i)))
> +      return this;
> +    if (iid.equals(Ci.nsIContentPrefService))
> +      return this._cps;

hm, nsIContentPrefService is in the array, so I can't see how we may reach the second if if iid is nsIContentPrefService.

::: toolkit/components/contentprefs/ContentPrefStore.jsm
@@ +7,5 @@
> +];
> +
> +function ContentPrefStore() {
> +  this._groups = {};
> +  this._globalNames = {};

I think we should use Dict.jsm for these... Well, I'd actually prefer Map, but looks like it's still marked unstable and I have no idea if it will be shipped in FF15 (right now it's marked for 14).

@@ +84,5 @@
> +        yield [sgroup, val];
> +    }
> +  },
> +
> +  matchGroups: function CPS_matchGroups(group, subdoms) {

Both match and matchGroups need decent documentation, and subdoms wants a better name that makes it look like a boolean
Attachment #614631 - Flags: review?(mak77) → feedback+
I recently stepped on this randomly when testing something else

http://pastebin.mozilla.org/1701000

Is this bug still on radar screens?
Keywords: main-thread-io
Whiteboard: [snappy:?]
Drew's been busy with some other work, but yes, this is still on the radar.
Whiteboard: [snappy:?] → [snappy]
(In reply to Marco Bonardo [:mak] from comment #53)
> should state if the callback is notified in case the pref doesn't exist, and
> what in case of errors fetching it?
> The same for most of the other callback params, either we invoke always or
> only on success. In the former case we may need to add a nsresult argument
> to them.
> 
> Based on async favicons API experience, there will be a time when some code
> will actually need to know if something went wrong or doesn't exist, and
> then it may be hard to change all of the callback interfaces to support that.

> I'm a little worried that we don't handle any way cases where _execStmts may
> fail (apart reportError)... Probably it's not extremely important for the
> specifics of this component, and it's a pretty much rare condition... though
> thinking about that.  Btw, just as a first step we should ensure we are
> consistent, so eigher we callback always or just on success (obviously when
> something async happens, if we throw synchronously I don't expect us to
> callback).

This is a good point.  Callbacks are called no matter what, even if there's an error.  I don't think it can be any other way for a callback-based async API.

But there's no easy way for the caller to see whether an error has occurred.  I think mozIStorageStatementCallback is a decent model, so how about something like this:

interface nsIContentPrefCallback : nsISupports
{
  void onPref(nsIContentPref pref);
  void onError(/* some kind of error object... */);
  void onComplete(/* a reason code? */);
};
interface nsIContentPref : nsISupports
{
  readonly attribute nsIVariant domain;
  readonly attribute AString name;
  readonly attribute nsIVariant value;
};

And then

interface nsIContentPrefService2 : nsISupports
{
  void get(in nsIVariant domain,
           in AString name,
           in nsIContentPrefCallback callback);

etc.  onPref would be called only for get and getSubdomains I guess.  If it turns out onPref would be useful for set and remove too, we could easily start calling it in those cases.  onError is called in any case of error.  onComplete is always called once, and after that, the callback won't be called anymore.

It's definitely more complex in the common case, but it's robust.  What do you think?
To be clear, onPref would be called multiple times for getSubdomains, one per each fetched preference.  Like how mozIStorageStatementCallback works for SQL rows.
Drew, over in bug 723002, we're looking into making the content prefs service accept an nsILoadContext parameter which would let it figure out whether a request is coming from PB mode or not.  As your patch is pretty much a rewrite of the service here, let's make sure that bug does not step on your toes and vice versa!  How close do you think your work is to landing?  We need bug 723002 for mobile, so we're looking into landing it as soon as we can...

Thanks!
Hi Ehsan, this isn't close to landing.  I'll rebase on your bug.  Thanks for asking.
Attached file new idl proposal (obsolete) —
Marco, how about this?  It's what I mentioned in comment 56.  (It doesn't take into account Ehsan's bug 723002 since it's just a proposed sketch right now.)
Attachment #667123 - Flags: feedback?(mak77)
(In reply to comment #60)
> Created attachment 667123 [details]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=667123&action=edit
> new idl proposal
> 
> Marco, how about this?  It's what I mentioned in comment 56.  (It doesn't take
> into account Ehsan's bug 723002 since it's just a proposed sketch right now.)

FWIW we decided to just add nsILoadContext parameters to the relevant methods there, and have a wrapper jsm which "binds" a given nsILoadContext to an instance of the content prefs service object.
Comment on attachment 667123 [details]
new idl proposal

I wonder whether we should completely avoid blind boolean param in:
  void removeByDomain(in nsIVariant domain,
                      in boolean includeSubdomains,
                      [optional] in nsIContentPrefCallback callback);

and rather follow the "get" example as:
  void removeByDomain(in nsIVariant domain,
                      [optional] in nsIContentPrefCallback callback);
  void removeBySubdomainsOf(in nsIVariant domain,
                            [optional] in nsIContentPrefCallback callback);

Now to the callback:
After seeing all of the different proposals here, I'm starting thinking the simplicity of a single function is a red herring, this one looks clean, well known and just working. The obvious defect is that it's not a function, but it can be easily wrapped as we do with all the multi-method callback we have around. So probably we are carying too much, it's a small price for giving a more common interface and more information. I think may be fine.

Probably I'd keep the most common naming conventions we have in the code. Some devs already know the handleResult, handleError, handleCompletion naming due to storage (and in minor part history), so could be there's more benefit keeping those names than making new ones and having to go looking the idl just to remember if it's onError or handleError.

Regarding the questions:
- I think it's unlikely that onPref (handleResult) may be called after handleError unless you have batch methods, for example in history you may try to update multiple places at once, some of them may succeed, some may fail. But in this case you don't have batch additions or modifications, so it's unlikely.
- I think we should keep the error simple, maybe even just an nsresult, this module doesn't have the complexity of storage (and an underlying third party library), so we don't need much extended error data. Again this would be different if the API would support batch changes cause you should be able to associate error->entry.

Due to the nature of this component and its usage, I don't think it's likely to get batching, so this likely answers the above.

Globally, yes I think there's no "clean" way out of this. Thank you for proceeding in this bug fix too.

If I missed anything please use the new "Need additional information from" bugzilla field, I suppose that is finally a trackable way to ask for feedback on a bug rather than just a patch (I should add it to my requests query!).
Attachment #667123 - Flags: feedback?(mak77) → feedback+
Whiteboard: [snappy] → [Snappy:P1]
(In reply to Graeme McCutcheon [:graememcc] from comment #23)
> If it's OK I'll switch back to just setting textbox.value
> manually, but set it immediately after setting the prefs and
> remove the page loading goop?

Sounds fine.  It sounds like you're saying you're removing the !s.canClearItem() check, but at some point you should check that after calling sanitize.
(In reply to Drew Willcoxon :adw from comment #63)

Sorry, wrong page.
This is updated for Marco's and my conversation, and all tests pass.  It is not updated for bug 723002.  That's next.

I've made some other changes to the interface.

* Domains are only specified with strings, not nsIURIs or null.
  This simplifies the interface, implementation, and tests at
  a small expense to some callers.
* Methods that previously could act on global prefs by specifying
  a null domain are now split into one method for acting on
  globals and one method for acting on non-globals.
* Methods that previously could act on subdomains by specifying a
  boolean are now split into one method for acting on subdomains
  and one method for acting on exact domain matches.
* Method names are now clear and unambiguous at the expense of
  terseness.  There are so many combinations of options now that
  I was having trouble remembering which method did what.
Attachment #614631 - Attachment is obsolete: true
Attachment #667123 - Attachment is obsolete: true
Attached patch patch, updated for bug 723002 (obsolete) — Splinter Review
Attachment #676779 - Attachment is obsolete: true
Attachment #679450 - Flags: review?(mak77)
Comment on attachment 679450 [details] [diff] [review]
patch, updated for bug 723002

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

I'm basically satisfied, though I think it's worth to get some feedback on private browsing behavior from ehsan before calling this ready. Once that is achieved I'll be happy to r+

As a global comment, I'd just like some more javadocs on internal helpers, since while names are often clear enough, arguments and behavior are not always.

::: dom/interfaces/base/nsIContentPrefService2.idl
@@ +30,5 @@
> + * to use storage appropriate to the context's usePrivateBrowsing attribute: if
> + * usePrivateBrowsing is true, temporary private-browsing storage is used, and
> + * permanent storage is used otherwise.  A context can be obtained from the
> + * window or channel whose content pertains to the preferences being modified or
> + * retrieved.

I wonder if we shouldn't rather just pass an inPrivateContext boolean, that is really easy to get from utils like isWindowPrivate or by checking the context externally... we don't need it for anything else than checking its usePrivateBrowsing property.

@@ +37,5 @@
> + *
> + * The methods of callback objects are always called asynchronously.  See
> + * nsIContentPrefCallback2 below for more information about callbacks.
> + */
> +[scriptable, uuid(51e1d34a-5e9d-4b77-b14c-0f8346e264ca)]

nit: newline to separate above would be welcome

@@ +241,5 @@
> +   * @param name      The preferences' name.
> +   * @param context   The private-browsing context, if any.
> +   * @param callback  handleCompletion is called when the operation completes.
> +   */
> +  void removeByName(in AString name,

While I understand how this differs from removeGlobal, it probably deserves some better explanation for someone not used to this component

@@ +325,5 @@
> +   */
> +  void handleCompletion(in unsigned short reason);
> +
> +  const unsigned short COMPLETE_OK = 0;
> +  const unsigned short COMPLETE_ERROR = 1;

may just be a bool? Do you think we may have to add further statuses in future? I think this is "simple" enough to maybe being just worth a bool.

@@ +334,5 @@
> +{
> +  readonly attribute AString domain;
> +  readonly attribute AString name;
> +  readonly attribute nsIVariant value;
> +};

I like the idl pretty much, much better than what we started from, nice job.

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +5,5 @@
> +let EXPORTED_SYMBOLS = [
> +  "ContentPrefService2",
> +];
> +
> +const { interfaces: Ci, classes: Cc, results: Cr, utils: Cu } = Components;

please don't use this, I think was discusses some time ago to avoid it since it's less performant than the classical simple assignments

@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/ContentPrefStore.jsm");
> +
> +function ContentPrefService2(cps) {

a comment explaining the future plans to deprecate cps and evolve this above could be nice

@@ +19,5 @@
> +
> +ContentPrefService2.prototype = {
> +
> +  getByDomainAndName: function CPS2_getByDomainAndName(group, name, context,
> +                                                       callback) {

nit: I'd prefer if this module would respect the coding style prefixing a to arguments... but I see it may be an overkill to do that now :(

@@ +57,5 @@
> +        let grp = row.getResultByName("grp");
> +        let val = row.getResultByName("value");
> +        this._cache.set(grp, name, val);
> +        if (!pbPrefs.has(group, name))
> +          cbHandleResult(callback, new ContentPref(grp, name, val));

So, I'm not sure how we are going to handle PB in this case, cause in most cases Ehsan stated in PB mode we should only expose PB contents, past contents should not "exist" in a pb context.
Thid works slightly differently, giving priority to PB contents in a PB context, we should probably ping Ehsan or JDM to discuss the wanted outcome.
I think it's not a big deal to expose past values in this case.

@@ +142,5 @@
> +    if (context && context.usePrivateBrowsing)
> +      storesToCheck.push(this._pbStore);
> +
> +    let outStore = new ContentPrefStore();
> +    storesToCheck.forEach(function (store) {

let's use for (let store of storesToCheck)

@@ +168,5 @@
> +  _set: function CPS2__set(group, name, value, context, callback) {
> +    group = this._parseGroup(group);
> +    checkName(name);
> +    if (value === undefined)
> +      throw invalidArg("value must not be undefined.");

well, you did 30, let's do 31 with a checkValue helper. I know it's used just once but looks more consistent (I hope I didn't say otherwise in past reviews!)

@@ +185,5 @@
> +
> +    // Create the setting if it doesn't exist.
> +    let stmt = this._stmt(
> +      "INSERT OR REPLACE INTO settings (id, name)",
> +      "VALUES((SELECT id FROM settings WHERE name = :name), :name)"

hm, I don't remember why here we have to use "or replace" instead of "or ignore", looks like we can just ignore adding a name if it already exists
Maybe I just suggested it blindly.

@@ +194,5 @@
> +    // Create the group if it doesn't exist.
> +    if (group) {
> +      stmt = this._stmt(
> +        "INSERT OR REPLACE INTO groups (id, name)",
> +        "VALUES((SELECT id FROM groups WHERE name = :group), :group)"

and also here

@@ +249,5 @@
> +      }
> +    });
> +  },
> +
> +  _schedule: function CPS2__schedule(fn) {

move this at the end with other helpers

@@ +564,5 @@
> +      this._statements[sql] = this._cps._dbConnection.createAsyncStatement(sql);
> +    return this._statements[sql];
> +  },
> +
> +  _execStmts: function execStmts(stmts, callbacks) {

would be nice to document what callbacks is supposed to be, when they are invoked, at least

@@ +571,5 @@
> +    this._cps._dbConnection.executeAsync(stmts, stmts.length, {
> +      handleResult: function handleResult(results) {
> +        try {
> +          let row = null;
> +          while (row = results.getNextRow()) {

nit: I'd prefer for (let row = results.getNextRow(); row; row = results.getNextRow()) since we don't need row outside the loop

@@ +605,5 @@
> +      }
> +    });
> +  },
> +
> +  _parseGroup: function CPS2__parseGroup(group) {

wants some documentation

@@ +625,5 @@
> +  removeObserverForName: function CPS2_removeObserverForName(name, observer) {
> +    this._cps.removeObserver(name, observer);
> +  },
> +
> +  // Tests use this as a backchannel by calling it directly.

javadoc? :)

@@ +639,5 @@
> +      break;
> +    }
> +  },
> +
> +  _reset: function CPS2__reset(callback) {

wants a javadoc stating what it does and that is only used for testing purposes.

@@ +676,5 @@
> +ContentPref.prototype = {
> +  QueryInterface: function QueryInterface(iid) {
> +    if (!iid.equals(Ci.nsIContentPref) && !iid.equals(Ci.nsISupports))
> +      throw Cr.NS_ERROR_NO_INTERFACE;
> +    return this;

any reason to not use XPCOMUtils.generateQI here?

@@ +696,5 @@
> +function safeCallback(callbackObj, methodName, args) {
> +  if (!callbackObj || typeof(callbackObj[methodName]) != "function")
> +    return;
> +  try {
> +    callbackObj[methodName].apply(callbackObj, args || []);

I think you can pass through undefined (thus args supposing it's optional), the documentations doesn't seem to make a difference between empty array or undefined...

@@ +713,5 @@
> +  if (!name || typeof(name) != "string")
> +    throw invalidArg("name must be nonempty string.");
> +}
> +
> +function checkCPCallback(callback, required) {

I think the various checkXXX functions should gain an "Arg" suffix, so checkGroupArg, checkNameArg... so it's  a bit more clear they are doing arguments checking and throwing invalidArg.

::: toolkit/components/contentprefs/tests/Makefile.in
@@ +12,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  MODULE		= test_toolkit_contentprefs
>  
>  ifdef MOZ_PHOENIX

wait, seriously?
I'd say we can burn it with fire here
we have xpcshell.ini files to disable tests now... maybe worth a tryrun just to check if android is unhappy.

::: toolkit/components/contentprefs/tests/unit_cps2/AsyncRunner.jsm
@@ +7,5 @@
> +];
> +
> +const { interfaces: Ci, classes: Cc } = Components;
> +
> +function AsyncRunner(callbacks) {

Well I'm not asking to rewrite all of this, since that'd be crazy, though we started using promises and Task.jsm in tests and those may actually simplify testing here removing the need for an AsyncRunner. You can spawn a Task and then yield waiting for async pieces (these have to return a promise, task continues when the promise is fulfilled)
Could be evaluated for a follow-up, eventually, and I won't hold my breath.

::: toolkit/components/contentprefs/tests/unit_cps2/xpcshell.ini
@@ +10,5 @@
> +[test_removeAllDomains.js]
> +[test_removeByName.js]
> +[test_getCached.js]
> +[test_getCachedSubdomains.js]
> +[test_observers.js]

as said, it's worth a tryrun without the MOZ_PHOENIX define and check Android is happym otherwise skip-if here
Attachment #679450 - Flags: review?(mak77) → feedback+
PS: when I suggest prefixing arguments, you DO NOT have to, it would be crazy. Looks like there's an increasing voice among our devs that doesn't like prefixing. I personally think it's useful to distinguish arguments from local vars, but this is not an argument to cause someone to rewrite hundreds of vars. Just to be clear, since I don't want you to lose half a day there :)
Comment on attachment 679450 [details] [diff] [review]
patch, updated for bug 723002

Ehsan, Marco would like your feedback on the private browsing behavior of this patch.  I'll quote Marco and then try to provide context and a summary:

(In reply to Marco Bonardo [:mak] (Away 10-11 Dec) from comment #67)
> ::: dom/interfaces/base/nsIContentPrefService2.idl
> @@ +30,5 @@
> > + * to use storage appropriate to the context's usePrivateBrowsing attribute: if
> > + * usePrivateBrowsing is true, temporary private-browsing storage is used, and
> > + * permanent storage is used otherwise.  A context can be obtained from the
> > + * window or channel whose content pertains to the preferences being modified or
> > + * retrieved.
> 
> I wonder if we shouldn't rather just pass an inPrivateContext boolean, that
> is really easy to get from utils like isWindowPrivate or by checking the
> context externally... we don't need it for anything else than checking its
> usePrivateBrowsing property.

In the patch, nsIContentPrefService methods take an nsILoadContext to determine private browsing state.  (That's unchanged from how nsIContentPrefService currently works.)  Should they take a simpler inPrivateContext boolean instead?

> @@ +57,5 @@
> > +        let grp = row.getResultByName("grp");
> > +        let val = row.getResultByName("value");
> > +        this._cache.set(grp, name, val);
> > +        if (!pbPrefs.has(group, name))
> > +          cbHandleResult(callback, new ContentPref(grp, name, val));
> 
> So, I'm not sure how we are going to handle PB in this case, cause in most
> cases Ehsan stated in PB mode we should only expose PB contents, past
> contents should not "exist" in a pb context.
> Thid works slightly differently, giving priority to PB contents in a PB
> context, we should probably ping Ehsan or JDM to discuss the wanted outcome.
> I think it's not a big deal to expose past values in this case.

Here's how the patch handles private browsing for nsIContentPrefService.  There are two stores: a normal, permanent store and a temporary store used for calls where the load context indicates private browsing is active.  (That's unchanged from how things currently work.)  When a load context indicates that private browsing should be used,

* getters search the PB store first.  If the pref is present in
  the PB store, then its value is returned.  If the pref is not
  present in the PB store, then the normal store is searched.

* setters always update the PB store, never the regular store.

* removers update both the PB and normal stores.
Attachment #679450 - Flags: feedback?(ehsan)
Marco,

(In reply to Marco Bonardo [:mak] (Away 10-11 Dec) from comment #67)
> @@ +325,5 @@
> > +   */
> > +  void handleCompletion(in unsigned short reason);
> > +
> > +  const unsigned short COMPLETE_OK = 0;
> > +  const unsigned short COMPLETE_ERROR = 1;
> 
> may just be a bool? Do you think we may have to add further statuses in
> future? I think this is "simple" enough to maybe being just worth a bool.

You suggested keeping the same naming conventions as mozIStorageStatementCallback, which is probably a good idea, so in that spirit I kept the same "reason" code.  In light of that, do you still think it should be a boolean?  (I'm not against it.)

> > +const { interfaces: Ci, classes: Cc, results: Cr, utils: Cu } = Components;
> 
> please don't use this, I think was discusses some time ago to avoid it since
> it's less performant than the classical simple assignments

Hmm, that's pretty hand-wavey. :-)  Any references?

> > +    storesToCheck.forEach(function (store) {
> 
> let's use for (let store of storesToCheck)

:*-(
Flags: needinfo?(mak77)
(In reply to Drew Willcoxon :adw from comment #70)
> > > +const { interfaces: Ci, classes: Cc, results: Cr, utils: Cu } = Components;
> > 
> > please don't use this, I think was discusses some time ago to avoid it since
> > it's less performant than the classical simple assignments
> 
> Hmm, that's pretty hand-wavey. :-)  Any references?

I think this is an example of something that's probably not worth even discussing, and is more of a stylistic preference. I think it's quick unlikely that the performance difference of de-structuring assignment is significant in this scenario, even if measured across every such line our entire code base :)
Comment on attachment 679450 [details] [diff] [review]
patch, updated for bug 723002

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

(In reply to Drew Willcoxon :adw from comment #69)
> Comment on attachment 679450 [details] [diff] [review]
> patch, updated for bug 723002
> 
> Ehsan, Marco would like your feedback on the private browsing behavior of
> this patch.  I'll quote Marco and then try to provide context and a summary:
> 
> (In reply to Marco Bonardo [:mak] (Away 10-11 Dec) from comment #67)
> > ::: dom/interfaces/base/nsIContentPrefService2.idl
> > @@ +30,5 @@
> > > + * to use storage appropriate to the context's usePrivateBrowsing attribute: if
> > > + * usePrivateBrowsing is true, temporary private-browsing storage is used, and
> > > + * permanent storage is used otherwise.  A context can be obtained from the
> > > + * window or channel whose content pertains to the preferences being modified or
> > > + * retrieved.
> > 
> > I wonder if we shouldn't rather just pass an inPrivateContext boolean, that
> > is really easy to get from utils like isWindowPrivate or by checking the
> > context externally... we don't need it for anything else than checking its
> > usePrivateBrowsing property.
> 
> In the patch, nsIContentPrefService methods take an nsILoadContext to
> determine private browsing state.  (That's unchanged from how
> nsIContentPrefService currently works.)  Should they take a simpler
> inPrivateContext boolean instead?

I see no reason why we would change the existing API convention here.

> > @@ +57,5 @@
> > > +        let grp = row.getResultByName("grp");
> > > +        let val = row.getResultByName("value");
> > > +        this._cache.set(grp, name, val);
> > > +        if (!pbPrefs.has(group, name))
> > > +          cbHandleResult(callback, new ContentPref(grp, name, val));
> > 
> > So, I'm not sure how we are going to handle PB in this case, cause in most
> > cases Ehsan stated in PB mode we should only expose PB contents, past
> > contents should not "exist" in a pb context.
> > Thid works slightly differently, giving priority to PB contents in a PB
> > context, we should probably ping Ehsan or JDM to discuss the wanted outcome.
> > I think it's not a big deal to expose past values in this case.
> 
> Here's how the patch handles private browsing for nsIContentPrefService. 
> There are two stores: a normal, permanent store and a temporary store used
> for calls where the load context indicates private browsing is active. 
> (That's unchanged from how things currently work.)  When a load context
> indicates that private browsing should be used,
> 
> * getters search the PB store first.  If the pref is present in
>   the PB store, then its value is returned.  If the pref is not
>   present in the PB store, then the normal store is searched.
> 
> * setters always update the PB store, never the regular store.
> 
> * removers update both the PB and normal stores.

Yes, this is the semantics that we want.  Basically the goal is to not store any permanent prefs for private windows, but behave in a way that updating prefs still works as expected (i.e., the ordering should be consistent from the callers viewpoint -- a getter should always return the setter's argument if it was called right after it, for example.)


f=me based on the above.  Note that I didn't actually read the full patch -- please let me know if you need me to, but that would take longer for me.  :-)
Attachment #679450 - Flags: feedback?(ehsan) → feedback+
(In reply to Drew Willcoxon :adw from comment #70)
> > may just be a bool? Do you think we may have to add further statuses in
> > future? I think this is "simple" enough to maybe being just worth a bool.
> 
> You suggested keeping the same naming conventions as
> mozIStorageStatementCallback, which is probably a good idea, so in that
> spirit I kept the same "reason" code.  In light of that, do you still think
> it should be a boolean?  (I'm not against it.)

Ah, yes I suggested the same naming, and likely makes sense to retain the reason or we'd take just half the benefits.

> > > +const { interfaces: Ci, classes: Cc, results: Cr, utils: Cu } = Components;
> Hmm, that's pretty hand-wavey. :-)  Any references?

Just a couple days ago gps asked in jsapi and they said the difference exists but is "unmeasurable", so whatever you prefer, let's not discuss that.


Thanks for the feedback Ehsan.
Flags: needinfo?(mak77)
Attached patch patch (obsolete) — Splinter Review
Tryserver is green: https://tbpl.mozilla.org/?tree=Try&rev=5c3055f0c6ad

Comments addressed with these notes:

> > +  void removeByName(in AString name,
> 
> While I understand how this differs from removeGlobal, it probably deserves
> some better explanation for someone not used to this component

I added more introductory text that explains what content prefs are and what global prefs are and then slightly adjusted the rest of the comments to match.  Let me know if it's still confusing.

> @@ +571,5 @@
> > +    this._cps._dbConnection.executeAsync(stmts, stmts.length, {
> > +      handleResult: function handleResult(results) {
> > +        try {
> > +          let row = null;
> > +          while (row = results.getNextRow()) {
> 
> nit: I'd prefer for (let row = results.getNextRow(); row; row =
> results.getNextRow()) since we don't need row outside the loop

That means writing results.getNextRow() twice, which is worse than setting row outside the loop, so I left it.

> @@ +676,5 @@
> > +ContentPref.prototype = {
> > +  QueryInterface: function QueryInterface(iid) {
> > +    if (!iid.equals(Ci.nsIContentPref) && !iid.equals(Ci.nsISupports))
> > +      throw Cr.NS_ERROR_NO_INTERFACE;
> > +    return this;
> 
> any reason to not use XPCOMUtils.generateQI here?

It needs to specially handle QI(Ci.nsIContentPrefService).  It returns the verson-1 CPS in that case.

> ::: toolkit/components/contentprefs/tests/unit_cps2/xpcshell.ini
> @@ +10,5 @@
> > +[test_removeAllDomains.js]
> > +[test_removeByName.js]
> > +[test_getCached.js]
> > +[test_getCachedSubdomains.js]
> > +[test_observers.js]
> 
> as said, it's worth a tryrun without the MOZ_PHOENIX define and check
> Android is happym otherwise skip-if here

I removed the MOZ_PHOENIX check, and the try run was green, but it looks like Android xpcshell tests didn't run.  I don't see any X's for Android on tbpl actually.
Attachment #679450 - Attachment is obsolete: true
Attachment #691593 - Flags: review?(mak77)
(In reply to Drew Willcoxon :adw from comment #74)
> > @@ +676,5 @@
> > > +ContentPref.prototype = {
> > > +  QueryInterface: function QueryInterface(iid) {
> > > +    if (!iid.equals(Ci.nsIContentPref) && !iid.equals(Ci.nsISupports))
> > > +      throw Cr.NS_ERROR_NO_INTERFACE;
> > > +    return this;
> > 
> > any reason to not use XPCOMUtils.generateQI here?
> 
> It needs to specially handle QI(Ci.nsIContentPrefService).  It returns the
> verson-1 CPS in that case.


maybe I am misreading it, but this code doesn't seem to differ in anything from generateQI, nor to handle nsIContentPrefService. I think you are referring to ContentPrefService.prototype.QueryInterface?

ContentPref.prototype = {
  QueryInterface: function QueryInterface(iid) {
    if (!iid.equals(Ci.nsIContentPref) && !iid.equals(Ci.nsISupports))
      throw Cr.NS_ERROR_NO_INTERFACE;
    return this;
  },
};

> I removed the MOZ_PHOENIX check, and the try run was green, but it looks
> like Android xpcshell tests didn't run.  I don't see any X's for Android on
> tbpl actually.

interesting, well if try is happy I don't see a problem, MOZ_PHOENIX should be removed regardless, it's so old!
Attached patch patchSplinter Review
Attachment #691593 - Attachment is obsolete: true
Attachment #691593 - Flags: review?(mak77)
Attachment #691856 - Flags: review?(mak77)
Comment on attachment 691856 [details] [diff] [review]
patch

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

Thank you, looks like I ran out of comments! Sorry if it took a while, you did a great job.

I'd suggest a try run with some repeated X runs, just to check there is not any unexpected intermittent failure.
And a final SR mark.

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +606,5 @@
> +   *                   onError(nsresult) (optional)
> +   *                     Called on error.
> +   *                     nsresult: The error code.
> +   */
> +  _execStmts: function execStmts(stmts, callbacks) {

nit: missing CPS2_ prefix
Attachment #691856 - Flags: review?(mak77) → review+
Comment on attachment 691856 [details] [diff] [review]
patch

Dave, would you mind super-reviewing the new nsIContentPrefService2 API in this patch?
Attachment #691856 - Flags: superreview?(dtownsend+bugmail)
before I forget:
> +          while (row = results.getNextRow()) {

this needs more parenthesis, or js will warn "test for equality (==) mistyped as assignment (=)?"
Comment on attachment 691856 [details] [diff] [review]
patch

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

Looks like a good API to me.
Attachment #691856 - Flags: superreview?(dtownsend+bugmail) → superreview+
https://hg.mozilla.org/mozilla-central/rev/cca7f05e2c05
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Drew, do you have a plan of attack to convert existing consumers already?
No plan right now.
Depends on: 829456
Is there any way I can test this?
(In reply to Bogdan Maris [QA] from comment #85)
> Is there any way I can test this?

there are not consumers at the momento, so I guess this is verified by its own automated tests passing and code in tree.
Comment on attachment 691856 [details] [diff] [review]
patch

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

::: dom/interfaces/base/nsIContentPrefService2.idl
@@ +332,5 @@
> +  void handleResult(in nsIContentPref pref);
> +
> +  /**
> +   * Called when an error occurs.  This may be called multiple times before
> +   * onComplete is called.

s/onComplete/handleCompletion/ ?
No longer depends on: 829456
side note, bug 820438 (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=820438&attachment=702858) is fixing stuff in the old service and using the old API in nsJSEnvironment.cpp

Unfortunately we didn't catch this earlier and since that is blocking b2g18 there's not much to do.
I asked to file a bug to port the changes and update nsJSEnvironment.cpp to use the new API.

We need to add some DEPRECATED notices in the old component and the old API to avoid these misuses.
Comment on attachment 691856 [details] [diff] [review]
patch

>+  void getCachedBySubdomainAndName(in AString domain,
>+                                   in AString name,
>+                                   in nsILoadContext context,
>+                                   out unsigned long len,
>+                                   [retval,array,size_is(len)] out nsIContentPref prefs);

>+  function erroneous() {
>+    do_check_throws(function ()
>+                    cps.getCachedBySubdomainAndName(null, "foo", null));
>+    do_check_throws(function ()
>+                    cps.getCachedBySubdomainAndName("", "foo", null));
>+    do_check_throws(function ()
>+                    cps.getCachedBySubdomainAndName("a.com", "", null));
>+    do_check_throws(function ()
>+                    cps.getCachedBySubdomainAndName("a.com", null, null));
>+    yield true;
>+  },
These tests are broken. The function throws, not because the parameters are invalid, but because len isn't optional, so XPConnect thinks you're failing to pass enough parameters. https://tbpl.mozilla.org/?tree=Try&rev=2a717fee6fbc shows that the tests continue to pass when len is made optional. I'll file a followup bug on fixing this.
Depends on: 913948
You need to log in before you can comment on or make changes to this bug.