854925 - Remove SetCharsetForURI and GetCharsetForURI from nsINavHistoryService

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

Attachment: Remove CPP code

Comment 1

[Raymond Lee [:raymondlee]]

Depreciate warning is added to both method.  We don't use them anymore in the mozilla-central.  Just several add-ons are still using them

Comment 2

[Marco Bonardo [:mak]]

we added deprecation in 25, I think we'll remove the code in 27.

Comment 3

[Marco Bonardo [:mak]]

Some widely (more than 2k users) used add-ons didn't update their code yet:
https://addons.mozilla.org/en-US/firefox/addon/pentadactyl/
https://addons.mozilla.org/en-US/firefox/addon/new-tab-jumpstart/
https://addons.mozilla.org/en-US/firefox/addon/cybersearch/
https://addons.mozilla.org/en-US/firefox/addon/vimperator/

we probably need help from the AMO team to reach their authors and see if they can move to the new APIs, provided they really need to set(and thus enforce)/get the charset.
The replacements are PlacesUtils.setCharsetForURI and PlacesUtils.getCharsetForURI, both return a Promise (this differs from the old API that was directly returning the value).

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

This should be easy to detect using the mass validations we do for every release. No need to block on notifying the developers.

Comment 5

[Marco Bonardo [:mak]]

it's probably just matter of unbitrotting this patch and check there are no consumers in mxr, so an easy one.

Comment 6

[Anup Kumar [:reznord]]

Hi,

I am interested in working on this bug. Can you please assign this bug to me?

Thanks in advance,

Regards,
Anup

Comment 7

[Anup Kumar [:reznord]]

Attachment: Removed SetCharsetForURI and GetCharsetForURI and updated the UUID.

Removed SetCharsetForURI and GetCharsetForURI from nsINavHistoryService and updated the UUID in nsINavHistoryService.idl

Comment 8

[Anup Kumar [:reznord]]

The try request link:

https://tbpl.mozilla.org/?tree=Try&rev=362aa575034c

Comment 9

[Carsten Book [:Tomcat]]

this still need review right? at least i see no r+ for attachment 8460264 [details] [diff] [review]

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8460264 [details] [diff] [review]
Removed SetCharsetForURI and GetCharsetForURI and updated the UUID.

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

It looks good!
you just need to add r=mak to the checkin comment, and it will be ready for check-in. no need for further reviews.

Comment 11

[Anup Kumar [:reznord]]

Attachment: bug854925.patch

added r=mak and added the checkin needed also.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8462797 [details] [diff] [review]
bug854925.patch

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

no need for a further review

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b70f46a09115

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b70f46a09115