land the MINIMO_01302003_BRANCH branch

RESOLVED FIXED

Status

SeaMonkey
General
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: Alec Flett, Assigned: dougt)

Tracking

(Blocks: 1 bug, {meta})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

16 years ago
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

16 years ago
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"?
(Assignee)

Comment 3

16 years ago
Created attachment 115045 [details]
diff between the tag and branch (zip)
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
(Assignee)

Comment 7

16 years ago
Created attachment 116266 [details] [diff] [review]
landed on trunk - no macprojects .xml diff
Attachment #115045 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Comment 10

16 years ago
Created attachment 116281 [details]
review comments for changes under modules/plugin

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...)

Comment 12

16 years ago
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.
(Assignee)

Comment 14

16 years ago
i am not going land any of the MOZ_XUL changes.
(Assignee)

Comment 15

16 years ago
Created attachment 116910 [details] [diff] [review]
current diff - no netwerk or XUL changes.
Attachment #116266 - Attachment is obsolete: true
Attachment #116281 - Attachment is obsolete: true
(Assignee)

Comment 16

16 years ago
Created attachment 116915 [details] [diff] [review]
without configure
Attachment #116910 - Attachment is obsolete: true
(Assignee)

Comment 17

16 years ago
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 :)
(Assignee)

Comment 19

16 years ago
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...
(Assignee)

Comment 21

16 years ago
Created attachment 117024 [details] [diff] [review]
patch - includes more suggestions and updated to tip.
(Assignee)

Updated

16 years ago
Attachment #116915 - Attachment is obsolete: true
(Reporter)

Comment 22

16 years ago
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)
(Reporter)

Comment 23

16 years ago
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+
(Assignee)

Comment 24

16 years ago
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?
(Assignee)

Comment 27

16 years ago
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.

(Assignee)

Comment 31

16 years ago
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
Last Resolved: 16 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.

Updated

16 years ago
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... 
(Reporter)

Comment 34

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

Comment 35

15 years ago
*** Bug 195247 has been marked as a duplicate of this bug. ***
Blocks: 187528
*** 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.