There are some addons using PlacesUtils.history.setCharsetForURI and PlacesUtils.history.getCharsetForURI so we have to add depreciate warning to those methods first before removing them. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#2773 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#2809

Could you please make or assign this to be done asap?

I assign this to Marcos because I won't be able to look into that now.

Thank you, should be trivial, matter of adding PLACES_WARN_DEPRECATED() and verify it works as expected.

Hi Marco, I'm trying to verify the deprecated warning shows up correctly. I see this xpcShell test calls these 2 methods: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_317472.js#35 How can I check the log to see if the warning is correctly shown when executing this test? Do I need to register a listener on nsiConsoleService or is there another way? Thanks, Marcos.

The test is calling the PlacesUtils methods, not the methods in nsNavHistory. To verify the deprecation you may just directly import PlacesUtils and invoke PlacesUtils.history.getCharsetForURI and setCharsetForURI in the Browser Console, and see that they properly issue a warning in the debug console and post the right information in the Browser Console.

Comment on attachment 772379 [details] [diff] [review] WIP Patch Review of attachment 772379 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with the requested verification.

Comment on attachment 772379 [details] [diff] [review] WIP Patch Hi Marco, JUst tested on the Browser console as you mention, like this: >> var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService); >> function uri(spec) { return ios.newURI(spec, null, null); } >> PlacesUtils.history.getCharsetForURI(uri("http://www.google.com")); GetCharsetForURI is deprecated and will be removed in the next version. >> PlacesUtils.history.setCharsetForURI(uri("http://www.google.com"), "UTF-8"); SetCharsetForURI is deprecated and will be removed in the next version. ********** It's working correctly. Is this enough? Cheers, Marcos.

Comment on attachment 772379 [details] [diff] [review] WIP Patch Review of attachment 772379 [details] [diff] [review]: ----------------------------------------------------------------- If you have a debug build you should also see a WARNING in the debug console, but that's not important since you already verified the console. Thanks!

Recreated patch so it includes user info and summary. Let me know if this is ok for check in. Cheers, Marcos.

Comment on attachment 773402 [details] [diff] [review] Final patch for bug 880922 - Add depreciate warning to both SetCharsetForURI and GetCharsetForURI in nsNavHistory.cpp Review of attachment 773402 [details] [diff] [review]: ----------------------------------------------------------------- The format looks strange, doesn't looks like proper mercurial annotations. Try to verify these steps https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment on attachment 773402 [details] [diff] [review] Final patch for bug 880922 - Add depreciate warning to both SetCharsetForURI and GetCharsetForURI in nsNavHistory.cpp Review of attachment 773402 [details] [diff] [review]: ----------------------------------------------------------------- Oh well, looks like some difference in mercurial, usually it doesn't generate a from header for me, could be different versions but looks like it may be fine. You should just add the reviewer to the commit message, sorry for noise.

BTW, I have pushed the patch to try for you. Please check the outcome. https://tbpl.mozilla.org/?tree=Try&rev=2ca10b665335

Comment on attachment 774040 [details] [diff] [review] Added reviewer name. you already have r+ so don't need to request a review again.

Deprecated warnings are cool. But, I would even more appreciate a hint to a replacement. What function do I have to use now instead of SetCharsetForURI and GetCharsetForURI?

(In reply to Gerd from comment #17) > Deprecated warnings are cool. But, I would even more appreciate a hint to a > replacement. What function do I have to use now instead of SetCharsetForURI > and GetCharsetForURI? PlacesUtils.getCharsetForURI and PlacesUtils.setCharsetForURI, both return a Promise.