We should not allow user to rename a tag to an empty one, untagURI accept the name as input but it could not distinguish between identical empty tags. And it does not make sense.

Comment on attachment 341267 [details] [diff] [review] patch As with anything else (readonly, for example), the code for this dialog should figure if the field is editable within itself. The service should throw, I think (and please document that in nsITaggingService)

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv

asking blocking because block a blocker. We are workarounding this bug at query level, unfortunately that causes SQLite to take wrong decisions regarding optimizations of the queries. Rather than workaround the issue, we should fix it once and for all.

while looking at this I've found a bug /toolkit/components/places/src/nsTaggingService.js line 431 -- if (this._tagFolders[aItemId] && this._bms.tagFolder == aOldParent && line 432 -- this._bms.tagFolder != aNewParent) this._bms.tagFolder does not exist, we have this._bms.tagsFolder... btw going to attach a use-modules part cleanup patch that will also fix this.

this is a bit of cleanup, just making the service use cached services from modules.

And part 2 that is the real patch. This will require a SR, I'm not changing the interface, and it was already throwing for invalid arguments, but now the invalid arguments check will be a bit stricter (checking type and length for strings).

Comment on attachment 482746 [details] [diff] [review] Part 1: use modules in taggingService r=mano for the first part.

Comment on attachment 482747 [details] [diff] [review] Part 2: do not allow creation of empty-named tags. There's a new trim method, please use it. r=mano

Comment on attachment 482747 [details] [diff] [review] Part 2: do not allow creation of empty-named tags. Robert, will you have time to check the idl changes here? The interface is not changing and we were already throwing NS_ERROR_INVALID_ARG for bad arguments, the difference is that now we throw also if the array contains bad entries (empty strings or not-string and not-number), while before we were throwing only for null arguments. The comment was already saying to pass strings or numbers though, so the change is purely making the old comment more true.

(In reply to comment #9) > There's a new trim method, please use it. omg, we really have it! I need to update my brain to JS 1.8.1 and 1.8.5

PS: I'm evaluating to add an Array.isArray check to the input, I just found a test that was using a string and nobody ever noticed that. with this change the test timed out (probably because we are trying to use a string as an array) and I discovered it. It should probably throw as well.

And test_adaptive.js needs a fix for the bogus call.

This fixes the bogus test and adds a Array.isArray() test on input (untagURI still allows to get null as a special case to remove all tags). moving SR request to this one.

Comment on attachment 483145 [details] [diff] [review] Part 2 (v1.1): do not allow creation of empty-named tags. >diff --git a/toolkit/components/places/src/nsTaggingService.js b/toolkit/components/places/src/nsTaggingService.js >--- a/toolkit/components/places/src/nsTaggingService.js >+++ b/toolkit/components/places/src/nsTaggingService.js >@@ -139,47 +139,75 @@ TaggingService.prototype = { > return -1; > }, > >+ /** >+ * Makes a proper array of tag objects like { id: number, name: string }. >+ * >+ * @throws Cr.NS_ERROR_INVALID_ARG if any element of the input array is not >+ * a valid tag. >+ */ please add @return for this >+ _convertInputMixedTagsArray: function TS__convertInputMixedTagsArray(aTags) >+ { >+ return aTags.map(function (val) looks great! sr=rs

fixed Robert's comment and pushed part 1: http://hg.mozilla.org/mozilla-central/rev/423d0afd6ef0 part 2: http://hg.mozilla.org/mozilla-central/rev/4e05764294ec