[webkit-reviews] review denied: [Bug 50553] Web Inspector: make script view editable : [Attachment 75657] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Adaikin
<aandrey at google.com>'s request for review:
Bug 50553: Web Inspector: make script view editable
https://bugs.webkit.org/show_bug.cgi?id=50553

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75657&action=review

> WebCore/inspector/front-end/SourceTokenizer.js:-53
> -	   return this._condition.subTokenizer;

This is likely to break HTML tokenizer (it has JavaScript and CSS
subtokenizers).

> WebCore/inspector/front-end/TextEditorHighlighter.js:46
> +	       this._tokenizerCondition =
JSON.stringify(this._tokenizer.initialCondition);

What was wrong with the object state?

> WebCore/inspector/front-end/TextEditorHighlighter.js:54
> +	   this._tokenizerCondition =
JSON.stringify(this._tokenizer.initialCondition);

ditto

> WebCore/inspector/front-end/TextEditorHighlighter.js:141
> +	       return; 

run WebKitTools/Scripts/check-webkit-style to make sure there are no trailing
spaces.

> WebCore/inspector/front-end/TextEditorModel.js:37
> +    this.upwards = upwards;

startLine can be > than endLine, there is no need for "upwards" flag.

> WebCore/inspector/front-end/TextViewer.js:36
> +    this._textModel.resetUndoStack();

I think undo/redo support can be added in a separate change.

> WebCore/inspector/front-end/TextViewer.js:54
> +    this.element.addEventListener("dragstart",
this._preventDefaultAndStopPropagation.bind(this), false);

Drag'n'drop should not be disabled.

> WebCore/inspector/front-end/TextViewer.js:58
> +    // TODO(aandrey): Disable the "Delete" context menu. HOW TO?!

We use // FIXME: foo bar. style in WebKit.

> WebCore/inspector/front-end/TextViewer.js:174
> +	       } else {

no need for { } in for single line blocks.

> WebCore/inspector/front-end/TextViewer.js:254
> +	       } else {

ditto

> WebCore/inspector/front-end/TextViewer.js:263
> +	       for (var i = 0; i < removedChunks.length; ++i) {

ditto

> WebCore/inspector/front-end/TextViewer.js:411
> +		   this._replaceSelectionWith("\t");

This is very fishy. You should not care about the nature of the change (Tab vs
Enter vs Backspace vs whatever). You should be able to re-build text model
based on the generic dom change.

> WebCore/inspector/front-end/TextViewer.js:438
> +	   this._shortcuts[WebInspector.KeyboardShortcut.makeKey("z",
modifiers.CtrlOrMeta)] = this._handleUndo.bind(this);

All of these are supported by the contentEditable. You should not register
them.