[webkit-reviews] review denied: [Bug 73382] split webkit gyp files for chromium build to break the circular dependency : [Attachment 117091] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 73382: split webkit gyp files for chromium build to break the circular
dependency
https://bugs.webkit.org/show_bug.cgi?id=73382

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117091&action=review


r- for bot failures

> Source/WebKit/chromium/All.gyp:42
> +		   'WebKit.gyp:*',
> +		   'WebKitUnitTests.gyp:*',
> +		   '../../../Tools/Tools.gyp:*',

Is it sufficient to just depend on webkit_unittests and DumpRenderTree?  That
way we can be more confident that the other dependencies are correct.

> Source/WebKit/chromium/WebKit.gyp:1100
> +	   # https://bugs.webkit.org/show_bug.cgi?id=68463. Until then, we
> +	   # leave the indentation alone to minimize whitespace diffs.

I would go ahead and indent. PrettyPatch should be smart enough to show that
it's just an indention change.

> Source/WebKit/chromium/WebKitUnitTests.gyp:35
> +    # https://bugs.webkit.org/show_bug.cgi?id=68463. Until then, we
> +    # leave the indentation alone to minimize whitespace diffs.

I would indent this properly too.

> Source/WebKit/chromium/gyp_webkit:136
> +  # Change to the base dir needed for the gyp file path above to work
correctly.
> +  os.chdir(script_dir)

This failed on the bots since when gclient runs it, it's already in the
script_dir.

> Tools/Tools.gyp:35
> +    # https://bugs.webkit.org/show_bug.cgi?id=68463. Until then, we
> +    # leave the indentation alone to minimize whitespace diffs.

Same as above.