190307 - Camino does not support MathML

Status: RESOLVED FIXED

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

Description

[distler]

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.

Comment 1

[distler]

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

Comment 2

[Chris Petersen]

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

Comment 3

[distler]

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

Comment 4

[Chris Petersen]

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. 

Comment 5

[Henri Sivonen (:hsivonen)]

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.

Comment 6

[distler]

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");

Comment 7

[Brian Foley]

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.

Comment 8

[Brian Foley]

Attachment: add MathML libs/files to the Camino builds

Comment 9

[Brian Foley]

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.

Comment 10

[Brian Foley]

Attachment: MathML patch, including updated nsIPrompt code

Comment 11

[rbs]

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.

Comment 12

[Brian Foley]

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)

Comment 13

[rbs]

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.

Comment 14

[Ludovic Hirlimann [:Usul]]

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 ?

Comment 15

[distler]

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. 

Comment 16

[Ludovic Hirlimann [:Usul]]

*** Bug 224028 has been marked as a duplicate of this bug. ***

Comment 17

[Ludovic Hirlimann [:Usul]]

updating summary. Having a better URL would be great.

Comment 18

[rbs]

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.

Comment 19

[Henri Sivonen (:hsivonen)]

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.

Comment 20

[rbs]

>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

Comment 21

[Jasper]

Please update, is the patch working?

Comment 22

[Jasper]

Please update.

Comment 23

[Geoff "temporarily off bugmail" Beier]

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

Comment 24

[distler]

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?

Comment 25

[rbs]

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.

Comment 26

[distler]

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?

Comment 27

[rbs]

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

Comment 28

[Simon Fraser [no longer active]]

Speak up those who would like to see MathML in Camino 1.0. Patch needs
refreshing too.

Comment 29

[distler]

I would but, then, I filed the bug.

Comment 30

[Wevah (Nate Weaver)]

I would, also.

Comment 31

[Christopher Henderson]

I too would like to see MathML support.

Comment 32

[Joe Java]

(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

Comment 33

[Simon Fraser [no longer active]]

Note that we need to package fontEncoding.properties when we turn on MathML.

Comment 34

[YAMASHITA Makoto]

Attachment: 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?

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/

Comment 35

[Mark Mentovai]

Comment on attachment 201115 [details] [diff] [review]
enables MathML in Camino

I'll look this over.

Comment 36

[Simon Fraser [no longer active]]

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

Comment 37

[Boris Zbarsky [:bzbarsky]]

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

Comment 38

[Boris Zbarsky [:bzbarsky]]

For what it's worth, looking at this dialog it looks like it can be a simple alert() call....

Comment 39

[Brian Ryner (not reading)]

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.

Comment 40

[rbs]

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.

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

Comment 42

[Simon Fraser [no longer active]]

We have a request for non-blocking (or at least tab-modal) alerts in bug 123913 :)

Comment 43

[Boris Zbarsky [:bzbarsky]]

tab-modal would also be unacceptable here.  This code needs a "control returns to me immediately, before any painting happens" kinda alert.

Comment 44

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

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.

Comment 45

[Boris Zbarsky [:bzbarsky]]

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

Comment 46

[Simon Fraser [no longer active]]

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

Comment 47

[Wevah (Nate Weaver)]

*** Bug 315751 has been marked as a duplicate of this bug. ***

Comment 48

[Mark Mentovai]

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.

Comment 49

[Mark Mentovai]

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.

Comment 50

[rbs]

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

Comment 51

[Mark Mentovai]

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.

Comment 52

[YAMASHITA Makoto]

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

Comment 53

[Samuel Sidler (old account; do not CC)]

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?

Comment 54

[YAMASHITA Makoto]

Attachment: turn MathML for Camino, implements the missing font alert dialog

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

Comment 55

[YAMASHITA Makoto]

Attachment: turn MathML for Camino, implements the missing font alert dialog

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

Comment 56

[YAMASHITA Makoto]

Attachment: nsMathMLChar - Cocoa bridge

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 57

[Simon Fraser [no longer active]]

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

Comment 58

[Simon Fraser [no longer active]]

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.

Comment 59

[YAMASHITA Makoto]

Attachment: Use common facility for missing fonts dialog

I've finally managed to make OpenWindow work...
The alert load is slow, but it goes as intended.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Er.... the change to nsGlobalWindow in that last patch is not acceptable.  It's very likely to cause security holes, if nothing else.

Comment 61

[Simon Fraser [no longer active]]

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.

Comment 62

[YAMASHITA Makoto]

Attachment: make a new "nonblocking alert" interface

The patch implemnts alert panel for a new interface nsINBAlertService for non-blocking alerts.

Comment 63

[YAMASHITA Makoto]

Attachment: new xpidl for the nonblocking alert service interface

place this in embedding/components/windowwatcher/public/

Comment 64

[Simon Fraser [no longer active]]

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 65

[Simon Fraser [no longer active]]

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.

Comment 66

[YAMASHITA Makoto]

Attachment: 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? I'm not sure where to do this. Make new nsNonblockingService.* in embedding/components/windowwatcher/src? or just add ShowNonBlocking... to nsWindowWatcher.cpp?

Comment 67

[YAMASHITA Makoto]

Attachment: renamed xpidl for the nonblocking alert service

Comment 68

[Simon Fraser [no longer active]]

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

Comment 69

[YAMASHITA Makoto]

Attachment: adds nonblocking alert implementation for nsPromptService.cpp

now nsMathMLChar.cpp is free of direct UI manipulation.

Comment 70

[Simon Fraser [no longer active]]

Comment on attachment 206594 [details] [diff] [review]
adds nonblocking alert implementation for nsPromptService.cpp

Excellent.

Comment 71

[Boris Zbarsky [:bzbarsky]]

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.

Comment 72

[YAMASHITA Makoto]

Attachment: nonblocking service idl with documentation

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

Comment 73

[YAMASHITA Makoto]

Attachment: adapt to the new nsINonBlockingAlertService

... so that nsMathMLChar.cpp:AlertMissingFonts is now completely free from UI specific things.

Comment 74

[Simon Fraser [no longer active]]

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.

Comment 75

[rbs]

 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?

Comment 76

[YAMASHITA Makoto]

Attachment: Utilizes mWatcher

fixed that.

Comment 77

[rbs]

You forgot the null check.

Comment 78

[YAMASHITA Makoto]

Attachment: with null check

oops... Removed unnecessary #include's from nsMathMLChar.cpp too.

Comment 79

[Joe Java]

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

Comment 80

[YAMASHITA Makoto]

Attachment: Update to cope with the tree change

The patch became inapplicable because Camino.xcode was changed.

Comment 81

[Simon Fraser [no longer active]]

You have a full path in the project file:

+			path = /Users/makoto/Development/mozilla/cmndev/dist/Embed/res/fonts;

Comment 82

[YAMASHITA Makoto]

Attachment: Fixes absolute path, unnecessary change to the test

Oops, I did that again, and it contained an unnecessary change to CocoaEmbed.pbproj...
won't happen again :(

Comment 83

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 208914 [details] [diff] [review]
Fixes absolute path, unnecessary change to the test

sr=pink

Comment 84

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 206749 [details]
nonblocking service idl with documentation

sr=pink

Comment 85

[Mike Pinkerton (not reading bugmail)]

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. 

Comment 86

[YAMASHITA Makoto]

(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?

Comment 87

[Mark Mentovai]

If your patch doesn't apply cleanly now, I'd appreciate an updated version.  I'll shoot for a checkin on Tuesday or Wednesday.

Comment 88

[YAMASHITA Makoto]

Attachment: update for tree change

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

Comment 89

[Mark Mentovai]

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.

Comment 90

[Mark Mentovai]

Attachment: Supplementary project file and static components patch (trunk)

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.

Comment 91

[Mark Mentovai]

Attachment: 1.8.0 branch patch

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.

Comment 92

[Mark Mentovai]

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.

Comment 93

[Mark Mentovai]

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.

Comment 94

[YAMASHITA Makoto]

(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 <[e-mail]>
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.)

Comment 95

[Mark Mentovai]

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.

Comment 96

[Mark Mentovai]

Checked in (with changes as described above) on CAMINO_1_0_BRANCH.

Comment 97

[Daniel Veditz [:dveditz]]

Comment on attachment 210431 [details] [diff] [review]
1.8.0 branch patch

approved for 180 branch, a=dveditz for drivers

Comment 98

[Mark Mentovai]

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.

Comment 99

[Mark Mentovai]

Branch patch landed on 1_8_0 and 1_8.  Context changes were needed for project.pbxproj on the 1_8 branch.

Comment 100

[Christian :Biesinger (don't email me, ping me on IRC)]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/embedding/components/windowwatcher/public&command=DIFF_FRAMESET&file=Makefile.in&rev1=1.8&rev2=1.8.26.1&root=/cvsroot

please don't add unfrozen interfaces to SDK_XPIDLSRC. thanks :-)

Comment 101

[Mark Mentovai]

Déjà vu.

Sorry, biesi, I forgot that this bug was the one with this problem.  Fixed.