[webkit-reviews] review denied: [Bug 49414] Implement ECMAScript I18N APIs (proposed) : [Attachment 78450] patch updated to the new source tree structure

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jungshik Shin
<jshin at chromium.org>'s request for review:
Bug 49414: Implement ECMAScript I18N APIs (proposed)
https://bugs.webkit.org/show_bug.cgi?id=49414

Attachment 78450: patch updated to the new source tree structure 
https://bugs.webkit.org/attachment.cgi?id=78450&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78450&action=review

> Source/WebCore/WebCore.gyp/WebCore.gyp:1404
> +	       'USING_EXPERIMENTAL_I18N_API=1',

I think this needs to be a WTF_ENABLE_ style macro, controlled via
features.gypi (and features_override.gypi).

> Source/WebCore/WebCore.gyp/WebCore.gyp:1498
> +	 'target_name': 'experimental_i18n_api',

It seems like this target should live in v8.gyp.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:65
> +#ifdef USING_EXPERIMENTAL_I18N_API

#if ENABLE(EXPERIMENTAL_I18N_API)

> Source/WebCore/page/Settings.h:356
> +	   void setJSI18NAPIEnabled(bool flag) { m_jsI18NAPIEnabled = flag; }

I think this may be better added to WebRuntimeFeatures since that is how we
enable other, similar features at runtime.  We don't need to vary this flag
on a per-Page basis, so storing this on Settings is not necessary.