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

VERIFIED FIXED

Status

Camino Graveyard
Annoyance Blocking
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: froodian (Ian Leue), Assigned: Aaron Schulman)

Tracking

({verified1.8.1})

Details

Attachments

(2 attachments, 2 obsolete attachments)

20.00 KB, application/x-tar
Stuart Morgan
: review+
froodian (Ian Leue)
: review+
Details
3.31 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

11 years ago
-> aschulm
Assignee: nobody → aschulm
(Assignee)

Comment 2

11 years ago
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
Attachment #227341 - Flags: review?(stuart.morgan)
(Assignee)

Comment 3

11 years ago
Created attachment 227342 [details] [diff] [review]
Patch
Attachment #227342 - Flags: review?(stuart.morgan)

Comment 4

11 years ago
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.
Attachment #227342 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 5

11 years ago
Created attachment 227488 [details] [diff] [review]
Patch (Fixed: Images)
Attachment #227342 - Attachment is obsolete: true
Attachment #227488 - Flags: review?(stuart.morgan)

Updated

11 years ago
Attachment #227341 - Flags: review?(stuart.morgan) → review+

Comment 6

11 years ago
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.
Attachment #227488 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 7

11 years ago
Created attachment 228043 [details] [diff] [review]
Patch (Fixed: Comments)
Attachment #227488 - Attachment is obsolete: true
Attachment #228043 - Flags: review?(stuart.morgan)
Blocks: 343938

Comment 8

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

r=me
Attachment #228043 - Flags: review?(stuart.morgan) → review+

Updated

11 years ago
Attachment #228043 - Flags: superreview?
Component: General → Annoyance Blocking
QA Contact: general → annoyance.blocking

Updated

11 years ago
Attachment #228043 - Flags: superreview? → superreview?(mikepinkerton)
(Assignee)

Updated

11 years ago
Attachment #227341 - Flags: superreview?(mikepinkerton)
Comment on attachment 228043 [details] [diff] [review]
Patch (Fixed: Comments)

sr=pink
Attachment #228043 - Flags: superreview?(mikepinkerton) → superreview+
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.
(Reporter)

Comment 11

11 years ago
Comment on attachment 227341 [details]
Modified Nib File

I had to apply the BrowserWrapper.h portion by hand, but the nib works fine.
Attachment #227341 - Flags: superreview?(mikepinkerton) → review+
Whiteboard: [needs checkin]

Comment 12

11 years ago
Fixed trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
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...
Status: RESOLVED → REOPENED
Keywords: fixed1.8.1
Resolution: FIXED → ---
(Reporter)

Comment 14

11 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Reporter)

Updated

11 years ago
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
You need to log in before you can comment on or make changes to this bug.