Closed Bug 693844 Opened 11 years ago Closed 11 years ago

Port xpinstallItemGeneric.png file removal from bug 572043 to Modern

Categories

(SeaMonkey :: Themes, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.7

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

References

Details

Attachments

(1 file)

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 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-
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 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+
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]
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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.7
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??
He, sorry, I meant f'up = follow-up. I'll try to avoid abbreviations next time. ;-)
Blocks: 694786
You need to log in before you can comment on or make changes to this bug.