Allow glue-finding code to specify additional features such as toolkit=gtk2 and xulrunner=true

RESOLVED FIXED in mozilla1.8beta4

Status

P1
normal
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

({fixed1.8})

unspecified
mozilla1.8beta4
fixed1.8
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Details

Attachments

(11 attachments, 5 obsolete attachments)

4.65 KB, patch
brendan
: first-review+
Details | Diff | Splinter Review
17.94 KB, patch
darin.moz
: first-review+
Details | Diff | Splinter Review
574 bytes, text/plain
darin.moz
: first-review+
Details
70.95 KB, patch
Details | Diff | Splinter Review
1.77 KB, patch
jshin1987
: first-review+
Details | Diff | Splinter Review
2.11 KB, patch
Details | Diff | Splinter Review
885 bytes, patch
Details | Diff | Splinter Review
2.32 KB, patch
Details | Diff | Splinter Review
28.99 KB, patch
darin.moz
: first-review+
Details | Diff | Splinter Review
3.46 KB, patch
Details | Diff | Splinter Review
2.22 KB, patch
benjamin
: first-review+
darin.moz
: second-review+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
We are probably going to have seamonkey and xulrunner 1.8 GREs out in the wild
:-(, and there are situations in which we may have GTK2 and QT xulrunners
installed on the same machine :-(. Finally, it should be possible to look for a
range of GRE versions, not just one particular version (this unfortunately
requires GRE IDs to be something other than opaque strings, something I haven't
figured out yet).

We need a function like GRE_FindGREPathForVersion that will find a GRE based on
these multiple characteristics.
(Assignee)

Updated

13 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta4
(Assignee)

Comment 1

13 years ago
Created attachment 191380 [details] [diff] [review]
Part JS, rev. 1 - Alter jsdhash and the plify script to use NS_COM_GLUE and avoid NSPR symbols
Attachment #191380 - Flags: second-review?(shaver)
Attachment #191380 - Flags: first-review?(brendan)
(Assignee)

Comment 2

13 years ago
Created attachment 191385 [details] [diff] [review]
Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue
Attachment #191385 - Flags: first-review?(darin)
(Assignee)

Comment 3

13 years ago
Created attachment 191395 [details]
Part XPCOMGlue (cvsmoves)
Attachment #191395 - Flags: first-review?(darin)
(Assignee)

Updated

13 years ago
Blocks: 296286

Comment 4

13 years ago
Comment on attachment 191385 [details] [diff] [review]
Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue

>Index: xpcom/ds/nsHashKeys.h

>+#ifdef MOZILLA_INTERNAL_API
>+#include "nsAString.h"
>+#include "nsString.h"
>+#else
>+#include "nsStringAPI.h"
>+#endif

Since we are trying to present nearly the same API for the
frozen strings as the internal strings, it might be nice to
create a header file that code can #include to get the 
proper string headers.


>Index: xpcom/ds/nsTHashtable.cpp

>+NS_COM PRUint32 HashString( const nsAString& aStr )
>+{
>+  PRUint32 code = 0;
>+
>+#ifdef MOZILLA_INTERNAL_API
>+  nsAString::const_iterator begin, end;
>+  aStr.BeginReading(begin);
>+  aStr.EndReading(end);
>+#else
>+  const PRUnichar *begin, *end;
>+  PRUint32 len = NS_StringGetData(aStr, &begin);
>+  end = begin + len;
>+#endif

Doesn't nsStringAPI.h define BeginReading/EndReading?
Why not use those here to avoid #ifdef?


>Index: xpcom/string/public/nsStringAPI.h

>+  NS_HIDDEN_(PRBool) Equals( const self_type &other ) const {
>+    const char_type *cself;
>+    const char_type *cother;
>+    PRUint32 selflen = NS_StringGetData(*this, &cself);
>+    PRUint32 otherlen = NS_StringGetData(other, &cother);
>+
>+    if (selflen != otherlen)
>+      return PR_FALSE;
>+
>+    for (const char_type *end = cself + selflen;
>+         cself < end;
>+         ++cself, ++cother) {
>+      if (*cself != *cother)
>+        return PR_FALSE;
>+    }
>+
>+    return PR_TRUE;
>+  }

Ugh.  I think this should be non-inline, and implemented in the
glue library.  Also, why not use |memcmp|?  I think you should
be able to use that function in both implementations.


Otherwise, this patch looks good to me.  r=darin
Attachment #191385 - Flags: first-review?(darin) → first-review+

Updated

13 years ago
Attachment #191395 - Flags: first-review?(darin) → first-review+
Comment on attachment 191380 [details] [diff] [review]
Part JS, rev. 1 - Alter jsdhash and the plify script to use NS_COM_GLUE and avoid NSPR symbols

Looks good, assuming sed works and the xpcom/ds output files diff well (against
their previous revs, and null diff against the js ones when sed'ed).  r=me.

/be
Attachment #191380 - Flags: first-review?(brendan) → first-review+
(Assignee)

Comment 6

13 years ago
Comment on attachment 191385 [details] [diff] [review]
Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue

This should be low risk, it's just changing a little of the linking strategy
and is almost necessary to fix the xulrunner startup process.
Attachment #191385 - Flags: approval1.8b4?
(Assignee)

Comment 7

13 years ago
Oh, and I did things the way I did for a reason:

1) The stringapi versions of beginreading/endreading would end up calling
NS_GetStringData twice, and this is a critical codepath.

2) the stringapi header currently does not require that you link against any
glue. If we want to require this, we have to think through which versions of the
glue this function gets defined in (we probably do not want it in the
"xpcom_core.dll" glue, only the xpcomglue_s.lib and xpcomglue.lib versions... we
currently don't have the link-magic to make this happen.
(Assignee)

Updated

13 years ago
Attachment #191380 - Flags: second-review?(shaver) → approval1.8b4?
(Assignee)

Comment 8

13 years ago
Created attachment 191720 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 1

This consolidates several of our existing variations on nsINIParser into the
xpcom glue... nsGREGlue.cpp will use this implementation instead of the
custom-hacked INI parser as the last patch here. I'm still building various
pieces of this, so I may attach minor build-fixes if there are problems, but I
think this should be good enough for review.
Attachment #191720 - Flags: first-review?(darin)

Comment 9

13 years ago
Comment on attachment 191720 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 1

>Index: xpcom/glue/nsINIParser.cpp

>+    rv = aFile->OpenANSIFileDesc("r", &fd);
...
>+    if (!fb) {
>+        rv = NS_ERROR_OUT_OF_MEMORY;
>+        goto bail;
>+    }
...
>+bail:
>+    if (fd)
>+        fclose(fd);
>+
>+    free(fb);

In the updater code, I created a AutoFILE class to avoid messy
goto statements.  You could do the same here, and use operator
new[] and delete[] with nsAutoArrayPtr.  Then, the code would
be a lot cleaner.


>+// copied from toolkit/mozapps/updater/src/updater/updater.cpp
>+// we could use nsCRT::strtok except that nsCRT isn't part of the glue,
>+// and may never be due to NSPR dependencies

Maybe we should add this function to the glue at some point.
I think we basically want to add a set of string helper functions
to the glue.  These would include nice functions for working
with nsAString/nsACString, but it might also include stuff like
this for working with raw char buffers.


>Index: toolkit/profile/src/nsToolkitProfileService.cpp

>+    rv = parser.GetString(NS_LITERAL_CSTRING("General"),
>+                          NS_LITERAL_CSTRING("StartWithLastProfile"),

I wonder.. is it really better for the first two
parameters to be nsACString here?  The result should
definitely be returned via nsACString, but i'm not so
sure about the input.  Maybe consistency is best :-/
RBool aValue)


>+      rv = parser.GetString(nsDependentCString(lastSectionName),
>+                                     nsDependentCString(transform->keyName),
>+                                     val);

fix indentation (this is in the opera profile migrator)


>+    nsCAutoSTring proxyServer;

compile error.


>+    rv = aParser.GetStringAlloc(NS_LITERAL_CSTRING("Proxy"),
>+                                 nsDependentCString(serverBuf),
>+                                 proxyServer);

indentation... what is GetStringAlloc?


>-#if defined(XP_WIN) || (defined(XP_UNIX) && !defined(XP_MACOSX))
>+// #if defined(XP_WIN) || (defined(XP_UNIX) && !defined(XP_MACOSX))
>+#if 1

why add the "#if 1" business?
Attachment #191720 - Flags: first-review?(darin) → first-review-
(Assignee)

Comment 10

13 years ago
Created attachment 191768 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 1.1

This one actually builds, and I've added the auto-closing classes with early
returns. The #if changes were just local changes to test-build that code since
this happens to be a mac tree.

I thought about keeping the signature "const char*, const char*, nsACString&
aResult", but I intend to write an XPCOM interface wrapper around this code
which I would prefer used ACString instead of string. The
NS_LITERAL_CSTRING/dependentstring wrappers around the current code aren't that
big a deal.
Attachment #191720 - Attachment is obsolete: true
Attachment #191768 - Flags: first-review?(darin)
(Assignee)

Updated

13 years ago
Blocks: 300139

Comment 11

13 years ago
Comment on attachment 191380 [details] [diff] [review]
Part JS, rev. 1 - Alter jsdhash and the plify script to use NS_COM_GLUE and avoid NSPR symbols

a=chase@mozilla.org
Attachment #191380 - Flags: approval1.8b4? → approval1.8b4+

Comment 12

13 years ago
Comment on attachment 191385 [details] [diff] [review]
Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue

a=chase@mozilla.org
Attachment #191385 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 13

13 years ago
The cvsmoves should be performed here before 1.8 branches, as they will be much
more difficult to manage afterwards.
Blocks: 300860
Whiteboard: needs cvsmoves from justdave
(Assignee)

Comment 14

13 years ago
Comment on attachment 191768 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 1.1

I need another "init" method which I can use from the standalone glue.
Attachment #191768 - Attachment is obsolete: true
Attachment #191768 - Flags: first-review?(darin)
(Assignee)

Comment 15

13 years ago
In fact, this can't use ACStrings for my purposes because you can't use
ACStrings until the XPCOM glue is started. Back to the char* drawing board.
(Assignee)

Comment 16

13 years ago
Created attachment 192007 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 2

This uses const char* instead of nsACString to satisfy the glue-init
chicken-and-egg problem.
Attachment #192007 - Flags: first-review?(darin)

Comment 17

13 years ago
Comment on attachment 192007 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 2

Do we need to worry about InitFromFILE being called twice on the same
nsINIParser instance?  What if memory allocation (of mFileContents) succeeds,
but the subsequent call to fseek fails?  Then, imagine InitFromFILE being
called again.  Won't we leak mFileContents in the process?  Do we need to worry
about mFileContents being non-nullterminated after an fread error?

aResult[aResultLen] writes off the end of the buffer no?  or should aResultLen
be one less than the actual length of aResult?	I think you should use
aResult[aResultLen-1] instead.

I think you should also just compute the length of val->value before trying to
copy it into aResult.  If its length is greater than aResultLen-1, then just
don't bother copying anything and return NS_ERROR_LOSS_OF...
Attachment #192007 - Flags: first-review?(darin) → first-review-

Comment 18

13 years ago
Comment on attachment 192007 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 2

r=darin w/ fix for aResultLen-1 and documentation for the other things.
Attachment #192007 - Flags: first-review- → first-review+
(Assignee)

Comment 19

13 years ago
Created attachment 192021 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 2.1

Comments and array access updated.
Attachment #192007 - Attachment is obsolete: true
Attachment #192021 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #192021 - Flags: approval1.8b4? → approval1.8b4+
someone pointed out to me that there were cvs moves on this bug...  are they
still waiting to happen? and if so, which attachments have the scripts in
question? (there appears to be more than one).  Assign the bug to me when you're
ready for it to happen.

Comment 22

13 years ago
CVS copy completed 12:45 Mountain View local time.

Comment 23

13 years ago
For future reference cc'ing me on this bug would have let me know much sooner
that a CVS move needed to happen since I only found out it was needed two days
ago via a concall.  If I missed a previous ping on IRC about this then I apologize.
(Assignee)

Comment 24

13 years ago
glue and iniparser patches landed... this no longer blocks branching
No longer blocks: 300860
Whiteboard: needs cvsmoves from justdave
(Assignee)

Comment 25

13 years ago
backed out due to various OS bustages... still does not blokck the branch
(Assignee)

Comment 26

13 years ago
Created attachment 192520 [details] [diff] [review]
Remove unneeded -D__STDC__ from intl makefiles, rev. 1

the intl makefiles contain odd windows-specific -D__STDC__ which are leftover
from ftang's original landing in 1999. I can't see what they're there for, and
they broke the nsHashKeys usage of strdup. Removing them.
Attachment #192520 - Flags: first-review?(jshin1987)
(Assignee)

Updated

13 years ago
Attachment #192520 - Flags: second-review?(cls)

Updated

13 years ago
Attachment #192520 - Flags: second-review?(cls) → second-review+

Updated

13 years ago
Attachment #192520 - Flags: first-review?(jshin1987) → first-review+
(Assignee)

Comment 27

13 years ago
Created attachment 192764 [details] [diff] [review]
Open in binary mode, and ignore semicolon comments
Attachment #192764 - Flags: first-review?(shaver)
Comment on attachment 192764 [details] [diff] [review]
Open in binary mode, and ignore semicolon comments

r=shaver
Attachment #192764 - Flags: first-review?(shaver) → first-review+
(Assignee)

Comment 29

13 years ago
I have landed the JS, xpcomglue, iniparser, intl makefiles, and binary mode patches.
Checkin's broke Camino's tinderboxen. Kreeger's working on a patch to fix this
(ObjC treats "id" special, needs to be changed in nsHashKeys.h).

CCing kreeger.

Comment 31

13 years ago
Created attachment 192792 [details] [diff] [review]
Fixes bustage on Camino tboxes

Gets rid of the use of the keyword "id", fixes bustage in the Camino tboxes
(In reply to comment #31)
> Created an attachment (id=192792) [edit]
> Fixes bustage on Camino tboxes
> 
> Gets rid of the use of the keyword "id", fixes bustage in the Camino tboxes

Since apparently the original patches haven't been checked into the 1.8 branch
yet, bsmedberg can just update them instead of applying this patch. In addition,
pink checked in the fix for the trunk and tboxen are no longer red.
(Assignee)

Updated

13 years ago
Attachment #192764 - Flags: approval1.8b4?
(Assignee)

Updated

13 years ago
Attachment #192520 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #192520 - Flags: approval1.8b4? → approval1.8b4+

Updated

13 years ago
Attachment #192764 - Flags: approval1.8b4? → approval1.8b4+
Depends on: 304874

Comment 33

13 years ago
mingw gcc doesn't like the NS_COM declarations in nsHashKeys.h so I'm trying
s/\<NS_COM\>/NS_COM_GLUE/ but I don't know if it's the right fix.

Comment 34

13 years ago
Created attachment 193302 [details] [diff] [review]
Fix mingw bustage
Attachment #193302 - Flags: first-review?(benjamin)
(Assignee)

Comment 35

13 years ago
Comment on attachment 193302 [details] [diff] [review]
Fix mingw bustage

I'm not sure why these are NS_COM *or* NS_COM_GLUE... shouldn't they be inlined
directly? What happens if you remove the NS_COM altogether?
(Assignee)

Comment 36

13 years ago
Created attachment 193846 [details] [diff] [review]
Search for GREs with version ranges and properties, rev. 1

I got darin to do an API review before he left... just needs a code review.
Attachment #193846 - Flags: first-review?(shaver)
(Assignee)

Updated

13 years ago
Blocks: 306334
(Assignee)

Comment 37

13 years ago
Comment on attachment 193846 [details] [diff] [review]
Search for GREs with version ranges and properties, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>+GRE_GetPathFromRegKey(HKEY aRegKey,

>     if (::RegQueryValueEx(subKey, "Version", NULL, NULL,
>                           (BYTE*) version, &versionlen) == ERROR_SUCCESS &&
>-        strncmp(aVersion, version, versionlen) == 0) {
>-      if (::RegQueryValueEx(subKey, "GreHome", NULL, &pathtype,
>-                            (BYTE*) pathbuf, &pathlen) == ERROR_SUCCESS &&
>-          *pathbuf &&
>-          CopyWithEnvExpansion(aBuffer, pathbuf, aBufLen, pathtype) &&
>-          access(aBuffer, R_OK) == 0) {
>-        RegCloseKey(subKey);
>-        return PR_TRUE;
>+        CheckVersion(version, versions, versionsLength) == 0) {

The == 0 is wrong here, since CheckVersion returns PRBool... using the result
directly is what I wanted.

--BDS
(Assignee)

Updated

13 years ago
Attachment #193846 - Flags: first-review?(shaver) → first-review?(darin)

Comment 38

13 years ago
Comment on attachment 193846 [details] [diff] [review]
Search for GREs with version ranges and properties, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>+#include "nsXPCOMPrivate.h"
>+
>+
> #include <stdio.h>

nit: bogus extra newline ?


>+GRE_FindGREFramework(const char* rootPath,
>+                     const GREVersionRange *versions,
...
>+  DIR *dir = opendir(buffer);
>+  if (dir) {
>+    struct dirent *entry;
>+    while (!found && (entry = readdir(dir))) {
>+      if (CheckVersion(entry->d_name, versions, versionsLength)) {
>+        snprintf(buffer, buflen,
>+                 "%s/Library/Frameworks/" GRE_FRAMEWORK_NAME
>+                 "/Versions/%s/" XPCOM_DLL, rootPath, entry->d_name);
>+        if (access(buffer, R_OK | X_OK) == 0)
>+          found = PR_TRUE;

I'm curious... so, it seems like we just return the first
GRE that satisfies the version requirements.  We don't 
necessarily find the most recent GRE, which would seem
like a desirable behavior to me.  This would probably
require sorting the list of directory names, and I can
imagine that being a bit of extra code.  You should at
least comment somewhere (in the API docs) that the GRE
is chosen in no particular order.


>Index: xpcom/glue/standalone/nsGREDirServiceProvider.cpp

>+  static const GREVersionRange version[] = {
>+    GRE_BUILD_ID, PR_TRUE,
>+    GRE_BUILD_ID, PR_TRUE
>+  };

The array notation seems a bit screwy here.  I know that GCC and MSVC
are happy with this, and perhaps it is part of the C++ standard, but
I would expect to see one of the following forms instead:

  static const GREVersionRange version = {
    GRE_BUILD_ID, PR_TRUE,
    GRE_BUILD_ID, PR_TRUE
  };

  static const GREVersionRange version[] = {
    { GRE_BUILD_ID, PR_TRUE,
      GRE_BUILD_ID, PR_TRUE }
  };

I think either of these would be more clear to the reader.


>Index: xpcom/glue/standalone/nsXPCOMGlue.h

>+ * @param properties       A null-terminated list of GRE property/value pairs
>+ *                         which must all be satisfied.

How does one learn about the set of properties that may be passed
in this parameter?  Maybe we should have some #define's in this
file for the property names (and values).


>Index: xulrunner/stub/nsXULStubOSX.cpp

>+  char maxVersion[VERSION_MAXLEN] = "2";
...
>+  rv = parser.GetString("Gecko", "MaxVersion", maxVersion, sizeof(maxVersion));
>+  if (NS_SUCCEEDED(rv))
>+    range.upperInclusive = PR_TRUE;

So, if the XUL app does not specify a max version, then you're
going to restrict it to Gecko 2 or earlier?  Some comments would
be good to explain this so that we update this properly when we
ever start shipping a Gecko 2.x.


r=darin
Attachment #193846 - Flags: first-review?(darin) → first-review+
(Assignee)

Updated

13 years ago
Blocks: 306688
(Assignee)

Comment 39

13 years ago
> necessarily find the most recent GRE, which would seem
> like a desirable behavior to me.  This would probably

Filed bug 306688 on this (1.9 fodder).

> How does one learn about the set of properties that may be passed
> in this parameter?  Maybe we should have some #define's in this

I'll do this with the xulrunner stub (bug 306689).


> So, if the XUL app does not specify a max version, then you're
> going to restrict it to Gecko 2 or earlier?  Some comments would
> be good to explain this so that we update this properly when we
> ever start shipping a Gecko 2.x.

I don't think we should update it; I'll add a basic explanatory comment. If you
don't specify a maxversion I'm going to assume that you mean that your app only
uses frozen APIs, and our frozen APIs are only guaranteed for the toolkit 1.x
lifetime.
No longer blocks: 306688
(Assignee)

Updated

13 years ago
Blocks: 306688
(Assignee)

Comment 40

13 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Attachment #193846 - Flags: approval1.8b5?
Attachment #193846 - Flags: approval1.8b4?

Comment 41

13 years ago
ben, can you tell us a bit more about why we need this and what is the risk?
(Assignee)

Comment 42

13 years ago
This is needed to make xulrunner embedding functional on linux in the presence
of mixed XULRunner and seamonkey GREs. It should have no impact on the aviary
apps, which do not use GRE lookups.

Updated

13 years ago
Attachment #193846 - Flags: approval1.8b5?
Attachment #193846 - Flags: approval1.8b4?
Attachment #193846 - Flags: approval1.8b4+
(Assignee)

Comment 43

13 years ago
I introduced an API inconsistency here: sometimes this function returns the path
with the DLLname, and sometimes not.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 44

13 years ago
Created attachment 195106 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1
Attachment #195106 - Flags: first-review?(darin)

Comment 45

13 years ago
Comment on attachment 195106 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>+  strncat(c->pathBuffer, "/" XPCOM_DLL, c->buflen - strlen(c->pathBuffer));
>+  if (access(c->pathBuffer, R_OK))
>+    return PR_TRUE;

Hmm... |strncat| does not promise to null terminate its first argument.  I
think you should be careful not to pass garbage to |access|.


>+        strncat(aBuffer, "\\" XPCOM_DLL, aBufLen - strlen(aBuffer));
>+        if (access(aBuffer, R_OK)) {

ditto


How about writing a subroutine that appends a string to a buffer?
Attachment #195106 - Flags: first-review?(darin) → first-review-
(Assignee)

Comment 46

13 years ago
Created attachment 196150 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.1
Attachment #195106 - Attachment is obsolete: true
Attachment #196150 - Flags: first-review?(darin)

Comment 47

13 years ago
Comment on attachment 196150 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.1

>Index: xpcom/glue/nsGREGlue.cpp

>+    ++dest, ++append;

cute


>+  if (*append)
>+    return PR_FALSE;
>+
>+  return PR_TRUE;

    return *append == '\0';


>+      else {
>+        if (!safe_strncat(aBuffer, "\\" XPCOM_DLL, aBufLen) ||
>+            access(aBuffer, R_OK)) {
>+          ok = PR_FALSE;
>+        }
>+      }

looks like you could reduce one level of indenting here by making
it an "else if" instead.


r=darin
Attachment #196150 - Flags: first-review?(darin) → first-review+
(Assignee)

Comment 48

13 years ago
Created attachment 196204 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.2

Final DLL-fix patch as checked in on trunk.
Attachment #196150 - Attachment is obsolete: true
Attachment #196204 - Flags: approval1.8b5?
(Assignee)

Comment 49

13 years ago
Fixed on trunk, this needs to make branch also.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Flags: blocking1.8b5+
Resolution: --- → FIXED

Comment 50

13 years ago
Created attachment 196216 [details] [diff] [review]
Alternate mingw bustage fix (checked in trunk and branch)
Attachment #196216 - Flags: first-review?(benjamin)
(Assignee)

Updated

13 years ago
Attachment #196216 - Flags: first-review?(benjamin)
Attachment #196216 - Flags: first-review+
Attachment #196216 - Flags: approval1.8b5?
(Assignee)

Updated

13 years ago
Attachment #193302 - Flags: first-review?(benjamin)

Comment 51

13 years ago
BSmedberg, can you tell us more about the risk here, especially any risk to
Firefox or Thunderbird?
I think this caused bug 308838.

Comment 53

13 years ago
Comment on attachment 196204 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.2

please re-request approvals when all regressions have been resolved.
Attachment #196204 - Flags: approval1.8b5?

Updated

13 years ago
Attachment #196216 - Flags: approval1.8b5?

Updated

13 years ago
Attachment #196216 - Flags: second-review?(darin)

Comment 54

13 years ago
Comment on attachment 196216 [details] [diff] [review]
Alternate mingw bustage fix (checked in trunk and branch)

r=darin
Attachment #196216 - Flags: second-review?(darin) → second-review+

Updated

13 years ago
Attachment #196216 - Flags: approval1.8b5?

Comment 55

13 years ago
CCing myself to watch for approval on attachment 196216 [details] [diff] [review] (checked in) :-/

Updated

13 years ago
Attachment #196216 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Updated

13 years ago
Depends on: 308838

Updated

13 years ago
Flags: blocking1.8b5+ → blocking1.8b5-
(Assignee)

Comment 56

13 years ago
Don't know why this got minused, but it's essential for embedding.
Flags: blocking1.8b5- → blocking1.8b5?
(Assignee)

Comment 57

13 years ago
Comment on attachment 196204 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.2

regression in bug 308838 needs to land with this (neither will affect toolkit
apps)
Attachment #196204 - Flags: approval1.8b5?

Updated

13 years ago
Attachment #196204 - Flags: approval1.8b5? → approval1.8b5+

Updated

13 years ago
Flags: blocking1.8b5? → blocking1.8b5+
(Assignee)

Comment 58

13 years ago
All fixed, with bug 308838.
Keywords: fixed1.8

Updated

13 years ago
Attachment #196216 - Attachment description: Alternate mingw bustage fix → Alternate mingw bustage fix (checked in trunk and branch)
The files under mozilla/xulrunner were missed when this was landed on the 1.8
branch.  I just checked them in.

Comment 60

13 years ago
Hmmpphhh .. fixes from here seem to break compile with Sun Studio:

"/export/stuff/mozilla/ff15b2/mozilla/xpcom/glue/nsGREGlue.cpp", line 472:
Error: A class with a reference member must have a user-defined constructor.
"/export/stuff/mozilla/ff15b2/mozilla/xpcom/glue/nsGREGlue.cpp", line 523:
Error: Expected an expression.
"/export/stuff/mozilla/ff15b2/mozilla/xpcom/glue/nsGREGlue.cpp", line 529:
Error: Use ";" to terminate statements.3 Error(s) detected.
gmake[4]: *** [nsGREGlue.o] Error 3
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.