[webkit-reviews] review denied: [Bug 84935] [Chromium/Windows] Add LocalizedDateWin : [Attachment 138968] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 84935: [Chromium/Windows] Add LocalizedDateWin
https://bugs.webkit.org/show_bug.cgi?id=84935

Attachment 138968: Patch
https://bugs.webkit.org/attachment.cgi?id=138968&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138968&action=review


> Source/WebCore/ChangeLog:17
> +	   No new tests. The behavior depends on system settings.

Landing this patch without tests seems a bit dangerous. Isn't there any way to
make it testable?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:67
> +    buffer.shrink(bufferSizeWithNUL - 1);

Is this needed?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:87
> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);

Shall we put labels->reserveCapacity(WTF_ARRAY_LENGTH(types))?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:91
> +	       labels->shrink(0);

Nit: I would prefer labels->clear() for such cases.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:103
> +    static Vector<String>* labels = createShortMonthLabels().leakPtr();

- Please use DEFINE_STATIC_LOCAL() for a static variable declaration.

- I know this code is correct, but using adoptPtr() => release() => leakPtr()
seems a bit dirty. How about writing like this?

static const Vector<String>& shortMonthLabels()
{
    DEFINE_STATIC_LOCAL(Vector<String>, labels, ());
    if (!labels.isEmpty())
	return labels;

    ...; /* The code in createShortMonthLabels() */
}

> Source/WebCore/platform/text/LocalizedDateWin.cpp:152
> +static unsigned countContinuousLetters(const String& format, unsigned i)

Nit: 'i' => 'index', for clarification? You are using 'index' in
parseNumberXXX().

> Source/WebCore/platform/text/LocalizedDateWin.cpp:184
> +		   if (format[i - 1] == '\'')

Maybe I am missing something, why do we need to handle '' specially? In other
words, why do we need a different logic for '' and 'foo'?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:193
> +	       if (i > 0 && format[i-1] == '\'')

When can this happen?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:227
> +	       // HTML5 date supports only A.D.

Nit: It would be interesting if HTML5 supported B.C.:)

> Source/WebCore/platform/text/LocalizedDateWin.cpp:261
> +	   if (input.substring(index, shortLabels[i].length()) ==
shortLabels[i]) {

What happens if input="Mayyyyyy" and shortLabels[i]="May". This will return 4,
but is it expected?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:268
> +	   if (input.substring(index, labels[i].length()) == labels[i]) {

Ditto.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:293
> +	       if (day < 1 || day > 31)

Just an out-of-curiosity question: Where is the difference of the number of
monthly dates treated? e.g. There are only 30 days in April.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:355
> +static void appendTwoDigitNumber(int value, StringBuilder& buffer)

appendTwoDigit*s*Number()? Dunno.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:362
> +static void appendFourDigitNumber(int value, StringBuilder& buffer)

appendFourDigit*s*Number?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:405
> +	       appendFourDigitNumber(year, buffer);

Maybe I am missing something, but isn't it 'else { appendFourDigitNumber(year,
buffer); }'?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:413
> +	       appendFourDigitNumber(year, buffer);

Ditto.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:508
> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);

You can put labels->reserveCapacity(WTF_ARRAY_LENGTH(types));

> Source/WebCore/platform/text/LocalizedDateWin.cpp:512
> +	       labels->shrink(0);

labels->clear() is better?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:525
> +	       labels->append("January");
> +	       labels->append("February");
> +	       labels->append("March");
> +	       labels->append("April");
> +	       labels->append("May");
> +	       labels->append("June");
> +	       labels->append("July");
> +	       labels->append("August");
> +	       labels->append("September");
> +	       labels->append("October");
> +	       labels->append("November");
> +	       labels->append("December");

Why didn't you use WTF::monthName[i]?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:534
> +    static Vector<String>* labels = createMonthLabels().leakPtr();

The same comment for shortMonthLabels().

> Source/WebCore/platform/text/LocalizedDateWin.cpp:549
> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);

You can put labels.reserveCapacity(WTF_ARRAY_LENGTH(types));

> Source/WebCore/platform/text/LocalizedDateWin.cpp:553
> +	       labels->shrink(0);

labels->clear() is better?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:567
> +    static Vector<String>* labels = createWeekDayShortLabels().leakPtr();

The same comment for shortMonthLabels().

> Source/WebCore/platform/text/LocalizedDateWin.cpp:582
> +    static unsigned firstDayOfWeek = createFirstDayOfWeek();

Please use DEFINE_STATIC_LOCAL().