Refactor CalendarPicker to not use global variables so we can introduce multiple pickers into one page popup.
Created attachment 162190 [details] Patch
Comment on attachment 162190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162190&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:247 > + if (args.isDatePopup) { We prefer early return. > Source/WebCore/Resources/pagepopups/calendarPicker.js:314 > + this._element.classList.add("calendar-picker"); This change should be explained in ChangeLog. > Source/WebCore/Resources/pagepopups/calendarPicker.js:321 > +CalendarPicker.prototype._layoutButtons = function() { Please do not move the function order in the source file. It makes the review harder. You can move the function position by another patch before this patch or after this patch. > Source/WebCore/Resources/pagepopups/calendarPicker.js:341 > +CalendarPicker.prototype._handleKeyDown = function(event) { ditto. > Source/WebCore/Resources/pagepopups/calendarPicker.js:461 > - this._monthPopup.addEventListener("click", bind(this._handleYearMonthChange, this), false); > - this._monthPopup.addEventListener("keydown", bind(this._handleMonthPopupKey, this), false); > - this._monthPopup.addEventListener("mousemove", bind(this._handleMouseMove, this), false); > + this._monthPopup.addEventListener("click", this._handleYearMonthChange.bind(this), false); > + this._monthPopup.addEventListener("keydown", this._handleMonthPopupKey.bind(this), false); > + this._monthPopup.addEventListener("mousemove", this._handleMouseMove.bind(this), false); Can you defer bind(fn,this) -> fn.bind(this) changes to another patch? > Source/WebCore/Resources/pagepopups/pickerCommon.js:80 > +function Picker(element, config) { Please add JSDoc annotation. > Source/WebCore/Resources/pagepopups/pickerCommon.js:91 > +Picker.prototype.submitValue = function(value) { ditto.
Created attachment 162215 [details] Patch
Comment on attachment 162190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162190&action=review >> Source/WebCore/Resources/pagepopups/calendarPicker.js:314 >> + this._element.classList.add("calendar-picker"); > > This change should be explained in ChangeLog. I removed this for now. I will add it later when I will be using it. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:321 >> +CalendarPicker.prototype._layoutButtons = function() { > > Please do not move the function order in the source file. It makes the review harder. > You can move the function position by another patch before this patch or after this patch. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:461 >> + this._monthPopup.addEventListener("mousemove", this._handleMouseMove.bind(this), false); > > Can you defer bind(fn,this) -> fn.bind(this) changes to another patch? Done. >> Source/WebCore/Resources/pagepopups/pickerCommon.js:80 >> +function Picker(element, config) { > > Please add JSDoc annotation. Done.
Comment on attachment 162215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162215&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:78 > + minimumDate: new Date(-62135596800000.0), > + maximumDate: new Date(8640000000000000.0) Are these needed? CalendarPIcker constructor always sets values. > Source/WebCore/Resources/pagepopups/calendarPicker.js:298 > + * @param {?Element} element I don't think element is nullable. should be {!Element}. > Source/WebCore/Resources/pagepopups/calendarPicker.js:366 > + var MainPadding = 6; // TODO: Fix name. TODO: -> FIXME: > Source/WebCore/Resources/pagepopups/calendarPicker.js:404 > */ > -function YearMonthController() { > +function YearMonthController(picker) { Please add annotation for 'picker' argument. > Source/WebCore/Resources/pagepopups/calendarPicker.js:771 > */ > -function DaysTable() { > +function DaysTable(picker) { ditto.
Created attachment 162409 [details] Patch
Comment on attachment 162409 [details] Patch ok
Comment on attachment 162409 [details] Patch Clearing flags on attachment: 162409 Committed r127727: <http://trac.webkit.org/changeset/127727>
All reviewed patches have been landed. Closing bug.