Bug 95830 – Refactor CalendarPicker to not use global variables.

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

Refactor CalendarPicker to not use global variables so we can introduce multiple pickers into one page popup.

[Keishi Hattori]

Created attachment 162190 [details]
Patch

[Kent Tamura]

Comment on attachment 162190 [details]
Patch

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.

[Keishi Hattori]

Created attachment 162215 [details]
Patch

[Keishi Hattori]

Comment on attachment 162190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162190&action=review

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:314
>> +    this._element.classList.add("calendar-picker");
> 
> This change should be explained in ChangeLog.

I removed this for now. I will add it later when I will be using it.

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

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:461
>> +    this._monthPopup.addEventListener("mousemove", this._handleMouseMove.bind(this), false);
> 
> Can you defer bind(fn,this) -> fn.bind(this) changes to another patch?

Done.

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

Done.

[Kent Tamura]

Comment on attachment 162215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162215&action=review

> Source/WebCore/Resources/pagepopups/calendarPicker.js:78
> +    minimumDate: new Date(-62135596800000.0),
> +    maximumDate: new Date(8640000000000000.0)

Are these needed? CalendarPIcker constructor always sets values.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:298
> + * @param {?Element} element

I don't think element is nullable.  should be {!Element}.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:366
> +    var MainPadding = 6; // TODO: Fix name.

TODO: -> FIXME:

> Source/WebCore/Resources/pagepopups/calendarPicker.js:404
>   */
> -function YearMonthController() {
> +function YearMonthController(picker) {

Please add annotation for 'picker' argument.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:771
>   */
> -function DaysTable() {
> +function DaysTable(picker) {

ditto.

[Keishi Hattori]

Created attachment 162409 [details]
Patch

[Kent Tamura]

Comment on attachment 162409 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 162409 [details]
Patch

Clearing flags on attachment: 162409

Committed r127727: <http://trac.webkit.org/changeset/127727>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.