880922 - Add depreciate warning to both SetCharsetForURI and GetCharsetForURI in nsNavHistory.cpp

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

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

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 2

[Raymond Lee [:raymondlee]]

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

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 4

[Marcos Aruj]

Attachment: WIP Patch

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.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 772379 [details] [diff] [review]
WIP Patch

Review of attachment 772379 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with the requested verification.

Comment 7

[Marcos Aruj]

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 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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!

Comment 9

[Marcos Aruj]

Attachment: Final patch for bug 880922 - Add depreciate warning to both SetCharsetForURI and GetCharsetForURI in nsNavHistory.cpp

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

Cheers,

Marcos.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 12

[Raymond Lee [:raymondlee]]

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

Comment 13

[Marcos Aruj]

Attachment: Added reviewer name.

Comment 14

[Raymond Lee [:raymondlee]]

Comment on attachment 774040 [details] [diff] [review]
Added reviewer name.

you already have r+ so don't need to request a review again.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1ea1c5fa24

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d1ea1c5fa24

Comment 17

[Gerd]

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?

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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.