Closed Bug 190307 Opened 22 years ago Closed 19 years ago

Camino does not support MathML

Categories

(Camino Graveyard :: Page Layout, enhancement, P2)

PowerPC
macOS
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: distler, Assigned: makotoy)

References

()

Details

(Keywords: fixed1.8.0.2, fixed1.8.1, Whiteboard: [camino-1.0])

Attachments

(6 files, 15 obsolete files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
rbs
: review-
Details | Diff | Splinter Review
(deleted), text/plain
sfraser_bugs
: review+
mikepinkerton
: superreview+
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030117 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021104 Chimera When this page is viewed as text/xml, Chimera's XML parser chokes. Under the same conditions, Mozilla renders the page just fine. The page is definitely valid XHTML + MathML (according to the W3C validator). And Mozilla's XML parser has no problem with it. Reproducible: Always Steps to Reproduce: 1. 2. 3. Actual Results: XML Parsing Error: undefined entity Location: http://golem.ph.utexas.edu/~distler/blog/ Line Number 73, Column 1: /span/td Expected Results: Render the page.
So as not to cut off Chimera users from my site, I am going to serve the above web page as "text/html" (which Chimera can render OK). Why don't you use http://www.mozilla.org/projects/mathml/demo/texvsmml.xml as a test case instead (or, really, any XML page -- I haven't found one that rendered correctly in Chimera).
I think the problem is Chimera doesn't yet support MathML. Chimera does properly parse and display other xml and xhtml files. http://mozilla.org/quality/browser/standards/xhtml/transitional/index.html http://mozilla.org/quality/browser/standards/xml/index.html
Status: UNCONFIRMED → NEW
Ever confirmed: true
In Mozilla, those pages get parsed by the "tag soup" HTML parser, not the XML parser. But, yes, Chimera does render pages like http://mozilla.org/quality/browser/standards/xml/entity_references.xml which, in Mozilla, are parsed by the XML parser. (I don't know how to check what parser is being used by Chimera, so I am using Mozilla as a guide to what is going on -- I don't know if that is really reliable.) It may be that this is a "we don't support MathML" problem, rather than a broken XML parser problem. If that's correct (ie, if Chimera really is rendering the above page with the XML parser), then perhaps this bug should be renamed to "Chimera does not support MathML".
Chimera uses the expat XML parser (same as Mozilla) except Chimera doesn't have all the additional checkins that the Mozilla trunk gets. So this initial problem is caused by not having support for MathML in Chimera.
Summary: Chimera's XML Parser returns error; Mozilla says A-OK → Chimera does not support MathML
There are two issues here: 1) Camino doesn't have the DTD where the MathML entities are defined. (In general, you should not rely on non-validating parsers being able to do something useful with stuff defined in an external DTD subset.) 2) The MathML support isn't enabled in Camino. Fixing this would be easy: 1) add --enable-mathml to configure options 2) Repeat the fix that was done in bug 158222 for xhtml11.dtd for mathml.dtd. 3) As polish bonus, hack the MathML font sniffing code to only alert about the Wolfram fonts. Installing the TeX fonts on OS X is a bad idea.
Do you need to change the font-sniffing code, or just set the default value for font.mathfont-family preference to: user_pref("font.mathfont-family", "Math1, Math2, Math4");
Here a patch that adds the MathML files to the MachO embedding configuration, as well as updating the ProjectBuilder project. Note that .mozconfig will need the --disable-mathml flag removed for this to build properly. I've done the changes for both static and dynamic builds, and they seem to be fine. The sample MathML seems to render fine, but the font warning window shows up as a blank window with two blank buttons. The code for that is in mozilla/layout/mathml/base/src/ nsMathMLChar.cpp, and I'm trying to figure out how to fix it at the moment.
OK, it turns out that the MathML AlertMissingFonts() code was manually building the warning dialog box, rather than using nsIPrompt. With the new patch below, it shows a proper alert sheet under OSX.
re: comment #9 It does that for a reason. To make it non-modal (i.e., non-blocking). Later on, a click link can be supported so that the user can go to a page to (auto)download the fonts.
Hmm. Thats fair enough, I guess. On OSX though, the dialog is a sheet, so it is only window modal. A workable (although perhaps not very elegant) solution would be to #ifdef this for XP_MACOSX. I'm wondering what the 'correct' solution is. Building up a window by hand looks like the sort of job that should only be done once. Should something like NonmodalAlert(...) be added to nsIPrompt? Or is there a another suitable method elsewhere? (Apologies for the mozilla newbie question)
The most "suitable" method (at the time) was what that code is doing... Your best bet might be to figure out how to make it work on Chimera. It is a public API and should be working.
Coimment #0 I don't see any problem on given URL with Build ID 2003082402 . Reported could you update issue with a nightly build test ?
Sigh. Please read comment #1. *Nothing* has changed with the status of this bug. All versions of Camino (including the latest nightly build) choke on MathML content.
*** Bug 224028 has been marked as a duplicate of this bug. ***
updating summary. Having a better URL would be great.
Summary: Chimera does not support MathML → [patch] Camino does not support MathML
Comment on attachment 124415 [details] [diff] [review] MathML patch, including updated nsIPrompt code This patch is not acceptable in its present form. The non-modal behavior is intentional, and a correct fix would be to get a similar behavior on Camino.
Attachment #124415 - Flags: review-
Summary: [patch] Camino does not support MathML → Camino does not support MathML
rbs: Isn't it more important to get MathML support enabled in Camino even without the dialog? Assuming what no one is going to write the non-modal dialog in the 0.8 timeframe, could we, please, get MathML enabled with the font issue "fixed" in documentation? I volunteer to write the relevant pieces of documentation for the Readme file and the Web site. I realize the dialog helps people who might stumble upon MathML content without the intent to browse MathML pages specifically. However, the current situation helps neither them nor the users who would read the docs.
>I realize the dialog helps people who might stumble upon MathML content without >the intent to browse MathML pages specifically. Actually, it is not a matter of stumbling per chance on MathML. It helps many people who purposely try out Mozilla/Netscape just because they heard that it has native MathML support. Such people are not geeks, and are quick to blame the open source model when something isn't right -- although part of the blame is that they don't have the necessary fonts (which is what the dialog is about). >However, the current situation helps neither them nor the users who would read >the docs. It currently affects only Camino. My problem is that I didn't see an attempt for the right fix (before conceding that it is hard). Just the quick fix. However, in regard to the 0.8 timeframe and pending a better fix, I wouldn't mind a temporary #ifdef (as comment 12) #ifded MACOS modal version #else non-modal version #endif
Please update, is the patch working?
Please update.
Severity: major → enhancement
(In reply to comment #22) > Please update. Based on a quick look, this patch can no longer be applied to the Camino project; it patches the old .pbproj instead of the new .xcode. In addition to addressing the comments from the reviewer, that will need to be changed before we can revisit this.
Why exactly is a Window-modal dialog box (sheet, actually) such a terrible thing? Clearly an (Application-)modal dialog box would be undesirable, but a Window-modal one does not seem terribly inappropriate. MathML support in Camino has been held up for two years (at least) because the "Missing Fonts" dialog box is Window-modal, rather than strictly non-modal?
The dialog can be turned off by a simple pref as you noted yourself in comment 6. That said, where do you get the data to back your comment 24. Comment 20 clearly said that the non-modal version isn't the one and only way, so long as Camino's weaknesses are not imposed on other platforms that are doing just fine. That principle wouldn't make sense. It is more effective on the whole if patches address review comments, when they are even there.
I was arguing in *favour* of something like the #ifdef in Comment 20. Unless I misunderstand, this produces a Window-modal dialog (Comment 12) in Camino and the standard non-modal dialog elsewhere. I think a Window-modal dialog is perfectly appropriate (on platforms that support it). So long as one not create an Application-modal (blocking) behaviour on other platforms (including, say, Firefox/Mac), what would the objection be?
What is needed is somebody to document the problem (why is modal non working) and then drives the patch they come up with. It could be that it can be made non-modal with a little extra. It could be that it can't be made non-modal. The lack of investigation just looks bad. Bug 130023 has a patch to make the link clickable (which only works in non-modal).
Speak up those who would like to see MathML in Camino 1.0. Patch needs refreshing too.
Priority: -- → P2
Target Milestone: --- → Camino1.0
I would but, then, I filed the bug.
I would, also.
I too would like to see MathML support.
(In reply to comment #31) > I too would like to see MathML support. I will stick with Firefox on Mac OS X BECAUSE it supports Mathml. I will consider switching to Camino when it can render the mathml torture page at http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml
Note that we need to package fontEncoding.properties when we turn on MathML.
Attached patch enables MathML in Camino (obsolete) (deleted) — Splinter Review
This patch turns on the MathML functionality on Camino. There remains the missing font alert dialog broken. It appears with the title "chrome://global/content/commonDialog.xul", blank content and two buttons with no label. Should we address it here? For reviewers: the change to Camino.xcode/project.pbxproj consists of libucvmath.dylib, mathml.dtd, mathml.css fontEncoding.properties and mathfont*.properties added to the Cmino target copy libucvmath.dylib (along with the other dylibs) into compnents/ of the Dest. Executables copy mathml.dtd (along with xhtml11.dtd) into res/dtd/ copy fontEncoding.properties and mathfont*.propeties into res/fonts/
Attachment #201115 - Flags: superreview?(bryner)
Attachment #201115 - Flags: review?(bryner)
Comment on attachment 201115 [details] [diff] [review] enables MathML in Camino I'll look this over.
Attachment #201115 - Flags: review?(bryner) → review?(mark)
(In reply to comment #34) > Created an attachment (id=201115) [edit] > enables MathML in Camino > > This patch turns on the MathML functionality on Camino. > There remains the missing font alert dialog broken. It appears with the title > "chrome://global/content/commonDialog.xul", blank content and two buttons with > no label. Should we address it here? Who's trying to throw XUL dialogs? This needs to be embedder-friendly, so has to use embedding APIs (nsIPrompt etc).
> Who's trying to throw XUL dialogs? Looks like AlertMissingFonts() at http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsMathMLChar.cpp#150 rbs, does that do anything that cannot be done via prompt() or alert() on the prompt service?
For what it's worth, looking at this dialog it looks like it can be a simple alert() call....
Comment on attachment 201115 [details] [diff] [review] enables MathML in Camino Please get signoff on this from pinkerton; as I remember it, the decision to not build MathML was a conscious one to reduce bloat.
Attachment #201115 - Flags: superreview?(bryner)
The point of the gneuflexions currently in AlertMissingFonts() is to make the dialog non-modal (i.e., non-blocking). This way: - it does not stop the page from displaying half-way -- until the frigthened user clicks OK. The page continues to display and is gentle to the user. - it does not defeat the next step, which is to utilmately make the "Go Get MathML Fonts" clickable as per bug 130023, which is not possible with the prompt() or alert() (at least to my knowledge). So, seems that something has to be sacrificed to make it embedder-friendly, then.
> - it does not stop the page from displaying half-way Hmm... This is a problem. I don't think we have an embedding api for posing a non-modal dialog... Doing it modal might be bad depending on when it'd come up; e.g. posing a modal dialog from the middle of reflow would be a really bad idea. > - it does not defeat the next step, which is to utilmately make the "Go Get > MathML Fonts" clickable This can be worked around with the embedding stuff, but not really cleanly.
We have a request for non-blocking (or at least tab-modal) alerts in bug 123913 :)
tab-modal would also be unacceptable here. This code needs a "control returns to me immediately, before any painting happens" kinda alert.
Can we throw something like the About dialogue? Or even just a popup window (that's what the default plugin does--still pointing to Netscape, no less), which is almost what the dialogue looks like in Fx. An app-modal dialogue--at least the ones we throw for quitting with tabs open or asking to set the default browser at first launch--doesn't stop page load, so if that's the main concern, app-modal could work.
> An app-modal dialogue--at least the ones we throw for quitting with tabs open > or asking to set the default browser at first launch--doesn't stop page load, But does it return immediately (that is, while the dialog is still open)? The main concern I have is not so much stopping page load as reentering layout code that is not reentrant.
(In reply to comment #45) > > An app-modal dialogue--at least the ones we throw for quitting with tabs open > > or asking to set the default browser at first launch--doesn't stop page load, But it is modal (as far as Cocoa is concerned). Not that Cocoa doesn't like you running app-modal sheets, which is why there's a complaint on the console. It's a hack. > But does it return immediately (that is, while the dialog is still open)? No, they'll block, but I guess PLEvents are getting through. The > main concern I have is not so much stopping page load as reentering layout code > that is not reentrant. We already do this with our window-modal sheets and, predictably, it causes problems (see bug 179307).
*** Bug 315751 has been marked as a duplicate of this bug. ***
rbs, what's a good value for font.mathfont-family on Mac OS X? The font installer linked from http://www.mozilla.org/projects/mathml/fonts/ doesn't include the TeX fonts, which I understand don't work on OS X (bug 161137). With the fonts from the installer, with or without disabling the warning by setting font.mathfont-family, many non-contrived examples in the MathML torture test at http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml still render improperly. If this flat-out isn't going to work on OS X because we can't use the TeX fonts, then it doesn't really matter what the dialog says or does or how it's presented. If we're going to do this, I'd like to make things as seamless and correct as possible "out of the box" for the largest possible number of users. I'd like to do it if it can be done properly.
Actually, I seem to get the best results "out of the box" without installing any extra fonts at all. Is it rational on OS X (or at least in Camino) to set font.mathfont-family to something that bypasses the font warning altogether? That makes the prompt issue almost moot. The most major issue I'm seeing is black space beneath radicals, a la attachment 197497 [details] (from bug 228804). There's also a lack of horizontal stretchiness in the braces (torture cases 19 and 22). This is certainly better than the incorrect glyphs that are used when the math fonts are installed.
>Actually, I seem to get the best results "out of the box" without installing >any extra fonts at all. This can only happen on basic pages, and it has *started* to happen on the Mac OS X precisely because the Symbol font has recently been fixed (bug 213702). I always remind that just like the letter "a" can be found in several fonts, so too can some of the special math characters be found in the Symbol font. And since the Symbol font is almost present on all platforms, it is a very valuable fallback for the MathML engine by design, giving the users something not that bad "out of the box", so that they feel encourage to take the next daunting step of installing other fonts. So the missing font dialog does not mean that the world has collapsed... But the Symbol font doesn't cover the sqrt and other characters, and that's where you start seeing those strange things. It has now been reported that the Type1 versions (as opposed to the TrueType versions) of the TeX fonts _just_ work out-of-the-box too. See bug 161137 comment #47 (and bug 161137 comment #50) So it might still be premature to shut them out. The missing font dialog has assisted early adopters. But as more people have now hooked it, we could indeed set the pref to something else for the maintstream, once the various reports are confirmed or denied. It should be remembered that it is a warning, and that just like the letter "a" can be found in several fonts, etc. To drop the lookup of TeX fonts, the pref is: user_pref("font.mathfont-family", "Math1, Math2, Math4, Symbol"); Note that should we hard-code the pref prematurely, we wouldn't have the investigation that has led to the recent fixes and further discoveries.
rbs, when I install the math fonts using the Mac math font installer at http://www.mozilla.org/projects/mathml/fonts/ (which, in turn, is suggested by the "your fonts are missing" prompt), MathML rendering is useless. That's where my motivation to suppress that warning altogether came from.
(In reply to comment #51) Now that there's a CM font (texture-type1) for gfxmac module and MathML is "supported" on Camino, what's happening is an inconsistent instruction at the font page. I don't think this is a Camino's fault and makes a reason for dropping the font alert.
So, based on the above comments, what are the chances of getting MathML in Camino 1.0? Mark, Simon? If this isn't something that's practical now, should we push it off to 1.1?
Omitted the unnecessary change for basebrowser-mac-macho (or do we need this to put the files in "Embed" dir?) and implemented non-blocking missing font alert. I couldn't get Camino.xcode diffed (I got "? Camino.xcode", why?): I did 1) add libucvmath.dylib, mathml.css, mathml.dtd, mathfont.properties & co. to the project 2) put libucvmath.dylib to the second "Copy Files" build phase for target "Camino" 3) same for mathml.css to the third (#34 desc missed this while the patch did certainly this, sorry) and mathml.dtd to the fourth 4) Create new "Copy Files" build phase for "Camino", with dest "Executables" subdir "res/fonts" with the files which should go there
Attachment #201115 - Attachment is obsolete: true
Attachment #201115 - Flags: review?(mark)
Omitted the unnecessary change for basebrowser-mac-macho (or do we need this to put the files in "Embed" dir?) and implemented non-blocking missing font alert. I couldn't get Camino.xcode diffed (I got "? Camino.xcode", why?): I did 1) add libucvmath.dylib, mathml.css, mathml.dtd, mathfont.properties & co. to the project 2) put libucvmath.dylib to the second "Copy Files" build phase for target "Camino" 3) same for mathml.css to the third (#34 desc missed this while the patch did certainly this, sorry) and mathml.dtd to the fourth 4) Create new "Copy Files" build phase for "Camino", with dest "Executables" subdir "res/fonts" with the files which should go there
Attachment #205329 - Attachment is obsolete: true
Attached file nsMathMLChar - Cocoa bridge (obsolete) (deleted) —
This throws a nonblocking window made from the alert panel content. Put this in layout/mathml/base/src. It's easy to implement "Go Get Fonts" feature with the commented out codes + a simple delegate object, but this is this....
Comment on attachment 205330 [details] [diff] [review] turn MathML for Camino, implements the missing font alert dialog I really don't think we want to add linkage requiresments with Cocoa to layout. You need to hide this behind an embedding-friendly IDL interface (like nsIPrompt).
Attachment #205330 - Flags: review-
Comment on attachment 205331 [details] nsMathMLChar - Cocoa bridge > NSPanel* alertPanel = NSGetInformationalAlertPanel( > titleStr, > msgStr, > nil, // the first button > nil, nil // no second & third buttons > ); > alertView = [alertPanel contentView]; > alertWindow = [[NSWindow alloc] initWithContentRect:[alertView bounds] > styleMask:NSTitledWindowMask | NSClosableWindowMask > backing:NSBackingStoreBuffered > defer:YES]; > [alertWindow setTitle:titleStr]; > [alertWindow setContentView:alertView]; > [alertWindow center]; > [alertWindow makeKeyAndOrderFront:nil]; // alertHandler]; Ripping the content view out of an alert panel, and sticking it into an NSWindow scares me.
Attachment #205331 - Flags: review-
Attached patch Use common facility for missing fonts dialog (obsolete) (deleted) — Splinter Review
I've finally managed to make OpenWindow work... The alert load is slow, but it goes as intended.
Attachment #205330 - Attachment is obsolete: true
Attachment #205331 - Attachment is obsolete: true
Er.... the change to nsGlobalWindow in that last patch is not acceptable. It's very likely to cause security holes, if nothing else.
I really don't think we should be opening XUL windows in Camino. AFAIK, this is the ONLY code that will ever invoke a top-level XUL window in Camino, so you're bringing in lots of widget code that is totally untested (like nsCocoaWindow). As I said before, you need to add an embedding-friendly nsIPromptService-like API. Using nsPIPromptService is not appropriate; this is an API used internally by the window watcher.
Target Milestone: Camino1.0 → Camino1.1
Attached patch make a new "nonblocking alert" interface (obsolete) (deleted) — Splinter Review
The patch implemnts alert panel for a new interface nsINBAlertService for non-blocking alerts.
Attachment #205547 - Attachment is obsolete: true
Attached file new xpidl for the nonblocking alert service interface (obsolete) (deleted) —
place this in embedding/components/windowwatcher/public/
Comment on attachment 206390 [details] new xpidl for the nonblocking alert service interface >#include "nsISupports.idl" >interface nsIDOMWindow; >interface nsIDialogParamBlock; > >[uuid(E7A87451-6ED3-11DA-A42F-00039386357A)] >interface nsINBAlertService : nsISupports Can we spell this out please? nsINonBlockingAlertService ? Is it really a service (in the XPCOM sense, i.e. a singleton)? Might be better to just call it nsINonBlockingAlert. >{ > // Throws a non blocking alert > // @param aWindow > // The window relevant to the alert Say that this is the "parent" window. > // @param aDParam > // The dialog parameter block specifying the content of the alert. > void throwNBAlert(in nsIDOMWindow aWindow, in nsIDialogParamBlock aDParam); I don't like the word "throw" here; I think Show would be fine. And since the interface is called nsINonBlockingAlert, do we have to call this method ShowNonBlockingAlert(), or is ShowAlert() sufficient?
Comment on attachment 206389 [details] [diff] [review] make a new "nonblocking alert" interface >Index: camino/Camino.xcode/project.pbxproj >+ 0E94133A094C5B1C00434D81 = { >+ fileEncoding = 30; >+ isa = PBXFileReference; >+ lastKnownFileType = text.xml; >+ name = mathml.dtd; >+ path = /Users/makoto/Development/mozilla/cmndev/dist/Embed/res/dtd/mathml.dtd; >+ refType = 0; >+ sourceTree = "<absolute>"; You have a full path here. >+ ); >+ isa = PBXGroup; >+ name = fonts; >+ path = /Users/makoto/Development/mozilla/cmndev/dist/Embed/res/fonts; >+ refType = 0; >+ sourceTree = "<absolute>"; And here. >Index: camino/src/browser/CocoaPromptService.mm >+// void ThrowNBAlert(nsIDOMWindow *aWindow, nsIDialogParamBlock *aDParam); >+NS_IMETHODIMP >+CocoaPromptService::ThrowNBAlert(nsIDOMWindow *aWindow, nsIDialogParamBlock *aDParam) >+{ >+ nsAlertController* controller = CHBrowserService::GetAlertController(); >+ PRUnichar* titlePtr; >+ PRUnichar* bodyPtr; >+ aDParam->GetString(nsPIPromptService::eDialogTitle, &titlePtr); >+ aDParam->GetString(nsPIPromptService::eMsg, &bodyPtr); You're leaking these two PRUnichar*. Use nsXPIDLString to avoid this. >+ NSString* titleStr = [NSString stringWithPRUnichars:titlePtr]; >+ NSString* msgStr = [NSString stringWithPRUnichars:bodyPtr]; >+ CHBrowserView* browserView = [CHBrowserView browserViewFromDOMWindow:aWindow]; >+ NSBeginInformationalAlertSheet(titleStr, >+ @"OK", nil, nil, // only one button >+ [browserView getNativeWindow], >+ controller, // delegate >+ @selector(alertDidEnd:rCode:cInfo:), Please don't abbreviate unnecessarily. We don't do this elsewhere. "alertDidEnd:returnCode:contextInfo:". Since the end sheet method does nothing, you might want to just pass NULL. Don't you need an implementation of nsINBAlertService somewhere that throws XUL dialogs? Regarding last comment about "nsINBAlertService" being called a "service"; since nsIPromptService is similarly named, this is probably OK.
> Don't you need an implementation of nsINBAlertService somewhere that throws XUL > dialogs? You mean to hide that OpenWindow call from nsMathMLChar.cpp by implementing nsINonBlockingAlertService somewhere for Firefox/Seamonkey? I'm not sure where to do this. Make new nsNonblockingService.* in embedding/components/windowwatcher/src? or just add ShowNonBlocking... to nsWindowWatcher.cpp?
Attachment #206389 - Attachment is obsolete: true
Attached file renamed xpidl for the nonblocking alert service (obsolete) (deleted) —
Attachment #206390 - Attachment is obsolete: true
(In reply to comment #66) > Created an attachment (id=206542) [edit] > make a new "nonblocking alert" interface, refined > > > Don't you need an implementation of nsINBAlertService somewhere that throws XUL > > dialogs? > You mean to hide that OpenWindow call from nsMathMLChar.cpp by implementing > nsINonBlockingAlertService somewhere for Firefox/Seamonkey? Yes. The MathML code should not have 2 code paths. It should just have one (using nsINBAlertService). > I'm not sure where > to do this. Make new nsNonblockingService.* in > embedding/components/windowwatcher/src? Yes. You could probably just make nsPromptService also implement nsINonBlockingAlertService.
now nsMathMLChar.cpp is free of direct UI manipulation.
Attachment #206542 - Attachment is obsolete: true
Comment on attachment 206594 [details] [diff] [review] adds nonblocking alert implementation for nsPromptService.cpp Excellent.
Attachment #206594 - Flags: review+
Attachment #206594 - Flags: superreview?(mikepinkerton)
Comment on attachment 206544 [details] renamed xpidl for the nonblocking alert service Please use the javadoc comment syntax (with /** at the start, etc). Please document the interface as a whole, not just the method. Also should aWindow be aParentWindow? Is it allowed to be non-null? What does "non blocking" mean in this context? What is and is not allowed in the aDParam block? All of this should be documented. If some of this is the same as the alert() method on nsIPromptService, say so.
Added documentation. In the course I've realized that DialogParamBlock is too much for our purpose and rewritten the interface. we are using nothing other than title/msg bogy on Camino...
Attachment #206544 - Attachment is obsolete: true
Attached patch adapt to the new nsINonBlockingAlertService (obsolete) (deleted) — Splinter Review
... so that nsMathMLChar.cpp:AlertMissingFonts is now completely free from UI specific things.
Attachment #206594 - Attachment is obsolete: true
Attachment #206594 - Flags: superreview?(mikepinkerton)
Attachment #206749 - Flags: review?(sfraser_bugs)
Attachment #206750 - Flags: review?(sfraser_bugs)
Attachment #206749 - Flags: review?(sfraser_bugs) → review+
Comment on attachment 206750 [details] [diff] [review] adapt to the new nsINonBlockingAlertService > Index: layout/mathml/base/src/nsMathMLChar.cpp > =================================================================== > + nsresult rv; > + nsCOMPtr<nsINonBlockingAlertService> prompter = > + do_GetService("@mozilla.org/embedcomp/nbalert-service;1", &rv); > > - nsCOMPtr<nsIDOMWindow> dialog; > - wwatch->OpenWindow(parent, "chrome://global/content/commonDialog.xul", "_blank", > - "dependent,centerscreen,chrome,titlebar", paramBlock, > - getter_AddRefs(dialog)); > + if (prompter && parent) { > + prompter->ShowNonBlockingAlert(parent, title.get(), message.get()); > + } Nit: you ignore rv here, so you might as well just remove it.
Attachment #206750 - Flags: review?(sfraser_bugs) → review+
nsresult nsPromptService::DoDialog(nsIDOMWindow *aParent, nsIDialogParamBlock *aParamBlock, const char *aChromeURL) { ... if (!mWatcher) return NS_ERROR_FAILURE; ============= +nsPromptService::ShowNonBlockingAlert(nsIDOMWindow *aParent, + const PRUnichar *aDialogTitle, + const PRUnichar *aText) +{ + nsCOMPtr<nsIWindowWatcher> wwatch(do_GetService(NS_WINDOWWATCHER_CONTRACTID)); + if (!wwatch) Seems you could use the cached |mWatcher| too?
Attached patch Utilizes mWatcher (obsolete) (deleted) — Splinter Review
fixed that.
Attachment #206750 - Attachment is obsolete: true
You forgot the null check.
Attached patch with null check (obsolete) (deleted) — Splinter Review
oops... Removed unnecessary #include's from nsMathMLChar.cpp too.
Attachment #206971 - Attachment is obsolete: true
Attachment #206987 - Flags: review?(sfraser_bugs)
(In reply to comment #32) > (In reply to comment #31) > > I too would like to see MathML support. > > I will stick with Firefox on Mac OS X BECAUSE it supports Mathml. > I will consider switching to Camino when it can render the mathml torture page at > http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml > (In reply to comment #32) > (In reply to comment #31) > > I too would like to see MathML support. > > I will stick with Firefox on Mac OS X BECAUSE it supports Mathml. > I will consider switching to Camino when it can render the mathml torture page at > http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml > Camino 1.0 Beta 2 does NOT work on above link. It would be very nice if MATHML DID work for Camino 1.0.
Attached patch Update to cope with the tree change (obsolete) (deleted) — Splinter Review
The patch became inapplicable because Camino.xcode was changed.
Attachment #206987 - Attachment is obsolete: true
Attachment #206987 - Flags: review?(sfraser_bugs)
Attachment #208844 - Flags: review?(sfraser_bugs)
You have a full path in the project file: + path = /Users/makoto/Development/mozilla/cmndev/dist/Embed/res/fonts;
Oops, I did that again, and it contained an unnecessary change to CocoaEmbed.pbproj... won't happen again :(
Attachment #208844 - Attachment is obsolete: true
Attachment #208914 - Flags: review?
Attachment #208844 - Flags: review?(sfraser_bugs)
Attachment #208914 - Flags: review? → review?(sfraser_bugs)
Attachment #208914 - Flags: review?(sfraser_bugs) → review+
Attachment #208914 - Flags: superreview?(mikepinkerton)
Attachment #206749 - Flags: superreview?(mikepinkerton)
Comment on attachment 208914 [details] [diff] [review] Fixes absolute path, unnecessary change to the test sr=pink
Attachment #208914 - Flags: superreview?(mikepinkerton) → superreview+
Comment on attachment 206749 [details] nonblocking service idl with documentation sr=pink
Attachment #206749 - Flags: superreview?(mikepinkerton) → superreview+
if we can get this in for 1.0, we might as well. what kind of risks does the non-blocking dialog pose? any? does this work correctly in static builds? we should try to get this on the 180branch as soon as possible, though we'll have to maxi-branch to get this for camino. wheeeeee.
(In reply to comment #85) > if we can get this in for 1.0, we might as well. what kind of risks does the > non-blocking dialog pose? any? > does this work correctly in static builds? I'm not familiar with risk estimation but the code path is one-way simple and just a modularization of the old alert code in nsMathMLChar.cpp. The patch introduces one "return;" in nsPromptService.cpp which should be "return NS_ERROR_FAILURE;" and causing error for gcc 4.0. Could you fix that before check-in? Then it will go ok with both gcc3.3/4.0 and shared/static, fx/camino. CocoaPromptServices.cpp is modified recently. Should I submit the updated patch?
If your patch doesn't apply cleanly now, I'd appreciate an updated version. I'll shoot for a checkin on Tuesday or Wednesday.
Attached patch update for tree change (deleted) — Splinter Review
Apparently I shouldn't upgrade to XCode 2.2... Could someone w/ v1.5 or 2.0 do the following for camino/Camino.xcode and submit the patch please? (before that you should apply this patch and build the tree once) Add libucvmath.dylib from dist/Embed/components to Gecko/Gecko Components (ref style is rel. to proj.) Add mathml.css from Embed/res to Gecko/Gecko Res (rel. to proj) Add mathml.dtd from res/dtd to Gecko Res/dtd (rel to the enclosing group) Create a new group "fonts" inside Gecko Res Set the path of fonts to Embed/res/fonts (rel. to the proj) Add the files in Embed/res/fonts to the fonts group (rel. to the enclosing grp) Add libucvmath.dylib to the second copy phase in the target Camino Add mathml.css to the third copy phase Add mathml.dtd to the fourth Create a new copy files phase at the fifth place Set its destination to "Executable" (from the popup) and "res/fonts" (the field below) Add the files inside fonts group to this phase
Attachment #208914 - Attachment is obsolete: true
Makoto, you're introducing a new file (nsINonBlockingAlertService.idl) that doesn't include the tri-license boilerplate. See http://www.mozilla.org/MPL/boilerplate-1.1/ . Please let me know what text you want to appear in the file either by attaching an updated .idl file or telling me how you want the boilerplate's blanks filled in on checkin.
Assignee: bryner → makotoy
Makoto, this didn't work with the static build. We use a separate CaminoStatic target that needed the static libucvmath added to the link phase and the various other files added to the copy phases. We also don't use the automatically-generated static components list but instead have a hard-coded one. I've accounted for all of these differences in this patch. I've also removed the MathML support files from Camino.app/Contents/Resources, where they were not needed. They were already in Camino.app/Contents/MacOS/res, .../res/dtd, and .../res/fonts, which is correct.
Attached patch 1.8.0 branch patch (deleted) — Splinter Review
This is the patch for the 1.8.0 branch, less the new nsINonBlockingAlertService.idl file. I'm going to branch for Camino 1.0 later and will take this patch on that branch. I'd like to get this patch on the 1.8.0 branch because the Camino 1.0 branch is expected to be a short-lived mini stub, and I'd like to release a possible future Camino 1.0.1 from 1.8.0.
Attachment #210431 - Flags: approval1.8.0.2?
I've checked this in on the trunk. Because this blocks the rapidly-approaching Camino 1.0rc1, I've checked it in without a license block per comment 89. Makoto, please check in a new file with a proper license block or let me know what you want done with that file as soon as you can.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Additional change to mozilla/embedding/components/windowwatcher/public/Makefile.in: SDK_XPIDLSRCS = nsIWindowWatcher.idl \ nsIPromptService.idl \ - nsINonBlockingAlertService.idl \ $(NULL) XPIDLSRCS = nsIDialogParamBlock.idl \ nsPIPromptService.idl \ nsPIWindowWatcher.idl \ nsIAuthPromptWrapper.idl \ + nsINonBlockingAlertService.idl \ $(NULL) nsINonBlockingAlertService.idl is not frozen, it doesn't belong in SDK_XPIDLSRCS. Thanks to biesi for pointing this out.
(In reply to comment #92) > I've checked this in on the trunk. Because this blocks the rapidly-approaching > Camino 1.0rc1, I've checked it in without a license block per comment 89. > Makoto, please check in a new file with a proper license block or let me know > what you want done with that file as soon as you can. Thanks Mark. I'm not 100% sure on the way license entries should be, but are teh followings ok? Original Code: mozilla.org code initial developer: YAMASHITA Makoto <makotoy@ms.u-tokyo.ac.jp> Copyright: 2006 Contributes: (blank) If there are more appropriate ones, feel free to modify these for check-in. (Anyway this file is too trivial so in fact I even don't mind if the file is not attributed to me.)
I checked in an updated file with correct copyright and license information. I also marked the interface as scriptable. Makoto, the important part is making sure that the initial rights holder is correct, whether it's you, or your employer, or someone you're doing contract work for. Thanks a whole lot for getting this working in Camino! note for branch checkins: use this new version of nsINonBlockingAlertService.idl and the modified Makefile.in.
Checked in (with changes as described above) on CAMINO_1_0_BRANCH.
Whiteboard: [camino-1.0]
Comment on attachment 210431 [details] [diff] [review] 1.8.0 branch patch approved for 180 branch, a=dveditz for drivers
Attachment #210431 - Flags: approval1.8.0.2? → approval1.8.0.2+
Flags: blocking1.8.0.2+
Comment on attachment 210431 [details] [diff] [review] 1.8.0 branch patch Requesting approval from rbs for 1.8.1 branch (why did I miss that before?) Ideally, I'll defer 1.8.0.2 branch landing until approval for 1.8.1.
Attachment #210431 - Flags: approval-branch-1.8.1?(rbs)
Attachment #210431 - Flags: approval-branch-1.8.1?(rbs) → approval-branch-1.8.1+
Branch patch landed on 1_8_0 and 1_8. Context changes were needed for project.pbxproj on the 1_8 branch.
Déjà vu. Sorry, biesi, I forgot that this bug was the one with this problem. Fixed.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: