332115 - A bunch of symbol-visibility fixups that stuart needs to review in cairo and libpr0n

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Benjamin Smedberg]

This is a catchall patch for a bunch of symbols which are currently exported from a variety of libpr0n and cairo libs that shuoldn't be exported when we're building libxul.

Comment 1

[Benjamin Smedberg]

Attachment: Fix cairo/libpr0n symbol visibility, rev. 1

Comment 2

[Darin Fisher]

Comment on attachment 216642 [details] [diff] [review]
Fix cairo/libpr0n symbol visibility, rev. 1

>Index: gfx/cairo/cairo/src/cairo-platform.h

>+#if defined(MOZ_ENABLE_LIBXUL)
>+
>+#define cairo_public extern
>+#define CCALLBACK
>+#define CCALLBACK_DECL
>+#define CSTATIC_CALLBACK(__x) static __x
>+
>+#else
>+
> #if defined(_MSC_VER)

Seems to me that this could be #elif


>Index: modules/libpr0n/decoders/icon/nsIconURI.cpp

>+#include "stdlib.h"

#include <stdlib.h>


>+static void
>+FillAtoms(AtomStruct* atoms, PRUint32 length)
>+{
>+  nsCOMPtr<nsIAtomService> as(do_GetService(NS_ATOMSERVICE_CONTRACTID));
>+  if (!as)
>+    return;
>+
>+  while (length) {
>+    --length;
>+    as->GetPermanentAtom(NS_ConvertUTF8toUTF16(atoms[length].string).get(), 
>+                         &atoms[length].atom);
>+  }
>+}

Since atoms are internally stored as UTF-8, it seems like nsIAtomService
should be updated to provide a fast-path for UTF-8 atom strings.

>+    char* d = PR_smprintf("%d", mSize);
>+    spec.Append(d);
>+    PR_smprintf_free(d);

For something like an integer conversion, PR_snprintf seems like it
would be a fine/better choice.


>+void extractAttributeValue(const char * searchString, const char * attributeName, nsCString& result)
> {
>   //NS_ENSURE_ARG_POINTER(extractAttributeValue);
> 
>-	char * attributeValue = nsnull;
>+  result.Truncate();
>+
> 	if (searchString && attributeName)
> 	{

Some indentation wierdness in this file.  looks like it is using tabs.


>+					result.Assign(nsDependentCString(startOfAttribute));

Why wrap startOfAttribute with nsDependentCString?  That seems
extraneous to me.


>+    mDummyFilePath.Assign(nsDependentCString(path));

ditto


>+        aFileExtension = NS_LITERAL_CSTRING(".");

          aFileExtension.Append('.');

Comment 3

[Stuart Parmenter]

Comment on attachment 216642 [details] [diff] [review]
Fix cairo/libpr0n symbol visibility, rev. 1

what darin said

Comment 4

[Benjamin Smedberg]

Attachment: Fix cairo/libpr0n symbol visibility, rev. 2

Comment 5

[Darin Fisher]

Comment on attachment 218709 [details] [diff] [review]
Fix cairo/libpr0n symbol visibility, rev. 2

Assign(".") should be Assign('.')

r=darin

Comment 6

[Benjamin Smedberg]

Attachment: Fix cairo/libpr0n symbol visibility, rev. 2.1

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

IMPL_THEBES never seems to get defined by the patch; is it supposed to be auto-defined by some build system bits?  Trying to build with this I get a bunch of:

c:/proj/mozilla-cvs/firefox-cairo/gfx/thebes/src/../../../../mozilla/gfx/thebes/src/gfxWindowsPlatform.cpp(43) : warning C4273: 'mFontList' : inconsistent dll linkage     c:\proj\mozilla-cvs\firefox-cairo\dist\include\thebes\gfxWindowsPlatform.h(74) : see previous definition of 'private: static nsStringArray * gfxWindowsPlatform::mFontList'
c:/proj/mozilla-cvs/firefox-cairo/gfx/thebes/src/../../../../mozilla/gfx/thebes/src/gfxWindowsPlatform.cpp(59) : warning C4273: 'gfxWindowsPlatform::Init' : inconsistent dll linkage        c:\proj\mozilla-cvs\firefox-cairo\dist\include\thebes\gfxWindowsPlatform.h(68) : see previous definition of 'Init'

type errors.  Adding a DEFINES += -DIMPL_THEBES to gfx/thebes/src/Makefile.in seemes to have fixed it; is this the correct fix?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Also (VC8):

c:/proj/mozilla-cvs/firefox-cairo/modules/libpr0n/decoders/icon/../../../../../mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp(230) : error C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
        Conversion loses qualifiers
c:/proj/mozilla-cvs/firefox-cairo/modules/libpr0n/decoders/icon/../../../../../mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp(231) : error C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
        Conversion loses qualifiers

Declaring both variables to be |const char *| instead of |char *| fixes that.

Comment 9

[Benjamin Smedberg]

Yeah, I added DEFINES += -DIMPL_THEBES to the makefile, and I'll fix the const issues if they are still a problem in my tree.

Comment 10

[Benjamin Smedberg]

Attachment: Fix cairo/libpr0n symbol visibility, rev. 2.2

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 219934 [details] [diff] [review]
Fix cairo/libpr0n symbol visibility, rev. 2.2

This builds and works fine for me on windows, but I can't vouch for the nsIconURI/nsIconChannel changes -- they look fine to me, but I'm not an expert in that area.

Comment 12

[Benjamin Smedberg]

Fixed on trunk.