[webkit-reviews] review denied: [Bug 95830] Refactor CalendarPicker to not use global variables. : [Attachment 162190] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 95830: Refactor CalendarPicker to not use global variables.
https://bugs.webkit.org/show_bug.cgi?id=95830

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=162190&action=review


> Source/WebCore/Resources/pagepopups/calendarPicker.js:247
> +    if (args.isDatePopup) {

We prefer early return.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:314
> +    this._element.classList.add("calendar-picker");

This change should be explained in ChangeLog.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:321
> +CalendarPicker.prototype._layoutButtons = function() {

Please do not move the function order in the source file.  It makes the review
harder.
You can move the function position by another patch before this patch or after
this patch.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:341
> +CalendarPicker.prototype._handleKeyDown = function(event) {

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:461
> -    this._monthPopup.addEventListener("click",
bind(this._handleYearMonthChange, this), false);
> -    this._monthPopup.addEventListener("keydown",
bind(this._handleMonthPopupKey, this), false);
> -    this._monthPopup.addEventListener("mousemove",
bind(this._handleMouseMove, this), false);
> +    this._monthPopup.addEventListener("click",
this._handleYearMonthChange.bind(this), false);
> +    this._monthPopup.addEventListener("keydown",
this._handleMonthPopupKey.bind(this), false);
> +    this._monthPopup.addEventListener("mousemove",
this._handleMouseMove.bind(this), false);

Can you defer bind(fn,this) -> fn.bind(this) changes to another patch?

> Source/WebCore/Resources/pagepopups/pickerCommon.js:80
> +function Picker(element, config) {

Please add JSDoc annotation.

> Source/WebCore/Resources/pagepopups/pickerCommon.js:91
> +Picker.prototype.submitValue = function(value) {

ditto.