Last Comment Bug 299058 - XUL error report in main window - <key id="addBookmarkKb key="&addCurPageCmd.commandkey;"/>
: XUL error report in main window - <key id="addBookmarkKb key="&addCurPageCmd....
Status: RESOLVED FIXED
: fixed1.8
Product: SeaMonkey
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: x86 BeOS
: -- major (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: 255036
  Show dependency treegraph
 
Reported: 2005-06-28 14:35 PDT by Sergei Dolgov
Modified: 2005-10-11 02:45 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (diff -up6) (5.21 KB, patch)
2005-10-04 17:04 PDT, Sergei Dolgov
kairo: review-
Details | Diff | Splinter Review
patch (5.21 KB, patch)
2005-10-05 03:06 PDT, Sergei Dolgov
no flags Details | Diff | Splinter Review
patch - 3rd attempts (5.21 KB, patch)
2005-10-05 13:52 PDT, Sergei Dolgov
no flags Details | Diff | Splinter Review
patch (4.91 KB, patch)
2005-10-05 14:10 PDT, Sergei Dolgov
cbiesinger: review+
neil: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description Sergei Dolgov 2005-06-28 14:35:13 PDT
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 2005-06-28 14:40:37 PDT
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) 2005-06-28 14:56:47 PDT
(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) 2005-06-28 15:01:38 PDT
(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) 2005-06-28 15:18:36 PDT
> This bug looks much like bug 298675,

why do you say that? the message is clearly different.
Comment 5 Stefan [:stefanh] 2005-06-28 16:02:48 PDT
Hmm, do you have a platformNavigationBindings.xul without that entity in your
old builds?
Comment 6 tqh 2005-06-28 23:27:52 PDT
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 2005-06-28 23:40:03 PDT
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 2005-06-29 05:30:02 PDT
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 neil@parkwaycc.co.uk 2005-06-29 05:31:35 PDT
My fault - I thought Windows and OS/2 were the only non-Unices...
Comment 10 Robert Kaiser 2005-06-29 06:21:06 PDT
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) 2005-06-29 14:43:42 PDT
(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 2005-07-01 03:55:53 PDT
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 2005-07-01 04:16:46 PDT
(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 2005-07-02 03:47:29 PDT
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 2005-07-02 07:13:45 PDT
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 2005-09-20 13:31:43 PDT
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 2005-09-20 14:05:41 PDT
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 neil@parkwaycc.co.uk 2005-09-20 16:17:21 PDT
(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 2005-09-27 01:31:38 PDT
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 neil@parkwaycc.co.uk 2005-09-27 02:54:42 PDT
(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 2005-10-04 17:04:11 PDT
Created attachment 198527 [details] [diff] [review]
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 2005-10-04 19:46:56 PDT
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) 2005-10-05 01:19:00 PDT
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 2005-10-05 03:06:08 PDT
Created attachment 198559 [details] [diff] [review]
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 2005-10-05 13:52:20 PDT
Created attachment 198622 [details] [diff] [review]
patch - 3rd attempts

taking in account Neil's notice about mistakenly mixed lines from communicator
and browser
Comment 26 Sergei Dolgov 2005-10-05 14:10:53 PDT
Created attachment 198626 [details] [diff] [review]
patch

restring empty line before en-US.jar:  header
Comment 27 Sergei Dolgov 2005-10-09 03:37:24 PDT
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 2005-10-09 05:00:35 PDT
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 2005-10-09 07:21:08 PDT
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) 2005-10-09 07:34:54 PDT
patch works fine on linux.
Comment 31 Christian :Biesinger (don't email me, ping me on IRC) 2005-10-09 12:21:25 PDT
also works on win32.
Comment 32 neil@parkwaycc.co.uk 2005-10-09 16:38:17 PDT
Fix checked in to the trunk.
Comment 33 Sergei Dolgov 2005-10-09 16:54:29 PDT
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 2005-10-10 01:27:09 PDT
(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 2005-10-10 03:07:45 PDT
Comment on attachment 198626 [details] [diff] [review]
patch

asking approval for SeaMonkey 1.0 branch

Note You need to log in before you can comment on or make changes to this bug.