Bug 92769 – Web Inspector: replace the Web Inspector editor with CodeMirror

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

[Pavel Feldman]

A provocative ticket.

Jan K. has shown me a codemirror-based editor implementation within the inspector and it impressed me a lot.
- It is a 3K lines
- Its runtime footprint is better (438MB of 600KLOC vs ~650MB for existing editor)
- It loads file faster (5s for 600KLOC vs ~20s for existing editor)
It is currently available under the MIT license which we should be ok with.

The downside is that inspector contributors don't know the source base and it is evolving rapidly. But if we get a nice agreement for the support from some of the codemirror members, it should be Ok though.

[Vivek Galatage]

+1

I am game for it.

[Jan Keromnes]

Obvious +1

I'd gladly provide support on the codemirror side.

[Jan Keromnes]

Created attachment 157382 [details]
Patch

[Jan Keromnes]

I integrated CodeMirror2 as a devtools experiment, replacing DefaultTextEditor when the experiment is active.

The above patch adds support for:
- Open/closing files
- Editing files
- Search/replacing (1)
- Saving files
- Setting breakpoints (UI only)
- Add/get/removeAttribute for any line (2)

Features that are not yet supported by this implementation:
- Communication with WebInspector's breakpoint manager
- Showing and hiding message bubbles (e.g. error messages in the code)
- Displaying popovers (i.e. when stopped on a breakpoint, showing the value for an expression on hover)
- Populated context menus

Please tell me if you think I forgot an important editor feature in the lists above.

Potential performance issues:
(1) The search functionality implemented by WebInspector doesn't scale well with CodeMirror on big files (> 10KLOC). It could also be a good idea to use CodeMirror's own search algorithm instead.
(2) WebInspector seems to check for the attribute "breakpoint" on every line of a file. The current implementation of getAttribute gets the actual CodeMirror line handle, and I have the impression that accessing it for every line deoptimizes what the viewport gains. Maybe the list of current breakpoints can be provided by the breakpoint manager instead.

[Pavel Feldman]

Comment on attachment 157382 [details]
Patch

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

Good stuff, gives a really good clue on what is missing and what not.

> Source/WebCore/WebCore.gypi:6258
> +            'inspector/front-end/css.js',

we'll need to fix the build so that we could store cm files under a separate folder (as we do to uglify)

> Source/WebCore/WebCore.gypi:6259
> +            'inspector/front-end/javascript.js',

Given the size of these, they are likely to slow down the load time. We should probably deploy them separately (as we do with worker scripts) and load them using XHR in case the experiment is turned on.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:75425
> +					RelativePath="..\inspector\front-end\codemirror.js"

you are missing the mode files.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:50
> +        onGutterClick: function(cm, line) {

I guess you should add this later along with the proper breakpoint marker. You should fire corresponding event to embedder, decoration will be added by the sourceframe later.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:58
> +        extraKeys: { "Ctrl-S": this._commitEditing.bind(this) }

Also might require a more careful treatment.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:76
> +        this._codeMirror.setOption("mode", mimeType);

Nuke this line.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:89
> +        console.log('called setReadOnly', arguments);

Remove logging here and below.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:90
> +        this._readOnly = readOnly;

Do you really need this state? You can synchronously query option for that.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:123
> +        this._codeMirror.setCursor({line: lineNumber});

should  be line: lineNumber, ch: 0

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:125
> +        this._codeMirror.scrollTo(coords.x, coords.y);

Doesn't cm do that on its own? If not, lets upstream it. Going through coords for such a simple operation sounds strange.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:164
> +        var mark = this._codeMirror.markText({line:lineNumber,ch:0},{line:lineNumber,ch:line.length},"CodeMirror-searching");

Please format arrays and objects as in WebKit.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:175
> +        console.log('called freeCachedElements', arguments);

We should just remove this (call it on willHide in DefaultTextViewer)

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:197
> +        console.log('called beginUpdates', arguments);

We call these around the calls into decorations. We should make it private and call it within addDecoration and such in the default editor.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:232
> +        this._delegate.beforeTextChanged();

beforeTextChanged expects to be called on the original state of the model. I can look into how we can workaround it. Worst case we would need an additional event.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:243
> +        this._delegate.commitEditing();

We should probably move this to the embedder...

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:256
> +        this._codeMirror.setCursor({line:lineNumber, ch:0});

What is the difference between the revealLine and scrollToLine?

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:266
> +        return null;

This should not be hard to mock.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:314
> +        return this._toRange({line:0, ch:0}, this._codeMirror.posFromIndex(this._codeMirror.getValue().length));

getValue is expensive in codemirror. You probably want something like {line: lineCount - 1, ch: getLine(lineCount - 1).length}

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:369
> +        if (handle.attributes !== undefined) delete handle.attributes[name];

You can't compare to undefined in JavaScript (not a good style), you do id (handle.attributes)

Also, delete should go on the next line.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:375
> +        if (this._content) {

This hack is no longer needed.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:386
> +    _textChanged: function(event)

This does not need to be used.

[Jan Keromnes]

Created attachment 157394 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157394 [details]
Patch

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

New failing tests:
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/modify-cross-domain-rule.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/extensions-headers.html
http/tests/inspector-enabled/injected-script-discard.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html
http/tests/inspector/extensions-useragent.html
fast/canvas/webgl/shader-precision-format.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/web-socket-frame-error.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
http/tests/inspector/console-cd.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html

[WebKit Review Bot]

Created attachment 157399 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Jan Keromnes]

(In reply to comment #5)
> we'll need to fix the build so that we could store cm files under a separate folder (as we do to uglify)
>
> Given the size of these, they are likely to slow down the load time. We should probably deploy them separately (as we do with worker scripts) and load them using XHR in case the experiment is turned on.

Good idea!

> I guess you should add this later along with the proper breakpoint marker. You should fire corresponding event to embedder, decoration will be added by the sourceframe later.

Removed in patch above.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:58
> > +        extraKeys: { "Ctrl-S": this._commitEditing.bind(this) }
> 
> Also might require a more careful treatment.

I tested it and it seems to work, it has the same behavior as DefaultTextEditor. Did you spot any particular problem?

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:76
> > +        this._codeMirror.setOption("mode", mimeType);
> 
> Nuke this line.

It is needed for proper language-specific highlighting.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:89
> > +        console.log('called setReadOnly', arguments);
> 
> Remove logging here and below.

Removed all logging instrumentation in patch above.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:90
> > +        this._readOnly = readOnly;
> 
> Do you really need this state? You can synchronously query option for that.

Indeed the state is not needed, I'll remove it.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:123
> > +        this._codeMirror.setCursor({line: lineNumber});
> 
> should  be line: lineNumber, ch: 0

`ch` defaults to 0 but if you prefer the explicit version I'll change it.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:125
> > +        this._codeMirror.scrollTo(coords.x, coords.y);
> 
> Doesn't cm do that on its own? If not, lets upstream it. Going through coords for such a simple operation sounds strange.

I think we might have to upstream that one. I'll remove it when it's done.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:164
> > +        var mark = this._codeMirror.markText({line:lineNumber,ch:0},{line:lineNumber,ch:line.length},"CodeMirror-searching");
> 
> Please format arrays and objects as in WebKit.

Will do.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:175
> > +        console.log('called freeCachedElements', arguments);
> 
> We should just remove this (call it on willHide in DefaultTextViewer)

ditto

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:197
> > +        console.log('called beginUpdates', arguments);
> 
> We call these around the calls into decorations. We should make it private and call it within addDecoration and such in the default editor.

ditto

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:232
> > +        this._delegate.beforeTextChanged();
> 
> beforeTextChanged expects to be called on the original state of the model. I can look into how we can workaround it. Worst case we would need an additional event.

I'll wait for your conclusion then.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:243
> > +        this._delegate.commitEditing();
> 
> We should probably move this to the embedder...

I'm not sure what you mean. This calls back to the SourceFrame to commit a working copy, should it work differently?

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:256
> > +        this._codeMirror.setCursor({line:lineNumber, ch:0});
> 
> What is the difference between the revealLine and scrollToLine?

Not sure anymore, I'll investigate this.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:266
> > +        return null;
> 
> This should not be hard to mock.

I'll implement this in the next patch.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:314
> > +        return this._toRange({line:0, ch:0}, this._codeMirror.posFromIndex(this._codeMirror.getValue().length));
> 
> getValue is expensive in codemirror. You probably want something like {line: lineCount - 1, ch: getLine(lineCount - 1).length}

I'll change it to your version.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:369
> > +        if (handle.attributes !== undefined) delete handle.attributes[name];
> 
> You can't compare to undefined in JavaScript (not a good style), you do id (handle.attributes)
> 
> Also, delete should go on the next line.

Ok.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:375
> > +        if (this._content) {
> 
> This hack is no longer needed.
>
> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:386
> > +    _textChanged: function(event)
> 
> This does not need to be used.

Removing the hack.

[Jan Keromnes]

Created attachment 157408 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157408 [details]
Patch

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

New failing tests:
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/modify-cross-domain-rule.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/extensions-headers.html
http/tests/inspector-enabled/injected-script-discard.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html
http/tests/inspector/extensions-useragent.html
fast/canvas/webgl/shader-precision-format.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/web-socket-frame-error.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
http/tests/inspector/console-cd.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html

[WebKit Review Bot]

Created attachment 157412 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Jan Keromnes]

Created attachment 157413 [details]
Patch

[Jan Keromnes]

Looks like the test bot is failing, all tests pass on my machine.

New patch with stripped down functional prototype, without logging instrumentation nor breakpoint UI, and with the changes you asked for in comment #5. Missing features and performance issues described in comment #4 still apply.

Still not sure about revealLine and scrollToLine. I'll let you work on deployment and refactoring.

[WebKit Review Bot]

Comment on attachment 157413 [details]
Patch

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

New failing tests:
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/modify-cross-domain-rule.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/extensions-headers.html
http/tests/inspector-enabled/injected-script-discard.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html
http/tests/inspector/extensions-useragent.html
fast/canvas/webgl/shader-precision-format.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/web-socket-frame-error.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
http/tests/inspector/console-cd.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html

[WebKit Review Bot]

Created attachment 157486 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pavel Feldman]

Created attachment 157497 [details]
[Patch] stripped away the code we are unsure of

[WebKit Review Bot]

Comment on attachment 157497 [details]
[Patch] stripped away the code we are unsure of

Clearing flags on attachment: 157497

Committed r125201: <http://trac.webkit.org/changeset/125201>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.