Closed
Bug 693844
Opened 13 years ago
Closed 13 years ago
Port xpinstallItemGeneric.png file removal from bug 572043 to Modern
Categories
(SeaMonkey :: Themes, defect)
SeaMonkey
Themes
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.7
People
(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)
References
Details
Attachments
(1 file)
3.15 KB,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
Found while working on bug 692765: Bug 572043 removed the xpinstallItemGeneric.png file (cf. bug 572043 comment 18) and let the corresponding chrome path point to the (then) new extensionGeneric.png. Modern missed this change, but we already have the new file so all we have to do is adapt the pointer and remove the old file. Original changesets (winstripe): http://hg.mozilla.org/mozilla-central/rev/e3ccdd93ba10 http://hg.mozilla.org/mozilla-central/rev/83489c0c3e00 Current use: http://mxr.mozilla.org/comm-central/search?string=xpinstallItemGeneric.png
Attachment #566378 -
Flags: review?(philip.chee)
Comment 1•13 years ago
|
||
Comment on attachment 566378 [details] [diff] [review] patch [Checkin: comment 4] There is one reference to chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png in suite/common/bindings/notification.xml KaiRo already noticed this in https://bugzilla.mozilla.org/show_bug.cgi?id=572043#c22 > Don't care about modern, and I actually hate how notification.xml directly > points to an URL without giving themes to point to a possibly > differently-named file. I think we should avoid hard coding image paths in themes (or anywhere else). Looking at the toolkit PopupNotifications.jsm for inspiration: In our notification.xml::addonInstallBlocked(), we can add at the end: var notification = this.appendNotification(messageString, notificationName, iconURL, priority, buttons); notification.setAttribute("class", notificationName); Then in navigator.css near /* ::::: notification popups ::::: */ we can add: .addon-install-blocked { list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png"); } or perhaps: .addon-install-blocked > .notification-inner > hbox > .messageImage { list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png"); } May need !important; to override the default icons in global/notifications.css.
Attachment #566378 -
Flags: review?(philip.chee) → review-
Assignee | ||
Comment 2•13 years ago
|
||
With my patch, notification.xml would be the only consumer left in SM code and since this is a chrome URL, it can easily be translated to some other ultimate file name using jar.mn. So if this needs proper fixing another way I'll leave that to you guys. I'd have to go through several iterations to get it right, which doesn't seem too enjoyable to me.
Assignee: jh → nobody
Status: ASSIGNED → NEW
Comment 3•13 years ago
|
||
Comment on attachment 566378 [details] [diff] [review] patch [Checkin: comment 4] Actually the notification.xml stuff should go in a followup bug. r=me.
Attachment #566378 -
Flags: review- → review+
Assignee | ||
Comment 4•13 years ago
|
||
Comment on attachment 566378 [details] [diff] [review] patch [Checkin: comment 4] http://hg.mozilla.org/comm-central/rev/2d95f8176f0e
Attachment #566378 -
Attachment description: patch → patch [Checkin: comment 4]
Assignee | ||
Comment 5•13 years ago
|
||
Phil, I think you already have a rough understanding of what that f'up should be about. Could you be so kind and create that bug? Thanks.
Assignee: nobody → jh
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.7
Comment 6•13 years ago
|
||
How about "Remove hard coded dependency on xpinstallItemGeneric.png from notification.xml"? And I don't know what's so f**ked up about this??
Assignee | ||
Comment 7•13 years ago
|
||
He, sorry, I meant f'up = follow-up. I'll try to avoid abbreviations next time. ;-)
You need to log in
before you can comment on or make changes to this bug.
Description
•