[webkit-reviews] review denied: [Bug 52609] Add navigator.registerProtocolHandler behind a flag. : [Attachment 79241] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 17 22:40:05 PST 2011
David Levin <levin 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 79241: Patch
https://bugs.webkit.org/attachment.cgi?id=79241&action=review
------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79241&action=review
I think you'll also need to put it behind a runtime flag for chromium but that
can (and likely should be saved) for another patch (to keep this one smaller).
> Source/WebCore/ChangeLog:12
> + behind a flag so as not to break JS feature detection.
Hurray!
> Source/WebCore/ChangeLog:14
> + No new tests. (OOPS!)
This should either point out the test or explain why none is possible.
> Source/WebCore/page/ChromeClient.h:139
> + virtual void registerProtocolHandler(const String&, const String&,
const String&, const String&) { }
I think this should be = 0 and be behind the if ENABLE (and have the parameter
names filled in).
> Source/WebCore/page/Navigator.cpp:28
> +#include "ExceptionCode.h"
Out of order.
> Source/WebCore/page/Navigator.cpp:178
> + // The specification requires that it is a SYNTAX_ERR if the the "%s"
token
"the the"
>> Source/WebCore/page/Navigator.cpp:190
>> + newURL.remove(index, sizeof(token) / sizeof(token[0]));
>
> sizeof(token) - 1, right?
Also use WTF_ARRAY_LENGTH (if possible). (It may not be possible due to the
variable begin a static const local.)
> Source/WebCore/page/Navigator.h:66
> + void registerProtocolHandler(const String& scheme, const String&
url, const String& title, ExceptionCode&);
This appears to be indented too far.
> Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:130
> +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS)
$(ENABLE_3D_CANVAS) $(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);
It looks like something went wrong here. This diff doesn't look right.
More information about the webkit-reviews
mailing list