563114 - Remove nsINonBlockingAlertService

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[Frédéric Wang (:fredw)]

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.

Comment 2

[Karl Tomlinson (:karlt)]

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.)

Comment 3

[Justin Dolske [:Dolske]]

(In reply to comment #2)

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

Yeah, pretty much.

Comment 4

[Justin Dolske [:Dolske]]

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.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

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...

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 443021 [details] [diff] [review]
Patch v.1

And Benjamin for the /embedding bits.

Comment 7

[Justin Dolske [:Dolske]]

(N.B., this interface was originally added back in bug 190307.)

Comment 8

[Justin Dolske [:Dolske]]

Oops, that patch should also have included the removal of 1 line in embedding/components/build/nsEmbeddingModule.cpp, which references NS_NONBLOCKINGALERTSERVICE_CONTRACTID.

Comment 9

[Karl Tomlinson (:karlt)]

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?

Comment 10

[Karl Tomlinson (:karlt)]

I mean ... and nsIWindowWatcher.h.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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 12

[Benjamin Smedberg]

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.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updated with nits.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

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).

Comment 15

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/f7a9b2f21b09