Closed Bug 106159 Opened 23 years ago Closed 23 years ago

contentAreaClick.js asks go nsIPref, but value is later clobbered

Categories

(SeaMonkey :: Tabbed Browser, defect)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: spam, Assigned: bnesse)

References

Details

Attachments

(5 files)

patch to convert nsIPref -> nsIPrefBranch
761 bytes, patch
Details | Diff | Splinter Review
More complete patch that switches all js files in /content/ to nsIPrefBranch
6.08 KB, patch
Details | Diff | Splinter Review
Updated for jst's change
6.08 KB, patch
Details | Diff | Splinter Review
Patch to remove all nsIPref references from js & xul files.
109.73 KB, patch
jag+mozilla
: review+
alecf
: superreview+
Details | Diff | Splinter Review
Patch to fix getComplexValue calls (and clean up some warnings)
21.78 KB, patch
samir_bugzilla
: review+
alecf
: superreview+
Details | Diff | Splinter Review 
current CVS build, linux:

When middle-mouse clicking to open a new tab (background) i get this error in
console and no tab opens:

Error: pref.GetBoolPref is not a function
Source File: chrome://communicator/content/contentAreaClick.js Line: 188
This is part of the pref changes that landed today. 

The problem is that while |pref| is initially a '[xpconnect wrapped nsIPref]'
when 'contentAreaClick.js' is loaded, by the time that 'contentAreaClick()' is
called, |pref| has been reset to be a '[xpconnect wrapped (nsISupports,
nsIPrefBranch, nsIPrefBranchInternal)]'. And with the api changes for
nsIPrefBranch, |GetBoolPref| should be |getBoolPref|.

Sorry to say, but I think there are a whole bunch of these type of errors in
the tree right now, and lots of little bits of UI are broken.

Lowercasing the G's did the trick - thanks :)
saw Hyatt just checked in v 1.16 w lowercase - resolving as fixed 
mozilla/xpfe/communicator/resources/content/contentAreaClick.js
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Thanks, R.K., although I'm going to reopen this bug (and gift it on blake).

The thing is that that file begins:

 42   var pref = null;
 43   pref = Components.classes["@mozilla.org/preferences;1"];
 44   pref = pref.getService();
 45   pref = pref.QueryInterface(Components.interfaces.nsIPref);

but by the time it actually uses |pref|, it isn't an nsIPref anymore, it's 
an nsIPrefBranch (likely bonked by navigator.js?).

(Is there a plan for eradicating nsIPref from the js/xul and the binary code?)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
-> blake.
Assignee: hyatt → blakeross
Status: REOPENED → NEW
Summary: middle-mouse click to open new tab gives Error: pref.GetBoolPref is not a function → contentAreaClick.js asks go nsIPref, but value is later clobbered
oops.. i saw no errors in js console, and it works like a dream now, thus the
"fixed". Are you saying there's a leak there?
>but by the time it actually uses |pref|, it isn't an nsIPref anymore, it's 
>an nsIPrefBranch (likely bonked by navigator.js?).
Oh, that's special... globals that replace other globals... :(

The right fix here will probably be to move this over to prefservice/branch. I
can do that.

>(Is there a plan for eradicating nsIPref from the js/xul and the binary code?)
Yes, unfortunately it is tedious and has been a fairly low priority.
->me
Assignee: blakeross → bnesse
The attached patch will create "pref" in a way that is compatible with the one 
created in navigator.js.

Does contentAreaClick.js need to initialize |pref| at all?
I'm going through that whole directory right now. There is some "sharing" of
global(s) going on in there. A number of the files declare pref (or prefs) and
initialize it (them). Other files (contentAreaUtils.js for instance) assume that
it has already been initialized.
jst@netscape.com just checked in a diff against contentAreaClick.js v 1.16
So the diff here should be made against new v 1.17 i guess, depending on what
jst checked in..
*** Bug 106215 has been marked as a duplicate of this bug. ***
jst's changes are unrelated, but I will update the patch...

    
    

    
  
*** Bug 106245 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 106752 has been marked as a duplicate of this bug. ***

    
    

    
  
This isn't really Linux only, right?

    
  
OS: Linux → All
Hardware: PC → All

    
    

    
  
*** Bug 106782 has been marked as a duplicate of this bug. ***

    
    

    
  
I just got a bug duped to this one. My problem is that middle clicking on a mail
link does not open a new window with the link , so my problem is not with tabs.
Anyway, i have today's build and i still cant do this.
Would somebody else take a look at  Bug 106782 and tell me if this will be fixed
by this bug?



    
    

    
  
There is a pretty good chance that there is a relation. It's not entirely tab
specific, it's more about preferences being confused by multiple pref objects
(with different types.)

    
    

    
  
I have not changed my prefs settings in any way like dark wrote in my duped bug,
therefore, it goes open again. I will be gladly to make it dup if when this one
is resolved mine does too

    
    

    
  
*** Bug 106885 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 106935 has been marked as a duplicate of this bug. ***

    
    

    
  
The attached patch should eliminate all possible conflicts between nsIPref and 
nsIPrefBranch in the mozilla tree.


    
    

    
  
Comment on attachment 55286 [details] [diff] [review]
Patch to remove all nsIPref references from js & xul files.

It looks okay. r=jag

        Attachment #55286 -
        Flags: review+

    
    

    
  
Comment on attachment 55286 [details] [diff] [review]
Patch to remove all nsIPref references from js & xul files.

whoo hoo! go brian
sr=alecf

        Attachment #55286 -
        Flags: superreview+

    
    

    
  
Patch checked in.

Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
Ugh, I completely missed that getComplexValue doesn't return a wstring, but a
nsIPrefLocalizedString or nsISupportsWString. This means that in certain cases
you'll need to call |.data| or |.toString()| explicitely to get at the inner
string that you can then manipulate as a js string (e.g. call |.toLowerCase()|).
Checked in one "bustage" fix already[0]. bnesse, could you go over your patch
and make sure there aren't similar cases lurking?

[0]
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/sidebar/resources&command=DIFF_FRAMESET&file=sidebarOverlay.js&rev1=1.83&rev2=1.84&root=/cvsroot

    
    

    
  
See bug 107104 for more "bustage".

    
  
Blocks: 106782

    
    

    
  
I'll review all of the getComplexValue calls now in the tree.

    
    

    
  
Comment on attachment 55814 [details] [diff] [review]
Patch to fix getComplexValue calls (and clean up some warnings)

sr=alecf

        Attachment #55814 -
        Flags: superreview+

    
    

    
  
Comment on attachment 55814 [details] [diff] [review]
Patch to fix getComplexValue calls (and clean up some warnings)

Thanks Brian.  r=sgehani

        Attachment #55814 -
        Flags: review+

    
    

    
  
Checked in.
QA Contact: blakeross → sairuh

    
    

    
  
i don't see this error in the js console [2001.11.28.08-comm, linux] when i
middle-mouse click a link to load in a new tab in the background. marking vrfy'd.
Status: RESOLVED → VERIFIED
Product: Core → SeaMonkey

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: