749931 - "Migration to a clean Firefox profile" feature doesn't suggest browser history migration

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Alexander L. Slovesnik]

Attachment: Screenshot in nightly

STR:
1) Open about:support
2) Click on "Reset Nightly"

Expected result:
"Browsing history" is present in list of migrated data

Expected result:
"Browsing history" is absent in list of migrated data

I use Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120427 Firefox/15.0a1

Comment 1

[Alexander L. Slovesnik]

Attachment: Screenshot in 13.0b1

Screenshot in 13.0b1 for comparison

Comment 2

[Alexander L. Slovesnik]

Last good build - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-04-14-03-07-31-mozilla-central/firefox-14.0a1.en-US.win32.zip
First bad build - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-04-15-03-07-25-mozilla-central/firefox-14.0a1.en-US.win32.zip

Regression window - http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=364f0a5a1d2d&tochange=0d871550085e

Perhaps caused by Bug 737381?

Comment 3

[Matthew N. [:MattN]]

(In reply to Alexander L. Slovesnik from comment #2)
> Perhaps caused by Bug 737381?

Yes, it was caused by that bug.  The dialog UI code should now use MU_getLocalizedString to get the names of the items.

Comment 4

[Matthew N. [:MattN]]

Attachment: v.1 Use new MigrationUtils.getLocalizedString to handle string overrides

Thanks for the report Alexander! Note that this feature is broken on Nightly until bug 748047 is fixed.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 619675 [details] [diff] [review]
v.1 Use new MigrationUtils.getLocalizedString to handle string overrides

MigrationUtils is a browser/ module.

So, I think you want to move these files to browser/, but keep them in the current chrome address. That way, each app would have to provide this ui, in that chrome address. That's better than forcing the app to provide locale files and modules...

Alternatively, about:support could be moved to browser/. I think the benefit of having it in toolkit is very limited.

Comment 6

[Matthew N. [:MattN]]

Attachment: v.2 Use new MigrationUtils.getLocalizedString to handle string overrides (Cu.import in getMigratedData)

(In reply to Mano from comment #5)
> Comment on attachment 619675 [details] [diff] [review]
> v.1 Use new MigrationUtils.getLocalizedString to handle string overrides
> 
> MigrationUtils is a browser/ module.
> 
> So, I think you want to move these files to browser/, but keep them in the
> current chrome address. That way, each app would have to provide this ui, in
> that chrome address. That's better than forcing the app to provide locale
> files and modules...

It doesn't seem necessary to require an application to re-implement the dialog when they already have to implement the migrator.  I moved the import into getMigratedData to address the issue since resetSupported already checks that a migrator for the app exists since migrators are also browser components.  Also keep in mind that this change needs to land on Aurora.

> Alternatively, about:support could be moved to browser/. I think the benefit
> of having it in toolkit is very limited.

That could be moved as a separate bug if desired but it shouldn't block this follow-up fix caused by a string change.  Note that other applications are using it.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 620856 [details] [diff] [review]
v.2 Use new MigrationUtils.getLocalizedString to handle string overrides (Cu.import in getMigratedData)

Please do file a bug about figuring out where should this code live. I agree that migration.properties dependency is just as bad as MigrationUtils dependency.

Comment 8

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86e0e6f1e48b

(In reply to Mano from comment #7)
> Please do file a bug about figuring out where should this code live. I agree
> that migration.properties dependency is just as bad as MigrationUtils
> dependency.

Filed bug 759596

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/86e0e6f1e48b

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 620856 [details] [diff] [review]
v.2 Use new MigrationUtils.getLocalizedString to handle string overrides (Cu.import in getMigratedData)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 737381
User impact if declined: history will not be included in the list of preserved data for a reset (which may deter users).
Testing completed (on m-c, etc.): Verified on Nightly 20120530
Risk to taking this patch (and alternatives if risky): Low risk. Using migrationutils API rather than relying directly on strings.
String or UUID changes made by this patch: Deletion of unused 32_firefox.

Comment 11

[Alex Keybl [:akeybl]]

We're looking to approve this, but want to double check with Axel whether anything needs to be done around the string removal before landing.

Comment 12

[Axel Hecht [:Pike]]

Per IRC, please don't approve attachment 620856 [details] [diff] [review], it's not doing what it should. Notably, it relies on strings in l10n not being there (32_firefox), which we can't guarantee.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 620856 [details] [diff] [review]
v.2 Use new MigrationUtils.getLocalizedString to handle string overrides (Cu.import in getMigratedData)

as per Axel's comment above, not approving for aurora.

Comment 14

[Alex Keybl [:akeybl]]

My understanding is that this feature is only available through about:support in FF14. SUMO and SUMO users will be the main consumer of this feature for this cycle. Given that, this isn't a blocker in my eyes. Please re-nominate if you disagree.

That being said, if a low risk solution was found (english only?), we'd definitely approve.

Comment 15

[Matthew N. [:MattN]]

Attachment: v.1 For 14 - Don't depend on existence of string

Still time for beta?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 737381
User impact if declined: history will not be included in the list of preserved data for a reset (which may deter users).
Testing completed (on m-c, etc.): Not yet as 1st approach landed on m-c already but was denied aurora.
Risk to taking this patch (and alternatives if risky): Low risk. Using specific strings rather than assuming existence of string implies it's migrated.
String or UUID changes made by this patch: Deletion of unused 32_firefox.

Comment 16

[Matthew N. [:MattN]]

Attachment: v.3 (m-c/15) Don't depend on the existence of the string (based on IRC discussion)

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 639949 [details] [diff] [review]
v.1 For 14 - Don't depend on existence of string

Why not just leave the string on beta? Removing it will cause confusion, which seems like more trouble than it's worth.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 639950 [details] [diff] [review]
v.3 (m-c/15) Don't depend on the existence of the string (based on IRC discussion)

Why is this code using "checkbox" as a variable containing a label element? :)

Maybe you should put a note in migration.properties that suggests making sure the hardcoded list in resetProfile.js is up to date?

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 639949 [details] [diff] [review]
v.1 For 14 - Don't depend on existence of string

[Triage Comment]
Approving for Beta 14. If we see any major regressions, we'll either disable for our final mozilla-release build or advise SUMO not to use the feature.

Comment 20

[Matthew N. [:MattN]]

attachment 639950 [details] [diff] [review]: https://hg.mozilla.org/integration/mozilla-inbound/rev/89e5bededa3e
attachment 639949 [details] [diff] [review]: https://hg.mozilla.org/releases/mozilla-beta/rev/2f60b52f5a6c (Tested in local build)

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 639950 [details] [diff] [review]
v.3 (m-c/15) Don't depend on the existence of the string (based on IRC discussion)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 737381 + l10n fallout from attachment 620856 [details] [diff] [review] which landed on m-c and migrated to aurora.
User impact if declined: bookmarks may be included twice in the list of preserved data for a reset in some locales if they don't delete the 32_firefox string. (Translated once from "Bookmarks" and again from "History and Bookmarks").
Testing completed (on m-c, etc.): Local build and pushed to inbound today.
Risk to taking this patch (and alternatives if risky): Low risk. Using a list of strings rather than assuming existence of string implies it's migrated.
String or UUID changes made by this patch: None

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89e5bededa3e

Comment 23

[Matthew N. [:MattN]]

Thanks Gavin and Alex.

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

Comment 24

[Virgil Dicu [:virgil] [QA]]

Attachment: Screenshot of Firefox 15beta3

Verified on Ubuntu 12.04, Mac OS 10.7, Windows 7.

All strings present in 13 are present in 15 as well, but in a different order.

Comment 25

[Md. Ehsanul Hassan]

I have reproduced this bug on Firefox nightly 15.0a1 on windows(64bit).

this bug is verified as fixed .
Latest Firefox nightly 47.0a1 on windows(64bit)
Build Id:20160304030206
user Agent:Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0