The default bug view has changed. See this FAQ.

Remove unused preference bidi.characterset

VERIFIED FIXED in mozilla6

Status

()

Core
Layout: Text
--
enhancement
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: crf, Assigned: ewong)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

7.99 KB, patch
ewong
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-GB; rv:1.8.0.9) Gecko/20061211 SeaMonkey/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-GB; rv:1.8.0.9) Gecko/20061211 SeaMonkey/1.0.7

In about:config there is a setting bidi.characterset , but other settings use within them the abbreviated term charset. For consistency, bidi.characterset could be renamed to bidi.charset.





Reproducible: Always

Steps to Reproduce:
1.
2.
3.
The setting isn't used and probably never will be (see bug 79676 comment 24, third paragraph from the end). Rather than renaming it, it should just be removed.
OS: Windows 98 → All
Hardware: PC → All
No comment in over a year.
Assignee: general → nobody
Component: General → Preferences
QA Contact: general → prefs
Version: unspecified → Trunk

Comment 3

9 years ago
(In reply to comment #1)
> The setting isn't used and probably never will be
I just traced this. There is an attribute bidiCharacterSet on nsIMarkupDocumentViewer.idl (implemented by nsDocumentViewer.cpp) which calls through to nsPresContext.cpp which reads the default from preferences.

Nobody actually uses the attribute ;-)
Editing the Subject to reflect what was said to comment #1 and #3, and confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Rename the setting bidi.characterset to bidi.charset to be consistent → Remove unused preference bidi.characterset
(Assignee)

Updated

6 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 520496 [details] [diff] [review]
Removed unused preference bidi.characterset (p1) (v1)
Attachment #520496 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #520496 - Flags: review? → review?(benjamin)
(Assignee)

Comment 6

6 years ago
Created attachment 520497 [details] [diff] [review]
Removed unused preference bidi.characterset (p2) (v1)
Attachment #520497 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

6 years ago
Created attachment 520498 [details] [diff] [review]
Removed unused preference bidi.characterset (p3) (v1)
Attachment #520498 - Flags: review?(benjamin)
(Assignee)

Comment 8

6 years ago
Created attachment 520499 [details] [diff] [review]
Removed unused preference bidi.characterset (p4) (v1)
Attachment #520499 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #520499 - Flags: review? → review?(mrbkap)
(Assignee)

Comment 9

6 years ago
Created attachment 520500 [details] [diff] [review]
Removed unused preference bidi.characterset (p5) (v1)
Attachment #520500 - Flags: review?(smontagu)
Comment on attachment 520500 [details] [diff] [review]
Removed unused preference bidi.characterset (p5) (v1)

> //  ------------------
> //  Charset Mode
> //  ------------------

These comment lines should be removed, and also the following #defines:
 IBMBIDI_CHARSET_STR
 IBMBIDI_CHARSET
 GET_BIDI_OPTION_CHARACTERSET
 SET_BIDI_OPTION_CHARACTERSET

>+#define IBMBIDI_NULL      0 // placeholder for future use.

This isn't necessary.
(Assignee)

Comment 11

6 years ago
Created attachment 520502 [details] [diff] [review]
Removed unused preference bidi.characterset (p5) (v2)
Attachment #520500 - Attachment is obsolete: true
Attachment #520502 - Flags: review?(smontagu)
Attachment #520500 - Flags: review?(smontagu)
Attachment #520502 - Flags: review?(smontagu) → review+
Comment on attachment 520496 [details] [diff] [review]
Removed unused preference bidi.characterset (p1) (v1)

If nobody objects, I think my review should be good for the other patches here too.
Attachment #520496 - Flags: review?(benjamin) → review+
Attachment #520497 - Flags: review?(bzbarsky) → review+
Comment on attachment 520498 [details] [diff] [review]
Removed unused preference bidi.characterset (p3) (v1)

> // ------------------
> //  Charset Mode
> // ------------------

Delete these lines here too. r=me with that
Attachment #520498 - Flags: review?(benjamin) → review+
Attachment #520499 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 520506 [details] [diff] [review]
Removed unused preference bidi.characterset (p3) (v2)
Attachment #520498 - Attachment is obsolete: true
Attachment #520506 - Flags: review+
Comment on attachment 520499 [details] [diff] [review]
Removed unused preference bidi.characterset (p4) (v1)

This isn't really necessary. This is just a test that's using the pref as part of some data.
Attachment #520499 - Flags: review+ → review-
By "pref" I meant "pref name".
(Assignee)

Updated

6 years ago
Attachment #520499 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Comment 17

6 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/582c92878440d956e227341c66603de34d2e3b8e
http://hg.mozilla.org/mozilla-central/rev/baf7456adc9854ea331f9defc096beb1d2990145
http://hg.mozilla.org/mozilla-central/rev/3f7277e15d8df0907d14d50f194cc0cef17f7da4
http://hg.mozilla.org/mozilla-central/rev/96bfa1480f5f9332650034b503cc34240c6579fd
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.2a1
Turned the whole tree red, I think because patch 5 has a missing close paren.
Backed out:
https://hg.mozilla.org/mozilla-central/rev/9bc24665869d
https://hg.mozilla.org/mozilla-central/rev/e49949eeaef3
https://hg.mozilla.org/mozilla-central/rev/2928c63ffb5b
https://hg.mozilla.org/mozilla-central/rev/8db1df84708c
https://hg.mozilla.org/mozilla-central/rev/cb191997bd5e
https://hg.mozilla.org/mozilla-central/rev/54b9f7d8db85
https://hg.mozilla.org/mozilla-central/rev/cca1fece061f
https://hg.mozilla.org/mozilla-central/rev/8302f5191d92
Assignee: ewong → nobody
Status: RESOLVED → REOPENED
Component: Preferences → Layout: Text
Product: SeaMonkey → Core
QA Contact: preferences → layout.fonts-and-text
Resolution: FIXED → ---
Target Milestone: seamonkey2.2a1 → ---
Assignee: nobody → ewong

Comment 20

6 years ago
Ah right:
+         (IBMBIDI_SUPPORTMODE_MOZILLA<<12)
should be:
+         (IBMBIDI_SUPPORTMODE_MOZILLA<<12))
(Assignee)

Comment 21

6 years ago
Created attachment 528059 [details] [diff] [review]
Remove unused preference bidi.characterset p5 (v3)

Fixed missing parenthesis.
Attachment #520502 - Attachment is obsolete: true
Attachment #528059 - Flags: review+
Edmund: if you don't mind, it'd be nice to have commit messages that more clearly explain what each patch does. Also, it seems like part 1 wouldn't compile without part 2, so these should be merged, and you need to update the uuid for nsIMarkupDocumentViewer and nsIMarkupDocumentViewer_MOZILLA_2_0_BRANCH.

Comment 23

6 years ago
> nsIMarkupDocumentViewer_MOZILLA_2_0_BRANCH.

Ixnay on this as there is an existing bug in progress to remove all *MOZILLA_2_0_BRANCH interfaces from mozilla-central.
(Assignee)

Comment 24

6 years ago
Created attachment 528093 [details] [diff] [review]
Removed the unused preference bidi.characterset references.

This patch merges all the aforementioned patches into one patch with the following changes:

Changed the UUID for nsIMarkupDocumentViewer and
nsIMarkupDocumentViewer_MOZILLA_2_0_BRANCH.
Attachment #528093 - Flags: review+

Comment 25

6 years ago
You *did* push to tryserver this time didn't you?
Status: REOPENED → ASSIGNED
(Assignee)

Comment 26

6 years ago
(In reply to comment #25)
> You *did* push to tryserver this time didn't you?

Pushed to try with a green tree.

Updated

6 years ago
Keywords: checkin-needed
Though the commit message should include "r=smontagu"
(Assignee)

Comment 28

6 years ago
Created attachment 529345 [details] [diff] [review]
Remove unused preference bidi.characterset(v4)
Attachment #528093 - Attachment is obsolete: true
Attachment #529345 - Flags: review+
So which patch needs to be checked in?  It'd be nice to mark obsolete patches as obsolete....
Anyone?
(Assignee)

Updated

6 years ago
Attachment #528059 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #520506 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #520497 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #520496 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Sure.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/95dad5bda710
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue using Windows XP, Windows 7, Ubuntu and Mac OS X - bidi.characterset preference is removed about:config.

Setting resolution to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.