[webkit-reviews] review denied: [Bug 29749] Integrate chromium into update-webkit and build-webkit : [Attachment 40267] Patch with Eric's feedback

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Yaar Schnitman <yaar at chromium.org>'s
request for review:
Bug 29749: Integrate chromium into update-webkit and build-webkit
https://bugs.webkit.org/show_bug.cgi?id=29749

Attachment 40267: Patch with Eric's feedback
https://bugs.webkit.org/attachment.cgi?id=40267&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
OK, this is closer.

Extra space intended?
2 if (isChromium()) {
 323	 
 324	 # Chromium doesn't build by project directories.

You should print instructions on how, or at least print a web url of where
instructions are:
 33	print "gclient is required for updating chromium dependencies.\n";
 34	print "Install depot_tools and add gclient to the environment path.\n";

 35	return 1;

If you ever expect "normal" WebKit folks to be able to use this, you'll need to
make the instructions very very simple.

What does "return 1" do in perl?  I assume you mean exit(1) in those cases?  Or
"die" would probably be better, especially with a die message.

die is probably better:
45     if ($result != 0) {
 46	    print "Error while configuring gclient for chromium port\n";
 47	    return 1;
 48	}

What does this mean?
 1364	  # Chromium port builds using GYP, so it can start right away

Why do you need two else if's?	Why not just an else?

1369	 } elsif (isCygwin()) {
 1370	      # Windows build
 1371	      # FIXME support windows.
 1372	      print "Windows build is not supported. Yet.";
 1373	  } elsif (isLinux()) {
 1374	      # Linux build
 1375	      # FIXME support linux.
 1376	      print "Linux build is not supported. Yet.";
 1377	  }