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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 14 14:27:08 PDT 2011


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 97065: Patch
https://bugs.webkit.org/attachment.cgi?id=97065&action=review

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

Marking as r- because at the very least the Xcode project changes in this patch
aren’t right.  I’d suggest investigating the approach using the path that I
outlined which would make some of the comments obsolete, but there are some
issues that would need to be addressed either way.

> Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:98
> +		4181731B138AD39D0057AAA4 /* WebCoreTestSupport.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name =
WebCoreTestSupport.h; path =
../../Source/WebCore/testing/js/WebCoreTestSupport.h; sourceTree = SOURCE_ROOT;
};

It doesn’t make sense to refer to a source file that lives above the root of
the project.  WebCoreTestSupport.h should be found relative to
BUILT_PRODUCTS_DIR like it is in DumpRenderTree.xcodeproj.

> Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:571
> +				LIBRARY_SEARCH_PATHS = (
> +					"$(inherited)",
> +				       
"\"$(SRCROOT)/../../WebKitBuild/Debug\"",
> +				);

It’s never correct to hard-code paths to your build directory in the Xcode
project.

> Tools/WebKitTestRunner/mac/run-install-name-tool.sh:31
> +install_name_tool -change '@executable_path/libWebCoreTestSupport.dylib'
'@loader_path/libWebCoreTestSupport.dylib'
"${BUILT_PRODUCTS_DIR}/${EXECUTABLE_PATH}"

This is an incredibly roundabout approach.  @executable_path resolves to the
path to the main executable of the process.  @loader_path resolves to the path
to the image that is loading the library.  The latter is almost always what is
desired, and it would obviously give the correct behavior for both
DumpRenderTree and the injected bundle if the dylib is going to live alongside
the image that loads it.  In other words, it would be more sensible to change
the install path at build time in WebCore.xcodeproj.


More information about the webkit-reviews mailing list