Last Comment Bug 539907 - Have getPref use asynchronous statements when called with an optional callback
: Have getPref use asynchronous statements when called with an optional callback
Status: RESOLVED FIXED
[TSnap]
: perf
Product: Toolkit
Classification: Components
Component: Preferences (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla1.9.3a2
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
:
: Neil Deakin
Mentors:
Depends on: 546445
Blocks: 541779
  Show dependency treegraph
 
Reported: 2010-01-15 03:20 PST by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2010-02-16 09:29 PST (History)
4 users (show)
rflint: in‑testsuite+
rflint: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (8.02 KB, patch)
2010-01-15 03:20 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
myk: review-
Details | Diff | Splinter Review
Patch v2 (10.64 KB, patch)
2010-01-24 01:20 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
myk: review+
Details | Diff | Splinter Review
Alternate patch (10.66 KB, patch)
2010-01-25 18:18 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
myk: review+
robert.strong.bugs: superreview+
Details | Diff | Splinter Review

Description User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-01-15 03:20:08 PST
Created attachment 421802 [details] [diff] [review]
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 User image Myk Melez [:myk] [@mykmelez] 2010-01-15 11:15:25 PST
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 User image Myk Melez [:myk] [@mykmelez] 2010-01-22 18:38:30 PST
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 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-01-24 01:20:59 PST
Created attachment 423214 [details] [diff] [review]
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 User image Myk Melez [:myk] [@mykmelez] 2010-01-25 13:36:45 PST
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 User image Myk Melez [:myk] [@mykmelez] 2010-01-25 13:37:55 PST
Erm, d'oh!  That's exactly what you did the first time.  Bummer that it doesn't work.  What's the issue?
Comment 6 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-01-25 18:18:01 PST
Created attachment 423465 [details] [diff] [review]
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 User image Myk Melez [:myk] [@mykmelez] 2010-01-25 21:20:05 PST
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 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-02-09 02:36:45 PST
http://hg.mozilla.org/mozilla-central/rev/57c91cab646b

Note You need to log in before you can comment on or make changes to this bug.