341974 - pop-up blocker's close button has no mouseover feedback

Status: VERIFIED FIXED

(Camino Graveyard :: Annoyance Blocking, defect)

Description

[froodian (Ian Leue)]

The close button in the popup blocker should get the mouseover enhancements brought on by bug 235864 and its followup, bug 337802.

Comment 1

[Chris Lawson (gone)]

-> aschulm

Comment 2

[Aaron Schulman]

Attachment: Modified Nib File

Nib Changes:
1. Class of close button changed to RolloverImageButton
2. Link made between close button and BrowserWrapper's mBlockedPopupCloseButton

Comment 3

[Aaron Schulman]

Attachment: Patch

Comment 4

[Stuart Morgan]

Comment on attachment 227342 [details] [diff] [review]
Patch

>+  IBOutlet RolloverImageButton* mBlockedPopupCloseButton; // loaded with mBlockedPopupView, can be nil, STRONG

This shouldn't be a STRONG ref, as I understand it (see below)

>-  // |mBlockedPopupView| has a retain count of 1 when it comes out of the nib,
>+  // |mBlockedPopupView and mBlockedPopupCloseButton| has a retain count of 1 when it comes out of the nib,
>   // we have to release it manually.
>   [mBlockedPopupView release];
>+  [mBlockedPopupCloseButton release];

This is only true for top-level nib items, which the button isn't.  This should crash...

>+    // XXX This is incorrect for the Popup window to depend on BrowserTabViewItem, but we should
>+    // not waste memory loading another copy of the close button images
>+    [mBlockedPopupCloseButton setImage:[BrowserTabViewItem closeIcon]];
>+    [mBlockedPopupCloseButton setAlternateImage:[BrowserTabViewItem closeIconPressed]];
>+    [mBlockedPopupCloseButton setHoverImage:[BrowserTabViewItem closeIconHover]];

NSImage caches named items, so you don't have to worry about the footprint. The file names have tab_ in them, but that's a lot better than calling into BTVI.

Comment 5

[Aaron Schulman]

Attachment: Patch (Fixed: Images)

Comment 6

[Stuart Morgan]

Comment on attachment 227488 [details] [diff] [review]
Patch (Fixed: Images)

Works, but:

>+  IBOutlet RolloverImageButton* mBlockedPopupCloseButton; // loaded with mBlockedPopupView, can be nil, STRONG

With 2 out of three of those comments wrong and the third not necessary, just remove the comment

>+    
>+    // XXX This is incorrect for the Popup window to depend on BrowserTabViewItem, but we should
>+    // not waste memory loading another copy of the close button 

This comment doesn't reflect reality anymore, so remove that as well

>+    

And don't include any whitespace on blank lines, please.

Comment 7

[Aaron Schulman]

Attachment: Patch (Fixed: Comments)

Comment 8

[Stuart Morgan]

Comment on attachment 228043 [details] [diff] [review]
Patch (Fixed: Comments)

r=me

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 228043 [details] [diff] [review]
Patch (Fixed: Comments)

sr=pink

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 227341 [details]
Modified Nib File

not in a position to test this, can someone verify this nib is ok?

rs=pink if someone does.

Comment 11

[froodian (Ian Leue)]

Comment on attachment 227341 [details]
Modified Nib File

I had to apply the BrowserWrapper.h portion by hand, but the nib works fine.

Comment 12

[Nick Kreeger]

Fixed trunk and branch.

Comment 13

[Samuel Sidler (old account; do not CC)]

This bug isn't fixed in the 2006091304 branch build (I know a bit old, but this should have been fixed a while ago).

Reopening...

Comment 14

[froodian (Ian Leue)]

Looks like there was just a missed file in the nib.  I've checked it in, so this should be fixed, but people should keep an eye out for this in tomorrow's branch.