Closed Bug 366774 Opened 18 years ago Closed 13 years ago

Remove unused preference bidi.characterset

Categories

(Core :: Layout: Text and Fonts, enhancement)

Product:
Core
Component:
Layout: Text and Fonts
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla6

People

(Reporter: fahlmanc_ca, Assigned: ewong)

Details

Attachments

(1 file, 9 obsolete files)

Remove unused preference bidi.characterset(v4)
7.99 KB, patch
ewong
: review+
Details | Diff | Splinter Review 
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
(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: nobody → ewong
Status: NEW → ASSIGNED
Attachment #520496 - Flags: review? → review?(benjamin)

        Attachment #520497 -
        Flags: review?(bzbarsky)

    
    

    
  


  

        Attachment #520498 -
        Flags: review?(benjamin)

    
  

        Attachment #520499 -
        Flags: review? → review?(mrbkap)

    
    

    
  


  

        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.

    
    

    
  


  

        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+

    
    

    
  


  

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

    
  

        Attachment #520499 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
Turned the whole tree red, I think because patch 5 has a missing close paren.

    
    

    
  
Ah right:
+         (IBMBIDI_SUPPORTMODE_MOZILLA<<12)
should be:
+         (IBMBIDI_SUPPORTMODE_MOZILLA<<12))

    
    

    
  


  
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.

    
    

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

    
    

    
  


  
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+

    
    

    
  
You *did* push to tryserver this time didn't you?
Status: REOPENED → ASSIGNED

    
    

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

Pushed to try with a green tree.

    
    

    
  
Though the commit message should include "r=smontagu"

    
    

    
  


  

        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?

    
  

        Attachment #528059 -
        Attachment is obsolete: true

    
  

        Attachment #520506 -
        Attachment is obsolete: true

    
  

        Attachment #520497 -
        Attachment is obsolete: true

    
  

        Attachment #520496 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
  
Keywords: checkin-needed

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/95dad5bda710
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: