299058 - XUL error report in main window - <key id="addBookmarkKb key="&addCurPageCmd.commandkey;"/>

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Sergei Dolgov]

Mozilla Suite 1.8b2 for BeOS (built either from latest source tarball or from cvs.

<key id="addBookmarkKb key="&addCurPageCmd.commandkey;"/> error message at
bottom of main window
http://beos.spb.ru/mozilla/BeZilla_DTD_OVerlay_Bug.png

deleting old profile don't help.

that code appears in
xpfe/browser/resources/content/win/platformNavigationBindings.xul
and maybe in 
xpfe/browser/resources/content/mac/platformNavigationBindings.xul

I suspect that we are using (should use) Unix bindings instead, but unsure, what
happened now, as before all worked OK.

Also wondering about proper component to assign this bug

Comment 1

[Sergei Dolgov]

JavaScript console output:
----------------------------------
Error: undefined entity
Source File: chrome://navigator/content/platformNavigationBindings.xul
Line: 26, Column: 5
Source Code:
    <key id="addBookmarkKb" key="&addCurPageCmd.commandkey;"
command="Browser:AddBookmark" modifiers="accel,shift"/>
-----------------------------------

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> http://beos.spb.ru/mozilla/BeZilla_DTD_OVerlay_Bug.png

This bug looks much like bug 298675,

> Also wondering about proper component to assign this bug

then it could be an Installer issue.

*****

<http://lxr.mozilla.org/mozilla/search?string=addCurPageCmd.commandkey>
{{
addCurPageCmd.commandkey

/xpfe/browser/resources/content/mac/platformNavigationBindings.xul, line 20 --
<key id="addBookmarkKb" key="&addCurPageCmd.commandkey;"
command="Browser:AddBookmark" modifiers="accel,shift"/>
/xpfe/browser/resources/content/win/platformNavigationBindings.xul, line 26 --
<key id="addBookmarkKb" key="&addCurPageCmd.commandkey;"
command="Browser:AddBookmark" modifiers="accel,shift"/>

/xpfe/browser/resources/locale/en-US/mac/platformNavigationBindings.dtd, line 7
-- <!ENTITY addCurPageCmd.commandkey "d">
/xpfe/browser/resources/locale/en-US/win/platformNavigationBindings.dtd, line 5
-- <!ENTITY addCurPageCmd.commandkey "d">
}}

Or your/BeOS build could be missing the .dtd file, more like bug 298769 !?

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> I suspect that we are using (should use) Unix bindings instead, but unsure, what
> happened now, as before all worked OK.

Or this is the true issue
<http://lxr.mozilla.org/mozilla/find?string=platformNavigationBindings.xul>
{{
/xpfe/browser/resources/content/mac/platformNavigationBindings.xul
/xpfe/browser/resources/content/unix/platformNavigationBindings.xul
/xpfe/browser/resources/content/win/platformNavigationBindings.xul
}}

But I can't tell, since I know nothing about BeOS.

Comment 4

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

> This bug looks much like bug 298675,

why do you say that? the message is clearly different.

Comment 5

[Stefan [:stefanh]]

Hmm, do you have a platformNavigationBindings.xul without that entity in your
old builds?

Comment 6

[tqh]

I think bug 255036 added platform specific #ifdefs for xpfe/browser/jar.mn that
has windows as the fallback option. If I'm not mistaken the fallback was UNIX in
some Makefile, which makes for quite an interesting combination.

Comment 7

[tqh]

The patch there looks broken for all platforms that are not Mac, Unix or
Windows. It uses Unix as a fallback in all the makefiles and Windows in the two
jar.mn so there is mix of a Unix/Windows built. If that's what causing the
XUL-error I'm not sure, but we are using this file on BeOS
/xpfe/browser/resources/content/win/platformNavigationBindings.xul with that
change while we didn't have one before.

Comment 8

[Robert Kaiser]

From what I see, the fallback is Unix all over the code (even in installer
platform detection etc.), so it clearly should also be in the jar.nm files. Only
OS/2 should share files with Windows, all others (that aren't clearly
win/mac/unix) fall into Unix category in our source code.

Comment 9

[[e-mail]]

My fault - I thought Windows and OS/2 were the only non-Unices...

Comment 10

[Robert Kaiser]

FWIW,
http://lxr.mozilla.org/mozilla/source/xpfe/communicator/jar.mn#22
has the same problem as
http://lxr.mozilla.org/mozilla/source/xpfe/browser/jar.mn#33
and should be patched in the same way...

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> > This bug looks much like bug 298675,
> 
> why do you say that? the message is clearly different.

Yes, I did not meant they were duplicates;
I meant they show up the same way, and the fixes might be of the same kind.
('look' was the key word ;->)

Comment 12

[Sergei Dolgov]

So, what is proper way to fix those two jar.mn-s?
1)Add BEOS case and leave all remaining as is (Win as default/fallback).
2)Add BeOS, add Win and OS2, remove UNIX, makeing last default.
3)Don't add BeOS, but Add Win and OS2 and make Unix default.

Comment 13

[Robert Kaiser]

(In reply to comment #12)
> 3)Don't add BeOS, but Add Win and OS2 and make Unix default.

That's the way to go for sure, as we're doing the same in multiple other cases,
and we should stick to the same way of things where possible.

Comment 14

[Sergei Dolgov]

Need advise - how to activate and test changes in jar.mn-s properly?
Tried
1)make clean, make, make chrome in related folders. No luck.
2)same in xpfe folder - no luck
3) make distclean in moxilla/xpfe, configure in mozilla, make, make chrome in
xpfe - again in vain.

Interesting is that make for sure processes jar.mn - when i leave extra space
after #ifdef XP_UNIX, make reported about non-existing "XP_UNIX " identifier

Comment 15

[Sergei Dolgov]

Problem looks more complex than expected.
No one of "make"s has any effect until i remove manually basic jar-s from
dist/bin/chrome. (so, in my case, maybe some bit different make target is
required, like 'make dist" or such).

But when done (unix as default in jar.mm-s, or corrected win bindings file),
effect is sad - menu toolbar is empty in best case, empty bug bar in XUL at
bottom of window - or, in worst case - Mozilla don't launch properly, only
half-empty windows with XUL bug report (or such) rises.

So, i suspect, we had use for BeZilla mix of unix and win defaults in XUL (but
maybe it was window everywhere) - and that worked before.

More investigation needed:(

Comment 16

[Sergei Dolgov]

Got the reason, but need idea about better fix.
We use xpfe/browser/resources/content/win/platformNavigationBindings.xul (so MS
Windows one)
but mozilla/xpfe/browser/resources/locale/en-US/unix/platformNavigationBindings.dtd
(so for unix).
And, as you can guess now, there is:
<!ENTITY addCurPageCmd.commandkey "d"> in Windows *.dtd
but no such item in unix *.dtd

Adding mentioned entry in unix *.dtd fixes problem for BeOS build,
but will affect also all unix platfroms, which isn't our goal

Comment 17

[timeless]

i'd be fine w/ adding the item to the unix dtd, but should beos be using the 
win dtd?

i don't think adding a missing entity to the unix file should hurt unix, unless 
someone manages to include some other dtd that defines it. so i'd suggest doing 
that first. you can arrange to figure out the mess later...

Comment 18

[[e-mail]]

(In reply to comment #16)
>Got the reason, but need idea about better fix.
As KaiRo already said, change those jar.mn files to use #ifdef XP_WIN & XP_OS2

Comment 19

[Sergei Dolgov]

Neil, KaiRo,
actually i like to have that working cmd-D shortcut in BeOS too, for bookmarking.
And if it don't conflict with anything in "unix" - why not to simply add it to
*.dtd?

Comment 20

[[e-mail]]

(In reply to comment #19)
>why not to simply add it to *.dtd?
To avoid confusion, platform .dtd files should match their .xul files.

Comment 21

[Sergei Dolgov]

Attachment: patch (diff -up6)

Moves ex-default lines to XP_WIN32 case, duplicates same to XP_OS2 case, and
moves ex XP_UNIX lines to default (last #else)
Does it for both communicator and browser

Anyone to look, review and commit?

Comment 22

[Robert Kaiser]

Comment on attachment 198527 [details] [diff] [review]
patch (diff -up6)

If in any way possibe, there should be a single ifdef case for win32 and os2,
not two different ones. basically, the trend is correct though.

Comment 23

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

I think you misplaced an #else in the second file of the patch

unfortunately I don't think that the preprocessor supports something like #if
defined(FOO) || defined (BAR)

Comment 24

[Sergei Dolgov]

Attachment: patch

same as previous, but #else now at proper place,I hope (thanks biesi).
Yeah, i tried #if defined() || defined() like contructions, but it didn't work.

pp is really old-fashioned

Comment 25

[Sergei Dolgov]

Attachment: patch - 3rd attempts

taking in account Neil's notice about mistakenly mixed lines from communicator
and browser

Comment 26

[Sergei Dolgov]

Attachment: patch

restring empty line before en-US.jar:  header

Comment 27

[Sergei Dolgov]

2005-10-08-05-trunk - patch DOESN'T HELP anymore.

bookmark naviagtion seems placed now in platformNavigationBindings.xul
and there wasn't such file mentioned in mn.jar-s, at least when i did the patch.

It seems development moves faster than patches can be reviewed.
That's bad idea - add changes ignoring obvious existing bugs:(((

Comment 28

[Sergei Dolgov]

Comment on attachment 198626 [details] [diff] [review]
patch

ignore previous comment, just full chrome remove and make chrome in root folder
was required.
Tired of double building.

asking review to get this fix in tree at last.

Comment 29

[Sergei Dolgov]

Comment on attachment 198626 [details] [diff] [review]
patch

asking Neil for sr (and commit)

Comment 30

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

patch works fine on linux.

Comment 31

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

also works on win32.

Comment 32

[[e-mail]]

Fix checked in to the trunk.

Comment 33

[Sergei Dolgov]

wondering if there is sence to land it also in older 1.8 Mozilla Suite branch,
if it exists.

In theory this bug may affect other than BeOS systems

Comment 34

[Robert Kaiser]

(In reply to comment #33)
> wondering if there is sence to land it also in older 1.8 Mozilla Suite branch,
> if it exists.

Surely it makes sense, as we plan to release SeaMonkey 1.0 from that branch.
Just request approval on the patch...

Comment 35

[Sergei Dolgov]

Comment on attachment 198626 [details] [diff] [review]
patch

asking approval for SeaMonkey 1.0 branch