Closed Bug 1062894 Opened 10 years ago Closed 9 years ago

Avoid the tagging batches if number of changed tags is low

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: mak, Assigned: mohamedwaleed2012, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 2 obsolete files)

the batch in the tagging service is doing more damage than benefit, we should batch only if the number of tags to add or remove is higher than a threshold, like 3.
What I would like to happen here:

1. add function decorator to nsINavHistoryBatchCallback so we can stop requiring an object and we can use simple js functions
2. assign the batching functions in nsTaggingService to local vars.
3. based on number of tags to add/remove, either directly invoke the function or pass it to runInBatchMode.
Mentor: mak77
Whiteboard: [good first bug][lang=js]
Assignee: nobody → akshat.kedia
I would like to work on this bug. I have setup the Gaia codebase.
Flags: needinfo?(mak77)
Hi, what do you mean by the Gaia codebase? This is a Firefox Desktop bug, see https://developer.mozilla.org/en-US/docs/Introduction
Flags: needinfo?(mak77)
OMG ! I am sorry. I did not read the fields properly I read b2g somewhere and thought it's a Gaia bug. Yeah I have mozilla-central all setup.
Flags: needinfo?(mak77)
ok sounds good. You can start with comment 1 suggestions, feel free to ask questions if you get lost.
Flags: needinfo?(mak77)
Hi, would like to work on this bug.
I'm unassigning the bug since we didn't hear back from Akshat so far.

I'll reassign once a first patch is attached. feel free to work on this.
Assignee: akshat.kedia → nobody
Hi Marco , I would like to work on this bug 
could you explain : 
1 - where is nsINavHistoryBatchCallback ?
2 - what is batching functions in the given .js file ?
3 - what is number of tags to add/remove ?
(In reply to Mohamed Waleed from comment #9)
> Hi Marco , I would like to work on this bug 
> could you explain : 
> 1 - where is nsINavHistoryBatchCallback ?
> 2 - what is batching functions in the given .js file ?
> 3 - what is number of tags to add/remove ?

1.
Hello, you can use the following services to search our codebase:
http://mxr.mozilla.org/mozilla-central/search?string=nsINavHistoryBatchCallback
https://dxr.mozilla.org/mozilla-central/search?q=nsINavHistoryBatchCallback&redirect=true

names starting by nsI are usually interfaces so they live in .idl files, this in particular is at
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#1419

2.
by batching function here I mean the function that currently is run as a Places batch:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsTaggingService.js#144

143     PlacesUtils.bookmarks.runInBatchMode({
144       runBatched: function (aUserData)
145       {

These functions ^

3.
As stated in comment 0, I'd say that for less than 3 tags, we should not batch.
Marco , what I did is 

create a decorator function

 let taggingFuntion = function(){
    	return {
	      runBatched: function (aUserData)
	      {
	        tags.forEach(function (tag)
	        {
	          if (tag.id == -1) {
	            // Tag does not exist yet, create it.
	            this._createTag(tag.name);
	          }

	          if (this._getItemIdForTaggedURI(aURI, tag.name) == -1) {
	            // The provided URI is not yet tagged, add a tag for it.
	            // Note that bookmarks under tag containers must have null titles.
	            PlacesUtils.bookmarks.insertBookmark(
	              tag.id, aURI, PlacesUtils.bookmarks.DEFAULT_INDEX, null
	            );
	          }

	          // Try to preserve user's tag name casing.
	          // Rename the tag container so the Places view matches the most-recent
	          // user-typed value.
	          if (PlacesUtils.bookmarks.getItemTitle(tag.id) != tag.name) {
	            // this._tagFolders is updated by the bookmarks observer.
	            PlacesUtils.bookmarks.setItemTitle(tag.id, tag.name);
	          }
	        }, taggingService);
	      }
	    }
    }
then I made a check for tags less than 3 to avoid run those tags in patch mode

    if(aTags.length < 3){
    	taggingFuntion().runBatched(null);
    }else
    	PlacesUtils.bookmarks.runInBatchMode(taggingFuntion(), null); 

but this "taggingFuntion().runBatched(null);" fails the unit test why ?
What I meant by using a function was to extract the runBatched function, like

let taggingFuntion = function() {
  tags.forEach...

And then later just

if (tags.length < 3) {
  taggingFunction();
else {
  PlacesUtils.bookmarks.runInBatchMode(taggingFuntion, null);
}

Your method doesn't work cause taggingFunction is a Function and doesn't have a runInBatchMode method... runInBatchMode is a method of the nsINavBookmarksService interface:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavBookmarksService.idl#582

Also your runInBatchMode(taggingFunction(), null); won't work for 2 reasons:
1. you are invoking the function and passinf the result, rather than passing the function
2. you must modify nsINavHistoryBatchCallback at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#1419
adding function after scriptable, to tell this interface can also take a simple function and doesn't need a full object.
let taggingFunction = function(){
    	tags.forEach(function (tag)
	        {
	          if (tag.id == -1) {
	            // Tag does not exist yet, create it.
	            this._createTag(tag.name);
	          }

	          if (this._getItemIdForTaggedURI(aURI, tag.name) == -1) {
	            // The provided URI is not yet tagged, add a tag for it.
	            // Note that bookmarks under tag containers must have null titles.
	            PlacesUtils.bookmarks.insertBookmark(
	              tag.id, aURI, PlacesUtils.bookmarks.DEFAULT_INDEX, null
	            );
	          }

	          // Try to preserve user's tag name casing.
	          // Rename the tag container so the Places view matches the most-recent
	          // user-typed value.
	          if (PlacesUtils.bookmarks.getItemTitle(tag.id) != tag.name) {
	            // this._tagFolders is updated by the bookmarks observer.
	            PlacesUtils.bookmarks.setItemTitle(tag.id, tag.name);
	          }
	        }, taggingService);
      }
  
    	if (tags.length >= 3) {
		  taggingFunction();
		}else {
		  PlacesUtils.bookmarks.runInBatchMode({runBatched:taggingFunction}, null);
		}
when I invok taggingFunction(); test fails , why ?
also when i tried to add aUserData parameter to  taggingFunction and pass it with null it also fails 

results of test : 

toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
------------------------------------------------------------------------
FAIL [Parent]
toolkit/components/places/tests/queries/test_history_queries_tags_liveUpdate.js
-------------------------------------------------------------------------------
FAIL [Parent]
this condition if (tags.length >= 3)  was   if (tags.length < 3)
sorry for typo error
please, attach diffs and ask with the feedback? flag when you need help, that helps keeping the bug comments cleaner and helps me looking at the changes.

The problem is likely the fact "this." in the function is not properly resolved.
You could solve this by using an arrow function:
let test = function() {
  this... // broken!
}
let test = () => {
  this... // works!
}
Here is the documentation

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

Also, 
PlacesUtils.bookmarks.runInBatchMode({runBatched:taggingFunction}, null);

As I said before, you must change nsINavHistoryBatchCallback adding the "function" decorator to its descriptors, for example see how nsIObserver is written
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserver.idl#13
That way you can directly pass taggingFunction without wrapping it into an object.

Please also setup your texteditor to use 2 spaces instead of tabs for indentation, and unix newlines.
Hi Mohamed, any news about this bug?
Flags: needinfo?(mohamedwaleed2012)
Attached patch Bug1062894_AvoidTagging.diff (obsolete) — Splinter Review
I am very sorry for this late but I had so much faculty work in previous days
I uploaded the patch for my current work until now 
what I did , I used the arrow function and tests passed but when I invoked the method in condition  if (tags.length >= 3)  the tests fail
Flags: needinfo?(mohamedwaleed2012)
Attachment #8597657 - Flags: review?(mak77)
Comment on attachment 8597657 [details] [diff] [review]
Bug1062894_AvoidTagging.diff

Review of attachment 8597657 [details] [diff] [review]:
-----------------------------------------------------------------

The basic approach to the patch is ok, but there are various minor things to fix.
Some of those failures were real too!
Since you said you're pretty busy, I made an updated patch for you, and will attach it here.

please take a look at the changes I did, it will be useful for your next patch.

::: toolkit/components/places/nsINavHistoryService.idl
@@ +1414,5 @@
>  
>  /**
>   * @see runInBatchMode of nsINavHistoryService/nsINavBookmarksService
>   */
> +[scriptable, function, uuid(5143f2bb-be0a-4faf-9acb-b0ed3f82952c)]

when touching an interface, it's uuid should always be bumped (a new uuid can be generated using ./mach uuid)

::: toolkit/components/places/nsTaggingService.js
@@ +140,5 @@
>      let tags = this._convertInputMixedTagsArray(aTags);
>  
>      let taggingService = this;
> +    let taggingFunction = () => {
> +    	tags.forEach( function (tag){

while here, it would have been nice to use for (let...of) instead of forEach

@@ +144,5 @@
> +    	tags.forEach( function (tag){
> +	          if (tag.id == -1) {
> +	            // Tag does not exist yet, create it.
> +	            this._createTag(tag.name);
> +	          }

Indentation should generally be proper, we use 2 spaces, most editors have a setting for that.
But I got this was a WIP so probably you didn't reach the cleanup point.

@@ +161,5 @@
> +	          if (PlacesUtils.bookmarks.getItemTitle(tag.id) != tag.name) {
> +	            // this._tagFolders is updated by the bookmarks observer.
> +	            PlacesUtils.bookmarks.setItemTitle(tag.id, tag.name);
> +	          }
> +	        }, taggingService);

using for...of, you would not even need to pass a "this" object.

@@ +166,2 @@
>        }
> +      

trailing spaces

::: xpcom/base/nsISupports.idl
@@ +21,5 @@
>   */
>  #if 0
>  %}
>  
> +[scriptable,uuid(00000000-0000-0000-c000-000000000046)]

There is no need to touch nsISupports (basically you'll never need to touch this file)
Attachment #8597657 - Flags: review?(mak77)
Attached patch patch v2Splinter Review
Tim could you please review the changes to the cpp files?
The query is just selecting more data, while the mRemovingURI thing was an optimization I did in the past (trying to not update tags when removing a bookmark) but it's optimizing too much.
These 2 failures were hidden due to Batches doing a complete refresh() of the query results.
Attachment #8597657 - Attachment is obsolete: true
Attachment #8598118 - Flags: review?(ttaubert)
Attachment #8598118 - Flags: review?(ttaubert) → review+
Assignee: nobody → mohamedwaleed2012
Status: NEW → ASSIGNED
I have no idea what that jetpack test is doing, it doesn't look sane off-hand.

Who is working on JP tests these days?
Flags: needinfo?(mohamedwaleed2012)
Attached patch Bug1062894.patch (obsolete) — Splinter Review
Attachment #8598773 - Flags: review?(mak77)
Hi mohamed, there's no more need to attach further version of the patch, the last one here is fine, we just need some hints about some failing tests.
Attachment #8598773 - Attachment is obsolete: true
Attachment #8598773 - Flags: review?(mak77)
I am so sorry , I thought that you need me to upload a final patch again
How can I help ??
I'm sorry, at the moment you can't help since we are blocked on something internal, but thank you very much for the work here and for the availability. I will pick-up remaining issues since they might not be easy to overcome. No worries, the bug will keep being yours :)

Now, you might look for a new bug to contribute to:
https://developer.mozilla.org/en-US/docs/Introduction#Find_a_bug_we%27ve_identified_as_being_good_for_newcomers
I wrote the original history/bookmark tests but no longer work on the SDK -- let me know if you have any questions about them and I'll try to help, but Erik Vold or Mossop might be more familiar with them now
Flags: needinfo?(jsantell)
Erik, can you help us?

Is there a way to run a single jetpack test using ./mach?
Flags: needinfo?(evold)
(In reply to Marco Bonardo [::mak] from comment #29)
> Erik, can you help us?
> 
> Is there a way to run a single jetpack test using ./mach?

mach jetpack-addon addon-sdk/source/test/addons/places.xpi will run just that test add-on. There isn't a way to filter within that test though.
Flags: needinfo?(evold)
Attached patch fix JP testSplinter Review
I don't think the test is extremely useful, since there is no JP api using batches, and we didn't decide about their future yet, but since it exposes these events, the simplest way to check for them is to force a batch directly.
It was currently using createBookmark, that was adding a tag, and it was expecting the tag addition to cause a batch, that's not a right assumption anymore.
Attachment #8601787 - Flags: review?(dtownsend)
Attachment #8601787 - Flags: review?(dtownsend) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/de64afb995d9ce5a1adfec85e2ae51b4b44d1c14
Bug 1062894 - Fix wrong assumption in test-places-events.js. r=mossop
You need to log in before you can comment on or make changes to this bug.