[webkit-reviews] review denied: [Bug 69341] Web Inspector: Fix GoToLineDialog and extract common dialog functionality. : [Attachment 109936] 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 109936: Patch
https://bugs.webkit.org/attachment.cgi?id=109936&action=review

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


> Source/WebCore/WebCore.gypi:6243
> +	       'inspector/front-end/Dialog.js',

Dialog should go after DO*

> Source/WebCore/WebCore.gypi:6352
> +	       'inspector/front-end/dialogs.css',

dialog.css

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

I'd leave the name of the class so that .install invocation looked logical.

> Source/WebCore/inspector/front-end/GoToLineDialogDelegate.js:69
> +    if (!sourceView || !sourceView.canHighlightLine())

Could we inline this method?

> Source/WebCore/inspector/front-end/GoToLineDialogDelegate.js:84
> +    get mainElement()

get contentElement ?

> Source/WebCore/inspector/front-end/GoToLineDialogDelegate.js:89
> +    get mainInput()

get defaultFocusedElement ?