[webkit-reviews] review denied: [Bug 53763] Web Inspector: Add "show more" data grid node and waiting message UI components : [Attachment 81208] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Mikhail Naganov
<mnaganov at chromium.org>'s request for review:
Bug 53763: Web Inspector: Add "show more" data grid node and waiting message UI
components
https://bugs.webkit.org/show_bug.cgi?id=53763

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81208&action=review

> Source/WebCore/inspector/front-end/DataGrid.js:1480
> +WebInspector.ShowMoreDataGridNode = function(callback, nextCount, allCount)

I think you should place this into a separate file.

> Source/WebCore/inspector/front-end/DataGrid.js:1494
> +    function populate(count)

define above the usage please.

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:35
> +    this.element.textContent = WebInspector.UIString("Please wait…");

Could you use \u notation here?

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:38
> +    this.cancelButton.setAttribute("type", "button");

No need to set button type to button.

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:54
> +WebInspector.PleaseWaitMessage.prototype._instance = new
WebInspector.PleaseWaitMessage();

This should be a lazy initializer in order not to compromise load time.

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:56
> +WebInspector.PleaseWaitMessage.prototype.startAction = function(element,
actionCallback, cancelCallback)

Please define within prototype's {}

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:65
> +    WebInspector.PleaseWaitMessage.prototype.show(element, cancelCallback);

this.show() ?

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:66
> +    window.setTimeout(doAction, 0);

setTimeout(doAction, 0) (with no window. prefix).

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:68
> +    function doAction()

Define then use.

> Source/WebCore/inspector/front-end/PleaseWaitMessage.js:79
> +WebInspector.PleaseWaitMessage.prototype.show = function(element,
cancelCallback)

Define within {}