Closed
Bug 563114
Opened 15 years ago
Closed 15 years ago
Remove nsINonBlockingAlertService
Categories
(Core :: MathML, defect)
Core
MathML
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: Dolske, Assigned: Dolske)
References
Details
Attachments
(1 file, 2 obsolete files)
11.73 KB,
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
(In reply to comment #2)
> Do you mean something similar to the "PluginCrashed" event?
Yeah, pretty much.
Assignee | ||
Comment 4•15 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•15 years ago
|
||
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•15 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•15 years ago
|
||
(N.B., this interface was originally added back in bug 190307.)
Assignee | ||
Comment 8•15 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 9•15 years ago
|
||
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+
Comment 10•15 years ago
|
||
I mean ... and nsIWindowWatcher.h.
Assignee | ||
Updated•15 years ago
|
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•15 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•15 years ago
|
||
Updated with nits.
Attachment #443021 -
Attachment is obsolete: true
Assignee | ||
Comment 14•15 years ago
|
||
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•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 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.
Description
•