[webkit-reviews] review denied: [Bug 52609] Add navigator.registerProtocolHandler behind a flag. : [Attachment 80604] Patch

Sun Jan 30 18:23:49 PST 2011

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied James Kozianski
<koz at chromium.org>'s request for review:
Bug 52609: Add navigator.registerProtocolHandler behind a flag.
https://bugs.webkit.org/show_bug.cgi?id=52609

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80604&action=review

Mostly style nits and the bit about xcconfig synchronization.

> LayoutTests/fast/dom/registerProtocolHandler.html:1
> +<html>

Typically test file names are dash-separated.html

> LayoutTests/fast/dom/registerProtocolHandler.html:8
> + }

Indentation oopsie.

> LayoutTests/fast/dom/registerProtocolHandler.html:10
> +function debug(str) {

Function braces start on new line (here and elsewhere).

> LayoutTests/fast/dom/registerProtocolHandler.html:16
> +if(window.navigator.registerProtocolHandler) {

Don't need extra braces for single-line statements (here and elsewhere).

> Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:126
> +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS)
$(ENABLE_WEBGL) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB)
$(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION)
$(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST)
$(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE)
$(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM)
$(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE)
$(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH)
$(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG)
$(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS)
$(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER)
$(ENABLE_SHARED_WORKERS) $(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_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML)
$(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT);

See the comment at the top of the file about keeping this value in sync with
all other FeatureDefines.xcconfig files.