Summary: | Add scroll view for new calendar picker | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||
Component: | Forms | Assignee: | 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
Keishi Hattori
2013-02-18 09:47:44 PST
Created attachment 188913 [details]
Patch
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)" Created attachment 189727 [details]
Patch
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 Created attachment 189746 [details]
Patch
Comment on attachment 189746 [details] Patch Clearing flags on attachment: 189746 Committed r143723: <http://trac.webkit.org/changeset/143723> All reviewed patches have been landed. Closing bug. |