Web Inspector: refactor SourceFrame to simplify implementing of js beautifier. * do not store breakpoints in SourceFrame (in SourceFrame._breakpoints and SourceFrame._textModel) since we already store them in BreakpointManager * move user input processing from SourceFrame to SourceFrameController, since SourceFrame doesn't know about line -> sourceID mapping and therefore is unable to modify model directly (set breakpoint and such) * introduce ScriptBreakpointView class that represents a single blue breakpoint bar on a source frame line gutter * BreakpointManager shouldn't require breakpoint url to set a breakpoint
Created attachment 75121 [details] Patch.
Comment on attachment 75121 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=75121&action=review I really don't see how this is better than what we have today. Why do you think it helps with the beautification? > WebCore/inspector/front-end/ScriptView.js:95 > + _getSourceIDs: function() WebCore does not use get prefix for getters. > WebCore/inspector/front-end/SourceFrame.js:181 > + hasTextViewer: function() SourceFrame should not expose textViewer notion outside itself and should entirely encapsulate viewer together with its lazy initialization business.
Created attachment 75264 [details] Patch.
(In reply to comment #2) > (From update of attachment 75121 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75121&action=review > > I really don't see how this is better than what we have today. Why do you think it helps with the beautification? The idea was to implement beautification in a presentation layer - just display formatted text and draw breakpoints and messages according to mapping between initial and formatted script. Currently SourceFrame is neither a pure view nor a pure model. It implements drawing functionality, owns model data and process user input. This makes further code modification nearly impossible. After isolating drawing aspects from model and input processing, beautification could be implemented with a several trivial modifications in presentation code. > > > WebCore/inspector/front-end/ScriptView.js:95 > > + _getSourceIDs: function() > > WebCore does not use get prefix for getters. Done. > > > WebCore/inspector/front-end/SourceFrame.js:181 > > + hasTextViewer: function() > > SourceFrame should not expose textViewer notion outside itself and should entirely encapsulate viewer together with its lazy initialization business. Done.
Comment on attachment 75264 [details] Patch. This change suffers from several things as we discussed offline: - too big coupling with source frame via ad-hoc listeners - not really a controller - more of a breakpoints aspect in SourceFrameController - lots of code moved for no good reason.
Created attachment 75490 [details] Remove breakpoint storages from SourceFrame.
Comment on attachment 75490 [details] Remove breakpoint storages from SourceFrame. View in context: https://bugs.webkit.org/attachment.cgi?id=75490&action=review Looks good, but still requires polish. > WebCore/inspector/front-end/BreakpointManager.js:71 > + breakpointsForSourceIDs: function(sourceIDs) It is more efficient, but API method like this does not look natural. It seems like having this filter belonging to the client would be better (i.e. making findBreakpoints(filter) public). > WebCore/inspector/front-end/ScriptView.js:111 > + _url: function() I don't see where this is used. > WebCore/inspector/front-end/SourceFrame.js:43 > + this._delegate = delegate; Generic name like "_delegate" with no declared api seems too bad. You should have defined a class for it. Like WebInspector.SourceFrameDelegate with constructor that receives all the callbacks.. > WebCore/inspector/front-end/SourceFrame.js:106 > if (this._textViewer) Who is calling this method? Should there be a listener within SourceFrame that handles it now? > WebCore/inspector/front-end/SourceFrame.js:431 > + var breakpoints = this._delegate.findAllBreakpoints(); findAllBreakpoints -> breakpoints() ? > WebCore/inspector/front-end/SourceFrame.js:475 > + if (!WebInspector.panels.scripts) We don't seem to depend on panels.scripts anymore. Should be based on delegate > WebCore/inspector/front-end/SourceView.js:137 > + WebInspector.breakpointManager.setBreakpoint(sourceID, this._url(), line, true, ""); Ok, _url is used here, but this is too bad. In fact, now that ResourceView is so basic, we can inherit SourceView and ScriptView from each other. Or both from a single base class. > WebCore/inspector/front-end/SourceView.js:142 > + _findAllBreakpoints: function() It would be nice to extract these into SourceFrameDelegateImpl
Created attachment 75673 [details] Comments addressed.
> > WebCore/inspector/front-end/SourceFrame.js:475 > > + if (!WebInspector.panels.scripts) > > We don't seem to depend on panels.scripts anymore. Should be based on delegate > SourceFrame still depends on panels.scripts a lot. I think fixing it deserves a separate patch.
Comment on attachment 75673 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=75673&action=review > WebCore/inspector/front-end/ScriptView.js:39 > + this.script.addEventListener("source-changed", this._scriptSourceChanged, this); You are introducing a lot of named events. You should start using constants for them. > WebCore/inspector/front-end/ScriptsPanel.js:293 > + var breakpoints = WebInspector.breakpointManager.findBreakpoints(function(b) { return b.sourceID === editData.sourceID }); Sound like this should belong to source frame now? > WebCore/inspector/front-end/SourceFrame.js:780 > +WebInspector.SourceFrameDelegate.prototype = { Most of these methods should belong to SourceFrame in order to reduce the delegate API surface. > WebCore/inspector/front-end/SourceView.js:247 > +WebInspector.ResourceFrameDelegate.prototype = { SourceFrame does not display arbitrary resources, why is it ResourceFrameDelegate? And hat is this _canEditScripts check used for?
Comment on attachment 75673 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=75673&action=review > WebCore/inspector/front-end/ScriptView.js:121 > + this._script = script; You should call superclass's constructor. > WebCore/inspector/front-end/ScriptView.js:125 > + _canEditScripts: function() You should not override private members declared in different units. > WebCore/inspector/front-end/ScriptView.js:127 > + return true; canEditScript returning 'true' made me think that you forgot about the Preference flag.
Created attachment 75817 [details] Comments addressed.
> > WebCore/inspector/front-end/ScriptsPanel.js:293 > > + var breakpoints = WebInspector.breakpointManager.findBreakpoints(function(b) { return b.sourceID === editData.sourceID }); > > Sound like this should belong to source frame now? > There's bunch of ScriptsPanel's methods that belong to SourceFrame. Let me fix this in a separate patch targeted at ScriptsPanel refactoring.
Attachment 75817 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75817 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 75817 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=75817&action=review Please look into further elimination of the delegate via passing script[] into constructor + relying upon new script listener for live edit. > WebCore/inspector/front-end/Script.js:60 > +WebInspector.Script.SourceChangedEvent = "source-changed"; Nit: I'd form it as WebInspector.Script.Events = { SourceChanged: "source-changed" } > WebCore/inspector/front-end/ScriptView.js:36 > + var delegate = new WebInspector.ScriptFrameDelegate(this.script); You could name it ScriptFrameDelegateImpl for better clarity. > WebCore/inspector/front-end/SourceFrame.js:428 > + breakpoint.addEventListener("enable-changed", this._breakpointChanged, this); Any chance these names get constants as well? > WebCore/inspector/front-end/SourceFrame.js:865 > + { Add comment // Implemented by subclasses.
Committed r73499: <http://trac.webkit.org/changeset/73499>