Have getPref use asynchronous statements when called with an optional callback

RESOLVED FIXED in mozilla1.9.3a2

Status

()

Toolkit
Preferences
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rflint, Assigned: rflint)

Tracking

({perf})

Trunk
mozilla1.9.3a2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [TSnap])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Attachment #421802 - Flags: review?(myk)
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 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!
Attachment #421802 - Flags: review?(myk) → review-
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.
Attachment #421802 - Attachment is obsolete: true
Attachment #423214 - Flags: review?(myk)
Blocks: 541779
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?
Attachment #423214 - Flags: review?(myk) → review+
Erm, d'oh!  That's exactly what you did the first time.  Bummer that it doesn't work.  What's the issue?
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!
Attachment #423465 - Flags: superreview?(mconnor)
Attachment #423465 - Flags: review?(myk)
Attachment #423465 - Flags: review?(myk) → review+
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
Attachment #423465 - Flags: superreview?(mconnor) → superreview?(robert.bugzilla)
Attachment #423465 - Flags: superreview?(robert.bugzilla) → superreview+
http://hg.mozilla.org/mozilla-central/rev/57c91cab646b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a1 → mozilla1.9.3a2
Depends on: 546445
You need to log in before you can comment on or make changes to this bug.