Last Comment Bug 749931 - "Migration to a clean Firefox profile" feature doesn't suggest browser history migration
: "Migration to a clean Firefox profile" feature doesn't suggest browser histor...
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: Trunk
: All All
: -- major (vote)
: Firefox 15
Assigned To: Matthew N. [:MattN] (PM me if requests are blocking you)
:
: Matthew N. [:MattN] (PM me if requests are blocking you)
Mentors:
Depends on: 737381
Blocks: 717070
  Show dependency treegraph
 
Reported: 2012-04-28 04:36 PDT by Alexander L. Slovesnik
Modified: 2016-03-19 11:07 PDT (History)
10 users (show)
MattN+bmo: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
verified
fixed


Attachments
Screenshot in nightly (21.79 KB, image/png)
2012-04-28 04:36 PDT, Alexander L. Slovesnik
no flags Details
Screenshot in 13.0b1 (27.83 KB, image/png)
2012-04-28 04:37 PDT, Alexander L. Slovesnik
no flags Details
v.1 Use new MigrationUtils.getLocalizedString to handle string overrides (2.62 KB, patch)
2012-04-30 13:50 PDT, Matthew N. [:MattN] (PM me if requests are blocking you)
asaf: review-
Details | Diff | Splinter Review
v.2 Use new MigrationUtils.getLocalizedString to handle string overrides (Cu.import in getMigratedData) (2.21 KB, patch)
2012-05-03 14:36 PDT, Matthew N. [:MattN] (PM me if requests are blocking you)
asaf: review+
lukasblakk+bugs: approval‑mozilla‑aurora-
MattN+bmo: checkin+
Details | Diff | Splinter Review
v.1 For 14 - Don't depend on existence of string (2.84 KB, patch)
2012-07-07 02:52 PDT, Matthew N. [:MattN] (PM me if requests are blocking you)
gavin.sharp: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
v.3 (m-c/15) Don't depend on the existence of the string (based on IRC discussion) (1.76 KB, patch)
2012-07-07 03:17 PDT, Matthew N. [:MattN] (PM me if requests are blocking you)
gavin.sharp: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Screenshot of Firefox 15beta3 (144.66 KB, image/png)
2012-08-03 07:14 PDT, Virgil Dicu [:virgil] [QA]
no flags Details

Description Alexander L. Slovesnik 2012-04-28 04:36:44 PDT
Created attachment 619277 [details]
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 2012-04-28 04:37:46 PDT
Created attachment 619278 [details]
Screenshot in 13.0b1

Screenshot in 13.0b1 for comparison
Comment 3 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-04-30 13:20:14 PDT
(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] (PM me if requests are blocking you) 2012-04-30 13:50:46 PDT
Created attachment 619675 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-30 14:24:15 PDT
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] (PM me if requests are blocking you) 2012-05-03 14:36:58 PDT
Created attachment 620856 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-05-29 01:28:18 PDT
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] (PM me if requests are blocking you) 2012-05-29 17:31:14 PDT
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] 2012-05-30 07:39:24 PDT
https://hg.mozilla.org/mozilla-central/rev/86e0e6f1e48b
Comment 10 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-05-30 16:14:37 PDT
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] 2012-06-01 14:59:17 PDT
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] 2012-06-01 15:41:18 PDT
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 2012-06-01 16:30:39 PDT
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] 2012-06-14 08:43:11 PDT
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] (PM me if requests are blocking you) 2012-07-07 02:52:32 PDT
Created attachment 639949 [details] [diff] [review]
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] (PM me if requests are blocking you) 2012-07-07 03:17:02 PDT
Created attachment 639950 [details] [diff] [review]
v.3 (m-c/15) Don't depend on the existence of the string (based on IRC discussion)
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-07 16:43:31 PDT
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: gavin@gavinsharp.com] 2012-07-07 16:44:53 PDT
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] 2012-07-08 08:59:06 PDT
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 21 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-07-08 13:48:41 PDT
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] 2012-07-08 17:49:56 PDT
https://hg.mozilla.org/mozilla-central/rev/89e5bededa3e
Comment 23 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-07-09 00:10:36 PDT
Thanks Gavin and Alex.

https://hg.mozilla.org/releases/mozilla-aurora/rev/b9e356acd3c5
Comment 24 Virgil Dicu [:virgil] [QA] 2012-08-03 07:14:28 PDT
Created attachment 648692 [details]
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 2016-03-19 11:07:59 PDT
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

Note You need to log in before you can comment on or make changes to this bug.