Bug 50223 – Web Inspector: refactor SourceFrame to simplify implementing of js beautifier

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

[Pavel Podivilov]

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

[Pavel Podivilov]

Created attachment 75121 [details]
Patch.

[Pavel Feldman]

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.

[Pavel Podivilov]

Created attachment 75264 [details]
Patch.

[Pavel Podivilov]

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

[Pavel Feldman]

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.

[Pavel Podivilov]

Created attachment 75490 [details]
Remove breakpoint storages from SourceFrame.

[Pavel Feldman]

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

[Pavel Podivilov]

Created attachment 75673 [details]
Comments addressed.

[Pavel Podivilov]

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

[Pavel Feldman]

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?

[Pavel Feldman]

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.

[Pavel Podivilov]

Created attachment 75817 [details]
Comments addressed.

[Pavel Podivilov]

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

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Pavel Feldman]

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.

[Pavel Podivilov]

Committed r73499: <http://trac.webkit.org/changeset/73499>