This API will follow closely with the new Places Query API: https://bugzilla.mozilla.org/show_bug.cgi?id=522572 This will be an async API where callers will need to pass in callbacks to manipulate results. There is no getting around this, as any new APIs touching Firefox need to stay off of the main thread.

Just wanted to mention that some discussion of this vis a vis Jetpack is happening here: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/b5f177128c75f35

Still getting this working, probably will not be ready for the 0.7 push, but most likely ready next week.

In any event, hank you for trying to get it in. 0.8 will be pretty feature rich I guess :)

I still need to write some tests that touch the UI as well as an example/demo

This is a wip patch, tests fail because: ++DOCSHELL 0xb57cdd30 == 2 ++DOMWINDOW == 3 (0xb76d9410) [serial = 3] [outer = (nil)] ++DOMWINDOW == 4 (0xb76d95e0) [serial = 4] [outer = 0xb76d93e0] error: An exception occurred. Traceback (most recent call last): object has no __exposedProps__! Tracked memory objects in testing sandbox: 2 0 of 1 tests passed. FAIL ----------------------------------------------------- my exposed props are listed here: exports.create = function create(jetpack, params) { return { __exposedProps__: { find: 'r' , bookmark: "r"}, ... } see patch for more details. I basically stuck my "PlacesQuery" function inside of an "init" method in the capability object. I use a getter to init the PlacesQuery object (if it is not already initialized) so it is treated like a Singleton.

Hmm, so one thing wrt: exposedProps is that *every* object your capability exposes to untrusted content needs to have __exposedProps__ on it, not just the initial capability--not sure if you are currently doing that or not, as I haven't yet looked at the code. As hanson mentioned in the workday meeting a few days ago, you'll also probably want to actually write the core chrome-space logic in a separate commonjs module and just have your capability do simple delegation with it. That way your unit tests can run in chrome space and test the actual chrome-space logic and you can have separate unit tests that make sure things work from untrusted code. Another advantage of this is that it allows your chrome-space functionality to be reused by chrome-space code, rather than just untrusted jetpacks.

I also just turned the exceptions thrown by the secure membrane code (e.g. "object has no __exposedProps__!") into full exceptions w/ tracebacks, so they should be much easier to debug now that they're not just naked strings.

Ahh! Ok, So I can perhaps (on this first go round) pull the places code out of the exports object and just make sure it has an __exposedProps__ set corerctly. Cool.

(In reply to comment #7) > I also just turned the exceptions thrown by the secure membrane code (e.g. > "object has no __exposedProps__!") into full exceptions w/ tracebacks, so they > should be much easier to debug now that they're not just naked strings. I just updated the jep-28 and atul-packages repos and I do not see the full exceptions w/ tracebacks

fixed diff issue

Comment on attachment 421963 [details] [diff] [review] v 1.0.1 Places Sync Patch for Jetpack >+ return new s.PlacesQuery({ phrase:"foo" }); "foo"? >+EXPORTED_SYMBOLS = ["PlacesQuery"]; const? >+this.__defineGetter__("hs", function (){ >+ let hs = Cc["@mozilla.org/browser/nav-history-service;1"]. >+ getService(Ci.nsINavHistoryService); >+ return hs; >+}); >+ >+this.__defineGetter__("io", function (){ >+ let io = Cc["@mozilla.org/network/io-service;1"]. >+ getService(Ci.nsIIOService); >+ return io; >+}); >+ >+this.__defineGetter__("bs", function (){ >+ let bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. >+ getService(Ci.nsINavBookmarksService); >+ return bs; >+}); >+ >+this.__defineGetter__("ts", function (){ >+ var ts = Cc["@mozilla.org/browser/tagging-service;1"]. >+ getService(Ci.nsITaggingService); >+ return ts; >+}); Perhaps memoize these unary getters? >+function isArray(aObj) { >+ // all of this is needed as an array created in a different frame >+ // will go undetected as an array, it will be thought of as an Object >+ if (typeof aObj.length === 'number' && >+ !(aObj.propertyIsEnumerable('length')) && >+ typeof aObj.splice === 'function') { >+ return true; >+ } >+ return false; >+} This duck typing seems reasonable, but note that other Mozilla code I've read (and written) tends to simply check |aObj.constructor.name == "Array"| when testing for arrays across JavaScript contexts. >+function isNotAString(aObj) { >+ return typeof aObj != 'string'; >+} >+ >+let ERRORS = { const? > handOff: function PlacesQuery_handOff() { This method doesn't seem to be called anywhere but from the constructor. Does its code need to be in a separate method? For that matter, PlacesQuery only does a little bit of work before creating and returning an instance of QueryExecutor. Perhaps the two objects could be consolidated? >+function PlacesQuery(aQueryConf, aCallback) { ... >+ this.queryConf = { phrase: phrase, >+ where: 'bookmarks', >+ callback: aCallback }; ... >+ let QueryX = new QueryExecutor(this.queryConf); ... >+function QueryExecutor(queryConf, aCallback) { ... >+ this.reconfigure = function reconfigure(queryConf, aCallback) { >+ self.queryConf = queryConf; >+ if (aCallback) { >+ self.queryConf.callback = aCallback; >+ } It's unclear whether or not query configuration objects are supposed to contain callbacks. In some places they do, while in other places there is a separate callback argument. This should be consistent. >+ if(this.firstRun == undefined) { >+ this.firstRun = true; >+ } >+ else { >+ this.firstRun = false; >+ } How could this.firstRun ever be defined in the constructor, given that it isn't defined by the constructor's prototype? >+ this.reconfigure = function reconfigure(queryConf, aCallback) { Is there a particular reason why this method is defined inside the constructor rather than in the prototype declaration? There doesn't seem to be anything in it that needs to close around the constructor's scope. >+QueryExecutor.prototype = { >+ >+ queryConfHistory: [], I don't see this being used anywhere. Is it needed? If so, is it supposed to be a global history across all query executors? If not, it needs to be defined in the constructor, since a literal array here will be shared by all instances. >+ >+ // main public API lives here >+ find: function QueryExecutor_find(queryConf, aCallback) { queryConf -> aQueryConf? (or aCallback -> callback?) >+ // root.containerOpen = false; Is this not needed, or is it not working as intended? It'd be good to have a comment here explaining why this is commented out. >+ narrow: function QueryExecutor_narrow(aObj) { >+ throw NS_ERROR_NOT_IMPLEMENTED; >+ }, >+ >+ widen: function QueryExecutor_widen(aObj) { >+ throw NS_ERROR_NOT_IMPLEMENTED; >+ }, Under what circumstances might these get called? >+function QE_bookmark() { >+ // getter that returns object that creates bookmarks Since this makes bookmarks but isn't a bookmark itself, perhaps it would be better called BookmarkFactory? >+ throw new Error("Url is not valid"); Add this string to ERRORS? >+ } >+ >+ // XXX: fixme: check for duplicates is not comprehensive >+ if (this.exists(aObj.url)) { >+ throw new Error(ERRORS.BOOKMARK_EXISTS); >+ } Isn't it ok to create a bookmark twice (f.e. in two different folders)? >+ throw new Error(ERORRS.FOUND_RECORD_MISMATCH); ERORRS -> ERRORS >+ for (let idx in results) { >+ self.createdBookmarks.push(results[idx]); >+ } Use |for each| or |for (;;)| to iterate arrays, as they explicitly iterate array items, while |for| iterates object properties (which happens to work for arrays but is less safe). But do you even need to iterate at all here? Right beforehand you made sure that results.length == 1, so you should be able to just push results[0], no? >+ } >+ else { >+ if (!strictTypeOf(aObj) == 'object') { This is going to evaluate as |((!strictTypeOf(aObj)) == 'object')| because the NOT operator has higher precedence than the equality one, and |(!strictTypeOf(aObj))| will always evaluate to true or false, neither of which are equal to "object", so this expression will always evaluate to false. If I understand the intent here correctly, then use |(strictTypeOf(aObj) != 'object') instead. >+ update: function QE_bookmark_update(aBookmark, aCallback) { This should throw something about not being implemented so potential consumers aren't misled. >+const TAGS = ", "; This confused me when I first saw it. Perhaps it could be called TAGS_SEPARATOR or the like? >+ get tags() { >+ try { >+ return this._rawNode.tags.split(TAGS); >+ } >+ catch (ex) { >+ return []; >+ } >+ }, What kind of exceptions might be thrown here? If it's just that tags might not exist, then note that children returns "null" if there are no children, while this getter returns an empty array. Those should be consistent, no? >+ duplicate: function QueryExecutor_duplicate(aObj) { >+ return duplicate(aObj); The duplicate function doesn't seem to exist. >+ResultItem_getNodeType = function () { >+ let typeObj = { >+ 0: 'uri', >+ 1: 'visit', >+ 2: 'full_visit', >+ 4: 'container', >+ 5: 'query', >+ 6: 'folder', >+ 7: 'separator', >+ 9: 'shortcut' >+ }; If a consumer iterates a large result set and checks each item's nodeType, then redefining these constants at every invocation could potentially impact performance. It'd be better to define them separately in a global const, as with ERRORS. Also, this seems to be defined as a class getter rather than an instance getter, although it appears to be trying to return the instance's node type. I don't think this is going to behave as expected; shouldn't this be defined as a member of ResultItem.prototype?

(In reply to comment #12) > (From update of attachment 421963 [details] [diff] [review]) > >+ return new s.PlacesQuery({ phrase:"foo" }); > > "foo"? > Becasue of the way this module will be used in Jetpack, where you have a getter that instantiates it, you have to have *a phrase* to initialize everything. It is never used, just overwritten immediately. In the latest iteration I use the string "__init__". > > >+EXPORTED_SYMBOLS = ["PlacesQuery"]; > > const? I can make it a const, i don't remember marco having an issue with that when he reviewed this. > > > >+this.__defineGetter__("hs", function (){ > >+ let hs = Cc["@mozilla.org/browser/nav-history-service;1"]. > >+ getService(Ci.nsINavHistoryService); > >+ return hs; > >+}); > >+ > >+this.__defineGetter__("io", function (){ > >+ let io = Cc["@mozilla.org/network/io-service;1"]. > >+ getService(Ci.nsIIOService); > >+ return io; > >+}); > >+ > >+this.__defineGetter__("bs", function (){ > >+ let bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. > >+ getService(Ci.nsINavBookmarksService); > >+ return bs; > >+}); > >+ > >+this.__defineGetter__("ts", function (){ > >+ var ts = Cc["@mozilla.org/browser/tagging-service;1"]. > >+ getService(Ci.nsITaggingService); > >+ return ts; > >+}); > > Perhaps memoize these unary getters? > The above getters were devised by the places team review. > > >+function isArray(aObj) { > >+ // all of this is needed as an array created in a different frame > >+ // will go undetected as an array, it will be thought of as an Object > >+ if (typeof aObj.length === 'number' && > >+ !(aObj.propertyIsEnumerable('length')) && > >+ typeof aObj.splice === 'function') { > >+ return true; > >+ } > >+ return false; > >+} > > This duck typing seems reasonable, but note that other Mozilla code I've read > (and written) tends to simply check |aObj.constructor.name == "Array"| when > testing for arrays across JavaScript contexts. > I'll try that, marco had a suggestion about using instanceof Array, but that did not work in jetpack. this is basically reipped right from crockford's site. > > >+function isNotAString(aObj) { > >+ return typeof aObj != 'string'; > >+} > >+ > >+let ERRORS = { > > const? > Yeah, the ERRORS should be a const, agian, this was not deemed needed in marco's review. > > > handOff: function PlacesQuery_handOff() { > > This method doesn't seem to be called anywhere but from the constructor. Does > its code need to be in a separate method? > > For that matter, PlacesQuery only does a little bit of work before creating and > returning an instance of QueryExecutor. Perhaps the two objects could be > consolidated? > I guess they could be. Not sure if I want to spend the time at this late stage. This iteration of jetpack.places will be very short-lived. it is synchronous after all. > > >+function PlacesQuery(aQueryConf, aCallback) { > ... > >+ this.queryConf = { phrase: phrase, > >+ where: 'bookmarks', > >+ callback: aCallback }; > ... > >+ let QueryX = new QueryExecutor(this.queryConf); > ... > >+function QueryExecutor(queryConf, aCallback) { > ... > >+ this.reconfigure = function reconfigure(queryConf, aCallback) { > >+ self.queryConf = queryConf; > >+ if (aCallback) { > >+ self.queryConf.callback = aCallback; > >+ } > > It's unclear whether or not query configuration objects are supposed to contain > callbacks. In some places they do, while in other places there is a separate > callback argument. This should be consistent. > The callback is always optional. > > >+ if(this.firstRun == undefined) { > >+ this.firstRun = true; > >+ } > >+ else { > >+ this.firstRun = false; > >+ } > > How could this.firstRun ever be defined in the constructor, given that it isn't > defined by the constructor's prototype? > Actually i think that code is going to be removed, part of an earlier direction. > > >+ this.reconfigure = function reconfigure(queryConf, aCallback) { > > Is there a particular reason why this method is defined inside the constructor > rather than in the prototype declaration? There doesn't seem to be anything in > it that needs to close around the constructor's scope. > Yes, the callback is not always going to be regenerated or passed in: e.g.: let PlacesQueryObject = jetpack.places.find({phrase: "groucho marx"}, callback); // now change the phrase and call again, using the same callback: PlacesQueryObject.find({phrase:"jaque cousteau"}); that was my intention anyway. I can see a need to re-use the callback as is, but slightly change the phrase, over and over. > > >+QueryExecutor.prototype = { > >+ > >+ queryConfHistory: [], > > I don't see this being used anywhere. Is it needed? If so, is it supposed to > be a global history across all query executors? If not, it needs to be defined > in the constructor, since a literal array here will be shared by all instances. > right. whoops. > > >+ > >+ // main public API lives here > >+ find: function QueryExecutor_find(queryConf, aCallback) { > > queryConf -> aQueryConf? (or aCallback -> callback?) > > > >+ // root.containerOpen = false; > > Is this not needed, or is it not working as intended? It'd be good to have a > comment here explaining why this is commented out. > i think it will be removed > > >+ narrow: function QueryExecutor_narrow(aObj) { > >+ throw NS_ERROR_NOT_IMPLEMENTED; > >+ }, > >+ > >+ widen: function QueryExecutor_widen(aObj) { > >+ throw NS_ERROR_NOT_IMPLEMENTED; > >+ }, > > Under what circumstances might these get called? > none whatsoever. will remove. > > >+function QE_bookmark() { > >+ // getter that returns object that creates bookmarks > > Since this makes bookmarks but isn't a bookmark itself, perhaps it would be > better called BookmarkFactory? > I wanted this api to be short and sweet. If you mean this function name, not the api, then sure, that would be better. > > >+ throw new Error("Url is not valid"); > > Add this string to ERRORS? > whoops. will do. > > >+ } > >+ > >+ // XXX: fixme: check for duplicates is not comprehensive > >+ if (this.exists(aObj.url)) { > >+ throw new Error(ERRORS.BOOKMARK_EXISTS); > >+ } > > Isn't it ok to create a bookmark twice (f.e. in two different folders)? > Yeah. Will remove. > > >+ throw new Error(ERORRS.FOUND_RECORD_MISMATCH); > > ERORRS -> ERRORS > > > >+ for (let idx in results) { > >+ self.createdBookmarks.push(results[idx]); > >+ } > > Use |for each| or |for (;;)| to iterate arrays, as they explicitly iterate > array items, while |for| iterates object properties (which happens to work for > arrays but is less safe). > > But do you even need to iterate at all here? Right beforehand you made sure > that results.length == 1, so you should be able to just push results[0], no? > push results[0] sounds more efficient. Python iterastion is the death of me in JS:) > > >+ } > >+ else { > >+ if (!strictTypeOf(aObj) == 'object') { > > This is going to evaluate as |((!strictTypeOf(aObj)) == 'object')| because the > NOT operator has higher precedence than the equality one, and > |(!strictTypeOf(aObj))| will always evaluate to true or false, neither of which > are equal to "object", so this expression will always evaluate to false. If I > understand the intent here correctly, then use |(strictTypeOf(aObj) != > 'object') instead. > Will do. > > >+ update: function QE_bookmark_update(aBookmark, aCallback) { > > This should throw something about not being implemented so potential consumers > aren't misled. > ok. > > >+const TAGS = ", "; > > This confused me when I first saw it. Perhaps it could be called > TAGS_SEPARATOR or the like? > Now I am laughing. I had something like that in the previous iteration. In Marco's review he asked me to change it to "TAGS" > > >+ get tags() { > >+ try { > >+ return this._rawNode.tags.split(TAGS); > >+ } > >+ catch (ex) { > >+ return []; > >+ } > >+ }, > > What kind of exceptions might be thrown here? If it's just that tags might not > exist, then note that children returns "null" if there are no children, while > this getter returns an empty array. Those should be consistent, no? > It may throw if you are getting history objects vs. bookmarks. > > >+ duplicate: function QueryExecutor_duplicate(aObj) { > >+ return duplicate(aObj); > > The duplicate function doesn't seem to exist. > This will be removed. > > >+ResultItem_getNodeType = function () { > >+ let typeObj = { > >+ 0: 'uri', > >+ 1: 'visit', > >+ 2: 'full_visit', > >+ 4: 'container', > >+ 5: 'query', > >+ 6: 'folder', > >+ 7: 'separator', > >+ 9: 'shortcut' > >+ }; > > If a consumer iterates a large result set and checks each item's nodeType, then > redefining these constants at every invocation could potentially impact > performance. It'd be better to define them separately in a global const, as > with ERRORS. > ok. right on. > Also, this seems to be defined as a class getter rather than an instance > getter, although it appears to be trying to return the instance's node type. I > don't think this is going to behave as expected; shouldn't this be defined as a > member of ResultItem.prototype? hmmm. I will investigate. Thanks Myk! a new patch is coming up asap.

(In reply to comment #13) > > >+QueryExecutor.prototype = { > > >+ > > >+ queryConfHistory: [], > > > > I don't see this being used anywhere. Is it needed? If so, is it supposed to > > be a global history across all query executors? If not, it needs to be defined > > in the constructor, since a literal array here will be shared by all instances. Actually it is used inside of the "reconfigure" method.

Comment on attachment 422285 [details] [diff] [review] v 1.0.2 Places Sync Patch for Jetpack (In reply to comment #13) > > >+EXPORTED_SYMBOLS = ["PlacesQuery"]; > > > > const? > > I can make it a const, i don't remember marco having an issue with that when > he reviewed this. It's not a big deal, but it's a good practice to declare all global identifiers using either const, var, or let. Also, I didn't realize someone else had reviewed the code, but multiple reviews are complementary (which is why they're beneficial in the first place), so consider these review comments independently of whether or not Marco identified the same issues (except where we explicitly disagree!). ... > > >+this.__defineGetter__("ts", function (){ > > >+ var ts = Cc["@mozilla.org/browser/tagging-service;1"]. > > >+ getService(Ci.nsITaggingService); > > >+ return ts; > > >+}); > > > > Perhaps memoize these unary getters? > > > > The above getters were devised by the places team review. And they work, but they will be inefficient if accessed frequently because they cross XPCOM boundaries every time they are accessed. Thus best practice is to memoize such getters, although this is not essential (especially if they will be accessed infrequently). > I'll try that, marco had a suggestion about using instanceof Array, but that > did not work in jetpack. this is basically reipped right from crockford's site. Right, |instanceof Array| won't work because Array != Array across JavaScript contexts. Crockford's suggestion is best practice for cross-browser JavaScript, as (at least) IE doesn't implement the non-standard Function.name property <https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Function/name>. But for Firefox code, where we have the luxury that property, best practice is to check the constructor name. > > >+ this.reconfigure = function reconfigure(queryConf, aCallback) { > > > > Is there a particular reason why this method is defined inside the constructor > > rather than in the prototype declaration? There doesn't seem to be anything in > > it that needs to close around the constructor's scope. > > > > Yes, the callback is not always going to be regenerated or passed in: > > e.g.: let PlacesQueryObject = jetpack.places.find({phrase: "groucho marx"}, > callback); > // now change the phrase and call again, using the same callback: > PlacesQueryObject.find({phrase:"jaque cousteau"}); > > that was my intention anyway. I can see a need to re-use the callback as is, > but slightly change the phrase, over and over. Ah, now I understand! So, in that case I would suggest iterating the queryConf argument and updating only those properties present in it (just as you do in the constructor!) to avoid the duplication and complexity of having the callback be passable both in the queryConf object and as a separate parameter, i.e.: this.reconfigure = function reconfigure(queryConf) { for (let prop in queryConf) self.queryConf[prop] = queryConf[prop]; But that is actually orthogonal to the question of why the reconfigure method is defined as a closure around the constructor's scope. It doesn't access that scope except via |self|, but |self| is unnecessary, since reconfigure is always called as a method of a QueryConf instance, so it can simply use |this|. It is conventional to define such a method inside its constructor's prototypes unless a closure is actually necessary, which it isn't in this case (and as a side effect, defining it in the prototype will make the constructor code much simpler). > > >+function QE_bookmark() { > > >+ // getter that returns object that creates bookmarks > > > > Since this makes bookmarks but isn't a bookmark itself, perhaps it would be > > better called BookmarkFactory? > > > > I wanted this api to be short and sweet. If you mean this function name, not > the api, then sure, that would be better. Yes, just the function name! I'm not reviewing the API, just the implementation. :-) > > >+const TAGS = ", "; > > > > This confused me when I first saw it. Perhaps it could be called > > TAGS_SEPARATOR or the like? > > > Now I am laughing. I had something like that in the previous iteration. In > Marco's review he asked me to change it to "TAGS" No worries, it's a minor issue, either way is fine. > > >+ get tags() { > > >+ try { > > >+ return this._rawNode.tags.split(TAGS); > > >+ } > > >+ catch (ex) { > > >+ return []; > > >+ } > > >+ }, > > > > What kind of exceptions might be thrown here? If it's just that tags might not > > exist, then note that children returns "null" if there are no children, while > > this getter returns an empty array. Those should be consistent, no? > > > > It may throw if you are getting history objects vs. bookmarks. So, if it makes no sense to get tags for history objects, then it might be worth actually throwing an exception here, but either way, tags and children should be consistent about how they behave if neither of them makes sense for history objects, i.e. they should both throw an exception, both return null, or both return an empty array. >+ "jetpack.places": function(context) { >+ var s = {}; >+ Cu.import("resource://jetpack/modules/places.js", s); >+ return new s.PlacesQuery({ phrase:"foo" }); Hmm, didn't you say "foo" was going to be replaced by "__init__"? You use "__init__" later in the code. Unless there's a reason to do otherwise, it seems like these should both use "__init__". >+function isDate(aObj) { >+ return typeof aObj == 'object' && aObj instanceof Date; >+} This is not going to work across JavaScript contexts for the same reason |ary instanceof Array| won't work. But I also don't see this being used except in strictTypeOf. Is it still needed? If so, it should check |aObj.constructor.name == Date|; otherwise it could be removed. >+ throw new Error("Url is not valid"); Nit: move this to the ERRORS collection. Otherwise, looking good!

(In reply to comment #16) > Also, I didn't realize someone else had reviewed the code, but multiple reviews > are complementary (which is why they're beneficial in the first place), so > consider these review comments independently fwiw, review was not yet complete, and you're right about complementary reviews, and also about this comment. i'll take a look at the rest, and eventually partecipate in the discussion if needed.

*cough*XPCOMUtils.defineLazy[Service]Getter*cough*

Myk: I was convinced I needed a closure in the constructor. my bad. thanks for pointing that out. as far as tags and children, I am going to return null for both. as far as lazy getters, what is was soposed to happen there totally escaped my mind. now I get it. Going with XPCOMUtils solution proferred by sdwilsh. new patch asap.

Comment on attachment 422285 [details] [diff] [review] v 1.0.2 Places Sync Patch for Jetpack >diff --git a/extension/content/js/jetpack-environment.js b/extension/content/js/jetpack-environment.js > > "jetpack.Menu": function (context) { just out of curiosity, why is Menu here uppercase, looks like others are not >+ return new s.PlacesQuery({phrase:"__init__"}); i agree that this obliged "phrase" here does not look good, i think you can easily solve the missing param in the constructor and just call new s.PlacesQuery(); >diff --git a/extension/content/js/tests/test-places.js b/extension/content/js/tests/test-places.js >+let domainPrefix = Date.now(); const DOMAIN_PREFIX >+let _url = "http://baz." + domainPrefix + ".mozilla.com/"; const TEST_URL >+var PlacesTests = { reason to use var here when you use let everywhere? >+ testAddAndFindBookmark: function (self) { >+ let me = this; >+ let bObj = { url: _url, >+ title: "my mozilla " + domainPrefix, >+ tags: ["baz", "mozilla"] >+ }; >+ function callbackA(createdBookmarks) { >+ self.assert(createdBookmarks.length == 1); >+ self.assert(createdBookmarks[0].uri == _url); you init "me", and then use "self"... that makes me think test is broken? >diff --git a/extension/modules/places.js b/extension/modules/places.js >+this.__defineGetter__("hs", function (){ myk is more than right, so as we discussed on irc, you should follow sdwilsh's suggestion about defineLazyServiceGetter >+function uri(spec) { >+ return io.newURI(spec, null, null); >+} you could also import and use NetUtil.newURI >+function log(message) { >+ dump(message + "\n"); >+} looks like not useful in final version of the module, do you use it? >+function isArray(aObj) { >+ return (aObj.constructor.name == "Array"); i think makes still sense to check typeof aObj == 'object' before this, unless it's not compatible. >+ let _results = new Array(); as i said previously, unless this is a jetpack convention, does not look like our convention having _var unless they are "private" properties >+QE_bookmarkFactory.prototype = { >+ createOne: function QE_bookmark_createOne(aObj) { >+ if (!aObj.url) { i think you missed some of the comments i put in my last review pass, this should first check ("url" in aObj) >+const RESULT_ITEM_TYPE_OBJ = { >+ 0: 'uri', >+ 1: 'visit', >+ 2: 'full_visit', >+ 4: 'container', >+ 5: 'query', >+ 6: 'folder', >+ 7: 'separator', >+ 9: 'shortcut' i guess nobdy will complain if you add a trailing comma here to avoid polluting blame in future additions >+ for (let i=0; i < this._rawNode.childCount; i++) { spacing in i=0, unless jetpack has different code style >+ get lastAccessTime() { >+ return this._rawNode.time; please check again my last review comments, i guess if these should be converted to timestamps or date object, since they are microseconds that is not really js default for time. apart these i agree with Myk's comments.

(In reply to comment #20) > (From update of attachment 422285 [details] [diff] [review]) > >diff --git a/extension/content/js/jetpack-environment.js b/extension/content/js/jetpack-environment.js > > > > > "jetpack.Menu": function (context) { > > just out of curiosity, why is Menu here uppercase, looks like others are not > no idea - not my code. > >+ return new s.PlacesQuery({phrase:"__init__"}); > > i agree that this obliged "phrase" here does not look good, i think you can > easily solve the missing param in the constructor and just call new > s.PlacesQuery(); > ok, i can change the constructor > >diff --git a/extension/content/js/tests/test-places.js b/extension/content/js/tests/test-places.js > >+let domainPrefix = Date.now(); > > const DOMAIN_PREFIX > done > >+let _url = "http://baz." + domainPrefix + ".mozilla.com/"; > > const TEST_URL > done > >+var PlacesTests = { > > reason to use var here when you use let everywhere? > my bad - fixed > > >+ testAddAndFindBookmark: function (self) { > >+ let me = this; > >+ let bObj = { url: _url, > >+ title: "my mozilla " + domainPrefix, > >+ tags: ["baz", "mozilla"] > >+ }; > >+ function callbackA(createdBookmarks) { > >+ self.assert(createdBookmarks.length == 1); > >+ self.assert(createdBookmarks[0].uri == _url); > > you init "me", and then use "self"... > that makes me think test is broken? > sorry "me" was part of old cruft - gone now > >diff --git a/extension/modules/places.js b/extension/modules/places.js > > >+this.__defineGetter__("hs", function (){ > > myk is more than right, so as we discussed on irc, you should follow sdwilsh's > suggestion about defineLazyServiceGetter > > >+function uri(spec) { > >+ return io.newURI(spec, null, null); > >+} > > you could also import and use NetUtil.newURI > done and done. very slick. > >+function log(message) { > >+ dump(message + "\n"); > >+} > removed > looks like not useful in final version of the module, do you use it? > > >+function isArray(aObj) { > >+ return (aObj.constructor.name == "Array"); > > i think makes still sense to check typeof aObj == 'object' before this, unless > it's not compatible. > > >+ let _results = new Array(); > > as i said previously, unless this is a jetpack convention, does not look like > our convention having _var unless they are "private" properties > changed to results, etc > >+QE_bookmarkFactory.prototype = { > >+ createOne: function QE_bookmark_createOne(aObj) { > >+ if (!aObj.url) { > > i think you missed some of the comments i put in my last review pass, this > should first check ("url" in aObj) > > >+const RESULT_ITEM_TYPE_OBJ = { > >+ 0: 'uri', > >+ 1: 'visit', > >+ 2: 'full_visit', > >+ 4: 'container', > >+ 5: 'query', > >+ 6: 'folder', > >+ 7: 'separator', > >+ 9: 'shortcut' > > i guess nobdy will complain if you add a trailing comma here to avoid polluting > blame in future additions > done > >+ for (let i=0; i < this._rawNode.childCount; i++) { > > spacing in i=0, unless jetpack has different code style > > >+ get lastAccessTime() { > >+ return this._rawNode.time; > fixed this so i do: return new Date((this._rawNode.time / 1000)); > please check again my last review comments, i guess if these should be > converted to timestamps or date object, since they are microseconds that is not > really js default for time. > going through your other comments now > apart these i agree with Myk's comments.

at first glance you did not convert creationTime and modificationTime, they should be the same format as lastAccessTime.

fixed all dates to return actual Date objects

Comment on attachment 423015 [details] [diff] [review] v 1.0.4 Places Sync Patch for Jetpack This is looking pretty good. There are just a few remaining issues... >+function PlacesQuery(aQueryConf, aCallback) { ... >+} >+ >+PlacesQuery.prototype = { ... >+}; Since this constructor and prototype just initializes and returns an instance of QueryExecutor without any side effects, it's unnecessary to have both classes, so they really should get consolidated into a single one, although I guess it's ok as is if this implementation is going to be as short-lived as you suggest. Also, JEP 22 says you can call jetpack.places with a simple string and callback, but this code doesn't seem to support that. Is that not being implemented yet, or have plans changed (and the JEP just hasn't been updated yet)? >+ if (this.firstRun == undefined) { >+ this.firstRun = true; >+ } >+ else { >+ this.firstRun = false; >+ } This property isn't used anywhere and should be removed. >+ this.queryConfHistory = []; This is modified in reconfigure(), but then nothing actually uses it. Unless this is supposed to be part of the public-facing API, there's no point in keeping that information around, so it should be removed. >+ reconfigure: function QE_reconfigure(aQueryConf, aCallback) { >+ for (let prop in aQueryConf) { >+ this.queryConf[prop] = aQueryConf[prop]; >+ } >+ if (aCallback) { >+ this.queryConf.callback = aCallback; >+ } In my first review I said we should drop the separate aCallback argument and just pass the callback as a property of the aQueryConf argument. But looking at the JEP, I see that a separate aCallback argument is a part of the proposed API. I still think it's redundant, and we'd be better off with just a single way to specify the callback, but we can cross that bridge at the next API review, and this is fine for now. >+ this.queryOptions.resultType = this.queryOptions.RESULTS_AS_URI; Nit: access the RESULTS_AS_URI constant via Ci.nsINavHistoryQueryOptions for consistency and per best practice. >+ find: function QueryExecutor_find(queryConf, aCallback) { Nit: queryConf -> aQueryConf or aCallback -> callback >+ find: function QueryExecutor_find(queryConf, aCallback) { >+ let callback = null; >+ // Check if a new phrase is being used with an existing PlacesQuery obj. >+ if (("callback" in this.queryConf) && aCallback == undefined) { >+ callback = this.queryConf.callback; >+ } >+ else if (typeof aCallback == 'function') { >+ callback = aCallback; >+ } >+ else { >+ // noop >+ } >+ if (queryConf) { >+ // aCallback is passed in here even if it is 'undefined'. >+ this.reconfigure(queryConf, callback); >+ } If |typeof aCallback == 'function'| was the first conditional tested, then you wouldn't have to check for |aCallback == undefined| when checking for |"callback" in this.queryConf|. However, if this code were to call reconfigure() first, passing it aCallback, then it wouldn't need the conditional at all, because reconfigure() would update this.queryConf.callback, and then this code could just use that, i.e.: find: function QueryExecutor_find(aQueryConf, aCallback) { this.reconfigure(aQueryConf, aCallback); ... if (this.queryConf.callback) { this.queryConf.callback(results); } In order for that not to cause an infinite loop, however, you'd have to eliminate the cycle between find() and reconfigure(), whereby find() calls reconfigure(), which then calls find() again. But eliminating that cycle is a good idea anyway, since the cycle causes the query to execute twice, which is unnecessary and inefficient, and cycle also creates the dangerous possibility for a future programmer to accidentally create an infinite loop (f.e. when trying to introduce the optimization I describe above). Fortunately, breaking the cycle is easy: instead of having the constructor call reconfigure(), which then calls find(), have the constructor call find(), which then calls reconfigure(), i.e.: function QueryExecutor(aQueryConf, aCallback) { ... if (aQueryConf.phrase != "__init__") this.find(aQueryConf, aCallback); } QueryExecutor.prototype = { reconfigure: function QueryExecutor_reconfigure(aQueryConf, aCallback) { ... // doesn't call find }, ... find: function QueryExecutor_find(aQueryConf, aCallback) { this.reconfigure(aQueryConf, aCallback); ... } ... } >+ if (results.length > 1) { >+ throw new Error(ERORRS.FOUND_RECORD_MISMATCH); ERORRS -> ERRORS >+ let queryConf = { where: "bookmarks", >+ phrase: bmUri.spec }; Since multiple bookmarks can share a URI spec, this can legitimately return multiple results, so you need to handle that case. Perhaps it would be better to retrieve the bookmark by the ID that insertBookmark returns. (In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 422285 [details] [diff] [review] [details]) > > >diff --git a/extension/content/js/jetpack-environment.js b/extension/content/js/jetpack-environment.js > > > > > > > > "jetpack.Menu": function (context) { > > > > just out of curiosity, why is Menu here uppercase, looks like others are not > > > > no idea - not my code. There's some inconsistency in our APIs, as some of them are designed to work like JavaScript constructors, where you instantiate a thing by calling: new jetpack.Thing(); While others are designed to work like jQuery constructors, where you instantiate a thing by calling: jetpack.thing(); We'll need to make these consistent in the reboot, but for now either way is fine.

(In reply to comment #25) > (From update of attachment 423015 [details] [diff] [review]) > This is looking pretty good. There are just a few remaining issues... > > > >+function PlacesQuery(aQueryConf, aCallback) { > ... > >+} > >+ > >+PlacesQuery.prototype = { > ... > >+}; > > Since this constructor and prototype just initializes and returns an instance > of QueryExecutor without any side effects, it's unnecessary to have both > classes, so they really should get consolidated into a single one, although I > guess it's ok as is if this implementation is going to be as short-lived as you > suggest. I'm not sure I want to combine them right now. perhaps if this version of the patch lives longer than I hope it would, I will do the combination in the next release. > > Also, JEP 22 says you can call jetpack.places with a simple string and > callback, but this code doesn't seem to support that. Is that not being > implemented yet, or have plans changed (and the JEP just hasn't been updated > yet)? > I have made some changes to JEP 22 to be in line with this patch's actual implementation. I have removed the ability to pass in a string. Tht is slated for the next go around when we have more control and are able to do unified searches. > > >+ if (this.firstRun == undefined) { > >+ this.firstRun = true; > >+ } > >+ else { > >+ this.firstRun = false; > >+ } > > This property isn't used anywhere and should be removed. > > I added it to the prototype. And it is used. > >+ this.queryConfHistory = []; > > This is modified in reconfigure(), but then nothing actually uses it. Unless > this is supposed to be part of the public-facing API, there's no point in > keeping that information around, so it should be removed. > removed. > > >+ reconfigure: function QE_reconfigure(aQueryConf, aCallback) { > >+ for (let prop in aQueryConf) { > >+ this.queryConf[prop] = aQueryConf[prop]; > >+ } > >+ if (aCallback) { > >+ this.queryConf.callback = aCallback; > >+ } > > In my first review I said we should drop the separate aCallback argument and > just pass the callback as a property of the aQueryConf argument. But looking > at the JEP, I see that a separate aCallback argument is a part of the proposed > API. I still think it's redundant, and we'd be better off with just a single > way to specify the callback, but we can cross that bridge at the next API > review, and this is fine for now. > cool > > >+ this.queryOptions.resultType = this.queryOptions.RESULTS_AS_URI; > > Nit: access the RESULTS_AS_URI constant via Ci.nsINavHistoryQueryOptions for > consistency and per best practice. > Done > > >+ find: function QueryExecutor_find(queryConf, aCallback) { > > Nit: queryConf -> aQueryConf or aCallback -> callback > > > >+ find: function QueryExecutor_find(queryConf, aCallback) { > >+ let callback = null; > >+ // Check if a new phrase is being used with an existing PlacesQuery obj. > >+ if (("callback" in this.queryConf) && aCallback == undefined) { > >+ callback = this.queryConf.callback; > >+ } > >+ else if (typeof aCallback == 'function') { > >+ callback = aCallback; > >+ } > >+ else { > >+ // noop > >+ } > >+ if (queryConf) { > >+ // aCallback is passed in here even if it is 'undefined'. > >+ this.reconfigure(queryConf, callback); > >+ } > > If |typeof aCallback == 'function'| was the first conditional tested, then you > wouldn't have to check for |aCallback == undefined| when checking for > |"callback" in this.queryConf|. > > However, if this code were to call reconfigure() first, passing it aCallback, > then it wouldn't need the conditional at all, because reconfigure() would > update this.queryConf.callback, and then this code could just use that, i.e.: > > find: function QueryExecutor_find(aQueryConf, aCallback) { > this.reconfigure(aQueryConf, aCallback); > ... > if (this.queryConf.callback) { > this.queryConf.callback(results); > } > > In order for that not to cause an infinite loop, however, you'd have to > eliminate the cycle between find() and reconfigure(), whereby find() calls > reconfigure(), which then calls find() again. > > But eliminating that cycle is a good idea anyway, since the cycle causes the > query to execute twice, which is unnecessary and inefficient, and cycle also > creates the dangerous possibility for a future programmer to accidentally > create an infinite loop (f.e. when trying to introduce the optimization I > describe above). > > Fortunately, breaking the cycle is easy: instead of having the constructor call > reconfigure(), which then calls find(), have the constructor call find(), which > then calls reconfigure(), i.e.: > > function QueryExecutor(aQueryConf, aCallback) { > ... > if (aQueryConf.phrase != "__init__") > this.find(aQueryConf, aCallback); > } > > QueryExecutor.prototype = { > reconfigure: function QueryExecutor_reconfigure(aQueryConf, aCallback) { > ... > // doesn't call find > }, > ... > find: function QueryExecutor_find(aQueryConf, aCallback) { > this.reconfigure(aQueryConf, aCallback); > ... > } > ... > } > Ok, sounds good. thanks for the in-depth look at this. > > >+ if (results.length > 1) { > >+ throw new Error(ERORRS.FOUND_RECORD_MISMATCH); > > ERORRS -> ERRORS > > > >+ let queryConf = { where: "bookmarks", > >+ phrase: bmUri.spec }; > > Since multiple bookmarks can share a URI spec, this can legitimately return > multiple results, so you need to handle that case. Perhaps it would be better > to retrieve the bookmark by the ID that insertBookmark returns. > > > (In reply to comment #21) > > (In reply to comment #20) > > > (From update of attachment 422285 [details] [diff] [review] [details] [details]) > > > >diff --git a/extension/content/js/jetpack-environment.js b/extension/content/js/jetpack-environment.js > > > > > > > > > > > "jetpack.Menu": function (context) { > > > > > > just out of curiosity, why is Menu here uppercase, looks like others are not > > > > > > > no idea - not my code. > > There's some inconsistency in our APIs, as some of them are designed to work > like JavaScript constructors, where you instantiate a thing by calling: > > new jetpack.Thing(); > > While others are designed to work like jQuery constructors, where you > instantiate a thing by calling: > > jetpack.thing(); > > We'll need to make these consistent in the reboot, but for now either way is > fine.

Comment on attachment 423445 [details] [diff] [review] v 1.0.5 Places Sync Patch for Jetpack Almost there! Just a couple remaining issues... (In reply to comment #26) > I'm not sure I want to combine them right now. perhaps if this version of the > patch lives longer than I hope it would, I will do the combination in the next > release. Ok. > I have made some changes to JEP 22 to be in line with this patch's actual > implementation. I have removed the ability to pass in a string. Tht is slated > for the next go around when we have more control and are able to do unified > searches. Got it. > > >+ if (this.firstRun == undefined) { > > >+ this.firstRun = true; > > >+ } > > >+ else { > > >+ this.firstRun = false; > > >+ } > > > > This property isn't used anywhere and should be removed. > > > > > I added it to the prototype. And it is used. I guess I don't understand how it's being used, as it isn't referenced anywhere else in the patch and isn't mentioned in the JEP. But if it really does need to exist, then the code to set it needs to be in either reconfigure or find; currently, it's in the constructor, which only gets called once, when an instance of QueryExecutor is first instantiated, at which point this.firstRun will always be undefined, so it will always get set to true, and it'll stay that way for the life of the instance. I'm pretty sure that's not what was intended. :-) >+ , FOUND_RECORD_MISMATCH: "More than 1 matching records found after creating bookmark" Nit: as FOUND_RECORD_MISMATCH isn't used anymore, it should be removed. >+ find: function QueryExecutor_find(aQueryConf, aCallback) { >+ let callback = null; >+ // Check if a new phrase is being used with an existing PlacesQuery obj. >+ if (("callback" in this.queryConf) && aCallback == undefined) { >+ callback = this.queryConf.callback; >+ } >+ else if (typeof aCallback == 'function') { >+ callback = aCallback; >+ } >+ else { >+ // noop >+ } >+ if (aQueryConf) { >+ // aCallback is passed in here even if it is 'undefined'. >+ this.reconfigure(aQueryConf, callback); >+ } Nit: now that reconfigure no longer calls find, this chunk of code should be simplified to: find: function QueryExecutor_find(aQueryConf, aCallback) { this.reconfigure(aQueryConf, aCallback); let callback = this.queryConf.callback; >+ get modificationTime() { >+ try { >+ return new Date((this._rawNode.lastModified / 1000)); >+ } >+ catch(ex) { >+ // must be a history item, just return the creation time >+ return new Date((this.creationTime / 1000)); >+ } >+ }, As this.creationTime is already a Date object, the catch block needs to simply return it, i.e.: try { return new Date((this._rawNode.lastModified / 1000)); } catch(ex) { // must be a history item, just return the creation time return this.creationTime; }

(In reply to comment #28) > (From update of attachment 423445 [details] [diff] [review]) > > > >+ if (this.firstRun == undefined) { > > > >+ this.firstRun = true; > > > >+ } > > > >+ else { > > > >+ this.firstRun = false; > > > >+ } > > > > > > This property isn't used anywhere and should be removed. > > > > > > > > I added it to the prototype. And it is used. > > I guess I don't understand how it's being used, as it isn't referenced anywhere > else in the patch and isn't mentioned in the JEP. Myk: you are 100 % correct. I need to stop smoking crack. REMOVED. > >+ , FOUND_RECORD_MISMATCH: "More than 1 matching records found after creating bookmark" > > Nit: as FOUND_RECORD_MISMATCH isn't used anymore, it should be removed. > ok > > >+ find: function QueryExecutor_find(aQueryConf, aCallback) { > >+ let callback = null; > >+ // Check if a new phrase is being used with an existing PlacesQuery obj. > >+ if (("callback" in this.queryConf) && aCallback == undefined) { > >+ callback = this.queryConf.callback; > >+ } > >+ else if (typeof aCallback == 'function') { > >+ callback = aCallback; > >+ } > >+ else { > >+ // noop > >+ } > >+ if (aQueryConf) { > >+ // aCallback is passed in here even if it is 'undefined'. > >+ this.reconfigure(aQueryConf, callback); > >+ } > > Nit: now that reconfigure no longer calls find, this chunk of code should be > simplified to: > > find: function QueryExecutor_find(aQueryConf, aCallback) { > this.reconfigure(aQueryConf, aCallback); > let callback = this.queryConf.callback; > Swanky! > > >+ get modificationTime() { > >+ try { > >+ return new Date((this._rawNode.lastModified / 1000)); > >+ } > >+ catch(ex) { > >+ // must be a history item, just return the creation time > >+ return new Date((this.creationTime / 1000)); > >+ } > >+ }, > > As this.creationTime is already a Date object, the catch block needs to simply good catch. Sorry for these noob-style errors you are reviewing with great patience. Thank you.

Comment on attachment 423567 [details] [diff] [review] v 1.0.6 Places Sync Patch for Jetpack >+ , FOUND_RECORD_MISMATCH: "More than 1 matching records found after creating bookmark" Nit: this is still in the patch, but it shouldn't be, since it doesn't get used anymore. >+ this.reconfigure(aQueryConf, aCallback); >+ let callback = this.queryConf.callback; >+ >+ // Check if a new phrase is being used with an existing PlacesQuery obj. >+ if (("callback" in this.queryConf) && aCallback == undefined) { >+ callback = this.queryConf.callback; >+ } >+ else if (typeof aCallback == 'function') { >+ callback = aCallback; >+ } >+ else { >+ // noop >+ } >+ if (aQueryConf) { >+ // aCallback is passed in here even if it is 'undefined'. >+ this.reconfigure(aQueryConf, callback); >+ } You added the new code here (the first two lines in the section above) but didn't remove the old code (the rest of it). Now that the new code is in there, you don't need the old code anymore, and keeping it in there may have unwanted consequences (they both initialize the same properties), so it should be removed, i.e. this section should be simply: this.reconfigure(aQueryConf, aCallback); let callback = this.queryConf.callback; >+ get creationTime() { >+ try { >+ return new Date((this._rawNode.lastModified / 1000)); >+ } >+ catch(ex) { >+ // must be a history item, just return the creation time >+ return this.creationTime; >+ } >+ }, >+ >+ get modificationTime() { >+ try { >+ return new Date((this._rawNode.lastModified / 1000)); >+ } >+ catch(ex) { >+ // must be a history item, just return the creation time >+ return new Date((this.creationTime / 1000)); >+ } >+ }, It looks like you changed the creationTime getter instead of the modificationTime getter. The modificationTime getter is what needs to be changed (the creationTime getter was fine the way it was).

i removed the error string, reverted and fixed the time getters, and stripped out the superflous configuration code.

Comment on attachment 423703 [details] [diff] [review] v 1.0.7 Places Sync Patch for Jetpack >+ get creationTime() { >+ return new Date((this._rawNode.lastModified / 1000)); >+ }, In the earlier patches, this was actually: return new Date((this._rawNode.dateAdded / 1000)); r+ with that fix.

updated for r+. yay!

I do have commit access now, but am unsure how to push this. Do I just do "hg push" or do I need to do it via ssh, so it picks up my public key?

(In reply to comment #35) > I do have commit access now, but am unsure how to push this. Do I just do "hg > push" or do I need to do it via ssh, so it picks up my public key? Assuming you've `hg qfinish`ed it, then yes, just `hg push`. You might want to double check `hg outgoing` to make sure you just have one commit and you're not pushing your queue. Make sure you have default-push set to ssh://hg.mozilla.org/labs/jetpack/ (in .hg/hgrc) and it will use ssh & pick up your public keys (that's actually the required way to push AFAIK). I was just going to push for you, but I didn't want to steal this moment, a person's first push is special :)

Comment on attachment 423716 [details] [diff] [review] v 1.0.8 Places Sync Patch for Jetpack >diff --git a/extension/modules/places.js b/extension/modules/places.js >+ reconfigure: function QE_reconfigure(aQueryConf, aCallback) { wrong double spacing after the label. >+ // main public API lives here reconfigure is public api i think? and is defined before this comment. >+ let id = bs.insertBookmark(bs.unfiledBookmarksFolder, >+ bmUri, >+ bs.DEFAULT_INDEX, >+ aObj.title); >+ let tags = []; >+ tags = tags.concat(aObj.tags); >+ ts.tagURI(bmUri, tags); shouldn't you check tags length before trying to assign them? internally this is a no-op, but it's a useless call still that could also init the TaggingService without a reason. If you need any kind of assistance with pushing this thing, feel free to nag me, i'll be happy to drive you.

mak: good call. How is something like this: if ("tags" in aObj) { if (isArray(aObj.tags) && aObj.tags.length > 0){ let tags = []; tags = tags.concat(aObj.tags); ts.tagURI(bmUri, tags); } }

also: i suppose if tags is not an array it should really throw

let makeTags; if ("tags" in aObj) { if (isArray(aObj.tags)) { if (aObj.tags.length > 0) { makeTags = true; } } } let id = bs.insertBookmark(bs.unfiledBookmarksFolder, bmUri, bs.DEFAULT_INDEX, aObj.title); if (makeTags) { let tags = []; tags = tags.concat(aObj.tags); ts.tagURI(bmUri, tags); }

mak: how is the above approach? ^^

> let tags = []; > tags = tags.concat(aObj.tags); > ts.tagURI(bmUri, tags); If it's important to pass tagURI a copy of the array, you can do so more easily with Array::slice: ts.tagURI(bmUri, aObj.tags.slice());

(In reply to comment #42) > > let tags = []; > > tags = tags.concat(aObj.tags); > > ts.tagURI(bmUri, tags); > > If it's important to pass tagURI a copy of the array, you can do so more easily > with Array::slice: > > ts.tagURI(bmUri, aObj.tags.slice()); the scope was make the api accept both a string OR an array. Not a copy. comment 40 looks like overengineering this. just check if "tags" in aObj or make it an empty array by default, as i said concat does not mind if it's an array or a single element, and i think you can simplify the interface to accept both (so i can pass tags: "testtag" or tags: ["testtag1", "testag2"]). then after the concat you can just check length if ("tags" in aObj) { let tags = []; tags = tags.concat(aObj.tags); if (tags.length) ts.tagURI(bmUri, tags); }

btw concat should be faster then slice afaik.

(In reply to comment #42) > If it's important to pass tagURI a copy of the array, you can do so more easily > with Array::slice: > > ts.tagURI(bmUri, aObj.tags.slice()); hmmm looks like i need to check if tags is a string or an array. i was trying to support either approach. maybe that is not the best way after all. tags.concat(aObj.tags) was for handling either type. hmmm...

how about this: let makeTags; if ("tags" in aObj) { if (isArray(aObj.tags) || typeof aObj.tags == "string") { makeTags = true; } } let id = bs.insertBookmark(bs.unfiledBookmarksFolder, bmUri, bs.DEFAULT_INDEX, aObj.title); if (makeTags) { let tags = []; tags = tags.concat(aObj.tags); ts.tagURI(bmUri, tags); }

Ok. after talking to mak on irc i have this: let makeTags; if ("tags" in aObj) { if (["array", "string"].indexOf(strictTypeOf(aObj) != -1)) { makeTags = true; } else { throw new Error(ERRORS.TAGS_INPUT_ERROR); } } let id = bs.insertBookmark(bs.unfiledBookmarksFolder, bmUri, bs.DEFAULT_INDEX, aObj.title); if (makeTags) { let tags = []; tags = tags.concat(aObj.tags); if (tags.length){ ts.tagURI(bmUri, tags); } } and I added a test that checks for a new TAGS_INPUT_ERROR

added checks for tags, new test. it does throw if you pass some bad input for tags before the bookmark is created

(In reply to comment #47) > Ok. after talking to mak on irc i have this: > > let makeTags; looks like bad indent? Btw here you should just let tags; > > if ("tags" in aObj) { > if (["array", "string"].indexOf(strictTypeOf(aObj) != -1)) { > makeTags = true; here you should put the concat > if (makeTags) { > let tags = []; > tags = tags.concat(aObj.tags); > if (tags.length){ > ts.tagURI(bmUri, tags); > } > } and all of this should just be > if (tags.length){ > ts.tagURI(bmUri, tags);

after latest changes with checking tags, etc. the main test will once in a while fail. very strange - so far maybe 1 out of 10 times there is a failure.

Comment on attachment 423829 [details] [diff] [review] v 1.1 Places Sync Patch for Jetpack Note: zpao's toolbar checkin conflicts with this, so you need to resolve those conflicts. >+ testIncorrectTagsInput: function(self) { >+ let domain_prefix = Date.now(); >+ let test_url = "http://biff." + domain_prefix + ".mozilla.com/"; >+ >+ let bObj = { url: test_url, >+ title: "my mozilla " + domain_prefix, >+ tags: ["biff", "mozilla"] >+ }; You need to assign the tags property an invalid value in order to test that the API throws an exception when you call it with invalid tags. |["biff", "mozilla"]| is a valid value and doesn't trigger the exception. Also, to prevent false negatives here, the test needs to fail if the exception isn't thrown, for example by being written like so: let caught = false, message; try { // no callback needed to test for an error this.places.bookmark.create(bObj); } catch(ex) { caught = true; message = ex.message; } self.assert(caught); self.assert(message == "Tags input is invalid. String or Array required"); (Only testing the value of message would be sufficient, but separately testing whether or not an exception was caught seems valuable here to explicitly distinguish between the two different kinds of test failures that can occur: an exception isn't thrown, and the wrong exception is thrown.) Also, assertTrue -> assert. Finally, once the above changes are made, this test starts failing because the message being caught is "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.runInBatchMode]". >+Cu.import("resource://gre/modules/NetUtil.jsm"); I just noticed that the NetUtil module isn't available on Firefox 3.5. I'm fine with requiring Firefox 3.6 now that it has been released, but we should check with Atul to make sure the extension fully supports Firefox 3.6 and is no longer marked as compatible with 3.5. >+ , TAGS_NOT_AN_ARRAY: "'Bookmark.tags' should be an array of strings" Nit: it looks like this isn't used anymore and should be removed. >+ let tags = []; >+ >+ if ("tags" in aObj) { >+ if (["array", "string"].indexOf(strictTypeOf(aObj) != -1)) { strictTypeOf(aObj) -> strictTypeOf(aObj.tags) >+ } >+ else { >+ throw new Error(ERRORS.TAGS_INPUT_ERROR); >+ } >+ } >+ >+ let id = bs.insertBookmark(bs.unfiledBookmarksFolder, >+ bmUri, >+ bs.DEFAULT_INDEX, >+ aObj.title); >+ >+ tags = tags.concat(aObj.tags); >+ if (tags.length){ >+ ts.tagURI(bmUri, tags); >+ } This code block should be written the way Marco specified in his review: let tags = []; if ("tags" in aObj) { if (["array", "string"].indexOf(strictTypeOf(aObj.tags) != -1)) tags = tags.concat(aObj.tags); else throw new Error(ERRORS.TAGS_INPUT_ERROR); } let id = bs.insertBookmark(bs.unfiledBookmarksFolder, bmUri, bs.DEFAULT_INDEX, aObj.title); if (tags.length) ts.tagURI(bmUri, tags); (brackets around single-line single-statement blocks at your discretion)

(In reply to comment #51) > Also, to prevent false negatives here, the test needs to fail if the exception > isn't thrown, for example by being written like so: since this is an xpcshell test (iirc) should also be possible to do: try { funcThatWillThrow(); do_throw("That should have thrown!"); } catch (ex) { /* this is expected */ } do_throw does not really throw, so the catch won't catch it

Good point! Jetpack actually has its own test harness, which doesn't have do_throw, but it has the equivalent in assertRaises, which you should be able to use as: let places = this.places; self.assertRaises(function() places.bookmark.create(bObj), Error, "Tags input is invalid. String or Array required");

this does not work: ["array", "string"].indexOf(strictTypeOf(aObj.tags) != -1 It allowed numbers through to the tagging procedure weird. reverted to isArray(aObj.tags) || typeof aObj.tags == "string" ////// I added a try that created a "NetUtil" object using an ioservice lazy getter in case this is running in Fx 3.5 new patch being attached now. (In reply to comment #52) > (In reply to comment #51) > > Also, to prevent false negatives here, the test needs to fail if the exception > > isn't thrown, for example by being written like so: > > since this is an xpcshell test (iirc) should also be possible to do: > > try { > funcThatWillThrow(); > do_throw("That should have thrown!"); > } catch (ex) { /* this is expected */ } > > do_throw does not really throw, so the catch won't catch it It is not an xpcshell test. jetpack tests. Will test this patch in xpcshell on bug 522572

sorry, this is taking forever:)

(In reply to comment #54) > this does not work: > > ["array", "string"].indexOf(strictTypeOf(aObj.tags) != -1 reminder: bad parenthesis, strictTypeOf(aObj.tags) != -1 is always true

(In reply to comment #53) > let places = this.places; > self.assertRaises(function() places.bookmark.create(bObj), > Error, > "Tags input is invalid. String or Array required"); I tried this, but it does not match the error message correctly. I double-checked the message string, but no dice.

(In reply to comment #56) > (In reply to comment #54) > > this does not work: > > > > ["array", "string"].indexOf(strictTypeOf(aObj.tags) != -1 > > reminder: bad parenthesis, strictTypeOf(aObj.tags) != -1 is always true still doesn't work like: if (["array", "string"].indexOf(strictTypeOf(aObj.tags)) != -1) { the 'isArray() || typeof "string"' is less complicated to look at.

Comment on attachment 423849 [details] [diff] [review] v 1.1.1 Places Sync Patch for Jetpack Note: in comment 53, I suggested you use assertRaises, but actually that's not possible, since this API is implemented in a JS module, and assertRaises doesn't work with exceptions raised in a different JS context (because the Error objects are different). So what you're doing in the test is fine. >+ self.assert(ex.message == >+ "Tags input is invalid. String or Array required"); Nit: this line is now duplicative and should be removed. >+ self.assert(message == "Tags input is invalid. String or Array required"); I probably should have suggested you use assertEquals here, but this works just as well and is fine. > this does not work: > > ["array", "string"].indexOf(strictTypeOf(aObj.tags) != -1 > > It allowed numbers through to the tagging procedure > > weird. reverted to isArray(aObj.tags) || typeof aObj.tags == "string" Presumably it'd work with the parentheses fixed, but this version, with two separate conditionals, is actually more readable so is preferable. > I added a try that created a "NetUtil" object using an ioservice lazy getter in > case this is running in Fx 3.5 Good idea! r=myk, but remove that duplicative statement before checking in.

denied!!! maybe all of hg is closed? or I am not really allowed to push:( ddahl-t500 ~/code/moz/jetpack % hg outgoing comparing with https://hg.mozilla.org/labs/jetpack searching for changes changeset: 872:1c9b7b345dcb user: David Dahl <ddahl@mozilla.com> date: Wed Jan 27 13:41:12 2010 -0800 summary: [mq]: placesJetpackSyncPatch.diff changeset: 873:c41c76708859 tag: tip user: David Dahl <ddahl@mozilla.com> date: Wed Jan 27 14:31:50 2010 -0800 summary: bug 531940 Implement jetpack.places r=myk ddahl-t500 ~/code/moz/jetpack % hg push The authenticity of host 'hg.mozilla.org (63.245.208.188)' can't be established. RSA key fingerprint is fa:e8:5a:cc:de:02:e0:c7:6e:ff:14:92:90:b9:12:be. Are you sure you want to continue connecting (yes/no)? yes remote: Warning: Permanently added 'hg.mozilla.org,63.245.208.188' (RSA) to the list of known hosts. remote: Permission denied (publickey,gssapi-with-mic). abort: no suitable response from remote hg!

pushers welcome to push.

Pushed http://hg.mozilla.org/labs/jetpack/rev/06457706d91d