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)
Toolkit
Places
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)
16.99 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
2.60 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
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]
Reporter | ||
Comment 2•10 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsTaggingService.js#143 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsTaggingService.js#219
Updated•10 years ago
|
Assignee: nobody → akshat.kedia
Comment 3•10 years ago
|
||
I would like to work on this bug. I have setup the Gaia codebase.
Flags: needinfo?(mak77)
Reporter | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 years ago
|
||
ok sounds good. You can start with comment 1 suggestions, feel free to ask questions if you get lost.
Flags: needinfo?(mak77)
Reporter | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
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 ?
Reporter | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
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 ?
Reporter | ||
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
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]
Assignee | ||
Comment 14•9 years ago
|
||
this condition if (tags.length >= 3) was if (tags.length < 3) sorry for typo error
Reporter | ||
Comment 15•9 years ago
|
||
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.
Reporter | ||
Comment 16•9 years ago
|
||
Hi Mohamed, any news about this bug?
Flags: needinfo?(mohamedwaleed2012)
Assignee | ||
Comment 17•9 years ago
|
||
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)
Reporter | ||
Comment 18•9 years ago
|
||
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)
Reporter | ||
Comment 19•9 years ago
|
||
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)
Reporter | ||
Comment 20•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f88baea8504
Updated•9 years ago
|
Attachment #8598118 -
Flags: review?(ttaubert) → review+
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → mohamedwaleed2012
Status: NEW → ASSIGNED
Backed out for JP orange in https://hg.mozilla.org/integration/fx-team/rev/2ba05c8a09fb https://treeherder.mozilla.org/logviewer.html#?job_id=2847763&repo=fx-team
Flags: needinfo?(mohamedwaleed2012)
Reporter | ||
Comment 23•9 years ago
|
||
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)
Flags: needinfo?(jsantell)
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8598773 -
Flags: review?(mak77)
Reporter | ||
Comment 25•9 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
Attachment #8598773 -
Attachment is obsolete: true
Attachment #8598773 -
Flags: review?(mak77)
Assignee | ||
Comment 26•9 years ago
|
||
I am so sorry , I thought that you need me to upload a final patch again How can I help ??
Reporter | ||
Comment 27•9 years ago
|
||
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
Comment 28•9 years ago
|
||
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)
Reporter | ||
Comment 29•9 years ago
|
||
Erik, can you help us? Is there a way to run a single jetpack test using ./mach?
Flags: needinfo?(evold)
Comment 30•9 years ago
|
||
(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)
Reporter | ||
Comment 31•9 years ago
|
||
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)
Reporter | ||
Comment 32•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cca954931f6
Updated•9 years ago
|
Attachment #8601787 -
Flags: review?(dtownsend) → review+
Comment 33•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9a5bd1aa9bce https://hg.mozilla.org/integration/fx-team/rev/0070705f8442
Comment 34•9 years ago
|
||
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
https://hg.mozilla.org/mozilla-central/rev/9a5bd1aa9bce https://hg.mozilla.org/mozilla-central/rev/0070705f8442
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•