The default bug view has changed. See this FAQ.

Remove SetCharsetForURI and GetCharsetForURI from nsINavHistoryService

RESOLVED FIXED in mozilla34

Status

()

Toolkit
Places
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: raymondlee, Assigned: Anupkumar, Mentored)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla34
addon-compat
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=cpp][good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 729590 [details] [diff] [review]
Remove CPP code
(Reporter)

Updated

4 years ago
Depends on: 846635
(Reporter)

Updated

4 years ago
Depends on: 846644
(Reporter)

Updated

4 years ago
Blocks: 880922
(Reporter)

Updated

4 years ago
No longer blocks: 880922
Depends on: 880922
(Reporter)

Updated

4 years ago
No longer depends on: 846635, 846644
(Reporter)

Comment 1

4 years ago
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
we added deprecation in 25, I think we'll remove the code in 27.

Updated

3 years ago
Blocks: 950073
Whiteboard: p=0

Updated

3 years ago
No longer blocks: 950073
Flags: firefox-backlog+
Whiteboard: p=0 → p=1
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).
Flags: needinfo?(jorge)
This should be easy to detect using the mass validations we do for every release. No need to block on notifying the developers.
Flags: needinfo?(jorge)
Keywords: addon-compat

Updated

3 years ago
Whiteboard: p=1 → p=1[lang=cpp][mentor=mak]

Updated

3 years ago
Whiteboard: p=1[lang=cpp][mentor=mak] → p=1[lang=cpp][mentor=mak][good next bug]

Updated

3 years ago
Attachment #729590 - Attachment is patch: true
it's probably just matter of unbitrotting this patch and check there are no consumers in mxr, so an easy one.
Whiteboard: p=1[lang=cpp][mentor=mak][good next bug] → p=1[lang=cpp][mentor=mak][good first bug]
Mentor: mak77@bonardo.net
Whiteboard: p=1[lang=cpp][mentor=mak][good first bug] → p=1[lang=cpp][good first bug]
(Assignee)

Comment 6

3 years ago
Hi,

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

Thanks in advance,

Regards,
Anup

Updated

3 years ago
Assignee: nobody → allamsetty.anup
Status: NEW → ASSIGNED

Updated

3 years ago
Points: --- → 1
Whiteboard: p=1[lang=cpp][good first bug] → [lang=cpp][good first bug]
(Assignee)

Comment 7

3 years ago
Created attachment 8460264 [details] [diff] [review]
Removed SetCharsetForURI and GetCharsetForURI and updated the UUID.

Removed SetCharsetForURI and GetCharsetForURI from nsINavHistoryService and updated the UUID in nsINavHistoryService.idl
Attachment #8460264 - Flags: review?(mak77)
(Assignee)

Comment 8

3 years ago
The try request link:

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

Updated

3 years ago
Keywords: checkin-needed
this still need review right? at least i see no r+ for attachment 8460264 [details] [diff] [review]
Keywords: checkin-needed

Updated

3 years ago
Attachment #729590 - Attachment is obsolete: true
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.
Attachment #8460264 - Flags: review?(mak77) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 11

3 years ago
Created attachment 8462797 [details] [diff] [review]
bug854925.patch

added r=mak and added the checkin needed also.
Attachment #8460264 - Attachment is obsolete: true
Attachment #8462797 - Flags: review?(mak77)
Comment on attachment 8462797 [details] [diff] [review]
bug854925.patch

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

no need for a further review
Attachment #8462797 - Flags: review?(mak77)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b70f46a09115
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b70f46a09115
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

3 years ago
Iteration: --- → 34.1
QA Whiteboard: [qa?]
QA Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.