Last Comment Bug 341974 - pop-up blocker's close button has no mouseover feedback
: pop-up blocker's close button has no mouseover feedback
Status: VERIFIED FIXED
: verified1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Annoyance Blocking (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal (vote)
: ---
Assigned To: Aaron Schulman
:
:
Mentors:
Depends on:
Blocks: 343938
  Show dependency treegraph
 
Reported: 2006-06-18 21:24 PDT by froodian (Ian Leue)
Modified: 2006-09-23 14:38 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Modified Nib File (20.00 KB, application/x-tar)
2006-06-27 19:22 PDT, Aaron Schulman
stuart.morgan+bugzilla: review+
froodian: review+
Details
Patch (4.09 KB, patch)
2006-06-27 19:23 PDT, Aaron Schulman
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (Fixed: Images) (3.54 KB, patch)
2006-06-28 18:52 PDT, Aaron Schulman
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (Fixed: Comments) (3.31 KB, patch)
2006-07-04 07:43 PDT, Aaron Schulman
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image froodian (Ian Leue) 2006-06-18 21:24:58 PDT
The close button in the popup blocker should get the mouseover enhancements brought on by bug 235864 and its followup, bug 337802.
Comment 1 User image Chris Lawson (gone) 2006-06-19 15:46:03 PDT
-> aschulm
Comment 2 User image Aaron Schulman 2006-06-27 19:22:30 PDT
Created attachment 227341 [details]
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 User image Aaron Schulman 2006-06-27 19:23:15 PDT
Created attachment 227342 [details] [diff] [review]
Patch
Comment 4 User image Stuart Morgan 2006-06-27 20:41:02 PDT
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 User image Aaron Schulman 2006-06-28 18:52:17 PDT
Created attachment 227488 [details] [diff] [review]
Patch (Fixed: Images)
Comment 6 User image Stuart Morgan 2006-06-29 20:20:10 PDT
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 User image Aaron Schulman 2006-07-04 07:43:55 PDT
Created attachment 228043 [details] [diff] [review]
Patch (Fixed: Comments)
Comment 8 User image Stuart Morgan 2006-07-11 18:32:49 PDT
Comment on attachment 228043 [details] [diff] [review]
Patch (Fixed: Comments)

r=me
Comment 9 User image Mike Pinkerton (not reading bugmail) 2006-08-02 07:39:02 PDT
Comment on attachment 228043 [details] [diff] [review]
Patch (Fixed: Comments)

sr=pink
Comment 10 User image Mike Pinkerton (not reading bugmail) 2006-08-02 07:49:11 PDT
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 User image froodian (Ian Leue) 2006-08-02 10:14:14 PDT
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 User image Nick Kreeger 2006-08-13 12:16:07 PDT
Fixed trunk and branch.
Comment 13 User image Samuel Sidler (old account; do not CC) 2006-09-21 20:19:06 PDT
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 User image froodian (Ian Leue) 2006-09-21 20:43:00 PDT
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.

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