Closed Bug 194240 Opened 17 years ago Closed 17 years ago

land the MINIMO_01302003_BRANCH branch

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alecf, Assigned: dougt)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(1 file, 5 obsolete files)

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.
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.
Blocks: 191970
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"?
Attached file diff between the tag and branch (zip) (obsolete) —
Attachment #115045 - Attachment mime type: application/octet-stream → application/zip
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.)
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.
Blocks: 126263
Both '--disable-xpfe-components' and '--disable-disk-cache' have "Disable disk
cache" as the comment.
QA Contact: asa → ian
Attachment #115045 - Attachment is obsolete: true
"landed on trunk"?
ie - this is a patch of the trunk with all of changes from the minimo diff.
some major problems :(
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...)
bz: doh!  of course.. that is definitely the preferred way ;-)
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.
i am not going land any of the MOZ_XUL changes.
Attachment #116266 - Attachment is obsolete: true
Attachment #116281 - Attachment is obsolete: true
Attached patch without configure (obsolete) — Splinter Review
Attachment #116910 - Attachment is obsolete: true
Comment on attachment 116915 [details] [diff] [review]
without configure

alec, darin - can I get some eyes on this patch?
Attachment #116915 - Flags: superreview?(darin)
Attachment #116915 - Flags: review?(alecf)
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 :)
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.
well, on that tinderbox url there is no green tree with Z numbers...
Attachment #116915 - Attachment is obsolete: true
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 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.
Attachment #117024 - Flags: superreview+
i also got a verbal r/sr=darin.  
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.
Is there a reason the patch for bug 195153 was backed out in the landing?  Merge
errors?
the whole patch was backed out or just parts?  if this happened, it would be do
to my merge.
Hmm... Looks like just the nsLayoutModule.cpp changes were affected.
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
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.

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.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
It looks like the files that were copied in the cvs repository to
xpcom/obsolete/ were not cvs-removed from their old locations yet.
Attachment #116915 - Flags: superreview?(darin)
Attachment #116915 - Flags: review?(alecf)
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... 
Yes, you're right. fortunately, the end of time hasn't occured yet, so we can
still do that.
*** Bug 195247 has been marked as a duplicate of this bug. ***
*** Bug 195731 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
Duplicate of this bug: 191810
You need to log in before you can comment on or make changes to this bug.