Bug 1345294 removed the callers for m-c, bug 1414096 removed support from the platform.

The script couldn't handle the following cases: ignoring suite/modules/PlacesUIUtils.jsm:80 because the third instruction isn't the call ignoring suite/common/utilityOverlay.js:1570 because the third instruction isn't the call ignoring suite/common/src/nsSuiteGlue.js:691 because the second instruction isn't the expected assigment ignoring suite/common/src/nsSuiteGlue.js:1526 because the third instruction isn't the call ignoring suite/browser/nsBrowserContentHandler.js:163 because the third instruction isn't the call ignoring suite/browser/nsBrowserContentHandler.js:215 because the third instruction isn't the call ignoring mail/base/modules/distribution.js:45 because the second instruction isn't the expected assigment I'm fixing the mail/ file by hand with this patch.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/2bfef5c6f2a7 Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (script-generated patch). r=jorgk https://hg.mozilla.org/comm-central/rev/48e041d4c81b Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (hand-written patch). r=jorgk

Comment on attachment 8926411 [details] [diff] [review] script generated patch [landed with slight mods and backed out] Review of attachment 8926411 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: chat/components/src/imAccounts.js @@ +232,5 @@ > > if (this.canJoinChat && > this.prefBranch.prefHasUserValue(kPrefAccountAutoJoin)) { > + let autojoin = this.prefBranch.getStringPref( > + kPrefAccountAutoJoin); I took the liberty to join this to the previous line.

Comment on attachment 8926415 [details] [diff] [review] hand written fix [landed and backed out] Thanks again.

FRG, Florian didn't cover the cases listed in comment #2. I landed this in a hurry, so I have to leave this to you. Shouldn't be hard since you have plenty of examples. Florian, I had to s/è/e/ since landing this from Windows would have ended up in mojibake, I know since we have a contributor with a "ü". Thanks for the patches. A couple of hours earlier would have been better since now we shipped a completely busted Daily build :-(

Chat and newsgroups looking fixed. But mail and feed accounts are still busted f.ex. "nobody on Local Folders" and renaming doesn't work.

(In reply to Richard Marti (:Paenglab) from comment #7) > Chat and newsgroups looking fixed. But mail and feed accounts are still > busted f.ex. "nobody on Local Folders" and renaming doesn't work. There's likely also some C++ code that needs fixing. FWIW the platform patch has just been backed out due to a devtools (trivial) test failure.

Looks like all these might need fixing. I'll do it in a minute: https://dxr.mozilla.org/comm-central/search?q=NS_GET_IID(nsISupportsString&redirect=true

You sure? > ignoring suite/modules/PlacesUIUtils.jsm:80 because the third instruction isn't the call e.g. I think this is not affected. Only support for prefs was removed or do I miss something? FRG

(In reply to Frank-Rainer Grahl (:frg) from comment #10) > You sure? > > > ignoring suite/modules/PlacesUIUtils.jsm:80 because the third instruction isn't the call > > e.g. I think this is not affected. Only support for prefs was removed or do > I miss something? Right, this specific instance isn't affected. The script just dumped to the terminal the places in the code that need a human review.

> Looks like all these might need fixing. Ok they are all affected. Just need to check the js.

I think all of the suite source cases not handled by the script are fine. Thanks for fixing the others.

(In reply to Florian Quèze [:florian] [:flo] from comment #8) > There's likely also some C++ code that needs fixing. FWIW the platform patch > has just been backed out due to a devtools (trivial) test failure. ETA: One hour from now.

Missed the deadline by two minutes :-( Richard, all working now? Aceman, review now or PLR? I give you 60 minutes ;-)

Comment on attachment 8926507 [details] [diff] [review] 1415567-complex-value.patch (v1) Looks much better. Thanks.

Notes for the reviewer: There is also nsPrefBranch::GetStringPref() https://dxr.mozilla.org/comm-central/source/mozilla/modules/libpref/Preferences.cpp#2413 which is geared around JS and setting a default value. It calls GetCharPref() anyway. nsPrefBranch::SetStringPref() https://dxr.mozilla.org/comm-central/source/mozilla/modules/libpref/Preferences.cpp#2434 calls SetCharPrefInternal() which in the end calls PREF_SetCStringPref() just like nsPrefBranch::SetCharPref() via nsPrefBranch::SetCharPrefInternal(). The [Get|Set]StringPref() functions are not used in M-C C++ code (since they are geared around JS), so I opted for the [Get|Set]CharPref() functions. http://searchfox.org/mozilla-central/search?q=SetStringPref(&case=false&regexp=false&path=*.cpp http://searchfox.org/mozilla-central/search?q=GetStringPref(&case=false&regexp=false&path=*.cpp

Jorg, found a lonely occurence in suite not covered. Tested and works. Could you give it your blessing and check it in with the other. Lookd at the remaining ones in mail and mailnews too and they all seem to be ok.

Comment on attachment 8926514 [details] [diff] [review] 1415567-lonelysuitechange.patch Thanks, I'll check it in with the C++ patch. I'll change the commit message.

Comment on attachment 8926507 [details] [diff] [review] 1415567-complex-value.patch (v1) Review of attachment 8926507 [details] [diff] [review]: ----------------------------------------------------------------- Reading bug 1345294, it seems setCharPref works in Latin1. I don't think it is safe to push UTF-8 to it. Please use *StringPref() even if it may look the same today. It may still be touched in the future and we want those changes.

Done.

Comment on attachment 8926532 [details] [diff] [review] 1415567-complex-value.patch (v2) Review of attachment 8926532 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/src/nsMsgTagService.cpp @@ +437,5 @@ > > nsresult nsMsgTagService::GetUnicharPref(const char *prefName, > nsAString &prefValue) > { > + nsCString valutf8; Please call this utf8value. 'valutf8' makes me always think there is some character missing in the name ;) ::: mailnews/base/test/TestMsgStripRE.cpp @@ +67,2 @@ > // set localizedRe pref > + rv = prefBranch->SetStringrPref("mailnews.localizedRe", NS_ConvertUTF8toUTF16(utf8Prefixes)); Typo in SetStringrPref() . Also, utf8Prefixes is already in utf-8 so why convert to utf16? It just needs to be adopted into a nsCString to send to SetStringPref(). ::: mailnews/base/util/nsMsgIdentity.cpp @@ +438,5 @@ > { > if (!mPrefBranch) > return NS_ERROR_NOT_INITIALIZED; > > + nsCString valutf8; utf8value please. ::: mailnews/base/util/nsMsgIncomingServer.cpp @@ +521,5 @@ > { > if (!mPrefBranch) > return NS_ERROR_NOT_INITIALIZED; > > + nsCString valutf8; utf8value ::: mailnews/base/util/nsMsgUtils.cpp @@ +1263,5 @@ > pbr = do_GetService(NS_PREFSERVICE_CONTRACTID); > prefBranch = pbr; > } > > + nsCString valutf8; utf8value

Fixed variable name and typo. Good to go now?

Forgot to fix TestMsgStripRE.cpp.

Comment on attachment 8926551 [details] [diff] [review] 1415567-complex-value.patch (v3b) Review of attachment 8926551 [details] [diff] [review]: ----------------------------------------------------------------- OK, builds for me, but the TestMsgStripRE.cpp does not seem to be compiled as part of normal build.

With correct check-in message.

This compiles for TestMsgStripRE.cpp and removes the horrible octal stuff.

All too late now since bug 1414096 was backed out. So backout of the first two landed changesets: https://hg.mozilla.org/comm-central/rev/3617ae4c65c86ddcc56f0b2b8651e7bbf73b0132 https://hg.mozilla.org/comm-central/rev/497ee9e16aff5daeb90d253773876185893f1e62

So we'll need to reland all four patches when bug 1414096 relands.

Comment on attachment 8926582 [details] [diff] [review] 1415567-complex-value.patch (v3c) Review of attachment 8926582 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/test/TestMsgStripRE.cpp @@ +61,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + // set localizedRe pref, value "SV,ÆØÅ" > + rv = prefBranch->SetStringPref("mailnews.localizedRe", > + NS_LITERAL_CSTRING("SV,\xC3\x86\xC3\x98\xC3\x85")); Probably OK, but please keep some comment that this is already utf8 encoded so we do not wonder in the future. @@ +66,1 @@ > NS_ENSURE_SUCCESS(rv, rv); For me these didn't compile are the function ought to return int. So NS_ENSURE_SUCCESS(rv, 1); worked for me in both cases.

Apologies for the breakage. I was intending to fix comm-central but it slipped my mind at the end :( Also, I don't think these patches needed to be backed out. They should be landable before bug 1414096 because they are switching from [gs]etComplexValue() to [gs]etStringPref(), which already works. I'm happy to wait for these patches to reland before relanding bug 1414096 if that's helpful?

Yeah, we may only need bug 1345294 in to get *StringPref() and the we can already use it with these patches.

(In reply to :aceman from comment #30) > Probably OK, but please keep some comment that this is already utf8 encoded > so we do not wonder in the future. That's actually quite obvious for someone working in that field. > For me these didn't compile are the function ought to return int. So > NS_ENSURE_SUCCESS(rv, 1); worked for me in both cases. You go and fix it then in another bug. And add the comment you like. (In reply to Nicholas Nethercote [:njn] from comment #31) > Also, I don't think these patches needed to be backed out. They should be > landable before bug 1414096 because they are switching from > [gs]etComplexValue() to [gs]etStringPref(), which already works. I'm happy > to wait for these patches to reland before relanding bug 1414096 if that's > helpful? Oops, how silly of me. I told Aceman that he didn't need to rebuild with the latest M-C to test this. Well, too late, they are backed out. You can reland bug 1414096 any time, I'm watching the M-C merges. But while we're here, I'll f? you on the C++ patch.

Comment on attachment 8926582 [details] [diff] [review] 1415567-complex-value.patch (v3c) We were discussing whether to use [Get|Set]CharPref or [Get|Set]StringPref. Internally it's all the same, no? There's a superseded patch that uses [Get|Set]CharPref .

> We were discussing whether to use [Get|Set]CharPref or [Get|Set]StringPref. > Internally it's all the same, no? There's a superseded patch that uses > [Get|Set]CharPref . They are definitely not the same! {Get,Set}StringPref and treat the strings as UTF8 (AUTF8String in the .idl), same as {Get,Set}ComplexValue(nsISupportsString). But {Get,Set}CharPref treat the strings as Latin1 (ACString in the .idl). In practice it might not make a difference, depending on the exact combination of getters and setters used. (Problems occur if you pass non-UTF8 strings through XPIDL methods that expect UTF8; it will throw "invalid UTF8" errors.) But in summary, you should use {Get,Set}StringPref, because that matches the existing code and will avoid any potential problems.

(In reply to Nicholas Nethercote [:njn] from comment #35) > They are definitely not the same! I was talking about C++ code only. According to my comment #17, the C++ interfaces {Get,Set}StringPref and {Get,Set}CharPref do the same thing.

Oh, from C++ code: - SetStringPref and SetCharPref are identical. - GetStringPref and GetCharPref have slightly different interfaces. GetCharPref is probably easier to use.

That's right. That's why I suggested attachment 8926507 [details] [diff] [review] using {Get,Set}CharPref, but Aceman spotted this hunk https://hg.mozilla.org/mozilla-central/rev/e843de356b7e#l6.19 and made me switch to the String versions. Both do pretty much the same. Maybe you can advise which patch should go ahead.

When I wrote that hunk I didn't realize that GetCharPref was a reasonable alternative. I might change that hunk in my own patch before landing.

Comment on attachment 8926582 [details] [diff] [review] 1415567-complex-value.patch (v3c) Review of attachment 8926582 [details] [diff] [review]: ----------------------------------------------------------------- This is fine, though as discussed you could switch from the "String" to the "Char" funcs, which would let you remove the `EmptyCString(), 0` arguments.

I still do not agree here. If we use *CharPref() and *StringPref() still exists, we will lose semantics that the passed in values are utf8 and are a bit different to plain latin1. We may need this information in the future. Like today, we could easily grep which calls are the full utf8/utf16 strings by looking for the *ComplexValue+nsISupportsString. If the C++ implementation is the same NOW, you call *CharPref() from *StringPref() internally. But callers should use the right API. It is as if in HTML you used <H1> for headings and for the second level headings you ignored that <H2> exists, but used <H1 style="fonts-size: 80%">, because "hey, it looks the same as H2 now and I see H2 is defined the same way internally in Firefox's default style". Yes, it may look the same for some time, but the sematics is lost that those are second level headings. And the default style may change in Firefox any time in the future.

Added comment and fixed NS_ENSURE_SUCCESS(rv, 1) as per comment #30. Aceman should be so happy now.

Since the M-C got backed out, we have time for some try runs. Try with three JS patches only: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8a18d18307536b20525b938f9b92009754d835e7 Try with JS patches and C++ patch: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3d0bc149d876885b60dc1b9cff2f5cb957cf1fc9 Try run with C++ patch only: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2169f202bf3e672d0c8eb05ce0098b43d1e9f14 I'm doing this since last night, with the M-C patch applied and all four C-C patches, I got a failure in calendar/test/unit/test_ltninvitationutils.js which can also be see here: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=296efe53253a743c9645942f3b84608145a68313&selectedJob=143027414 and here https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=48e041d4c81b126540eb255ad6b4a5ccce86feb1&selectedJob=143083995 So let's see who is the culprit here.

Tries are green, well, one failure on Linux with the C++ patch only. I'll land this during the day when my latest push turn out OK.

Comment on attachment 8926754 [details] [diff] [review] 1415567-complex-value.patch (v3d) Review of attachment 8926754 [details] [diff] [review]: ----------------------------------------------------------------- I hope we are both happy now ;)

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/3a0c2f6597ef Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (script-generated patch). r=jorgk https://hg.mozilla.org/comm-central/rev/4b31b1b319fc Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (hand-written patch). r=jorgk https://hg.mozilla.org/comm-central/rev/90cf5e82cb3d Port bug 1414096 to SM: Remove {get,set}ComplexValue use of nsISupportsString (hand-written patch). r=jorgk https://hg.mozilla.org/comm-central/rev/6025ae6cc524 Port bug 1414096 to mailnews: Remove {get,set}ComplexValue use of nsISupportsString (C++ code). r=aceman