Bug 110137 – Add scroll view for new calendar picker

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Add scroll view for new calendar picker

[Keishi Hattori]

Created attachment 188913 [details]
Patch

[Kent Tamura]

Comment on attachment 188913 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +
> +        No new tests. Code not yet used.

Please mention this is a part of Bug 109439.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:895
> +    this.element.$view = this;

I don't like to add non-standard property to the standard DOM node.  However it would be acceptable if there are no other efficient ways.
Anyway, you should mention this trick in a comment.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:966
> +    this.contentElement = createElement("div", "scroll-view-content");
> +    this.element.appendChild(this.contentElement);
> +    this.minimumContentOffset = -Infinity;
> +    this.maximumContentOffset = Infinity;
> +    this._contentOffset = 0;
> +    this._width = 0;
> +    this._height = 0;
> +    this._scrollAnimator = new Animator();
> +    this._scrollAnimator.step = this.onScrollAnimatorStep;
> +
> +    this.element.addEventListener("mousewheel", this.onMouseWheel, false);
> +
> +    // The content offset is partitioned so the it can go beyond the CSS limit
> +    // of 33554433px.
> +    this._partitionNumber = 0;

* should declare this.delegate in the constructor.
* I recommend to add JsDoc comments to each of data members.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:982
> +    this.element.style.width = width + "px";
> +    if (this._width === width)
> +        return;
> +    this._width = width;
> +    this.element.style.width = this._width + "px";

You update this.element.style.width twice.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1078
> +    if (this.delegate)
> +        this.delegate.scrollViewDidChangeContentOffset(this);
> +    if (partitionChanged) {
> +        if (this.delegate)
> +            this.delegate.scrollViewDidChangePartition(this);
> +    }

You can wrap this block by one "if (this.delegate)"

[Keishi Hattori]

Created attachment 189727 [details]
Patch

[Kent Tamura]

Comment on attachment 189727 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +
> +        No new tests. Code not yet used.

Please mention this is a part of Bug 109439.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:906
> +    // Adding property to DOM node so we can access the View instance from Event.target.

This information should be provided to users of this class. So you should add this comment to the JsDoc comment.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1003
> +     this.delegate = null;

wrong indentation

[Keishi Hattori]

Created attachment 189746 [details]
Patch

[WebKit Review Bot]

Comment on attachment 189746 [details]
Patch

Clearing flags on attachment: 189746

Committed r143723: <http://trac.webkit.org/changeset/143723>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.