Bug 97899

Summary:

Clean up Localizer-related functions

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Platform

Assignee:

Kent Tamura <tkent>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, haraken, mifenton, morrita, rwlbuis, webkit.review.bot, yutak

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

96351

Attachments:

Description	Flags
Patch	none
Patch 2	haraken: review+

Kent Tamura

Reported 2012-09-28 05:28:45 PDT

Clean up Localizer-related functions

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 attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 2012-09-28 05:38:58 PDT

Created attachment 166227 [details] Patch

Kentaro Hara

Comment 2 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.

Kent Tamura

Comment 3 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.

Kentaro Hara

Comment 4 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.

Kent Tamura

Comment 5 2012-09-28 06:11:52 PDT

Created attachment 166235 [details] Patch 2

Kentaro Hara

Comment 6 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

Kent Tamura

Comment 7 2012-09-28 08:52:09 PDT

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

Note You need to log in before you can comment on or make changes to this bug.