[webkit-reviews] review denied: [Bug 53299] Web Inspector: Use DIVs instead of TABLE in TextViewer : [Attachment 80444] 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 53299: Web Inspector: Use DIVs instead of TABLE in TextViewer
https://bugs.webkit.org/show_bug.cgi?id=53299

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

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

This change needs further splitting. Otherwise, it is hard to follow.

> Source/WebCore/inspector/front-end/Settings.js:34
> +    sourceEditorEnabled: false,

Is this change relevant?

> Source/WebCore/inspector/front-end/SourceFrame.js:-212
> -	   if (this._canEditScripts)

Where did this go?

> Source/WebCore/inspector/front-end/SourceFrame.js:853
> +	   if (!Preferences.canEditScriptSource || !this._isScript ||
Preferences.sourceEditorEnabled)

I think it is too early to introduce sourceEditorEnable switch.

> Source/WebCore/inspector/front-end/TextViewer.js:-44
> -    this.element.addEventListener("beforecopy", this._beforeCopy.bind(this),
false);

Where did these go?

> Source/WebCore/inspector/front-end/TextViewer.js:46
> +    var mainElm = this._mainPanel.element;

Do not abbreviate ("mainElement")

> Source/WebCore/inspector/front-end/TextViewer.js:47
> +    mainElm.addEventListener("scroll", this._syncScroll.bind(this), false);

It would be nice to encapsulate these since they serve same goal. Also, you
might want to bind once.

> Source/WebCore/inspector/front-end/TextViewer.js:52
> +    // FIXME: Remove old live editing functionality and
Preferences.sourceEditorEnabled flag.

ditto (too early)

> Source/WebCore/inspector/front-end/TextViewer.js:75
> +	   this._mainPanel.addDecoration(lineNumber, decoration);

Why is this code getting more complex?

> Source/WebCore/inspector/front-end/TextViewer.js:84
> +	   this._mainPanel.removeDecoration(lineNumber, decoration);

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:112
> +    _handleKeyDown: function()

Did this method change? It is nearly impossible to track changes in this diff.
Can we please split it into a series of refactorings?

> Source/WebCore/inspector/front-end/TextViewer.js:236
>
+//============================================================================
======================

No comments like this in WebKit please.

> Source/WebCore/inspector/front-end/TextViewer.js:237
> +// WebInspector.TextEditorChunkedPanel

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:421
>
+//============================================================================
======================

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:458
>
+//============================================================================
======================

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:-341
> -	   var offset = 0;

Why did this change?