Closed Bug 494382 Opened 12 years ago Closed 11 years ago

Reduce notifications work for tagging service

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [TSnappiness])

Attachments

(1 file, 1 obsolete file)

Attached patch patch v1.0 (obsolete) — Splinter Review
if we have created a tag there's no reason to get itemType, and we should only act on title change notifications.
Attachment #379136 - Flags: review?(dietrich)
Comment on attachment 379136 [details] [diff] [review]
patch v1.0


>@@ -396,6 +401,7 @@ TaggingService.prototype = {
>   },
>   onItemAdded: function(aItemId, aFolderId, aIndex) {
>     if (aFolderId == this._bms.tagsFolder &&
>+        !this._tagFolders[aItemId] &&
>         this._bms.getItemType(aItemId) == this._bms.TYPE_FOLDER)
>       this._tagFolders[aItemId] = this._bms.getItemTitle(aItemId);
>   },

given your change to _createTag, is this even necessary? is it to catch tag creation from outside the tagging service? do we even want to support that?
ugh, no, when i create a tag bookmarks service will notify me onItemAdded and i would execute getItemType, but since i just created it i do know it added a tag so there's no need to do anything.
my comment 2 means imo patch is correct like it is (sorry i was not much clear) those changes are one dependant on the other one
Attachment #379136 - Attachment is obsolete: true
Attachment #379136 - Flags: review?(dietrich)
Comment on attachment 379136 [details] [diff] [review]
patch v1.0

hm, oh wait, the notification will arrive before i add the folder to the hash, so those lines should be inverted.
Or better, i can't simply do that since i don't know the itemId at all till the observer has been called.
Your point is correct, but i'm scaried by extensions, those could add tags as normal folders totally skipping the service due to our implementation flaws.
So, i'll try to enqueue the observer work on the main thread, so that when it will be called we will know if a new tag has been added... and will mark as a temporary workaround till bug 494380 is fixed.
Attached patch patch v1.1Splinter Review
i've verified this works as expected, so if we add a tag through createTag getItemType is skipped.
I think we should not remove this code path till tags are changed from being bookmarks.
Attachment #379562 - Flags: review?(dietrich)
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
Whiteboard: [needs review dietrich]
Comment on attachment 379562 [details] [diff] [review]
patch v1.1

>   onItemAdded: function(aItemId, aFolderId, aIndex) {
>-    if (aFolderId == this._bms.tagsFolder &&
>-        this._bms.getItemType(aItemId) == this._bms.TYPE_FOLDER)
>-      this._tagFolders[aItemId] = this._bms.getItemTitle(aItemId);
>+    // Nothing to do if this is not a tag.
>+    if (aFolderId != this._bms.tagsFolder)
>+      return;
>+
>+    // If we are correctly called through createTag the itemId will be added
>+    // to _tagFolders just after onItemAdded is called.  To avoid an useless
>+    // call to getItemType we enqueue this check, so that when it runs the hash
>+    // has already been updated.
>+    // TODO: once bug 494380 is fixed, this 'workaround' can go away.

seems overly complicated. why not just set a private property for _tagAddInProgress, and check that?
(In reply to comment #7)
> (From update of attachment 379562 [details] [diff] [review])
> >   onItemAdded: function(aItemId, aFolderId, aIndex) {
> >-    if (aFolderId == this._bms.tagsFolder &&
> >-        this._bms.getItemType(aItemId) == this._bms.TYPE_FOLDER)
> >-      this._tagFolders[aItemId] = this._bms.getItemTitle(aItemId);
> >+    // Nothing to do if this is not a tag.
> >+    if (aFolderId != this._bms.tagsFolder)
> >+      return;
> >+
> >+    // If we are correctly called through createTag the itemId will be added
> >+    // to _tagFolders just after onItemAdded is called.  To avoid an useless
> >+    // call to getItemType we enqueue this check, so that when it runs the hash
> >+    // has already been updated.
> >+    // TODO: once bug 494380 is fixed, this 'workaround' can go away.
> 
> seems overly complicated. why not just set a private property for
> _tagAddInProgress, and check that?

since this is a workaround i wanted to contain it in only one method, rather than spreading it around, plus i thought if we want to move toward async having private properties could be bad, it would be a bool since i don't know the id i'm going to add, so could be i would end up ignoring a notification for another item add that happens between my createTag and the notification. This way i simply delay the work so i can be sure i'll always ignore the correct id.
Whiteboard: [needs review dietrich] → [needs review dietrich][TSnappiness]
Attachment #379562 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich][TSnappiness] → [TSnappiness]
http://hg.mozilla.org/mozilla-central/rev/8facc489ed31
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.