[webkit-reviews] review denied: [Bug 61073] Teach WebKit2 about window.internals : [Attachment 97293] Patch

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Dominic Cooney
<dominicc at chromium.org>'s request for review:
Bug 61073: Teach WebKit2 about window.internals
https://bugs.webkit.org/show_bug.cgi?id=61073

Attachment 97293: Patch
https://bugs.webkit.org/attachment.cgi?id=97293&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=97293&action=review

> Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:31
> -INSTALL_PATH = $(INSTALL_PATH_$(CONFIGURATION));
> -INSTALL_PATH_Debug = @executable_path;
> -INSTALL_PATH_Release = $(INSTALL_PATH_Debug);
> -INSTALL_PATH_Production = /usr/local/lib;
> +INSTALL_PATH = @rpath;

This isn’t correct.  It will prevent the dylib from being copied to
/usr/local/lib during production builds, which would make it impossible for
DumpRenderTree and WebKitTestRunner to find it at build time.

I think to make this work for production we want something like:

LD_DYLIB_INSTALL_NAME = $(LD_DYLIB_INSTALL_NAME_$(CONFIGURATION));
LD_DYLIB_INSTALL_NAME_Debug = @rpath;
LD_DYLIB_INSTALL_NAME_Release = $(LD_DYLIB_INSTALL_NAME_Debug);

This will set the install name to @rpath for Debug and Release and leave it at
its default for Production (e.g., the file path).

I think we can skip setting INSTALL_PATH altogether since I believe
/usr/local/lib is the default if it is unset.

> Tools/DumpRenderTree/mac/Configurations/DumpRenderTree.xcconfig:25
> +LD_RUNPATH_SEARCH_PATHS = "@loader_path/.";

Given the change I suggested above this rpath would be unnecessary for
Production builds.  I’m not sure that it’s useful to complicate this by
conditionalizing our setting of LD_RUNPATH_SEARCH_PATHS though.