Bug 137973 - Disable Linux-specific code in a Windows build
Summary: Disable Linux-specific code in a Windows build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 137488
  Show dependency treegraph
 
Reported: 2014-10-22 11:31 PDT by Milan Crha
Modified: 2015-04-10 11:56 PDT (History)
5 users (show)

See Also:


Attachments
proposed wk patch (1.92 KB, patch)
2014-10-22 11:32 PDT, Milan Crha
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
proposed wk patch ][ (1.95 KB, patch)
2014-11-01 10:05 PDT, Milan Crha
joepeck: review+
Details | Formatted Diff | Diff
proposed wk patch ]I[ (2.00 KB, patch)
2014-11-03 20:47 PST, Milan Crha
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
proposed wk patch IV (1.98 KB, patch)
2015-04-09 20:15 PDT, Milan Crha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Milan Crha 2014-10-22 11:31:13 PDT
The used functions are not available under windows, but it can be built otherwise, thus, in a price of a lighter functionality, disable the code under Windows. Please let me know, if I missed anything.
Comment 1 Milan Crha 2014-10-22 11:32:34 PDT
Created attachment 240283 [details]
proposed wk patch
Comment 2 Benjamin Poulain 2014-10-27 14:08:29 PDT
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.
Comment 3 Milan Crha 2014-10-29 00:08:39 PDT
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 4 Joseph Pecoraro 2014-10-29 10:51:42 PDT
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.
Comment 5 Milan Crha 2014-11-01 10:05:44 PDT
Created attachment 240788 [details]
proposed wk patch ][

You are right, that works too.
Comment 6 Joseph Pecoraro 2014-11-03 11:01:19 PST
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?
Comment 7 Milan Crha 2014-11-03 20:47:43 PST
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.
Comment 8 Brent Fulgham 2014-12-17 17:23:05 PST
Joe, it looks like the changes you requested are in the patch. Is this okay to take now?
Comment 9 Joseph Pecoraro 2015-04-09 11:35:34 PDT
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.
Comment 10 Milan Crha 2015-04-09 20:15:56 PDT
Created attachment 250497 [details]
proposed wk patch IV

Updated webkit patch with the suggested changes applied. I agree with both.
Comment 11 Milan Crha 2015-04-09 20:25:32 PDT
(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.
Comment 12 Joseph Pecoraro 2015-04-10 10:53:03 PDT
(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 13 Joseph Pecoraro 2015-04-10 10:53:31 PDT
Comment on attachment 250497 [details]
proposed wk patch IV

r=me
Comment 14 WebKit Commit Bot 2015-04-10 11:56:51 PDT
Comment on attachment 250497 [details]
proposed wk patch IV

Clearing flags on attachment: 250497

Committed r182636: <http://trac.webkit.org/changeset/182636>
Comment 15 WebKit Commit Bot 2015-04-10 11:56:56 PDT
All reviewed patches have been landed.  Closing bug.