566568 - Preference sync should better recognize localizable strings

Status: RESOLVED WORKSFORME

(Firefox :: Sync, defect)

Description

[Lingfeng.Guan]

When sync homepage from Firefox China Edition to another Firefox, the homepage got set to "data:text/plain,browser.startup.homepage=http://i.g-fox.cn/", and display just browser.startup.homepage=http://i.g-fox.cn/ on the page.

I guess that was because the default homepage in China Edition is written using LocalizablePreference in distribution.ini, 
it was like "data:text/plain,browser.startup.homepage=http://i.g-fox.cn/" when you read it as a normal string.
But if you set that to the user preference, it won't work as a LocalizablePreference.
And that seems to be what weave did.

Comment 1

[Mike Connor [:mconnor]]

Well... this kinda sucks, we expect that to be a string pref.  We could extend Preferences.js to handle these, or you could disable homepage syncing for these builds... hard to say.

Comment 2

[Lingfeng.Guan]

Em... How can I disable homepage syncing when Weave is not bundled in these builds?
Besides, I guess this bug happens to every LocalizablePreference, not only homepage, right?

Comment 3

[Dan Mills [:thunder]]

Sadly, the Firefox pref system is completely bonkers in this regard.

See bugs 386696 and 438925.

Weave should probably match nsIURLFormatter's behavior and (at least for the homepage pref and a handful of others) test whether the pref looks like it's localizable.

Or maybe we could all just agree on what the type of these prefs should be... wouldn't that be nice.

Comment 4

[Lingfeng.Guan]

The first suggestion sounds like a good idea to me :)

Comment 5

[Mike Connor [:mconnor]]

We probably want to declare pref types, in order to special-case prefs like this.

Comment 6

[Mike Kaply [:mkaply]]

Is this still an issue?

Comment 7

[Richard Newman [:rnewman]]

Yes, in both senses: Firefox's pref types are still bonkers (Comment 3, which causes pain like Bug 1091803 Comment 6), and -- to my knowledge -- Firefox Sync hasn't really been reworked to handle them better.

Comment 8

[Mike Kaply [:mkaply]]

I agree that the localizable prefs are weird, but we're not going to fix that.

I'm more trying to understand why a default pref got synced at all.

In Mozilla China, the homepage right now is:

data:text/plain,browser.startup.homepage=about:cehome 

and this is a default preference. So Sync shouldn't sync it at all.

Do we know for a fact that Sync is still trying to sync default preferences? Because that would be the bug...

data:text/plain,browser.startup.homepage=about:cehome does not work as a user preference (and it never did)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mike Kaply [:mkaply] from comment #8)
> Do we know for a fact that Sync is still trying to sync default preferences?

Yeah - bug 744626.

> Because that would be the bug...

Would it be *the* bug, or would it just mitigate the bug for this specific pref?

Comment 10

[Mike Kaply [:mkaply]]

> Would it be *the* bug, or would it just mitigate the bug for this specific pref?

I think it would be the bug. Localizable prefs like these can only be default prefs.

So if sync wasn't syncing default prefs, it wouldn't be an issue.

default prefs shouldn't be turned into user prefs...

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mike Kaply [:mkaply] from comment #10)
> > Would it be *the* bug, or would it just mitigate the bug for this specific pref?
> 
> I think it would be the bug. Localizable prefs like these can only be
> default prefs.

Right - I've never particularly understood these "complex values" for preferences. If I'm reading the code correctly, a call to setComplexValue() still ends up with a normal string it writes to the prefs file, which isn't what I thought happened, but if it does, it supports the idea that simply not syncing default prefs will indeed solve this too.

Comment 12

[Mike Kaply [:mkaply]]

(In reply to Mark Hammond [:markh] from comment #11)
> (In reply to Mike Kaply [:mkaply] from comment #10)
> > > Would it be *the* bug, or would it just mitigate the bug for this specific pref?
> > 
> > I think it would be the bug. Localizable prefs like these can only be
> > default prefs.
> 
> Right - I've never particularly understood these "complex values" for
> preferences. If I'm reading the code correctly, a call to setComplexValue()
> still ends up with a normal string it writes to the prefs file, which isn't
> what I thought happened, but if it does, it supports the idea that simply
> not syncing default prefs will indeed solve this too.

It is confusing.

There are two types of complex prefs, nsISupportsString and nsIPrefLocalizedString.

nsISupportsString complex prefs are used to write UTF-8 values to prefs since they aren't supported by default by setCharPref/getCharPref. And they end up getting written out to prefs as a normal UTF-8 strings.

nsIPrefLocalizedString is used for strings that exist in translations. So the pref looks like chrome://browser-region/locale/region.properties and that file is searched for the value that matches the prefs. The data:text syntax is used when you want to set the preferences directly. You can read this post for more info on that:

https://mike.kaply.com/2012/08/29/setting-the-default-firefox-homepage-with-autoconfig/

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Mike,
  Now bug 744626 has landed, I think we can close this? If you are confident that's true, please just go ahead and close it, or if you'd like to test further that would obviously be awesome too!

Comment 14

[Mike Kaply [:mkaply]]

I'm confident.