Closed
Bug 190307
Opened 22 years ago
Closed 19 years ago
Camino does not support MathML
Categories
(Camino Graveyard :: Page Layout, enhancement, P2)
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
|
rbs
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2+
|
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).
Comment 2•22 years ago
|
||
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".
Comment 4•22 years ago
|
||
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");
Comment 7•22 years ago
|
||
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•22 years ago
|
||
Comment 9•22 years ago
|
||
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•22 years ago
|
||
Comment 11•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•21 years ago
|
||
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 ?
Reporter | ||
Comment 15•21 years ago
|
||
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•21 years ago
|
||
*** Bug 224028 has been marked as a duplicate of this bug. ***
Comment 17•21 years ago
|
||
updating summary. Having a better URL would be great.
Summary: Chimera does not support MathML → [patch] Camino does not support MathML
Comment 18•21 years ago
|
||
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.
Comment 20•21 years ago
|
||
>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•21 years ago
|
||
Please update, is the patch working?
Comment 22•21 years ago
|
||
Please update.
Comment 23•20 years ago
|
||
(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.
Reporter | ||
Comment 24•20 years ago
|
||
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•20 years ago
|
||
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.
Reporter | ||
Comment 26•20 years ago
|
||
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•20 years ago
|
||
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•19 years ago
|
||
Speak up those who would like to see MathML in Camino 1.0. Patch needs
refreshing too.
Reporter | ||
Comment 29•19 years ago
|
||
I would but, then, I filed the bug.
Comment 30•19 years ago
|
||
I would, also.
Comment 31•19 years ago
|
||
I too would like to see MathML support.
Comment 32•19 years ago
|
||
(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•19 years ago
|
||
Note that we need to package fontEncoding.properties when we turn on MathML.
Assignee | ||
Comment 34•19 years ago
|
||
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 35•19 years ago
|
||
Comment on attachment 201115 [details] [diff] [review]
enables MathML in Camino
I'll look this over.
Attachment #201115 -
Flags: review?(bryner) → review?(mark)
Comment 36•19 years ago
|
||
(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•19 years ago
|
||
> 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•19 years ago
|
||
For what it's worth, looking at this dialog it looks like it can be a simple alert() call....
Comment 39•19 years ago
|
||
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)
Comment 40•19 years ago
|
||
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•19 years ago
|
||
> - 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•19 years ago
|
||
We have a request for non-blocking (or at least tab-modal) alerts in bug 123913 :)
Comment 43•19 years ago
|
||
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.
Comment 45•19 years ago
|
||
> 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•19 years ago
|
||
(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•19 years ago
|
||
*** Bug 315751 has been marked as a duplicate of this bug. ***
Comment 48•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
>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•19 years ago
|
||
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.
Assignee | ||
Comment 52•19 years ago
|
||
(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•19 years ago
|
||
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?
Assignee | ||
Comment 54•19 years ago
|
||
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)
Assignee | ||
Comment 55•19 years ago
|
||
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
Assignee | ||
Comment 56•19 years ago
|
||
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•19 years ago
|
||
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 58•19 years ago
|
||
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-
Assignee | ||
Comment 59•19 years ago
|
||
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
Comment 60•19 years ago
|
||
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•19 years ago
|
||
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.
Updated•19 years ago
|
Target Milestone: Camino1.0 → Camino1.1
Assignee | ||
Comment 62•19 years ago
|
||
The patch implemnts alert panel for a new interface nsINBAlertService for non-blocking alerts.
Attachment #205547 -
Attachment is obsolete: true
Assignee | ||
Comment 63•19 years ago
|
||
place this in embedding/components/windowwatcher/public/
Comment 64•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 66•19 years ago
|
||
> 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
Assignee | ||
Comment 67•19 years ago
|
||
Attachment #206390 -
Attachment is obsolete: true
Comment 68•19 years ago
|
||
(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.
Assignee | ||
Comment 69•19 years ago
|
||
now nsMathMLChar.cpp is free of direct UI manipulation.
Attachment #206542 -
Attachment is obsolete: true
Comment 70•19 years ago
|
||
Comment on attachment 206594 [details] [diff] [review]
adds nonblocking alert implementation for nsPromptService.cpp
Excellent.
Attachment #206594 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #206594 -
Flags: superreview?(mikepinkerton)
Comment 71•19 years ago
|
||
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.
Assignee | ||
Comment 72•19 years ago
|
||
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
Assignee | ||
Comment 73•19 years ago
|
||
... so that nsMathMLChar.cpp:AlertMissingFonts is now completely free from UI specific things.
Attachment #206594 -
Attachment is obsolete: true
Attachment #206594 -
Flags: superreview?(mikepinkerton)
Assignee | ||
Updated•19 years ago
|
Attachment #206749 -
Flags: review?(sfraser_bugs)
Assignee | ||
Updated•19 years ago
|
Attachment #206750 -
Flags: review?(sfraser_bugs)
Updated•19 years ago
|
Attachment #206749 -
Flags: review?(sfraser_bugs) → review+
Comment 74•19 years ago
|
||
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+
Comment 75•19 years ago
|
||
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 77•19 years ago
|
||
You forgot the null check.
Assignee | ||
Comment 78•19 years ago
|
||
oops... Removed unnecessary #include's from nsMathMLChar.cpp too.
Attachment #206971 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #206987 -
Flags: review?(sfraser_bugs)
Comment 79•19 years ago
|
||
(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.
Assignee | ||
Comment 80•19 years ago
|
||
The patch became inapplicable because Camino.xcode was changed.
Attachment #206987 -
Attachment is obsolete: true
Attachment #206987 -
Flags: review?(sfraser_bugs)
Assignee | ||
Updated•19 years ago
|
Attachment #208844 -
Flags: review?(sfraser_bugs)
Comment 81•19 years ago
|
||
You have a full path in the project file:
+ path = /Users/makoto/Development/mozilla/cmndev/dist/Embed/res/fonts;
Assignee | ||
Comment 82•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #208914 -
Flags: review? → review?(sfraser_bugs)
Updated•19 years ago
|
Attachment #208914 -
Flags: review?(sfraser_bugs) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #208914 -
Flags: superreview?(mikepinkerton)
Assignee | ||
Updated•19 years ago
|
Attachment #206749 -
Flags: superreview?(mikepinkerton)
Comment 83•19 years ago
|
||
Comment on attachment 208914 [details] [diff] [review]
Fixes absolute path, unnecessary change to the test
sr=pink
Attachment #208914 -
Flags: superreview?(mikepinkerton) → superreview+
Comment 84•19 years ago
|
||
Comment on attachment 206749 [details]
nonblocking service idl with documentation
sr=pink
Attachment #206749 -
Flags: superreview?(mikepinkerton) → superreview+
Comment 85•19 years ago
|
||
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.
Assignee | ||
Comment 86•19 years ago
|
||
(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•19 years ago
|
||
If your patch doesn't apply cleanly now, I'd appreciate an updated version. I'll shoot for a checkin on Tuesday or Wednesday.
Assignee | ||
Comment 88•19 years ago
|
||
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
Comment 89•19 years ago
|
||
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.
Updated•19 years ago
|
Assignee: bryner → makotoy
Comment 90•19 years ago
|
||
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•19 years ago
|
||
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?
Comment 92•19 years ago
|
||
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
Comment 93•19 years ago
|
||
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.
Assignee | ||
Comment 94•19 years ago
|
||
(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.)
Comment 95•19 years ago
|
||
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•19 years ago
|
||
Checked in (with changes as described above) on CAMINO_1_0_BRANCH.
Updated•19 years ago
|
Whiteboard: [camino-1.0]
Comment 97•19 years ago
|
||
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+
Updated•19 years ago
|
Flags: blocking1.8.0.2+
Comment 98•19 years ago
|
||
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+
Comment 99•19 years ago
|
||
Branch patch landed on 1_8_0 and 1_8. Context changes were needed for project.pbxproj on the 1_8 branch.
Keywords: fixed1.8.0.2,
fixed1.8.1
Comment 100•19 years ago
|
||
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•19 years ago
|
||
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.
Description
•