Comment on attachment 397652 [details] [diff] [review] v1.0 >diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js >+ XPCOMUtils.defineLazyGetter(this, "_defaultQuery", function() { > let replacementText = ""; >- return this._defaultQuery = this._db.createStatement( >+ return this._db.createStatement( > SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", replacementText, "g") > ); nit: this code is repeated in all the next queries lazy getters, maybe you could provide a getACStatement(replacementText) or something like that > >- this.__defineGetter__("_keywordQuery", function() { >- delete this._keywordQuery; >+ XPCOMUtils.defineLazyGetter(this, "_keywordQuery", function() { > return this._keywordQuery = this._db.createStatement( just return this._db.createStatement, no need to assign. since this looks cleaner, can we fix LivemarkService to do the same? these are all Places __defineGetter__ calls http://mxr.mozilla.org/mozilla-central/search?string=defineGetter&find=places&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

(In reply to comment #2) > nit: this code is repeated in all the next queries lazy getters, maybe you > could provide a getACStatement(replacementText) or something like that I didn't do that in the original code because I thought it added an extra level of indirection that made the code harder to follow. I'd rather not do that now if that's OK with you. > since this looks cleaner, can we fix LivemarkService to do the same? I don't know how I missed the livemark service :/ > these are all Places __defineGetter__ calls I'd rather fix the browser ones in a different bug (I'd actually rather not do it. I did this bug to make sure my implementation was sane and usable).

Addresses review comments.

(In reply to comment #3) > (In reply to comment #2) > I didn't do that in the original code because I thought it added an extra level > of indirection that made the code harder to follow. I'd rather not do that now > if that's OK with you. sure, i just wanted to get your feedback about that, was a nit exactly because i was not sure it was worth doing it.

http://hg.mozilla.org/mozilla-central/rev/ec44dfe85a0d Hurray for cleaner code!

Bustage fix. I guess I didn't run the tests with the second iteration of the patch :/ http://hg.mozilla.org/mozilla-central/rev/81c827ddc262

Comment on attachment 398210 [details] [diff] [review] v1.1 >+++ b/toolkit/components/places/src/PlacesDBUtils.jsm >+ XPCOMUtils.defineLazyServiceGetter(this, "_bms", BS_CONTRACTID, >+ "nsINavBookmarksService"); >+ >+ XPCOMUtils.defineLazyServiceGetter(this, "_hs", HS_CONTRACTID, >+ "nsINavHistoryService"); >+ >+ XPCOMUtils.defineLazyServiceGetter(this, "_os", OS_CONTRACTID, >+ "nsIObserverService"); >+ >+ XPCOMUtils.defineLazyServiceGetter(this, "_idlesvc", IS_CONTRACTID, >+ "nsIIdleService"); Curious.. why not :p [["bms", "BS", "NavBookmarks"], ["hs", "HS", "NavHistory"], ["os", "OS", "Observer"], ["idlesvc", "IS", "Idle"], ].forEach(function([name, cid, iface]) XPCOMUtils.defineLazyServiceGetter( this, "_" + name, cid + "_CONTRACTID", "nsI" + iface + "Service"), this); ;) Woah. defineLazyServiceGetter is such a long name. :p >+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js >+ XPCOMUtils.defineLazyGetter(this, "_defaultQuery", function() { > let replacementText = ""; >+ return this._db.createStatement( > SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", replacementText, "g") > ); > }); > >+ XPCOMUtils.defineLazyGetter(this, "_historyQuery", function() { > let replacementText = "AND h.visit_count > 0"; >+ return this._db.createStatement( > SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", replacementText, "g") > ); > }); > >+ XPCOMUtils.defineLazyGetter(this, "_bookmarkQuery", function() { > let replacementText = "AND bookmark IS NOT NULL"; >+ return this._db.createStatement( > SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", replacementText, "g") > ); > }); > >+ XPCOMUtils.defineLazyGetter(this, "_tagsQuery", function() { > let replacementText = "AND tags IS NOT NULL"; >+ return this._db.createStatement( > SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", replacementText, "g") > ); > }); > >+ XPCOMUtils.defineLazyGetter(this, "_typedQuery", function() { > let replacementText = "AND h.typed = 1"; >+ return this._db.createStatement( > SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", replacementText, "g") > ); > }); [["default", ""], ["history", "h.visit_count > 0"], ["bookmark", "bookmark IS NOT NULL"], ["tags", "tags IS NOT NULL"], ["typed", "h.typed = 1"], ].forEach(function([query, replace]) XPCOMUtils.defineLazyGetter(this, "_" + query, function() this._db.createStatement(SQL_BASE.replace( "{ADDITIONAL_CONDITIONS}", replace ? "AND " + replace : "", "g"))), this); ;) ;) ;) (untested)

(In reply to comment #8) > >+ XPCOMUtils.defineLazyServiceGetter(this, "_bms", BS_CONTRACTID, > >+ "nsINavBookmarksService"); > [["bms", "BS", "NavBookmarks"], > ].forEach(function([name, cid, iface]) XPCOMUtils.defineLazyServiceGetter( > this, "_" + name, cid + "_CONTRACTID", "nsI" + iface + "Service"), this); Oh wait, you want the actual variable BS_CONTRACTID. Well.. I suppose you could get global[cid + "_CONTRACTID"] or something....

that's an interesting compression, but looks globally hard to read.

Yes, it would be a disaster of comprehension, and wouldn't turn up when some desperate . If you want to reduce duplication, make a helper function and pass it some self-documenting object literal or suchlike addLazyGetter({variable:"_nm", contract:OS_CONTRACTID, smell:"funky"})

(In reply to comment #11) > Yes, it would be a disaster of comprehension, and wouldn't turn up when some > desperate person was trying to grep or lxr for where the holy heck "_nm" was defined.

Well, you can still take it a step back without the dynamic string-concat name generation and save on typing out XPCOMUtils.defineLazyServiceGetter multiple times. You get the same mxr-searchability as the current code. [["_bms", BS_CONTRACTID, "nsINavBookmarksService"], ["_hs", HS_CONTRACTID, "nsINavHistoryService"], ].forEach(function([name, cid, iface]) XPCOMUtils.defineLazyServiceGetter( this, name, cid, iface), this); Also, the items in the array are named as part of the forEach callback. It's just not directly next to the declaration. No worse than if you had a wrapper but without the extra call to wrap: function package(name, cid, iface) ({ name: name, cid: cid, iface: iface }) [package("_bms", BS_CONTRACTID, "nsINavBookmarksService"), { name: "_hs", cid: HS_CONTRACTID, iface: "nsINavHistoryService" }, ].forEach(function({name, cid, iface}) XPCOMUtils... [["_defaultQuery", ""], ["_historyQuery", "h.visit_count > 0"], ].forEach(function([queryName, replace]) XPCOMUtils.defineLazyGetter(this, queryName, function() this._db.createStatement(SQL_BASE.replace( "{ADDITIONAL_CONDITIONS}", replace ? "AND " + replace : "", "g"))), this);

Yes, that is a block of line noise, and you have to unroll many layers of brackets and such to find the names that match, and then go back up and match them, and then get into the forEach before you actually figure out why the heck this code exists at all. I suggested object literals (which can destruct easily on the callee) because they put the name before the value, so that humans can read the name and from that have some context in which to interpret the value. addLazyServiceGetter({name:"_name", contractID:SOME_CONTRACTID, interface:"nsIThing"}); reads left to right as ``add a lazy service getter with name "_name", using contractID of SOME_CONTRACTID and interface of "nsIThing"''. Typing cost in initialization code is the least of our concerns for our code! :-P

> (In reply to comment #2) > I didn't do that in the original code because I thought it added an extra level > of indirection that made the code harder to follow. Shawn, how is making a query helper different from using the defineLazyGetter helper instead of explicitly doing the __defineGetter__, delete, assign+return service? Is this much harder to follow? // Create lazy queries that can take additional conditions function makeLazyQuery([queryName, conditions]) { XPCOMUtils.defineLazyGetter(this, queryName, function() { return this._db.createStatement( SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", conditions, "g") ); }); } [["_defaultQuery", ""], ["_historyQuery", "AND h.visit_count > 0"], ["_bookmarkQuery", "AND bookmark IS NOT NULL"], ["_tagsQuery", "AND tags IS NOT NULL"], ["_typedQuery", "AND h.typed = 1"], ].forEach(makeLazyQuery, this); If you don't want the array of args, you can always just explicitly call it: makeLazyQuery("_defaultQuery", ""); makeLazyQuery("_historyQuery", "AND h.visit_count > 0"); (Yes, you'll need to adjust the args and either .call(this) or save this/self.)

writing SQL in JavaScript is losing. this patch can't save you.

Beware of delete on an object you want to be fast and "hidden-classy". Is the property deleted guaranteed to be the last (most recent) one added? If not, then you will pay, not only in TM but in Nitro (aka SFX) and AFAIK in v8. /be

(In reply to comment #17) > Beware of delete on an object you want to be fast and "hidden-classy". Is the > property deleted guaranteed to be the last (most recent) one added? If not, > then you will pay, not only in TM but in Nitro (aka SFX) and AFAIK in v8. No, but this is an incredibly common pattern in our code to defer work like creating a statement or caching a service until we need it (in the hope of not needing it ever).

Memoizing pattern examples welcome, in email if too long. If they all use delete then we may have to optimize accordingly. /be

I've seen 3 basic patterns: get foo() { delete this.foo; return this.foo = lazy(); } Uses delete but converts getter func to value. Short to write. (But not re-entrant-friendly.) get foo() { let val = lazy(); this.__defineGetter__("foo", function() val); return val; } Redefine the getter to look for val in its closure. (Hope there's no prototype conflict.) get foo() { if (this._foo === undefined) this._foo = lazy(); return this._foo; } "C-style" (?), dynamically checked each access. A "defineLazy" would probably declare "this._foo" in its own closure. In terms of optimization, I would assume the last choice is easiest to optimize. A obj.foo would notice that the getter function branch is biased and simplify to a lookup of obj._foo. The "delete" pattern only does the delete once during warm-up, and the common case is just a plain property access, so perhaps it isn't too bad. ?

(In reply to comment #20) > get foo() { > delete this.foo; > return this.foo = lazy(); > } This case doesn't work on the prototype, however.

(In reply to comment #21) > > get foo() { > > delete this.foo; > > return this.foo = lazy(); > This case doesn't work on the prototype, however. Oh, maybe that's what I was thinking about with prototype issues. But nothing a little bit of hackery can't fix! ;) get foo() { let proto = this.__proto__; this.__proto__ = {}; this.foo = lazy(); this.__proto__ = proto; return this.foo; } Oh hey! No more deletes :) (But I suppose __proto__ will break things..) function a() 0; A = 0; a.prototype = { get a() { let p = this.__proto__; this.__proto__ = {}; this.a = A++; this.__proto__ = p; return this.a; } }; [x,y,z] = [new a(), new a(), new a()]; [x.a,y.a,z.a,x.a,y.a,z.a] js> 0,1,2,0,1,2

(In reply to comment #22) > get foo() { > let proto = this.__proto__; > this.__proto__ = {}; > this.foo = lazy(); > this.__proto__ = proto; > return this.foo; > } I believe that this is overkill - you just can't delete the property on the prototype, but you can assign a property (untested though).

asking approval because: - no risk (just replacing code with analogue code) - cleaner code - sync code between branches (avoid bitrotting patches if possible)