Thi will result in a compilation warning which pollutes the build output. In JavaScriptCore/API/JSBase.h instead of just testing for defined(__GNUC__), there should also be a check to exclude the ARM compiler. Essentially what we want here is a test for COMPILER(GCC), but since Platform.h is not visible from JavaScriptCore/API/JSBase.h the logic for testing for COMPILER(GCC) needs to be repeated.
Created attachment 39263 [details] proposed patch
The warning message - that this patch is trying to address - is the following: "\webkit\JavaScriptCore\API\JSObjectRef.h", line 596: Warning: #1207-D: attribute "visibility" ignored JS_EXPORT bool JSObjectSetPrivate(JSObjectRef object, void* data); ^
Comment on attachment 39263 [details] proposed patch Is it really the ARM issue that makes this not support visibility? Or is it version-specific? r=me, but I suspect this is not as good a condition as it could be.
(In reply to comment #3) > (From update of attachment 39263 [details]) > Is it really the ARM issue that makes this not support visibility? Or is it > version-specific? r=me, but I suspect this is not as good a condition as it > could be. Darin, thanks for the review. This is an issue with the compiler syntax. ARM compiler does support visibility declarations (which is different from GCC so the extra test is needed). For now we do not really need the visibility decaration for the ARM ports now so my patch leaves the JS_EXPORT empty for ARM.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 39263 [details] [details]) > > Is it really the ARM issue that makes this not support visibility? Or is it > > version-specific? r=me, but I suspect this is not as good a condition as it > > could be. > > Darin, thanks for the review. > > This is an issue with the compiler syntax. ARM compiler does support visibility > declarations (which is different from GCC so the extra test is needed). For now > we do not really need the visibility decaration for the ARM ports now so my > patch leaves the JS_EXPORT empty for ARM. Who is "we"? Boston Team? Will this affect compiling Qt 4.6 for ARM in general?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 39263 [details] [details] [details]) > > > Is it really the ARM issue that makes this not support visibility? Or is it > > > version-specific? r=me, but I suspect this is not as good a condition as it > > > could be. > > > > Darin, thanks for the review. > > > > This is an issue with the compiler syntax. ARM compiler does support visibility > > declarations (which is different from GCC so the extra test is needed). For now > > we do not really need the visibility decaration for the ARM ports now so my > > patch leaves the JS_EXPORT empty for ARM. > > Who is "we"? Boston Team? Will this affect compiling Qt 4.6 for ARM in general? Kenneth, sorry for the vague use of words. The change only has an impact for the ARMCC compiler (also known as ADS or RVCT). As far as I know only the Symbian port using the RVCT compiler, although in theory other ports/platforms might be able to use it as well. This change has no impact on those ARM ports (including the QtWebKit ARM ports) that are using GCC.
Added Kent Hansen and Simon Hausmann to the CC list and set commit-queue- to give a change for Kent and Simon to look at this patch.
Committed as http://trac.webkit.org/changeset/48251