Bug 137973

Created attachment 240283 [details] proposed wk patch

Comment on attachment 240283 [details] proposed wk patch I don't really see the point. We optimize binary size for ARM, it does not really matter for Windows. If there is no negative side effect, we can leave the code as it is. Adding #ifdefs is adding complexity to the codebase.

The functions being "commented out" are Linux specific, they are not available under windows (mine mingw/msys build environment). Having them available/enabled is not about binary size, it's about being able to build the source file at all, for a price of a less functionality being provided in the windows builds (no stacktraces).

Comment on attachment 240283 [details] proposed wk patch View in context: https://bugs.webkit.org/attachment.cgi?id=240283&action=review > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:47 > +#if !PLATFORM(WIN) In wtf/Assertions.cpp they use: #if OS(DARWIN) || (OS(LINUX) && !defined(__UCLIBC__)) ... #endif I would be fine with doing that here.

Created attachment 240788 [details] proposed wk patch ][ You are right, that works too.

Comment on attachment 240788 [details] proposed wk patch ][ View in context: https://bugs.webkit.org/attachment.cgi?id=240788&action=review > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:172 > +#endif Do you also need: #else UNUSED_PARAM(callStack); #endif To avoid other warnings?

Created attachment 240906 [details] proposed wk patch ]I[ Nice, I didn't know you've a macro for it. I noticed the warning, but there are shown more other from various places when I compile the code on Windows, thus I didn't pay attention to it. My fault, I'm sorry for that.

Joe, it looks like the changes you requested are in the patch. Is this okay to take now?

Comment on attachment 240906 [details] proposed wk patch ]I[ View in context: https://bugs.webkit.org/attachment.cgi?id=240906&action=review > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:152 > +#if OS(DARWIN) || (OS(LINUX) && !defined(__UCLIBC__)) Based on wtf/Assertions.cpp, which has: #if OS(DARWIN) || OS(LINUX) # if PLATFORM(GTK) # if defined(__GLIBC__) && !defined(__UCLIBC__) # define WTF_USE_BACKTRACE_SYMBOLS 1 # endif # else # define WTF_USE_DLADDR 1 # endif #endif This should probably be: #if OS(DARWIN) || (OS(LINUX) && !PLATFORM(GTK)) Same up above in the includes. Do you agree? > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:177 > + UNUSED_PARAM(callStack); Nit: Too much indentation.

Created attachment 250497 [details] proposed wk patch IV Updated webkit patch with the suggested changes applied. I agree with both.

(In reply to comment #9) > This should probably be: > > #if OS(DARWIN) || (OS(LINUX) && !PLATFORM(GTK)) > > Same up above in the includes. Do you agree? Hmm, thinking of it, this will disable backtraces for any GTK platform build, not only for those which do not support it, like those not having dlfcn.h and execinfo.h.

(In reply to comment #10) > Created attachment 250497 [details] > proposed wk patch IV > > Updated webkit patch with the suggested changes applied. I agree with both. (In reply to comment #11) > (In reply to comment #9) > > This should probably be: > > > > #if OS(DARWIN) || (OS(LINUX) && !PLATFORM(GTK)) > > > > Same up above in the includes. Do you agree? > > Hmm, thinking of it, this will disable backtraces for any GTK platform > build, not only for those which do not support it, like those not having > dlfcn.h and execinfo.h. I believe this code is only reachable through ENABLE_REMOTE_INSPECTOR so it really doesn't matter ;)

Comment on attachment 250497 [details] proposed wk patch IV r=me

Comment on attachment 250497 [details] proposed wk patch IV Clearing flags on attachment: 250497 Committed r182636: <http://trac.webkit.org/changeset/182636>

All reviewed patches have been landed. Closing bug.