[webkit-reviews] review denied: [Bug 110140] Add list view for new calendar picker : [Attachment 188916] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura (ooo until Mar 15) <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 110140: Add list view for new calendar picker
https://bugs.webkit.org/show_bug.cgi?id=110140

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

------- Additional Comments from Kent Tamura (ooo until Mar 15)
<tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188916&action=review


> Source/WebCore/Resources/pagepopups/calendarPicker.js:1097
> +    this.row = null;
> +    this._width = 0;
> +    this._position = 0;

Data members should have JsDoc comments in the constructor.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1105
> + * @return {!Array}

Array of what?

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1116
> +    if (this._recycleBin().length < limit)
> +	   this._recycleBin().push(this);

this._recycleBin() should be stored to a local variable.  Calling it twice
makes unnecessary constraint.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1128
> + * @return {!number}

should mention it's in pixel unit.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1135
> + * @param {!number} width

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1145
> + * @return {!number}

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1152
> + * @param {!number} y

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1187
> +    this._width = 0;
> +    this._cells = {};
> +    
> +    this.selectedRow = ListView.NoSelection;
> +
> +    this.scrollView = new ScrollView();

Data members should have JsDoc comments in the constructor.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1196
> +    this._needsUpdateCells = false;

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1201
> +ListView.NoSelection = -1;

Should this be public?

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1218
> +	   AnimationManager.shared.on("animationFrameWillFinish",
this.onAnimationFrameWillFinish);
> +    else
> +	   AnimationManager.shared.removeListener("animationFrameWillFinish",
this.onAnimationFrameWillFinish);

Use AnimationManager.EventTypeAnimationFrameWillFinish

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1357
> + * @return {!number}

mention it's in pixel unit.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1364
> + * @param {!number} height

ditto.