194240 - land the MINIMO_01302003_BRANCH branch

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Alec Flett]

This is the tracking bug for the landing of the minimo branch. we should use
this bug to keep people up to date with the branch landing, talk about code
reviews, and so forth.

Comment 1

[Darin Fisher]

my configure changes for netwerk/ don't need to be part of any carpool.  i'd
prefer to just land them separately.  i think that will be easier.  see bug 191970.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I'm a little confused by the "Make --disable-xul work" changes.  In particular:

1)  Why are XUL atoms addrefed, released, or event cared about if MOZ_XUL is not
    defined?  Is this so that XUL scrollbars will work?  If so, please comment
    that here and in all other places where XUL stuff lives outside MOZ_XUL
    defines because of scrollbars (frame constructor, makefiles, etc).
2)  Same for the nsCSSDeclaration changes.
3)  Same for the computed style changes.
4)  Same for the nsCSSStyleRule changes.  (I stopped looking for other places
    that have this issue at this point).
5)  What's the point of leaving HasGfxScrollbars() in the frame constructor if
    it always returns "true"?

Comment 3

[Doug Turner (:dougt)]

Attachment: diff between the tag and branch (zip)

Comment 4

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

In nsXBLBinding.cpp, you need to move
  anonymous->SetDocument(nsnull, PR_TRUE, PR_TRUE); // Kill it.
outside of the |#ifdef|, I think.

I agree with bz about the nsCSSDeclaration changes -- why were they made?  Isn't
the problem rather that you need more ifdefs elsewhere?  (Also, these changes
are likely to give me merge conflicts.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I see you decided to set use Gfx scrollbars always --- excellent decision! The
good news is that it will make converting nsListControlFrame to use
nsGfxScrollFrame (bug 126263) much simpler since it will be unconditional. And
once we've done that, we can get rid of nsScrollFrame and the associated class
nsScrollingView completely. And this will allow further simplifications in
nsCSSFrameConstructor and the view system.

Comment 6

[jhp (no longer active)]

Both '--disable-xpfe-components' and '--disable-disk-cache' have "Disable disk
cache" as the comment.

Comment 7

[Doug Turner (:dougt)]

Attachment: landed on trunk - no macprojects .xml diff

Comment 8

[Boris Zbarsky [:bzbarsky]]

"landed on trunk"?

Comment 9

[Doug Turner (:dougt)]

ie - this is a patch of the trunk with all of changes from the minimo diff.

Comment 10

[Darin Fisher]

Attachment: review comments for changes under modules/plugin

some major problems :(

Comment 11

[Boris Zbarsky [:bzbarsky]]

One way of getting the dir from a file given an nsIFile to the file is to get the parent, then get the path from that
(this is in response to one of Darin's last comments...)

Comment 12

[Darin Fisher]

bz: doh!  of course.. that is definitely the preferred way ;-)

Comment 13

[Brian Ryner (not reading)]

dbaron: what I was going for with the #ifdefs was to make having MOZ_XUL
disabled mean that XUL scrollbars should still be enabled.  I didn't see the
need for a separate set of #ifdef's for disabling all XUL including scrollbars,
since there's currently no way to have a functioning gecko without XUL
scrollbars working.

Comment 14

[Doug Turner (:dougt)]

i am not going land any of the MOZ_XUL changes.

Comment 15

[Doug Turner (:dougt)]

Attachment: current diff - no netwerk or XUL changes.

Comment 16

[Doug Turner (:dougt)]

Attachment: without configure

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 116915 [details] [diff] [review]
without configure

alec, darin - can I get some eyes on this patch?

Comment 18

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

Comment on attachment 116915 [details] [diff] [review]
without configure

ok, hope you don't mind my commenting on this patch...

+[  --disable-xpfe-components	 Disable disk cache],

this does not look right to me...

+  { "Netscape URI Loader Service", NS_URI_LOADER_CID,
NS_URI_LOADER_CONTRACTID, nsURILoaderConstructor, },
(and similar entries)

Should this be Mozilla isntead of Netscape? Or rather, why does this need a
vendor name at all?

+CKutil_ProfileDirectory(nsIFile** out) {
Is this one-line function really needed? Why can't the callers just call
NS_GetSpecialDirectory themselves?

Out of curiousity, for that native uconv support, do you plan to turn it on by
default?

+#define NS_MOZILLA_DIR_NAME		".mozilla"
Don't we have a configure argument that should be used for this...?

+<<<<<<< dlldeps.cpp
+    nsUnescape(nsnull);
+    nsEscape(nsnull, url_XAlphas);
+=======

looks like a cvs conflict in this patch

+<<<<<<< Makefile.in
+=======
		nsStatistics.cpp \
		nsStringEnumerator.cpp \
+>>>>>>> 1.79
another one

+    printf("Failed to find directory for key: %s\n", prop);

don't think you should have printf calls outside #ifdef DEBUG...

Index: xpcom/io/nsEscape.h

you remove nsEscapeCount, but leave nsUnescapeCount in? this sound strange to
me, is there a reason for that?

-MOZ_PKG_APPNAME = mozilla
+MOZ_PKG_APPNAME = minimo

I guess you don't want to land this on the trunk?

-MOZILLA_BIN	= $(DIST)/bin/mozilla$(BIN_SUFFIX)
+MOZILLA_BIN	= $(DIST)/bin/winembed.exe
 else
-MOZILLA_BIN	= $(DIST)/bin/mozilla-bin
+MOZILLA_BIN	= $(DIST)/bin/TestGtkEmbed

and this?

-	cd $(DIST)/$(MOZ_PKG_APPNAME); cp mozilla $(MOZ_PKG_APPNAME)
+	cd $(DIST)/$(MOZ_PKG_APPNAME); if [ -f mozilla ]; then cp mozilla
$(MOZ_PKG_APPNAME); fi
and maybe this


Also, out of curiousity, what are the footprint savings from this patch?

thanks for listening, I hope my comments were not too stupid :)

Comment 19

[Doug Turner (:dougt)]

thank you for your comments!

I fixed just about all of them.

Alec can comment on the use of "Netscape" instead of mozilla and the change to
xpcom/io/nsEscape.h (or was that darin?)

I am not going to fix CKutil_ProfileDirectory().  The code in that area is a tarpit.

To answer your questions:

the native uconv will not be enabled any time soon.  It is an experiment and I
need some help from intl experts before I do anything serious with it.

Assuming you use the same build options we did, the footprint savings can be
seen here:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=MiniMo

If you don't use the same options (in the case of mozilla), there is no savings.

Comment 20

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

well, on that tinderbox url there is no green tree with Z numbers...

Comment 21

[Doug Turner (:dougt)]

Attachment: patch - includes more suggestions and updated to tip.

Comment 22

[Alec Flett]

Comment on attachment 117024 [details] [diff] [review]
patch - includes more suggestions and updated to tip.

-	 delete new_cookie;
-	 strm.close();
+	 deleteCookie((void*)new_cookie, nsnull);
+	 strm->Close();

don't break the new_cookie stuff - that was part of trunk work.

 #ifdef  RICKG_DEBUG
 void WriteTokenToLog(CToken* aToken) {
-
-  static nsFileSpec fileSpec("c:\\temp\\tokenlog.html");
-  static nsOutputFileStream outputStream(fileSpec);
-  aToken->DebugDumpSource(outputStream); //write token without close
bracket...
 }
 #endif

Can we just get rid of the whole RICKG_DEBUG bit? (I see 2 of these in
COtherDTD.cpp and CNavDTD.cpp)

-    PRInt32 numread=0;
-    char* buf = new char[kBufsize+1];
+    PRUint32 numread=0;
+    char buf[kBufsize+1];
     buf[kBufsize]=0;

thank you, whoever this was!!! (I just ran into this code and was bugged by it)

I'm a little scared by our removal of GetComplexPref() and GetFilePref() for
nsIFileSpec - I think mail might just use that.. I just manually went through
all callers to GetComplexPref() and I didn't see anything. But its something to
watch out for if there are regressions.
Index: modules/plugin/base/src/nsPluginHostImpl.cpp

+  nsCOMPtr<nsIFile> file = do_CreateInstance("@mozilla.org/file/local;1");
+  nsCOMPtr<nsILocalFile> localfile = do_QueryInterface(file);
+  localfile->InitWithNativePath(filePath);

Why the extra QI()? we should just request the right interface in the first
place (I see at least two other instances of this)
Index: modules/plugin/base/src/nsPluginsDirDarwin.cpp
===================================================================
+    nsCAutoString temp;
+    mPlugin->GetNativePath(temp);
+    path = temp.get();
+

I don't like the idea of keeping "path" around - can we just use temp.get()
everwhere we need it?

(I stopped there, more in a bit.. about 50% through the patch)

Comment 23

[Alec Flett]

Comment on attachment 117024 [details] [diff] [review]
patch - includes more suggestions and updated to tip.

+	const char* path;
+
+    if (!mPlugin)
+	 return NS_ERROR_NULL_POINTER;
+
+    nsCAutoString temp;
+    mPlugin->GetNativePath(temp);
+    path = temp.get();
+    
+    versionsize = ::GetFileVersionInfoSize((char*)path, &zerome);

can you just use temp.get()?
also, use NS_CONST_CAST here and include a comment about why const-casting is
safe (but do you even need to  cast here?)

with all that nsTransferable stuff, did you test drag & drop with the system
(i.e. dragging link icons into mozilla, and vice versa and so forth)

other than that, it looks great!
sr=alecf with those changes.

Comment 24

[Doug Turner (:dougt)]

i also got a verbal r/sr=darin.  

Comment 25

[hacker formerly known as [e-mail]]

Erk.  For future reference, when adding configure options, make sure that both
cases are covered, ie:

MOZ_XPFE_COMPONENTS=1 
MOZ_ARG_DISABLE_BOOL(xpfe-components, 
[  --disable-xpfe-components    Disable xpfe components], 
    MOZ_XPFE_COMPONENTS=,
    MOZ_XPFE_COMPONENTS=1 )

It appears redundant but properly covers the case of overriding a set of default
configure options (in mozconfig or whatever) with per-build specific options.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Is there a reason the patch for bug 195153 was backed out in the landing?  Merge
errors?

Comment 27

[Doug Turner (:dougt)]

the whole patch was backed out or just parts?  if this happened, it would be do
to my merge.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Hmm... Looks like just the nsLayoutModule.cpp changes were affected.

Comment 29

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

This also seems to have broken the static build.  The errors building
TestGtkEmbed are:

../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`nsPrefMigrationModule_NSGetmodule':
EmbedComponents.o(.text+0x52): undefined reference to
`nsPrefMigrationModule_gModuleInfo'
../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`nsTransactionManagerModule_NSGetmodule':
EmbedComponents.o(.text+0x6a): undefined reference to
`nsTransactionManagerModule_gModuleInfo'
../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`nsMNGDecoderModule_NSGetmodule':
EmbedComponents.o(.text+0xca): undefined reference to
`nsMNGDecoderModule_gModuleInfo'
../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`nsMorkModule_NSGetmodule':
EmbedComponents.o(.text+0xe2): undefined reference to `nsMorkModule_gModuleInfo'
../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`nsCJVMManagerModule_NSGetmodule':
EmbedComponents.o(.text+0x1a2): undefined reference to
`nsCJVMManagerModule_gModuleInfo'
../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`nsWalletViewerModule_NSGetmodule':
EmbedComponents.o(.text+0x1d2): undefined reference to
`nsWalletViewerModule_gModuleInfo'
../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`XRemoteClientModule_NSGetmodule':
EmbedComponents.o(.text+0x202): undefined reference to
`XRemoteClientModule_gModuleInfo'
../../../../dist/lib/libgtkembedmoz.a(EmbedComponents.o): In function
`nsAutoConfigModule_NSGetmodule':
EmbedComponents.o(.text+0x21a): undefined reference to
`nsAutoConfigModule_gModuleInfo'

The differences in the command line linking TestGtkEmbed are:

the following are removed:
  -lgkview
  -ljsurl
  -lnsappshell
  -lprofile
  -lshistory
  -luconv
  -luriloader

the following are added:
  -lxpcom_compat_c
  -lxpcom_compat
  -ljsurl_s
  -lgkview_s
  -lshistory_s

Comment 30

[hacker formerly known as [e-mail]]

All components must have a MODULE_NAME makefile variable that's the same as the 
generic module name of the component or static builds will fail.

Comment 31

[Doug Turner (:dougt)]

static build bustages should be fixed now.  
bz - I fixed nsLayoutModule.cpp

I am closing this bug because the branch did land.  If you find problems, please
open new bug.

Comment 32

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

It looks like the files that were copied in the cvs repository to
xpcom/obsolete/ were not cvs-removed from their old locations yet.

Comment 33

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

it would have been good if you had removed the now-useless files in uriloader/build/ so as to 
not make people believe that they serve a purpose... 

Comment 34

[Alec Flett]

Yes, you're right. fortunately, the end of time hasn't occured yet, so we can
still do that.

Comment 35

[timeless]

*** Bug 195247 has been marked as a duplicate of this bug. ***

Comment 36

[Serge Gautherie (:sgautherie)]

*** Bug 195731 has been marked as a duplicate of this bug. ***