In bug 1423896, we're changing the menu/toolbar/other/mobile folders into virtual queries. As a result, we should no longer need to rely on the on-disk version of the root folder names - this means we can remove the code that is checking and updating the folder names in the background (to account for different locale builds, or breakages etc).

I think I've now covered all the necessary call points, but please keep an eye out for any more.

Comment on attachment 8949374 [details] Bug 1432614 - Remove now unnecessary updating and maintenance of Places' root folder titles. https://reviewboard.mozilla.org/r/218724/#review224798 Thanks! The Sync parts look good; you can undo the mirror test changes if you rebase atop bug 1436837. I looked yesterday, and I'm reasonably sure iOS (https://github.com/mozilla-mobile/firefox-ios/blob/0f746ddb22948587f939b999d039a8dc068fa1ac/Storage/Bookmarks/Bookmarks.swift, search for `titleForSpecialGUID`) and Android (https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java#213,220,815,831,842,846-848,989-993) already do the right thing for root folder names. The old Desktop bookmarks engine does not, it uses `parentTitle` directly, but only for deduping, and it's already broken if you sync between two devices with different locales. ::: toolkit/components/places/tests/sync/head_sync.js:13 (Diff revision 2) > ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); > > ChromeUtils.import("resource://testing-common/PlacesTestUtils.jsm"); > ChromeUtils.import("resource://testing-common/httpd.js"); > > +// These titles are defined in Database::CreateBookmarkRoots Bug 1436837 reworks the mirror to never apply titles (or other value changes) for roots, so I think you can drop all the Sync test changes. Apologies for the collision. :-/

Comment on attachment 8949374 [details] Bug 1432614 - Remove now unnecessary updating and maintenance of Places' root folder titles. https://reviewboard.mozilla.org/r/218724/#review224826 There may be one problem with the other left pane queries fixup. Did you figure out if bookmarks.html exposes the roots titles and thus should use the UITitle? ::: browser/components/places/PlacesUIUtils.jsm (Diff revision 2) > - if (bs.getItemTitle(query.itemId) != query.title) > - bs.setItemTitle(query.itemId, query.title); > - if ("concreteId" in query) { > - if (bs.getItemTitle(query.concreteId) != query.concreteTitle) > - bs.setItemTitle(query.concreteId, query.concreteTitle); > - } Shouldn't this still happen for other queries that are not the root shortcuts? Like from "History" to "Cronologia" At least until we make the whole left pane virtual. ::: toolkit/components/places/Bookmarks.jsm:176 (Diff revision 2) > guid == PlacesUtils.bookmarks.virtualToolbarGuid || > guid == PlacesUtils.bookmarks.virtualUnfiledGuid; > }, > > /** > + * Returns the title to use for the UI for a bookmark item. Root folders nit: s/for the UI/on the UI/ (or in?) ::: toolkit/components/places/Bookmarks.jsm:180 (Diff revision 2) > /** > + * Returns the title to use for the UI for a bookmark item. Root folders > + * in the database don't store fully localised versions of the title. To > + * get those this function should be called. > + * > + * Hence, this function should only be called if an root folder object is nit: s/an/a/

Comment on attachment 8949374 [details] Bug 1432614 - Remove now unnecessary updating and maintenance of Places' root folder titles. https://reviewboard.mozilla.org/r/218724/#review224840 setting r- just to clarify the patch status since it's multiple reviewers

Comment on attachment 8949374 [details] Bug 1432614 - Remove now unnecessary updating and maintenance of Places' root folder titles. https://reviewboard.mozilla.org/r/218724/#review224798 > Bug 1436837 reworks the mirror to never apply titles (or other value changes) for roots, so I think you can drop all the Sync test changes. Apologies for the collision. :-/ Well not quite. We still need to have the newer titles for when we compare with the local trees... so I'll keep those parts.

Comment on attachment 8949374 [details] Bug 1432614 - Remove now unnecessary updating and maintenance of Places' root folder titles. https://reviewboard.mozilla.org/r/218724/#review224826 I've just tested with the latest Chrome on my Mac, and it appears that Chrome does use the titles provided in the HTML for the toolbar & other bookmarks folders on its imported set. Therefore, we should do some kind of fixup here. > Shouldn't this still happen for other queries that are not the root shortcuts? Like from "History" to "Cronologia" > At least until we make the whole left pane virtual. I think it doesn't actually get displayed, but just in case, lets keep it anyway, it'll soon go away.

Comment on attachment 8949374 [details] Bug 1432614 - Remove now unnecessary updating and maintenance of Places' root folder titles. https://reviewboard.mozilla.org/r/218724/#review226842 ::: toolkit/components/places/Bookmarks.jsm:188 (Diff revision 3) > + * @param {Object} info An object representing a bookmark-item. > + * @returns {String} The correct string. > + * @throws {Error} If the guid in PlacesUtils.bookmarks.userContentRoots is > + * not supported. > + */ > + getUITitle(info) { Sorry for late notice on the change, I'd probably rename this to "getLocalizedTitle"

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8976a2e3a3b5 Remove now unnecessary updating and maintenance of Places' root folder titles. r=kitcambridge,mak