[webkit-reviews] review denied: [Bug 27206] Add --websocket flag and ENABLE_WEBSOCKET define : [Attachment 32649] Add --websocket flag in build-webkit and ENABLE_WEBSOCKET define.

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 27206: Add --websocket flag and ENABLE_WEBSOCKET define
https://bugs.webkit.org/show_bug.cgi?id=27206

Attachment 32649: Add --websocket flag in build-webkit and ENABLE_WEBSOCKET
define.
https://bugs.webkit.org/attachment.cgi?id=32649&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> diff --git a/WebCore/Configurations/FeatureDefines.xcconfig
b/WebCore/Configurations/FeatureDefines.xcconfig
> index 1f196ea..3dade71 100644
> --- a/WebCore/Configurations/FeatureDefines.xcconfig
> +++ b/WebCore/Configurations/FeatureDefines.xcconfig
> @@ -52,5 +52,6 @@ ENABLE_WML = ;
>  ENABLE_WORKERS = ENABLE_WORKERS;
>  ENABLE_XPATH = ENABLE_XPATH;
>  ENABLE_XSLT = ENABLE_XSLT;
> +ENABLE_WEBSOCKET = ;
>  
> -FEATURE_DEFINES = $(ENABLE_3D_RENDERING) $(ENABLE_CHANNEL_MESSAGING)
$(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DOM_STORAGE) $(ENABLE_FILTERS)
$(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_JAVASCRIPT_DEBUGGER)
$(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION)
$(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS)
$(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WML)
$(ENABLE_WORKERS) $(ENABLE_XPATH) $(ENABLE_XSLT);
> +FEATURE_DEFINES = $(ENABLE_3D_RENDERING) $(ENABLE_CHANNEL_MESSAGING)
$(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DOM_STORAGE) $(ENABLE_FILTERS)
$(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_JAVASCRIPT_DEBUGGER)
$(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION)
$(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS)
$(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WML)
$(ENABLE_WORKERS) $(ENABLE_XPATH) $(ENABLE_XSLT) $(ENABLE_WEBSOCKET);

You need to make the same change to FeatureDefines.xcconfig in the other
locations that it exists (JavaScriptCore and WebKit/mac).  You should also
continue the tradition of keeping this list of variables sorted in alphabetical
order.

> diff --git a/WebKitTools/Scripts/build-webkit
b/WebKitTools/Scripts/build-webkit
> index 9e2f41c..83013dd 100755
> --- a/WebKitTools/Scripts/build-webkit
> +++ b/WebKitTools/Scripts/build-webkit
> @@ -48,7 +48,7 @@ my ($threeDRenderingSupport, $channelMessagingSupport,
$databaseSupport, $domSto
>      $filtersSupport, $geolocationSupport, $gnomeKeyringSupport,
$iconDatabaseSupport,
>      $javaScriptDebuggerSupport, $offlineWebApplicationSupport, $svgSupport,
$svgAnimationSupport,
>      $svgAsImageSupport, $svgDOMObjCBindingsSupport, $svgFontsSupport,
$svgForeignObjectSupport,
> -    $svgUseSupport, $videoSupport, $wmlSupport, $workersSupport,
$xpathSupport, $xsltSupport,
> +    $svgUseSupport, $videoSupport, $wmlSupport, $workersSupport,
$xpathSupport, $xsltSupport, $websocketSupport,
>      $coverageSupport);
>  
>  my @features = (
> @@ -120,6 +120,8 @@ my @features = (
>  
>      { option => "xslt", desc => "Toggle XSLT support",
>	 define => "ENABLE_XSLT", default => 1, value => \$xsltSupport },
> +    { option => "websocket", desc => "Toggle WebSocket support",
> +	 define => "ENABLE_WEBSOCKET", default => 0, value=> \$websocketSupport
},
>  );

Is it "Web Socket", "WebSocket" or "web socket"?  You use different names here
than in configure.ac.  If it's "Web Socket" I would expect the related flag
names to use "web-socket".  If it's the latter, I'd expect "websocket".  I
couldn't find the spec to check this myself, but either way we should be
consistent.

You appear to have missed the related .vcproj changes to apply this to Windows
as well.

r- to address these issues.