Bug 110137

Summary: Add scroll view for new calendar picker
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110132    
Bug Blocks: 109439, 110140    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Keishi Hattori 2013-02-18 09:47:44 PST
Add scroll view for new calendar picker
Comment 1 Keishi Hattori 2013-02-18 10:00:47 PST
Created attachment 188913 [details]
Patch
Comment 2 Kent Tamura 2013-02-21 06:08:55 PST
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)"
Comment 3 Keishi Hattori 2013-02-22 02:30:09 PST
Created attachment 189727 [details]
Patch
Comment 4 Kent Tamura 2013-02-22 04:03:13 PST
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
Comment 5 Keishi Hattori 2013-02-22 04:38:49 PST
Created attachment 189746 [details]
Patch
Comment 6 WebKit Review Bot 2013-02-22 05:45:52 PST
Comment on attachment 189746 [details]
Patch

Clearing flags on attachment: 189746

Committed r143723: <http://trac.webkit.org/changeset/143723>
Comment 7 WebKit Review Bot 2013-02-22 05:45:55 PST
All reviewed patches have been landed.  Closing bug.