[Webkit-unassigned] [Bug 147602] [INTL] Implement Intl.NumberFormat.prototype.resolvedOptions ()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147602

--- Comment #11 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
Thanks for the review

(In reply to comment #10)
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:87
> > +    unsigned m_minimumIntegerDigits;
> > +    unsigned m_minimumFractionDigits;
> > +    unsigned m_maximumFractionDigits;
> 
> These need initializers.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:91
> > +    bool m_useGrouping;
> 
> And this.

The only functions that read these attributes are Intl.NumberFormat.prototype.resolvedOptions and (in the upcoming patch) Intl.NumberFormat.prototype.format. They always check that the NumberFormat object has been initialized. If not, they initialize it. These attributes are always set in the initialization. So, I think it's fine to not have initializers for these. What do you think?

Related question: The only time resolvedOptions() is called, but the NumberFormat object has not been initialized, is when we call "Intl.NumberFormat.prototype.resolvedOptions()". The spec says that "The Intl.NumberFormat prototype object is itself an Intl.NumberFormat instance ... whose internal slots are set as if it had been constructed by the expression Construct(%NumberFormat%)." But I don't want to populate the internal slots of the prototype eagerly, because Intl.NumberFormat.prototype is created when JSGlobalObject is created. Is it fine to do lazy initialization like this?

> > Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:73
> > +    Vector<String> keyLocaleData({
> 
> Can we use std::array here instead? We don't intend to grow or shrink this
> data structure.

The localeData() function is passed to the resolveLocale() function, which expects a function pointer of type Vector<String> (*localeData)(const String&, const String&). I can't change the return type to std::array because vectors of different size are also passed to it.

> > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:156
> > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumIntegerDigits"), jsNumber(nf->minimumIntegerDigits()));
> > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumFractionDigits"), jsNumber(nf->minimumFractionDigits()));
> > +    options->putDirect(vm, Identifier::fromString(&vm, "maximumFractionDigits"), jsNumber(nf->maximumFractionDigits()));
> > +    if (nf->minimumSignificantDigits())
> > +        options->putDirect(vm, Identifier::fromString(&vm, "minimumSignificantDigits"), jsNumber(nf->minimumSignificantDigits()));
> > +    if (nf->maximumSignificantDigits())
> > +        options->putDirect(vm, Identifier::fromString(&vm, "maximumSignificantDigits"), jsNumber(nf->maximumSignificantDigits()));
> > +    options->putDirect(vm, Identifier::fromString(&vm, "useGrouping"), jsBoolean(nf->useGrouping()));
> 
> I think we want these to be CommonIdentifiers so we don't have to keep
> allocating and hashing them.

There are only two places that use these identifiers: the constructor and prototype.resolvedOptions(), which probably won't be called very often. Should I still add these to CommonIdentifiers?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151103/6016feec/attachment.html>