[webkit-reviews] review denied: [Bug 97201] Add suggestionPicker to CalendarPicker : [Attachment 165071] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 97201: Add suggestionPicker to CalendarPicker
https://bugs.webkit.org/show_bug.cgi?id=97201

Attachment 165071: Patch
https://bugs.webkit.org/attachment.cgi?id=165071&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165071&action=review


> Source/WebCore/Resources/pagepopups/pickerCommon.js:80
> + * @return {!Element}

This is nullable.

> Source/WebCore/Resources/pagepopups/pickerCommon.js:82
> +Node.prototype.enclosingNodeOrSelfWithClass = function(className)

Polluting the Node prototype is not good in general.
Adding this function to the Node prototype looks no benefit.

> Source/WebCore/Resources/pagepopups/suggestionPicker.css:20
> +    background-color: #3875d7;

How did you decide this color?

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:63
> + * @param {!String} title

The typename should be "string", not "String"

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:90
> +    var entryElement = createElement("div", "suggestion-entry");

The class name should be "suggestion-entry-or-action" or something.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:102
> +    this._containerElement = createElement("div", "suggestion-list");

nit: because it's a list, how about using <ul> and <li>?

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:104
> +    for (var i = 0; i < this._config.suggestionValues.length; i++) {

We prefer ++i

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:113
> +	   var otherEntry =
this._createActionEntryElement(this._config.otherDateLabel,
"openCalendarPicker");

We had better introduce a const variable for the action name.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:118
> +    // To measure the required width, we first set the class to
"measuring-width" which

We had better move the code below to its own function(s).

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:133
> +    for (var i = 0; i < this._containerElement.childNodes.length; i++) {

We prefer ++i.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:180
> +    for (var i = 0; i < childNodes.length; i++) {

We prefer ++i.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:196
> +    for (var i = childNodes.length - 1; i >= 0; i--){

We prefer --i.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:217
> +	       for (var node = document.activeElement.previousElementSibling;
node && node; node = node.previousElementSibling) {

'node && node' is redundant.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:230
> +	       for (var node = document.activeElement.nextElementSibling; node
&& node; node = node.nextElementSibling) {

ditto.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:277
> +    if (!document.activeElement.classList.contains("suggestion-entry"))

This class name appears multiple times.  Please consider introducing const
variable for it like calendarPicker.js does.