1207108 - Duplicated 'bookmarks_title' string in android_strings.dtd

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Francesco Lodolo [:flod]]

We don't have any test or similar to find duplicate keys, this was randomly found by another tool that thought this was a string changed without a new ID.

http://hg.mozilla.org/mozilla-central/diff/26b367ff0033/mobile/android/base/locales/en-US/android_strings.dtd
This moved 'bookmarks_title' from bookmarks.inc to android_strings.dtd, value of the string "Mobile".

The problem is that we already have a string with this id, value "Bookmarks"
http://hg.mozilla.org/mozilla-central/file/3d97a673734c/mobile/android/base/locales/en-US/android_strings.dtd#l27

I tried to check the code but I couldn't figure out if we're actually still using the "Mobile" string. The "Bookmarks" one should be the one used for the home panel.

I think we should try to fix this as soon as possible, and probably break string freeze in aurora. No idea what localization tools will do, if they'll expose the first string or the second for localization.

Comment 1

[Francesco Lodolo [:flod]]

Looks like Pootle is ignoring the first occurrence of the string, which is pretty bad considering it's the most important of the two

The string has been "untranslated" and the product results complete in Pootle's dashboard
http://hg.mozilla.org/releases/l10n/mozilla-aurora/cy/diff/21bfcad6f83b/mobile/android/base/android_strings.dtd

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Flod, do you know which versions this is a problem in? Is it only 43 & 44?

Comment 3

[Allison Naaktgeboren :ally]

We don't obviously to actually consume the bookmarks_title value which from the bookmarks.inc file. 

The area of concern on IRC is whether this is used to create the mobile bookmark root. Looks like we use the guid on creation, or when manipulating mobile bookmarks

code looks like 
       final long folderID = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
        if (folderID == FOLDER_NOT_FOUND) {
            Log.e(LOGTAG, "No mobile folder: cannot add distribution bookmarks.");
            return offset;
        }
which is defined as
public static final String MOBILE_FOLDER_GUID = "mobile"; in BrowserContract.java

so I think we're in the clear to just remove the from the old .inc from the dtd file.

Comment 4

[Allison Naaktgeboren :ally]

Can't be any other versions, it came in from http://hg.mozilla.org/mozilla-central/rev/26b367ff0033, Bug 1197054 - Build changes to get rid of bookmarks.inc

Comment 5

[Allison Naaktgeboren :ally]

Attachment: idCollisionForBookmarkStrings

Comment 6

[Francesco Lodolo [:flod]]

Sadly it's in 42, which means we need to break string freeze in Aurora to fix it and let localization tools pick up the right string.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8664385 [details] [diff] [review]
idCollisionForBookmarkStrings

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

I trust you got the right one.

Comment 8

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #6)
> Sadly it's in 42, which means we need to break string freeze in Aurora to
> fix it and let localization tools pick up the right string.

Brain failure. I meant 43 and 44, the former being now in Aurora, not 42.

Comment 9

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/4ac91cde9a3d299dc3c05d1c6d247ba30d0ff3be
Bug 1207108 - Duplicated 'bookmarks_title' string in android_strings.dtd.r=nalexander

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ac91cde9a3d

Comment 11

[Francesco Lodolo [:flod]]

Comment on attachment 8664385 [details] [diff] [review]
idCollisionForBookmarkStrings

Approval Request Comment
[Feature/regressing bug #]: duplicated string ID breaks localization tools that expose only one of the two English strings.
[User impact if declined]: localized builds might have wrong translation for the home panel Bookmarks.
[Describe test coverage new/current, TreeHerder]: -
[Risks and why]: low.
[String/UUID change made/needed]: removed one unused string.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8664385 [details] [diff] [review]
idCollisionForBookmarkStrings

Fix a basic issue, taking it.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac18fcc6bb9f