[webkit-reviews] review denied: [Bug 92769] Web Inspector: replace the Web Inspector editor with CodeMirror : [Attachment 157382] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Jan Keromnes
<janx at chromium.org>'s request for review:
Bug 92769: Web Inspector: replace the Web Inspector editor with CodeMirror
https://bugs.webkit.org/show_bug.cgi?id=92769

Attachment 157382: Patch
https://bugs.webkit.org/attachment.cgi?id=157382&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.lengt
h},"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.