Bug 97899 – Clean up Localizer-related functions

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kent Tamura]

Clean up Localizer-related functions

[Kent Tamura]

Created attachment 166227 [details]
Patch

[Kentaro Hara]

Comment on attachment 166227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166227&action=review

> Source/WebCore/ChangeLog:12
> +        - Add Localizer::createDefault to improvde code readability

Nit: improvde => improve

> Source/WebCore/platform/text/Localizer.h:39
> +    static PassOwnPtr<Localizer> create(const AtomicString& localeIdentifier);

Nit: localeIdentifier might not be needed. It's up to you.

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:88
> +    return m_element->document()->getCachedLocalizer(nullAtom);

Given that you're introducing Element::localizer() as a short path for document()->getCachedLocalizer(computeInheritedLanguage()), you might also want to introduce a short path for m_element->document()->getCachedLocalizer(nullAtom). Maybe you can introduce Element::localizer(const AtomicString& locale = nullAtom)?

> Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.cpp:123
> +    return m_element->document()->getCachedLocalizer(nullAtom);

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:155
> +    return m_element->document()->getCachedLocalizer(nullAtom);

Ditto.

[Kent Tamura]

Comment on attachment 166227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166227&action=review

>> Source/WebCore/ChangeLog:12
>> +        - Add Localizer::createDefault to improvde code readability
> 
> Nit: improvde => improve

will fix

>> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:88
>> +    return m_element->document()->getCachedLocalizer(nullAtom);
> 
> Given that you're introducing Element::localizer() as a short path for document()->getCachedLocalizer(computeInheritedLanguage()), you might also want to introduce a short path for m_element->document()->getCachedLocalizer(nullAtom). Maybe you can introduce Element::localizer(const AtomicString& locale = nullAtom)?

I object it because a Localizer with nullAtom is not a object associated to this Element.
However, adding the default argument to getCachedLocalizer() would make sense.

[Kentaro Hara]

(In reply to comment #3)
> However, adding the default argument to getCachedLocalizer() would make sense.

That sounds better.

[Kent Tamura]

Created attachment 166235 [details]
Patch 2

[Kentaro Hara]

Comment on attachment 166235 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=166235&action=review

Looks OK

> Source/WebCore/dom/Document.h:1163
> +    // Return a Localizer for the default locale if the argumetn is null or empty.

Typo: argument

[Kent Tamura]

Committed r129903: <http://trac.webkit.org/changeset/129903>