If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove nsINonBlockingAlertService

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
MathML
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
mozilla1.9.3a5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.)
(Assignee)

Comment 3

8 years ago
(In reply to comment #2)

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

Yeah, pretty much.
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
Created attachment 443021 [details] [diff] [review]
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...
Assignee: nobody → dolske
Attachment #443021 - Flags: superreview?(roc)
Attachment #443021 - Flags: review?(karlt)
(Assignee)

Comment 6

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

And Benjamin for the /embedding bits.
Attachment #443021 - Flags: review?(benjamin)
(Assignee)

Comment 7

8 years ago
(N.B., this interface was originally added back in bug 190307.)
(Assignee)

Updated

8 years ago
Blocks: 562258
(Assignee)

Comment 8

8 years ago
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.
(Assignee)

Updated

8 years ago
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 12

8 years ago
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+
(Assignee)

Comment 13

8 years ago
Created attachment 443495 [details] [diff] [review]
Patch v.2

Updated with nits.
Attachment #443021 - Attachment is obsolete: true
(Assignee)

Comment 14

8 years ago
Created attachment 443514 [details] [diff] [review]
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).
Attachment #443495 - Attachment is obsolete: true
(Assignee)

Comment 15

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/f7a9b2f21b09
Status: NEW → RESOLVED
Last Resolved: 8 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.