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

Keishi Hattori
Reported 2013-02-18 09:47:44 PST
Add scroll view for new calendar picker
Attachments
Patch (7.55 KB, patch)
2013-02-18 10:00 PST, Keishi Hattori
no flags
Patch (8.44 KB, patch)
2013-02-22 02:30 PST, Keishi Hattori
no flags
Patch (8.52 KB, patch)
2013-02-22 04:38 PST, Keishi Hattori
no flags
Keishi Hattori
Comment 1 2013-02-18 10:00:47 PST
Kent Tamura
Comment 2 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)"
Keishi Hattori
Comment 3 2013-02-22 02:30:09 PST
Kent Tamura
Comment 4 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
Keishi Hattori
Comment 5 2013-02-22 04:38:49 PST
WebKit Review Bot
Comment 6 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>
WebKit Review Bot
Comment 7 2013-02-22 05:45:55 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.