[webkit-reviews] review granted: [Bug 42911] Update ruby tools to work with shallow framework bundles : [Attachment 62460] Patch

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 42911: Update ruby tools to work with shallow framework bundles
https://bugs.webkit.org/show_bug.cgi?id=42911

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

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
A few minor comments, primarily stylistic

I think that:
has_shallow_bundle = (ENV['SHALLOW_BUNDLE'] || "NO").upcase == “YES”
would be clearer than:
has_shallow_bundle = !ENV['SHALLOW_BUNDLE'].nil? &&
ENV['SHALLOW_BUNDLE'].upcase == “YES”

You’re using has_shallow_bundle and is_shallow to mean the same thing.	I’d
pick one name and stick with it.  I think “is_shallow_bundle” would be a good
compromise.


 49   all_headers = is_shallow ? `find WebKit.framework/{,Private}Headers -type
f -name '*.h'`.split \
 50			       : `find
WebKit.framework/Versions/A/{,Private}Headers -type f -name '*.h'`.split

Duplicating the entire command here is unfortunate.  I’d suggest something
like:

current_version_path = is_shallow_bundle ? “” : “Versions/A/”
all_headers = `find WebKit.framework/#{current_version_path}{,Private}Headers
-type f -name '*.h'`.split

(Excuse the curly quotes :-/)

Other than that, r=me!