907578 - Clicking on an nsIPrefLocalizedString setting in about:config results in a box with "chrome:/.../.properties"

Status: RESOLVED INVALID

(Firefox for Metro Graveyard :: General, defect)

Description

[donrhummy]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Tried to click on a setting in about:config to change it


Actual results:

It shows as an input box with some weird string (which looks like an XUL path to a file where all the about:config settings are stored) in it. This string is the same for every setting. It was something like: "chrome:/somepath/somfile.properties"


Expected results:

It should have shown an input box with the current setting in there.

Comment 1

[Matt Brubeck (:mbrubeck)]

Does this happen for literally every pref, or just for ones that have .properties file URIs in the default prefs file, like these?
http://hg.mozilla.org/mozilla-central/file/89294cd501d9/browser/metro/profile/metro.js#l243

For reference, these are preferences that use the nsIPrefLocalizedString interface:
https://developer.mozilla.org/en-US/docs/Code_snippets/Preferences#nsIPrefLocalizedString

Comment 2

[donrhummy]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Does this happen for literally every pref, or just for ones that have
> .properties file URIs in the default prefs file, like these?
> http://hg.mozilla.org/mozilla-central/file/89294cd501d9/browser/metro/
> profile/metro.js#l243
> 
> For reference, these are preferences that use the nsIPrefLocalizedString
> interface:
> https://developer.mozilla.org/en-US/docs/Code_snippets/
> Preferences#nsIPrefLocalizedString

I'm not sure. It happened with every one I tried including setting the default search engine (which was Bing!?!).

I'll try to check some others, can you suggest one that you know is not in that default file?

Comment 3

[Matt Brubeck (:mbrubeck)]

"layout.css.dpi" or "intl.direction.ar" are some that are not nsIPrefLocalizedString.  ("browser.search.defaultenginename" happens to be one of the few that is a nsIPrefLocalizedString.)

We have code that is supposed to fetch the actual value for these prefs, but apparently it is not working.  If anyone wants to work on this bug, you can start debugging here to figure out why getComplexValue isn't being called or isn't returning the correct result:
http://hg.mozilla.org/mozilla-central/file/44e615a66f3f/browser/metro/base/content/config.js#l310

For comparison, here is the equivalent code used in desktop Firefox:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewconfig/content/config.js#313

Comment 4

[donrhummy]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> "layout.css.dpi" or "intl.direction.ar" are some that are not
> nsIPrefLocalizedString.  ("browser.search.defaultenginename" happens to be
> one of the few that is a nsIPrefLocalizedString.)
> 
> We have code that is supposed to fetch the actual value for these prefs, but
> apparently it is not working.  If anyone wants to work on this bug, you can
> start debugging here to figure out why getComplexValue isn't being called or
> isn't returning the correct result:
> http://hg.mozilla.org/mozilla-central/file/44e615a66f3f/browser/metro/base/
> content/config.js#l310
> 
> For comparison, here is the equivalent code used in desktop Firefox:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewconfig/
> content/config.js#313

I might be misunderstanding this since I don't know how Mozilla changed things from Desktop to Metro but isn't the error starting on line 309? It sets "pref.value" instead of "pref.valueCol".

FROM Metro Version
> pref.value = branch.getComplexValue(aPrefName, Ci.nsISupportsString).data;

FROM Desktop Version
> pref.valueCol = gPrefBranch.getComplexValue(prefName, nsISupportsString).data;

They're setting different properties.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to donrhummy from comment #4)
> I might be misunderstanding this since I don't know how Mozilla changed
> things from Desktop to Metro but isn't the error starting on line 309? It
> sets "pref.value" instead of "pref.valueCol".

".value" is correct for the Metro version.  (In both cases, these identifiers are just used internally within the about:config code, and are not shared with any other files.)

Comment 6

[donrhummy]

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> (In reply to donrhummy from comment #4)
> > I might be misunderstanding this since I don't know how Mozilla changed
> > things from Desktop to Metro but isn't the error starting on line 309? It
> > sets "pref.value" instead of "pref.valueCol".
> 
> ".value" is correct for the Metro version.  (In both cases, these
> identifiers are just used internally within the about:config code, and are
> not shared with any other files.)

Sorry for going off tangent, but doesn't using different variable/field names for what's essentially the same code seem a bit less maintainable? Why aren't they named the same thing so that people working on both desktop/metro can easily see what's being changed and how they relate?

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to donrhummy from comment #6)
> Sorry for going off tangent, but doesn't using different variable/field
> names for what's essentially the same code seem a bit less maintainable? Why
> aren't they named the same thing so that people working on both
> desktop/metro can easily see what's being changed and how they relate?

Yes, it would be good if more of the code for our different about:config pages was shared.  If anyone would like to extract the common parts into a shared source file, or make the desktop about:config UI more touch-friendly so it can be used on Metro/Android/B2G (and we could discard the platform-specific UIs), that would be great.

Comment 8

[Matt Brubeck (:mbrubeck)]

(By the way, the difference in names reflects a real difference in the UI -- the desktop UI has always used table built with <tree> and <treecol> elements, and one of the treecols has id="valueCol".   The Android and Metro UIs use list views instead of tables, so they don't have any column elements like "valueCol".)

Comment 9

[Abhinav Singhal]

Please assign me this bug to solve. It would be a pleasure to solve this.

Comment 10

[Matt Brubeck (:mbrubeck)]

Closing because Metro Firefox is gone.