Bug 69341 – Web Inspector: Fix GoToLineDialog and extract common dialog functionality.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Vsevolod Vlasov]

Fix GoToLineDialog and extract common dialog functionality.

- In current implementation dialog never hides if you press mouse button on Go button and releae it somewhere else.
- Same dialog functionality will be used for search.

[Vsevolod Vlasov]

Created attachment 109637 [details]
Patch

[Vsevolod Vlasov]

Created attachment 109666 [details]
Patch

[Pavel Feldman]

Comment on attachment 109666 [details]
Patch

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

[Vsevolod Vlasov]

Created attachment 109835 [details]
Patch (comments addressed)

[Vsevolod Vlasov]

(In reply to comment #3)
> (From update of attachment 109666 [details])
> 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
Done

> > Source/WebCore/inspector/front-end/GoToLineDialogView.js:32
> > +{
> 
> Please add new classes to the compilation.
Done

> > 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.
As discussed offline this dialog is implemented as a singletone and the only instance is available inside Dialog.js as WeBInspector.Dialog._instance.

> > 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?
Renamed to Dialog.
> 
> > 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)
Implemented based on glass pane.

> > Source/WebCore/inspector/front-end/PanelDialog.js:135
> > +WebInspector.PanelDialogView = function()
> 
> Please declare as @interface with members defined in one line each. (get mainInput() { })
I forgot to do this in the patch, will address in the next version.

> 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.
Renamed to DialogDelegate.

> > Source/WebCore/inspector/front-end/panelDialogs.css:1
> > +.panel-dialog {
> 
> .panel is confusing here as well
Done

[Vsevolod Vlasov]

Created attachment 109936 [details]
Patch

[Pavel Feldman]

Comment on attachment 109936 [details]
Patch

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 ?

[Vsevolod Vlasov]

Created attachment 110528 [details]
Patch

[Vsevolod Vlasov]

(In reply to comment #7)
> (From update of attachment 109936 [details])
> 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*
Should it? Other files in this file are sorted ignoring case.

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

> > 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.
Done.

> > Source/WebCore/inspector/front-end/GoToLineDialogDelegate.js:69
> > +    if (!sourceView || !sourceView.canHighlightLine())
> 
> Could we inline this method?
Which one and why?

> > Source/WebCore/inspector/front-end/GoToLineDialogDelegate.js:84
> > +    get mainElement()
> 
> get contentElement ?
> > Source/WebCore/inspector/front-end/GoToLineDialogDelegate.js:89
> > +    get mainInput()
> 
> get defaultFocusedElement ?
renamed to defaultFocusedElement

[WebKit Review Bot]

Comment on attachment 110528 [details]
Patch

Attachment 110528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10027564

[Vsevolod Vlasov]

Created attachment 110531 [details]
Patch

[Early Warning System Bot]

Comment on attachment 110531 [details]
Patch

Attachment 110531 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10026617

[Pavel Feldman]

Comment on attachment 110531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110531&action=review

> Source/WebCore/inspector/front-end/Dialog.js:94
> +            if (typeof(this._dialogDelegate.defaultFocusedElement.select) === "function")

This check is hard to compile

> Source/WebCore/inspector/front-end/Dialog.js:148
> +    get okButton() { },

What if the dialog is YesNo ?

[Vsevolod Vlasov]

Committed r105043: <http://trac.webkit.org/changeset/105043>

[Csaba Osztrogonác]

(In reply to comment #14)
> Committed r105043: <http://trac.webkit.org/changeset/105043>

Reopen, becaue it broke the Qt build. Why did you commit the wrong patch when the Qt EWS said it will break the build? Could you fix it as soon as possible?

[Csaba Osztrogonác]

I fixed it myself, but reluctantly: http://trac.webkit.org/changeset/105045
You should have done it before landing ...