201809 - Fork bookmarks for Phoenix work

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[David Hyatt]

The rule is "don't add Phoenix ifdefs", which means I need to fork bookmarks 
for the Web Panels work in Phoenix.  Here is the patch that shifts bookmarks 
stuff into the already-existing #ifndef MOZ_PHOENIX blocks.

Comment 1

[David Hyatt]

Attachment: xpfe/components builds changes

Comment 2

[Blake Ross]

Comment on attachment 120335 [details] [diff] [review]
xpfe/components builds changes

Hyatt continues to amaze me.  Between his work at apple and his grossly illegal
underage porn selling business, I don't know where he finds the time.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't think we have a hard-and-fast rule; just a dislike for adding ifdefs
instead of solving the real problem.... (that's what the situation was last time
I saw this come up).

Maybe we can make bookmarks flexible enough to not need forking by every UI
implementor?

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 120335 [details] [diff] [review]
xpfe/components builds changes

>Index: build/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/build/Makefile.in,v
>retrieving revision 1.49
>diff -u -r1.49 Makefile.in
>--- build/Makefile.in	8 Apr 2003 08:51:49 -0000	1.49
>+++ build/Makefile.in	12 Apr 2003 21:38:07 -0000
>@@ -86,6 +85,7 @@
> 
> ifndef MOZ_PHOENIX
> SHARED_LIBRARY_LIBS += \
>+  $(DIST)/lib/$(LIB_PREFIX)bookmarks_s.$(LIB_SUFFIX) \
> 	$(DIST)/lib/$(LIB_PREFIX)downloadmanager_s.$(LIB_SUFFIX) \
> 	$(DIST)/lib/$(LIB_PREFIX)autocomplete_s.$(LIB_SUFFIX) \

The rest of these lines are using hard tabs; please do the same.

r/sr=bryner other than that.

Comment 5

[Brian Ryner (not reading)]

Actually, there's one other change you might want to make.  As you have it 
now, we'll compile stuff in xpfe/components/bookmarks, and just not link it 
into the dll.  If you forked nsIBookmarksService.idl too, you could skip 
building in xpfe/components/bookmarks at all (there's a section in 
xpfe/components/Makefile.in for directories that are only built for 
seamonkey).  You'll hit ordering problems though... for reasons I'm sure I'd 
rather not know about, nsHTMLDocument needs nsIBookmarksService.h, and since 
browser is in a later 'tier' than content in the toplevel Makefile.in, that 
header won't be exported at the time content needs it.

Comment 6

[Brendan Eich [:brendan]]

Why does Gecko, a more primitive thing, depend on an interface header exported
by xpfe?  I've beaten others up about such mistakes in the past.  Who do I beat
up now?  Let's consult lxr, shall we?

revision 3.239
date: 2000/06/01 01:13:45;  author: jbetak%netscape.com;  state: Exp;  lines: +53 -5
18022, bookmark doc charset caching
r=rjc, ftang, a=ftang

Before the era of super-review, wheeee!

I'll pre-sr anyone on the cc: list who gets rid of this bogus dependency.

/be

Comment 7

[David Hyatt]

Fixed.

Comment 8

[Duncan Mak]

I followed the instructions on http://phoenix.ragweed.net/build and downloaded
the latest mozilla tarball. I did a 'gmake -f client.mk fast-update' and sync'd
the tree before building. On my laptop, running Red Hat 8.0, I get this error
upon entering the mozilla/browser/components/build dir:

gmake[5]: Entering directory
`/home/duncan/src/phoenix-source/mozilla/browser/components/build'
...

c++ -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion
-Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy
-pedantic -Wno-long-long -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O2
-march=i686 -fPIC -shared -Wl,-h -Wl,libbrowsercomps.so -o libbrowsercomps.so 
nsModule.o       -Wl,--whole-archive ../../../dist/lib/libdownload_s.a
../../../dist/lib/libbookmarks_s.a  -Wl,--no-whole-archive 
../../../dist/lib/libunicharutil_s.a -L../../../dist/bin -lxpcom 
-L../../../dist/bin -L/home/duncan/src/phoenix-source/mozilla/dist/lib -lplds4
-lplc4 -lnspr4 -lpthread -ldl  -L../../../dist/lib -lxpcom_compat
-L../../../dist/bin -lmozjs  -Wl,--version-script
-Wl,../../../build/unix/gnu-ld-scripts/components-version-script -Wl,-Bsymbolic
-ldl -lm  -lole32 -lshell32   
/usr/bin/ld: cannot find -lole32
collect2: ld returned 1 exit status

Is this to be expected?

Comment 9

[[e-mail]]

Brendan, it looks as if the document gets the charset from the bookmark (if
there's a bookmark for that page with a forced charset). I expect it would be
reasonable to make opening a bookmark force the charset instead.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that those are not exactly equivalent... (though I do prefer Neil's
solution... ;))

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 120335 [details] [diff] [review]
xpfe/components builds changes

This doesn't even compile for me, since you didn't ifdef the #include of
nsBookmarksService.h at the top of nsModule.cpp (but you did change the
LOCAL_INCLUDES conditionally for phoenix so that it can't be found).

I also get the error in comment 8.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: some attempts to fix bustage

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 120447 [details] [diff] [review]
some attempts to fix bustage

er, yeah. r/sr=me.

Comment 14

[Brian Ryner (not reading)]

checked in the patch.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: additional bustage fix

Yeah, I didn't clobber.  This should help.  (Although I see bryner's checked in
a bit of other stuff already.)

Comment 16

[Chris Petersen]

Verified per last comments