Bug 97899 - Clean up Localizer-related functions
Summary: Clean up Localizer-related functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 96351
  Show dependency tree / graph
 
Reported: 2012-09-28 05:28 PDT by Kent Tamura
Modified: 2012-09-28 08:52 PDT (History)
7 users (show)

See Also:

Attachments
Patch (15.09 KB, patch)
2012-09-28 05:38 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Patch 2 (15.24 KB, patch)
2012-09-28 06:11 PDT, Kent Tamura 		haraken: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-09-28 05:28:45 PDT 
Clean up Localizer-related functions
Comment 1 Kent Tamura 2012-09-28 05:38:58 PDT 
Created attachment 166227 [details]
Patch
Comment 2 Kentaro Hara 2012-09-28 05:54:38 PDT 
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.
Comment 3 Kent Tamura 2012-09-28 06:06:21 PDT 
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.
Comment 4 Kentaro Hara 2012-09-28 06:07:57 PDT 
(In reply to comment #3)
> However, adding the default argument to getCachedLocalizer() would make sense.

That sounds better.
Comment 5 Kent Tamura 2012-09-28 06:11:52 PDT 
Created attachment 166235 [details]
Patch 2
Comment 6 Kentaro Hara 2012-09-28 06:13:57 PDT 
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
Comment 7 Kent Tamura 2012-09-28 08:52:09 PDT 
Committed r129903: <http://trac.webkit.org/changeset/129903>