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.
Created attachment 109637 [details] Patch
Created attachment 109666 [details] Patch
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
Created attachment 109835 [details] Patch (comments addressed)
(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
Created attachment 109936 [details] Patch
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 ?
Created attachment 110528 [details] Patch
(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
Comment on attachment 110528 [details] Patch Attachment 110528 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10027564
Created attachment 110531 [details] Patch
Comment on attachment 110531 [details] Patch Attachment 110531 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10026617
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 ?
Committed r105043: <http://trac.webkit.org/changeset/105043>
(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?
I fixed it myself, but reluctantly: http://trac.webkit.org/changeset/105045 You should have done it before landing ...