remove unused browser.search.param.yahoo-fr* prefs

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gavin, Assigned: abdelrahman, Mentored)

Tracking

Trunk
Firefox 38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

See bug 995390 comment 6. These are unused now and can be removed from all of our default pref files:

https://mxr.mozilla.org/mozilla-central/search?string=yahoo-fr
The reference in yahoometrofx.xml can be ignored since that plugin is no longer used.
(Assignee)

Comment 2

4 years ago
Posted patch rev 1 - Remove unused prefs (obsolete) — Splinter Review
Attachment #8546609 - Flags: review?(gavin.sharp)
Comment on attachment 8546609 [details] [diff] [review]
rev 1 - Remove unused prefs

Please omit the change to the yahoometrofx.xml file, that file is also unused (and will be removed). r=me with that change reverted.

The "ms-pc-metro" pref you're removing from browser/branding/official/pref/firefox-branding.js is not actually in scope for this bug, but since it is only used by a metro search plugin (which will soon be removed), that's fine.

Thanks for the patch!
Attachment #8546609 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 4

4 years ago
Posted patch rev 2 - Remove unused prefs (obsolete) — Splinter Review
Attachment #8546929 - Flags: review?(gavin.sharp)
Comment on attachment 8546929 [details] [diff] [review]
rev 2 - Remove unused prefs

First, apologies for the delayed review response.

My mention of "ms-pc-metro" in comment 3 was not meant to suggest that you should undo that change, I was just providing some context for posterity. "That's fine" meant that you should make that change, so please add that back in.

In general, if you receive r+ with instruction to make a change, you do not need to request review again on the updated patch, as long as you've only made the requested change.

If you attach an updated patch here, you can also go ahead and add r=gavin to the commit message, so that it can easily be checked in.

This looks good with all of that taken into account, thanks again.
Attachment #8546929 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 6

4 years ago
Can rev 2 be Obsoleted? and just check-in rev 1.
(Assignee)

Comment 7

4 years ago
(In reply to Abdelrhman Ahmed from comment #6)
> Can rev 2 be Obsoleted? and just check-in rev 1.

and also do I have privilege to perform check-in?
Flags: needinfo?(gavin.sharp)
Rev 1 is not suitable because it contains the yahoometrofx.xml change (which must be excluded). Rev 2 is not suitable because it does not remove browser.search.param.ms-pc-metro from browser/branding/nightly/pref/firefox-branding.js.

Rev 3 should include the removal of browser.search.param.ms-pc-metro from browser/branding/nightly/pref/firefox-branding.js, but not change yahoometrofx.xml.

It would be best to attach a new patch so that you can also include the revised commit comment (including "r=gavin").
Flags: needinfo?(gavin.sharp)
(Assignee)

Comment 9

4 years ago
Attachment #8546609 - Attachment is obsolete: true
Attachment #8546929 - Attachment is obsolete: true
Thanks, now I'll mark this checkin-needed so that someone can check it in for you.

(No try push needed since this is a simple pref removal.)
Assignee: nobody → a.ahmed1026
Keywords: checkin-needed
Mentor: gavin.sharp
https://hg.mozilla.org/integration/fx-team/rev/c8bca1420a46
Keywords: checkin-needed
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c8bca1420a46
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug]
Target Milestone: --- → Firefox 38
Blocks: 1195683
tl;dr this bug removed codes that are used outside of mozilla-central, namely localized files.  We should be careful at all times with anything related to partner codes.

Comment 14

4 years ago
i presume mike's comment 13 relates to bug 1195683
You need to log in before you can comment on or make changes to this bug.