Closed Bug 563114 Opened 10 years ago Closed 10 years ago

Remove nsINonBlockingAlertService

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 2 obsolete files)

I'm cleaning up some prompt related code. MathML is the only thing that uses this interface, and I want to remove it.

I think the right solution for what MathML is doing (alerting the user that fonts are missing) is to fire a notification, and let the app's front-end code deal with that however it wants.

The ALERT_MISSING_FONTS #ifdef currently seems to be undefined in both mozilla-central and comm-central, so this code isn't being used anyway in shipping Mozilla products.
Yes, we no longer use this dialog. The proposed replacement is an information bar (bug 309090) but it seems this feature is only specific to Firefox.
Yes, don't let the dead MathML code get in the way of what you want to do.

(In reply to comment #0)
> I think the right solution for what MathML is doing (alerting the user that
> fonts are missing) is to fire a notification, and let the app's front-end code
> deal with that however it wants.

Do you mean something similar to the "PluginCrashed" event?
http://hg.mozilla.org/mozilla-central/annotate/67b93e5e8f5d/content/base/src/nsObjectLoadingContent.cpp#l276

(I'm not sure whether MathML is really a special case.  It's mostly just another lack-of-font-support situation, except that in MathML we know which fonts are good.  Anyway, for MathML, efforts like bug 414277 to do our best with the fonts we have are probably higher priority.)
(In reply to comment #2)

> Do you mean something similar to the "PluginCrashed" event?

Yeah, pretty much.
CCing a couple of Camino folks, since this (upcoming) patch potentially breaks them on trunk. Camino MXR shows them as implementing the interface being removed, but they don't seem to define ALERT_MISSING_FONTS, so that's probably dead code for them anyway.
Attached patch Patch v.1 (obsolete) — Splinter Review
Removes the interface and MathML's call to it. I could have nuked the rest of the code in AlertMissingFonts(), but it's already dead. Don't care either way...
Assignee: nobody → dolske
Attachment #443021 - Flags: superreview?(roc)
Attachment #443021 - Flags: review?(karlt)
Comment on attachment 443021 [details] [diff] [review]
Patch v.1

And Benjamin for the /embedding bits.
Attachment #443021 - Flags: review?(benjamin)
(N.B., this interface was originally added back in bug 190307.)
Blocks: 562258
Oops, that patch should also have included the removal of 1 line in embedding/components/build/nsEmbeddingModule.cpp, which references NS_NONBLOCKINGALERTSERVICE_CONTRACTID.
Attachment #443021 - Flags: superreview?(roc) → superreview+
Comment on attachment 443021 [details] [diff] [review]
Patch v.1

r=karlt to mean that MathML shouldn't be using this, so there's no need to keep the interface for MathML.

Can you remove the nsIDOMWindow.h and nsIDOMWindow.h includes too, please?
Attachment #443021 - Flags: review?(karlt) → review+
I mean ... and nsIWindowWatcher.h.
Blocks: 563274
No longer blocks: 562258
Thanks for the heads-up, dolske.

That said, I haven't seen the "missing fonts" alert since karlt rewrote MathML for Cairo/Thebes back in 1.9.0 (and I know I don't have any special math fonts installed; 1.8.1-based Caminos still prompt me). My guess from quick MXRing is that the "dead" ALERT_MISSING_FONTS was inserted to stop the mandatory alerting now that out-of-the-box Unicode fonts would work for MathML (bug 400938).

If the event-firing ends up happening in some bug other than bug 309090 (to which I've already cc'd myself), would someone please cc me there?  Thanks. :-)
Comment on attachment 443021 [details] [diff] [review]
Patch v.1

We should probably come up with a better API for firing/handling notification-bar type alerts, but that's definitely a different bug.
Attachment #443021 - Flags: review?(benjamin) → review+
Attached patch Patch v.2 (obsolete) — Splinter Review
Updated with nits.
Attachment #443021 - Attachment is obsolete: true
Attached patch Patch v.3Splinter Review
Bah, somehow I missed changing a NS_IMPL_ISUPPORTS4() to ...3 in posted patches. This update really includes it, builds from a full clobber on Windows, and pwmgr prompt tests pass (just to be sure).
Attachment #443495 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/f7a9b2f21b09
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.