Closed Bug 1434607 Opened 3 years ago Closed 3 years ago

Remove synchronous Bookmarks::setKeywordForBookmark

Categories

(Toolkit :: Places, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(1 file, 1 obsolete file)

This is only used by tests
https://searchfox.org/mozilla-central/search?q=setKeywordForBookmark&path=

Along with it, we can remove the bookmarks observer from gKeywordsCachePromise, and we can stop initializing the cache with PlacesUtils.keywords (https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/toolkit/components/places/PlacesUtils.jsm#1925), that should be a perf win.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1313188
Unfortunately we can't remove the observer, because the UI still links keywords to bookmarks :(
Attachment #8947464 - Attachment is obsolete: true
This is not ready for 2 reasons:
1. Bug 1313188, the new API doesn't handle the case properly
2. test_bookmark_value_changes.js fails, that's strange and I'm not sure why.

Kit, any idea why with these changes (that are pretty much just removing an unused old API) test_bookmark_value_changes.js starts failing on the "fb" keyword?
Flags: needinfo?(kit)
I'm sorry, the keyword is "tb".
Comment on attachment 8947465 [details]
Bug 1434607 - Remove synchronous Bookmarks::setKeywordForBookmark.

https://reviewboard.mozilla.org/r/217164/#review223010

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
>                        parentGuid, oldVal, source) {
> -          if (gIgnoreKeywordNotifications) {
> +          if (prop == "uri") {
> -            return;
> -          }
> -
> -          if (prop == "keyword") {

The mirror still passes the `"keyword"` prop to update the keywords cache (https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/toolkit/components/places/SyncedBookmarksMirror.jsm#3997-4005), which is why I think it's failing now.

Would it make sense to add a `PlacesUtils.keywords.updateCachedKeyword` method that the mirror can call to update the cache?

Alternatively, could we just invalidate the entire keyword cache, as we do for livemarks? If we did that, we could remove keyword change tracking from the mirror entirely (https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/toolkit/components/places/SyncedBookmarksMirror.jsm#1329-1353), and avoid issues with the cache becoming out of sync with the database...but might be too expensive. WDYT?
Flags: needinfo?(kit)
Ah right, because the mirror doesn't go through the API, so it doesn't know about any caches.

Yes, I think we could invalidate the keywords cache completely through an API.
(In reply to Marco Bonardo [::mak] from comment #7)
> Yes, I think we could invalidate the keywords cache completely through an
> API.

Cool. I'm happy to put together a patch tomorrow, unless you'd like to. :-)
I have quite some reviews in my queue, not sure I could do that today regardless.
Depends on: 1435354
Blocks: 1313188
No longer depends on: 1313188
Blocks: 1435354
No longer depends on: 1435354
No longer blocks: 1313188
Depends on: 1313188
Blocks: 1433307
Attachment #8947465 - Flags: review?(standard8)
So, this really depends on Bug 1435354, because otherwise test_bookmark_value_changes.js fails.
No longer blocks: 1435354
Depends on: 1435354
Comment on attachment 8947465 [details]
Bug 1434607 - Remove synchronous Bookmarks::setKeywordForBookmark.

https://reviewboard.mozilla.org/r/217164/#review228584

Looks good, nice cleanup in PlacesUtils.jsm
Attachment #8947465 - Flags: review?(standard8) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/21d50f9ec4f9
Remove synchronous Bookmarks::setKeywordForBookmark. r=standard8
https://hg.mozilla.org/mozilla-central/rev/21d50f9ec4f9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.