[webkit-reviews] review denied: [Bug 101024] Introduce Month class to calendar picker : [Attachment 172002] 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 101024: Introduce Month class to calendar picker
https://bugs.webkit.org/show_bug.cgi?id=101024

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

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


Nice refactoring.  I have some comments.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:146
> + Month.prototype.toLocaleString = function() {

Need to update the jsdoc annotation above.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:214
> +function Month(valueOrMonthOrYear, month) {

Please add jsdoc annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:223
> +	   this.year = Math.floor(arguments[0] / 12) + 1970;
> +	   this.month = arguments[0] % 12;

nit: using arguments[0] is inconsistent.  valueOrMonthOrYear is better for
consistency.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:242
> +Month.parse = function(str) {

Please add jsdoc annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:251
> +Month.containing = function(date) {

nit: The name "containing" looks not good.  createFromDate?

> Source/WebCore/Resources/pagepopups/calendarPicker.js:267
> +Month.prototype.minimum = function() {

nit: not good name.  createMInimumDate?

> Source/WebCore/Resources/pagepopups/calendarPicker.js:271
> +Month.prototype.maximum = function() {

Ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:629
> + * @return {?month}

nit: month -> Month

> Source/WebCore/Resources/pagepopups/calendarPicker.js:639
>   * @param {!number} year
>   * @param {!number} month
>   */
> -YearMonthController.prototype.setYearMonth = function(year, month) {
> -    this._currentYear = year;
> -    this._currentMonth = month;
> +YearMonthController.prototype.setMonth = function(month) {

Update the jsdoc annotation please.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:990
>   * @param {!number} year
>   * @param {!number} month
>   */
> -DaysTable.prototype._navigateToMonth = function(year, month) {
> -    this.picker.yearMonthController.setYearMonth(year, month);
> -    this._renderMonth(year, month);
> +DaysTable.prototype._navigateToMonth = function(month) {

Need to update the jsdoc annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:999
>   * @param {!number} year
>   * @param {!number} month
>   */
> -DaysTable.prototype._navigateToMonthWithAnimation = function(year, month) {
> -    if (this._currentYear >= 0 && this._currentMonth >= 0) {
> -	   if (year == this._currentYear && month == this._currentMonth)
> +DaysTable.prototype._navigateToMonthWithAnimation = function(month) {

Ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1028
>   * @param {!number} year
>   * @param {!number} month
>   */
> -DaysTable.prototype.navigateToMonthAndKeepSelectionPosition = function(year,
month) {
> -    if (year == this._currentYear && month == this._currentMonth)
> +DaysTable.prototype.navigateToMonthAndKeepSelectionPosition =
function(month) {
> +    if (this._currentMonth.equals(month))

Ditto.