539907 - Have getPref use asynchronous statements when called with an optional callback

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

This will eventually allow us to stop blocking the UI thread for IO caused by the page zoom check on every onLocationChange.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

This works, but it would be better for getPref to pass through the callback to _selectPref/_selectGlobalPref, which would then construct and execute the statement either synchronously or asynchronously depending on whether or not they received a callback.  That way there would be less code repetition and better encapsulation of the statement initialization and execution code.

You'll need to factor out the nsIContentPrefCallback literal into a constructor function with a prototype so you don't repeat it in both functions, but that should be pretty simple.

And the try/finally statements in those functions could complicate things slightly, but they probably don't, since calling reset on an asynchronously-executing statement appears safe.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 421802 [details] [diff] [review]
Patch

Oops, forgot to set review status.  This is a very welcome improvement that'll get r+ with the changes specified in comment 1!

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2

And I forgot to pop this off as a patch before adding something on top of it! :)

What I wanted to do doesn't seem to be valid XPIDLese and blew up once I tried to compile native code depending on it. This new version isn't quite as elegant, but I guess it's a decent compromise.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 423214 [details] [diff] [review]
Patch v2

This looks good, r=myk.

One API question, however: now that XPCOM/XPIDL supports optional parameters, i.e. those with the [optional] prefix prepended to their names, like nsIHandlerInfo::launchWithURI <http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/public/nsIMIMEInfo.idl#123>, wouldn't it be better to make |getPref| a polymorphic function that works either synchronously or asynchronously depending on the presence of an optional aCallback parameter?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Erm, d'oh!  That's exactly what you did the first time.  Bummer that it doesn't work.  What's the issue?

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Alternate patch

Went back and messed with the first version again and found that the compiler error I was getting confusingly made it seem like there were too many arguments when there were really too few. Null to the rescue!

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 423465 [details] [diff] [review]
Alternate patch

>+   * @param    aCallback   an optional nsIContentPrefCallback to receive the
>+   *                       results instead of returning

It might be worth noting in a comment here that JS callers can pass a JS function in addition to an nsIContentPrefCallback as the callback.


>+        let astmt = new AsyncStatement(this._stmtSelectPref);
>+        astmt.execute(aCallback);

You could simplify these to:

        new AsyncStatement(this._stmtSelectPref).execute(aCallback);

But overall it looks great.  I'm psyched you can implement it via polymorphism! r=myk

Comment 8

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/mozilla-central/rev/57c91cab646b