[webkit-reviews] review denied: [Bug 69341] Web Inspector: Fix GoToLineDialog and extract common dialog functionality. : [Attachment 109666] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 69341: Web Inspector: Fix GoToLineDialog and extract common dialog
functionality.
https://bugs.webkit.org/show_bug.cgi?id=69341

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

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


> Source/WebCore/inspector/front-end/GoToLineDialogView.js:31
> +WebInspector.GoToLineDialogView = function(view)

@implements

> Source/WebCore/inspector/front-end/GoToLineDialogView.js:32
> +{

Please add new classes to the compilation.

> Source/WebCore/inspector/front-end/GoToLineDialogView.js:65
> +    WebInspector.PanelDialog.show(sourceView, new
WebInspector.GoToLineDialogView(sourceView));

It is usually expressed as:

new WebInspector.Dialog(new WebInspector.GoToLineDialogDelegate()).show()

creating an instance is generally useful when you'd like to have a programmatic
way of closing the dialog from within the content.

> Source/WebCore/inspector/front-end/PanelDialog.js:29
> +WebInspector.PanelDialog = function(parentView, dialogView)

What does "Panel" part of the name mean? It is not related to
WebInspector.Panel, right?

> Source/WebCore/inspector/front-end/PanelDialog.js:46
> +    var focusableElements = dialogView.focusableElements;

This seems fragile. What if the delegate screws this? You don't really need the
list of focusable elements to close dialog on lose focus. We typically use
"glass pane" to solve this (see SoftContextMenu.js)

> Source/WebCore/inspector/front-end/PanelDialog.js:96
> +	   setTimeout(onTimeout.bind(this), 0);

This is a hack, you should not need that.

> Source/WebCore/inspector/front-end/PanelDialog.js:135
> +WebInspector.PanelDialogView = function()

Please declare as @interface with members defined in one line each. (get
mainInput() { })

Also, naming of this class is confusing. It should either inherit from View or
be named differently. Should either be WebInspector.Dialog.Delegate or
WebInspector.Dialog.Model.

> Source/WebCore/inspector/front-end/panelDialogs.css:1
> +.panel-dialog {

.panel is confusing here as well