With browser.show_about_as_stupid_modal_window set to true we get: XML Parsing Error: undefined entity Location: chrome://global/content/about.xul Line Number 49, Column 1:<dialog xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" Now I know this isn't a standard feature, but as it used to work before, I think we either need to remove the pref or fix the problem. (and this doesn't need to block the suiterunner bug, but I'm adding the link anyway so where know where all these bugs are).

I think removing the pref would probably do it, I don't know why we'd need that dialog...

I think this pref is somewhat useful to those of us who prefer to open a small box rather than a full browser window to display the About info, even if there aren't very many people who use it (or maybe I'm the only one :P). Anyway, this bug is being caused by the fact that Toolkit's about.dtd is being packed into en-US.jar instead of the old XPFE one, so the entities that about.xul needs aren't there. Here is a patch that moves the entities into a new file in the suite/ directory. I'm pretty sure I did this right, but I can't build Suiterunner right now, so this patch is untested. Adding the necessary entities to the Toolkit's about.dtd fixes the bug, though, so it's just the build-related parts that I'm not 100% sure about. Could someone please try it out to make sure it works?

I don't think this is useful or needed. Either our "About SeaMonkey" entry opens the dialog _BY DEFAULT_, then it should work. Or that menu entry always opens the about: page, which is what SeaMonkey always did, and in this case the dialog should not be even be integrated. Additionally, this dialog's XUL is coming from xpfe/global/ apparently, which we should drop one time anyways - across all products. This XUL file is probably even included in other apps than SeaMonkey, so the communicator/ reference can't be just put in there. And using a communicator/ .dtd file for the main strings of a global/ .xul seems very wrong anyways. Please just drop support for this dialog.

(In reply to comment #3) > Additionally, this dialog's XUL is coming from xpfe/global/ apparently, which > we should drop one time anyways - across all products. This XUL file is > probably even included in other apps than SeaMonkey, so the communicator/ > reference can't be just put in there. And using a communicator/ .dtd file for > the main strings of a global/ .xul seems very wrong anyways. Sorry, I wrote the patch under the assumption that about.xul would eventually be moved under suite/ if we were keeping it, but I should have moved it along with the .dtd within the patch. Firefox and the other Toolkit apps have their own About dialogs in their respective directories, so adding a reference to communicator/ won't affect them: http://lxr.mozilla.org/mozilla/find?string=aboutDialog.xul > Please just drop support for this dialog. Fair enough. I still don't like the idea of opening an entire browser window just for the About info, though, so maybe I'll eventually write a patch with a pref to open it in a tab instead.

KaiRo, would this be an acceptable solution? This patch removes support for the About dialog and replaces its pref with a pref for opening About in a new tab.

Comment on attachment 246447 [details] [diff] [review] Patch to remove About dialog Hmm, you really think anyone will press shift while clicking "About SeaMonkey" in the menu? I highly doubt that. Besides, pages that need chrome privs like "about:" does might force new windows in the future anyways, not sure if your tab pref is helpful there. Anyways, I'd much more prefer if we wouldn't invent a new pref there but at least reuse the "open new windows in new tabs" pref we already have...

(In reply to comment #6) > Hmm, you really think anyone will press shift while clicking "About SeaMonkey" > in the menu? I highly doubt that. Besides, pages that need chrome privs like > "about:" does might force new windows in the future anyways, not sure if your > tab pref is helpful there. > Anyways, I'd much more prefer if we wouldn't invent a new pref there but at > least reuse the "open new windows in new tabs" pref we already have... I think we need input from more people on this bug. Yes, the easiest solution would be to simply remove the dialog, but work has been done in the past to go the opposite direction and remove the new window in favor of the dialog (see bug 250624) since it is inconsistent with About Plug-ins and with NN 4.x's About, both of which open in the same window rather than opening a new one.

One big problem with the about: dialog is that its links don't work usefully. It might be a good idea to use IanN's new openAsExternal function (bug 361708).

(In reply to comment #8) > One big problem with the about: dialog is that its links don't work usefully. > > It might be a good idea to use IanN's new openAsExternal function (bug 361708). Thanks for the suggestion. However, when I tried implementing that function for About on a trunk build, I ran into a security error that prevented 'about:' from being loaded. I'm not sure why openTopWin() works and openAsExternal() doesn't, since they seem to be using similar code to load the URL...

(In reply to comment #9) > I'm not sure why openTopWin() works and openAsExternal() > doesn't, since they seem to be using similar code to load the URL... By taking another look at the code, I answered my own question: it's the urlSecurityCheck function on line 117 of contentAreaUtils.js.

Since I couldn't get openAsExternal() working, here is a new patch that combines some of its code with the code from my previous patch. This also takes care of the inconsistent behavior of About Plug-ins. Another alternative would be to implement a Firefox-style About window to match the other Toolkit apps, but I think this approach would go over better with SeaMonkey users.

Another reason we can't use openAsExternal is that it's not available in all windows e.g. the Error Console.

Comment on attachment 255527 [details] [diff] [review] Patch to remove About dialog v2 >Index: mozilla/suite/browser/browser-prefs.js Need to update xpfe/bootstrap/browser-prefs.js too. >+ if (!aProtocol) >+ aProtocol = ""; >+ var url = "about:" + aProtocol; Could use (aProtocol || "") here. > try { > var pref = Components.classes["@mozilla.org/preferences-service;1"] > .getService(Components.interfaces.nsIPrefBranch); >- defaultAboutState = pref.getBoolPref("browser.show_about_as_stupid_modal_window"); >+ defaultAboutState = pref.getIntPref("browser.link.open_external"); > } I think we can assume that this pref exists. >+ if (!pref.getBoolPref("browser.tabs.loadInBackground")) >+ browser.selectedTab = newTab; I'm not sure we should do this conditionally. Thoughts, anyone? >+ openTopWin(url); We certainly shouldn't do this. We should just load the page.

(In reply to comment #13) > Need to update xpfe/bootstrap/browser-prefs.js too. Fixed in this patch. > Could use (aProtocol || "") here. Fixed. > I think we can assume that this pref exists. Fixed. > I'm not sure we should do this conditionally. Thoughts, anyone? Good point, users probably wouldn't expect About: to open in the background after choosing it from the menu. I changed it in this patch. > We certainly shouldn't do this. We should just load the page. Oops, fixed. I also moved focus() so that it would be triggered if an existing window is used.

(In reply to comment #13) > (From update of attachment 255527 [details] [diff] [review]) > >Index: mozilla/suite/browser/browser-prefs.js > Need to update xpfe/bootstrap/browser-prefs.js too. Ermm, if we're removing it from xpfe/bootstrap/browser-prefs.js then we need to fix xpfe/communicator/resources/content/utilityOverlay.js as well - as we copied from that file originally although we're not using it now, camino or minimo may be using it.

(In reply to comment #15) > (In reply to comment #13) > > (From update of attachment 255527 [details] [diff] [review] [details]) > > >Index: mozilla/suite/browser/browser-prefs.js > > Need to update xpfe/bootstrap/browser-prefs.js too. > Ermm, if we're removing it from xpfe/bootstrap/browser-prefs.js then we need to > fix xpfe/communicator/resources/content/utilityOverlay.js as well - as we > copied from that file originally although we're not using it now, camino or > minimo may be using it. Or we could just not fix either of the xpfe files?

(In reply to comment #15) >Ermm, if we're removing it from xpfe/bootstrap/browser-prefs.js then we >need to fix xpfe/communicator/resources/content/utilityOverlay.js as well utilityOverlay.js has that pref in a try/catch so it doesn't matter.

Wait a minute, Camino/minimo don't even use our browser-prefs.js do they?

Comment on attachment 256860 [details] [diff] [review] Patch to remove About dialog v3 >+ browserWin.focus(); This is incorrect for two reasons: the focus should go on the content window, and you should not change the focus before changing the tab. >+ } >+ else While "else {" (as used above) is OK, I don't like "} else". Either put in an extra pair of {}s or swtich the if around so that this becomes an "else {". sr=me with these fixed.

Since I moved some code around, I created a new patch. (In reply to comment #19) > This is incorrect for two reasons: the focus should go on the content window, > and you should not change the focus before changing the tab. Fixed so that it is below the tab code and so that it focuses the content window. > While "else {" (as used above) is OK, I don't like "} else". Either put in an > extra pair of {}s or swtich the if around so that this becomes an "else {". Fixed.

Comment on attachment 256974 [details] [diff] [review] Patch to remove About dialog v3.1 Neil, would it make sense to reuse navigator.js's code that determines what to do based on browser.link.open_external?

Comment on attachment 256974 [details] [diff] [review] Patch to remove About dialog v3.1 Also, how does this relate to attachment 255902 [details] [diff] [review] on bug 370698?

They aren't dependent one another (so I haven't marked it as such). The only issue is that they would conflict. I.e., the fix for bug 370698 landing would require changes here before landing (vice-versa). Since that one is less important, I do prefer to wait this one to land before. Both touch "goAboutDialog()". It's much trivial to resolve anyway.

(In reply to comment #21) >(From update of attachment 256974 [details] [diff] [review]) >Neil, would it make sense to reuse navigator.js's code that determines what to >do based on browser.link.open_external? We can't do that because of its security check. See comment #9.

I just checked in the patch on Neuos' behalf. Checking in suite/browser/browser-prefs.js; /cvsroot/mozilla/suite/browser/browser-prefs.js,v <-- browser-prefs.js new revision: 1.61; previous revision: 1.60 done Checking in suite/common/utilityOverlay.js; /cvsroot/mozilla/suite/common/utilityOverlay.js,v <-- utilityOverlay.js new revision: 1.99; previous revision: 1.98 done Checking in suite/common/utilityOverlay.xul; /cvsroot/mozilla/suite/common/utilityOverlay.xul,v <-- utilityOverlay.xul new revision: 1.52; previous revision: 1.51 done Checking in xpfe/bootstrap/browser-prefs.js; /cvsroot/mozilla/xpfe/bootstrap/browser-prefs.js,v <-- browser-prefs.js new revision: 1.61; previous revision: 1.60

Thanks for the checkin!