Closed Bug 1432614 Opened 6 years ago Closed 6 years ago

Don't rely on the saved folder names for the root bookmark folders (menu/toolbar/other/mobile), but use the L10n names direct

Categories

(Toolkit :: Places, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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).
Depends on: 1423896
Status: NEW → ASSIGNED
Blocks: 627582
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. :-/
Attachment #8949374 - Flags: review?(kit) → review+
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/
Attachment #8949374 - Flags: review?(mak77)
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
Attachment #8949374 - Flags: review-
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"
Attachment #8949374 - Flags: review?(mak77) → review+
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
https://hg.mozilla.org/mozilla-central/rev/8976a2e3a3b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
See Also: → 1475108
You need to log in before you can comment on or make changes to this bug.